1243343 - UnicodeEncodeError: 'ascii' codec can't encode characters in position 56-57: ordinal not in range(128) when trying to publish a review request to dbaron, who has an emoji in his bugzilla user name and is not accepting review requests at the moment

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Markus Stange [:mstange]]

I ended up publishing by pressing the Publish button on the mozreview web UI instead. I got an error message saying that David Baron [:dbaron] ⌚️UTC+13 is not accepting review requests at the moment, so I redirected the review to somebody else.

> mstange:mozilla?$ hg push -c . review
> pushing to ssh://reviewboard-hg.mozilla.org/gecko
> searching for changes
> remote: adding changesets                                                                           
> remote: adding manifests
> remote: adding file changes
> remote: added 1 changesets with 1 changes to 10 files (+1 heads)
> remote: recorded push in pushlog
> submitting 1 changesets for review
> 
> changeset:  311002:61cab4e535c6
> summary:    Bug 1241275 - Change the way -moz-window-dragging works. r?dbaron, r?roc
> review:     https://reviewboard.mozilla.org/r/32503 (draft)
> 
> review id:  bz://1241275/mstange
> review url: https://reviewboard.mozilla.org/r/32501 (draft)
> 
> publish these review requests now (Yn)?  
> remote: ** Unknown exception encountered with possibly-broken third-party extension pushlog-feed
> remote: ** which supports versions unknown of Mercurial.
> remote: ** Please disable pushlog-feed and try your action again.
> remote: ** If that fixes the bug please report it to the extension author.
> remote: ** Python 2.7.10 (default, May 28 2015, 09:58:55) [GCC 4.4.7 20120313 (Red Hat 4.4.7-11)]
> remote: ** Mercurial Distributed SCM (version 3.6.2)
> remote: ** Extensions loaded: pushlog, pushlog-feed, buglink, serverlog, rbserver
> remote: Traceback (most recent call last):
> remote:   File "/repo/hg/venv_pash/bin/hg", line 43, in <module>
> remote:     mercurial.dispatch.run()
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 54, in run
> remote:     sys.exit((dispatch(request(sys.argv[1:])) or 0) & 255)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 116, in dispatch
> remote:     ret = _runcatch(req)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 187, in _runcatch
> remote:     return _dispatch(req)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 920, in _dispatch
> remote:     cmdpats, cmdoptions)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 679, in runcommand
> remote:     ret = _runcommand(ui, options, cmd, d)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 1051, in _runcommand
> remote:     return checkargs()
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 1011, in checkargs
> remote:     return cmdfunc()
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 917, in <lambda>
> remote:     d = lambda: util.checksignature(func)(ui, *args, **cmdoptions)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/util.py", line 801, in check
> remote:     return func(*args, **kwargs)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/commands.py", line 5919, in serve
> remote:     s.serve_forever()
> remote:   File "/repo/hg/version-control-tools/hgext/serverlog/__init__.py", line 321, in serve_forever
> remote:     return super(sshserverwrapped, self).serve_forever()
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/sshserver.py", line 103, in serve_forever
> remote:     while self.serve_one():
> remote:   File "/repo/hg/version-control-tools/hgext/serverlog/__init__.py", line 350, in serve_one
> remote:     return super(sshserverwrapped, self).serve_one()
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/sshserver.py", line 121, in serve_one
> remote:     rsp = wireproto.dispatch(self.repo, self, cmd)
> remote:   File "/repo/hg/version-control-tools/hgext/serverlog/__init__.py", line 342, in dispatch
> remote:     return origdispatch(repo, proto, cmd)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/wireproto.py", line 471, in dispatch
> remote:     return func(repo, proto, *args)
> remote:   File "/repo/hg/version-control-tools/hgext/reviewboard/hgrb/proto.py", line 421, in publishreviewseries
> remote:     res = _processpublishreview(repo, req)
> remote:   File "/repo/hg/version-control-tools/hgext/reviewboard/hgrb/proto.py", line 444, in _processpublishreview
> remote:     res['results'].append({'rrid': rrid, 'error': str(e)})
> remote: UnicodeEncodeError: 'ascii' codec can't encode characters in position 56-57: ordinal not in range(128)
> abort: unexpected response: empty string

Comment 1

[Gregory Szorc [:gps]]

Please do this again and report the new error message (as I expect it to change slightly).

Comment 2

[Markus Stange [:mstange]]

> mstange:mozilla?$ hg push -c 80357558913b review
> pushing to ssh://reviewboard-hg.mozilla.org/gecko
> searching for changes
> no changes found
> submitting 1 changesets for review
> 
> changeset:  312247:80357558913b
> summary:    Bug 1241275 - Change the way -moz-window-dragging works. r?dbaron, r?roc
> review:     https://reviewboard.mozilla.org/r/32503 (draft)
> 
> review id:  bz://1241275/mstange
> review url: https://reviewboard.mozilla.org/r/32501 (draft)
> 
> publish these review requests now (Yn)?  
> remote: ** unknown exception encountered, please report by visiting
> remote: ** https://mercurial-scm.org/wiki/BugTracker
> remote: ** Python 2.7.10 (default, May 28 2015, 09:58:55) [GCC 4.4.7 20120313 (Red Hat 4.4.7-11)]
> remote: ** Mercurial Distributed SCM (version 3.6.2)
> remote: ** Extensions loaded: pushlog, pushlog-feed, buglink, serverlog, rbserver
> remote: Traceback (most recent call last):
> remote:   File "/repo/hg/venv_pash/bin/hg", line 43, in <module>
> remote:     mercurial.dispatch.run()
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 54, in run
> remote:     sys.exit((dispatch(request(sys.argv[1:])) or 0) & 255)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 116, in dispatch
> remote:     ret = _runcatch(req)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 187, in _runcatch
> remote:     return _dispatch(req)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 920, in _dispatch
> remote:     cmdpats, cmdoptions)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 679, in runcommand
> remote:     ret = _runcommand(ui, options, cmd, d)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 1051, in _runcommand
> remote:     return checkargs()
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 1011, in checkargs
> remote:     return cmdfunc()
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/dispatch.py", line 917, in <lambda>
> remote:     d = lambda: util.checksignature(func)(ui, *args, **cmdoptions)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/util.py", line 801, in check
> remote:     return func(*args, **kwargs)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/commands.py", line 5919, in serve
> remote:     s.serve_forever()
> remote:   File "/repo/hg/version-control-tools/hgext/serverlog/__init__.py", line 321, in serve_forever
> remote:     return super(sshserverwrapped, self).serve_forever()
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/sshserver.py", line 103, in serve_forever
> remote:     while self.serve_one():
> remote:   File "/repo/hg/version-control-tools/hgext/serverlog/__init__.py", line 350, in serve_one
> remote:     return super(sshserverwrapped, self).serve_one()
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/sshserver.py", line 121, in serve_one
> remote:     rsp = wireproto.dispatch(self.repo, self, cmd)
> remote:   File "/repo/hg/version-control-tools/hgext/serverlog/__init__.py", line 342, in dispatch
> remote:     return origdispatch(repo, proto, cmd)
> remote:   File "/repo/hg/venv_pash/lib64/python2.7/site-packages/mercurial/wireproto.py", line 471, in dispatch
> remote:     return func(repo, proto, *args)
> remote:   File "/repo/hg/version-control-tools/hgext/reviewboard/hgrb/proto.py", line 427, in publishreviewseries
> remote:     res = _processpublishreview(repo, req)
> remote:   File "/repo/hg/version-control-tools/hgext/reviewboard/hgrb/proto.py", line 450, in _processpublishreview
> remote:     res['results'].append({'rrid': rrid, 'error': str(e)})
> remote: UnicodeEncodeError: 'ascii' codec can't encode characters in position 56-57: ordinal not in range(128)
> abort: unexpected response: empty string

Comment 3

[Gregory Szorc [:gps]]

Looks like a regression from bug 1234413. I'll look into it.

Comment 4

[Gregory Szorc [:gps]]

glob: this appears to be triggered by the Bugzilla preference to not accept any new review or feedback requests. How would I toggle this profile/pref bit from a test? Can you adjust it from the web API or do I need to fake the form submission from userprefs.cgi?

Comment 5

[Gregory Szorc [:gps]]

Unassigning for now b/c I don't know how to test.

Comment 6

[:glob ✱]

(In reply to Gregory Szorc [:gps] from comment #4)
> glob: this appears to be triggered by the Bugzilla preference to not accept
> any new review or feedback requests. How would I toggle this profile/pref
> bit from a test? Can you adjust it from the web API or do I need to fake the
> form submission from userprefs.cgi?

this isn't exposed via the api.  i'll write a quick script for this so you don't have to fake form submission - bug 1244996.

Comment 7

[:glob ✱]

https://git.mozilla.org/?p=webtools/bmo/bugzilla.git;a=commitdiff;h=92ec0f3 [github]

$bmo_path/scripts/user-prefs.pl <bugzilla-login> <get|set> [param] [value]

eg.
user-prefs.pl [e-mail] set api_key_only on
user-prefs.pl [e-mail] set api_key_only off
user-prefs.pl [e-mail] get api_key_only

Comment 8

[Gregory Szorc [:gps]]

Byron: thank you for writing that script so quickly!

Comment 9

[Gregory Szorc [:gps]]

This has turned into a rabbit hole of sorts.

If a review request has multiple reviewers, we submit a single attachment to Bugzilla with multiple review flags set. It appears Bugzilla atomically accepts or rejects the attachment change. So, if a subset of reviewers have reviews blocked, the request fails.

Here's the problem. The error code from Bugzilla is 32000 and the error message is e.g. "Romulus :romulus <[e-mail]> is not currently accepting 'review' requests." That error code is not unique and the error message requires parsing to figure out which reviewer is blocking. Parsing error messages always feels a bit fragile to me, as they can change at any time. MozReview /could/ parse out the failing reviewer from the request and try the request again without flags set for that reviewer.

I /think/ it makes sense for MozReview to treat "reviewer is blocking" as a warning, not a fatal error. We already have a mechanism for reporting reviewer-related warnings to the client. I don't think we should prevent publishing because a reviewer isn't accepting reviews. This would seem to require a "accept what you can" attachment API from Bugzilla and/or a machine readable response indicating errors with reviewers blocking requests.

glob: I'm curious what your thoughts are here.

Comment 10

[:glob ✱]

(In reply to Gregory Szorc [:gps] from comment #9)
> That error code is not unique and the error message requires parsing to figure out which reviewer is blocking.

out of curiosity why do you need to parse the error message?  could you fail the whole request and display the error thrown by bmo?

we should update bugzilla to return a unique error code, but having a structured error returned to cover the multi-reviewer case would be a problem, even with the batch api (the whole error mechanism within bugzilla only allows us to throw a code and a string).

> I /think/ it makes sense for MozReview to treat "reviewer is blocking" as a warning, not a fatal error.

i kinda like this idea.. but..

we had this for a while in bugzilla, but it didn't work out; the warning was being missed and patches sat for a long time without review (more common with new contributors).  i can picture this happening when someone pushes from the command line due to the amount of output and the tenancy to 'fire and forget'.

does it even make sense to allow someone to push something to review board without any reviewers set?

> This would seem to require a "accept what you can" attachment API from Bugzilla
> and/or a machine readable response indicating errors with reviewers blocking requests.

if we take this path we'd have to extend the batch api to support this.  we'd explicitly check each reviewer and not to set the review flag if we know it won't be accepted, returning the excluded reviewers in the 'success' result.


an alternative would be to add the 'blocking-review-request' status to the user api, and have mozreview perform a pre-push check of reviewers - this would also have the advantage of catching invalid/multi-match nicks before submission.

Comment 11

[Markus Stange [:mstange]]

I've hit this again by requesting review from dbaron on a different bug.

Comment 12

[Gregory Szorc [:gps]]

I'm not actively working on this.

Comment 13

[:glob ✱]

bug 1320976 is related, but focused on the pre-push user validation part (let's focus this bug on the unicode error part).

Comment 14

[Piotr Zalewa [:zalun]]

Attachment: mozreview: provide error msg when reviewer with emoji blocks reviews (bug 1243343);

MozReview can't process the error message from Bugzilla when user blocks
reviews and has an emoji character in the name field.

Instead of str() we need to use unicode().

Review commit: https://reviewboard.mozilla.org/r/112086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112086/

Comment 15

[Māris Fogels [:mars] (please needinfo)]

Comment on attachment 8836738 [details]
mozreview: provide error msg when reviewer with emoji blocks reviews (bug 1243343);

https://reviewboard.mozilla.org/r/112086/#review113422

LGTM

Comment 16

[:glob ✱]

Comment on attachment 8836738 [details]
mozreview: provide error msg when reviewer with emoji blocks reviews (bug 1243343);

https://reviewboard.mozilla.org/r/112086/#review113566

rs=glob

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/dbe9bc1d5367
mozreview: provide error msg when reviewer with emoji blocks reviews ; r=glob,mars