Closed Bug 684171 Opened 9 years ago Closed 8 years ago

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

Categories

(Bugzilla :: Creating/Changing Bugs, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 4.4

People

(Reporter: asa, Assigned: dkl)

References

Details

Attachments

(1 file, 3 obsolete files)

+++ 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.
Severity: normal → enhancement
Assignee: create-and-change → dkl
Status: NEW → ASSIGNED
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
Attachment #557930 - Flags: review?(LpSolit)
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.
(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 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.
Attachment #557930 - Flags: review?(LpSolit) → review-
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
Attachment #557930 - Attachment is obsolete: true
Attachment #577192 - Flags: review?(LpSolit)
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.
Attachment #577192 - Flags: review?(LpSolit) → review-
New patch that simplifies the javascript needed as well as addresses your suggested changes.

dkl
Attachment #577192 - Attachment is obsolete: true
Attachment #628879 - Flags: review?(LpSolit)
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.
Attachment #628879 - Flags: review?(LpSolit) → review-
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
Attachment #628879 - Attachment is obsolete: true
Attachment #632120 - Flags: review?(LpSolit)
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.
Attachment #632120 - Flags: review?(LpSolit) → review+
Flags: approval+
Keywords: relnote
Target Milestone: --- → Bugzilla 4.4
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://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified js/field.js
modified template/en/default/bug/edit.html.tmpl
Committed revision 8319
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Added to relnotes for 4.4.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.