Bug 294021: Allow requestees to set attachment flags even if they don't have editbugs...

r=gerv a=justdave

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index c90d8ea8e4a9fc6415d810030ec24d57a0a54f45..8aa2c3c63087b0103b2a81ec24852925bd428641 100644 (file)
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -700,28 +700,27 @@ sub get_attachments_by_bug {
 
 =pod
 
-=item C<validate_can_edit($attachment, $product_id)>
+=item C<validate_can_edit>
 
 Description: validates if the user is allowed to view and edit the attachment.
              Only the submitter or someone with editbugs privs can edit it.
              Only the submitter and users in the insider group can view
              private attachments.
 
-Params:      $attachment - the attachment object being edited.
-             $product_id - the product ID the attachment belongs to.
+Params:      none
 
 Returns:     1 on success, 0 otherwise.
 
 =cut
 
 sub validate_can_edit {
-    my ($attachment, $product_id) = @_;
+    my $attachment = shift;
     my $user = Bugzilla->user;
 
     # The submitter can edit their attachments.
     return ($attachment->attacher->id == $user->id
             || ((!$attachment->isprivate || $user->is_insider)
-                 && $user->in_group('editbugs', $product_id))) ? 1 : 0;
+                 && $user->in_group('editbugs', $attachment->bug->product_id))) ? 1 : 0;
 }
 
 =item C<validate_obsolete($bug, $attach_ids)>
@@ -758,7 +757,7 @@ sub validate_obsolete {
           || ThrowUserError('invalid_attach_id', $vars);
 
         # Check that the user can view and edit this attachment.
-        $attachment->validate_can_edit($bug->product_id)
+        $attachment->validate_can_edit
           || ThrowUserError('illegal_attachment_edit', { attach_id => $attachment->id });
 
         if ($attachment->bug_id != $bug->bug_id) {

diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index 177857adaca5b106f3e283074c6c2623a219e9e4..ff9d236db6cd05ada74a2a6d3d5e75dff10cf9f8 100644 (file)
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -825,7 +825,7 @@ sub extract_flags_from_cgi {
     # Extract a list of existing flag IDs.
     my @flag_ids = map(/^flag-(\d+)$/ ? $1 : (), $cgi->param());
 
-    return () if (!scalar(@flagtype_ids) && !scalar(@flag_ids));
+    return ([], []) unless (scalar(@flagtype_ids) || scalar(@flag_ids));
 
     my (@new_flags, @flags);
     foreach my $flag_id (@flag_ids) {

diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm
index 09f6e1adc36ba3f039eba2fbb4d13c33b660ac8d..3af8169b44fda46ee1615c78790c787cfec9185f 100644 (file)
--- a/Bugzilla/WebService/Bug.pm
+++ b/Bugzilla/WebService/Bug.pm
@@ -852,7 +852,7 @@ sub update_attachment {
           || ThrowUserError("invalid_attach_id", { attach_id => $id });
         my $bug = $attachment->bug;
         $attachment->_check_bug;
-        $attachment->validate_can_edit($bug->product_id)
+        $attachment->validate_can_edit
           || ThrowUserError("illegal_attachment_edit", { attach_id => $id });
 
         push @attachments, $attachment;

diff --git a/attachment.cgi b/attachment.cgi
index 5a0d6f9fbf29dff2bf971455290701e90529e226..94510fb190f4f178ea3ded10138819b9fdacbdf2 100755 (executable)
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -626,7 +626,7 @@ sub update {
     my $attachment = validateID();
     my $bug = $attachment->bug;
     $attachment->_check_bug;
-    my $can_edit = $attachment->validate_can_edit($bug->product_id);
+    my $can_edit = $attachment->validate_can_edit;
 
     if ($can_edit) {
         $attachment->set_description(scalar $cgi->param('description'));
@@ -680,11 +680,33 @@ sub update {
 
     $bug->add_cc($user) if $cgi->param('addselfcc');
 
+    my ($flags, $new_flags) =
+      Bugzilla::Flag->extract_flags_from_cgi($bug, $attachment, $vars);
+
     if ($can_edit) {
-        my ($flags, $new_flags) =
-          Bugzilla::Flag->extract_flags_from_cgi($bug, $attachment, $vars);
         $attachment->set_flags($flags, $new_flags);
     }
+    # Requestees can set flags targetted to them, even if they cannot
+    # edit the attachment. Flag setters can edit their own flags too.
+    elsif (scalar @$flags) {
+        my %flag_list = map { $_->{id} => $_ } @$flags;
+        my $flag_objs = Bugzilla::Flag->new_from_list([keys %flag_list]);
+
+        my @editable_flags;
+        foreach my $flag_obj (@$flag_objs) {
+            if ($flag_obj->setter_id == $user->id
+                || ($flag_obj->requestee_id && $flag_obj->requestee_id == $user->id))
+            {
+                push(@editable_flags, $flag_list{$flag_obj->id});
+            }
+        }
+
+        if (scalar @editable_flags) {
+            $attachment->set_flags(\@editable_flags, []);
+            # Flag changes must be committed.
+            $can_edit = 1;
+        }
+    }
 
     # Figure out when the changes were made.
     my $timestamp = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');

diff --git a/template/en/default/flag/list.html.tmpl b/template/en/default/flag/list.html.tmpl
index 8acef56317fe6ed19082d89882406d1f13324bc9..47406d6c035b7e39106aa279b11373fdad31b88c 100644 (file)
--- a/template/en/default/flag/list.html.tmpl
+++ b/template/en/default/flag/list.html.tmpl
@@ -6,7 +6,7 @@
   # defined by the Mozilla Public License, v. 2.0.
   #%]
 
-[% IF user.id && !read_only_flags && (!bug || bug.check_can_change_field('flagtypes.name', 0, 1)) %]
+[% IF user.id && (!bug || bug.check_can_change_field('flagtypes.name', 0, 1)) %]
 
 [%# We list flags by looping twice over the flag types relevant for the bug.
   # In the first loop, we display existing flags and then, for active types,
@@ -41,7 +41,9 @@
     [% FOREACH flag = type.flags %]
       [% PROCESS flag_row flag = flag type = type %]
     [% END -%]
+
     [% SET flag = "" %]
+    [% NEXT IF read_only_flags %]
 
     [%-# Step 1b: Display UI for setting flag. %]
     [% IF (!type.flags || type.flags.size == 0) && type.is_active %]
@@ -49,16 +51,18 @@
     [% END %]
   [% END %]
 
-  [%# Step 2: Display flag type again (if type is multiplicable). %]
-  [% FOREACH type = flag_types %]
-    [% NEXT UNLESS type.flags && type.flags.size > 0 && type.is_multiplicable && type.is_active %]
-    [% IF !separator_displayed %]
-      <tbody class="bz_flag_type">
-        <tr><td colspan="3"><hr></td></tr>
-      </tbody>
-      [% separator_displayed = 1 %]
+  [% IF !read_only_flags %]
+    [%# Step 2: Display flag type again (if type is multiplicable). %]
+    [% FOREACH type = flag_types %]
+      [% NEXT UNLESS type.flags && type.flags.size > 0 && type.is_multiplicable && type.is_active %]
+      [% IF !separator_displayed %]
+        <tbody class="bz_flag_type">
+          <tr><td colspan="3"><hr></td></tr>
+        </tbody>
+        [% separator_displayed = 1 %]
+      [% END %]
+      [% PROCESS flag_row type = type addl_text = "addl." %]
     [% END %]
-    [% PROCESS flag_row type = type addl_text = "addl." %]
   [% END %]
 </table>
 
@@ -93,6 +97,7 @@
 [% BLOCK flag_row %]
   [% RETURN IF !flag && !((type.is_requestable && user.can_request_flag(type)) || user.can_set_flag(type)) %]
   [% SET fid = flag ? "flag-$flag.id" : "flag_type-$type.id" %]
+  [% can_edit_flag = (!read_only_flags || (flag && (flag.setter_id == user.id || (flag.requestee_id && flag.requestee_id == user.id)))) ? 1 : 0 %]
   <tbody[% ' class="bz_flag_type"' IF !flag %]>
     <tr>
       <td>
@@ -111,12 +116,13 @@
         <select id="[% fid FILTER html %]" name="[% fid FILTER html %]"
                 title="[% type.description FILTER html %]"
                 onchange="toggleRequesteeField(this);"
-                class="flag_select flag_type-[% type.id %]">
+                class="flag_select flag_type-[% type.id %]"
+                [% IF !can_edit_flag %] disabled="disabled"[% END %]>
         [%# Only display statuses the user is allowed to set. %]
-        [% IF !flag || user.can_request_flag(type) || flag.setter_id == user.id %]
+        [% IF !flag || (can_edit_flag && user.can_request_flag(type)) || flag.setter_id == user.id %]
           <option value="X"></option>
         [% END %]
-        [% IF type.is_active %]
+        [% IF type.is_active && can_edit_flag %]
           [% IF (type.is_requestable && user.can_request_flag(type)) || (flag && flag.status == "?") %]
             <option value="?" [% "selected" IF flag && flag.status == "?" %]>?</option>
           [% END %]
@@ -136,12 +142,13 @@
           [% IF (type.is_active && type.is_requestable && type.is_requesteeble) || (flag && flag.requestee) %]
               [% SET grant_list = [] %]
               [% IF Param('usemenuforusers') %]
-                [% grant_list = type.grant_list %]
-                [% IF flag && !(type.is_active && type.is_requestable && type.is_requesteeble) %]
+                [% IF !can_edit_flag || (flag && !(type.is_active && type.is_requestable && type.is_requesteeble)) %]
                   [%# We are here only because there was already a requestee. In this case,
                       the only valid action is to remove the requestee or leave it alone;
                       nothing else. %]
                   [% grant_list = [flag.requestee] %]
+                [% ELSE %]
+                  [% grant_list = type.grant_list %]
                 [% END %]
               [% END %]
               [% SET flag_name = flag ? "requestee-$flag.id" : "requestee_type-$type.id" %]
@@ -156,6 +163,7 @@
                          emptyok  => flag_empty_ok
                          classes  => ["requestee"]
                          custom_userlist => grant_list
+                         disabled => !can_edit_flag
               %]
           [% END %]
         </td>