649691 - Add a "mentor" and "mentored bug type" field to b.m.o

Status: RESOLVED FIXED

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

Description

[Paul Biggar]

We've started an informal thing of adding [mentor=pbiggar] to whiteboard status of bugs which have mentors who'd like to help. This is a start, but it would be great to have that be a real field:

- who is "pbiggar"? Why, he's pbiggar on irc, but actually he's [e-mail] on bugzilla. Whoops.

- it would make the search faster

- autocompletition, no typos

Comment 1

[Reed Loden [:reed]]

The use case seems too low for the added UI and issues a new field will cause.

Comment 2

[Paul Biggar]

I didn't know there were issues, and it seemed like if we had space for things like in-litmus, we would have space for this.

However, I see your point: [mentor=name] is a new thing, and hasn't had many people taking it up yet (esp. since we havent actually announced it). Hopefully once people start using it, we can revisit this issue.

Comment 3

[Reed Loden [:reed]]

in-litmus is a flag... We could create this as a flag, though it generally has nothing to do with how flags work. Would that work?

Comment 4

[Paul Biggar]

I dont know. I'm not sure what the semantics of that flag would be?

Comment 5

[David Lawrence [:dkl]]

(In reply to comment #2)
> I didn't know there were issues, and it seemed like if we had space for things
> like in-litmus, we would have space for this.
> 
> However, I see your point: [mentor=name] is a new thing, and hasn't had many
> people taking it up yet (esp. since we havent actually announced it). Hopefully
> once people start using it, we can revisit this issue.

I think Reed was referring to creating a new "custom" field to satisfy this request which we try to keep away from doing except for very critical needs. in-litmus is actually a Bugzilla flag (?, +, and - values only) which we add fairly easily for different needs. I am not sure if a flag will work for you though since you need to tie to a particular Bugzilla user. Although when a flag is set to ? it can have a value in the requestee field which could work.

If you just need to mark a bug that doesn't require a username, a MentorNeeded or similar Keyword could be added.

Dave

Comment 6

[Paul Biggar]

So a |mentor| flag would have 5 states (unset, ?, +, -, and ?username). I can find a meaning for two of those states:

 mentor? - can I get a mentor for this (which seems like a good idea)
 mentor?username - which is a bit of a weird way to express what we want.

The other 3 states wouldn't mean anything, which would add to the confusion.

So unless I'm wrong about the above, I'm thinking a flag isn't suitable.

Comment 7

[David Lawrence [:dkl]]

True it seems flags may be too limited especially for the case where a bug has an accepted mentor already and you want a way to show that person's name.

I suppose the only real choices then would be:

1. Overload a user field such as QA contact to mean the Mentor's name.
2. Continue to use the status whiteboard as a free form field to do [mentor=username]
3. Or add a free text custom field called Mentor that can start any arbitrary text such as a username or other word to mean a mentor is needed.

1 and 2 are doable already without any admin intervention. Number 3 will need some pre-planning and might even mean some BMO downtime do to the pains we have
had lately in adding new custom fields.

dkl

Comment 8

[Gervase Markham [:gerv]]

Overloading QA might become more possible once we deploy Component Watching, because then people will not need to watch generic QA contacts. And it's good to have the mentor in a position where he is CCed on all the bugmail.

I think you would need to demonstrate wide use in e.g. the status whiteboard before we would add a custom field. This is not something we do lightly, because every Bugzilla user has to cope with the added complexity.

Gerv

Comment 9

[David Lawrence [:dkl]]

Paul, sorry for letting this fall to the wayside. Do you still consider this an issue? Component Watching has widely replaced use of the qa contact field so that field is now more free for general use if that would work. Of course QA may want to use that field on certain bugs for their purposes. Let me know if this still requires more discussion.

Comment 10

[Josh Matthews [:jdm]]

Paul's not involved any more, so I can take this up. I think a real mentor field would be very useful, and it would be clearer to new contributors what it mean than the current whiteboard tag. It would also be clearer than repurposing the QA field.

Comment 11

[David Lawrence [:dkl]]

(In reply to Josh Matthews [:jdm] from comment #10)
> Paul's not involved any more, so I can take this up. I think a real mentor
> field would be very useful, and it would be clearer to new contributors what
> it mean than the current whiteboard tag. It would also be clearer than
> repurposing the QA field.

One of the BMO team's goals is to move the current tracking flags (which are custom fields on the back end) to their own tables. This will give us considerably more ability to add custom fields like this once that is done. We are planning the migration in the near future so we can revisit this issue one then. 

One concern is we will need to configure the mentor field to only be visible for specific products as it may not be useful globally.

dkl

Comment 12

[Benjamin Smedberg]

As discussed in private email, the coding contribution team would like to make this item a priority. We've been using the whiteboard for both the mentor and the mentoring type, and there are some limitations that make actual fields advisable in this case. In particular, it is difficult for new users to map "mentor=nickname" to the actual person who has agreed to do the mentoring. And secondly, we want to provide a more discrete list of mentored bug types for mentors to choose from:

* Good first bug
* Good next bug
* Student project
* Experience required

Per email discussion, this will wait until bug 750742 is complete.

Comment 13

[Mike Hoye [:mhoye]]

"Good first bug" and "mentored bug" should be distinct things, and my hope is that completion of GFB be considered a prerequisite for a mentored bug. One of the main frustrations our engineers report with the mentoring process is having to handhold people through the dev-environment setup, and I agree that while this is something we can simplify, it's also something prospective mentorees should be able to spin up on their own before we can take them on.

I have this list as:

* Good first bug
* Good next bug
* Mentored bug
* Student bug
* Student project
* Experience required

Thanks.

Comment 14

[Benjamin Smedberg]

mhoye, we should talk again ;-) Because I don't think that "mentored bug" or "student bug" really ought to be separate categories here.

Comment 15

[Mike Hoye [:mhoye]]

The distinction only matters because of timing. 

A mentored bug doesn't necessarily have a schedule or deadline associated with it, but a student will probably need to get their thing done within a pretty well-constrained three-month period. I'd intended "student project" to be a good student bug not term-constrained, but I suppose that the lack of time pressure makes it pretty much identical to a mentored bug, so that's seems like a distinction without a difference. 

We may be able to roll them all into the one category, but I would be able to flag a mentored bug in some way that indicates that the mentoree in this case is probably young, probably under a fair bit of time & social pressure, and has to turn this bug around before exams.

Comment 16

[Mike Hoye [:mhoye]]

would like be able, I mean.

Comment 17

[Mike Hoye [:mhoye]]

Ok, so: after conversations with :bsmedberg and :lizzard and some reflection, I've reversed my position. Liz argued, convincingly, that one class of mentoree should not expect better or different treatment than any other, and that we should be timely about answering mentorees whoever they are. Some information about whether or not somebody is a student somewhere is certainly nice to have, but I no longer believe that it belongs in a Bugzilla field.

So:

* Good first bug
* Good next bug
* Mentored bug
* Experience required

Thanks, 

- mhoye

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

I'd love to be able to add myself as "mentor" in a field, for any bug.  
And also the option to set a complexity/difficulty level, for any bug. 

So a bug where someone has set it as good first or good next bug can also be a mentored bug.   More complicated bugs can also be mentored!  But none of them have to be mentored. 

That would be pretty straightforward. This way, some people who don't mind walking a new contributor through the steps to do a build and submit a patch in a "good first bug" can sign up to mentor that type of bug.  And more complicated bugs that fit our current "mentored bug" idea can also have mentors.

Comment 19

[David Lawrence [:dkl]]

(In reply to Liz Henry :lizzard from comment #18)
> I'd love to be able to add myself as "mentor" in a field, for any bug.  
> And also the option to set a complexity/difficulty level, for any bug. 
> 
> So a bug where someone has set it as good first or good next bug can also be
> a mentored bug.   More complicated bugs can also be mentored!  But none of
> them have to be mentored. 
> 
> That would be pretty straightforward. This way, some people who don't mind
> walking a new contributor through the steps to do a build and submit a patch
> in a "good first bug" can sign up to mentor that type of bug.  And more
> complicated bugs that fit our current "mentored bug" idea can also have
> mentors.

We should be able to do this now that we have moved the tracking flags out. I will talk to glob on monday about adding these new fields and see if we can close this out.

dkl

Comment 20

[David Teller [:Yoric] - still alive but not very active]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> * Good first bug
> * Good next bug
> * Mentored bug
> * Experience required

I may be misreading this, but for me, Good first bug, Good next bug and Experience required are just subcategories of Mentored bug, not other options.

Comment 22

[Mike Hoye [:mhoye]]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #20)
>
> I may be misreading this, but for me, Good first bug, Good next bug and
> Experience required are just subcategories of Mentored bug, not other
> options.

Part of the idea is to make good-first-bugs a hands-off way for people to get into the code without needing a lot of guidance. They're mostly about getting people's environments set up for more challenging work, not about resolving the bug itself.

Comment 23

[David Lawrence [:dkl]]

glob and I discussed this and we are going to implement this as an extension and that we will use a specific mapping table for the mentors instead of relying on a custom field. There is a bug upstream (bug 287332) about adding a new user specific custom field which we originally thought of using. But we need to be able to record more than one mentor per bug in some cases so that would not work. A new upstream bug should be filed after the new user custom field is done to create a new custom field type that supports multiple users similar to the cc list of a bug. Once that is complete we can migrate over to that.

Also we would like the UI to be a single text field that can hold multiple users instead of a drop down list. The field will support autocomplete. 

I would like to work on this as time allows.

dkl

Comment 24

[David Lawrence [:dkl]]

Attachment: 649691_1.patch

Low priority, when you get a chance.

Comment 25

[:glob ✱]

most of the remaining work here is centred around how we're going to deal with the migration from whiteboard mentor tracking to that field.  dkl and i have had several rounds of discussions, but we don't have a solid plan for that yet.

the discussion points are:
- after the field is live, it doesn't make sense to have the mentor in both the whiteboard and in a mentor field
  - we'll need migration code to move the mentor out of the whiteboard
  - should do a pre-migration check for bugs where we're unable to resolve the mentor
- any existing tools (notably bugs-ahoy) need to be updated at the same time that we add the field and migrate
  - this may be mitigated by search shenanigans mentioned below
- is it worth detecting when someone enters a mentor into the whiteboard, and instead update the mentor field?
  - difficultly level: low
  - need to ensure a message is displayed informing the user that their update has been redirected
- is it worth detecting when a search is performed against the whiteboard for a mentor, and instead search the mentor field?
  - difficulty: moderate
  - doesn't need to be 100% reliable, just case simple and common queries
  - trawl through the logs for mentor related queries to determine the common cases

Comment 26

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #25)
> most of the remaining work here is centred around how we're going to deal
> with the migration from whiteboard mentor tracking to that field.  dkl and i
> have had several rounds of discussions, but we don't have a solid plan for
> that yet.
> 
> the discussion points are:
> - after the field is live, it doesn't make sense to have the mentor in both
> the whiteboard and in a mentor field
>   - we'll need migration code to move the mentor out of the whiteboard
>   - should do a pre-migration check for bugs where we're unable to resolve
> the mentor
> - any existing tools (notably bugs-ahoy) need to be updated at the same time
> that we add the field and migrate

We will need to give ample time for people to update their tools to use the new field such as bug-ahoy and others. But give a firm deadline.

>   - this may be mitigated by search shenanigans mentioned below
> - is it worth detecting when someone enters a mentor into the whiteboard,
> and instead update the mentor field?
>   - difficultly level: low
>   - need to ensure a message is displayed informing the user that their
> update has been redirected
> - is it worth detecting when a search is performed against the whiteboard
> for a mentor, and instead search the mentor field?
>   - difficulty: moderate
>   - doesn't need to be 100% reliable, just case simple and common queries
>   - trawl through the logs for mentor related queries to determine the
> common cases

I personally vote to not code in these checks and workarounds as we will need to maintain them going forward and we really should be focusing on simplifying what we maintain. I think with the proper pre-announcements, time period for people to convert over and other education about the new field, we can just do the migration and people will be able to switch over without much issue.

dkl

Comment 27

[Benjamin Smedberg]

Once you make the field live, mhoye and I will personally make sure that the fields get migrated. I can do most of it automatically with bzapi, and the hard cases we'll deal with manually.

Comment 28

[:glob ✱]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #27)
> Once you make the field live, mhoye and I will personally make sure that the
> fields get migrated. I can do most of it automatically with bzapi, and the
> hard cases we'll deal with manually.

thanks for the offer of help :)

as we have a lot of bugs to update (3237 currently) i'd prefer for bugzilla to handle the migration of the field where unambiguous, and report bugs that manual updating (that's where you could help!).  we already have code which parses the whiteboard field for mentors, and we can skip triggering bugmail for these changes.

note the bzapi proxy won't know about the mentor field, you'll have to use our native rest services instead.

Comment 29

[:glob ✱]

Comment on attachment 8375867 [details] [diff] [review]
649691_1.patch

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

this patch mostly looks good.

searching using the 'bug mentors' field in a custom search fails with:
> Unknown column 'bugs.bug_mentors' in 'where clause'

it also appears to be missing from the webservice layer, and the xml output.

we also need to migrate whiteboard entries without triggering bugmail, so please create a stand-alone migration script for that.  use the old whiteboard parsing code, and report on any mentors that could not be found or matched more than one person.

::: extensions/Review/Extension.pm
@@ +81,5 @@
> +        my $mentor_ids = $dbh->selectcol_arrayref("
> +            SELECT user_id FROM bug_mentors WHERE bug_id = ?",
> +            undef,
> +            $self->id);
> +        $self->{bug_mentors} = Bugzilla::User->new_from_list($mentor_ids);

as the mentor list is likely to be small, it would be better to create each user object individually passing cache=>1 so we can leverage memcached.

@@ +594,5 @@
> +        INDEXES => [
> +            bug_mentors_idx => {
> +                FIELDS => [ 'bug_id', 'user_id' ],
> +                TYPE => 'UNIQUE',
> +            },

add an index for the bug_id lookup

::: template/en/default/bug/create/create.html.tmpl
@@ +648,4 @@
>        value_span = 3 %]
>    </tr>
>  [% END %]
> +[% Hook.process('after_custom_fields') %]

the width of this field on enter_bug is unique, resulting is a messy layout.
can you make the keywords, see_also, and mentors fields all the same width.

i was playing around and came up with:
adding value_span=3 to see_also's bug/field INCLUDE
#keyword_container, #bug_mentors_autocomplete, #container_see_also { width: 25em }

Comment 30

[Mike Hoye [:mhoye]]

Since we now have this list of "diamond" priority bugs coming from our triage meetings, I'd like to change the request slightly to:

* Good first bug
* Good next bug
* Diamond bug
* Experience required

... and, obvs, that if one of them is selected that a mentor be required.

Comment 31

[Mike Hoye [:mhoye]]

Can I find out where this is on whatever priority list it's on?

Comment 32

[Mark Côté [:mcote]]

Kind of low, sadly.  Hopefully in a few weeks.

Comment 33

[David Lawrence [:dkl]]

Attachment: 649691_2.patch

Comment 34

[David Lawrence [:dkl]]

Comment on attachment 8432327 [details] [diff] [review]
649691_2.patch

Sigh, forgot to address the search error. Removing r?

Comment 35

[David Lawrence [:dkl]]

Attachment: 649691_3.patch

Comment 36

[:glob ✱]

Comment on attachment 8433498 [details] [diff] [review]
649691_3.patch

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

i think it would be nice if we could add 'mentor' to advanced search's "search by people" options.

::: Bugzilla/WebService/Bug.pm
@@ +1359,4 @@
>          $item{'is_creator_accessible'} = $self->type('boolean', $bug->reporter_accessible);
>      }
>  
> +    # BMO Bug 649691

there's no need to mention a bug number, that's what 'blame' is for.  instead a comment should contain directly usable text.

eg.  # BMO - support our custom mentor field

@@ +1365,5 @@
> +        foreach my $mentor (@{ $bug->mentors }) {
> +            push(@mentors, $self->_user_to_hash($mentor, $params, undef, 'mentors'));
> +        }
> +        $item{'mentors'} = \@mentors;
> +    }

to match treatment of existing user fields, we should return "mentors" as just an array of logins, and "mentors_detail" with a list of all information.

::: extensions/BMO/template/en/default/hook/global/variables-end.none.tmpl.orig
@@ +1,3 @@
> +[%
> +  terms.BugzillaTitle = "Bugzilla@Development"
> +%]

this file shouldn't be here.

::: extensions/Review/Extension.pm
@@ +91,1 @@
>          }

the list of mentors should be sorted by login to ensure they are always presented in the same order.

@@ +578,5 @@
> +    }
> +    else {
> +        $args->{term} = "$check_field IS NOT NULL";
> +    }
> +}

rather than duplicate search.pm's _user_* subs, i think it would be better to edit search.pm directly and add mentor support to OPERATOR_FIELD_OVERRIDES and USER_FIELDS.

@@ +863,5 @@
>          'profiles',
>          'needinfo_request_count', { TYPE => 'INT2', NOTNULL => 1, DEFAULT => 0 }
>      );
> +
> +    my $field = new Bugzilla::Field({ name => 'bug_mentors' });

nit: use Bugzilla::Field->new() not the old style

@@ +868,5 @@
> +    if (!$field) {
> +        Bugzilla::Field->create({
> +            name        => 'bug_mentors',
> +            description => 'Bug Mentors'
> +        });

the description for this field should be "Mentor", as it's treated as singular in most places.  "bug" is redundant, and doesn't make the naming of any other fields.

for example, currently you can search for:
(bug mentors) (is equal to) ([e-mail])
this will show bugs where i'm a mentor, not bugs where i'm _the only_ mentor

this reads better as:
(mentor) (is equal to) ([e-mail])

::: extensions/Review/template/en/default/hook/bug/show-bug_end.xml.tmpl
@@ +10,5 @@
> +  [% FOREACH mentor = bug.mentors %]
> +    <bug_mentor name="[% mentor.name FILTER xml %]">
> +      [% mentor.login FILTER email FILTER xml %]</bug_mentor>
> +  [% END %]
> +[% END %]

nit: you don't need to check bug.mentors.size, as the foreach will no-op if bug.mentors is empty

Comment 37

[David Lawrence [:dkl]]

Attachment: 649691_4.patch

Comment 38

[:glob ✱]

Comment on attachment 8437889 [details] [diff] [review]
649691_4.patch

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

is_involved_in

Comment 39

[:glob ✱]

Comment on attachment 8437889 [details] [diff] [review]
649691_4.patch

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

as per our discussion last week, please change the search code, and add the mentor field to $user->is_involved_in

::: Bugzilla/Search.pm
@@ +431,5 @@
> +    };
> +    Bugzilla::Hook::process('search_user_fields',
> +                            { user_fields => $user_fields });
> +    return $user_fields;
> +}

instead of adding custom hooks to search, please just add the mentor field to search.pm.
both methods (hooks vs direct updates) involve carrying customisations.  using hooks in this case is actually a greater change from upstream.
once upstream has custom user fields, these changes should go away.

Comment 40

[David Lawrence [:dkl]]

Attachment: 649691_5.patch

Comment 41

[David Lawrence [:dkl]]

Attachment: Schema Only Changes

Comment 42

[David Lawrence [:dkl]]

If these pass review, feel free to push out.

Comment 43

[Mike Hoye [:mhoye]]

I've been digging into the mentored bug whiteboard tags for the last few days, and that data is kind messy, but not horribly so. I'm working on cleaned up to the point that the migration can happen automatically; do you have a timeline after deploying when you'd like to do that?

Comment 44

[David Lawrence [:dkl]]

(In reply to Mike Hoye [:mhoye] from comment #43)
> I've been digging into the mentored bug whiteboard tags for the last few
> days, and that data is kind messy, but not horribly so. I'm working on
> cleaned up to the point that the migration can happen automatically; do you
> have a timeline after deploying when you'd like to do that?

As part of bug 1017648, we should be able to migrate the majority of them without much clean up. I was able to migrate 1059 with 217 left over in my test database. Once the migration is done you will be able to do a search of the whiteboard for the remaining values and we can help fix them manually.

Some examples of ones that could not be migrated are bug 1002583 and bug 1003806 among others.

dkl

Comment 45

[:glob ✱]

(In reply to Mike Hoye [:mhoye] from comment #43)
> I've been digging into the mentored bug whiteboard tags for the last few
> days, and that data is kind messy, but not horribly so. I'm working on
> cleaned up to the point that the migration can happen automatically; do you
> have a timeline after deploying when you'd like to do that?

it's probably best to not try to clean up the existing tags until after the first migration run of the script of bug 1017648 (to avoid unnecessary bugmail).  the script will generate a report of all the bugs it wasn't able to migrate along with a reason why parsing the whiteboard failed.

Comment 46

[:glob ✱]

Comment on attachment 8440870 [details] [diff] [review]
Schema Only Changes

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

r=glob

Comment 47

[:glob ✱]

schema only:
To ssh://[e-mail]/webtools/bmo/bugzilla.git
   0a2592d..aaba5db  master -> master

Comment 48

[:glob ✱]

Comment on attachment 8440859 [details] [diff] [review]
649691_5.patch

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

r=glob

will commit to land prior to today's push.

::: Bugzilla/User.pm
@@ +775,5 @@
>          return 1 if $user_id == $bug->qa_contact->id;
>      }
>  
> +    # BMO - Bug mentors are considered involved with the bug
> +    return 1 if ($bug->can('is_mentor') && $bug->is_mentor($self));

nit: no need to check if $bug->can('is_mentor') as it's a bmo specific modification

Comment 49

[:glob ✱]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   aaba5db..464be05  master -> master

Comment 50

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #48)
> nit: no need to check if $bug->can('is_mentor') as it's a bmo specific
> modification

I did it as a precaution if we ever disabled the Review extension for debugging purposes or another more serious issue.

dkl