Bug 573173: Make Bugzilla::Bug's add_group and remove_group take group

names instead of ids
r=dkl, a=mkanat

6 files changed, 53 insertions, 59 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 58901c57e..ea8e4bc3d 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -2682,8 +2682,7 @@ sub add_group {
     my ($self, $group) = @_;
     # Invalid ids are silently ignored. (We can't tell people whether
     # or not a group exists.)
-    $group = new Bugzilla::Group($group) unless ref $group;
-    return unless $group;
+    $group = Bugzilla::Group->check($group) if !blessed $group;
 
     return if !$group->is_active or !$group->is_bug_group;
 
@@ -2691,7 +2690,7 @@ sub add_group {
     # to this group by the current user.
     $self->product_obj->group_is_settable($group)
          || ThrowUserError('group_invalid_restriction',
-                { product => $self->product, group_id => $group->id,
+                { product => $self->product, group => $group,
                   bug => $self });
 
     # OtherControl people can add groups only during a product change,
@@ -2702,7 +2701,7 @@ sub add_group {
             || $controls->{othercontrol} == CONTROLMAPNA)
         {
             ThrowUserError('group_change_denied',
-                           { bug => $self, group_id => $group->id });
+                           { bug => $self, group => $group });
         }
     }
 
@@ -2714,8 +2713,7 @@ sub add_group {
 
 sub remove_group {
     my ($self, $group) = @_;
-    $group = new Bugzilla::Group($group) unless ref $group;
-    return unless $group;
+    $group = Bugzilla::Group->check($group) if !blessed $group;
     
     # First, check if this is a valid group for this product.
     # You can *always* remove a group that is not valid for this product
@@ -2731,7 +2729,7 @@ sub remove_group {
         # Nobody can ever remove a Mandatory group.
         if ($controls->{membercontrol} == CONTROLMAPMANDATORY) {
             ThrowUserError('group_invalid_removal',
-                { product => $self->product, group_id => $group->id,
+                { product => $self->product, group => $group,
                   bug => $self });
         }
 
@@ -2743,7 +2741,7 @@ sub remove_group {
                 || $controls->{othercontrol} == CONTROLMAPNA)
             {
                 ThrowUserError('group_change_denied',
-                               { bug => $self, group_id => $group->id });
+                               { bug => $self, group => $group });
             }
         }
     }
diff --git a/process_bug.cgi b/process_bug.cgi
index 7759d03dc..f6da2af61 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -337,27 +337,10 @@ foreach my $field (grep(/^defined_isprivate/, $cgi->param())) {
 }
 $set_all_fields{comment_is_private} = \%is_private;
 
-my %groups = ( add => [], remove => [] );
-my %checked_bit; # Used to avoid adding groups twice (defined_ + actual bit-)
-foreach my $param_name (grep(/bit-\d+$/, $cgi->param())) {
-    $param_name =~ /bit-(\d+)$/;
-    my $gid = $1;
-    next if $checked_bit{$gid};
-    my $bit_param = "bit-$gid";
-    if (should_set($bit_param, 1)) {
-        # Check ! first to avoid having to check defined below.
-        if (!$cgi->param($bit_param)) {
-            push(@{ $groups{remove} }, $gid);
-        }
-        # "== 1" is important because mass-change uses -1 to mean
-        # "don't change this restriction"
-        elsif ($cgi->param($bit_param) == 1) {
-            push(@{ $groups{add} }, $gid);
-        }
-    }
-    $checked_bit{$gid} = 1;
-}
-$set_all_fields{groups} = \%groups;
+my @check_groups = $cgi->param('defined_groups');
+my @set_groups = $cgi->param('groups');
+my ($removed_groups) = diff_arrays(\@check_groups, \@set_groups);
+$set_all_fields{groups} = { add => \@set_groups, remove => $removed_groups };
 
 my @custom_fields = Bugzilla->active_custom_fields;
 foreach my $field (@custom_fields) {
diff --git a/template/en/default/bug/edit.html.tmpl b/template/en/default/bug/edit.html.tmpl
index 479e67c1e..0ef3cba8f 100644
--- a/template/en/default/bug/edit.html.tmpl
+++ b/template/en/default/bug/edit.html.tmpl
@@ -654,14 +654,15 @@
       [% END %]
 
       [% IF group.ingroup %]
-        <input type="hidden" name="defined_bit-[% group.bit %]" value="1">
+        <input type="hidden" name="defined_groups" 
+               value="[% group.name FILTER html %]">
       [% END %]
 
-      <input type="checkbox" value="1" name="bit-[% group.bit %]" 
-             id="bit-[% group.bit %]"
+      <input type="checkbox" value="[% group.name FILTER html %]"
+             name="groups" id="group_[% group.bit %]"
              [% ' checked="checked"' IF group.ison %]
              [% ' disabled="disabled"' IF NOT group.ingroup %]>
-      <label for="bit-[% group.bit %]">
+      <label for="group_[% group.bit %]">
       [%- group.description FILTER html_light %]</label>
       <br>
     [% END %]
diff --git a/template/en/default/bug/process/verify-new-product.html.tmpl b/template/en/default/bug/process/verify-new-product.html.tmpl
index 1cc186c44..20270c0aa 100644
--- a/template/en/default/bug/process/verify-new-product.html.tmpl
+++ b/template/en/default/bug/process/verify-new-product.html.tmpl
@@ -40,7 +40,7 @@
 
 [% SET exclude_items = ['version', 'component', 'target_milestone'] %]
 [% IF verify_bug_groups %]
-  [% exclude_items.push('bit-\d+') %]
+  [% exclude_items.push('groups', 'defined_groups') %]
 [% END %]
 [% Hook.process('exclude') %]
 
@@ -124,9 +124,10 @@
     [%+ terms.Bugs %] will no longer be restricted to these groups and may become
     public if no other group applies:<br>
     [% FOREACH group = old_groups %]
-      <input type="checkbox" id="bit-[% group.id FILTER html %]"
-             name="bit-[% group.id FILTER html %]" disabled="disabled" value="1">
-      <label for="bit-[% group.id FILTER html %]">
+      <input type="checkbox" id="group_[% group.group.id FILTER html %]"
+             name="groups" disabled="disabled"
+             value="[% group.group.name FILTER html %]">
+      <label for="group_[% group.group.id FILTER html %]">
         [% group.name FILTER html %]: [% group.description FILTER html %]
       </label>
       <br>
@@ -154,16 +155,15 @@
     <p>These groups are optional. You can decide to restrict [% terms.bugs %] to
     one or more of the following groups:<br>
     [% FOREACH group = optional_groups %]
-      <input type="hidden" name="defined_bit-[% group.group.id FILTER html %]"
-             value="1">
-      <input type="checkbox" id="bit-[% group.group.id FILTER html %]"
-             name="bit-[% group.group.id FILTER html %]"
-             [%+ ((group.membercontrol == constants.CONTROLMAPDEFAULT && user.in_group(group.group.name))
+      <input type="hidden" name="defined_groups"
+             value="[% group.group.name FILTER html %]">
+      <input type="checkbox" id="group_[% group.group.id FILTER html %]"
+             name="groups"
+             [% ' checked="checked"' IF ((group.membercontrol == constants.CONTROLMAPDEFAULT && user.in_group(group.group.name))
                  || (group.othercontrol == constants.CONTROLMAPDEFAULT && !user.in_group(group.group.name))
-                 || cgi.param("bit-$group.group.id") == 1) ?
-                 'checked="checked"' : ''
-             %] value="1">
-      <label for="bit-[% group.group.id FILTER html %]">
+                 || cgi.param("groups").contains(group.group.name)) %]
+             %] value="[% group.group.name FILTER html %]">
+      <label for="group_[% group.group.id FILTER html %]">
         [% group.group.name FILTER html %]: [% group.group.description FILTER html %]
       </label>
       <br>
@@ -175,9 +175,10 @@
     <p>These groups are mandatory and [% terms.bugs %] will be automatically
     restricted to these groups:<br>
     [% FOREACH group = mandatory_groups %]
-      <input type="checkbox" id="bit-[% group.group.id FILTER html %]" checked="checked"
-             name="bit-[% group.group.id FILTER html %]" value="1" disabled="disabled">
-      <label for="bit-[% group.group.id FILTER html %]">
+      <input type="checkbox" id="group_[% group.group.id FILTER html %]"
+              checked="checked" disabled="disabled"
+             name="groups" value="[% group.group.name FILTER html %]">
+      <label for="group_[% group.group.id FILTER html %]">
         [% group.group.name FILTER html %]: [% group.group.description FILTER html %]
       </label>
       <br>
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 373a9dc31..91211d56b 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -701,7 +701,7 @@
 
   [% ELSIF error == "group_change_denied" %]
     [% title = "Cannot Add/Remove That Group" %]
-    You tried to add or remove group id [% group_id FILTER html %]
+    You tried to add or remove the '[% group.name FILTER html %]' group
     from [% terms.bug %] [%+ bug.id FILTER html %], but you do not
     have permissions to do so.
 
@@ -726,13 +726,13 @@
 
   [% ELSIF error == "group_invalid_removal" %]
     You tried to remove [% terms.bug %] [%+ bug.id FILTER html %]
-    from group id [% group_id FILTER html %], but [% terms.bugs %] in the 
-    '[% product FILTER html %]' product can not be removed from that
+    from the '[% group.name FILTER html %]' group, but [% terms.bugs %]
+     in the '[% product FILTER html %]' product can not be removed from that
     group.
     
   [% ELSIF error == "group_invalid_restriction" %]
     You tried to restrict [% terms.bug %] [%+ bug.id FILTER html %] to
-    to group id [% group_id FILTER html %], but [% terms.bugs %] in the
+    to the '[% group.name FILTER html %]' group, but [% terms.bugs %] in the
     '[% product FILTER html %]' product can not be restricted to
     that group.
 
diff --git a/template/en/default/list/edit-multiple.html.tmpl b/template/en/default/list/edit-multiple.html.tmpl
index c6bfbd336..619afe8dd 100644
--- a/template/en/default/list/edit-multiple.html.tmpl
+++ b/template/en/default/list/edit-multiple.html.tmpl
@@ -309,10 +309,18 @@
 
 [% IF groups.size > 0 %]
 
+  <script type="text/javascript">
+    function turn_off(myself, id) {
+        var other_checkbox = document.getElementById(id);
+        if (myself.checked && other_checkbox) {
+            other_checkbox.checked = false;
+        }
+    }
+  </script>
+
   <b>Groups:</b><br>
   <table border="1">
     <tr>
-      <th>Don't<br>change<br>this group<br>restriction</th>
       <th>Remove<br>[% terms.bugs %]<br>from this<br>group</th>
       <th>Add<br>[% terms.bugs %]<br>to this<br>group</th>
       <th>Group Name:</th>
@@ -321,14 +329,17 @@
     [% FOREACH group = groups %]
     <tr>
       <td align="center">
-        <input type="radio" name="bit-[% group.id %]" value="-1" checked="checked">
-      </td>
-      <td align="center">
-        <input type="radio" name="bit-[% group.id %]" value="0">
+        <input type="checkbox" name="defined_groups" 
+               id="defined_group_[% group.id %]"
+               value="[% group.name FILTER html %]"
+               onchange="turn_off(this, 'group_[% group.id %]')">
       </td>
       [% IF group.is_active %]
         <td align="center">
-          <input type="radio" name="bit-[% group.id %]" value="1">
+          <input type="checkbox" name="groups" 
+                 id="group_[% group.id FILTER html %]"
+                 value="[% group.name FILTER html %]"
+                 onchange="turn_off(this, 'defined_group_[% group.id %]')">
         </td>
       [% ELSE %]
         <td>&nbsp;</td>