234530 - Let users change the status of a bug when adding an attachment

Status: ASSIGNED

(Bugzilla :: Attachments & Requests, enhancement, P3)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

since the last bugzilla upgrade on bmo, "create attachment" has a "take bug"
option if the bug is not assigned to onself, which is nice.

What I would like to see, though, is an "Accept bug" option when the bug is
already assigned to me, but in NEW state. This should mark the bug ASSIGNED.

Comment 1

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 2

[Frédéric Buclin]

*** Bug 348629 has been marked as a duplicate of this bug. ***

Comment 3

[Reed Loden [:reed]]

Attachment: implement comment #0 - v1

Reuse the current takebug code, but just show different wording ("take bug" versus "accept bug").

Comment 4

[Reed Loden [:reed]]

Attachment: implement comment #0 - v2

Oops, forgot to change something. :)

Comment 5

[Frédéric Buclin]

Comment on attachment 241028 [details] [diff] [review]
implement comment #0 - v2

>Index: template/en/default/attachment/create.html.tmpl

>-    [% IF (user.id != bugassignee_id) AND user.groups.editbugs %]
>+    [% IF ((user.id != bugassignee_id) OR ((user.id == bugassignee_id) AND (bugstatus == "NEW"))) AND user.groups.editbugs %]

If the bug is in the UNCONFIRMED state, you should still be allowed to change its status to ASSIGNED as users with editbugs privs can confirm bugs anyway.


>         <th>Reassignment:</th>
>         <td>
>           <em>If you want to assign this [% terms.bug %] to yourself,

This message is confusing if the bug is already assigned to you. Maybe should we have a separate IF block for the case where you already own the bug.

[% IF I'm not the current assignee %]
    ...
[% ELSIF I'm the current assignee and I have editbugs privs and the bug status is UNCO or NEW %]
    ...
[% END %]

Comment 6

[Frédéric Buclin]

reed, if you want this bug fixed for 3.0, you have to hurry up. :)

Comment 7

[Frédéric Buclin]

Morphing this bug to let users change the status of the bug independently of the current status of the bug and whether the user is the assignee or not. Now that Bugzilla 3.2 has custom bug statuses, ASSIGNED is no longer hardcoded, making this RFE irrelevant as is.

reed, do you still plan to fix it? The code in attachment.cgi is much easier to use now, and all you need to do is to decouple "takebug" from editing the bug status. If you don't plan to fix it in the coming month, please reassign the bug to me.

Comment 10

[Reed Loden [:reed]]

I wish I had more time to fix things like this, but I just don't have the time to get myself up to speed with 3.2 to fix this right now. If you could take it, that would be great.

Comment 11

[Frédéric Buclin]

too late for 3.4.

Comment 12

[Reed Loden [:reed]]

Attachment: patch - v3

Untested patch. Will request review once I test.

Comment 13

[Frédéric Buclin]

Too late for 3.8. We are frozen.

Comment 14

[Reed Loden [:reed]]

Attachment: patch - v4

Had to change attachment.cgi some to get this work. Tests ok, though.

Comment 15

[Max Kanat-Alexander]

Comment on attachment 452393 [details] [diff] [review]
patch - v4

pyrzak: Would you do a UI review on this before I do a code review?

Comment 16

[Guy Pyrzak]

Comment on attachment 452393 [details] [diff] [review]
patch - v4

>=== modified file 'attachment.cgi'
>--- attachment.cgi	2010-06-03 19:18:43 +0000
>+++ attachment.cgi	2010-06-19 00:13:21 +0000
>@@ -504,23 +504,28 @@
>                                   type => CMT_ATTACHMENT_CREATED,
>                                   extra_data => $attachment->id });
> 
>-  # Assign the bug to the user, if they are allowed to take it
>   my $owner = "";
>-  if ($cgi->param('takebug') && $user->in_group('editbugs', $bug->product_id)) {
>-      # When taking a bug, we have to follow the workflow.
>+  if ($user->in_group('editbugs', $bug->product_id))) {
>+      # Assign the bug to the user, if they are allowed to take it
>+      my $takebug = $cgi->param('takebug');
>+      if ($takebug) {
>+          # Make sure the person we are taking the bug from gets mail.
>+          $owner = $bug->assigned_to->login;
>+          $bug->set_assigned_to($user);
>+      }
>+      # Change the bug's status if requested status is different than current
>       my $bug_status = $cgi->param('bug_status') || '';
>-      ($bug_status) = grep {$_->name eq $bug_status} @{$bug->status->can_change_to};
>+      if ($bug_status && ($takebug || $user->id == $bug->assigned_to->id)) {
>+          # When taking a bug, we have to follow the workflow.
>+          ($bug_status) = grep {$_->name eq $bug_status} @{$bug->status->can_change_to};
> 
>-      if ($bug_status && $bug_status->is_open
>-          && ($bug_status->name ne 'UNCONFIRMED' 
>-              || $bug->product_obj->allows_unconfirmed))
>-      {
>-          $bug->set_bug_status($bug_status->name);
>-          $bug->clear_resolution();
>+          if ($bug_status && $bug_status->is_open
>+              && ($bug_status->name ne 'UNCONFIRMED' 
>+                  || $bug->product_obj->allows_unconfirmed)) {
>+              $bug->set_bug_status($bug_status->name);
>+              $bug->clear_resolution();
>+          }
>       }
>-      # Make sure the person we are taking the bug from gets mail.
>-      $owner = $bug->assigned_to->login;
>-      $bug->set_assigned_to($user);
>   }
>   $bug->update($timestamp);
> 
>
>=== modified file 'template/en/default/attachment/create.html.tmpl'
>--- template/en/default/attachment/create.html.tmpl	2010-03-03 06:13:28 +0000
>+++ template/en/default/attachment/create.html.tmpl	2010-06-19 00:13:21 +0000
>@@ -67,32 +67,50 @@
>       </td>
>     </tr>
> 
>-    [% IF (user.id != bug.assigned_to.id) AND user.in_group("editbugs", bug.product_id) %]
>-      <tr>
>-        <th>Reassignment:</th>
>-        <td>
>-          <em>If you want to assign this [% terms.bug %] to yourself,
>-              check the box below.</em><br>
>-          <input type="checkbox" id="takebug" name="takebug" value="1">
>-          <label for="takebug">take [% terms.bug %]</label>
>-          [% bug_statuses = [] %]
>-          [% FOREACH bug_status = bug.status.can_change_to %]
>-            [% NEXT IF bug_status.name == "UNCONFIRMED" 
>-                       && !bug.product_obj.allows_unconfirmed %]
>-            [% bug_statuses.push(bug_status) IF bug_status.is_open %]
>-          [% END %]
>-          [% IF bug_statuses.size %]
>-            <label for="takebug">and set the [% terms.bug %] status to</label>
>-            <select id="bug_status" name="bug_status">
>-              <option value="[% bug.status.name FILTER html %]">[% display_value("bug_status", bug.status.name) FILTER html %] (current)</option>
>-              [% FOREACH bug_status = bug_statuses %]
>-                [% NEXT IF bug_status.id == bug.status.id %]
>-                <option value="[% bug_status.name FILTER html %]">[% display_value("bug_status", bug_status.name) FILTER html %]</option>
>-              [% END %]
>-            </select>
>-          [% END %]
>-        </td>
>-      </tr>
>+    [% IF user.in_group("editbugs", bug.product_id) %]
>+      [% bug_statuses = [] %]
>+      [% FOREACH bug_status = bug.status.can_change_to %]
>+         [% NEXT IF bug_status.name == "UNCONFIRMED" 
>+                    && !bug.product_obj.allows_unconfirmed %]
>+         [% bug_statuses.push(bug_status) IF bug_status.is_open %]
>+      [% END %]
>+      [% IF (user.id != bug.assigned_to.id) %]
>+        <tr>
>+          <th>Reassignment:</th>
>+          <td>
>+            <em>If you want to assign this [% terms.bug %] to yourself,
>+                check the box below.</em><br>
>+            <input type="checkbox" id="takebug" name="takebug" value="1">
>+            <label for="takebug">take [% terms.bug %]</label>
>+            [% IF bug_statuses.size %]
>+              <label for="takebug">and set the [% terms.bug %] status to</label>
>+              <select id="bug_status" name="bug_status">
>+                <option value="[% bug.status.name FILTER html %]">[% display_value("bug_status", bug.status.name) FILTER html %] (current)</option>
>+                [% FOREACH bug_status = bug_statuses %]
>+                  [% NEXT IF bug_status.id == bug.status.id %]
>+                  <option value="[% bug_status.name FILTER html %]">[% display_value("bug_status", bug_status.name) FILTER html %]</option>
>+                [% END %]
>+              </select>
>+            [% END %]
>+          </td>
>+        </tr>
>+      [% ELSE %]
>+        [% IF bug_statuses.size %]
>+          <tr>
>+            <th>Status:</th>
>+            <td>
>+              <label for="bug_status">Set the [% terms.bug %] status to</label>
>+              <select id="bug_status" name="bug_status">
>+                <option value="[% bug.status.name FILTER html %]">[% display_value("bug_status", bug.status.name) FILTER html %] (current)</option>
>+                [% FOREACH bug_status = bug_statuses %]
>+                  [% NEXT IF bug_status.id == bug.status.id %]
>+                  <option value="[% bug_status.name FILTER html %]">[% display_value("bug_status", bug_status.name) FILTER html %]</option>
>+                [% END %]
>+              </select>
>+            </td>
>+          </tr>
>+        [% END %]
>+      [% END %]
>     [% END %]
>     <tr>
>       <th><label for="comment">Comment:</label></th>
>

Comment 17

[Guy Pyrzak]

Comment on attachment 452393 [details] [diff] [review]
patch - v4

Ok 2nd try...

So I looked at the existing create UI and it seems to follow the following pattern.

Label followed by some help text, new line, form widget. We should update the old UI and the new UI to follow this layout since I do think it is helpful for new users and is easy for advanced user to know where to click (down one column).

So here is what i propose.

Reassignment:	If you want to assign this bug to yourself, check the box below.
                [checkbox] Assign this bug to me ([e-mail])

Status:  If you want to change the status of bug 1111, change the field below:
         [select] 

I think this UI should be used for both situations, where the re-assignment happens as well as when there is no-reassignment. This consistency should make the ui easier as well simplify the code. This is obviously just an opinion and just my attempts at maintaining consistency on the create page. Feel free to play around with the help text to ensure it make sense and looks right.

Also there is a bug in the CGI 
if ($user->in_group('editbugs', $bug->product_id)))

one too many close parens

Comment 18

[Max Kanat-Alexander]

Comment on attachment 452393 [details] [diff] [review]
patch - v4

  Also, there's no need to do those special checks and the clear_resolution anymore--instead, you can just set the status and Bugzilla::Bug will do the right thing. Also, I'd recommend using set_all if possible, which will assure that security is correct and that updates are done in the right order.

  Also, it would be best to use field.html.tmpl for bug_status, and use override_legal_values to change the value list.

Comment 19

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.