Bug 448593: Move code to edit product group settings from editproducts.cgi to Bugzilla/Product.pm - Patch by Frédéric Buclin <LpSolit@gmail.com> r=mkanat a=LpSolit

8 files changed, 330 insertions, 344 deletions

diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm
index febebfb0c..235a06926 100644
--- a/Bugzilla/Product.pm
+++ b/Bugzilla/Product.pm
@@ -241,7 +241,121 @@ sub update {
         }
         $changes->{'confirmed_bugs'} = \@updated_bugs;
     }
+
+    # Also update group settings.
+    if ($self->{check_group_controls}) {
+        require Bugzilla::Bug;
+        import Bugzilla::Bug qw(LogActivityEntry);
+
+        my $old_settings = $self->new($self->id)->group_controls;
+        my $new_settings = $self->group_controls;
+        my $timestamp = $dbh->selectrow_array('SELECT NOW()');
+
+        foreach my $gid (keys %$new_settings) {
+            my $old_setting = $old_settings->{$gid} || {};
+            my $new_setting = $new_settings->{$gid};
+            # If all new settings are 0 for a given group, we delete the entry
+            # from group_control_map, so we have to track it here.
+            my $all_zero = 1;
+            my @fields;
+            my @values;
+
+            foreach my $field ('entry', 'membercontrol', 'othercontrol', 'canedit',
+                               'editcomponents', 'editbugs', 'canconfirm')
+            {
+                my $old_value = $old_setting->{$field};
+                my $new_value = $new_setting->{$field};
+                $all_zero = 0 if $new_value;
+                next if (defined $old_value && $old_value == $new_value);
+                push(@fields, $field);
+                # The value has already been validated.
+                detaint_natural($new_value);
+                push(@values, $new_value);
+            }
+            # Is there anything to update?
+            next unless scalar @fields;
+
+            if ($all_zero) {
+                $dbh->do('DELETE FROM group_control_map
+                          WHERE product_id = ? AND group_id = ?',
+                          undef, $self->id, $gid);
+            }
+            else {
+                if (exists $old_setting->{group}) {
+                    # There is already an entry in the DB.
+                    my $set_fields = join(', ', map {"$_ = ?"} @fields);
+                    $dbh->do("UPDATE group_control_map SET $set_fields
+                              WHERE product_id = ? AND group_id = ?",
+                              undef, (@values, $self->id, $gid));
+                }
+                else {
+                    # No entry yet.
+                    my $fields = join(', ', @fields);
+                    # +2 because of the product and group IDs.
+                    my $qmarks = join(',', ('?') x (scalar @fields + 2));
+                    $dbh->do("INSERT INTO group_control_map (product_id, group_id, $fields)
+                              VALUES ($qmarks)", undef, ($self->id, $gid, @values));
+                }
+            }
+
+            # If the group is mandatory, restrict all bugs to it.
+            if ($new_setting->{membercontrol} == CONTROLMAPMANDATORY) {
+                my $bug_ids =
+                  $dbh->selectcol_arrayref('SELECT bugs.bug_id
+                                              FROM bugs
+                                                   LEFT JOIN bug_group_map
+                                                   ON bug_group_map.bug_id = bugs.bug_id
+                                                   AND group_id = ?
+                                             WHERE product_id = ?
+                                                   AND bug_group_map.bug_id IS NULL',
+                                             undef, $gid, $self->id);
+
+                if (scalar @$bug_ids) {
+                    my $sth = $dbh->prepare('INSERT INTO bug_group_map (bug_id, group_id)
+                                             VALUES (?, ?)');
+
+                    foreach my $bug_id (@$bug_ids) {
+                        $sth->execute($bug_id, $gid);
+                        # Add this change to the bug history.
+                        LogActivityEntry($bug_id, 'bug_group', '',
+                                         $new_setting->{group}->name,
+                                         Bugzilla->user->id, $timestamp);
+                    }
+                    push(@{$changes->{'group_controls'}->{'now_mandatory'}},
+                         {name      => $new_setting->{group}->name,
+                          bug_count => scalar @$bug_ids});
+                }
+            }
+            # If the group can no longer be used to restrict bugs, remove them.
+            elsif ($new_setting->{membercontrol} == CONTROLMAPNA) {
+                my $bug_ids =
+                  $dbh->selectcol_arrayref('SELECT bugs.bug_id
+                                              FROM bugs
+                                                   INNER JOIN bug_group_map
+                                                   ON bug_group_map.bug_id = bugs.bug_id
+                                             WHERE product_id = ? AND group_id = ?',
+                                             undef, $self->id, $gid);
+
+                if (scalar @$bug_ids) {
+                    $dbh->do('DELETE FROM bug_group_map WHERE group_id = ? AND ' .
+                              $dbh->sql_in('bug_id', $bug_ids), undef, $gid);
+
+                    # Add this change to the bug history.
+                    foreach my $bug_id (@$bug_ids) {
+                        LogActivityEntry($bug_id, 'bug_group',
+                                         $old_setting->{group}->name, '',
+                                         Bugzilla->user->id, $timestamp);
+                    }
+                    push(@{$changes->{'group_controls'}->{'now_na'}},
+                         {name => $old_setting->{group}->name,
+                          bug_count => scalar @$bug_ids});
+                }
+            }
+        }
+    }
     $dbh->bz_commit_transaction();
+    # Changes have been committed.
+    delete $self->{check_group_controls};
 
     # Now that changes have been committed, we can send emails to voters.
     foreach my $msg (@msgs) {
@@ -471,6 +585,63 @@ sub set_votes_per_user { $_[0]->set('votesperuser', $_[1]); }
 sub set_votes_per_bug { $_[0]->set('maxvotesperbug', $_[1]); }
 sub set_votes_to_confirm { $_[0]->set('votestoconfirm', $_[1]); }
 
+sub set_group_controls {
+    my ($self, $group, $settings) = @_;
+
+    $group->is_active_bug_group
+      || ThrowUserError('product_illegal_group', {group => $group});
+
+    scalar(keys %$settings)
+      || ThrowCodeError('product_empty_group_controls', {group => $group});
+
+    # We store current settings for this group.
+    my $gs = $self->group_controls->{$group->id};
+    # If there is no entry for this group yet, create a default hash.
+    unless (defined $gs) {
+        $gs = { entry          => 0,
+                membercontrol  => CONTROLMAPNA,
+                othercontrol   => CONTROLMAPNA,
+                canedit        => 0,
+                editcomponents => 0,
+                editbugs       => 0,
+                canconfirm     => 0,
+                group          => $group };
+    }
+
+    # Both settings must be defined, or none of them can be updated.
+    if (defined $settings->{membercontrol} && defined $settings->{othercontrol}) {
+        #  Legality of control combination is a function of
+        #  membercontrol\othercontrol
+        #                 NA SH DE MA
+        #              NA  +  -  -  -
+        #              SH  +  +  +  +
+        #              DE  +  -  +  +
+        #              MA  -  -  -  +
+        foreach my $field ('membercontrol', 'othercontrol') {
+            my ($is_legal) = grep { $settings->{$field} == $_ }
+              (CONTROLMAPNA, CONTROLMAPSHOWN, CONTROLMAPDEFAULT, CONTROLMAPMANDATORY);
+            defined $is_legal || ThrowCodeError('product_illegal_group_control',
+                                   { field => $field, value => $settings->{$field} });
+        }
+        unless ($settings->{membercontrol} == $settings->{othercontrol}
+                || $settings->{membercontrol} == CONTROLMAPSHOWN
+                || ($settings->{membercontrol} == CONTROLMAPDEFAULT
+                    && $settings->{othercontrol} != CONTROLMAPSHOWN))
+        {
+            ThrowUserError('illegal_group_control_combination', {groupname => $group->name});
+        }
+        $gs->{membercontrol} = $settings->{membercontrol};
+        $gs->{othercontrol} = $settings->{othercontrol};
+    }
+
+    foreach my $field ('entry', 'canedit', 'editcomponents', 'editbugs', 'canconfirm') {
+        next unless defined $settings->{$field};
+        $gs->{$field} = $settings->{$field} ? 1 : 0;
+    }
+    $self->{group_controls}->{$group->id} = $gs;
+    $self->{check_group_controls} = 1;
+}
+
 sub components {
     my $self = shift;
     my $dbh = Bugzilla->dbh;
@@ -488,25 +659,33 @@ sub components {
 }
 
 sub group_controls {
-    my $self = shift;
+    my ($self, $full_data) = @_;
     my $dbh = Bugzilla->dbh;
 
-    if (!defined $self->{group_controls}) {
-        my $query = qq{SELECT
-                       groups.id,
-                       group_control_map.entry,
-                       group_control_map.membercontrol,
-                       group_control_map.othercontrol,
-                       group_control_map.canedit,
-                       group_control_map.editcomponents,
-                       group_control_map.editbugs,
-                       group_control_map.canconfirm
-                  FROM groups
-                  LEFT JOIN group_control_map
-                        ON groups.id = group_control_map.group_id
-                  WHERE group_control_map.product_id = ?
-                  AND   groups.isbuggroup != 0
-                  ORDER BY groups.name};
+    # By default, we don't return groups which are not listed in
+    # group_control_map. If $full_data is true, then we also
+    # return groups whose settings could be set for the product.
+    my $where_or_and = 'WHERE';
+    my $and_or_where = 'AND';
+    if ($full_data) {
+        $where_or_and = 'AND';
+        $and_or_where = 'WHERE';
+    }
+
+    # If $full_data is true, we collect all the data in all cases,
+    # even if the cache is already populated.
+    # $full_data is never used except in the very special case where
+    # all configurable bug groups are displayed to administrators,
+    # so we don't care about collecting all the data again in this case.
+    if (!defined $self->{group_controls} || $full_data) {
+        # Include name to the list, to allow us sorting data more easily.
+        my $query = qq{SELECT id, name, entry, membercontrol, othercontrol,
+                              canedit, editcomponents, editbugs, canconfirm
+                         FROM groups
+                              LEFT JOIN group_control_map
+                              ON id = group_id 
+                $where_or_and product_id = ?
+                $and_or_where isbuggroup = 1};
         $self->{group_controls} = 
             $dbh->selectall_hashref($query, 'id', undef, $self->id);
 
@@ -515,6 +694,21 @@ sub group_controls {
         my $groups = Bugzilla::Group->new_from_list(\@gids);
         $self->{group_controls}->{$_->id}->{group} = $_ foreach @$groups;
     }
+
+    # We never cache bug counts, for the same reason as above.
+    if ($full_data) {
+        my $counts =
+          $dbh->selectall_arrayref('SELECT group_id, COUNT(bugs.bug_id) AS bug_count
+                                      FROM bug_group_map
+                                INNER JOIN bugs
+                                        ON bugs.bug_id = bug_group_map.bug_id
+                                     WHERE bugs.product_id = ? ' .
+                                     $dbh->sql_group_by('group_id'),
+                          {'Slice' => {}}, $self->id);
+        foreach my $data (@$counts) {
+            $self->{group_controls}->{$data->{group_id}}->{bug_count} = $data->{bug_count};
+        }
+    }
     return $self->{group_controls};
 }
 
@@ -730,7 +924,10 @@ below.
  Description: Returns a hash (group id as key) with all product
               group controls.
 
- Params:      none.
+ Params:      $full_data (optional, false by default) - when true,
+              the number of bugs per group applicable to the product
+              is also returned. Moreover, bug groups which have no
+              special settings for the product are also returned.
 
  Returns:     A hash with group id as key and hash containing 
               a Bugzilla::Group object and the properties of group
diff --git a/editproducts.cgi b/editproducts.cgi
index 68c64ca41..e3af7986e 100755
--- a/editproducts.cgi
+++ b/editproducts.cgi
@@ -35,7 +35,7 @@ use Bugzilla;
 use Bugzilla::Constants;
 use Bugzilla::Util;
 use Bugzilla::Error;
-use Bugzilla::Bug;
+use Bugzilla::Group;
 use Bugzilla::Product;
 use Bugzilla::Classification;
 use Bugzilla::Token;
@@ -273,7 +273,54 @@ if ($action eq 'edit' || (!$action && $product_name)) {
 }
 
 #
-# action='updategroupcontrols' -> update the product
+# action='update' -> update the product
+#
+if ($action eq 'update') {
+    check_token_data($token, 'edit_product');
+    my $product_old_name = trim($cgi->param('product_old_name') || '');
+    my $product = $user->check_can_admin_product($product_old_name);
+
+    $product->set_name($product_name);
+    $product->set_description(scalar $cgi->param('description'));
+    $product->set_default_milestone(scalar $cgi->param('defaultmilestone'));
+    $product->set_milestone_url(scalar $cgi->param('milestoneurl'));
+    $product->set_disallow_new(scalar $cgi->param('disallownew'));
+    $product->set_votes_per_user(scalar $cgi->param('votesperuser'));
+    $product->set_votes_per_bug(scalar $cgi->param('maxvotesperbug'));
+    $product->set_votes_to_confirm(scalar $cgi->param('votestoconfirm'));
+
+    my $changes = $product->update();
+
+    delete_token($token);
+
+    if (Bugzilla->params->{'useclassification'}) {
+        $vars->{'classification'} = new Bugzilla::Classification($product->classification_id);
+    }
+    $vars->{'product'} = $product;
+    $vars->{'changes'} = $changes;
+
+    $template->process("admin/products/updated.html.tmpl", $vars)
+        || ThrowTemplateError($template->error());
+    exit;
+}
+
+#
+# action='editgroupcontrols' -> display product group controls
+#
+
+if ($action eq 'editgroupcontrols') {
+    my $product = $user->check_can_admin_product($product_name);
+
+    $vars->{'product'} = $product;
+    $vars->{'token'} = issue_session_token('edit_group_controls');
+
+    $template->process("admin/products/groupcontrol/edit.html.tmpl", $vars)
+        || ThrowTemplateError($template->error());
+    exit;
+}
+
+#
+# action='updategroupcontrols' -> update product group controls
 #
 
 if ($action eq 'updategroupcontrols') {
@@ -308,10 +355,9 @@ if ($action eq 'updategroupcontrols') {
                    {'Slice' => {}}, $product->id);
         }
 
-#
-# return the mandatory groups which need to have bug entries added to the bug_group_map
-# and the corresponding bug count
-#
+        # return the mandatory groups which need to have bug entries
+        # added to the bug_group_map and the corresponding bug count
+
         my $mandatory_groups;
         if (@now_mandatory) {
             $mandatory_groups = $dbh->selectall_arrayref(
@@ -339,302 +385,35 @@ if ($action eq 'updategroupcontrols') {
             $vars->{'mandatory_groups'} = $mandatory_groups;
             $template->process("admin/products/groupcontrol/confirm-edit.html.tmpl", $vars)
                 || ThrowTemplateError($template->error());
-            exit;                
+            exit;
         }
     }
 
-    my $groups = $dbh->selectall_arrayref('SELECT id, name FROM groups
-                                           WHERE isbuggroup != 0
-                                           AND isactive != 0');
+    my $groups = Bugzilla::Group->match({isactive => 1, isbuggroup => 1});
     foreach my $group (@$groups) {
-        my ($groupid, $groupname) = @$group;
-        my $newmembercontrol = $cgi->param("membercontrol_$groupid") || 0;
-        my $newothercontrol = $cgi->param("othercontrol_$groupid") || 0;
-        #  Legality of control combination is a function of
-        #  membercontrol\othercontrol
-        #                 NA SH DE MA
-        #              NA  +  -  -  -
-        #              SH  +  +  +  +
-        #              DE  +  -  +  +
-        #              MA  -  -  -  +
-        unless (($newmembercontrol == $newothercontrol)
-              || ($newmembercontrol == CONTROLMAPSHOWN)
-              || (($newmembercontrol == CONTROLMAPDEFAULT)
-               && ($newothercontrol != CONTROLMAPSHOWN))) {
-            ThrowUserError('illegal_group_control_combination',
-                            {groupname => $groupname});
-        }
-    }
-    $dbh->bz_start_transaction();
-
-    my $sth_Insert = $dbh->prepare('INSERT INTO group_control_map
-                                    (group_id, product_id, entry, membercontrol,
-                                     othercontrol, canedit, editcomponents,
-                                     canconfirm, editbugs)
-                                    VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)');
-
-    my $sth_Update = $dbh->prepare('UPDATE group_control_map
-                                       SET entry = ?, membercontrol = ?,
-                                           othercontrol = ?, canedit = ?,
-                                           editcomponents = ?, canconfirm = ?,
-                                           editbugs = ?
-                                     WHERE group_id = ? AND product_id = ?');
-
-    my $sth_Delete = $dbh->prepare('DELETE FROM group_control_map
-                                     WHERE group_id = ? AND product_id = ?');
-
-    $groups = $dbh->selectall_arrayref('SELECT id, name, entry, membercontrol,
-                                               othercontrol, canedit,
-                                               editcomponents, canconfirm, editbugs
-                                          FROM groups
-                                     LEFT JOIN group_control_map
-                                            ON group_control_map.group_id = id
-                                           AND product_id = ?
-                                         WHERE isbuggroup != 0
-                                           AND isactive != 0',
-                                         undef, $product->id);
-
-    foreach my $group (@$groups) {
-        my ($groupid, $groupname, $entry, $membercontrol, $othercontrol,
-            $canedit, $editcomponents, $canconfirm, $editbugs) = @$group;
-        my $newentry = $cgi->param("entry_$groupid") || 0;
-        my $newmembercontrol = $cgi->param("membercontrol_$groupid") || 0;
-        my $newothercontrol = $cgi->param("othercontrol_$groupid") || 0;
-        my $newcanedit = $cgi->param("canedit_$groupid") || 0;
-        my $new_editcomponents = $cgi->param("editcomponents_$groupid") || 0;
-        my $new_canconfirm = $cgi->param("canconfirm_$groupid") || 0;
-        my $new_editbugs = $cgi->param("editbugs_$groupid") || 0;
-
-        my $oldentry = $entry;
-        # Set undefined values to 0.
-        $entry ||= 0;
-        $membercontrol ||= 0;
-        $othercontrol ||= 0;
-        $canedit ||= 0;
-        $editcomponents ||= 0;
-        $canconfirm ||= 0;
-        $editbugs ||= 0;
-
-        # We use them in placeholders only. So it's safe to detaint them.
-        detaint_natural($newentry);
-        detaint_natural($newothercontrol);
-        detaint_natural($newmembercontrol);
-        detaint_natural($newcanedit);
-        detaint_natural($new_editcomponents);
-        detaint_natural($new_canconfirm);
-        detaint_natural($new_editbugs);
-
-        if (!defined($oldentry)
-            && ($newentry || $newmembercontrol || $newcanedit
-                || $new_editcomponents || $new_canconfirm || $new_editbugs))
-        {
-            $sth_Insert->execute($groupid, $product->id, $newentry,
-                                 $newmembercontrol, $newothercontrol, $newcanedit,
-                                 $new_editcomponents, $new_canconfirm, $new_editbugs);
-        }
-        elsif (($newentry != $entry)
-               || ($newmembercontrol != $membercontrol)
-               || ($newothercontrol != $othercontrol)
-               || ($newcanedit != $canedit)
-               || ($new_editcomponents != $editcomponents)
-               || ($new_canconfirm != $canconfirm)
-               || ($new_editbugs != $editbugs))
-        {
-            $sth_Update->execute($newentry, $newmembercontrol, $newothercontrol,
-                                 $newcanedit, $new_editcomponents, $new_canconfirm,
-                                 $new_editbugs, $groupid, $product->id);
-        }
-
-        if (!$newentry && !$newmembercontrol && !$newothercontrol
-            && !$newcanedit && !$new_editcomponents && !$new_canconfirm
-            && !$new_editbugs)
-        {
-            $sth_Delete->execute($groupid, $product->id);
-        }
-    }
-
-    my $sth_Select = $dbh->prepare(
-                     'SELECT bugs.bug_id,
-                   CASE WHEN (lastdiffed >= delta_ts) THEN 1 ELSE 0 END
-                        FROM bugs
-                  INNER JOIN bug_group_map
-                          ON bug_group_map.bug_id = bugs.bug_id
-                       WHERE group_id = ?
-                         AND bugs.product_id = ?
-                    ORDER BY bugs.bug_id');
-
-    my $sth_Select2 = $dbh->prepare('SELECT name, NOW() FROM groups WHERE id = ?');
-
-    $sth_Update = $dbh->prepare('UPDATE bugs SET delta_ts = ? WHERE bug_id = ?');
-
-    my $sth_Update2 = $dbh->prepare('UPDATE bugs SET delta_ts = ?, lastdiffed = ?
-                                     WHERE bug_id = ?');
-
-    $sth_Delete = $dbh->prepare('DELETE FROM bug_group_map
-                                 WHERE bug_id = ? AND group_id = ?');
-
-    my @removed_na;
-    foreach my $groupid (@now_na) {
-        my $count = 0;
-        my $bugs = $dbh->selectall_arrayref($sth_Select, undef,
-                                            ($groupid, $product->id));
-
-        my ($removed, $timestamp) =
-            $dbh->selectrow_array($sth_Select2, undef, $groupid);
-
-        foreach my $bug (@$bugs) {
-            my ($bugid, $mailiscurrent) = @$bug;
-            $sth_Delete->execute($bugid, $groupid);
-
-            LogActivityEntry($bugid, "bug_group", $removed, "",
-                             $whoid, $timestamp);
-
-            if ($mailiscurrent) {
-                $sth_Update2->execute($timestamp, $timestamp, $bugid);
-            }
-            else {
-                $sth_Update->execute($timestamp, $bugid);
-            }
-            $count++;
-        }
-        my %group = (name => $removed, bug_count => $count);
-
-        push(@removed_na, \%group);
-    }
-
-    $sth_Select = $dbh->prepare(
-                  'SELECT bugs.bug_id,
-                CASE WHEN (lastdiffed >= delta_ts) THEN 1 ELSE 0 END
-                     FROM bugs
-                LEFT JOIN bug_group_map
-                       ON bug_group_map.bug_id = bugs.bug_id
-                      AND group_id = ?
-                    WHERE bugs.product_id = ?
-                      AND bug_group_map.bug_id IS NULL
-                 ORDER BY bugs.bug_id');
-
-    $sth_Insert = $dbh->prepare('INSERT INTO bug_group_map
-                                 (bug_id, group_id) VALUES (?, ?)');
-
-    my @added_mandatory;
-    foreach my $groupid (@now_mandatory) {
-        my $count = 0;
-        my $bugs = $dbh->selectall_arrayref($sth_Select, undef,
-                                            ($groupid, $product->id));
-
-        my ($added, $timestamp) =
-            $dbh->selectrow_array($sth_Select2, undef, $groupid);
-
-        foreach my $bug (@$bugs) {
-            my ($bugid, $mailiscurrent) = @$bug;
-            $sth_Insert->execute($bugid, $groupid);
-
-            LogActivityEntry($bugid, "bug_group", "", $added,
-                             $whoid, $timestamp);
-
-            if ($mailiscurrent) {
-                $sth_Update2->execute($timestamp, $timestamp, $bugid);
-            }
-            else {
-                $sth_Update->execute($timestamp, $bugid);
-            }
-            $count++;
-        }
-        my %group = (name => $added, bug_count => $count);
-
-        push(@added_mandatory, \%group);
+        my $group_id = $group->id;
+        $product->set_group_controls($group,
+                                     {entry          => scalar $cgi->param("entry_$group_id") || 0,
+                                      membercontrol  => scalar $cgi->param("membercontrol_$group_id") || CONTROLMAPNA,
+                                      othercontrol   => scalar $cgi->param("othercontrol_$group_id") || CONTROLMAPNA,
+                                      canedit        => scalar $cgi->param("canedit_$group_id") || 0,
+                                      editcomponents => scalar $cgi->param("editcomponents_$group_id") || 0,
+                                      editbugs       => scalar $cgi->param("editbugs_$group_id") || 0,
+                                      canconfirm     => scalar $cgi->param("canconfirm_$group_id") || 0});
     }
-    $dbh->bz_commit_transaction();
+    my $changes = $product->update;
 
     delete_token($token);
 
-    $vars->{'removed_na'} = \@removed_na;
-    $vars->{'added_mandatory'} = \@added_mandatory;
-    $vars->{'product'} = $product;
-
-    $template->process("admin/products/groupcontrol/updated.html.tmpl", $vars)
-        || ThrowTemplateError($template->error());
-    exit;
-}
-
-#
-# action='update' -> update the product
-#
-if ($action eq 'update') {
-    check_token_data($token, 'edit_product');
-    my $product_old_name = trim($cgi->param('product_old_name') || '');
-    my $product = $user->check_can_admin_product($product_old_name);
-
-    $product->set_name($product_name);
-    $product->set_description(scalar $cgi->param('description'));
-    $product->set_default_milestone(scalar $cgi->param('defaultmilestone'));
-    $product->set_milestone_url(scalar $cgi->param('milestoneurl'));
-    $product->set_disallow_new(scalar $cgi->param('disallownew'));
-    $product->set_votes_per_user(scalar $cgi->param('votesperuser'));
-    $product->set_votes_per_bug(scalar $cgi->param('maxvotesperbug'));
-    $product->set_votes_to_confirm(scalar $cgi->param('votestoconfirm'));
-
-    my $changes = $product->update();
-
-    delete_token($token);
-
-    if (Bugzilla->params->{'useclassification'}) {
-        $vars->{'classification'} = new Bugzilla::Classification($product->classification_id);
-    }
     $vars->{'product'} = $product;
     $vars->{'changes'} = $changes;
 
-    $template->process("admin/products/updated.html.tmpl", $vars)
+    $template->process("admin/products/groupcontrol/updated.html.tmpl", $vars)
         || ThrowTemplateError($template->error());
     exit;
 }
 
 #
-# action='editgroupcontrols' -> update product group controls
-#
-
-if ($action eq 'editgroupcontrols') {
-    my $product = $user->check_can_admin_product($product_name);
-
-    # Display a group if it is either enabled or has bugs for this product.
-    my $groups = $dbh->selectall_arrayref(
-        'SELECT id, name, entry, membercontrol, othercontrol, canedit,
-                editcomponents, editbugs, canconfirm,
-                isactive, COUNT(bugs.bug_id) AS bugcount
-           FROM groups
-      LEFT JOIN group_control_map
-             ON group_control_map.group_id = groups.id
-            AND group_control_map.product_id = ?
-      LEFT JOIN bug_group_map
-             ON bug_group_map.group_id = groups.id
-      LEFT JOIN bugs
-             ON bugs.bug_id = bug_group_map.bug_id
-            AND bugs.product_id = ?
-          WHERE isbuggroup != 0
-            AND (isactive != 0 OR entry IS NOT NULL OR bugs.bug_id IS NOT NULL) ' .
-           $dbh->sql_group_by('name', 'id, entry, membercontrol,
-                              othercontrol, canedit, isactive,
-                              editcomponents, canconfirm, editbugs'),
-        {'Slice' => {}}, ($product->id, $product->id));
-
-    $vars->{'product'} = $product;
-    $vars->{'groups'} = $groups;
-    $vars->{'token'} = issue_session_token('edit_group_controls');
-
-    $vars->{'const'} = {
-        'CONTROLMAPNA' => CONTROLMAPNA,
-        'CONTROLMAPSHOWN' => CONTROLMAPSHOWN,
-        'CONTROLMAPDEFAULT' => CONTROLMAPDEFAULT,
-        'CONTROLMAPMANDATORY' => CONTROLMAPMANDATORY,
-    };
-
-    $template->process("admin/products/groupcontrol/edit.html.tmpl", $vars)
-        || ThrowTemplateError($template->error());
-    exit;                
-}
-
-
-#
 # No valid action found
 #
 
diff --git a/template/en/default/admin/products/edit.html.tmpl b/template/en/default/admin/products/edit.html.tmpl
index a3d5089c3..e6480c453 100644
--- a/template/en/default/admin/products/edit.html.tmpl
+++ b/template/en/default/admin/products/edit.html.tmpl
@@ -107,7 +107,7 @@ versions:</a>
       </th>
       <td>
         [% IF product.group_controls.size %]
-          [% FOREACH g = product.group_controls.values %]
+          [% FOREACH g = product.group_controls.values.sort("name") %]
             <b>[% g.group.name FILTER html %]:</b>&nbsp;
             [% IF g.group.isactive %]
               [% group_control.${g.membercontrol} FILTER html %]/
diff --git a/template/en/default/admin/products/groupcontrol/edit.html.tmpl b/template/en/default/admin/products/groupcontrol/edit.html.tmpl
index c793ff683..8c634ebfe 100644
--- a/template/en/default/admin/products/groupcontrol/edit.html.tmpl
+++ b/template/en/default/admin/products/groupcontrol/edit.html.tmpl
@@ -31,8 +31,6 @@
   <input type="hidden" name="action" value="updategroupcontrols">
   <input type="hidden" name="product" value="[% product.name FILTER html %]">
   <input type="hidden" name="token" value="[% token FILTER html %]">
-  <input type="hidden" name="classification" 
-         value="[% classification.name FILTER html %]">
 
   <table id="form" cellspacing="0" cellpadding="4" border="1">
     <tr bgcolor="#6666ff">
@@ -46,23 +44,23 @@
       <th>editbugs</th>
       <th>[% terms.Bugs %]</th>
     </tr>
-    [% FOREACH group = groups %]
-      [% IF group.isactive == 0 AND group.bugcount > 0 %]
+    [% FOREACH group = product.group_controls(1).values.sort("name") %]
+      [% IF !group.group.isactive AND group.bug_count %]
         <tr bgcolor="#bbbbbb">
           <td>
-            [% group.name FILTER html %]
+            [% group.group.name FILTER html %]
           </td>
           <td align="center" colspan=7>
             Disabled
           </td>
           <td>
-            [% group.bugcount %]
+            [% group.bug_count FILTER html %]
           </td>
         <tr>
-      [% ELSIF group.isactive != 0 %]
+      [% ELSIF group.group.is_active %]
         <tr>
           <td>
-            [% group.name FILTER html %]
+            [% group.group.name FILTER html %]
           </td>
           <td>
             <input type=checkbox value=1 name=entry_[% group.id %]
@@ -70,48 +68,48 @@
           </td>
           <td>
             <select name="membercontrol_[% group.id %]">
-              <option value=[% const.CONTROLMAPNA %]
+              <option value=[% constants.CONTROLMAPNA %]
                 [% " selected=\"selected\""
-                  IF group.membercontrol == const.CONTROLMAPNA %]
+                  IF group.membercontrol == constants.CONTROLMAPNA %]
                 >NA
               </option>
-              <option value=[% const.CONTROLMAPSHOWN %]
+              <option value=[% constants.CONTROLMAPSHOWN %]
                 [% " selected=\"selected\""
-                  IF group.membercontrol == const.CONTROLMAPSHOWN %]
+                  IF group.membercontrol == constants.CONTROLMAPSHOWN %]
                 >Shown
               </option>
-              <option value=[% const.CONTROLMAPDEFAULT %]
+              <option value=[% constants.CONTROLMAPDEFAULT %]
                 [% " selected=\"selected\""
-                  IF group.membercontrol == const.CONTROLMAPDEFAULT %]
+                  IF group.membercontrol == constants.CONTROLMAPDEFAULT %]
                 >Default
               </option>
-              <option value=[% const.CONTROLMAPMANDATORY %]
+              <option value=[% constants.CONTROLMAPMANDATORY %]
                 [% " selected=\"selected\""
-                  IF group.membercontrol == const.CONTROLMAPMANDATORY %]
+                  IF group.membercontrol == constants.CONTROLMAPMANDATORY %]
                 >Mandatory
               </option>
             </select>
           </td>
           <td>
             <select name="othercontrol_[% group.id %]">
-              <option value=[% const.CONTROLMAPNA %]
+              <option value=[% constants.CONTROLMAPNA %]
                 [% " selected=\"selected\""
-                  IF group.othercontrol == const.CONTROLMAPNA %]
+                  IF group.othercontrol == constants.CONTROLMAPNA %]
                 >NA
               </option>
-              <option value=[% const.CONTROLMAPSHOWN %]
+              <option value=[% constants.CONTROLMAPSHOWN %]
                 [% " selected=\"selected\""
-                  IF group.othercontrol == const.CONTROLMAPSHOWN %]
+                  IF group.othercontrol == constants.CONTROLMAPSHOWN %]
                 >Shown
               </option>
-              <option value=[% const.CONTROLMAPDEFAULT %]
+              <option value=[% constants.CONTROLMAPDEFAULT %]
                 [% " selected=\"selected\""
-                  IF group.othercontrol == const.CONTROLMAPDEFAULT %]
+                  IF group.othercontrol == constants.CONTROLMAPDEFAULT %]
                 >Default
               </option>
-              <option value=[% const.CONTROLMAPMANDATORY %]
+              <option value=[% constants.CONTROLMAPMANDATORY %]
                 [% " selected=\"selected\""
-                  IF group.othercontrol == const.CONTROLMAPMANDATORY %]
+                  IF group.othercontrol == constants.CONTROLMAPMANDATORY %]
                 >Mandatory
               </option>
             </select>
@@ -133,7 +131,7 @@
             [% " checked=\"checked\"" IF group.editbugs %]>
           </td>
           <td>
-            [% group.bugcount %]
+            [% group.bug_count || 0 FILTER html %]
           </td>
         </tr>
       [% END %]
diff --git a/template/en/default/admin/products/groupcontrol/updated.html.tmpl b/template/en/default/admin/products/groupcontrol/updated.html.tmpl
index 52456a473..2f59cae68 100644
--- a/template/en/default/admin/products/groupcontrol/updated.html.tmpl
+++ b/template/en/default/admin/products/groupcontrol/updated.html.tmpl
@@ -15,10 +15,8 @@
   #%]
 
 [%# INTERFACE:
-  #      removed_na: array of hashes; groups not applicable for the product.  
-  # added_mandatory: array of hashes; groups mandatory for the product.  
-  #  classification: Bugzilla::Classification object;  product classification.
-  #         product: Bugzilla::Product object; the product.
+  # product: Bugzilla::Product object; the product.
+  # changes: Hashref with changes made to the product group controls.
   #%]
 
 [% title = BLOCK %]
@@ -29,16 +27,16 @@
   title = title
 %]
 <p>
-[% IF removed_na.size > 0 %]
-  [% FOREACH g = removed_na %]
+[% IF changes.group_controls.now_na.size %]
+  [% FOREACH g = changes.group_controls.now_na %]
     Removing [% terms.bugs %] from group '[% g.name FILTER html %]' which
     no longer applies to this product<p>
     [% g.bug_count FILTER html %] [%+ terms.bugs %] removed<p>
   [% END %]
 [% END %]
 
-[% IF added_mandatory.size > 0 %]
-  [% FOREACH g = added_mandatory %]
+[% IF changes.group_controls.now_mandatory.size %]
+  [% FOREACH g = changes.group_controls.now_mandatory %]
     Adding [% terms.bugs %] to group '[% g.name FILTER html %]' which is 
     mandatory for this product<p>
     [% g.bug_count FILTER html %] [%+ terms.bugs %] added<p>
diff --git a/template/en/default/filterexceptions.pl b/template/en/default/filterexceptions.pl
index 056341b53..361a1f469 100644
--- a/template/en/default/filterexceptions.pl
+++ b/template/en/default/filterexceptions.pl
@@ -462,12 +462,11 @@
 ],
 
 'admin/products/groupcontrol/edit.html.tmpl' => [
-  'group.bugcount', 
-  'group.id', 
-  'const.CONTROLMAPNA', 
-  'const.CONTROLMAPSHOWN', 
-  'const.CONTROLMAPDEFAULT', 
-  'const.CONTROLMAPMANDATORY', 
+  'group.id',
+  'constants.CONTROLMAPNA', 
+  'constants.CONTROLMAPSHOWN',
+  'constants.CONTROLMAPDEFAULT',
+  'constants.CONTROLMAPMANDATORY',
 ],
 
 'admin/products/list.html.tmpl' => [
diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl
index 80645a851..b93b92efd 100644
--- a/template/en/default/global/code-error.html.tmpl
+++ b/template/en/default/global/code-error.html.tmpl
@@ -324,6 +324,16 @@
     a <code>[% param FILTER html %]</code> argument, and that
     argument was not set.
 
+  [% ELSIF error == "product_empty_group_controls" %]
+    [% title = "Missing Group Controls" %]
+    New settings must be defined to edit group controls for
+    the [% group.name FILTER html %] group.
+
+  [% ELSIF error == "product_illegal_group_control" %]
+    [% title = "Illegal Group Control" %]
+    '[% value FILTER html %]' is not a legal value for
+    the '[% field FILTER html %]' field.
+
   [% ELSIF error == "protection_violation" %]
     The function <code>[% function FILTER html %]</code> was called
 
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index b04c9e7dc..c8df08e5e 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -1236,6 +1236,11 @@
     [% title = "Specified Product Does Not Exist" %]
     The product '[% product FILTER html %]' does not exist.
 
+  [% ELSIF error == "product_illegal_group" %]
+    [% title = "Illegal Group" %]
+    [% group.name FILTER html %] is not an active [% terms.bug %] group
+    and so you cannot edit group controls for it.
+
   [% ELSIF error == "product_illegal_votes" %]
     [% title = "Votes Must Be Non-negative" %]
     [% admindocslinks = {'voting.html' => 'Setting up the voting feature'} %]