When you change the product of a bug, process_bug checks to see if you need to set the milestone, version, and component in the new product. Some of that code can move into Bugzilla::Bug, although it's going to be a little tricky to move the version-checking code until we're really using set_product and set_version, etc.

Okay, here we go! This is fairly cleaner than the old code from process_bug, if maybe slightly slower. However, it should be generally more reliable when doing mass-changes. I did test this pretty well, and it seems to be working properly. By the way, we should modify process_bug to just call $cgi->delete() on any param that's set to dontchange--so then we can just always assume that "undef" means "don't change." (That's something for another bug, though.)

Comment on attachment 258348 [details] [diff] [review] v1 Okay, wait. Nevermind, I was testing on the wrong installation. This patch is still broken. It doesn't compile, but even when I fixed the compiling, it breaks email sending for some mysterious reason!!

Okay, this version compiles and seems to work just fine.

Comment on attachment 258353 [details] [diff] [review] v2 >diff -u process_bug.cgi process_bug.cgi >+ # We do this until we have Bugzilla::Bug->new_from_list. >+ push(@bug_objects, new Bugzilla::Bug($id)); That's not useful for now as you only use these objects when moving bugs to another product. I would skip this part till we use these objects more widely. >-if (((defined $cgi->param('id') && $cgi->param('product') ne $oldproduct) >- || (!$cgi->param('id') >- && $cgi->param('product') ne $cgi->param('dontchange'))) >- && CheckonComment( "reassignbycomponent" )) >+# For each bug, make sure the new values are valid in the product field. >+if (!defined $cgi->param('dontchange') >+ || $cgi->param('product') ne $cgi->param('dontchange')) First, you remove the CheckonComment() check. Secondly, you now do this check *all the time* as $cgi->param('dontchange') is always defined, even when you don't move bugs to another product. Even if I'm fine by doing the check all the time, I think you should add a comment here to make this clear (and you have to reimplement CheckonComment() anyway). >+ foreach my $bug (@bug_objects) { >+ $bug->_check_product_extended( >+ scalar $cgi->param('product'), >+ scalar $cgi->param('component'), >+ scalar $cgi->param('version'), >+ scalar $cgi->param('milestone'), >+ scalar $cgi->param('dontchange'), >+ scalar $cgi->param('confirm_product_change')); > } That's the main part I disagree with, for the following reasons: 1) You will redo the same checks for each bug, in opposition to what we do now (do checks only once). When you edit several tens of bugs at once, as we sometimes do, this could severely slow down Bugzilla. 2) I don't like passing $cgi->param('dontchange') and $cgi->param('confirm_product_change') to the internal method. Both are process_bug specific things. My opinion is that you should replace $cgi->param('foo') by undef if |$cgi->param('foo') eq $cgi->param('dontchange')| and keep the 'confirm_product_change' check in process_bug.cgi. Bugzilla::Bug internal checks should be CGI-independent (else you wouldn't be able to use it from importxml.pl or webservices). >diff -u Bugzilla/Bug.pm Bugzilla/Bug.pm >+ if ($new->has_default_groups && !$already_confirmed) { >+ $vars{verify_bug_group} = 1; >+ } As I said, this check should be outside Bug.pm. >+ if (scalar %vars) { >+ my $template = Bugzilla->template; >+ $template->process("bug/process/verify-new-product.html.tmpl", \%vars) >+ || ThrowTemplateError($template->error()); >+ exit; >+ } No template thing should be inside this internal method. All it should do is to tell you if the new values are valid or not. >diff -u template/en/default/global/select-menu.html.tmpl >+ [% IF values_are_objects %] >+ [% SET value = value.name %] >+ [% END %] You don't always want to display the ->name of objects. This looks like a hack (and it is). Let the caller extract the desired data and pass it to this template.

(In reply to comment #4) > That's not useful for now as you only use these objects when moving bugs to > another product. I would skip this part till we use these objects more widely. We'll be using them in all of process_bug very soon, so I'm just going to keep them. > First, you remove the CheckonComment() check. Secondly, you now do this check > *all the time* as $cgi->param('dontchange') is always defined, even when you > don't move bugs to another product. Even if I'm fine by doing the check all the > time, I think you should add a comment here to make this clear (and you have to > reimplement CheckonComment() anyway). Okay, I'll look at that and fix it appropriately. > 1) You will redo the same checks for each bug, in opposition to what we do now > (do checks only once). When you edit several tens of bugs at once, as we > sometimes do, this could severely slow down Bugzilla. That's actually one of the *major points* of moving to update(). Our security checks are invalid and tricky because we do them for all the bugs at once, instead of each bug individually as they should be. It doesn't slow down Bugzilla in any noticeable way, I checked. > 2) I don't like passing $cgi->param('dontchange') and > $cgi->param('confirm_product_change') to the internal method. Both are > process_bug specific things. My opinion is that you should replace > $cgi->param('foo') by undef if |$cgi->param('foo') eq > $cgi->param('dontchange')| and keep the 'confirm_product_change' check in > process_bug.cgi. Bugzilla::Bug internal checks should be CGI-independent (else > you wouldn't be able to use it from importxml.pl or webservices). I agree. But we'll have to do that after we're done moving to update(), because otherwise we could break the rest of process_bug, no? > No template thing should be inside this internal method. All it should do is to > tell you if the new values are valid or not. No, all _check functions *die* if things are invalid. That's throwing a template. This is doing the same thing--throwing a template. > You don't always want to display the ->name of objects. Yes, you do, for this sort of thing. > This looks like a hack (and it is). Only because we're still using hashes all over Bugzilla where we should be using objects, and those hashes unfortunately have "value" instead of "name." This is a temporary hack until we move away from hashes entirely and we can delete the hash part of the IF block.

Here, I unbitrotted the above patch--see my responses to your comments above.

Oops. 2.1 was the wrong patch.

I'm removing group checks when moving bugs to another product, see my patch in bug 347213. I think mine should land first as I don't want your method in Product.pm.

(In reply to comment #8) > I'm removing group checks when moving bugs to another product, see my patch in > bug 347213. I think mine should land first as I don't want your method in > Product.pm. Okay, I agree. I did a review over there.

Comment on attachment 268587 [details] [diff] [review] v2.2 Bug 347213 has been checked in. This patch no longer applies, especially the part affecting group checks.

This is what I have so far, now that we've converted this bug into the set_product bug. Unfortunately, this patch is neither simple nor focused, but LpSolit (who will be the reviewer) said he'd be OK with it.

Okay! Here we go, set_product. This is a fairly large patch. We do lose a small bit of functionality on mass-change with the group settings. (They'll be based on the first bug in the list instead of the whole list.) What I'd like to do is add that back in later, and just go with this for now.

(In reply to comment #5) > No, all _check functions *die* if things are invalid. That's throwing a > template. This is doing the same thing--throwing a template. Not exactly. If we are not calling the error from the web browser, Throw*Error() calls die(), i.e. plain text. A good example would be importxml.pl which has no UI. You have to pass %vars back to the caller. I won't change my mind about it.

(In reply to comment #13) > Not exactly. If we are not calling the error from the web browser, > Throw*Error() calls die(), i.e. plain text. Yes, the new version accounts for that! Did you even read the patch?

Comment on attachment 273788 [details] [diff] [review] set_product, v1 >diff -u Bugzilla/Bug.pm Bugzilla/Bug.pm >+sub set_product { >+ my $template = Bugzilla->template; >+ $template->process("bug/process/verify-new-product.html.tmpl", >+ \%vars) || ThrowTemplateError($template->error()); Yes, I did look at your patch! See above.

Max, do you have a cvs diff version of your last patch so that I can use context=file? :)

Here it is as a cvs diff, and with some bitrot fixed.

Comment on attachment 278098 [details] [diff] [review] set_product, v2 >Index: process_bug.cgi > if ($requiremilestone) { >+ my $value = $bug_objects{$id}->target_milestone; > if (!defined $value || $value eq $cgi->param('dontchange')) { > $value = $oldhash{'target_milestone'}; > } This test is useless as $bug_objects{$id}->target_milestone is always set to a valid target milestone, even if they are not in use (in which case they are set to the default milestone). Also, there is no need to define $value anymore as it's only used in a single place with this IF block above removed. >Index: Bugzilla/Bug.pm >@@ -417,14 +423,14 @@ > $params->{target_milestone} = $class->_check_target_milestone($product, > $params->{target_milestone}); You have to switch both arguments to match what is used when updating bugs, else Bugzilla crashes on bug creation. > sub _check_component { >+ ($product = $invocant->product_obj) if ref $invocant; Nit: parens are useless. > sub _check_target_milestone { >+ ($product = $invocant->product_obj) if ref $invocant; Nit: parens are useless. > $target = $product->default_milestone if !defined $target; I agree setting it to the default milestone if no milestone is given and it's a new bug, but if the bug exists, it should be left unaltered. > sub _check_version { >+ ($product = $invocant->product_obj) if ref $invocant; Nit: parens are useless. >+sub set_product { >+ $vars{defaults} = { >+ component => $self->component, >+ version => $self->version, >+ milestone => $milestone_ok ? $self->target_milestone >+ : $product->default_milestone >+ }; These defaults are incorrect. If $foo_ok = 1, then this means its value was fine and you should reuse it. If $foo_ok = 0 and foo only contains one legal value, use it, else leave it empty. Not doing this regresses the way default values are selected in the select boxes. >+ # Get the ID of groups which are no longer valid in the new product. >+ WHERE bgm.bug_id = ? As discussed on IRC, you should pass the whole list of bugs being processed, else you won't get any notification about groups being removed from bugs which are not the first one in the list, which is a major regression. >+ if (%vars) { >+ $vars{product} = $product; >+ my $template = Bugzilla->template; >+ $template->process("bug/process/verify-new-product.html.tmpl", >+ \%vars) || ThrowTemplateError($template->error()); >+ exit; >+ } I still think that's not the job of set_product() to call the template. It's the job of the caller. But I won't block this patch on it. >Index: template/en/default/bug/process/verify-new-product.html.tmpl >+[% IF verify_bug_groups %] >+ [% exclude_items.push('bit-\d+') %] >+[% END %] >+ > [% PROCESS "global/hidden-fields.html.tmpl" >- exclude=("^version|component|target_milestone|bit-\\d+$") %] >+ exclude = '^' _ exclude_items.join('|') _ '$' %] Note the distinction between bit-\\d+ and bit-\d+. bit-\d+ doesn't work.

(In reply to comment #18) > I agree setting it to the default milestone if no milestone is given and it's a > new bug, but if the bug exists, it should be left unaltered. Well, if people specifically pass "undef" to set_milestone, I want that to mean "reset to default". > Note the distinction between bit-\\d+ and bit-\d+. bit-\d+ doesn't work. Are you certain about that? Previously, it only wouldn't work because we were in a "" string: perl -e "print 'bit-\d+'" bit-\d+ perl -e 'print "bit-\d+"' bit-d+ Patch upcoming that fixes everything else.

Okay, this fixes everything that you mentioned. (I left the parens. They don't hurt, and they make it slightly less ambiguous what's being "if"ed.) Note that I fixed the defaults thing in the template, which I think is even better than fixing it in the code.

Comment on attachment 279005 [details] [diff] [review] set_product, v3 >Index: process_bug.cgi >+ groups_confirmed => scalar $cgi->param('confirm_product_change'), Nit: I don't see why you name this param 'groups_confirmed' as 'confirm_product_change' is not limited to groups but to the intermediate page as a whole. I would keep 'confirm_product_change'. >Index: Bugzilla/Bug.pm > cclist_accessible => \&Bugzilla::Object::check_boolean, > reporter_accessible => \&Bugzilla::Object::check_boolean, > resolution => \&_check_resolution, >+ target_milestone => \&_check_target_milestone, >+ version => \&_check_version, Nit: You could align lines on "=>". > sub _check_target_milestone { >+ if (ref $invocant && !Bugzilla->params->{'usetargetmilestone'}) { >+ return $invocant->target_milestone; > } This check is incorrect if target milestones are not in use and you move a bug to another product. The old TM is returned despite it is (most of the time) not a valid TM in the new product, and sanitycheck.cgi correctly complains. If TM are not in use and the current TM doesn't apply, the new default one should be returned. >+sub set_product { >+ my $tm_name = $params->{target_milestone} || $self->target_milestone; Wrong param name. It's named "milestone", not "target_milestone", see process_bug.cgi. >+ $vars{defaults} = { >+ component => $self->component, >+ version => $self->version, >+ milestone => $milestone_ok ? $self->target_milestone >+ : $product->default_milestone >+ }; Nit: You should add a comment explaining that $self->foo already returns the newly selected value if this one was a valid one. It took me a long time to understand why you didn't write $foo_name instead, till I realized it already replaced the old value of the foo field. >+ $vars{'old_groups'} = Bugzilla::Group->new_from_list($gids); >+ exit; Nit: Please remove trailing whitespaces (at least 10 on the same line!!) from these 2 lines and at some other places, mainly in set_product(). Your patch looks good otherwise.

(In reply to comment #21) > Nit: I don't see why you name this param 'groups_confirmed' as > 'confirm_product_change' is not limited to groups but to the intermediate page > as a whole. I would keep 'confirm_product_change'. Because we no longer confirm if all the fields are OK. I fixed everything else, and I have a new patch coming up as soon as I test it.

Okay, this addresses all your comments. I fixed set_product for the target_milestone, instead of _check_target_milestone, because I think if somebody calls set_target_milestone directly, it should behave sensibly--that is, if you actually pass some invalid value to set_target_milestone, it should always complain.

(In reply to comment #22) > > as a whole. I would keep 'confirm_product_change'. > > Because we no longer confirm if all the fields are OK. Sure, but we don't only confirm the group change, but also if the TM, version or component is incorrect. So we are still confirming the product change.

(In reply to comment #24) > Sure, but we don't only confirm the group change, but also if the TM, version > or component is incorrect. So we are still confirming the product change. No, that's what I'm trying to say--if you don't *have* to confirm the TM, version, and component, then we don't show the page.

Comment on attachment 280817 [details] [diff] [review] set_product, v4 >Index: Bugzilla/Bug.pm >+sub set_product { >+ if (!$component_ok || !$version_ok || !$milestone_ok) { >+ $vars{verify_fields} = 1; Note that verify_fields is undefined if all 3 field values exist in the new product... >Index: template/en/default/bug/process/verify-new-product.html.tmpl >+[% IF verify_fields %] >+ [%# Verify the version, component, and target milestone fields. %] > <h3>Verify Version, Component[% ", Target Milestone" IF Param("usetargetmilestone") %]</h3> ... in which case this block is hidden and the user has no chance to edit them. You should write: [% IF verify_fields || !confirm_product_change %] This regresses bug 309167.

Comment on attachment 280817 [details] [diff] [review] set_product, v4 No, that's intentional. The point of that bug was that the assignee was wrong, but does that happen still with this patch? If it does, that's a separate issue, and can easily be fixed when we do set_assignee and set_qa_contact.

Comment on attachment 280817 [details] [diff] [review] set_product, v4 Okay, I re-read the bug you referenced and some other bugs it points to, and I agree with you now.

Okay, here you go. I'm not even sure if verify_bug_groups is necessary anymore, but it might be useful somewhere along the line...

Comment on attachment 281171 [details] [diff] [review] set_product, v5 >Index: Bugzilla/Bug.pm >+sub set_product { >+ if ($product_changed && Bugzilla->usage_mode == USAGE_MODE_BROWSER) { >+ ... >+ } >+ else { >+ # When we're not in the browser (or we didn't change the product), we >+ # just die if any of these are invalid. >+ $self->set_component($comp_name); >+ $self->set_version($vers_name); >+ $self->set_target_milestone($tm_name); >+ } Bah, I found an annoying testcase which fails: if the first bug of your buglist is already in the target product, then $product_changed = 0 and you fall into the ELSE block above. But $comp_name and other $foo_name variables are set to --do_not_change-- and so set_foo($foo_name) fails: Product TestProduct does not have a component named --do_not_change--. I didn't detect this problem earlier because the first bug of my buglists usually were in a different product than the targetted one, and so subsequent bugs (even those already being in the target product) got the already updated component, version and TM. Not sure how to fix it. Maybe should you consider $product_changed as true if the product field is different from --do_not_change-- in a mass-change?

I think we can check for --do-not-change-- in process_bug and just not pass those fields if they're set to that.

Okay, here it is, implementing the solution that I said. Also, I got really tired of checking if a field was defined and not set to dontchange, so I made a subroutine that does it.

Comment on attachment 281252 [details] [diff] [review] set_product, v6 When usetargetmilestone is off, moving a bug into a new product changes the TM to --- and this change appears in bugmail. Maybe we shouldn't notify users about this change, i.e. we should omit it from the bugmail, but that's another bug. This patch seems to work fine. So r=LpSolit

Yeah, I wondered whether that happened. Yeah, we should suppress the change from BugMail, or people might be confused. I'm not sure, though, how common it is that people turn on usetargetmilestone and then turn it off later. Checking in process_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v <-- process_bug.cgi new revision: 1.386; previous revision: 1.385 done Checking in Bugzilla/Bug.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v <-- Bug.pm new revision: 1.203; previous revision: 1.202 done Checking in template/en/default/bug/process/verify-new-product.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/verify-new-product.html.tmpl,v <-- verify-new-product.html.tmpl new revision: 1.24; previous revision: 1.23 done