909822 - mcmerge should also set the assignee for components where the unassigneed name != nobody

Status: NEW

(Tree Management :: Bugherder, defect, P3)

Description

[Nathan Froyd [:froydnj]]

Josh was saying on dev-tech-js-engine that a lot of JS bugs that get fixed don't have their assignee altered.  I know that I rarely set assignee in the bugs that I fix.

But as long as I'm filing inbound links via mcmerge, it'd be great to be able to auto-assign those bugs to myself.  Maybe some people won't use this, but I have at least been setting in-testsuite more often when using mcmerge since it's right there.

Comment 1

[Ed Morley [:emorley]]

(In reply to Nathan Froyd (:froydnj) from comment #0)
> Josh was saying on dev-tech-js-engine that a lot of JS bugs that get fixed
> don't have their assignee altered. 

I think some of these were due to bug 897442, which is now fixed :-)
(Presuming people update their local copies of bzexport; probably should email dev.<something>)

> But as long as I'm filing inbound links via mcmerge, it'd be great to be
> able to auto-assign those bugs to myself.  Maybe some people won't use this,
> but I have at least been setting in-testsuite more often when using mcmerge
> since it's right there.

mcmerge should already be setting the assignee (https://hg.mozilla.org/webtools/tbpl/file/318535d10b10/mcmerge/js/Step.js#l166) is it not?

Comment 2

[Ryan VanderMeulen [:RyanVM]]

The issue is with [e-mail] being the assignee instead of nobody.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Ed Morley [:edmorley UTC+1] from comment #1)
> mcmerge should already be setting the assignee
> (https://hg.mozilla.org/webtools/tbpl/file/318535d10b10/mcmerge/js/Step.
> js#l166) is it not?

I mcmerge'd bug 909328 yesterday and the assignee is not set to me.

I also mcmerge'd bug 906088 and it did not see the assignee.

I can point to other cases. :)

Comment 4

[Ed Morley [:emorley]]

Ah:
https://hg.mozilla.org/webtools/tbpl/file/318535d10b10/mcmerge/js/BugData.js#l105

We should just use /\.bugs$/.match or something

Comment 5

[Ed Morley [:emorley]]

Attachment: Patch v1

Set the assignee for any Bugzilla placeholder assignee, not just 'nobody'.
Ryan, don't suppose you could test this locally next time you merge (or I will if I'm next) to make sure it works? :-)

Comment 6

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley UTC+1] from comment #1)
> I think some of these were due to bug 897442, which is now fixed :-)
> (Presuming people update their local copies of bzexport; probably should
> email dev.<something>)

Sorry I meant bug 764881.

Comment 7

[Ed Morley [:emorley]]

Ty :-)
https://hg.mozilla.org/webtools/tbpl/rev/b5d61de7b036

Comment 8

[Ed Morley [:emorley]]

In production :-)

Comment 9

[Ed Morley [:emorley]]

Ok this isn't going to work, since (a) the current patch causes a type error, (b) when not logged in (ie bzapi) bugzilla only returns the part of the email address before the @ sign.

eg:
https://api-dev.bugzilla.mozilla.org/latest/bug?id=285663&include_fields=id%2Cresolution%2Cstatus%2Cwhiteboard%2Ckeywords%2Ctarget_milestone%2Csummary%2Cproduct%2Ccomponent%2Cflags%2Cassigned_to%2Ccf_tracking_firefox26%2Ccf_status_firefox26

...which is going to vary to much for the generic component email addresses.

What we could do is just check for the presence of real_name to *roughly* decide if the bug is assigned, perhaps?

eg see this assigned bug:
https://api-dev.bugzilla.mozilla.org/latest/bug?id=838396&include_fields=id%2Cresolution%2Cstatus%2Cwhiteboard%2Ckeywords%2Ctarget_milestone%2Csummary%2Cproduct%2Ccomponent%2Cflags%2Cassigned_to%2Ccf_tracking_firefox26%2Ccf_status_firefox26

Anyway, I've backed out the change for now, to unbreak mcmerge:
https://hg.mozilla.org/webtools/tbpl/rev/16cf91ab5d57

Comment 10

[Ed Morley [:emorley]]

Glob, any ideas how best to determine if a bug is unassigned, given not all components use 'nobody' & bzapi doesn't give you the full email address (to match against '.bugs$') when not logged in? Would checking for the absence of the real_name field be a close approximation do you think? (Given I'm presuming most people have set a real name).

Comment 11

[Josh Matthews [:jdm]]

(In reply to Ed Morley [:edmorley UTC+1] from comment #10)
> Glob, any ideas how best to determine if a bug is unassigned, given not all
> components use 'nobody' & bzapi doesn't give you the full email address (to
> match against '.bugs$') when not logged in? Would checking for the absence
> of the real_name field be a close approximation do you think? (Given I'm
> presuming most people have set a real name).

When creating the lists for posts like https://blog.mozilla.org/community/2013/05/28/firefox-21-new-contributors-2/, I routinely have just an email address for several authors, indicating no real name is present.

Comment 12

[Graeme McCutcheon [:graememcc]]

(In reply to Ed Morley [:edmorley UTC+1] from comment #9)
> Ok this isn't going to work, since (a) the current patch causes a type
> error, (b) when not logged in (ie bzapi) bugzilla only returns the part of
> the email address before the @ sign.

In any case even with the email address, this patch would have busted assignee-setting in the common case. '[e-mail]' does not end with 'bugs'.

(As an aside I would have wanted to see a polyfill. String.prototype.endsWith isn't widely supported yet).

Note the decision to use anonymous calls was deliberate: I didn't want mcmerge acquiring credentials until it really needed them.

The ideal solution would be if the Configuration API call returned the default assignee for each component. However, I'm guessing it's possible for the default assignee to be a real email rather than a placeholder, in which case the default assignee info would need to be behind an authenticated call, leaving us no further forward.

You could inspect the email mid-submit by interjecting some code around here:
http://hg.mozilla.org/webtools/tbpl/file/8defe52551ff/mcmerge/js/Step.js#l323
Before submitting changes, mcMerge makes an authenticated call for the bug data, as it needs an update token and last modification time to submit changes. However, this would need some tricky logic to ensure we don't reinsert the assignee in a retry - if an initial submission attempt with an assignee is rejected, an attempt is made without the assignee information (there are occasions where changeset author email != their bugzilla account email).

I'm not sure having partial information is something we need to worry about too much:
$ hg log --template "{author|email}\n" | grep -ci "nobody"
0
$ hg log --template "{author|email}\n" | grep -ci "general"
0

and I don't know of any placeholder assignees that start with anything other than 'nobody' or 'general' (although given that I forgot about [e-mail] when writing this code in the first place, I'm hardly a credible witness on this particular point).

Comment 13

[Ed Morley [:emorley]]

(In reply to Graeme McCutcheon [:graememcc] from comment #12)
> In any case even with the email address, this patch would have busted
> assignee-setting in the common case. '[e-mail]' does not end with
> 'bugs'.

Ha yeah, this patch was total fail on my part all ways around, oops!

Comment 14

[:glob ✱]

(In reply to Ed Morley [:edmorley UTC+1] from comment #10)
> Glob, any ideas how best to determine if a bug is unassigned
> Would checking for the absence of the real_name field be a close approximation do you think?

no, that wouldn't work.


as per comment 12 a bug is unassigned when bug.assigned_to == component.default_assigned_to

unlike the bzapi proxy, bugzilla's native rest api _does_ return full email addresses for unauthenticated calls, so you can do:

https://bugzilla.mozilla.org/rest/product?names=Core&include_fields=components

Comment 15

[Ed Morley [:emorley]]

Glob, thank you for the advice - agree best way to fix this is to wait for bug 924405 :-)

Unassigning until that bug is fixed (which itself depends on the 4.4 rollout).

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Will need https://bugzilla.mozilla.org/rest/product/[Product]?include_fields=components.name,components.default_assigned_to most likely.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: PR 31

This is on top of the PR from 924405 which switches Bugherder to using the native REST API.

Getting bug 1194737 fixed and deployed to BMO would make this patch much smaller, since the default assignee values would either be exposed via /configuration or as part of each individual bug returned by the API. Until that happens, we're stuck pulling the default assignees from this new call to /product...

Comment 18

[Graeme McCutcheon [:graememcc]]

Want to warn that I won't get a chance to properly look over bug 924405 and this until Wednesday 19th Aug at the earliest: I'm backed up with prep for a job interview on Tuesday 18th.

Comment 19

[Ed Morley [:emorley]]

Comment on attachment 8648168 [details] [review]
PR 31

(Haven't tested locally)

Comment 20

[Graeme McCutcheon [:graememcc]]

Comment on attachment 8648168 [details] [review]
PR 31

Have written a couple of minor notes inline in the PR, but I think I'd prefer to have the assignee loading and checking as a discrete step kicked off in bugherder.js after the bugs have been loaded and processed. This would be consistent with the pattern of data processing in the rest of Bugherder, rather than fetching and processing some more data within another data processing step. However, I recognise that there's a tighter coupling here than between other processing steps.

In particular, if it's a huge merge where we have to load the bug data in chunks (due to a bzAPI limitation that might not be relevant now), currently the bugs loaded in the first chunk will be iterated over multiple times.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Rebased what I had before onto the current master branch (gets rid of all of the native REST changes in the PR), then addressed the in-line comments.

I haven't tested with these changes yet, and I haven't even attempted to get the fetching split out into a separate step.