684171 - add a "(take)" link to the QA Contact field

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Asa Dotzler [:asa]]

+++ This bug was initially created as a clone of Bug #684088 +++

Because we'd like to reclaim the QA Contact field in bmo for QA instead of component watching, we would like to havea "(take)" link for the QA Contact field just like the Assignee field has.

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to add take action link for qa contact field (v1)

Patch adds a (take) link similar to the assignee field that a user can click to add their account login to the qa contact field.

dkl

Comment 2

[Frédéric Buclin]

Just to be sure, why is bug filed in the Bugzilla product? We have no such link upstream for the assignee, so it doesn't make sense to add one for the QA contact.

Comment 3

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #2)
> Just to be sure, why is bug filed in the Bugzilla product? We have no such
> link upstream for the assignee, so it doesn't make sense to add one for the
> QA contact.

It is in trunk, bug 626658. If you do not want QA contact link also, thats fine. I can add it to BMO. I just think it woould be a nice to have for upstream as well and to be consistent with the assignee field.

dkl

Comment 4

[Frédéric Buclin]

Comment on attachment 557930 [details] [diff] [review]
Patch to add take action link for qa contact field (v1)

When a bug has no QA contact, the (take) link is not displayed as the field is already displayed as a text field. This forces the user to type his email address.

Comment 5

[David Lawrence [:dkl]]

Attachment: Patch to add take action link for qa contact field (v2)

Thanks for the review. This new patch adds a take link also when the qa contact field is empty which fills in the current user's login.

dkl

Comment 6

[Frédéric Buclin]

Comment on attachment 577192 [details] [diff] [review]
Patch to add take action link for qa contact field (v2)

>+            [% IF bug.qa_contact.id != user.id %]
>+              <span id="bz_qa_contact_take_container">
>+                (<a title="Change qa contact to yourself" 
>+                    href="#" id="bz_qa_contact_take_action">take</a>)
>+              </span>
>+            [% END %]

Why is it not possible to reuse the existing <span></span> define right above it? Wouldn't this let you use hideEditableField() instead of all the JS code you had to add? Also, the title should contain QA uppercase.


>+            [% IF bug.qa_contact.id != user.id %]
>+              <span id="bz_qa_contact_empty_take_container">
>+                (<a title="Change qa contact to yourself" 
>+                    href="#" id="bz_qa_contact_empty_take_action">take</a>)
>+              </span>
>+            [% END %]

Is there no way to avoid to duplicate the (take) link? I think the one above is useless and only this one is useful, right? Also, same comment about QA which should be uppercase in the title.


>+              YAHOO.util.Event.addListener('bz_qa_contact_take_action', 'click', function (e) {
>+                showEditableField(e, new Array('bz_qa_contact_edit_container', 
>+                                  'bz_qa_contact_input', '[% user.login FILTER js %]'));
>+                YAHOO.util.Dom.addClass('bz_qa_contact_empty_take_container', 'bz_default_hidden'); 
>+              });
>+            [% ELSE %]
>+              YAHOO.util.Event.addListener('bz_qa_contact_empty_take_action', 'click', showEditableField, 
>+                                           new Array('bz_qa_contact_empty_take_container', 
>+                                                     'bz_qa_contact_input', 
>+                                                     '[% user.login FILTER js %]'));
>             [% END %]

I think that with only one (take) link, this could be simplified a bit (but I didn't investigate if that's true or not).


Now, there is a problem with your patch. When I click the (edit) link for the QA contact, (take) remains visible. This problem doesn't occur with the assignee field.

Comment 7

[David Lawrence [:dkl]]

Attachment: Patch to add take action link for qa contact field (v3)

New patch that simplifies the javascript needed as well as addresses your suggested changes.

dkl

Comment 8

[Frédéric Buclin]

Comment on attachment 628879 [details] [diff] [review]
Patch to add take action link for qa contact field (v3)

This looks good and I like it much more than the previous patch, but we shouldn't display (take) *above* the text field when there is no QA contact yet. Instead, I suggest we hide the text field in all cases by default (unless JS is turned off, of course), and only display it when the user clicks either (edit) or (take). This way, its behavior would be similar to the assignee field. I know that by default, fields with no value are displayed as text fields, but in this case, I think we should be consistent with the assignee field, which would also solve the problem I mentioned above.

Comment 9

[David Lawrence [:dkl]]

Attachment: Patch to add take action link for qa contact field (v4)

New patch that hides input even when qa contact is empty. User can click on either (edit) or (take) to show the input field.

dkl

Comment 10

[Frédéric Buclin]

Comment on attachment 632120 [details] [diff] [review]
Patch to add take action link for qa contact field (v4)

>-                emptyok => 1

emptyok is required to be able to remove the QA contact when usemenuforusers is turned on.


>-              hideEditableField('bz_qa_contact_edit_container', 
>-                                 ...
>-                                 'qa_contact', 

>+            hideEditableField('bz_qa_contact_edit_container', 
>+                              ...
>+                              '', 

The 4th argument must not be '', it must remain 'qa_contact' else the userlist is not displayed when usemenuforusers is turned on.


r=LpSolit with both changes reverted.

Comment 11

[David Lawrence [:dkl]]

Thanks. I had to make a small change to js/field.js which I should have done before instead of omitting the qa_contact field id to achieve the same result. All works properly.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified js/field.js
modified template/en/default/bug/edit.html.tmpl
Committed revision 8319

Comment 12

[Frédéric Buclin]

Added to relnotes for 4.4.