All right, these (status and resolution) are the last fields that have to be moved and then we just have to clean up process_bug.cgi.

All right, here it is. I re-worked how a few things go, but everything should be nicer than before, from the user's perspective. No complaining allowed about how we now do things once per bug instead of on the whole list. :-) I did test this a fair amount. The dupe code got some pretty heavy testing, as did everything else I could think of.

Comment on attachment 296675 [details] [diff] [review] v1 >--- process_bug.cgi 12 Jan 2008 01:27:51 -0000 >+ # We don't use set_resolution here because the MOVED resolution is >+ # special and is normally rejected by set_resolution. >+ $bug->{resolution} = $resolution; Are we sure that a dupe which is marked as MOVED is correctly removed from the duplicates table? Because set_resolution() and its checks are not triggered here. >+ if ($status) { >+ $resolution = $cgi->param('resolution_knob_' . $status->id); >+ } If $status is an open state, $resolution will be undefined. That's the expected behavior, right? I would even check if (!$status->is_open) so that you ignore the resolution completely (in case someone hacks the URL or the UI). >+ else { >+ $resolution = $cgi->param('resolution_knob_change_resolution'); >+ } Let's hope this one won't trigger any unexpected error when we indeed don't care about its value. >- if ($cgi->param('comment') || $cgi->param('work_time') || $duplicate) { >- $bug_changed = 1; Is $bug_changed still in use in process_bug.cgi? And is it correctly set to 1 when needed? >+ # We may have zeroed the remaining time, if we moved into a closed >+ # status, so we should inform the user about that. >+ if (!is_open_state($new_status) && $changes->{'remaining_time'}) { >+ $vars->{'message'} = "remaining_time_zeroed" >+ if Bugzilla->user->in_group(Bugzilla->params->{'timetrackinggroup'}); >+ } Why not have this in set_resolution()? >--- Bugzilla/Bug.pm 12 Jan 2008 01:27:51 -0000 >- $vars->{comment_exists} = ($params->{comment} =~ /\S+/) ? 1 : 0; > sub _check_bug_status { >+ my $comments = $invocant->{added_comments} || []; >+ $comment = $comments->[-1]; Is it intentional to no longer check that $comment =~ /\S+/ ? Else you could pass " ". >+ if ($self->reporter->can_see_bug($dupe_of)) { >+ $self->{_add_dup_cc} = 1; >+ } You shouldn't CC the reporter if he is the reporter of the original bug already (regression). >+sub _clear_dup_id { $_[0]->{dup_id} = undef } Nit: add a semi-colon after undef. >+sub set_dup_id { >+ # Update the other bug. >+ my $dupe_of = new Bugzilla::Bug($self->dup_id); What happens if $self->dup_id in undefined? Shouldn't you add a ThrowCodeError() here as this should never happen? Else if you are sure that it's always defined, why adding || 0 a few lines above? >+sub process_knob { > elsif ($action eq 'clearresolution') { >+ $self->set_status($action); set_status() will fail as 'clearresolution' is not a legal bug status. Also, what happened to all these 'commenton*' parameters? It seems you now ignore them completely. Special actions have no entry in status_workflow, and so looking at require_comment in that table is not enough. >--- template/en/default/global/user-error.html.tmpl 12 Jan 2008 01:27:51 -0000 >+ [% IF old && new %] >+ <b>comment</b> when changing the status of [% terms.abug %] from >+ [% old.name FILTER html %] to [% new.name FILTER html %]. Must be [%+ old.name ... %] >+ [% ELSIF error == "dupe_id_required" %] >+ [% title = "Duplicate Bug Id Required" %] >+ You must specify a [% terms.bug %] id to mark this [% terms.bug %] "specify [% terms.abug %]" instead of "specify a [% terms.bug %]" >--- Bugzilla/Status.pm 9 Oct 2007 10:34:47 -0000 1.5 >+sub allow_change_from { >+ my ($cond, $values) = $self->_status_condition($old_status); Nit: please move this line later in the method. I couldn't understand why you called it till I read the code at the end of the method. So group them so that we immediately understand why you are doing this call. >+ my ($transition_allowed) = Bugzilla->dbh->selectrow_array( >+ "SELECT 1 FROM status_workflow WHERE $cond", undef, @$values); Nit: as discussed on IRC, you omitted isactive = 1 from the query. >+sub _status_condition { >+ my $cond = 'old_status IS NULL'; >+ my @values = ($self->id); Nit: the way you do it is awkward IMO. You write a condition which doesn't require any input but pass a value to @values at the same time. I had to read your method carefully to understand how things work... >+ $cond .= " AND new_status = ?"; ... Why not pushing $self->id only now into @values, for clarity? Also, your new methods need POD. >--- template/en/default/global/code-error.html.tmpl 20 Aug 2007 18:25:04 -0000 1.102 >+ [% ELSIF error == "no_manual_moved" %] >+ You cannot set the resolution of [% terms.abug %] to MOVED without >+ moving the bug. 009bugwords.t fails due to "the bug" which should be "the [% terms.bug %]". I didn't test your patch yet, but it looks great. I will test with your updated patch.

(In reply to comment #2) > If $status is an open state, $resolution will be undefined. That's the expected > behavior, right? I would even check if (!$status->is_open) so that you ignore > the resolution completely (in case someone hacks the URL or the UI). Yeah, that's the expected behavior. $bug->set_status ignores resolutions if you're in an open status, so we don't have to do any additional checks in process_bug. > Let's hope this one won't trigger any unexpected error when we indeed don't > care about its value. I'm pretty sure it won't. > Is $bug_changed still in use in process_bug.cgi? And is it correctly set to 1 > when needed? It is not used. And it doesn't matter. It will be removed in the cleanup bug following this one. > >+ # We may have zeroed the remaining time, if we moved into a closed > >+ # status, so we should inform the user about that. > [snip] > > Why not have this in set_resolution()? Because this is a very process_bug-specific sort of thing that has to do only with the User Interface. set_resolution could be called from lots of places, like the webservice, where that wouldn't matter. Also, set_resolution knows nothing about $vars and shouldn't have to. I won't be changing that. > >--- Bugzilla/Bug.pm 12 Jan 2008 01:27:51 -0000 > Is it intentional to no longer check that $comment =~ /\S+/ ? Else you could > pass " ". It is intentional. add_comment already checks that for us. > >+sub set_dup_id { > > >+ # Update the other bug. > >+ my $dupe_of = new Bugzilla::Bug($self->dup_id); > > What happens if $self->dup_id in undefined? Shouldn't you add a > ThrowCodeError() here as this should never happen? That's already done in _check_dup_id. > set_status() will fail as 'clearresolution' is not a legal bug status. Ah, thank you. Fixed. I don't even understand when or why clearresolution is called, but I've implemented it. :-) > Nit: as discussed on IRC, you omitted isactive = 1 from the query. Intentionally. isactive should not be used. It is in Bugzilla by my mistake. You were right about everything else, and I fixed it all in the upcoming patch.

Okay, this contains all the fixes. I tested all the commenton parameters, also, this time.

Comment on attachment 297468 [details] [diff] [review] v2 >Index: process_bug.cgi Around line 500, remove the following block as Bugzilla::Bug does it for us. I tested your patch with these lines removed and it's working great. if (($cgi->param('set_default_assignee') || $cgi->param('set_default_qa_contact')) && Bugzilla->params->{'commentonreassignbycomponent'} && !comment_exists()) { ThrowUserError('comment_required'); } As nothing else uses comment_exists(), you can remove this subroutine from process_bug.cgi as well. >Index: Bugzilla/Bug.pm >+sub clear_resolution { You have to make sure that the bug is open before removing its resolution. Adding: ThrowUserError('foo_bar', {bug_id => $self->bug_id}) if !$self->status->is_open; correctly catches this testcase. I did some testing and your patch is working fine. Great cleanup! r=LpSolit

Don't forget to address my comments on checkin.

Checking in process_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v <-- process_bug.cgi new revision: 1.397; previous revision: 1.396 done Checking in Bugzilla/Bug.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v <-- Bug.pm new revision: 1.226; previous revision: 1.225 done Checking in Bugzilla/Status.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Status.pm,v <-- Status.pm new revision: 1.6; previous revision: 1.5 done Checking in template/en/default/global/code-error.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v <-- code-error.html.tmpl new revision: 1.103; previous revision: 1.102 done Checking in template/en/default/global/user-error.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v <-- user-error.html.tmpl new revision: 1.240; previous revision: 1.239 done