diff options
author | lpsolit%gmail.com <> | 2007-06-20 20:03:04 +0200 |
---|---|---|
committer | lpsolit%gmail.com <> | 2007-06-20 20:03:04 +0200 |
commit | 9de3481eb1733f20b1ceac522fcc94ea0d4f27c2 (patch) | |
tree | dda5dc6f3c31ec7358ec964280b177fe4a320a68 | |
parent | 1ca7c677f5cde209e71ce478762ca87168d8828f (diff) | |
download | bugzilla-9de3481eb1733f20b1ceac522fcc94ea0d4f27c2.tar.gz bugzilla-9de3481eb1733f20b1ceac522fcc94ea0d4f27c2.tar.xz |
Bug 347213: When moving a bug to another product, Bugzilla should let us preemptively put the bug into a (valid) group which is not a default one for this product
Bug 303183: Bugzilla fails to warn when a product change would force the removal of an optional group
Patch by Frédéric Buclin <LpSolit@gmail.com> r=mkanat a=LpSolit
-rwxr-xr-x | process_bug.cgi | 386 | ||||
-rw-r--r-- | template/en/default/bug/process/verify-new-product.html.tmpl | 93 |
2 files changed, 178 insertions, 301 deletions
diff --git a/process_bug.cgi b/process_bug.cgi index 18acb0c91..e3143ac98 100755 --- a/process_bug.cgi +++ b/process_bug.cgi @@ -78,34 +78,6 @@ local our $PrivilegesRequired = 0; # Subroutines ###################################################################### -sub BugInGroupId { - my ($bug_id, $group_id) = @_; - detaint_natural($bug_id); - detaint_natural($group_id); - my ($in_group) = Bugzilla->dbh->selectrow_array( - "SELECT CASE WHEN bug_id != 0 THEN 1 ELSE 0 END - FROM bug_group_map - WHERE bug_id = ? AND group_id = ?", undef, ($bug_id, $group_id)); - return $in_group; -} - -# This function checks if there are any default groups defined. -# If so, then groups may have to be changed when bugs move from -# one bug to another. -sub AnyDefaultGroups { - my $dbh = Bugzilla->dbh; - my $any_default = - $dbh->selectrow_array('SELECT 1 - FROM group_control_map - INNER JOIN groups - ON groups.id = group_control_map.group_id - WHERE isactive != 0 - AND (membercontrol = ? OR othercontrol = ?) ' . - $dbh->sql_limit(1), - undef, (CONTROLMAPDEFAULT, CONTROLMAPDEFAULT)); - return $any_default; -} - # Used to send email when an update is done. sub send_results { my ($bug_id, $vars) = @_; @@ -329,8 +301,7 @@ if ((defined $cgi->param('id') && $cgi->param('product') ne $bug->product) # shown. Also show the verification form if the product-specific fields # somehow still need to be verified, or if we need to verify whether or not # to add the bugs to their new product's group. - if (!$vok || !$cok || !$mok || !defined $cgi->param('confirm_product_change') - || (AnyDefaultGroups() && !defined $cgi->param('addtonewgroup'))) { + if (!$vok || !$cok || !$mok || !defined $cgi->param('confirm_product_change')) { if (Bugzilla->usage_mode == USAGE_MODE_EMAIL) { if (!$vok) { @@ -349,52 +320,53 @@ if ((defined $cgi->param('id') && $cgi->param('product') ne $bug->product) milestone => $cgi->param('target_milestone')}); } } - - if (!$vok || !$cok || !$mok - || !defined $cgi->param('confirm_product_change')) - { - $vars->{'verify_fields'} = 1; - my %defaults; - # We set the defaults to these fields to the old value, - # if it's a valid option, otherwise we use the default where - # that's appropriate - $vars->{'versions'} = \@version_names; - if ($vok) { - $defaults{'version'} = $cgi->param('version'); - } - elsif (scalar(@version_names) == 1) { - $defaults{'version'} = $version_names[0]; - } - $vars->{'components'} = \@component_names; - if ($cok) { - $defaults{'component'} = $cgi->param('component'); - } - elsif (scalar(@component_names) == 1) { - $defaults{'component'} = $component_names[0]; - } + $vars->{'product'} = $product; + my %defaults; + # We set the defaults to these fields to the old value, + # if it's a valid option, otherwise we use the default where + # that's appropriate + $vars->{'versions'} = \@version_names; + if ($vok) { + $defaults{'version'} = $cgi->param('version'); + } + elsif (scalar(@version_names) == 1) { + $defaults{'version'} = $version_names[0]; + } - if (Bugzilla->params->{"usetargetmilestone"}) { - $vars->{'use_target_milestone'} = 1; - $vars->{'milestones'} = \@milestone_names; - if ($mok) { - $defaults{'target_milestone'} = $cgi->param('target_milestone'); - } else { - $defaults{'target_milestone'} = $product->default_milestone; - } - } - else { - $vars->{'use_target_milestone'} = 0; - } - $vars->{'defaults'} = \%defaults; + $vars->{'components'} = \@component_names; + if ($cok) { + $defaults{'component'} = $cgi->param('component'); } - else { - $vars->{'verify_fields'} = 0; + elsif (scalar(@component_names) == 1) { + $defaults{'component'} = $component_names[0]; } - - $vars->{'verify_bug_group'} = (AnyDefaultGroups() - && !defined $cgi->param('addtonewgroup')); - + + if (Bugzilla->params->{"usetargetmilestone"}) { + $vars->{'milestones'} = \@milestone_names; + if ($mok) { + $defaults{'target_milestone'} = $cgi->param('target_milestone'); + } else { + $defaults{'target_milestone'} = $product->default_milestone; + } + } + $vars->{'defaults'} = \%defaults; + + # Get the ID of groups which are no longer valid in the new product. + my $gids = + $dbh->selectcol_arrayref('SELECT bgm.group_id + FROM bug_group_map AS bgm + WHERE bgm.bug_id IN (' . join(', ', @idlist) . ') + AND bgm.group_id NOT IN + (SELECT gcm.group_id + FROM group_control_map AS gcm + WHERE gcm.product_id = ? + AND ((gcm.membercontrol != ? + AND gcm.group_id IN (' . $grouplist . ')) + OR gcm.othercontrol != ?))', + undef, ($product->id, CONTROLMAPNA, CONTROLMAPNA)); + $vars->{'old_groups'} = Bugzilla::Group->new_from_list($gids); + $template->process("bug/process/verify-new-product.html.tmpl", $vars) || ThrowTemplateError($template->error()); exit; @@ -578,36 +550,6 @@ sub DoComma { $::comma = ","; } -# 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 -# to, and for each one, see if it's selected. -# If the form element isn't present, or the user isn't in the group, leave -# it as-is - -my @groupAdd = (); -my @groupDel = (); - -my $groups = $dbh->selectall_arrayref( - qq{SELECT groups.id, isactive FROM groups - WHERE id IN($grouplist) AND isbuggroup = 1}); -foreach my $group (@$groups) { - my ($b, $isactive) = @$group; - # The multiple change page may not show all groups a bug is in - # (eg product groups when listing more than one product) - # Only consider groups which were present on the form. We can't do this - # for single bug changes because non-checked checkboxes aren't present. - # All the checkboxes should be shown in that case, though, so it isn't - # an issue there - if (defined $cgi->param('id') || defined $cgi->param("bit-$b")) { - if (!$cgi->param("bit-$b")) { - push(@groupDel, $b); - } elsif ($cgi->param("bit-$b") == 1 && $isactive) { - push(@groupAdd, $b); - } - } -} - foreach my $field ("rep_platform", "priority", "bug_severity", "bug_file_loc", "short_desc", "version", "op_sys", "target_milestone", "status_whiteboard") { @@ -966,8 +908,7 @@ if (!grep($keywordaction eq $_, qw(add delete makeexact))) { } if ($::comma eq "" - && (! @groupAdd) && (! @groupDel) - && (!Bugzilla::Keyword::keyword_count() + && (!Bugzilla::Keyword::keyword_count() || (0 == @keywordlist && $keywordaction ne "makeexact")) && defined $cgi->param('masscc') && ! $cgi->param('masscc') ) { @@ -1285,18 +1226,21 @@ foreach my $id (@idlist) { { product => $oldhash{'product'} }); } + # If we are editing a single bug or if bugs are being moved into + # a specific product, $product is defined. If $product is undefined, + # then we don't move bugs, so we can use their current product. + my $new_product = $product || new Bugzilla::Product({name => $oldhash{'product'}}); if ($requiremilestone) { # musthavemilestoneonaccept applies only if at least two - # target milestones are defined for the current product. - my $old_product = new Bugzilla::Product({name => $oldhash{'product'}}); - if (scalar(@{$old_product->milestones}) > 1) { + # target milestones are defined for the product. + if (scalar(@{$new_product->milestones}) > 1) { my $value = $cgi->param('target_milestone'); if (!defined $value || $value eq $cgi->param('dontchange')) { $value = $oldhash{'target_milestone'}; } # if musthavemilestoneonaccept == 1, then the target # milestone must be different from the default one. - if ($value eq $old_product->default_milestone) { + if ($value eq $new_product->default_milestone) { ThrowUserError("milestone_required", { bug_id => $id }); } } @@ -1415,60 +1359,69 @@ foreach my $id (@idlist) { $dbh->do(q{DELETE FROM duplicates WHERE dupe = ?}, undef, $id); } - my %groupsrequired = (); - my %groupsforbidden = (); - my $group_controls = - $dbh->selectall_arrayref(q{SELECT id, membercontrol - FROM groups - LEFT JOIN group_control_map - ON id = group_id - AND product_id = ? - WHERE isactive != 0}, - undef, $oldhash{'product_id'}); - foreach my $group_control (@$group_controls) { - my ($group, $control) = @$group_control; - $control ||= 0; - unless ($control > CONTROLMAPNA) { - $groupsforbidden{$group} = 1; + # First of all, get all groups the bug is currently restricted to. + my $initial_groups = + $dbh->selectcol_arrayref('SELECT group_id, name + FROM bug_group_map + INNER JOIN groups + ON groups.id = bug_group_map.group_id + WHERE bug_id = ?', {Columns=>[1,2]}, $id); + my %original_groups = @$initial_groups; + my %updated_groups = %original_groups; + + # Now let's see which groups have to be added or removed. + foreach my $gid (keys %{$new_product->group_controls}) { + my $group = $new_product->group_controls->{$gid}; + # Leave inactive groups alone. + next unless $group->{group}->is_active; + + if ($group->{membercontrol} == CONTROLMAPMANDATORY + || ($group->{othercontrol} == CONTROLMAPMANDATORY && !$user->in_group_id($gid))) + { + $updated_groups{$gid} = $group->{group}->name; } - if ($control == CONTROLMAPMANDATORY) { - $groupsrequired{$group} = 1; + elsif ($group->{membercontrol} == CONTROLMAPNA + || ($group->{othercontrol} == CONTROLMAPNA && !$user->in_group_id($gid))) + { + delete $updated_groups{$gid}; } - } - - my @groupAddNames = (); - my @groupAddNamesAll = (); - my $sth = $dbh->prepare(q{INSERT INTO bug_group_map (bug_id, group_id) - VALUES (?, ?)}); - foreach my $grouptoadd (@groupAdd, keys %groupsrequired) { - next if $groupsforbidden{$grouptoadd}; - my $group_obj = new Bugzilla::Group($grouptoadd); - push(@groupAddNamesAll, $group_obj->name); - if (!BugInGroupId($id, $grouptoadd)) { - push(@groupAddNames, $group_obj->name); - $sth->execute($id, $grouptoadd); + # When editing several bugs at once, only consider groups which + # have been displayed. + elsif (defined $cgi->param('id') || defined $cgi->param("bit-$gid")) { + if (!$cgi->param("bit-$gid")) { + delete $updated_groups{$gid}; + } + # Note that == 1 is important, because == -1 means "ignore this group". + elsif ($cgi->param("bit-$gid") == 1) { + $updated_groups{$gid} = $group->{group}->name; + } } } - my @groupDelNames = (); - my @groupDelNamesAll = (); - $sth = $dbh->prepare(q{DELETE FROM bug_group_map - WHERE bug_id = ? AND group_id = ?}); - foreach my $grouptodel (@groupDel, keys %groupsforbidden) { - my $group_obj = new Bugzilla::Group($grouptodel); - push(@groupDelNamesAll, $group_obj->name); - next if $groupsrequired{$grouptodel}; - if (BugInGroupId($id, $grouptodel)) { - push(@groupDelNames, $group_obj->name); - } - $sth->execute($id, $grouptodel); + # We also have to remove groups which are not legal in the new product. + foreach my $gid (keys %updated_groups) { + delete $updated_groups{$gid} + unless exists $new_product->group_controls->{$gid}; } + my ($removed, $added) = diff_arrays([keys %original_groups], [keys %updated_groups]); - my $groupDelNames = join(',', @groupDelNames); - my $groupAddNames = join(',', @groupAddNames); + # We can now update the DB. + if (scalar(@$removed)) { + $dbh->do('DELETE FROM bug_group_map WHERE bug_id = ? + AND group_id IN (' . join(', ', @$removed) . ')', + undef, $id); + } + if (scalar(@$added)) { + my $sth = $dbh->prepare('INSERT INTO bug_group_map + (bug_id, group_id) VALUES (?, ?)'); + $sth->execute($id, $_) foreach @$added; + } - if ($groupDelNames ne $groupAddNames) { - LogActivityEntry($id, "bug_group", $groupDelNames, $groupAddNames, - $whoid, $timestamp); + # Add the changes to the bug_activity table. + if (scalar(@$removed) || scalar(@$added)) { + my @removed_names = map { $original_groups{$_} } @$removed; + my @added_names = map { $updated_groups{$_} } @$added; + LogActivityEntry($id, 'bug_group', join(',', @removed_names), + join(',', @added_names), $whoid, $timestamp); $bug_changed = 1; } @@ -1581,128 +1534,7 @@ foreach my $id (@idlist) { } } - # When a bug changes products and the old or new product is associated - # with a bug group, it may be necessary to remove the bug from the old - # group or add it to the new one. There are a very specific series of - # conditions under which these activities take place, more information - # about which can be found in comments within the conditionals below. - # Check if the user has changed the product to which the bug belongs; - if ($cgi->param('product') ne $cgi->param('dontchange') - && $cgi->param('product') ne $oldhash{'product'}) - { - # Depending on the "addtonewgroup" variable, groups with - # defaults will change. - # - # For each group, determine - # - The group id and if it is active - # - The control map value for the old product and this group - # - The control map value for the new product and this group - # - Is the user in this group? - # - Is the bug in this group? - my $groups = $dbh->selectall_arrayref( - qq{SELECT DISTINCT groups.id, isactive, - oldcontrolmap.membercontrol, - newcontrolmap.membercontrol, - CASE WHEN groups.id IN ($grouplist) THEN 1 ELSE 0 END, - CASE WHEN bug_group_map.group_id IS NOT NULL - THEN 1 ELSE 0 END - FROM groups - LEFT JOIN group_control_map AS oldcontrolmap - ON oldcontrolmap.group_id = groups.id - AND oldcontrolmap.product_id = ? - LEFT JOIN group_control_map AS newcontrolmap - ON newcontrolmap.group_id = groups.id - AND newcontrolmap.product_id = ? - LEFT JOIN bug_group_map - ON bug_group_map.group_id = groups.id - AND bug_group_map.bug_id = ?}, - undef, $oldhash{'product_id'}, $product->id, $id); - my @groupstoremove = (); - my @groupstoadd = (); - my @defaultstoremove = (); - my @defaultstoadd = (); - my @allgroups = (); - my $buginanydefault = 0; - my $buginanychangingdefault = 0; - foreach my $group (@$groups) { - my ($groupid, $isactive, $oldcontrol, $newcontrol, - $useringroup, $bugingroup) = @$group; - # An undefined newcontrol is none. - $newcontrol = CONTROLMAPNA unless $newcontrol; - $oldcontrol = CONTROLMAPNA unless $oldcontrol; - push(@allgroups, $groupid); - if (($bugingroup) && ($isactive) - && ($oldcontrol == CONTROLMAPDEFAULT)) { - # Bug was in a default group. - $buginanydefault = 1; - if (($newcontrol != CONTROLMAPDEFAULT) - && ($newcontrol != CONTROLMAPMANDATORY)) { - # Bug was in a default group that no longer is. - $buginanychangingdefault = 1; - push (@defaultstoremove, $groupid); - } - } - if (($isactive) && (!$bugingroup) - && ($newcontrol == CONTROLMAPDEFAULT) - && ($useringroup)) { - push (@defaultstoadd, $groupid); - } - if (($bugingroup) && ($isactive) && ($newcontrol == CONTROLMAPNA)) { - # Group is no longer permitted. - push(@groupstoremove, $groupid); - } - if ((!$bugingroup) && ($isactive) - && ($newcontrol == CONTROLMAPMANDATORY)) { - # Group is now required. - push(@groupstoadd, $groupid); - } - } - # If addtonewgroups = "yes", old default groups will be removed - # and new default groups will be added. - # If addtonewgroups = "yesifinold", old default groups will be removed - # and new default groups will be added only if the bug was in ANY - # of the old default groups. - # If addtonewgroups = "no", old default groups will be removed and not - # replaced. - push(@groupstoremove, @defaultstoremove); - if (AnyDefaultGroups() - && (($cgi->param('addtonewgroup') eq 'yes') - || (($cgi->param('addtonewgroup') eq 'yesifinold') - && ($buginanydefault)))) { - push(@groupstoadd, @defaultstoadd); - } - - # Now actually update the bug_group_map. - my @DefGroupsAdded = (); - my @DefGroupsRemoved = (); - my $sth_insert = - $dbh->prepare(q{INSERT INTO bug_group_map (bug_id, group_id) - VALUES (?, ?)}); - my $sth_delete = $dbh->prepare(q{DELETE FROM bug_group_map - WHERE bug_id = ? - AND group_id = ?}); - foreach my $groupid (@allgroups) { - my $thisadd = grep( ($_ == $groupid), @groupstoadd); - my $thisdel = grep( ($_ == $groupid), @groupstoremove); - if ($thisadd) { - my $group_obj = new Bugzilla::Group($groupid); - push(@DefGroupsAdded, $group_obj->name); - $sth_insert->execute($id, $groupid); - } elsif ($thisdel) { - my $group_obj = new Bugzilla::Group($groupid); - push(@DefGroupsRemoved, $group_obj->name); - $sth_delete->execute($id, $groupid); - } - } - if ((@DefGroupsAdded) || (@DefGroupsRemoved)) { - LogActivityEntry($id, "bug_group", - join(', ', @DefGroupsRemoved), - join(', ', @DefGroupsAdded), - $whoid, $timestamp); - } - } - - # get a snapshot of the newly set values out of the database, + # get a snapshot of the newly set values out of the database, # and then generate any necessary bug activity entries by seeing # what has changed since before we wrote out the new values. # diff --git a/template/en/default/bug/process/verify-new-product.html.tmpl b/template/en/default/bug/process/verify-new-product.html.tmpl index 2355f7506..ddca10393 100644 --- a/template/en/default/bug/process/verify-new-product.html.tmpl +++ b/template/en/default/bug/process/verify-new-product.html.tmpl @@ -17,20 +17,16 @@ # Rights Reserved. # # Contributor(s): Myk Melez <myk@mozilla.org> + # Frédéric Buclin <LpSolit@gmail.com> #%] [%# INTERFACE: - # verify_fields: boolean; whether or not to verify - # the version, component, and target milestone fields + # product: object; the new product. # versions: array; versions for the new product. # components: array; components for the new product. # milestones: array; milestones for the new product. # defaults: hash; keys are names of fields, values are defaults for # those fields - # verify_bug_group: boolean; whether or not to ask the user - # if they want to add the bug to its new product's group - # use_target_milestone: boolean; whether or not to use - # the target milestone field #%] [%# The global Bugzilla->cgi object is used to obtain form variable values. %] @@ -45,15 +41,14 @@ <form action="process_bug.cgi" method="post"> [% PROCESS "global/hidden-fields.html.tmpl" - exclude=(verify_fields ? "^version|component|target_milestone$" : "") %] + exclude=("^version|component|target_milestone|bit-\\d+$") %] <input type="hidden" name="confirm_product_change" value="1"> [%# Verify the version, component, and target milestone fields. %] -[% IF verify_fields %] - <h3>Verify Version, Component[% ", Target Milestone" IF use_target_milestone %]</h3> + <h3>Verify Version, Component[% ", Target Milestone" IF Param("usetargetmilestone") %]</h3> <p> - [% IF use_target_milestone %] + [% IF Param("usetargetmilestone") %] You are moving the [% terms.bug %](s) to the product <b>[% cgi.param("product") FILTER html %]</b>, and the version, component, and/or target milestone fields are no longer @@ -84,7 +79,7 @@ default=defaults.component size=10 %] </td> - [% IF use_target_milestone %] + [% IF Param("usetargetmilestone") %] <td> <b>Target Milestone:</b><br> [% PROCESS "global/select-menu.html.tmpl" @@ -97,22 +92,72 @@ </tr> </table> -[% END %] - -[% IF verify_bug_group %] <h3>Verify [% terms.Bug %] Group</h3> - <p> - Do you want to add the [% terms.bug %] to its new product's default groups (if any)? - </p> + [% IF old_groups.size %] + <p>These groups are not legal for the '[% product.name FILTER html %]' + product or you are not allowed to restrict [% terms.bugs %] to these groups. + [%+ terms.Bugs %] will no longer be restricted to these groups and may become + public if no other group applies:<br> + [% FOREACH group = old_groups %] + <input type="checkbox" id="bit-[% group.id FILTER html %]" + name="bit-[% group.id FILTER html %]" disabled="disabled" value="1"> + <label for="bit-[% group.id FILTER html %]"> + [% group.name FILTER html %]: [% group.description FILTER html %] + </label> + <br> + [% END %] + </p> + [% END %] - <p> - <input type="radio" name="addtonewgroup" value="no"><b>no</b><br> - <input type="radio" name="addtonewgroup" value="yes"><b>yes</b><br> - <input type="radio" name="addtonewgroup" value="yesifinold" checked="checked"> - <b>yes, but only if the [% terms.bug %] was in any of its old product's default groups</b><br> - </p> -[% END %] + [% mandatory_groups = [] %] + [% optional_groups = [] %] + + [% FOREACH gid = product.group_controls.keys %] + [% group = product.group_controls.$gid %] + [% NEXT UNLESS group.group.is_active %] + + [% IF group.membercontrol == constants.CONTROLMAPMANDATORY + || (group.othercontrol == constants.CONTROLMAPMANDATORY && !user.in_group(group.group.name)) %] + [% mandatory_groups.push(group) %] + [% ELSIF (group.membercontrol != constants.CONTROLMAPNA && user.in_group(group.group.name)) + || group.othercontrol != constants.CONTROLMAPNA %] + [% optional_groups.push(group) %] + [% END %] + [% END %] + + [% IF optional_groups.size %] + <p>These groups are optional. You can decide to restrict [% terms.bugs %] to + one or more of the following groups:<br> + [% FOREACH group = optional_groups %] + <input type="checkbox" id="bit-[% group.group.id FILTER html %]" + name="bit-[% group.group.id FILTER html %]" + [% ((group.membercontrol == constants.CONTROLMAPDEFAULT && user.in_group(group.group.name)) + || (group.othercontrol == constants.CONTROLMAPDEFAULT && !user.in_group(group.group.name)) + || cgi.param("bit-$group.group.id") == 1) ? + 'checked="checked"' : '' + %] value="1"> + <label for="bit-[% group.group.id FILTER html %]"> + [% group.group.name FILTER html %]: [% group.group.description FILTER html %] + </label> + <br> + [% END %] + </p> + [% END %] + + [% IF mandatory_groups.size %] + <p>These groups are mandatory and [% terms.bugs %] will be automatically + restricted to these groups:<br> + [% FOREACH group = mandatory_groups %] + <input type="checkbox" id="bit-[% group.group.id FILTER html %]" checked="checked" + name="bit-[% group.group.id FILTER html %]" value="1" disabled="disabled"> + <label for="bit-[% group.group.id FILTER html %]"> + [% group.group.name FILTER html %]: [% group.group.description FILTER html %] + </label> + <br> + [% END %] + </p> + [% END %] <input type="submit" id="change_product" value="Commit"> |