1226080 - mozreview should tag all bugzilla comments with 'mozreview-...'

Status: RESOLVED FIXED

(MozReview Graveyard :: Review Board: Extension, defect)

Description

[:glob ✱]

mozreview should tag all bugzilla comments with 'review'.  this will make filtering of the comments easier on the bugzilla side, and allow for features such as bug 1225972.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

I don't think tagging them as 'review' is a good idea. 'review' looks like for review comments or result, and doesn't sound like a good tag for other operations on commits.

Comment 2

[Gregory Szorc [:gps]]

The tag should probably be something more specific than "review." We could potentially use a separate tag for each distinct MozReview action. mozreview-review, mozreview-review-reply, mozreview-submit-review-request, etc.

I'm not sure how a BMO admin feels about abusing the tags mechanism, but we could even add "hidden metadata" in the tags values so Bugzilla or even a browser extension could do crazy things like XHR into Review Board to retrieve additional data for specific comment types. We could probably enable a lot by encoding some integers in the tags. You could imagine such craziness as displaying iframes into rich diff views inline in Bugzilla. Just thinking out loud here. It's definitely scope bloat and there are plenty of service dependency, scalability, and security concerns. We should definitely start with simple tag values.

Comment 3

[:glob ✱]

(In reply to Gregory Szorc [:gps] from comment #2)
> The tag should probably be something more specific than "review." We could
> potentially use a separate tag for each distinct MozReview action.
> mozreview-review, mozreview-review-reply, mozreview-submit-review-request,
> etc.

sounds reasonable.
be aware comment tags are limited to 24 characters (and i personally think "review" > "mozreview").

> I'm not sure how a BMO admin feels about abusing the tags mechanism, but we
> could even add "hidden metadata" in the tags values [..]

comment tags are for simple user-visible properties; i don't like this :)

if you want to store additional metadata against each comment for mozreview, we'd be significantly better off extending bugzilla to do this in a way that doesn't abuse the tagging system.

Comment 4

[Mark Côté [:mcote]]

We should do this when/after we move to the new BMO batch API.

Comment 5

[Mark Côté [:mcote]]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #1)
> I don't think tagging them as 'review' is a good idea. 'review' looks like
> for review comments or result, and doesn't sound like a good tag for other
> operations on commits.

Finally about done this.  The first step will be when review requests are created or updated.  I'm thinking "review-request" for those.  Sound good?

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

What about just "mozreview"? I suppose there wouldn't be any confusion and conflict since it is the name of the product.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

And probably we could make that tag hidden by default and disallow people adding it via the UI?

Comment 8

[Gregory Szorc [:gps]]

mozreview-request - used when a MozReview review request is published
mozreview-review - when MozReview reviews/comments are published
mozreview-review-reply - a reply to a MozReview review

We arguably don't need the "reply" one (could roll into "review"). But, having it does mirror Review Board's data model. It could potentially also be used by Bugzilla or browser extensions to reinvent threaded discussion in the Bugzilla UI, filter by top-level reviews, etc. So I think it is useful to distinguish between a review and a reply to that review.

Comment 9

[Mark Côté [:mcote]]

An issue here is you need editbugs to tag comments.  You do not need editbugs for general MozReview use (posting attachments with flags and comments).  Glob, I'm always loathe to suggest special cases, but should we have a special case for mozreview-*, possibly along with what xidorn suggested in comment 7?

Comment 10

[:glob ✱]

(In reply to Mark Côté [:mcote] from comment #9)
> Glob, I'm always loathe to suggest special cases, but should we
> have a special case for mozreview-*

that should be ok, however the special-casing should happen for any tags set via mozreview's batch api, not based on tag name.

> possibly along with what xidorn suggested in comment 7?

i'm not convinced either of these are a good idea - you need editbugs to add/remove tags, so taggers are already trusted, and other comment tags set by systems haven't been problematic (eg. bugherder).

Comment 11

[Mark Côté [:mcote]]

The necessary BMO change landed last week, so I'll implement the MozReview side soon.

Comment 13

[Mark Côté [:mcote]]

Update: installation changed significantly.  After some fighting, got a vendor tarball of dependencies from dylan, which got create-test-environment running (will need a proper location for that; see bug 1282997).  Auth changed as well; there are no more auth cookies.  Had to change the login procedure to use API keys to fetch user data after logging in.  Will rip out all the cookie code later.

Currently running into an error when running `./mozreview create-user`: Fault 304: "Sorry, you aren't a member of the 'editusers' group, and so you are not authorized to add new users."  Might be an auth-related error not being caught.

Comment 14

[Mark Côté [:mcote]]

... And that update was meant for bug 1281272, not this one.

Comment 15

[Mark Côté [:mcote]]

Attachment: mozreview: Add functions to get diff URLs to rb_utils (bug 1226080);

Getting a diff URL from a review request or a review request URL is a common
operation.

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

Comment 16

[Mark Côté [:mcote]]

Attachment: mozreview: Mirror comments on parents to just the bug of the first child (bug 1226080);

This should have no real effect now because all commits in a single series are
currently associated with a single bug.  Regardless, we need to choose only
a single bug so that we can later associate the comment with a single
attachment.

If we later do allow multiple bugs in a single series, we'll have to pick
one attachment for each unique bug.  There is a TODO to that effect.

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

Comment 17

[Mark Côté [:mcote]]

Attachment: mozreview: Tag BMO comments when posting review requests (bug 1226080);

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

Comment 18

[Mark Côté [:mcote]]

Attachment: mozreview: Tag BMO comments when posting reviews and replies (bug 1226080);

The Bugzilla.post_comments() method now uses the MozReview.attachments() BMO
API, which allows tagging comments with mozreview-* tags even if the current
user is not a member of the editbugs group, which is usually required for
comment tags.

A side effect is that comments from reviews and replies will always be
associated with the corresponding MozReview attachment, which is more
appropriate than posting independent comments.  This is why the logic
around parent reviews had to change in an earlier commit to be associated
only with the bug of the first commit (and is now assoicated with that
attachment).

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

Comment 19

[:glob ✱]

Comment on attachment 8777530 [details]
mozreview: Add functions to get diff URLs to rb_utils (bug 1226080);

https://reviewboard.mozilla.org/r/69084/#review66264

Comment 20

[:glob ✱]

Comment on attachment 8777531 [details]
mozreview: Mirror comments on parents to just the bug of the first child (bug 1226080);

https://reviewboard.mozilla.org/r/69086/#review66272

Comment 21

[:glob ✱]

Comment on attachment 8777532 [details]
mozreview: Tag BMO comments when posting review requests (bug 1226080);

https://reviewboard.mozilla.org/r/69088/#review66274

Comment 22

[:glob ✱]

Comment on attachment 8777533 [details]
mozreview: Tag BMO comments when posting reviews and replies (bug 1226080);

https://reviewboard.mozilla.org/r/69090/#review66300

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/ea02257ba2b9
mozreview: Add functions to get diff URLs to rb_utils ; r=glob
https://hg.mozilla.org/hgcustom/version-control-tools/rev/03145e095c3a
mozreview: Mirror comments on parents to just the bug of the first child ; r=glob
https://hg.mozilla.org/hgcustom/version-control-tools/rev/5b846d3b23cf
mozreview: Tag BMO comments when posting review requests ; r=glob
https://hg.mozilla.org/hgcustom/version-control-tools/rev/62187bf3de98
mozreview: Tag BMO comments when posting reviews and replies ; r=glob