From 2bd57ce8e6f2c2bb59a99d825fc9d181ea2cb4a5 Mon Sep 17 00:00:00 2001 From: "lpsolit%gmail.com" <> Date: Sun, 27 May 2007 03:27:45 +0000 Subject: Bug 344965: Fix process_bug.cgi and bug/* templates to work with custom bug status workflow - Patch by Frédéric Buclin r=mkanat a=LpSolit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Bugzilla/Bug.pm | 232 +++++++++++++++++---- Bugzilla/Constants.pm | 6 - Bugzilla/Install/DB.pm | 2 +- Bugzilla/Status.pm | 116 +++++++++++ buglist.cgi | 22 +- editworkflow.cgi | 42 ++-- process_bug.cgi | 228 ++++++-------------- reports.cgi | 1 + sanitycheck.cgi | 1 + .../en/default/admin/workflow/comment.html.tmpl | 8 +- template/en/default/admin/workflow/edit.html.tmpl | 16 +- template/en/default/bug/edit.html.tmpl | 2 +- template/en/default/bug/knob.html.tmpl | 108 +++------- template/en/default/filterexceptions.pl | 7 +- template/en/default/global/user-error.html.tmpl | 15 +- template/en/default/list/edit-multiple.html.tmpl | 76 ++----- 16 files changed, 499 insertions(+), 383 deletions(-) create mode 100644 Bugzilla/Status.pm diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index dcaef8004..ce4423a27 100755 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -42,6 +42,7 @@ use Bugzilla::Error; use Bugzilla::Product; use Bugzilla::Component; use Bugzilla::Group; +use Bugzilla::Status; use List::Util qw(min); use Storable qw(dclone); @@ -52,8 +53,9 @@ use base qw(Bugzilla::Object Exporter); bug_alias_to_id ValidateBugAlias ValidateBugID RemoveVotes CheckIfVotedConfirmed LogActivityEntry - is_open_state + BUG_STATE_OPEN is_open_state editable_bug_fields + SPECIAL_STATUS_WORKFLOW_ACTIONS ); ##################################################################### @@ -176,6 +178,19 @@ use constant VALID_ENTRY_STATUS => qw( ASSIGNED ); +use constant SPECIAL_STATUS_WORKFLOW_ACTIONS => qw( + none + duplicate + change_resolution + clearresolution +); + +sub BUG_STATE_OPEN { + # XXX - We should cache this list. + my $dbh = Bugzilla->dbh; + return @{$dbh->selectcol_arrayref('SELECT value FROM bug_status WHERE is_open = 1')}; +} + ##################################################################### sub new { @@ -213,12 +228,6 @@ sub new { return $error_self; } - # XXX At some point these should be moved into accessors. - # They only are here because this is how Bugzilla::Bug - # originally did things, before it was a Bugzilla::Object. - $self->{'isunconfirmed'} = ($self->{bug_status} eq 'UNCONFIRMED'); - $self->{'isopened'} = is_open_state($self->{bug_status}); - return $self; } @@ -1025,8 +1034,7 @@ sub set_status { my ($self, $status) = @_; $self->set('bug_status', $status); # Check for the everconfirmed transition - $self->_set_everconfirmed(1) if ($status eq 'NEW' - || $status eq 'ASSIGNED'); + $self->_set_everconfirmed(1) if (is_open_state($status) && $status ne 'UNCONFIRMED'); } ######################## @@ -1247,6 +1255,16 @@ sub flag_types { return $self->{'flag_types'}; } +sub isopened { + my $self = shift; + return is_open_state($self->{bug_status}) ? 1 : 0; +} + +sub isunconfirmed { + my $self = shift; + return ($self->bug_status eq 'UNCONFIRMED') ? 1 : 0; +} + sub keywords { my ($self) = @_; return $self->{'keywords'} if exists $self->{'keywords'}; @@ -1317,6 +1335,13 @@ sub reporter { return $self->{'reporter'}; } +sub status { + my $self = shift; + return undef if $self->{'error'}; + + $self->{'status'} ||= new Bugzilla::Status({name => $self->{'bug_status'}}); + return $self->{'status'}; +} sub show_attachment_flags { my ($self) = @_; @@ -1530,18 +1555,159 @@ sub bug_alias_to_id { # Workflow Control routines ##################################################################### +# Make sure that the new status is valid for ALL bugs. +sub check_status_transition { + my ($self, $new_status, $bug_ids) = @_; + my $dbh = Bugzilla->dbh; + + check_field('bug_status', $new_status); + trick_taint($new_status); + + my $illegal_statuses = + $dbh->selectcol_arrayref('SELECT DISTINCT bug_status.value + FROM bug_status + INNER JOIN bugs + ON bugs.bug_status = bug_status.value + WHERE bug_id IN (' . join (',', @$bug_ids). ') + AND bug_status.id NOT IN (SELECT old_status + FROM status_workflow + INNER JOIN bug_status b_s + ON b_s.id = status_workflow.new_status + WHERE b_s.value = ?)', + undef, $new_status); + + if (scalar(@$illegal_statuses)) { + ThrowUserError('illegal_bug_status_transition', {old => $illegal_statuses, + new => $new_status}) + } +} + +# Make sure all checks triggered by the workflow are successful. +# Some are hardcoded and come from older versions of Bugzilla. +sub check_status_change_triggers { + my ($self, $action, $bug_ids, $vars) = @_; + my $dbh = Bugzilla->dbh; + $vars ||= {}; + + # First, make sure no comment is required if there is none. + # If a comment is given, then this check is useless. + if (!$vars->{comment_exists}) { + if (grep { $action eq $_ } SPECIAL_STATUS_WORKFLOW_ACTIONS) { + # 'commentonnone' doesn't exist, so this is safe. + ThrowUserError('comment_required') if Bugzilla->params->{"commenton$action"}; + } + else { + my $required_for_transitions = + $dbh->selectcol_arrayref('SELECT DISTINCT bug_status.value + FROM bug_status + INNER JOIN bugs + ON bugs.bug_status = bug_status.value + INNER JOIN status_workflow + ON bug_status.id = old_status + INNER JOIN bug_status b_s + ON b_s.id = new_status + WHERE bug_id IN (' . join (',', @$bug_ids). ') + AND b_s.value = ? + AND require_comment = 1', + undef, $action); + + if (scalar(@$required_for_transitions)) { + ThrowUserError('comment_required', {old => $required_for_transitions, + new => $action}); + } + } + } + + # Now run hardcoded checks. + # There is no checks for these actions. + return if ($action eq 'none' || $action eq 'clearresolution'); + + if ($action eq 'duplicate') { + # You cannot mark bugs as duplicates when changing + # several bugs at once. + $vars->{bug_id} || ThrowUserError('dupe_not_allowed'); + + # Make sure we can change the original bug (issue A on bug 96085) + $vars->{dup_id} || ThrowCodeError('undefined_field', { field => 'dup_id' }); + ValidateBugID($vars->{dup_id}, 'dup_id'); + + # Make sure a loop isn't created when marking this bug + # as duplicate. + my %dupes; + my $dupe_of = $vars->{dup_id}; + my $sth = $dbh->prepare('SELECT dupe_of FROM duplicates + WHERE dupe = ?'); + + while ($dupe_of) { + if ($dupe_of == $vars->{bug_id}) { + ThrowUserError('dupe_loop_detected', { bug_id => $vars->{bug_id}, + dupe_of => $vars->{dup_id} }); + } + # If $dupes{$dupe_of} is already set to 1, then a loop + # already exists which does not involve this bug. + # As the user is not responsible for this loop, do not + # prevent him from marking this bug as a duplicate. + last if exists $dupes{"$dupe_of"}; + $dupes{"$dupe_of"} = 1; + $sth->execute($dupe_of); + $dupe_of = $sth->fetchrow_array; + } + + # Also, let's see if the reporter has authorization to see + # the bug to which we are duping. If not we need to prompt. + $vars->{DuplicateUserConfirm} = 1; + + # DUPLICATE bugs should have no time remaining. + $vars->{remove_remaining_time} = 1; + } + elsif ($action eq 'change_resolution' || !is_open_state($action)) { + # don't resolve as fixed while still unresolved blocking bugs + if (Bugzilla->params->{"noresolveonopenblockers"} + && $vars->{resolution} eq 'FIXED') + { + my @dependencies = Bugzilla::Bug::CountOpenDependencies(@$bug_ids); + if (scalar @dependencies > 0) { + ThrowUserError("still_unresolved_bugs", + { dependencies => \@dependencies, + dependency_count => scalar @dependencies }); + } + } + + # You cannot use change_resolution if there is at least one open bug + # nor can you close open bugs if no resolution is given. + my $open_states = join(',', map {$dbh->quote($_)} BUG_STATE_OPEN); + my $idlist = join(',', @$bug_ids); + my $is_open = + $dbh->selectrow_array("SELECT 1 FROM bugs WHERE bug_id IN ($idlist) + AND bug_status IN ($open_states)"); + + if ($is_open) { + ThrowUserError('resolution_not_allowed') if ($action eq 'change_resolution'); + ThrowUserError('missing_resolution', {status => $action}) if !$vars->{resolution}; + } + # Now is good time to validate the resolution, if any. + check_field('resolution', $vars->{resolution}, + Bugzilla::Bug->settable_resolutions) if $vars->{resolution}; + + $vars->{remove_remaining_time} = 1 if ($action ne 'change_resolution'); + } + elsif ($action eq 'ASSIGNED' + && Bugzilla->params->{"usetargetmilestone"} + && Bugzilla->params->{"musthavemilestoneonaccept"}) + { + $vars->{requiremilestone} = 1; + } +} + sub get_new_status_and_resolution { my ($self, $action, $resolution) = @_; my $dbh = Bugzilla->dbh; my $status; + my $everconfirmed = $self->everconfirmed; if ($action eq 'none') { # Leaving the status unchanged doesn't need more investigation. - return ($self->bug_status, $self->resolution); - } - elsif ($action eq 'reopen') { - $status = $self->everconfirmed ? 'REOPENED' : 'UNCONFIRMED'; - $resolution = ''; + return ($self->bug_status, $self->resolution, $self->everconfirmed); } elsif ($action eq 'duplicate') { # Only alter the bug status if the bug is currently open. @@ -1560,37 +1726,21 @@ sub get_new_status_and_resolution { $resolution = ''; } else { - # That's where actions not requiring any specific trigger (such as future - # custom statuses) come. - # XXX - This is hardcoded here for now, but will disappear soon when - # this routine will look at the DB directly to get the workflow. - if ($action eq 'confirm') { - $status = 'NEW'; - } - elsif ($action eq 'accept') { - $status = 'ASSIGNED'; - } - elsif ($action eq 'resolve') { - $status = 'RESOLVED'; - } - elsif ($action eq 'verify') { - $status = 'VERIFIED'; - } - elsif ($action eq 'close') { - $status = 'CLOSED'; - } - else { - ThrowCodeError('unknown_action', { action => $action }); - } - + $status = $action; if (is_open_state($status)) { # Open bugs have no resolution. $resolution = ''; + $everconfirmed = ($status eq 'UNCONFIRMED') ? 0 : 1; } - else { - # All non-open statuses must have a resolution. + elsif (is_open_state($self->bug_status)) { + # A resolution is required to close bugs. $resolution || ThrowUserError('missing_resolution', {status => $status}); } + else { + # Already closed bugs can only change their resolution + # using the change_resolution action. + $resolution = $self->resolution + } } # Now it's time to validate the bug resolution. # Bug resolutions have no workflow specific rules, so any valid @@ -1598,7 +1748,7 @@ sub get_new_status_and_resolution { check_field('resolution', $resolution) if ($resolution ne ''); trick_taint($resolution); - return ($status, $resolution); + return ($status, $resolution, $everconfirmed); } ##################################################################### @@ -1968,7 +2118,7 @@ sub CountOpenDependencies { "FROM bugs, dependencies " . "WHERE blocked IN (" . (join "," , @bug_list) . ") " . "AND bug_id = dependson " . - "AND bug_status IN ('" . (join "','", BUG_STATE_OPEN) . "') " . + "AND bug_status IN (" . join(', ', map {$dbh->quote($_)} BUG_STATE_OPEN) . ") " . $dbh->sql_group_by('blocked')); $sth->execute(); diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm index 0e2635895..d811796d0 100644 --- a/Bugzilla/Constants.pm +++ b/Bugzilla/Constants.pm @@ -120,8 +120,6 @@ use File::Basename; FIELD_TYPE_FREETEXT FIELD_TYPE_SINGLE_SELECT - BUG_STATE_OPEN - USAGE_MODE_BROWSER USAGE_MODE_CMDLINE USAGE_MODE_WEBSERVICE @@ -351,10 +349,6 @@ use constant SAFE_PROTOCOLS => ('afs', 'cid', 'ftp', 'gopher', 'http', 'https', 'irc', 'mid', 'news', 'nntp', 'prospero', 'telnet', 'view-source', 'wais'); -# States that are considered to be "open" for bugs. -use constant BUG_STATE_OPEN => ('NEW', 'REOPENED', 'ASSIGNED', - 'UNCONFIRMED'); - # Usage modes. Default USAGE_MODE_BROWSER. Use with Bugzilla->usage_mode. use constant USAGE_MODE_BROWSER => 0; use constant USAGE_MODE_CMDLINE => 1; diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm index f1a148353..7dd5ad269 100644 --- a/Bugzilla/Install/DB.pm +++ b/Bugzilla/Install/DB.pm @@ -22,7 +22,7 @@ package Bugzilla::Install::DB; use strict; -use Bugzilla::Bug qw(is_open_state); +use Bugzilla::Bug qw(BUG_STATE_OPEN is_open_state); use Bugzilla::Constants; use Bugzilla::Hook; use Bugzilla::Install::Util qw(indicate_progress); diff --git a/Bugzilla/Status.pm b/Bugzilla/Status.pm new file mode 100644 index 000000000..e83fd3533 --- /dev/null +++ b/Bugzilla/Status.pm @@ -0,0 +1,116 @@ +# -*- Mode: perl; indent-tabs-mode: nil -*- +# +# The contents of this file are subject to the Mozilla Public +# License Version 1.1 (the "License"); you may not use this file +# except in compliance with the License. You may obtain a copy of +# the License at http://www.mozilla.org/MPL/ +# +# Software distributed under the License is distributed on an "AS +# IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or +# implied. See the License for the specific language governing +# rights and limitations under the License. +# +# The Original Code is the Bugzilla Bug Tracking System. +# +# The Initial Developer of the Original Code is Frédéric Buclin. +# Portions created by Frédéric Buclin are Copyright (C) 2007 +# Frédéric Buclin. All Rights Reserved. +# +# Contributor(s): Frédéric Buclin + +use strict; + +package Bugzilla::Status; + +use base qw(Bugzilla::Object); + +################################ +##### Initialization ##### +################################ + +use constant DB_TABLE => 'bug_status'; + +use constant DB_COLUMNS => qw( + id + value + sortkey + isactive + is_open +); + +use constant NAME_FIELD => 'value'; +use constant LIST_ORDER => 'sortkey, value'; + +############################### +##### Accessors #### +############################### + +sub name { return $_[0]->{'value'}; } +sub sortkey { return $_[0]->{'sortkey'}; } +sub is_active { return $_[0]->{'isactive'}; } +sub is_open { return $_[0]->{'is_open'}; } + +############################### +##### Methods #### +############################### + +sub can_change_to { + my $self = shift; + my $dbh = Bugzilla->dbh; + + if (!defined $self->{'can_change_to'}) { + my $new_status_ids = $dbh->selectcol_arrayref('SELECT new_status + FROM status_workflow + INNER JOIN bug_status + ON id = new_status + WHERE isactive = 1 + AND old_status = ?', + undef, $self->id); + + $self->{'can_change_to'} = Bugzilla::Status->new_from_list($new_status_ids); + } + + return $self->{'can_change_to'}; +} + + +1; + +__END__ + +=head1 NAME + +Bugzilla::Status - Bug status class. + +=head1 SYNOPSIS + + use Bugzilla::Status; + + my $bug_status = new Bugzilla::Status({name => 'ASSIGNED'}); + my $bug_status = new Bugzilla::Status(4); + +=head1 DESCRIPTION + +Status.pm represents a bug status object. It is an implementation +of L, and thus provides all methods that +L provides. + +The methods that are specific to C are listed +below. + +=head1 METHODS + +=over + +=item C + + Description: Returns the list of active statuses a bug can be changed to + given the current bug status. + + Params: none. + + Returns: A list of Bugzilla::Status objects. + +=back + +=cut diff --git a/buglist.cgi b/buglist.cgi index 338017fa1..6d47b69f9 100755 --- a/buglist.cgi +++ b/buglist.cgi @@ -1139,8 +1139,26 @@ if ($dotweak) { $vars->{'unconfirmedstate'} = 'UNCONFIRMED'; - $vars->{'bugstatuses'} = [ keys %$bugstatuses ]; - + # Convert bug statuses to their ID. + my @bug_statuses = map {$dbh->quote($_)} keys %$bugstatuses; + my $bug_status_ids = + $dbh->selectcol_arrayref('SELECT id FROM bug_status + WHERE value IN (' . join(', ', @bug_statuses) .')'); + + # This query collects new statuses which are common to all current bug statuses. + # It also accepts transitions where the bug status doesn't change. + $bug_status_ids = + $dbh->selectcol_arrayref('SELECT DISTINCT new_status + FROM status_workflow sw1 + WHERE NOT EXISTS (SELECT * FROM status_workflow sw2 + WHERE sw2.old_status != sw1.new_status + AND sw2.old_status IN (' . join(', ', @$bug_status_ids) . ') + AND NOT EXISTS (SELECT * FROM status_workflow sw3 + WHERE sw3.new_status = sw1.new_status + AND sw3.old_status = sw2.old_status))'); + + $vars->{'current_bug_statuses'} = [keys %$bugstatuses]; + $vars->{'new_bug_statuses'} = Bugzilla::Status->new_from_list($bug_status_ids); # The groups to which the user belongs. $vars->{'groups'} = GetGroups(); diff --git a/editworkflow.cgi b/editworkflow.cgi index 9a369c974..ac914f76d 100644 --- a/editworkflow.cgi +++ b/editworkflow.cgi @@ -27,6 +27,7 @@ use Bugzilla; use Bugzilla::Constants; use Bugzilla::Error; use Bugzilla::Token; +use Bugzilla::Status; my $cgi = Bugzilla->cgi; my $dbh = Bugzilla->dbh; @@ -42,12 +43,6 @@ $user->in_group('admin') my $action = $cgi->param('action') || 'edit'; my $token = $cgi->param('token'); -sub get_statuses { - my $statuses = $dbh->selectall_arrayref('SELECT id, value, is_open FROM bug_status - ORDER BY sortkey, value', { Slice => {} }); - return $statuses; -} - sub get_workflow { my $workflow = $dbh->selectall_arrayref('SELECT old_status, new_status, require_comment FROM status_workflow'); @@ -64,7 +59,7 @@ sub load_template { my $template = Bugzilla->template; my $vars = {}; - $vars->{'statuses'} = get_statuses(); + $vars->{'statuses'} = [Bugzilla::Status->get_all]; $vars->{'workflow'} = get_workflow(); $vars->{'token'} = issue_session_token("workflow_$filename"); $vars->{'message'} = $message; @@ -79,9 +74,8 @@ if ($action eq 'edit') { } elsif ($action eq 'update') { check_token_data($token, 'workflow_edit'); - my $statuses = get_statuses; + my $statuses = [Bugzilla::Status->get_all]; my $workflow = get_workflow(); - my $initial_state = {id => 0}; my $sth_insert = $dbh->prepare('INSERT INTO status_workflow (old_status, new_status) VALUES (?, ?)'); @@ -90,22 +84,28 @@ elsif ($action eq 'update') { my $sth_delnul = $dbh->prepare('DELETE FROM status_workflow WHERE old_status IS NULL AND new_status = ?'); - foreach my $old ($initial_state, @$statuses) { - # Hashes cannot have undef as a key, so we use 0. But the DB - # must store undef, for referential integrity. - my $old_id_for_db = $old->{'id'} || undef; + # Part 1: Initial bug statuses. + foreach my $new (@$statuses) { + if ($cgi->param('w_0_' . $new->id)) { + $sth_insert->execute(undef, $new->id) + unless defined $workflow->{0}->{$new->id}; + } + else { + $sth_delnul->execute($new->id); + } + } + + # Part 2: Bug status changes. + foreach my $old (@$statuses) { foreach my $new (@$statuses) { - next if $old->{'id'} == $new->{'id'}; + next if $old->id == $new->id; - if ($cgi->param('w_' . $old->{'id'} . '_' . $new->{'id'})) { - $sth_insert->execute($old_id_for_db, $new->{'id'}) - unless defined $workflow->{$old->{'id'}}->{$new->{'id'}}; - } - elsif ($old_id_for_db) { - $sth_delete->execute($old_id_for_db, $new->{'id'}); + if ($cgi->param('w_' . $old->id . '_' . $new->id)) { + $sth_insert->execute($old->id, $new->id) + unless defined $workflow->{$old->id}->{$new->id}; } else { - $sth_delnul->execute($new->{'id'}); + $sth_delete->execute($old->id, $new->id); } } } diff --git a/process_bug.cgi b/process_bug.cgi index a28efa02f..bb0d608a5 100755 --- a/process_bug.cgi +++ b/process_bug.cgi @@ -120,6 +120,11 @@ sub send_results { $vars->{'header_done'} = 1; } +sub comment_exists { + my $cgi = Bugzilla->cgi; + return ($cgi->param('comment') && $cgi->param('comment') =~ /\S+/) ? 1 : 0; +} + ###################################################################### # Begin Data/Security Validation ###################################################################### @@ -244,32 +249,6 @@ if ($cgi->cookie("BUGLIST") && defined $cgi->param('id')) { $vars->{'bug_list'} = \@bug_list; } -# This function checks if there is a comment required for a specific -# function and tests, if the comment was given. -# If comments are required for functions is defined by params. -# -sub CheckonComment { - my ($function) = (@_); - my $cgi = Bugzilla->cgi; - - # Param is 1 if comment should be added ! - my $ret = Bugzilla->params->{ "commenton" . $function }; - - # Allow without comment in case of undefined Params. - $ret = 0 unless ( defined( $ret )); - - if( $ret ) { - if (!defined $cgi->param('comment') - || $cgi->param('comment') =~ /^\s*$/) { - # No comment - sorry, action not allowed ! - ThrowUserError("comment_required"); - } else { - $ret = 0; - } - } - return( ! $ret ); # Return val has to be inverted -} - # Figure out whether or not the user is trying to change the product # (either the "product" variable is not set to "don't change" or the # user is changing a single bug and has changed the bug's product), @@ -287,11 +266,13 @@ if (defined $cgi->param('id')) { defined($cgi->param('product')) || ThrowCodeError('undefined_field', { field => 'product' }); -if (((defined $cgi->param('id') && $cgi->param('product') ne $oldproduct) +if ((defined $cgi->param('id') && $cgi->param('product') ne $oldproduct) || (!$cgi->param('id') && $cgi->param('product') ne $cgi->param('dontchange'))) - && CheckonComment( "reassignbycomponent" )) { + if (Bugzilla->params->{'commentonreassignbycomponent'} && !comment_exists()) { + ThrowUserError('comment_required'); + } # Check to make sure they actually have the right to change the product if (!$bug->check_can_change_field('product', $oldproduct, $cgi->param('product'), \$PrivilegesRequired)) @@ -439,6 +420,7 @@ defined($cgi->param('component')) # Confirm that the reporter of the current bug can access the bug we are duping to. sub DuplicateUserConfirm { + my ($dupe, $original) = @_; my $cgi = Bugzilla->cgi; my $dbh = Bugzilla->dbh; my $template = Bugzilla->template; @@ -448,11 +430,6 @@ sub DuplicateUserConfirm { return; } - # Remember that we validated both these ids earlier, so we know - # they are both valid bug ids - my $dupe = $cgi->param('id'); - my $original = $cgi->param('dup_id'); - my $reporter = $dbh->selectrow_array( q{SELECT reporter FROM bugs WHERE bug_id = ?}, undef, $dupe); my $rep_user = Bugzilla::User->new($reporter); @@ -618,16 +595,6 @@ sub DoComma { $::comma = ","; } -sub DoConfirm { - my $bug = shift; - if ($bug->check_can_change_field("canconfirm", 0, 1, - \$PrivilegesRequired)) - { - DoComma(); - $::query .= "everconfirmed = 1"; - } -} - # Changing this so that it will process groups from checkboxes instead of # select lists. This means that instead of looking for the bit-X values in # the form, we need to loop through all the bug groups this user has access @@ -941,127 +908,54 @@ if (defined $cgi->param('qa_contact') && !$cgi->param('set_default_qa_contact')) } } -if ($cgi->param('set_default_assignee') || $cgi->param('set_default_qa_contact')) { - CheckonComment('reassignbycomponent'); +if (($cgi->param('set_default_assignee') || $cgi->param('set_default_qa_contact')) + && Bugzilla->params->{'commentonreassignbycomponent'} && !comment_exists()) +{ + ThrowUserError('comment_required'); } my $duplicate; # It will store the ID of the bug we are pointing to, if any. -SWITCH: for ($cgi->param('knob')) { - /^none$/ && do { - last SWITCH; - }; - /^confirm$/ && CheckonComment( "confirm" ) && do { - DoConfirm($bug); - last SWITCH; - }; - /^accept$/ && CheckonComment( "accept" ) && do { - DoConfirm($bug); - if (Bugzilla->params->{"usetargetmilestone"} - && Bugzilla->params->{"musthavemilestoneonaccept"}) - { - $requiremilestone = 1; - } - last SWITCH; - }; - /^clearresolution$/ && CheckonComment( "clearresolution" ) && do { - last SWITCH; - }; - /^(resolve|change_resolution)$/ && CheckonComment( "resolve" ) && do { - # Check here, because it's the only place we require the resolution - check_field('resolution', scalar $cgi->param('resolution'), - Bugzilla::Bug->settable_resolutions); - - # don't resolve as fixed while still unresolved blocking bugs - if (Bugzilla->params->{"noresolveonopenblockers"} - && $cgi->param('resolution') eq 'FIXED') - { - my @dependencies = Bugzilla::Bug::CountOpenDependencies(@idlist); - if (scalar @dependencies > 0) { - ThrowUserError("still_unresolved_bugs", - { dependencies => \@dependencies, - dependency_count => scalar @dependencies }); - } - } - - if ($cgi->param('knob') eq 'resolve') { - # RESOLVED bugs should have no time remaining; - # more time can be added for the VERIFY step, if needed. - _remove_remaining_time(); - } - else { - # You cannot use change_resolution if there is at least - # one open bug. - my $open_states = join(',', map {$dbh->quote($_)} BUG_STATE_OPEN); - my $idlist = join(',', @idlist); - my $is_open = - $dbh->selectrow_array("SELECT 1 FROM bugs WHERE bug_id IN ($idlist) - AND bug_status IN ($open_states)"); - - ThrowUserError('resolution_not_allowed') if $is_open; - } - last SWITCH; - }; - /^reopen$/ && CheckonComment( "reopen" ) && do { - last SWITCH; - }; - /^verify$/ && CheckonComment( "verify" ) && do { - last SWITCH; - }; - /^close$/ && CheckonComment( "close" ) && do { - # CLOSED bugs should have no time remaining. - _remove_remaining_time(); - last SWITCH; - }; - /^duplicate$/ && CheckonComment( "duplicate" ) && do { - # You cannot mark bugs as duplicates when changing - # several bugs at once. - unless (defined $cgi->param('id')) { - ThrowUserError('dupe_not_allowed'); - } - - # Make sure we can change the original bug (issue A on bug 96085) - defined($cgi->param('dup_id')) - || ThrowCodeError('undefined_field', { field => 'dup_id' }); - - $duplicate = $cgi->param('dup_id'); - ValidateBugID($duplicate, 'dup_id'); - $cgi->param('dup_id', $duplicate); - - # Make sure a loop isn't created when marking this bug - # as duplicate. - my %dupes; - my $dupe_of = $duplicate; - my $sth = $dbh->prepare('SELECT dupe_of FROM duplicates - WHERE dupe = ?'); - - while ($dupe_of) { - if ($dupe_of == $cgi->param('id')) { - ThrowUserError('dupe_loop_detected', { bug_id => $cgi->param('id'), - dupe_of => $duplicate }); - } - # If $dupes{$dupe_of} is already set to 1, then a loop - # already exists which does not involve this bug. - # As the user is not responsible for this loop, do not - # prevent him from marking this bug as a duplicate. - last if exists $dupes{"$dupe_of"}; - $dupes{"$dupe_of"} = 1; - $sth->execute($dupe_of); - $dupe_of = $sth->fetchrow_array; - } - - # Also, let's see if the reporter has authorization to see - # the bug to which we are duping. If not we need to prompt. - DuplicateUserConfirm(); - - # DUPLICATE bugs should have no time remaining. - _remove_remaining_time(); - last SWITCH; - }; - - ThrowCodeError("unknown_action", { action => $cgi->param('knob') }); +# Make sure the bug status transition is legal for all bugs. +my $knob = scalar $cgi->param('knob'); +# Special actions (duplicate, change_resolution and clearresolution) are outside +# the workflow. +if (!grep { $knob eq $_ } SPECIAL_STATUS_WORKFLOW_ACTIONS) { + Bugzilla::Bug->check_status_transition($knob, \@idlist); + my $bug_status = new Bugzilla::Status({name => $knob}); + # Fill the resolution field with the correct value (e.g. in case the + # workflow allows several open -> closed transitions). + if ($bug_status->is_open) { + $cgi->delete('resolution'); + } + else { + $cgi->param('resolution', $cgi->param('resolution_knob_' . $bug_status->id)); + } +} +elsif ($knob eq 'change_resolution') { + # Fill the resolution field with the correct value. + $cgi->param('resolution', $cgi->param('resolution_knob_change_resolution')); +} +else { + # The resolution field is not in use. + $cgi->delete('resolution'); } +# The action is a valid one. +trick_taint($knob); +# Some information is required for checks. +$vars->{comment_exists} = comment_exists(); +$vars->{bug_id} = $cgi->param('id'); +$vars->{dup_id} = $cgi->param('dup_id'); +$vars->{resolution} = $cgi->param('resolution') || ''; +Bugzilla::Bug->check_status_change_triggers($knob, \@idlist, $vars); + +# Some triggers require extra actions. +$duplicate = $vars->{dup_id}; +$requiremilestone = $vars->{requiremilestone}; +DuplicateUserConfirm($vars->{bug_id}, $duplicate) if $vars->{DuplicateUserConfirm}; +_remove_remaining_time() if $vars->{remove_remaining_time}; + my @keywordlist; my %keywordseen; @@ -1252,14 +1146,15 @@ foreach my $id (@idlist) { my $comma = $::comma; my $old_bug_obj = new Bugzilla::Bug($id); - my $status; + my ($status, $everconfirmed); my $resolution = $old_bug_obj->resolution; - # These are the only actions where we care about the resolution field. - if ($cgi->param('knob') =~ /^(?:resolve|change_resolution)$/) { + # We only care about the resolution field if the user explicitly edits it + # or if he closes the bug. + if ($knob eq 'change_resolution' || $cgi->param('resolution')) { $resolution = $cgi->param('resolution'); } - ($status, $resolution) = - $old_bug_obj->get_new_status_and_resolution(scalar $cgi->param('knob'), $resolution); + ($status, $resolution, $everconfirmed) = + $old_bug_obj->get_new_status_and_resolution($knob, $resolution); if ($status ne $old_bug_obj->bug_status) { $query .= "$comma bug_status = ?"; @@ -1271,6 +1166,11 @@ foreach my $id (@idlist) { push(@bug_values, $resolution); $comma = ','; } + if ($everconfirmed ne $old_bug_obj->everconfirmed) { + $query .= "$comma everconfirmed = ?"; + push(@bug_values, $everconfirmed); + $comma = ','; + } # We have to check whether the bug is moved to another product # and/or component before reassigning. If $component is defined, @@ -1314,7 +1214,7 @@ foreach my $id (@idlist) { "user_group_map READ", "group_group_map READ", "flagtypes READ", "flaginclusions AS i READ", "flagexclusions AS e READ", "keyworddefs READ", "groups READ", "attachments READ", - "group_control_map AS oldcontrolmap READ", + "bug_status READ", "group_control_map AS oldcontrolmap READ", "group_control_map AS newcontrolmap READ", "group_control_map READ", "email_setting READ", "classifications READ"); diff --git a/reports.cgi b/reports.cgi index 0be2ab64b..065e6d73a 100755 --- a/reports.cgi +++ b/reports.cgi @@ -43,6 +43,7 @@ use Bugzilla; use Bugzilla::Constants; use Bugzilla::Util; use Bugzilla::Error; +use Bugzilla::Bug; eval "use GD"; $@ && ThrowCodeError("gd_not_installed"); diff --git a/sanitycheck.cgi b/sanitycheck.cgi index 8be69dec6..c1444ac82 100755 --- a/sanitycheck.cgi +++ b/sanitycheck.cgi @@ -32,6 +32,7 @@ use Bugzilla; use Bugzilla::Constants; use Bugzilla::Util; use Bugzilla::Error; +use Bugzilla::Bug; ########################################################################### # General subs diff --git a/template/en/default/admin/workflow/comment.html.tmpl b/template/en/default/admin/workflow/comment.html.tmpl index 5e9a788d6..df9d5e872 100644 --- a/template/en/default/admin/workflow/comment.html.tmpl +++ b/template/en/default/admin/workflow/comment.html.tmpl @@ -50,24 +50,24 @@   [% FOREACH status = statuses %] - [% status.value FILTER html %] + [% status.name FILTER html %] [% END %] [%# This defines the entry point in the workflow %] - [% p = [{id => 0, value => "{Start}", is_open => 1}] %] + [% p = [{id => 0, name => "{Start}", is_open => 1}] %] [% FOREACH status = p.merge(statuses) %] - [% status.value FILTER html %] + [% status.name FILTER html %] [% FOREACH new_status = statuses %] [% IF workflow.${status.id}.${new_status.id}.defined %] + title="From [% status.name FILTER html %] to [% new_status.name FILTER html %]"> diff --git a/template/en/default/admin/workflow/edit.html.tmpl b/template/en/default/admin/workflow/edit.html.tmpl index dee71c0a1..d602171a1 100644 --- a/template/en/default/admin/workflow/edit.html.tmpl +++ b/template/en/default/admin/workflow/edit.html.tmpl @@ -34,8 +34,12 @@

- This page allows you to define which status transitions are valid - in your workflow. + This page allows you to define which status transitions are valid in your workflow. + For compatibility with older versions of [% terms.Bugzilla %], reopening [% terms.abug %] + will only display either UNCONFIRMED or REOPENED (if allowed by your workflow) but not + both. The decision depends on whether the [% terms.bug %] has ever been confirmed or not. + So it is a good idea to allow both transitions and let [% terms.Bugzilla %] select the + correct one.

@@ -50,24 +54,24 @@   [% FOREACH status = statuses %] - [% status.value FILTER html %] + [% status.name FILTER html %] [% END %] [%# This defines the entry point in the workflow %] - [% p = [{id => 0, value => "{Start}", is_open => 1}] %] + [% p = [{id => 0, name => "{Start}", is_open => 1}] %] [% FOREACH status = p.merge(statuses) %] - [% status.value FILTER html %] + [% status.name FILTER html %] [% FOREACH new_status = statuses %] [% IF status.id != new_status.id %] + title="From [% status.name FILTER html %] to [% new_status.name FILTER html %]"> diff --git a/template/en/default/bug/edit.html.tmpl b/template/en/default/bug/edit.html.tmpl index 619c594e1..fe3adbbe9 100644 --- a/template/en/default/bug/edit.html.tmpl +++ b/template/en/default/bug/edit.html.tmpl @@ -595,7 +595,7 @@ Status: - [% status_descs.${bug.bug_status} FILTER html %] + [% get_status(bug.bug_status) FILTER html %] diff --git a/template/en/default/bug/knob.html.tmpl b/template/en/default/bug/knob.html.tmpl index 0e1a928f4..99aed9c22 100644 --- a/template/en/default/bug/knob.html.tmpl +++ b/template/en/default/bug/knob.html.tmpl @@ -18,42 +18,39 @@ # # Contributor(s): Gervase Markham # Vaskin Kissoyan + # Frédéric Buclin #%] [% PROCESS global/variables.none.tmpl %] -[%# *** Knob *** %] -
- - [% knum = 1 %] [% initial_action_shown = 0 %] - [% IF bug.isunconfirmed && bug.user.canconfirm %] - [% PROCESS initial_action %] - - -
- [% knum = knum + 1 %] - [% END %] - - [% IF bug.isopened && bug.bug_status != "ASSIGNED" && bug.user.canedit - && (!bug.isunconfirmed || bug.user.canconfirm) %] + [%# These actions are based on the current custom workflow. %] + [% FOREACH bug_status = bug.status.can_change_to %] + [% NEXT IF bug.isunconfirmed && bug_status.is_open && !bug.user.canconfirm %] + [% NEXT IF bug.isopened && !bug.isunconfirmed && bug_status.is_open && !bug.user.canedit %] + [% NEXT IF !bug_status.is_open && !bug.user.canedit && !bug.user.isreporter %] + [% NEXT IF !bug_status.is_open && bug_status.is_open && !bug.user.canedit && !bug.user.isreporter %] + [%# Special hack to only display UNCO or REOP when reopening, but not both; + # for compatibility with older versions. %] + [% NEXT IF !bug.isopened && (bug.everconfirmed && bug_status.name == "UNCONFIRMED" + || !bug.everconfirmed && bug_status.name == "REOPENED") %] [% PROCESS initial_action %] - -
- [% knum = knum + 1 %] [% END %] - - [% PROCESS initial_action %] - - - [% PROCESS select_resolution %] - - [% PROCESS duplicate %] [% ELSE %] - [% IF bug.resolution != "MOVED" || - (bug.resolution == "MOVED" && bug.user.canmove) %] + [% IF bug.resolution != "MOVED" || bug.user.canmove %] [% PROCESS initial_action %] - -
- + [% IF bug.user.canmove %] -   |   - + [% END %]
@@ -143,23 +102,20 @@ [% END %] [% BLOCK select_resolution %] - [% FOREACH r = bug.choices.resolution %] [% END %] -
- [% knum = knum + 1 %] [% END %] [% BLOCK duplicate %] - -