1238353 - specifying reviewer with dot sign in the nickname doesn't work

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[simon]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20160105164030

Steps to reproduce:

-Make a commit
-type as reviewer in the comment a nickname with a dot sign, for example
Bug 1231810 - Fix files not following flake8 convention ; r?ted.mielczarek
-push it to mozreview


Actual results:

Commit is pushed, but the reviewer request is not done:
"unrecognized reviewer: ted"


Expected results:

The commit should have recognized ted.mielczarek as the reviewer.

Comment 1

[simon]

Either a nick including a dot is not a valid nick, or the regex REVIEWER at https://github.com/mozilla/version-control-tools/blob/master/pylib/mozautomation/mozautomation/commitparser.py is not valid.

Comment 2

[:glob ✱]

a dot is not valid in an irc nick, which is why it isn't present in that regex.

if we were to add it, we'd have to ensure that it doesn't match at the end of the string to avoid "r?glob." from matching "glob." instead of "glob"


ted's :handle on bmo (:ted.mielczarek) doesn't match his irc handle (ted).
of all the users on bmo, only ted (:ted.mielczarek) and bas (:bas.schouten) do this (should be :ted and :bas respectively).

i suspect i may be better to update pylib/mozreview/mozreview/bugzilla/models.py to use the REVIEWER character class from commitparser.py instead of its own BZ_IRCNICK_RE, and fix up and users with periods in their names in the rb database.

Comment 4

[:glob ✱]

Attachment: MozReview Request: mozreview: fix pep 8 issues in bugzilla/models.py (bug 1238353) r?smacleod

Fix minor PEP 8 issues.

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

Comment 5

[:glob ✱]

Attachment: MozReview Request: mozreview: unify irc nick parsing and disallow periods (bug 1238353) r?smacleod

MozReview and mozautomation's commitparser disagree when it comes to deciding
if a period is valid in an IRC nick.  According to IRC a period is not valid,
which is mozautomation's behavour.  Updating mozreview and rbbz extensions to
use the RE defined in mozautomation instead of duplicating the code.

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

Comment 6

[Nick Alexander :nalexander [he/him]]

ted: your MozReview nickname is "ted.mielczarek", which has been... outlawed!  Can you update it and feel the thundering herd land on your review queue?  Thanks!

glob: other MozReview folks: some outreach is needed to move people who have a period in their nick forward.

Comment 7

[:glob ✱]

(In reply to Nick Alexander :nalexander from comment #6)
> glob: other MozReview folks: some outreach is needed to move people who have
> a period in their nick forward.

as per comment 2, only two people are impacted, and i was going to look into fixing their names directly rather than reaching out to them.

ted - don't do anything yet; this code hasn't landed.  if action is required of you, it's likely all you'd have to do is log out then back into mozreview.

Comment 8

[Steven MacLeod [:smacleod]]

Comment on attachment 8719484 [details]
MozReview Request: mozreview: fix pep 8 issues in bugzilla/models.py (bug 1238353) r?smacleod

https://reviewboard.mozilla.org/r/34971/#review32467

Comment 9

[Steven MacLeod [:smacleod]]

Comment on attachment 8719485 [details]
MozReview Request: mozreview: unify irc nick parsing and disallow periods (bug 1238353) r?smacleod

https://reviewboard.mozilla.org/r/34973/#review32471

::: pylib/mozautomation/mozautomation/commitparser.py:39
(Diff revision 1)
> +BMO_IRC_NICK_RE = re.compile(':(' + IRC_NICK + ')')

can you stick an `r` prefix on your string literals here, just to stay consistent with the rest of them and incase the regex ever grows.

Comment 10

[:glob ✱]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/894624260e8c
https://hg.mozilla.org/hgcustom/version-control-tools/rev/fc3513aad180

(setting a reminder to fix the accounts once this hits production)

Comment 11

[:glob ✱]

forgot to clear this.