116819 - Attach and Reassign in one fell swoop

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement, P1)

Description

[Jacob Steenhagen]

Was talking to timeless this evening and he suggested that one of the options on
the create attachment page should be something along the lines of:

[ ] Reassign this bug to me

as often times when you're attaching a patch, you also want to take ownership of
the bug.  I think that this should automatically set the bug's status to
ASSIGNED.  And of course, it should only be an option for those with EditBugs
(ie, those that could assign it to themselves anyway).

Comment 1

[Matthew T (active 1999-2002)]

I think this is effectively a duplicate of bug 37008.

Comment 2

[Gervase Markham [:gerv]]

Now this _is_ a good idea.

Gerv

Comment 3

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This should do the trick. I think this would be a much-used shortcut.

Gerv

Comment 4

[Jacob Steenhagen]

The code looks strait forward, but I haven't tested it yet... I'll try to get to
that this evening (don't have time right now).

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 99751 [details] [diff] [review]
Patch v.1

Oooh, great idea.

>Index: attachment.cgi

>+  SendSQL("SELECT short_desc, assigned_to FROM bugs 
>+           WHERE bug_id = $::FORM{'bugid'}");

Nit: avoid line breaks in SQL statements (so they appear nicely in logs).


>+  $vars->{'bugassignee_id'} = $assignee_id;
>+  $vars->{'bugsummary'} = $bugsummary;

Note to self and others: clean up attachment.cgi/pm interfaces.


>+      SendSQL("UPDATE bugs SET assigned_to = $vars->{'user'}{'userid'}, 
>+               bug_status = 'ASSIGNED', resolution = '' 
>+               WHERE bug_id = $::FORM{'bugid'}");

Same nit here.

You are missing the adding of entries to the bugs activity table to reflect
these changes.


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

>+        <th>Reassignment:</th>
>+        <td>
>+          <em>If you want to assign this bug to yourself, 
>+              check the box below.</em><br>
>+          <input type="checkbox" id="takebug" name="takebug" value="1"> 
>+          <label for="takebug">take bug</label>

Nit: "take" is funny when "reassign" was used before, and the whole thing is a
bit wordy, but so is everything else on that form, so I guess it's consistent.

Comment 6

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Nothing's as simple as it seems, is it? :-) Try this.

Gerv

Comment 7

[Joel Peshkin]

Comment on attachment 100099 [details] [diff] [review]
Patch v.2


Software error:

Undefined subroutine &main::GetSQLData called at
/home/bugzilla/tip/attachment.cgi line 487, <STDIN> line 306.

Comment 8

[Gervase Markham [:gerv]]

Attachment: Patch v.3

Joel: try this :-)

Gerv

Comment 9

[Gervase Markham [:gerv]]

CCing Joel.

Gerv

Comment 10

[Christian Reis]

Comment on attachment 102551 [details] [diff] [review]
Patch v.3

>+  # Retrieve the bug summary (for displaying on screen) and assignee.
>+  SendSQL("SELECT short_desc, assigned_to FROM bugs 
>+           WHERE bug_id = $::FORM{'bugid'}");

You missed a line-break-in-SQL here, as per myk's nit.

>+      SendSQL("select NOW()");

I forget, what is this for again?

>+        <th>Reassignment:</th>
>+        <td>
>+          <em>If you want to assign this bug to yourself, 
>+              check the box below.</em><br>
>+          <input type="checkbox" id="takebug" name="takebug" value="1"> 
>+          <label for="takebug">take bug</label>
>+        </td>
>+      </tr>
>+    [% END %]

This should really be a single line saying:

  [ ] Reassign this bug to me

It's less text on an already busy column, and the text to the checkbox is clear
enough for anybody to understand what it means. Thanks for using label, btw.

The only word I am unsure of is "me", since I'm not sure it is used anywhere
else. But it is better than "you" at any rate.

Comment 11

[Gervase Markham [:gerv]]

> >+      SendSQL("select NOW()");
> 
> I forget, what is this for again?

It's for timestamping the bugs_activity entry.

> This should really be a single line saying:
> 
>   [ ] Reassign this bug to me

It's done the way it is for consistency with the format of the other things on
that page. As Myk says: "the whole thing is a bit wordy, but so is everything
else on that form, so I guess it's consistent."

New patch this evening.

Gerv

Comment 12

[Gervase Markham [:gerv]]

Attachment: Patch v.4

Fixed nit. Looking for r= from kiko.

Gerv

Comment 13

[Gervase Markham [:gerv]]

kiko: ping? :-)

Gerv

Comment 14

[Dave Miller [:justdave]]

Comment on attachment 102622 [details] [diff] [review]
Patch v.4

needs-work: The original assignee is not pushed on the forcecc stack when
calling processmail, so the original assignee never receives mail that the bug
has been taken from them.

Comment 15

[Dave Miller [:justdave]]

other than that this looks and performs beautifully, so if you can fix the
emailing to the original assignee, I think this is good to go.

Comment 16

[Dave Miller [:justdave]]

I take that back, found one more thing.

The activity table has the user IDs of the old and new assignee instead of their
email address.

Comment 17

[Dave Miller [:justdave]]

OK, maybe I should have finished testing before I posted. :)

Here's the entire list, for review (including the new stuff I just found) :

1. Original assignee doesn't receive email that they were removed from assigneeship
2. Original and new assignee are listed in the activity log as user IDs instead
of email addresses
3. Old status is |NEW| and new status is |'ASSIGNED'| in the activity log. 
Notice the single-quotes around it.
4. Resolution is listed as changing from || to ||.  (i.e. it didn't change - but
it's listed in the activity log anyway)

For most of the above reasons (2,3,4) shouldn't we be abstracting out the
activity log code from process_bug.cgi and using the same code from both places
a) to prevent stuff like this, and b) to maintain future compatibility in case
activity logging changes in the future?

Comment 18

[Gervase Markham [:gerv]]

Attachment: Patch v.5

I think this fixes all the issues Dave noticed. Yes, the activity logging
should be centralised - but probably when we do everything via a Bug.pm, where
it can all be done behind the scenes when you call $bug->write() :-)

Gerv

Comment 19

[Gervase Markham [:gerv]]

Accepting.

Gerv

Comment 20

[Gervase Markham [:gerv]]

Dave: ping? You have a long-standing review request here...

Gerv

Comment 21

[Dave Miller [:justdave]]

and I had a long-standing deadline to meet yesterday, too. :)  I still have a
few loose ends to tie up with that before Friday, so I may or may not get to
this before then (I probably will, not much left on my rush list), but this one
is at the top of my queue, so you're first :)

Comment 22

[Dave Miller [:justdave]]

Comment on attachment 105169 [details] [diff] [review]
Patch v.5

r= justdave

Everything seems to work now :)

Comment 23

[Dave Miller [:justdave]]

still applies to the tip without conflicts, too :)

Comment 24

[Gervase Markham [:gerv]]

Fixed.

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.35; previous revision: 1.34
done
Checking in template/en/default/attachment/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/create.html.tmpl,v
 <--  create.html.tmpl
new revision: 1.13; previous revision: 1.12
done

Gerv