Bug 354627: Improve the UI for adding/removing inheritance in editgroups.cgi

Patch By Max Kanat-Alexander <mkanat@bugzilla.org r=LpSolit, a=LpSolit

6 files changed, 601 insertions, 417 deletions

diff --git a/Bugzilla/Group.pm b/Bugzilla/Group.pm
index c80d2333c..9e5a601c7 100644
--- a/Bugzilla/Group.pm
+++ b/Bugzilla/Group.pm
@@ -53,11 +53,23 @@ use constant VALIDATORS => {
     name        => \&_check_name,
     description => \&_check_description,
     userregexp  => \&_check_user_regexp,
+    isactive    => \&_check_is_active,
     isbuggroup  => \&_check_is_bug_group,
 };
 
 use constant REQUIRED_CREATE_FIELDS => qw(name description isbuggroup);
 
+use constant UPDATE_COLUMNS => qw(
+    name
+    description
+    userregexp
+    isactive
+);
+
+# Parameters that are lists of groups.
+use constant GROUP_PARAMS => qw(chartgroup insidergroup timetrackinggroup
+                                querysharegroup);
+
 ###############################
 ####      Accessors      ######
 ###############################
@@ -67,10 +79,112 @@ sub is_bug_group { return $_[0]->{'isbuggroup'};   }
 sub user_regexp  { return $_[0]->{'userregexp'};   }
 sub is_active    { return $_[0]->{'isactive'};     }
 
+sub members_direct {
+    my ($self) = @_;
+    return $self->{members_direct} if defined $self->{members_direct};
+    my $dbh = Bugzilla->dbh;
+    my $user_ids = $dbh->selectcol_arrayref(
+        "SELECT user_group_map.user_id
+           FROM user_group_map
+          WHERE user_group_map.group_id = ?
+                AND grant_type = " . GRANT_DIRECT . "
+                AND isbless = 0", undef, $self->id);
+    require Bugzilla::User;
+    $self->{members_direct} = Bugzilla::User->new_from_list($user_ids);
+    return $self->{members_direct};
+}
+
+sub grant_direct {
+    my ($self, $type) = @_;
+    $self->{grant_direct} ||= {};
+    return $self->{grant_direct}->{$type} 
+        if defined $self->{members_direct}->{$type};
+    my $dbh = Bugzilla->dbh;
+
+    my $ids = $dbh->selectcol_arrayref(
+      "SELECT member_id FROM group_group_map
+        WHERE grantor_id = ? AND grant_type = $type", 
+      undef, $self->id) || [];
+
+    $self->{grant_direct}->{$type} = $self->new_from_list($ids);
+    return $self->{grant_direct}->{$type};
+}
+
+sub granted_by_direct {
+    my ($self, $type) = @_;
+    $self->{granted_by_direct} ||= {};
+    return $self->{granted_by_direct}->{$type}
+         if defined $self->{granted_by_direct}->{$type};
+    my $dbh = Bugzilla->dbh;
+
+    my $ids = $dbh->selectcol_arrayref(
+      "SELECT grantor_id FROM group_group_map
+        WHERE member_id = ? AND grant_type = $type",
+      undef, $self->id) || [];
+
+    $self->{granted_by_direct}->{$type} = $self->new_from_list($ids);
+    return $self->{granted_by_direct}->{$type};
+}
+
 ###############################
 ####        Methods        ####
 ###############################
 
+sub set_description { $_[0]->set('description', $_[1]); }
+sub set_is_active   { $_[0]->set('isactive', $_[1]);    }
+sub set_name        { $_[0]->set('name', $_[1]);        }
+sub set_user_regexp { $_[0]->set('userregexp', $_[1]);  }
+
+sub update {
+    my $self = shift;
+    my $changes = $self->SUPER::update(@_);
+
+    if (exists $changes->{name}) {
+        my ($old_name, $new_name) = @{$changes->{name}};
+        my $update_params;
+        foreach my $group (GROUP_PARAMS) {
+            if ($old_name eq Bugzilla->params->{$group}) {
+                SetParam($group, $new_name);
+                $update_params = 1;
+            }
+        }
+        write_params() if $update_params;
+    }
+
+    # If we've changed this group to be active, fix any Mandatory groups.
+    $self->_enforce_mandatory if (exists $changes->{isactive} 
+                                  && $changes->{isactive}->[1]);
+
+    $self->_rederive_regexp() if exists $changes->{userregexp};
+    return $changes;
+}
+
+# Add missing entries in bug_group_map for bugs created while
+# a mandatory group was disabled and which is now enabled again.
+sub _enforce_mandatory {
+    my ($self) = @_;
+    my $dbh = Bugzilla->dbh;
+    my $gid = $self->id;
+
+    my $bug_ids =
+      $dbh->selectcol_arrayref('SELECT bugs.bug_id
+                                  FROM bugs
+                            INNER JOIN group_control_map
+                                    ON group_control_map.product_id = bugs.product_id
+                             LEFT JOIN bug_group_map
+                                    ON bug_group_map.bug_id = bugs.bug_id
+                                   AND bug_group_map.group_id = group_control_map.group_id
+                                 WHERE group_control_map.group_id = ?
+                                   AND group_control_map.membercontrol = ?
+                                   AND bug_group_map.group_id IS NULL',
+                                 undef, ($gid, CONTROLMAPMANDATORY));
+
+    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);
+    }
+}
+
 sub is_active_bug_group {
     my $self = shift;
     return $self->is_active && $self->is_bug_group;
@@ -183,8 +297,11 @@ sub _check_name {
     my ($invocant, $name) = @_;
     $name = trim($name);
     $name || ThrowUserError("empty_group_name");
-    my $exists = new Bugzilla::Group({name => $name });
-    ThrowUserError("group_exists", { name => $name }) if $exists;
+    # If we're creating a Group or changing the name...
+    if (!ref($invocant) || $invocant->name ne $name) {
+        my $exists = new Bugzilla::Group({name => $name });
+        ThrowUserError("group_exists", { name => $name }) if $exists;
+    }
     return $name;
 }
 
@@ -202,9 +319,11 @@ sub _check_user_regexp {
     return $regex;
 }
 
+sub _check_is_active { return $_[1] ? 1 : 0; }
 sub _check_is_bug_group {
     return $_[1] ? 1 : 0;
 }
+
 1;
 
 __END__
diff --git a/editgroups.cgi b/editgroups.cgi
index 0c49db698..5e2a3baf6 100755
--- a/editgroups.cgi
+++ b/editgroups.cgi
@@ -57,34 +57,6 @@ $user->in_group('creategroups')
 my $action = trim($cgi->param('action') || '');
 my $token  = $cgi->param('token');
 
-# Add missing entries in bug_group_map for bugs created while
-# a mandatory group was disabled and which is now enabled again.
-sub fix_bug_permissions {
-    my $gid = shift;
-    my $dbh = Bugzilla->dbh;
-
-    detaint_natural($gid);
-    return unless $gid;
-
-    my $bug_ids =
-      $dbh->selectcol_arrayref('SELECT bugs.bug_id
-                                  FROM bugs
-                            INNER JOIN group_control_map
-                                    ON group_control_map.product_id = bugs.product_id
-                             LEFT JOIN bug_group_map
-                                    ON bug_group_map.bug_id = bugs.bug_id
-                                   AND bug_group_map.group_id = group_control_map.group_id
-                                 WHERE group_control_map.group_id = ?
-                                   AND group_control_map.membercontrol = ?
-                                   AND bug_group_map.group_id IS NULL',
-                                 undef, ($gid, CONTROLMAPMANDATORY));
-
-    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);
-    }
-}
-
 # CheckGroupID checks that a positive integer is given and is
 # actually a valid group ID. If all tests are successful, the
 # trimmed group ID is returned.
@@ -148,6 +120,66 @@ sub CheckGroupRegexp {
     return $regexp;
 }
 
+# A helper for displaying the edit.html.tmpl template.
+sub get_current_and_available {
+    my ($group, $vars) = @_;
+
+    my @all_groups         = Bugzilla::Group->get_all;
+    my @members_current    = @{$group->grant_direct(GROUP_MEMBERSHIP)};
+    my @member_of_current  = @{$group->granted_by_direct(GROUP_MEMBERSHIP)};
+    my @bless_from_current = @{$group->grant_direct(GROUP_BLESS)};
+    my @bless_to_current   = @{$group->granted_by_direct(GROUP_BLESS)};
+    my (@visible_from_current, @visible_to_me_current);
+    if (Bugzilla->params->{'usevisibilitygroups'}) {
+        @visible_from_current  = @{$group->grant_direct(GROUP_VISIBLE)};
+        @visible_to_me_current = @{$group->granted_by_direct(GROUP_VISIBLE)};
+    }
+
+    # Figure out what groups are not currently a member of this group,
+    # and what groups this group is not currently a member of.
+    my (@members_available, @member_of_available,
+        @bless_from_available, @bless_to_available,
+        @visible_from_available, @visible_to_me_available);
+    foreach my $group_option (@all_groups) {
+        if (Bugzilla->params->{'usevisibilitygroups'}) {
+            push(@visible_from_available, $group_option)
+                if !grep($_->id == $group_option->id, @visible_from_current);
+            push(@visible_to_me_available, $group_option)
+                if !grep($_->id == $group_option->id, @visible_to_me_current);
+        }
+
+        # The group itself should never show up in the bless or 
+        # membership lists.
+        next if $group_option->id == $group->id;
+
+        push(@members_available, $group_option)
+            if !grep($_->id == $group_option->id, @members_current);
+        push(@member_of_available, $group_option)
+            if !grep($_->id == $group_option->id, @member_of_current);
+        push(@bless_from_available, $group_option)
+            if !grep($_->id == $group_option->id, @bless_from_current);
+        push(@bless_to_available, $group_option)
+           if !grep($_->id == $group_option->id, @bless_to_current);
+    }
+
+    $vars->{'members_current'}     = \@members_current;
+    $vars->{'members_available'}   = \@members_available;
+    $vars->{'member_of_current'}   = \@member_of_current;
+    $vars->{'member_of_available'} = \@member_of_available;
+
+    $vars->{'bless_from_current'}   = \@bless_from_current;
+    $vars->{'bless_from_available'} = \@bless_from_available;
+    $vars->{'bless_to_current'}     = \@bless_to_current;
+    $vars->{'bless_to_available'}   = \@bless_to_available;
+
+    if (Bugzilla->params->{'usevisibilitygroups'}) {
+        $vars->{'visible_from_current'}    = \@visible_from_current;
+        $vars->{'visible_from_available'}  = \@visible_from_available;
+        $vars->{'visible_to_me_current'}   = \@visible_to_me_current;
+        $vars->{'visible_to_me_available'} = \@visible_to_me_available;
+    }
+}
+
 # If no action is specified, get a list of all groups available.
 
 unless ($action) {
@@ -169,62 +201,10 @@ unless ($action) {
 if ($action eq 'changeform') {
     # Check that an existing group ID is given
     my $group_id = CheckGroupID($cgi->param('group'));
-    my ($name, $description, $regexp, $isactive, $isbuggroup) =
-        $dbh->selectrow_array("SELECT name, description, userregexp, " .
-                              "isactive, isbuggroup " .
-                              "FROM groups WHERE id = ?", undef, $group_id);
-
-    # For each group, we use left joins to establish the existence of
-    # a record making that group a member of this group
-    # and the existence of a record permitting that group to bless
-    # this one
-
-    my @groups;
-    my $group_list =
-      $dbh->selectall_arrayref('SELECT groups.id, groups.name, groups.description,
-                                       CASE WHEN group_group_map.member_id IS NOT NULL
-                                            THEN 1 ELSE 0 END,
-                                       CASE WHEN B.member_id IS NOT NULL
-                                            THEN 1 ELSE 0 END,
-                                       CASE WHEN C.member_id IS NOT NULL
-                                            THEN 1 ELSE 0 END
-                                  FROM groups
-                                  LEFT JOIN group_group_map
-                                    ON group_group_map.member_id = groups.id
-                                   AND group_group_map.grantor_id = ?
-                                   AND group_group_map.grant_type = ?
-                                  LEFT JOIN group_group_map as B
-                                    ON B.member_id = groups.id
-                                   AND B.grantor_id = ?
-                                   AND B.grant_type = ?
-                                  LEFT JOIN group_group_map as C
-                                    ON C.member_id = groups.id
-                                   AND C.grantor_id = ?
-                                   AND C.grant_type = ?
-                                 ORDER by name',
-                                undef, ($group_id, GROUP_MEMBERSHIP,
-                                        $group_id, GROUP_BLESS,
-                                        $group_id, GROUP_VISIBLE));
-
-    foreach (@$group_list) {
-        my ($grpid, $grpnam, $grpdesc, $grpmember, $blessmember, $membercansee) = @$_;
-        my $group = {};
-        $group->{'grpid'}       = $grpid;
-        $group->{'grpnam'}      = $grpnam;
-        $group->{'grpdesc'}     = $grpdesc;
-        $group->{'grpmember'}   = $grpmember;
-        $group->{'blessmember'} = $blessmember;
-        $group->{'membercansee'}= $membercansee;
-        push(@groups, $group);
-    }
+    my $group = new Bugzilla::Group($group_id);
 
-    $vars->{'group_id'}    = $group_id;
-    $vars->{'name'}        = $name;
-    $vars->{'description'} = $description;
-    $vars->{'regexp'}      = $regexp;
-    $vars->{'isactive'}    = $isactive;
-    $vars->{'isbuggroup'}  = $isbuggroup;
-    $vars->{'groups'}      = \@groups;
+    get_current_and_available($group, $vars);
+    $vars->{'group'} = $group;
     $vars->{'token'}       = issue_session_token('edit_group');
 
     print $cgi->header();
@@ -481,82 +461,61 @@ if ($action eq 'delete') {
 
 if ($action eq 'postchanges') {
     check_token_data($token, 'edit_group');
-    # ZLL: Bug 181589: we need to have something to remove explicitly listed users from
-    # groups in order for the conversion to 2.18 groups to work
-    my $action;
-
-    if ($cgi->param('remove_explicit_members')) {
-        $action = 1;
-    } elsif ($cgi->param('remove_explicit_members_regexp')) {
-        $action = 2;
-    } else {
-        $action = 3;
-    }
-    
-    my ($gid, $chgs, $name, $regexp) = doGroupChanges();
-    
-    $vars->{'action'}  = $action;
-    $vars->{'changes'} = $chgs;
-    $vars->{'gid'}     = $gid;
-    $vars->{'name'}    = $name;
-    if ($action == 2) {
-        $vars->{'regexp'} = $regexp;
-    }
+    my $changes = doGroupChanges();
     delete_token($token);
 
+    my $group = new Bugzilla::Group($cgi->param('group_id'));
+    get_current_and_available($group, $vars);
+    $vars->{'message'} = 'group_updated';
+    $vars->{'group'}   = $group;
+    $vars->{'changes'} = $changes;
+    $vars->{'token'} = issue_session_token('edit_group');
+
     print $cgi->header();
-    $template->process("admin/groups/change.html.tmpl", $vars)
+    $template->process("admin/groups/edit.html.tmpl", $vars)
       || ThrowTemplateError($template->error());
     exit;
 }
 
-if (($action eq 'remove_all_regexp') || ($action eq 'remove_all')) {
+if ($action eq 'confirm_remove') {
+    my $group = new Bugzilla::Group(CheckGroupID($cgi->param('group_id')));
+    $vars->{'group'} = $group;
+    $vars->{'regexp'} = CheckGroupRegexp($cgi->param('regexp'));
+    $vars->{'token'} = issue_session_token('remove_group_members');
+    $template->process('admin/groups/confirm-remove.html.tmpl', $vars)
+        || ThrowTemplateError($template->error());
+    exit;
+}
+
+if ($action eq 'remove_regexp') {
+    check_token_data($token, 'remove_group_members');
     # remove all explicit users from the group with
     # gid = $cgi->param('group') that match the regular expression
     # stored in the DB for that group or all of them period
 
-    my $gid = CheckGroupID($cgi->param('group'));
-
-    my ($name, $regexp) =
-      $dbh->selectrow_array('SELECT name, userregexp FROM groups
-                             WHERE id = ?', undef, $gid);
+    my $group  = new Bugzilla::Group(CheckGroupID($cgi->param('group_id')));
+    my $regexp = CheckGroupRegexp($cgi->param('regexp'));
 
     $dbh->bz_lock_tables('groups WRITE', 'profiles READ',
                          'user_group_map WRITE');
 
-    my $sth = $dbh->prepare("SELECT user_group_map.user_id, profiles.login_name
-                               FROM user_group_map
-                         INNER JOIN profiles
-                                 ON user_group_map.user_id = profiles.userid
-                              WHERE user_group_map.group_id = ?
-                                AND grant_type = ?
-                                AND isbless = 0");
-    $sth->execute($gid, GRANT_DIRECT);
-
-    my @users;
-    my $sth2 = $dbh->prepare("DELETE FROM user_group_map
-                              WHERE user_id = ?
-                              AND isbless = 0
-                              AND group_id = ?");
-
-    while ( my ($userid, $userlogin) = $sth->fetchrow_array() ) {
-        if ((($regexp =~ /\S/) && ($userlogin =~ m/$regexp/i))
-            || ($action eq 'remove_all'))
-        {
-            $sth2->execute($userid, $gid);
-
-            my $user = {};
-            $user->{'login'} = $userlogin;
-            push(@users, $user);
+    my $users = $group->members_direct();
+    my $sth_delete = $dbh->prepare(
+        "DELETE FROM user_group_map
+           WHERE user_id = ? AND isbless = 0 AND group_id = ?");
+
+    my @deleted;
+    foreach my $member (@$users) {
+        if ($regexp eq '' || $member->login =~ m/$regexp/i) {
+            $sth_delete->execute($member->id, $group->id);
+            push(@deleted, $member);
         }
     }
     $dbh->bz_unlock_tables();
 
-    $vars->{'users'}      = \@users;
-    $vars->{'name'}       = $name;
-    $vars->{'regexp'}     = $regexp;
-    $vars->{'remove_all'} = ($action eq 'remove_all');
-    $vars->{'gid'}        = $gid;
+    $vars->{'users'}  = \@deleted;
+    $vars->{'regexp'} = $regexp;
+    delete_token($token);
     
     print $cgi->header();
     $template->process("admin/groups/remove.html.tmpl", $vars)
@@ -586,116 +545,100 @@ sub doGroupChanges {
                          'priority READ', 'bug_severity READ', 'rep_platform READ',
                          'op_sys READ');
 
-    # Check that the given group ID and regular expression are valid.
-    # If tests are successful, trimmed values are returned by CheckGroup*.
-    my $gid = CheckGroupID($cgi->param('group'));
-    my $regexp = CheckGroupRegexp($cgi->param('regexp'));
+    # Check that the given group ID is valid and make a Group.
+    my $group = new Bugzilla::Group(CheckGroupID($cgi->param('group_id')));
+
+    if (defined $cgi->param('regexp')) {
+        $group->set_user_regexp($cgi->param('regexp'));
+    }
 
-    # The name and the description of system groups cannot be edited.
-    # We then need to know if the group being edited is a system group.
-    my $isbuggroup = $dbh->selectrow_array('SELECT isbuggroup FROM groups
-                                            WHERE id = ?', undef, $gid);
-    my $name;
-    my $desc;
-    my $isactive;
-    my $chgs = 0;
-
-    # We trust old values given by the template. If they are hacked
-    # in a way that some of the tests below become negative, the
-    # corresponding attributes are not updated in the DB, which does
-    # not hurt.
-    if ($isbuggroup) {
-        # Check that the group name and its description are valid
-        # and return trimmed values if tests are successful.
-        $name = CheckGroupName($cgi->param('name'), $gid);
-        $desc = CheckGroupDesc($cgi->param('desc'));
-        $isactive = $cgi->param('isactive') ? 1 : 0;
-
-        if ($name ne $cgi->param('oldname')) {
-            $chgs = 1;
-            $dbh->do('UPDATE groups SET name = ? WHERE id = ?',
-                      undef, ($name, $gid));
-            # If the group is used by some parameters, we have to update
-            # these parameters too.
-            my $update_params = 0;
-            foreach my $group (SPECIAL_GROUPS) {
-                if ($cgi->param('oldname') eq Bugzilla->params->{$group}) {
-                    SetParam($group, $name);
-                    $update_params = 1;
-                }
-            }
-            write_params() if $update_params;
+    if ($group->is_bug_group) {
+        if (defined $cgi->param('name')) {
+            $group->set_name($cgi->param('name'));
         }
-        if ($desc ne $cgi->param('olddesc')) {
-            $chgs = 1;
-            $dbh->do('UPDATE groups SET description = ? WHERE id = ?',
-                      undef, ($desc, $gid));
+        if (defined $cgi->param('desc')) {
+            $group->set_description($cgi->param('desc'));
         }
-        if ($isactive ne $cgi->param('oldisactive')) {
-            $chgs = 1;
-            $dbh->do('UPDATE groups SET isactive = ? WHERE id = ?',
-                      undef, ($isactive, $gid));
-            # If the group was mandatory for some products before
-            # we deactivated it and we now activate this group again,
-            # we have to add all bugs created while this group was
-            # disabled in bug_group_map to correctly protect them.
-            if ($isactive) { fix_bug_permissions($gid); }
+        # Only set isactive if we came from the right form.
+        if (defined $cgi->param('regexp')) {
+            $group->set_is_active($cgi->param('isactive'));
         }
     }
-    if ($regexp ne $cgi->param('oldregexp')) {
-        $chgs = 1;
-        $dbh->do('UPDATE groups SET userregexp = ? WHERE id = ?',
-                  undef, ($regexp, $gid));
-        Bugzilla::Group::RederiveRegexp($regexp, $gid);
+
+    my $changes = $group->update();
+
+    my $sth_insert = $dbh->prepare('INSERT INTO group_group_map
+                                    (member_id, grantor_id, grant_type)
+                                    VALUES (?, ?, ?)');
+
+    my $sth_delete = $dbh->prepare('DELETE FROM group_group_map
+                                     WHERE member_id = ?
+                                           AND grantor_id = ?
+                                           AND grant_type = ?');
+
+    # First item is the type, second is whether or not it's "reverse" 
+    # (granted_by) (see _do_add for more explanation).
+    my %fields = (
+        members       => [GROUP_MEMBERSHIP, 0],
+        bless_from    => [GROUP_BLESS, 0],
+        visible_from  => [GROUP_VISIBLE, 0],
+        member_of     => [GROUP_MEMBERSHIP, 1],
+        bless_to      => [GROUP_BLESS, 1],
+        visible_to_me => [GROUP_VISIBLE, 1]
+    );
+    while (my ($field, $data) = each %fields) {
+        _do_add($group, $changes, $sth_insert, "${field}_add", 
+                $data->[0], $data->[1]);
+        _do_remove($group, $changes, $sth_delete, "${field}_remove",
+                   $data->[0], $data->[1]);
+    }
+
+    $dbh->bz_unlock_tables();
+    return $changes;
+}
+
+sub _do_add {
+    my ($group, $changes, $sth_insert, $field, $type, $reverse) = @_;
+
+    my $current;
+    # $reverse means we're doing a granted_by--that is, somebody else
+    # is granting us something.
+    if ($reverse) {
+        $current = $group->granted_by_direct($type);
+    }
+    else {
+        $current = $group->grant_direct($type);
     }
 
-    my $sthInsert = $dbh->prepare('INSERT INTO group_group_map
-                                   (member_id, grantor_id, grant_type)
-                                   VALUES (?, ?, ?)');
-
-    my $sthDelete = $dbh->prepare('DELETE FROM group_group_map
-                                    WHERE member_id = ?
-                                      AND grantor_id = ?
-                                      AND grant_type = ?');
-
-    foreach my $b (grep {/^oldgrp-\d*$/} $cgi->param()) {
-        if (defined($cgi->param($b))) {
-            $b =~ /^oldgrp-(\d+)$/;
-            my $v = $1;
-            my $grp = $cgi->param("grp-$v") || 0;
-            if (($v != $gid) && ($cgi->param("oldgrp-$v") != $grp)) {
-                $chgs = 1;
-                if ($grp != 0) {
-                    $sthInsert->execute($v, $gid, GROUP_MEMBERSHIP);
-                } else {
-                    $sthDelete->execute($v, $gid, GROUP_MEMBERSHIP);
-                }
-            }
-
-            my $bless = $cgi->param("bless-$v") || 0;
-            my $oldbless = $cgi->param("oldbless-$v");
-            if ((defined $oldbless) and ($oldbless != $bless)) {
-                $chgs = 1;
-                if ($bless != 0) {
-                    $sthInsert->execute($v, $gid, GROUP_BLESS);
-                } else {
-                    $sthDelete->execute($v, $gid, GROUP_BLESS);
-                }
-            }
-
-            my $cansee = $cgi->param("cansee-$v") || 0;
-            if (Bugzilla->params->{"usevisibilitygroups"} 
-               && ($cgi->param("oldcansee-$v") != $cansee)) {
-                $chgs = 1;
-                if ($cansee != 0) {
-                    $sthInsert->execute($v, $gid, GROUP_VISIBLE);
-                } else {
-                    $sthDelete->execute($v, $gid, GROUP_VISIBLE);
-                }
-            }
+    my $add_items = Bugzilla::Group->new_from_list([$cgi->param($field)]);
 
-        }
+    foreach my $add (@$add_items) {
+        next if grep($_->id == $add->id, @$current);
+
+        $changes->{$field} ||= [];
+        push(@{$changes->{$field}}, $add->name);
+        # They go this direction for a normal "This group is granting
+        # $add something."
+        my @ids = ($add->id, $group->id);
+        # But they get reversed for "This group is being granted something
+        # by $add."
+        @ids = reverse @ids if $reverse;
+        $sth_insert->execute(@ids, $type);
+    }
+}
+
+sub _do_remove {
+    my ($group, $changes, $sth_delete, $field, $type, $reverse) = @_;
+    my $remove_items = Bugzilla::Group->new_from_list([$cgi->param($field)]);
+
+    foreach my $remove (@$remove_items) {
+        my @ids = ($remove->id, $group->id);
+        # See _do_add for an explanation of $reverse
+        @ids = reverse @ids if $reverse;
+        # Deletions always succeed and are harmless if they fail, so we
+        # don't need to do any checks.
+        $sth_delete->execute(@ids, $type);
+        $changes->{$field} ||= [];
+        push(@{$changes->{$field}}, $remove->name);
     }
-    $dbh->bz_unlock_tables();
-    return $gid, $chgs, $name, $regexp;
 }
diff --git a/template/en/default/admin/groups/confirm-remove.html.tmpl b/template/en/default/admin/groups/confirm-remove.html.tmpl
new file mode 100644
index 000000000..7c8df2701
--- /dev/null
+++ b/template/en/default/admin/groups/confirm-remove.html.tmpl
@@ -0,0 +1,66 @@
+[%# 1.0@bugzilla.org %]
+[%# 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 Netscape Communications
+  # Corporation. Portions created by Netscape are
+  # Copyright (C) 1998 Netscape Communications Corporation. All
+  # Rights Reserved.
+  #
+  # Contributor(s): Dave Miller <justdave@syndicomm.com>
+  #                 Joel Peshkin <bugreport@peshkin.net>
+  #                 Jacob Steenhagen <jake@bugzilla.org>
+  #                 Vlad Dascalu <jocuri@softhome.net>
+  #                 Max Kanat-Alexander <mkanat@bugzilla.org>
+  #%]
+
+[%# INTERFACE:
+  # group: The Bugzilla::Group being changed.
+  # regexp: the regexp according to which the update is performed.
+  #%]
+
+[% IF regexp %]
+  [% title = "Confirm: Remove Explicit Members in the Regular Expression?" %]
+[% ELSE %]
+  [% title = "Confirm: Remove All Explicit Members?" %]
+[% END %]
+
+[% PROCESS global/header.html.tmpl %]
+
+[% IF regexp %]
+  <p>This option will remove all users from '[% group.name FILTER html %]'
+    whose login names match the regular expression:
+    '[% regexp FILTER html %]'</p>
+[% ELSE %]
+  <p>This option will remove all explicitly defined users
+    from '[% group.name FILTER html %].'</p>
+[% END %]
+  
+<p>Generally, you will only need to do this when upgrading groups
+  created with [% terms.Bugzilla %] versions 2.16 and prior. Use
+  this option with <b>extreme care</b> and consult the documentation
+  for further information.
+</p>
+    
+<form method="post" action="editgroups.cgi">
+  <input type="hidden" name="group_id" value="[% group.id FILTER html %]">
+  <input type="hidden" name="regexp" value="[% regexp FILTER html %]">
+  <input type="hidden" name="action" value="remove_regexp">
+  
+  <input name="token" type="hidden" value="[% token FILTER html %]">
+  <input name="confirm" type="submit" value="Confirm">
+  <p>Or <a href="editgroups.cgi">return to the Edit Groups page</a>.</p>
+</form>
+    
+<p>Back to the <a href="editgroups.cgi">group list</a>.</p>
+
+[% PROCESS global/footer.html.tmpl %] 
diff --git a/template/en/default/admin/groups/edit.html.tmpl b/template/en/default/admin/groups/edit.html.tmpl
index 6f333f5c3..89dd66ce6 100644
--- a/template/en/default/admin/groups/edit.html.tmpl
+++ b/template/en/default/admin/groups/edit.html.tmpl
@@ -20,51 +20,45 @@
   #                 Joel Peshkin <bugreport@peshkin.net>
   #                 Jacob Steenhagen <jake@bugzilla.org>
   #                 Vlad Dascalu <jocuri@softhome.net>
+  #                 Max Kanat-Alexander <mkanat@bugzilla.org>
   #%]
 
 [%# INTERFACE:
-  # group_id: number. The group ID.
-  # name: string. The name of the group. [grantor]
-  # description: string. The description of the group.
-  # regexp: string. The regular expression for the users of the group.
-  # isactive: boolean int. Shows if the group is still active.
-  # isbuggroup: boolean int. Is 1 if this is a bug group.
-  # groups: array with group objects having the properties:
-  #   - grpid: number. The ID of the group.
-  #   - grpname: string. The name of the group. [member]
-  #   - grpdesc: string. The description of the group.
-  #   - grpmember: boolean int. Is 1 if members of the group are to inherit
-  #                membership in the group being edited.
-  #   - blessmember: boolean int. Is 1 if members of the group are to be able
-  #                  to bless users into the group being edited.
-  #   - membercansee: boolean int. Is 1 if the members of the group are to
-  #                   be aware of the group being edited and its members.
+  # group - A Bugzilla::Group representing the group being edited.
+  # *_current - Arrays of Bugzilla::Group objects that show the current
+  #             values for this group, as far as grants.
+  # *_available - Arrays of Bugzilla::Group objects that show the current 
+  #               available values for each grant.
   #%]
 
 [% title = BLOCK %]Change Group: [% name FILTER html %][% END %]
 
 [% PROCESS global/header.html.tmpl
-  title = title
-  style = "tr.odd_row {
-               background: #e9e9e9; 
-           }
-           .permissions th {
-               background: #000000;
-               color: #ffffff;
-           }
-          "
+  style = "
+    .grant_table { border-collapse: collapse; }
+    .grant_table td, .grant_table th {
+        padding-left: .5em;
+    }
+    .grant_table td.one, .grant_table th.one {
+        border-right: 1px solid black;
+        padding-right: .5em;
+    }
+  "
 %]
 
 <form method="post" action="editgroups.cgi">
+  <input type="hidden" name="action" value="postchanges">
+  <input type="hidden" name="group_id" value="[% group.id FILTER html %]">
+
   <table border="1" cellpadding="4">
     <tr>
       <th>Group:</th>
       <td>
-        [% IF isbuggroup %]
-          <input type="hidden" name="oldname" value="[% name FILTER html %]">
-          <input type="text" name="name" size="60" value="[% name FILTER html %]">
+        [% IF group.is_bug_group %]
+          <input type="text" name="name" size="60" 
+                 value="[% group.name FILTER html %]">
         [% ELSE %]
-          [% name FILTER html %]
+          [% group.name FILTER html %]
         [% END %]
       </td>
     </tr>
@@ -72,11 +66,11 @@
     <tr>
       <th>Description:</th>
       <td>
-        [% IF isbuggroup %]
-          <input type="hidden" name="olddesc" value="[% description FILTER html %]">
-          <input type="text" name="desc" size="70" value="[% description FILTER html %]">
+        [% IF group.is_bug_group %]
+          <input type="text" name="desc" size="70" 
+                 value="[% group.description FILTER html %]">
         [% ELSE %]
-          [% description FILTER html %]
+          [% group.description FILTER html %]
         [% END %]
       </td>
     </tr>
@@ -84,143 +78,157 @@
     <tr>
       <th>User Regexp:</th>
       <td>
-        <input type="hidden" name="oldregexp" value="[% regexp FILTER html %]">
-        <input type="text" name="regexp" size="40" value="[% regexp FILTER html %]">
+        <input type="text" name="regexp" size="40" 
+               value="[% group.user_regexp FILTER html %]">
       </td>
     </tr>
 
-    [% IF isbuggroup %]
+    [% IF group.is_bug_group %]
       <tr>
         <th>Use For [% terms.Bugs %]:</th>
         <td>
-          <input type="checkbox" name="isactive" value="1" [% (isactive == 1) ? "checked" : "" %]>
-          <input type="hidden" name="oldisactive" value="[% isactive FILTER html %]">
+          <input type="checkbox" name="isactive" 
+                 value="1" [% 'checked="checked"' IF group.is_active %]>
         </td>
       </tr>
     [% END %]
   </table>
 
-  <p>Users become members of this group in one of three ways:</p>
-    <ul>
-      <li> by being explicity included when the user is edited.
-      <li> by matching the user regexp above.
-      <li> by being a member of one of the groups included in this group
-           by checking the boxes below.
-    </ul>
-
-  [% usevisibility = Param('usevisibilitygroups') %]
-
-    <h4>Group Permissions</h4>
-  <table class="permissions" cellspacing="0" cellpadding="2">
-    <tr>     
-      [% IF usevisibility %]
-        <th>
-          Visible
-        </th>
-      [% END %]
-      <th>
-        Grant
-      </th>
-      <th>
-        Inherit
-      </th>
-      <th>
-        Group
-      </th>
-      <th>
-        Description
-      </th>
+  <h4>Group Permissions</h4>
+
+  <table class="grant_table">
+    <tr>
+      <th class="one">Groups That Are a Member of This Group<br>
+        (&quot;Users in <var>X</var> are automatically in  
+         [%+ group.name FILTER html %]&quot;)</th>
+      <th>Groups That This Group Is a Member Of<br>
+        (&quot;If you are in [% group.name FILTER html %], you are 
+         automatically also in...&quot;)</th>
+    </tr>
+    <tr>
+      <td class="one">
+        [% PROCESS select_pair name = "members" size = 10
+                   items_available = members_available
+                     items_current = members_current %]
+      </td>
+  
+      <td>[% PROCESS select_pair name = "member_of" size = 10
+                     items_available = member_of_available
+                       items_current = member_of_current %]</td>
     </tr>
-    [% row = 0 %]
-    [% FOREACH group = groups %]
-      [% row = row + 1 %]
-      <tr [% 'class="odd_row"' IF row % 2 %]>
-        [% IF usevisibility %]
-          <td align="center">
-            <input type="checkbox" name="cansee-[% group.grpid FILTER none %]" 
-              [% group.membercansee ? "checked " : "" %]value="1">
-            <input type="hidden" name="oldcansee-[% group.grpid FILTER none %]"
-              value="[% group.membercansee FILTER none %]">
-          </td>
-        [% END %]
-        [% IF group_id != group.grpid %]
-          <td align="center">
-            <input type="checkbox" name="bless-[% group.grpid FILTER html %]" [% group.blessmember ? "checked " : "" %]value="1">
-            <input type="hidden" name="oldbless-[% group.grpid FILTER html %]" value="[% group.blessmember FILTER html %]">
-          </td>
-          <td align="center">
-            <input type="checkbox" name="grp-[% group.grpid FILTER html %]" [% group.grpmember ? "checked " : "" %]value="1">
-            <input type="hidden" name="oldgrp-[% group.grpid FILTER html %]" value="[% group.grpmember FILTER html %]">
-          </td>
-          <td align="left" class="groupname">
-            <a href="[% "editgroups.cgi?action=changeform&group=${group.grpid}" FILTER html %]">
-              [% group.grpnam FILTER html %]
-            </a>
-          </td>
-        [% ELSE %]
-          <td>
-            <input type="hidden" name="oldbless-[% group.grpid FILTER html %]" value="0">
-          </td>
-          <td>
-            <input type="hidden" name="oldgrp-[% group.grpid FILTER html %]" value="0">
-          </td>
-          <td align="left" class="groupname">
-            <em>
-              [% group.grpnam FILTER html %]
-            </em>
-          </td>
-        [% END %]
-        <td align="left" class="groupdesc">[% group.grpdesc FILTER html_light %]</td>
-      </tr>
-    [% END %]
   </table>
 
-  <input type="submit" id="update" value="Save Changes">
-  <br>
-  <dl>
-    [% IF usevisibility %]
-      <dt>Visibility:</dt>
-      <dd>
-        Members of the selected groups can be aware of the 
-        "[% name FILTER html %]" group
-      </dd>
-    [% END %]
-    <dt>Grant:</dt>
-    <dd>
-    Members of the selected groups can grant membership to the
-    "[% name FILTER html %]" group
-    </dd>
-    <dt>Inherit:</dt>
-    <dd>
-      Members of the selected groups are automatically members of the
-      "[% name FILTER html %]" group
-    </dd>
-  </dl>
-  <table width="76%" border="0">
+  <table class="grant_table">
     <tr>
-      <td>
-        <h4>Conversion of groups created with [% terms.Bugzilla %]
-        versions 2.16 and prior:</h4>
-
-        <ul>
-          <li>Remove all explicit memberships from this group: 
-            <input name="remove_explicit_members" type="submit" id="remove_explicit_members" value="Remove Memberships">
-          </li>
-
-          <li>Remove all explicit memberships that are included in the above
-            regular expression: 
-            <input name="remove_explicit_members_regexp" type="submit" id="remove_explicit_members_regexp" value="Remove memberships included in regular expression"> 
-          </li>
-        </ul>
+      <th class="one">
+        Groups That Can Grant Membership in This Group<br>
+        (&quot;Users in <var>X</var> can add other users to 
+         [%+ group.name FILTER html %]&quot;)
+
+      </th>
+     <th>Groups That This Group Can Grant Membership In<br>
+       (&quot;Users in [% group.name FILTER html %] can add users to...&quot;)
+     </th>
+    </tr>
+    <tr>
+      <td class="one">
+        [% PROCESS select_pair name = "bless_from" size = 10
+                   items_available = bless_from_available
+                     items_current = bless_from_current %]
+      </td>
+      <td>[% PROCESS select_pair name = "bless_to" size = 10
+                     items_available = bless_to_available
+                       items_current = bless_to_current %]
       </td>
     </tr>
-  </table>
+  <table>
 
-  <input type="hidden" name="action" value="postchanges">
-  <input type="hidden" name="group" value="[% group_id FILTER html %]">
+  [% IF Param('usevisibilitygroups') %]
+    <table class="grant_table">
+      <tr>
+        <th class="one">
+          Groups That Can See This Group<br>
+          (&quot;Users in <var>X</var> can see users in
+           [%+ group.name FILTER html %]&quot;)
+        </th>
+       <th>Groups That This Group Can See<br>
+         (&quot;Users in [% group.name FILTER html %] can see users in...&quot;)
+       </th>
+      </tr>
+      <tr>
+        <td class="one">
+          [% PROCESS select_pair name = "visible_from" size = 10
+                     items_available = visible_from_available
+                       items_current = visible_from_current %]
+        </td>
+        <td>[% PROCESS select_pair name = "visible_to_me" size = 10
+                       items_available = visible_to_me_available
+                         items_current = visible_to_me_current %]
+        </td>
+      </tr>
+    <table>
+  [% END %]
+
+  <input type="submit" value="Update Group">
   <input type="hidden" name="token" value="[% token FILTER html %]">
 </form>
+  
+<h4>Mass Remove</h4>
+
+<p>You can use this form to do mass-removal of users from groups.
+  This is often very useful if you upgraded from [% terms.Bugzilla %] 
+  2.16.</p>
 
+<table<tr><td>
+<form method="post" action="editgroups.cgi">
+  <fieldset>
+    <legend>Remove all explict memberships from users whose login names
+      match the following regular expression:</legend>
+    <input type="text" size="20" name="regexp">
+    <input type="submit" value="Remove Memberships">
+
+    <p>If you leave the field blank, all explicit memberships in 
+      this group will be removed.</p>
+
+    <input type="hidden" name="action" value="confirm_remove">
+    <input type="hidden" name="group_id" value="[% group.id FILTER html %]">
+  </fieldset>
+</form>
+</td></tr></table>
+ 
 <p>Back to the <a href="editgroups.cgi">group list</a>.</p>
 
 [% PROCESS global/footer.html.tmpl %] 
+
+[% BLOCK select_pair %]
+  <table class="select_pair">
+    <tr>
+      <th><label for="[% "${name}_add" FILTER html %]">Add<br>
+        (select to add)</label></th>
+      <th><label for="[% "${name}_remove" FILTER html %]">Current<br>
+        (select to remove)</label></th>
+    </tr>
+    <tr>
+      <td>
+        <select multiple="multiple" size="[% size FILTER html %]"
+                name="[% "${name}_add" FILTER html %]"
+                id="[% "${name}_add" FILTER html %]">
+          [% FOREACH item = items_available %]
+            <option value="[% item.id FILTER html %]">
+              [% item.name FILTER html %]</option>
+          [% END %]
+        </select>
+      </td>
+      <td>
+        <select multiple="multiple" size="[% size FILTER html %]"
+                name="[% "${name}_remove" FILTER html %]"
+                id="[% "${name}_remove" FILTER html %]">
+          [% FOREACH item = items_current %]
+            <option value="[% item.id FILTER html %]">
+              [% item.name FILTER html %]</option>
+          [% END %]
+        </select>
+      </td>
+    </tr>
+  </table>
+[% END %]
diff --git a/template/en/default/admin/groups/remove.html.tmpl b/template/en/default/admin/groups/remove.html.tmpl
index 8c41333e4..fc7613359 100644
--- a/template/en/default/admin/groups/remove.html.tmpl
+++ b/template/en/default/admin/groups/remove.html.tmpl
@@ -20,36 +20,21 @@
   #                 Joel Peshkin <bugreport@peshkin.net>
   #                 Jacob Steenhagen <jake@bugzilla.org>
   #                 Vlad Dascalu <jocuri@softhome.net>
+  #                 Max Kanat-Alexander <mkanat@bugzilla.org>
   #%]
 
 [%# INTERFACE:
-  # remove_all: boolean int. Is 1 if the action was remove_all,
-  #         and 0 if the action was remove_all_regexp.
-  # name: string. The place where removal is performed.
-  # regexp: string. The regexp according to which the removal is performed.
-  # users: array with group objects having the properties:
-  #   - login: string. The login which is removed.
+  # group: The Bugzilla::Group being modified.
+  # regexp: string. The regexp according to which the removal was performed.
+  # users: Array of Bugzilla::User objects who were removed from this group.
   #%]
 
 
-[% IF remove_all %]
-  [% title = BLOCK %]
-    Removing All Explicit Group Memberships from '[% name FILTER html %]'
-  [% END %]
-[% ELSE %]
-  [% title = BLOCK %]
-    Removing All Explicit Group Memberships Matching Group RegExp from '[% name FILTER html %]'
-  [% END %]
-[% END %]
-
-[% PROCESS global/header.html.tmpl %]
+[% PROCESS global/header.html.tmpl
+           title = "Removing Explicit Group Membership" %]
 
-[% IF remove_all %]
-  <p><b>Removing explicit membership</b></p>
-[% ELSE %]
-  <p><b>Removing explicit memberships of users matching
-  '[% regexp FILTER html %]'...</b></p>
-[% END %]
+<p><b>Removing explicit memberships[% IF regexp %] of users matching
+  '[% regexp FILTER html %]'[% END %]...</b></p>
     
 [% FOREACH user = users %]
   [% user.login FILTER html %] removed<br>
diff --git a/template/en/default/global/messages.html.tmpl b/template/en/default/global/messages.html.tmpl
index ef03f7614..bebed5579 100644
--- a/template/en/default/global/messages.html.tmpl
+++ b/template/en/default/global/messages.html.tmpl
@@ -209,6 +209,69 @@
     An error occured while validating flags:
     [%+ flag_creation_error FILTER none %]
 
+  [% ELSIF message_tag == "group_updated" %]
+    [% IF changes.keys.size %]
+      The following changes have been made to the '[% group.name FILTER html %]
+      group:
+      <ul>
+      [% FOREACH field = changes.keys.sort %]
+        [% SWITCH field %]
+          [% CASE 'name' %]
+            <li>The name was changed to '[% changes.name.1 FILTER html %]'</li>
+          [% CASE 'description' %]
+            <li>The description was updated.</li>
+          [% CASE 'userregexp' %]
+            <li>The regular expression was updated.</li>
+          [% CASE 'isactive' %]
+            [% IF changes.isactive.1 %]
+              <li>The group will now be used for [% terms.bugs %].</li>
+            [% ELSE %]
+              <li>The group will no longer be used for [% terms.bugs %].</li>
+            [% END %]
+          [% CASE 'members_add' %]
+            <li>The following groups are now members of this group:
+              [%+ changes.members_add.join(', ') FILTER html %]</li>
+          [% CASE 'members_remove' %]
+            <li>The following groups are no longer members of this group:
+              [%+ changes.members_remove.join(', ') FILTER html %]</li>
+          [% CASE 'member_of_add' %]
+            <li>This group is now a member of the following groups:
+              [%+ changes.member_of_add.join(', ') FILTER html %]</li>
+          [% CASE 'member_of_remove' %]
+            <li>This group is no longer a member of the following groups:
+              [%+ changes.member_of_remove.join(', ') FILTER html %]</li>
+          [% CASE 'bless_from_add' %]
+            <li>The following groups may now add users to this group:
+              [%+ changes.bless_from_add.join(', ') FILTER html %]</li>
+          [% CASE 'bless_from_remove' %]
+            <li>The following groups may no longer add users to this group:
+              [%+ changes.bless_from_remove.join(', ') FILTER html %]</li>
+          [% CASE 'bless_to_add' %]
+            <li>This group may now add users to the following groups:
+              [%+ changes.bless_to_add.join(', ') FILTER html %]</li>
+          [% CASE 'bless_to_remove' %]
+            <li>This group may no longer add users to the following groups:
+              [%+ changes.bless_to_remove.join(', ') FILTER html %]</li>
+          [% CASE 'visible_from_add' %]
+            <li>The following groups can now see users in this group:
+              [%+ changes.visible_from_add.join(', ') FILTER html %]</li>
+          [% CASE 'visible_from_remove' %]
+            <li>The following groups may no longer see users in this group:
+              [%+ changes.visible_from_remove.join(', ') FILTER html %]</li>
+          [% CASE 'visible_to_me_add' %]
+            <li>This group may now see users in the following groups:
+              [%+ changes.visible_to_me_add.join(', ') FILTER html %]</li>
+          [% CASE 'visible_to_me_remove' %]
+            <li>This group may no longer see users in the following groups:
+              [%+ changes.visible_to_me_remove.join(', ') FILTER html %]</li>
+        [% END %]
+      [% END %]
+      </ul>
+    [% ELSE %]
+      You didn't request any change for the '[% group.name FILTER html %]'
+      group.
+    [% END %]
+
   [% ELSIF message_tag == "logged_out" %]
     [% title = "Logged Out" %]
     [% url = "index.cgi?GoAheadAndLogIn=1" %]