943146 - Needinfo dropdown should include mentor

Status: VERIFIED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Mentored bugs would really benefit from having the bug's mentor in the needinfo dropdown. The convention we use is to specify the mentor in the whiteboard, ie: [mentor=[e-mail]]

There is the occasional case where the mentor is specified as an IRC nick, but we can just discourage that practice.

Comment 1

[:glob ✱]

while the review extension provides a bug.mentor field which supports both fully qualified and irc nick entries in the mentor field, it can be slow due to the additional searches required, so i'd like to avoid bringing that onto show_bug.

once we have a field for bug mentors (bug 649691), then that no longer becomes a concern.

Comment 2

[David Lawrence [:dkl]]

Attachment: 943146_1.patch

I realize you would rather wait on the proper Mentor extension that brings it's own tables for storing the mentor values, but this particular request was simple enough to implement using current functionality and will be converted automatically once the new extension is complete. bug.mentors currently provided by extensions/BMO/Extension.pm will move to extension/Mentor/Extension.pm and the return data should be the same. Feel free to hold off on this review if you feel like it would be a drain on performance but I thought I would put it out there anyway.

dkl

Comment 3

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #2) 
> bug.mentors
> currently provided by extensions/BMO/Extension.pm will move to
> extension/Mentor/Extension.pm and the return data should be the same.

Actually after thinking about this some more, we can just leave it in the Review extension and add the needed changes there.

dkl

Comment 4

[:glob ✱]

Comment on attachment 8375734 [details] [diff] [review]
943146_1.patch

this _needs_ bug 649691 implemented first, as generating the mentor list right now is expensive.

clearing review until that's ready.

Comment 5

[David Lawrence [:dkl]]

Attachment: 943146_2.patch

Comment 6

[:glob ✱]

Comment on attachment 8442203 [details] [diff] [review]
943146_2.patch

Review of attachment 8442203 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

::: extensions/Needinfo/template/en/default/bug/needinfo.html.tmpl
@@ +100,5 @@
>                  } else if (role == 'user') {
>                    identity = '[% user.realname || user.login FILTER html FILTER js %]';
> +                [% FOREACH mentor = bug.mentors %]
> +                } else if (role == '[% mentor.login FILTER js %]') {
> +                  identity = '[% mentor.realname || mentor.login FILTER html FILTER js %] (mentor)';

nit: indent the code within the FOREACH loop

Comment 9

[:glob ✱]

Comment on attachment 8442203 [details] [diff] [review]
943146_2.patch

Review of attachment 8442203 [details] [diff] [review]:
-----------------------------------------------------------------

i was thinking about this some more, and i think for the common case where there's only one mentor the <select> should say just "mentor".

if there's more than one mentor then it makes sense to use the current implementation and show each mentor's email address.

Comment 10

[David Lawrence [:dkl]]

Attachment: 943146_3.patch

Comment 11

[:glob ✱]

Comment on attachment 8442880 [details] [diff] [review]
943146_3.patch

Review of attachment 8442880 [details] [diff] [review]:
-----------------------------------------------------------------

thanks, that looks much better.
there's some redundancy that needs to be removed and then we should be good to go :)

::: extensions/Needinfo/template/en/default/bug/needinfo.html.tmpl
@@ +100,5 @@
>                  } else if (role == 'user') {
>                    identity = '[% user.realname || user.login FILTER html FILTER js %]';
> +                [% FOREACH mentor = bug.mentors %]
> +                  } else if (role == '[% mentor.login FILTER js %]') {
> +                    identity = '[% mentor.realname || mentor.login FILTER html FILTER js %] (mentor)';

there's no need to append "mentor" if there's only one mentor, because that information is already provided by the selected option.

@@ +141,5 @@
>                <option value="qa_contact">qa contact</option>
>              [% END %]
>              <option value="user">myself</option>
> +            [% FOREACH mentor = bug.mentors %]
> +              <option title="mentor" value="[% mentor.login FILTER html %]">

this tooltip is redundant when there's only one mentor.

Comment 12

[David Lawrence [:dkl]]

Attachment: 943146_4.patch

Comment 13

[:glob ✱]

Comment on attachment 8443504 [details] [diff] [review]
943146_4.patch

Review of attachment 8443504 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

Comment 14

[David Lawrence [:dkl]]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   f16dc32..4e8e717  master -> master

Comment 15

[Tony Mechelynck [:tonymec]]

I hereby VERIFY that the needinfo dropdown includes "mentor" on BMO bugs having a nonempty "mentor" field in the header. Examples: bug 137014, bug 96972