1169724 - MozReview reviewer deduction not working

Status: RESOLVED DUPLICATE of bug 1177567

(MozReview Graveyard :: General, defect, P1)

Description

[Botond Ballo [:botond]]

STR:
  Push a review request containing a commit whose commit message
  contains the string "r=tn".

Expected results:
  "tn" (Timothy Nikkel) gets automatically populated in the reviewer
   field for the commit.

Actual results:
  The reviewer field stays blank.

Note that I have the latest version of version-control-tools, and I get the 

  "(It appears you are using r= to specify reviewers for a patch under review. 
    Please use r? to avoid ambiguity as to whether or not review has been granted.)" 

warning message, as expected.

It doesn't work with "r=kats" either, but there the problem might be related to bug 1139662, because entering "kats" manually triggers an internal server error (as described in that bug). Entering "tn" manually, however, works fine, so this seems to be a  different bug.

Comment 1

[Dan Minor [:dminor]]

I had r?gps fail for me as well. :mcote was able to use it successfully when we pushed it to production, so I'm not sure if it is failing for some reviewers and not others, or if something has subsequently gone wrong.

Fixing Bug 1164127 to give feedback on unrecognized reviewers should help identify what's going wrong.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

I saw this in https://reviewboard.mozilla.org/r/9729/diff/1/

All the tests at https://dxr.mozilla.org/hgcustom:version-control-tools/source/pylib/mozautomation/tests/test_commitparser.py have a space before the r, maybe my string is off.

Comment 3

[Botond Ballo [:botond]]

I had a space before the 'r' in my commit messages.

Comment 5

[Chenxia Liu [:liuche]]

I'm also seeing this, for r?ally .

Comment 6

[Botond Ballo [:botond]]

Not working with "r=ehsan" either.

Comment 7

[(not currently active) Ted Mielczarek]

I got the warning message on a review push today:
```
unrecognized reviewer: jrmuizel

changeset:  250793:d143c2f403fd
summary:    bug 1174415 - Make gfx build for iOS. r?jrmuizel
review:     https://reviewboard.mozilla.org/r/11357 (draft)
```

Going to the review and putting that same nick in the reviewer field works, however.

Comment 8

[Dan Minor [:dminor]]

I broke out the relevant portion of code [1] into a standalone command line utility. It seems to work fine against both reviewboard and reviewboard-dev for reviewers that fail for me when I push to mozreview.

I think the next step is to log the actual exception at [2] to see if we're getting more than just APIErrors due to bad usernames.

[1] https://dxr.mozilla.org/hgcustom:version-control-tools/source/pylib/reviewboardmods/reviewboardmods/pushhooks.py#166

[2] https://dxr.mozilla.org/hgcustom:version-control-tools/source/pylib/reviewboardmods/reviewboardmods/pushhooks.py#201

Comment 9

[Botond Ballo [:botond]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> I got the warning message on a review push today:
> unrecognized reviewer: jrmuizel
>
> [...]
> 
> Going to the review and putting that same nick in the reviewer field works,
> however.

Likewise, for 'kats'.

Comment 10

[Botond Ballo [:botond]]

(In reply to Dan Minor [:dminor] from comment #8)
> I broke out the relevant portion of code [1] into a standalone command line
> utility. It seems to work fine against both reviewboard and reviewboard-dev
> for reviewers that fail for me when I push to mozreview.
> 
> I think the next step is to log the actual exception at [2] to see if we're
> getting more than just APIErrors due to bad usernames.

This logging was added and deployed to the dev instance. I tried pushing there with "r=kats", and got:

error identifying reviewer: kats: HTTP 500

Comment 11

[Steven MacLeod [:smacleod]]

Attachment: xmlresponse

We're breaking because the xml rpc we're making gets a response that the expat parser can't parse. The character which explodes things is '\x1b'.

I've attached the body of the xml response bugzilla gives back.

The user that appears to break us is https://bugzilla.mozilla.org/user_profile?login=yokada%40art.osaka-med.ac.jp due to their full name.

Comment 12

[Mark Côté [:mcote]]

Filed bug 1176452 with example script.  I set it to block this bug, but we can remove that if we want to work around it somehow.

Comment 13

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)
> (In reply to Dan Minor [:dminor] from comment #8)
> > I broke out the relevant portion of code [1] into a standalone command line
> > utility. It seems to work fine against both reviewboard and reviewboard-dev
> > for reviewers that fail for me when I push to mozreview.
> > 
> > I think the next step is to log the actual exception at [2] to see if we're
> > getting more than just APIErrors due to bad usernames.
> 
> This logging was added and deployed to the dev instance. I tried pushing
> there with "r=kats", and got:
> 
> error identifying reviewer: kats: HTTP 500

Today I did a different push with "r=karlt", and got a different error:

error identifying reviewer: karlt: Bugzilla error: Logged-out users cannot use the "match" argument to this function to access any user information. (HTTP 500, API Error 226)

Is that a different issue?

Comment 14

[Steven MacLeod [:smacleod]]

(In reply to Botond Ballo [:botond] from comment #13) 
> Today I did a different push with "r=karlt", and got a different error:
> 
> error identifying reviewer: karlt: Bugzilla error: Logged-out users cannot
> use the "match" argument to this function to access any user information.
> (HTTP 500, API Error 226)
> 
> Is that a different issue?

Yes, it appears we have another bug here. We finally deployed some new error reporting to the hg server and this showed up.

Comment 16

[Mark Côté [:mcote]]

I split the auth problem into another bug: bug 1177454.

Comment 17

[Mark Côté [:mcote]]

Apparently fixing the bug in Bugzilla is nontrivial, and XMLRPC is deprecated now anyway.  I'm going to dupe this bug over to bug 1177567, where we'll be moving to Bugzilla's REST API.

In the meantime, we've edited the problematic user's real name to strip out the escape character, since the user has never done anything with BMO after signing up.  I verified that r?kats now correctly sets him as reviewer.  If anyone runs into more problematic user name before we switch to REST, you can comment here and we'll try to sanitize them as well.

Comment 18

[Botond Ballo [:botond]]

I can confirm that reviewer deduction is now working for me (I've tried with 'r=kats' and 'r=tn'). Thanks for the investigation and fix!