Bug 251837: Extend group_group_map to control which groups can see each other

r=kiko
a=justdave

9 files changed, 286 insertions, 74 deletions

diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm
index e3cdf539d..d580dddc7 100644
--- a/Bugzilla/Constants.pm
+++ b/Bugzilla/Constants.pm
@@ -54,6 +54,10 @@ use base qw(Exporter);
     GRANT_DIRECT
     GRANT_DERIVED
     GRANT_REGEXP
+
+    GROUP_MEMBERSHIP
+    GROUP_BLESS
+    GROUP_VISIBLE
 );
 
 @Bugzilla::Constants::EXPORT_OK = qw(contenttypes);
@@ -122,4 +126,8 @@ use constant GRANT_DIRECT => 0;
 use constant GRANT_DERIVED => 1;
 use constant GRANT_REGEXP => 2;
 
+use constant GROUP_MEMBERSHIP => 0;
+use constant GROUP_BLESS => 1;
+use constant GROUP_VISIBLE => 2;
+
 1;
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index b3d953945..e9b7fe0c4 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -227,6 +227,40 @@ sub in_group {
     return defined($res);
 }
 
+# visible_groups_inherited returns a reference to a list of all the groups
+# whose members are visible to this user.
+sub visible_groups_inherited {
+    my $self = shift;
+    return $self->{visible_groups_inherited} if defined $self->{visible_groups_inherited};
+    my @visgroups = @{$self->visible_groups_direct};
+    @visgroups = flatten_group_membership(@visgroups);
+    $self->{visible_groups_inherited} = \@visgroups;
+    return $self->{visible_groups_inherited};
+}
+
+# visible_groups_direct returns a reference to a list of all the groups that
+# are visible to this user.
+sub visible_groups_direct {
+    my $self = shift;
+    my @visgroups = ();
+    return $self->{visible_groups_direct} if defined $self->{visible_groups_direct};
+
+    my $dbh = Bugzilla->dbh;
+    my $glist = join(',',(-1,values(%{$self->groups})));
+    my $sth = $dbh->prepare("SELECT DISTINCT grantor_id
+                                FROM group_group_map
+                               WHERE member_id IN($glist)
+                                 AND grant_type=" . GROUP_VISIBLE);
+    $sth->execute();
+
+    while (my ($row) = $sth->fetchrow_array) {
+        push @visgroups,$row;
+    }
+    $self->{visible_groups_direct} = \@visgroups;
+
+    return $self->{visible_groups_direct};
+}
+
 sub derive_groups {
     my ($self, $already_locked) = @_;
 
@@ -287,9 +321,10 @@ sub derive_groups {
             $group_sth ||= $dbh->prepare(q{SELECT grantor_id
                                              FROM group_group_map
                                             WHERE member_id=?
-                                              AND isbless=0});
+                                              AND grant_type=' . 
+                                                  GROUP_MEMBERSHIP . '});
             $group_sth->execute($group);
-            while (my $groupid = $group_sth->fetchrow_array) {
+            while (my ($groupid) = $group_sth->fetchrow_array) {
                 if (!defined($groupidschecked{"$groupid"})) {
                     push(@groupidstocheck,$groupid);
                 }
@@ -332,7 +367,8 @@ sub can_bless {
                                             FROM user_group_map, group_group_map
                                            WHERE user_group_map.user_id=?
                                              AND user_group_map.group_id=member_id
-                                             AND group_group_map.isbless=1},
+                                             AND group_group_map.grant_type=} .
+                                                 GROUP_BLESS,
                                         undef,
                                         $self->{id});
     }
@@ -342,6 +378,30 @@ sub can_bless {
     return $self->{can_bless};
 }
 
+sub flatten_group_membership {
+    my (@groups) = @_;
+
+    my $dbh = Bugzilla->dbh;
+    my $sth;
+    my @groupidstocheck = @groups;
+    my %groupidschecked = ();
+    $sth = $dbh->prepare("SELECT member_id FROM group_group_map
+                             WHERE grantor_id = ? 
+                               AND grant_type = " . GROUP_MEMBERSHIP);
+    while (my $node = shift @groupidstocheck) {
+        $sth->execute($node);
+        my $member;
+        while (($member) = $sth->fetchrow_array) {
+            if (!$groupidschecked{$member}) {
+                $groupidschecked{$member} = 1;
+                push @groupidstocheck, $member;
+                push @groups, $member unless grep $_ == $member, @groups;
+            }
+        }
+    }
+    return @groups;
+}
+
 sub match {
     # Generates a list of users whose login name (email address) or real name
     # matches a substring or wildcard.
@@ -364,17 +424,28 @@ sub match {
     # first try wildcards
 
     my $wildstr = $str;
+    my $user = Bugzilla->user;
 
     if ($wildstr =~ s/\*/\%/g && # don't do wildcards if no '*' in the string
         Param('usermatchmode') ne 'off') { # or if we only want exact matches
 
         # Build the query.
         my $sqlstr = &::SqlQuote($wildstr);
-        my $query  = "SELECT userid, realname, login_name " .
-                     "FROM profiles " .
-                     "WHERE (login_name LIKE $sqlstr " .
+        my $query  = "SELECT DISTINCT userid, realname, login_name " .
+                     "FROM profiles ";
+        if (&::Param('usevisibilitygroups')) {
+            $query .= ", user_group_map ";
+        }
+        $query    .= "WHERE (login_name LIKE $sqlstr " .
                      "OR realname LIKE $sqlstr) ";
-        $query    .= "AND disabledtext = '' " if $exclude_disabled;
+        if (&::Param('usevisibilitygroups')) {
+            $query .= "AND user_group_map.user_id = userid " .
+                      "AND isbless = 0 " .
+                      "AND group_id IN(" .
+                      join(', ', (-1, @{$user->visible_groups_inherited})) . ") " .
+                      "AND grant_type <> " . GRANT_DERIVED;
+        }
+        $query    .= " AND disabledtext = '' " if $exclude_disabled;
         $query    .= "ORDER BY length(login_name) ";
         $query    .= "LIMIT $limit " if $limit;
 
@@ -410,14 +481,23 @@ sub match {
 
         my $sqlstr = &::SqlQuote(uc($str));
 
-        my $query  = "SELECT  userid, realname, login_name " .
-                     "FROM  profiles " .
-                     "WHERE  (INSTR(UPPER(login_name), $sqlstr) " .
-                     "OR INSTR(UPPER(realname), $sqlstr)) ";
-        $query    .= "AND disabledtext = '' " if $exclude_disabled;
+        my $query  = "SELECT DISTINCT userid, realname, login_name " .
+                     "FROM  profiles ";
+        if (&::Param('usevisibilitygroups')) {
+            $query .= ", user_group_map ";
+        }
+        $query     .= "WHERE  (INSTR(UPPER(login_name), $sqlstr) " .
+                      "OR INSTR(UPPER(realname), $sqlstr)) ";
+        if (&::Param('usevisibilitygroups')) {
+            $query .= "AND user_group_map.user_id = userid " .
+                      "AND isbless = 0 " .
+                      "AND group_id IN(" .
+                      join(', ', (-1, @{$user->visible_groups_inherited})) . ") " .
+                      "AND grant_type <> " . GRANT_DERIVED;
+        }
+        $query    .= " AND disabledtext = '' " if $exclude_disabled;
         $query    .= "ORDER BY length(login_name) ";
         $query    .= "LIMIT $limit " if $limit;
-
         &::PushGlobalSQLState();
         &::SendSQL($query);
         push(@users, new Bugzilla::User(&::FetchSQLData())) while &::MoreSQLData();
@@ -843,10 +923,20 @@ care of by the constructor. However, when updating the email address, the
 user may be placed into different groups, based on a new email regexp. This
 method should be called in such a case to force reresolution of these groups.
 
+=item C<visible_groups_inherited>
+
+Returns a list of all groups whose members should be visible to this user.
+Since this list is flattened already, there is no need for all users to
+be have derived groups up-to-date to select the users meeting this criteria.
+
+=item C<visible_groups_direct>
+
+Returns a list of groups that the user is aware of.
+
 =begin undocumented
 
 This routine takes an optional argument. If true, then this routine will not
-lock the tables, but will rely on the caller to ahve done so itsself.
+lock the tables, but will rely on the caller to have done so itsself.
 
 This is required because mysql will only execute a query if all of the tables
 are locked, or if none of them are, not a mixture. If the caller has already
diff --git a/checksetup.pl b/checksetup.pl
index 433810f62..e3de12d27 100755
--- a/checksetup.pl
+++ b/checksetup.pl
@@ -1912,12 +1912,19 @@ $table{user_group_map} =
 
      unique(user_id, group_id, grant_type, isbless)';
 
+# This table determines which groups are made a member of another
+# group, given the ability to bless another group, or given
+# visibility to another groups existence and membership
+# grant_type:
+# if GROUP_MEMBERSHIP - member groups are made members of grantor
+# if GROUP_BLESS - member groups may grant membership in grantor
+# if GROUP_VISIBLE - member groups may see grantor group
 $table{group_group_map} =
     'member_id mediumint not null,
      grantor_id mediumint not null,
-     isbless tinyint not null default 0,
+     grant_type tinyint not null default 0,
 
-     unique(member_id, grantor_id, isbless)';
+     unique(member_id, grantor_id, grant_type)';
 
 # This table determines which groups a user must be a member of
 # in order to see a bug.
@@ -3947,7 +3954,18 @@ if (GetFieldDef("user_group_map", "isderived")) {
 
 AddField('flags', 'is_active', 'tinyint not null default 1');
 
-    
+# 2004-07-16 - Make it possible to have group-group relationships other than
+# membership and bless.
+if (GetFieldDef("group_group_map", "isbless")) {
+    AddField('group_group_map', 'grant_type', 'tinyint not null default 0');
+    $dbh->do("UPDATE group_group_map SET grant_type = " .
+                             "IF(isbless, " . GROUP_BLESS . ", " .
+                             GROUP_MEMBERSHIP . ")");
+    DropIndexes("group_group_map");
+    DropField("group_group_map", "isbless");
+    $dbh->do("ALTER TABLE group_group_map 
+              ADD UNIQUE (member_id, grantor_id, grant_type)");
+}    
 
 # If you had to change the --TABLE-- definition in any way, then add your
 # differential change code *** A B O V E *** this comment.
@@ -4030,13 +4048,17 @@ if (@admins) {
     while ( my ($id) = $sth->fetchrow_array() ) {
         # Admins can bless every group.
         $dbh->do("INSERT INTO group_group_map 
-            (member_id, grantor_id, isbless) 
-            VALUES ($adminid, $id, 1)");
+            (member_id, grantor_id, grant_type) 
+            VALUES ($adminid, $id," . GROUP_BLESS . ")");
+        # Admins can see every group.
+        $dbh->do("INSERT INTO group_group_map 
+            (member_id, grantor_id, grant_type) 
+            VALUES ($adminid, $id," . GROUP_VISIBLE . ")");
         # Admins are initially members of every group.
         next if ($id == $adminid);
         $dbh->do("INSERT INTO group_group_map 
-            (member_id, grantor_id, isbless) 
-            VALUES ($adminid, $id, 0)");
+            (member_id, grantor_id, grant_type) 
+            VALUES ($adminid, $id," . GROUP_MEMBERSHIP . ")");
     }
 }
 
@@ -4231,8 +4253,8 @@ if ($sth->rows == 0) {
         VALUES ($userid, $id, 1, " . GRANT_DIRECT . ")");
     foreach my $group ( @groups ) {
         $dbh->do("INSERT INTO group_group_map
-            (member_id, grantor_id, isbless)
-            VALUES ($id, $group, 1)");
+            (member_id, grantor_id, grant_type)
+            VALUES ($id, $group, 1, " . GROUP_BLESS . ")");
     }
 
   print "\n$login is now set up as an administrator account.\n";
diff --git a/defparams.pl b/defparams.pl
index 6f8dcf595..84d9d5aeb 100644
--- a/defparams.pl
+++ b/defparams.pl
@@ -744,6 +744,14 @@ You will get this message once a day until you\'ve dealt with these bugs!
   },
 
   {
+   name => 'usevisibilitygroups',
+   desc => 'Do you wish to restrict visibility of users to members of ' .
+           'specific groups?',
+   type => 'b',
+   default => 0
+  },
+
+  {
    name => 'webdotbase',
    desc => 'It is possible to show graphs of dependent bugs. You may set ' .
            'this parameter to any of the following:
diff --git a/editgroups.cgi b/editgroups.cgi
index 29e28dd9a..96503a125 100755
--- a/editgroups.cgi
+++ b/editgroups.cgi
@@ -137,21 +137,27 @@ if ($action eq 'changeform') {
 
     my @groups;
     SendSQL("SELECT groups.id, groups.name, groups.description," .
-             " group_group_map.member_id IS NOT NULL," .
-             " B.member_id IS NOT NULL" .
+             " IF(group_group_map.member_id IS NOT NULL, 1, 0)," .
+             " IF(B.member_id IS NOT NULL, 1, 0)," .
+             " IF(C.member_id IS NOT NULL, 1, 0)" .
              " FROM groups" .
              " LEFT JOIN group_group_map" .
              " ON group_group_map.member_id = groups.id" .
              " AND group_group_map.grantor_id = $group_id" .
-             " AND group_group_map.isbless = 0" .
+             " AND group_group_map.grant_type = " . GROUP_MEMBERSHIP .
              " LEFT JOIN group_group_map as B" .
              " ON B.member_id = groups.id" .
              " AND B.grantor_id = $group_id" .
-             " AND B.isbless = 1" .
+             " AND B.grant_type = " . GROUP_BLESS .
+             " LEFT JOIN group_group_map as C" .
+             " ON C.member_id = groups.id" .
+             " AND C.grantor_id = $group_id" .
+             " AND C.grant_type = " . GROUP_VISIBLE .
              " WHERE groups.id != $group_id ORDER by name");
 
     while (MoreSQLData()) {
-        my ($grpid, $grpnam, $grpdesc, $grpmember, $blessmember) = FetchSQLData();
+        my ($grpid, $grpnam, $grpdesc, $grpmember, $blessmember, $membercansee) 
+            = FetchSQLData();
 
         my $group = {};
         $group->{'grpid'}       = $grpid;
@@ -159,6 +165,7 @@ if ($action eq 'changeform') {
         $group->{'grpdesc'}     = $grpdesc;
         $group->{'grpmember'}   = $grpmember;
         $group->{'blessmember'} = $blessmember;
+        $group->{'membercansee'}= $membercansee;
         push(@groups, $group);
     }
 
@@ -237,10 +244,14 @@ if ($action eq 'new') {
     SendSQL("SELECT last_insert_id()");
     my $gid = FetchOneColumn();
     my $admin = GroupNameToId('admin');
-    SendSQL("INSERT INTO group_group_map (member_id, grantor_id, isbless)
-             VALUES ($admin, $gid, 0)");
-    SendSQL("INSERT INTO group_group_map (member_id, grantor_id, isbless)
-             VALUES ($admin, $gid, 1)");
+    # Since we created a new group, give the "admin" group all privileges
+    # initially.
+    SendSQL("INSERT INTO group_group_map (member_id, grantor_id, grant_type)
+             VALUES ($admin, $gid, " . GROUP_MEMBERSHIP . ")");
+    SendSQL("INSERT INTO group_group_map (member_id, grantor_id, grant_type)
+             VALUES ($admin, $gid, " . GROUP_BLESS . ")");
+    SendSQL("INSERT INTO group_group_map (member_id, grantor_id, grant_type)
+             VALUES ($admin, $gid, " . GROUP_VISIBLE . ")");
     # Permit all existing products to use the new group if makeproductgroups.
     if ($cgi->param('insertnew')) {
         SendSQL("INSERT INTO group_control_map " .
@@ -524,12 +535,12 @@ sub doGroupChanges {
                 $chgs = 1;
                 if ($grp != 0) {
                     SendSQL("INSERT INTO group_group_map 
-                             (member_id, grantor_id, isbless)
-                             VALUES ($v, $gid, 0)");
+                             (member_id, grantor_id, grant_type)
+                             VALUES ($v, $gid," . GROUP_MEMBERSHIP . ")");
                 } else {
                     SendSQL("DELETE FROM group_group_map
                              WHERE member_id = $v AND grantor_id = $gid
-                             AND isbless = 0");
+                             AND grant_type = " . GROUP_MEMBERSHIP);
                 }
             }
 
@@ -538,12 +549,27 @@ sub doGroupChanges {
                 $chgs = 1;
                 if ($bless != 0) {
                     SendSQL("INSERT INTO group_group_map 
-                             (member_id, grantor_id, isbless)
-                             VALUES ($v, $gid, 1)");
+                             (member_id, grantor_id, grant_type)
+                             VALUES ($v, $gid," . GROUP_BLESS . ")");
+                } else {
+                    SendSQL("DELETE FROM group_group_map
+                             WHERE member_id = $v AND grantor_id = $gid
+                             AND grant_type = " . GROUP_BLESS);
+                }
+            }
+
+            my $cansee = $cgi->param("cansee-$v") || 0;
+            if (Param("usevisibilitygroups") 
+               && ($cgi->param("oldcansee-$v") != $cansee)) {
+                $chgs = 1;
+                if ($cansee != 0) {
+                    SendSQL("INSERT INTO group_group_map 
+                             (member_id, grantor_id, grant_type)
+                             VALUES ($v, $gid," . GROUP_VISIBLE . ")");
                 } else {
                     SendSQL("DELETE FROM group_group_map
                              WHERE member_id = $v AND grantor_id = $gid
-                             AND isbless = 1");
+                             AND grant_type = " . GROUP_VISIBLE);
                 }
             }
 
diff --git a/editproducts.cgi b/editproducts.cgi
index 99640a44e..bd71bdd6d 100755
--- a/editproducts.cgi
+++ b/editproducts.cgi
@@ -394,10 +394,14 @@ if ($action eq 'new') {
         SendSQL("SELECT last_insert_id()");
         my $gid = FetchOneColumn();
         my $admin = GroupNameToId('admin');
-        SendSQL("INSERT INTO group_group_map (member_id, grantor_id, isbless)
-                 VALUES ($admin, $gid, 0)");
-        SendSQL("INSERT INTO group_group_map (member_id, grantor_id, isbless)
-                 VALUES ($admin, $gid, 1)");
+        # If we created a new group, give the "admin" group priviledges
+        # initially.
+        SendSQL("INSERT INTO group_group_map (member_id, grantor_id, grant_type)
+                 VALUES ($admin, $gid," . GROUP_MEMBERSHIP .")");
+        SendSQL("INSERT INTO group_group_map (member_id, grantor_id, grant_type)
+                 VALUES ($admin, $gid," . GROUP_BLESS .")");
+        SendSQL("INSERT INTO group_group_map (member_id, grantor_id, grant_type)
+                 VALUES ($admin, $gid," . GROUP_VISIBLE .")");
 
         # Associate the new group and new product.
         SendSQL("INSERT INTO group_control_map " .
diff --git a/editusers.cgi b/editusers.cgi
index c40cc327d..5b1c88e98 100755
--- a/editusers.cgi
+++ b/editusers.cgi
@@ -165,7 +165,7 @@ sub EmitFormElements ($$$$)
                     WHERE $groupid = grantor_id 
                     AND user_group_map.user_id = $user_id
                     AND user_group_map.isbless = 0
-                    AND group_group_map.isbless = 1
+                    AND group_group_map.grant_type = " . GROUP_BLESS . "
                     AND user_group_map.group_id = member_id");
                 my $derivedbless = FetchOneColumn();
                 PopGlobalSQLState();
@@ -473,6 +473,8 @@ if ($action eq 'new') {
     SendSQL("SELECT last_insert_id()");
     my ($newuserid) = FetchSQLData();
 
+    my $changeduser = new Bugzilla::User($newuserid);
+    $changeduser->derive_groups();
     print "To change ${user}'s permissions, go back and " .
         "<a href=\"editusers.cgi?action=edit&user=" . url_quote($user) .
         "\">edit</a> this user.";
diff --git a/globals.pl b/globals.pl
index e31c8e717..07eb573f5 100644
--- a/globals.pl
+++ b/globals.pl
@@ -1217,7 +1217,7 @@ sub UserCanBlessGroup {
         WHERE groups.id = grantor_id 
         AND user_group_map.user_id = $::userid
         AND user_group_map.isbless = 0
-        AND group_group_map.isbless = 1
+        AND group_group_map.grant_type = " . GROUP_BLESS . "
         AND user_group_map.group_id = member_id
         AND groups.name = " . SqlQuote($groupname));
     $result = FetchOneColumn();
diff --git a/template/en/default/admin/groups/edit.html.tmpl b/template/en/default/admin/groups/edit.html.tmpl
index 619db777c..bdda7e27b 100644
--- a/template/en/default/admin/groups/edit.html.tmpl
+++ b/template/en/default/admin/groups/edit.html.tmpl
@@ -24,23 +24,34 @@
 
 [%# INTERFACE:
   # group_id: number. The group ID.
-  # name: string. The name of the group.
+  # name: string. The name of the group. [grantor]
   # description: string. The description of the group.
   # rexp: 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.
+  #   - grpname: string. The name of the group. [member]
   #   - grpdesc: string. The description of the group.
-  #   - grpmember: boolean int. Is 1 if the current user is a group member.
-  #   - blessmember: boolean int. Is 1 if the current user can bless members
-  #                  in the current 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.
   #%]
 
 
 [% PROCESS global/header.html.tmpl
-  title = "Change Group"
+  title = "Change Group: $name"
+  style = "tr.odd_row {
+               background: #e9e9e9; 
+           }
+           .permissions th {
+               background: #000000;
+               color: #ffffff;
+           }
+          "
 %]
 
 <form method="post" action="editgroups.cgi">
@@ -96,45 +107,86 @@
       <li> by being a member of one of the groups included in this group
            by checking the boxes below.
     </ul>
-  </p>
 
-  <table>
-    <tr>
-      <td colspan="4">Members of these groups can grant membership to this group</td>
-    </tr>
-    <tr>
-      <td align="center">|</td>
-      <td colspan="3">Members of these groups are included in this group</td>
-    </tr>
-    <tr>
-      <td align="center">|</td>
-      <td align="center">|</td>
-      <td colspan="2"></td>
+  [% 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>
     </tr>
+    [% row = 0 %]
     [% FOREACH group = groups %]
-      <tr>
-        <td>
+      [% 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 %]
+        <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>
+        <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><b>[% group.grpnam FILTER html %]</b></td>
-        <td>[% group.grpdesc 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>
+        <td align="left" class="groupdesc">[% group.grpdesc FILTER html %]</td>
       </tr>
     [% END %]
   </table>
   
-  <input type="submit" value="Submit">
-
-  <p>
-  <table width="76%" border="1">
+  <input type="submit" 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">
     <tr>
       <td>
-        <p><strong>Conversion of groups created with [% terms.Bugzilla %]
-        versions 2.16 and prior:</strong></p>
+        <h4>Conversion of groups created with [% terms.Bugzilla %]
+        versions 2.16 and prior:</h4>
         
         <ul>
           <li>Remove all explicit memberships from this group: