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

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: glob, Assigned: mcote)

Tracking

(Blocks: 2 bugs)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

3 years ago
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.
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

3 years ago
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.
(Reporter)

Comment 3

3 years ago
(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.
Product: Developer Services → MozReview
(Assignee)

Comment 4

3 years ago
We should do this when/after we move to the new BMO batch API.
Depends on: 1211791
(Assignee)

Updated

3 years ago
Component: General → Review Board: Extension
(Assignee)

Comment 5

3 years ago
(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?
What about just "mozreview"? I suppose there wouldn't be any confusion and conflict since it is the name of the product.
And probably we could make that tag hidden by default and disallow people adding it via the UI?

Comment 8

3 years ago
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.
(Assignee)

Comment 9

3 years ago
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?
Flags: needinfo?(glob)
(Reporter)

Comment 10

3 years ago
(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).
Flags: needinfo?(glob)
(Assignee)

Updated

3 years ago
Depends on: 1256051
(Assignee)

Comment 11

2 years ago
The necessary BMO change landed last week, so I'll implement the MozReview side soon.
(Assignee)

Updated

2 years ago
Assignee: nobody → mcote
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1272091
Comment hidden (typo)
(Assignee)

Comment 14

2 years ago
... And that update was meant for bug 1281272, not this one.
No longer depends on: 1282997
(Assignee)

Comment 15

2 years ago
Created attachment 8777530 [details]
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/
Attachment #8777530 - Flags: review?(glob)
Attachment #8777531 - Flags: review?(glob)
Attachment #8777532 - Flags: review?(glob)
Attachment #8777533 - Flags: review?(glob)
(Assignee)

Comment 16

2 years ago
Created attachment 8777531 [details]
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/
(Assignee)

Comment 17

2 years ago
Created attachment 8777532 [details]
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/
(Assignee)

Comment 18

2 years ago
Created attachment 8777533 [details]
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/
(Reporter)

Comment 19

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

https://reviewboard.mozilla.org/r/69084/#review66264
Attachment #8777530 - Flags: review?(glob) → review+
(Reporter)

Comment 20

2 years ago
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
Attachment #8777531 - Flags: review?(glob) → review+
(Reporter)

Comment 21

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

https://reviewboard.mozilla.org/r/69088/#review66274
Attachment #8777532 - Flags: review?(glob) → review+
(Reporter)

Comment 22

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

https://reviewboard.mozilla.org/r/69090/#review66300
Attachment #8777533 - Flags: review?(glob) → review+

Comment 23

2 years ago
Pushed by mcote@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Summary: mozreview should tag all bugzilla comments with 'review' → mozreview should tag all bugzilla comments with 'mozreview-...'
(Assignee)

Updated

2 years ago
Blocks: 1211791
No longer depends on: 1211791
You need to log in before you can comment on or make changes to this bug.