1244263 - Bugzilla attachment names for mozreview requests should be less heavy

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

I think one thing that subconciously makes me like MozReview less and make it feel like a "heavier" workflow is the verbosity it dumps into Bugzilla. A specific example of this is the attachment names it uses for patches. For example on bug 1201327 these are the three patch names:

MozReview Request: Bug 1201327 - Rename mDestRect to mImageLayerDestRect. r?mattwoodrow
MozReview Request: Bug 1201327 - Let DLBI detect background-position changes. r?mattwoodrow
MozReview Request: Bug 1201327 - Don't repaint the whole frame subtree when background-position changes. r?dbaron

Of these names, the "MozReview Request" is unnecessary; the attachment mime type already indicates this, and the review link already goes to mozreview. The bug number is redundant, because all bugmail/bugzilla activity already has a bug number. The r?<reviewer> is also redundant, because the reviewer gets flagged in bugzilla.

Instead I feel like the following names would be better:

Part 1/3 - Rename mDestRect to mImageLayerDestRect.
Part 2/3 - Let DLBI detect background-position changes.
Part 3/3 - Don't repaint the whole frame subtree when background-position changes.

Less useless cruft, and more in line what people actually do when uploading patches to Bugzilla. I realize that you're probably just taking the first line of the commit message, but doing some postprocessing on it might result in better UX.

Comment 1

[Botond Ballo [:botond] [away until July 9]]

On a slightly related note, it would be nice to be able to specify a custom attachment description (different from the first line of the commit message or something derived from it), the way you can for manually uploaded patches. The reason is that when you're looking at the attachments on a bug, you have some context that you don't have when looking at the commit message in version control history, so sometimes a different / briefer description is appropriate.

Comment 2

[Gregory Szorc [:gps]]

glob: will the switch to use the bulk review flag API make this less of a problem?

Comment 3

[:glob ✱]

(In reply to Gregory Szorc [:gps] from comment #2)
> glob: will the switch to use the bulk review flag API make this less of a
> problem?

no - the problem is the naming scheme of the attachments, not the api call used to create them.

currently the "MozReview Request: " string is prefixed to the review request summary (ie. the first line of the commit description).

because the summary is free-form performing fixups on it quickly becomes problematic -- yes, there's conventions, but note there are multiple conventions and because there's no enforcement there's always edge cases that make parsing and string replacement error prone.

at the very least we should stop the "mozreview request: " prefix, and remove the r?nick part using the commit rewriting method.

removing the bug number initially sounds easy, however it's the post-removal cleanup that would be problematic.

Comment 4

[:glob ✱]

Attachment: MozReview Request: mozreview: remove 'MozReview Request: ' prefix from Bugzilla attachment names (bug 1244263); r?smacleod

The 'MozReview Request: ' prefix on Bugzilla attachments adds unnecessary noise
to Bugzilla.  Remove it.

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

Comment 5

[:glob ✱]

Attachment: MozReview Request: mozreview: remove reviewers from bugzilla attachment name (bug 1244263); r?smacleod

When an attachment is created in Bugzilla ensure any review request markers
(eg. r?nick) are removed from the description.  In addition to removing
unncessary clutter, it avoids confusion when a reviewer is changed in
MozReview, which will result in a description with one reviewer, but flags with
another.

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

Comment 6

[Piotr Zalewa [:zalun]]

Comment on attachment 8756717 [details]
MozReview Request: mozreview: remove reviewers from bugzilla attachment name (bug 1244263); r?smacleod

https://reviewboard.mozilla.org/r/55354/#review52256

::: pylib/mozautomation/mozautomation/commitparser.py:122
(Diff revision 1)
>          yield reviewer
>  
>  
>  def replace_reviewers(commit_description, reviewers):
>      if not reviewers:
> -        return commit_description
> +        reviewers_str = ''

Just a question. This changes the way the replace_reviewers is working. Before for None it was doing nothing, now it strips reviewers from comment. Isn't it affecting other code?

Comment 7

[:glob ✱]

https://reviewboard.mozilla.org/r/55354/#review52256

> Just a question. This changes the way the replace_reviewers is working. Before for None it was doing nothing, now it strips reviewers from comment. Isn't it affecting other code?

yes, but there's test coverage so we should be good.

Comment 8

[Piotr Zalewa [:zalun]]

Comment on attachment 8756717 [details]
MozReview Request: mozreview: remove reviewers from bugzilla attachment name (bug 1244263); r?smacleod

https://reviewboard.mozilla.org/r/55354/#review52398

All good then

Comment 9

[Piotr Zalewa [:zalun]]

Comment on attachment 8756716 [details]
MozReview Request: mozreview: remove 'MozReview Request: ' prefix from Bugzilla attachment names (bug 1244263); r?smacleod

https://reviewboard.mozilla.org/r/55352/#review52404

::: hgext/reviewboard/tests/test-unicode.t:84
(Diff revision 1)
>    Bug 1:
>      attachments:
>      - attacher: [e-mail]
>        content_type: text/x-review-board-request
>        data: http://$DOCKER_HOSTNAME:$HGPORT1/r/2/diff/#index_header
> -      description: "MozReview Request: Bug 1 - Initial commit to review \u2019 \u3053"
> +      description: "Bug 1 - Initial commit to review \u2019 \u3053"

dump-bug before was printing the description without quotation marks, now it has them, is this inconsistency normal?

Comment 10

[Piotr Zalewa [:zalun]]

Comment on attachment 8756716 [details]
MozReview Request: mozreview: remove 'MozReview Request: ' prefix from Bugzilla attachment names (bug 1244263); r?smacleod

https://reviewboard.mozilla.org/r/55352/#review52406

Comment 11

[:glob ✱]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/0b8884a313fd
https://hg.mozilla.org/hgcustom/version-control-tools/rev/aa43b02b7565

Comment 12

[:glob ✱]

fix up the test cases added after this code was written but before it was autolanded:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/55e6f10e36d4