From 0ee52e9d7d307d6ceed73a53de1a64589bb79f62 Mon Sep 17 00:00:00 2001 From: Max Kanat-Alexander Date: Mon, 15 Mar 2010 22:37:38 -0700 Subject: Bug 526189: Refactor the way that groups are checked for being validly settable in a particular product, to eliminate the possibility of ever setting an inactive or invalid group on a product. r=LpSolit, a=LpSolit --- Bugzilla/Bug.pm | 21 +++++++++++---------- Bugzilla/Product.pm | 28 ++++++++++++++++++++++------ 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index d435d5442..9282bc3d2 100644 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -1410,7 +1410,7 @@ sub _check_groups { # so instead of doing check(), we just do "next" on an invalid # group. my $group = new Bugzilla::Group({ name => $name }) or next; - next if !$product->group_is_valid($group); + next if !$product->group_is_settable($group); $add_groups{$group->id} = $group; } } @@ -2074,15 +2074,14 @@ sub set_product { } if ($product_changed) { - # Remove groups that aren't valid in the new product. This will also - # have the side effect of removing the bug from groups that aren't - # active anymore. + # Remove groups that can't be set in the new product, or that aren't + # active. # # We copy this array because the original array is modified while we're # working, and that confuses "foreach". my @current_groups = @{$self->groups_in}; foreach my $group (@current_groups) { - if (!grep($group->id == $_->id, @{$product->groups_valid})) { + if (!$group->is_active or !$product->group_is_valid($group)) { $self->remove_group($group); } } @@ -2326,8 +2325,8 @@ sub add_group { return if !$group->is_active or !$group->is_bug_group; # Make sure that bugs in this product can actually be restricted - # to this group. - grep($group->id == $_->id, @{$self->product_obj->groups_valid}) + # to this group by the current user. + $self->product_obj->group_is_settable($group) || ThrowUserError('group_invalid_restriction', { product => $self->product, group_id => $group->id }); @@ -2355,12 +2354,14 @@ sub remove_group { return unless $group; # First, check if this is a valid group for this product. - # You can *always* remove a group that is not valid for this product, so - # we don't do any other checks if that's the case. (set_product does this.) + # You can *always* remove a group that is not valid for this product + # or that is not active, so we don't do any other checks if either of + # those are the case. (Users might remove inactive groups, and set_product + # removes groups that aren't valid for this product.) # # This particularly happens when isbuggroup is no longer 1, and we're # moving a bug to a new product. - if (grep($_->id == $group->id, @{$self->product_obj->groups_valid})) { + if ($group->is_active and $self->product_obj->group_is_valid($group)) { my $controls = $self->product_obj->group_controls->{$group->id}; # Nobody can ever remove a Mandatory group. diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm index 5c4057595..9416d3eef 100644 --- a/Bugzilla/Product.pm +++ b/Bugzilla/Product.pm @@ -666,8 +666,10 @@ sub groups_mandatory { # is Mandatory, the group is Mandatory for everybody, regardless of their # group membership. my $ids = Bugzilla->dbh->selectcol_arrayref( - "SELECT group_id FROM group_control_map - WHERE product_id = ? + "SELECT group_id + FROM group_control_map + INNER JOIN groups ON group_control_map.group_id = groups.id + WHERE product_id = ? AND isactive = 1 AND (membercontrol = $mandatory OR (othercontrol = $mandatory AND group_id NOT IN ($groups)))", @@ -677,8 +679,8 @@ sub groups_mandatory { } # We don't just check groups_valid, because we want to know specifically -# if this group is valid for the currently-logged-in user. -sub group_is_valid { +# if this group can be validly set by the currently-logged-in user. +sub group_is_settable { my ($self, $group) = @_; my $group_id = blessed($group) ? $group->id : $group; my $is_mandatory = grep { $group_id == $_->id } @@ -688,6 +690,11 @@ sub group_is_valid { return ($is_mandatory or $is_available) ? 1 : 0; } +sub group_is_valid { + my ($self, $group) = @_; + return grep($_->id == $group->id, @{ $self->groups_valid }) ? 1 : 0; +} + sub groups_valid { my ($self) = @_; return $self->{groups_valid} if defined $self->{groups_valid}; @@ -910,7 +917,7 @@ is set to Default for the currently-logged-in user. Tells you what groups are mandatory for bugs in this product, for the currently-logged-in user. Returns an arrayref of C objects. -=item C +=item C =over @@ -946,7 +953,9 @@ C<1> if the group is valid in this product, C<0> otherwise. Returns an arrayref of L objects, representing groups that bugs could validly be restricted to within this product. Used mostly -by L to assure that you're adding valid groups to a bug. +when you need the list of all possible groups that could be set in a product +by anybody, disregarding whether or not the groups are active or who the +currently logged-in user is. B: This doesn't check whether or not the current user can add/remove bugs to/from these groups. It just tells you that bugs I these @@ -958,6 +967,13 @@ groups, in this product. =back +=item C + +Returns C<1> if the passed-in L or group id could be set +on a bug by I, in this product. Even inactive groups are considered +valid. (This is a shortcut for searching L to find out if +a group is valid in a particular product.) + =item C Description: Returns all valid versions for that product. -- cgit v1.2.3-24-g4f1b