Bug 388045: Move updating of cc_accessible and reporter_accessible into Bugzilla::Bug (from process_bug)

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

4 files changed, 49 insertions, 31 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index cf1051a74..ad9146504 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -151,12 +151,15 @@ sub VALIDATORS {
 
 use constant UPDATE_VALIDATORS => {
     bug_status => \&_check_bug_status,
+    cclist_accessible   => \&Bugzilla::Object::check_boolean,
+    reporter_accessible => \&Bugzilla::Object::check_boolean,
     resolution => \&_check_resolution,
 };
 
 sub UPDATE_COLUMNS {
     my @columns = qw(
         alias
+        cclist_accessible
         everconfirmed
         bug_file_loc
         bug_severity
@@ -164,6 +167,7 @@ sub UPDATE_COLUMNS {
         op_sys
         priority
         rep_platform
+        reporter_accessible
         resolution
         short_desc
         status_whiteboard
@@ -1207,6 +1211,7 @@ sub _set_global_validator {
 #################
 
 sub set_alias { $_[0]->set('alias', $_[1]); }
+sub set_cclist_accessible { $_[0]->set('cclist_accessible', $_[1]); }
 sub set_custom_field {
     my ($self, $field, $value) = @_;
     ThrowCodeError('field_not_custom', { field => $field }) if !$field->custom;
@@ -1226,6 +1231,7 @@ sub _set_everconfirmed { $_[0]->set('everconfirmed', $_[1]); }
 sub set_op_sys         { $_[0]->set('op_sys',        $_[1]); }
 sub set_platform       { $_[0]->set('rep_platform',  $_[1]); }
 sub set_priority       { $_[0]->set('priority',      $_[1]); }
+sub set_reporter_accessible { $_[0]->set('reporter_accessible', $_[1]); }
 sub set_resolution     { $_[0]->set('resolution',    $_[1]); }
 sub set_severity       { $_[0]->set('bug_severity',  $_[1]); }
 sub set_status { 
diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index 37e4b9349..3da4b9379 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -333,6 +333,12 @@ sub get_all {
     return @$objects;
 }
 
+###############################
+####      Validators     ######
+###############################
+
+sub check_boolean { return $_[1] ? 1 : 0 }
+
 1;
 
 __END__
@@ -679,6 +685,20 @@ be the same as the name of the field in L</VALIDATORS>, if it exists there.
 
 =back
 
+=head2 Simple Validators
+
+You can use these in your subclass L</VALIDATORS> or L</UPDATE_VALIDATORS>.
+Note that you have to reference them like C<\&Bugzilla::Object::check_boolean>,
+you can't just write C<\&check_boolean>.
+
+=over
+
+=item C<check_boolean>
+
+Returns C<1> if the passed-in value is true, C<0> otherwise.
+
+=back
+
 =head1 CLASS FUNCTIONS
 
 =over
diff --git a/process_bug.cgi b/process_bug.cgi
index 7773d15e7..8316979b2 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -651,36 +651,27 @@ if ($cgi->param('component') ne $cgi->param('dontchange')) {
     }
 }
 
-# Since aliases are unique (like bug numbers), they can only be changed
-# for one bug at a time. So if we're doing a mass-change, we ignore
-# the alias field.
-if (Bugzilla->params->{"usebugaliases"} && defined $cgi->param('alias')
-    && scalar(@bug_objects) == 1)
-{
-    $bug_objects[0]->set_alias($cgi->param('alias'));
-}
-
-# If the user is submitting changes from show_bug.cgi for a single bug,
-# and that bug is restricted to a group, process the checkboxes that
-# allowed the user to set whether or not the reporter
-# and cc list can see the bug even if they are not members of all groups 
-# to which the bug is restricted.
+# Certain changes can only happen on individual bugs, never on mass-changes.
 if (defined $cgi->param('id')) {
-    my ($havegroup) = $dbh->selectrow_array(
-        q{SELECT group_id FROM bug_group_map WHERE bug_id = ?},
-        undef, $cgi->param('id'));
-    if ( $havegroup ) {
-        foreach my $field ('reporter_accessible', 'cclist_accessible') {
-            if ($bug->check_can_change_field($field, 0, 1, \$PrivilegesRequired)) {
-                DoComma();
-                $cgi->param($field, $cgi->param($field) ? '1' : '0');
-                $::query .= " $field = ?";
-                push(@values, $cgi->param($field));
-            }
-            else {
-                $cgi->delete($field);
-            }
-        }
+    my $bug = $bug_objects[0];
+    
+    # Since aliases are unique (like bug numbers), they can only be changed
+    # for one bug at a time.
+    if (Bugzilla->params->{"usebugaliases"} && defined $cgi->param('alias')) {
+        $bug->set_alias($cgi->param('alias'));
+    }
+
+    # reporter_accessible and cclist_accessible--these are only set if
+    # the user can change them and there are groups on the bug.
+    # (If the user can't change the field, the checkboxes don't appear
+    #  on show_bug, thus it would look like the user was trying to
+    #  uncheck them, which would then be denied by the set_ functions,
+    #  throwing a confusing error.)
+    if (scalar @{$bug->groups}) {
+        $bug->set_cclist_accessible($cgi->param('cclist_accessible'))
+            if $bug->check_can_change_field('cclist_accessible', 0, 1);
+        $bug->set_reporter_accessible($cgi->param('reporter_accessible'))
+            if $bug->check_can_change_field('reporter_accessible', 0, 1);
     }
 }
 
@@ -1377,6 +1368,7 @@ foreach my $id (@idlist) {
             # Bugzilla::Bug does these for us already.
             next if grep($_ eq $col, qw(keywords op_sys rep_platform priority
                                         bug_severity short_desc alias
+                                        reporter_accessible cclist_accessible
                                         status_whiteboard bug_file_loc),
                                      Bugzilla->custom_field_names);
 
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 7eac20cf2..072a10a56 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -651,10 +651,10 @@
     [% title = "Not allowed" %]
     You tried to change the 
     <strong>[% field_descs.$field FILTER html %]</strong> field 
-    [% IF oldvalue %]
+    [% IF oldvalue.defined %]
       from <em>[% oldvalue FILTER html %]</em>
     [% END %]
-    [% IF newvalue %]
+    [% IF newvalue.defined %]
       to <em>[% newvalue FILTER html %]</em>
     [% END %]
     , but only