Closed Bug 1197879 Opened 9 years ago Closed 8 years ago

use BMO-style review flag (r?/r+/r-) instead of "ship it"

Categories

(MozReview Graveyard :: General, defect)

Product:
MozReview Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: bkelly, Assigned: mdoglio)

References

Details

Attachments

(9 files, 1 obsolete file)

Screen Shot 2015-11-30 at 3.18.13 PM.png
89.61 KB, image/png
Details
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley
58 bytes, text/x-review-board-request
mconley
: review+
smacleod
: review+
Details
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod
58 bytes, text/x-review-board-request
smacleod
: review+
Details
MozReview Request: mozreview: add set_attachment_flag to bugzilla client (bug 1197879); r?smacleod r?mcote
58 bytes, text/x-review-board-request
mcote
: review+
smacleod
: review+
Details
MozReview Request: mozreview: use set_attachment_flag on review publishing (bug 1197879) r?smacleod r?mcote
58 bytes, text/x-review-board-request
mcote
: review+
smacleod
: review+
Details
MozReview Request: mozreview: show review flag in review request history (bug 1197879); r?smacleod
58 bytes, text/x-review-board-request
smacleod
: review+
Details
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote
58 bytes, text/x-review-board-request
mcote
: review+
smacleod
: review+
Details
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod
58 bytes, text/x-review-board-request
mcote
: review+
smacleod
: review+
Details
MozReview Request: docs: Update user docs to reflect new flag system (bug 1197879); r?glob
58 bytes, text/x-review-board-request
glob
: review+
Details 
Is there any way to r- a patch?  Either an explicit r- or just dropping the review flag would be adequate.  All I see is the Ship It button.  ("Ship It" is terrible, btw, because I almost never want to say that.  Instead I usually need "r+ with comments addressed".)

I don't see how to r- in the docs either:

  https://www.reviewboard.org/docs/manual/2.0/users/reviews/publishing/

Right now mozreview is materially worse than splinter for me because it requires extra steps to manually clear the review flag.
Publishing a review without a ship it will clear the review flag.  Hitting "ship it" with issues opened in Review Board is the equivalent to "r+ with nits"; it will r+ in Bugzilla and show the nits, and in Review Board it will read "Fix it, then ship it" until all the issues are closed, at which it will just read "ship it".

Btw the docs you linked to are good for basic Review Board usage, but for the bigger MozReview system, including Bugzilla integration, you should check the MozReview manual: http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/  See http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/reviewboard.html#reviewing-code in particular for your questions.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
This has not been my experience.  For example, see:

  https://bugzilla.mozilla.org/show_bug.cgi?id=1189661#c3

My review flag is still set there and I used bugzilla to flip it to r-.

Also, I don't think its reasonable UX to modify the review flag as an implicit side effect of publishing review comments.

The review flag is a first class part of our review policy and should be managed explicitly.  Magically changing the flag without indication on the mozreview UI (even "Ship It" doesn't say it will change the bugzilla review flag) makes me distrust the entire system.

Essentially it seems like mozreview is missing a high level view like splinter's overview which lets me see a summary of everything I've written, see what the current state of the flags on the patch are, and then set the flags on the patch.
Status: RESOLVED → REOPENED
Flags: needinfo?(mcote)
Resolution: WORKSFORME → ---
(In reply to Mark Côté [:mcote] from comment #1)
> Btw the docs you linked to are good for basic Review Board usage, but for
> the bigger MozReview system, including Bugzilla integration, you should
> check the MozReview manual:

I clicked Support->Documentation in mozreview to get there. Don't blame me.
(In reply to Ben Kelly [:bkelly] from comment #2)
> This has not been my experience.  For example, see:
> 
>   https://bugzilla.mozilla.org/show_bug.cgi?id=1189661#c3
> 
> My review flag is still set there and I used bugzilla to flip it to r-.

Ah.  This is a problem we're struggling with, in which people are putting reviews on the parent review request and not on the commits.  This tends to only be a problem when there's a single commit.  We're going to make it more obvious that you need to do your reviews on the child, possibly by omitting the parent altogether (it's less useful than we originally thought to see a squashed view of multiple commits, which was its original purpose).

> Also, I don't think its reasonable UX to modify the review flag as an
> implicit side effect of publishing review comments.
> 
> The review flag is a first class part of our review policy and should be
> managed explicitly.  Magically changing the flag without indication on the
> mozreview UI (even "Ship It" doesn't say it will change the bugzilla review
> flag) makes me distrust the entire system.

I'm not sure how to respond to this.  I guess it's mainly an education thing; we need to do more training.  The mirroring to Bugzilla is a convenience (with some problems like I mentioned above, which we're working on); we want MozReview to be the central place where code review happens, since it is much more powerful than Bugzilla, which should be a place to discuss problems and solutions, but not the particulars of code.  Few people want to do the extra step of modifying the review flag after doing a review in MozReview.

> Essentially it seems like mozreview is missing a high level view like
> splinter's overview which lets me see a summary of everything I've written,
> see what the current state of the flags on the patch are, and then set the
> flags on the patch.

Straddling two systems is really difficult.  Once we have autoland from MozReview, the status there (ship its) will be even more important.

I realize this isn't perfect, but Bugzilla's review system is inherently limited.  We've been struggling with how to extend it and make it more powerful for years, and we realized we just need a dedicated code-review system.  This is a painful time where people are using both Bugzilla's review system and MozReview at the same time.  It's our belief that with more education and more UX improvements, the use of Bugzilla for code review will fade into the background.

(In reply to Ben Kelly [:bkelly] from comment #3)
> (In reply to Mark Côté [:mcote] from comment #1)
> > Btw the docs you linked to are good for basic Review Board usage, but for
> > the bigger MozReview system, including Bugzilla integration, you should
> > check the MozReview manual:
> 
> I clicked Support->Documentation in mozreview to get there. Don't blame me.

I apologize; I certainly didn't mean to blame you.  I just wanted to point it out; it's under MozReview->User Guide.
Flags: needinfo?(mcote)
(In reply to Mark Côté [:mcote] from comment #4)
> Ah.  This is a problem we're struggling with, in which people are putting
> reviews on the parent review request and not on the commits.  This tends to
> only be a problem when there's a single commit.  We're going to make it more
> obvious that you need to do your reviews on the child, possibly by omitting
> the parent altogether (it's less useful than we originally thought to see a
> squashed view of multiple commits, which was its original purpose).

I click "complete diff" because that's the only way I've figured out how to consistently see all the changes I'm being asked to review.

> > Also, I don't think its reasonable UX to modify the review flag as an
> > implicit side effect of publishing review comments.
> > 
> > The review flag is a first class part of our review policy and should be
> > managed explicitly.  Magically changing the flag without indication on the
> > mozreview UI (even "Ship It" doesn't say it will change the bugzilla review
> > flag) makes me distrust the entire system.
> 
> I'm not sure how to respond to this.  I guess it's mainly an education
> thing; we need to do more training.

I never needed training to figure out how to do reviews in splinter.  I find mozreview *much* harder to understand and never really know if I'm doing the right thing.  I don't think I'm the only one that feels that way as well.  For example, there are some reviewers who "r- for using mozreview" right now.

Please consider the real feedback that people find mozreview hard to use before pushing it out to everyone and deprecating the docs to the old system.

> The mirroring to Bugzilla is a
> convenience (with some problems like I mentioned above, which we're working
> on); we want MozReview to be the central place where code review happens,
> since it is much more powerful than Bugzilla, which should be a place to
> discuss problems and solutions, but not the particulars of code.  Few people
> want to do the extra step of modifying the review flag after doing a review
> in MozReview.

I'm not asking for an extra step.  I'm asking for the tool to make the flag state explicit.  (Also, I'd love to see the survey, focus group, or other research you did to determine what reviewers want.)

And I don't really care if the flag lives in bugzilla or elsewhere.  What I care about is the policy decision being communicated.  "I reviewed this code and I agree its ok to land."  Or "I reviewed this code and I am explicitly telling you its not ok to land."  Those should not be magical side effects.

> Straddling two systems is really difficult.  Once we have autoland from
> MozReview, the status there (ship its) will be even more important.

So its not clear now that ship it or other actions effect the review flag.  Now you want it to commit to the tree directly?  This terrifies me with the current, confusing UX.

Also, as a user I don't really care about the underlying architecture of the review system.  I see no reason we have to "straddle two systems".  It seems that's something we imposed on ourselves.

> I realize this isn't perfect, but Bugzilla's review system is inherently
> limited.  We've been struggling with how to extend it and make it more
> powerful for years, and we realized we just need a dedicated code-review
> system.  This is a painful time where people are using both Bugzilla's
> review system and MozReview at the same time.  It's our belief that with
> more education and more UX improvements, the use of Bugzilla for code review
> will fade into the background.

I totally agree splinter is limited.  I've been asked to make a bazillion interdiffs for a bug before and it sucks.  But it does have a clear user interface and work flow.

Right now I (and other reviews AFIACT) would rather deal with splinter's limitations than struggle with the confusing interface currently in mozreview.

> (In reply to Ben Kelly [:bkelly] from comment #3)
> > I clicked Support->Documentation in mozreview to get there. Don't blame me.
> 
> I apologize; I certainly didn't mean to blame you.  I just wanted to point
> it out; it's under MozReview->User Guide.

I guess I will add having two different help menus to the list of confusing things in mozreview.

Seriously, the user should not have to understand that this is built on a generic project that was then customized to be mozilla specific.
Flags: needinfo?(mcote)
(In reply to Ben Kelly [:bkelly] from comment #5)
> (In reply to Mark Côté [:mcote] from comment #4)
> > Ah.  This is a problem we're struggling with, in which people are putting
> > reviews on the parent review request and not on the commits.  This tends to
> > only be a problem when there's a single commit.  We're going to make it more
> > obvious that you need to do your reviews on the child, possibly by omitting
> > the parent altogether (it's less useful than we originally thought to see a
> > squashed view of multiple commits, which was its original purpose).
> 
> I click "complete diff" because that's the only way I've figured out how to
> consistently see all the changes I'm being asked to review.

Yes, that's the one use for it.  What I need to do (and will do shortly) is make it clear that reviews need to be done on the individual child commits, not the parent, since the commits are what are landing (similar to a Bugzilla bug with multiple patches attached).  I know this is pretty unclear right now.

> > > Also, I don't think its reasonable UX to modify the review flag as an
> > > implicit side effect of publishing review comments.
> > > 
> > > The review flag is a first class part of our review policy and should be
> > > managed explicitly.  Magically changing the flag without indication on the
> > > mozreview UI (even "Ship It" doesn't say it will change the bugzilla review
> > > flag) makes me distrust the entire system.
> > 
> > I'm not sure how to respond to this.  I guess it's mainly an education
> > thing; we need to do more training.
> 
> I never needed training to figure out how to do reviews in splinter.  I find
> mozreview *much* harder to understand and never really know if I'm doing the
> right thing.  I don't think I'm the only one that feels that way as well. 
> For example, there are some reviewers who "r- for using mozreview" right now.

We just rolled out what I hope are a couple small improvements this week.  I hope you agree that they're a step in the right direction; we have work in progress to continue to make it clearer who has granted a review and who hasn't.

> Please consider the real feedback that people find mozreview hard to use
> before pushing it out to everyone and deprecating the docs to the old system.

I apologize if it seems like we aren't considering feedback.  I think I have mistaken your comments, thinking they were about Bugzilla integration when they weren't.  As with any software we can't make everyone happy, but we definitely consider all feedback, and if it sounds like we're just ignoring it, it's either us not understanding or (more likely) us not having time yet to address the concerns.  We regularly talk to (and hopefully not at) our users.

I'm not sure what you mean by "deprecating the docs", though--we aren't planning to deprecate anything anytime soon.

> > The mirroring to Bugzilla is a
> > convenience (with some problems like I mentioned above, which we're working
> > on); we want MozReview to be the central place where code review happens,
> > since it is much more powerful than Bugzilla, which should be a place to
> > discuss problems and solutions, but not the particulars of code.  Few people
> > want to do the extra step of modifying the review flag after doing a review
> > in MozReview.
> 
> I'm not asking for an extra step.  I'm asking for the tool to make the flag
> state explicit.  (Also, I'd love to see the survey, focus group, or other
> research you did to determine what reviewers want.)
> 
> And I don't really care if the flag lives in bugzilla or elsewhere.  What I
> care about is the policy decision being communicated.  "I reviewed this code
> and I agree its ok to land."  Or "I reviewed this code and I am explicitly
> telling you its not ok to land."  Those should not be magical side effects.

This is where I think I misunderstood you.  We're improving the summary table to be clearer as to the state of all reviews.

> > Straddling two systems is really difficult.  Once we have autoland from
> > MozReview, the status there (ship its) will be even more important.
> 
> So its not clear now that ship it or other actions effect the review flag. 
> Now you want it to commit to the tree directly?  This terrifies me with the
> current, confusing UX.

Fair enough.  I hope no one will feel compelled to use it if they are unsure as to what they will be landing.
Flags: needinfo?(mcote)
I tried to r- the patch here: https://reviewboard.mozilla.org/r/19415/#review17385

It cleared the review? flag in Bugzilla, but inside mozreview the patch still shows as r?

Clearing the review? flag is typically done to indicate the wrong person was asked to review, AFAIK, so it's quite different from r- ("bad idea").

I'd also like to echo pretty much everything said in Comment 5. I cringe every time I get a bug with a mozreview, because I know it's going to take me significantly longer to even get very basic things accomplished, it's never intuitive how something should be done, what will happen when you do it, and even simple UX interactions don't have the expected result. (Clicking bug nr to go to the bug? Nope Clicking r?  to set review status? Nope)

How much software have you used in the last 10 years where you had to use the manual to get basic interactions done? If you have to use the manual to figure out how to r- a patch, IMHO the issue isn't user training, it's unacceptably bad UX.
Yes, it's still confusing, and we know the UX is bad.  We've been too focussed on big-ticket items like Autoland, since we had a number of early users who understood the MozReview parent-child model (apparently the biggest source of confusion) and were relatively content.  We haven't been devoting sufficient time to improvements that will help people who find MozReview confusing.  We're shifting into that and trying to find more resources.  Please bear with us for a bit longer.
(Just capturing what I complained on IRC)
A case which happened to me today. A patch had got r+ from someone and was waiting for r? from me. I wanted to explicitly give r-, but since MozReview doesn't have a way to do that, my r? flag was just removed and from bugzilla it looked like the patch got review.
I had to explicitly r- in bugzilla.
Reasonably certain we can monkeypatch and extend RB.ReviewDialogView to hide the ship-it checkbox and show this instead. Then we could store the r?/r-/r+ state in the extra_data of a review.

If we did this, we'd need to move the autoland logic off of the ship-it flag. We'd also need to decide how autoland should react to someone having given an r+, and another given an r-, and how to reconcile that.
(Note that I'd accidentally flipped the positions of r- and r+ in my mock-up - these should probably match the order that people are used to from Bugzilla).
mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?mcote
Attachment #8695132 - Flags: review?(mcote)
So this change to MozReview adds the r?, r+, r- dropdown to the Review Dialog, much like the mock-up in attachment 8693744 [details].

It also hides the Ship It checkbox.

Setting this flag and hitting save will save "", "r?", "r+", or "r-" in the Review model's extra_data with the p2rb.review_state key.

I have no idea if that's where you want me to store it, so I took a guess. Let me know if you want it moved somewhere else.

If we go this route, we'll probably want a way of displaying r?, r+ and r- counts in the dashboard with a custom column. We'll also probably want to update the commits table to display the right thing.

And we'll also need to update autoland to somehow use this flag instead of Ship It to determine whether or not something can be landed.

Anyhow, it's a start.
Comment on attachment 8695132 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27021/diff/1-2/
Thanks, I think making Mozreview to use the same terminology as bugzilla would help quite a bit with
all the confusion.
https://reviewboard.mozilla.org/r/27021/#review24441

::: pylib/mozreview/mozreview/extension.py:94
(Diff revision 1)
> -    js_extensions = [ParentJSExtension]
> +    js_extensions = [ParentJSExtension, RPlusAndMinusExtension]

While I'm not sure a convention even exists for this, I wonder if best practice would be to create many small JS extensions, or a larger one.

What worries me is if we want to start having these flags affected by other data/settings which comes from other parts of MozReview do we want everything namespaced into little extensions? or one object we can work with?

This isn't a hard blocker or anything and I'd probably trust your opinion on this much more than mine, just thought it was worth bringing up.
Comment on attachment 8695132 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?mcote

https://reviewboard.mozilla.org/r/27021/#review24481

I like the general idea (and roughly agree with smacleod's & glob's comments), but I will defer code review to smacleod.
Attachment #8695132 - Flags: review?(mcote)
We kind of got caught in a flag-naming quagmire in Mozlando, but this is still something that a number of our reviewers are asking for.

Can we try to ship the r?, r-, r+ scheme for now until we can come up with something better?
Hey mcote,

I'm hearing about this one a lot. Do you think we can push ahead with just having r?, r-, r+ for now, until we can come up with something better?
Flags: needinfo?(mcote)
Yes, I think, for now at least, we should move to the same flag(s) as in BMO.  I don't have time to review this, though, as I'm working on some larger vision/plans for MozReview.  I'm good with the screenshot you attached though.  Can you reassign the review request to a MozReview dev?
Flags: needinfo?(mcote)
Product: Developer Services → MozReview
Comment on attachment 8695132 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?mcote

Could you take a look at this?  It almost certainly will require a rebase, though, so maybe I could get you to take it over, if so.
Attachment #8695132 - Flags: review?(mdoglio)
Assignee: nobody → mdoglio
Status: REOPENED → ASSIGNED
Comment on attachment 8725302 [details]
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37413/diff/1-2/
Attachment #8725302 - Attachment description: MozReview Request: mozreview: [wip] show review status in the commits table → MozReview Request: mozreview: [wip] show review status in the commits table (bug 1197879)
The new function will make it easier to transition to the new flag-based review status.

Review commit: https://reviewboard.mozilla.org/r/38205/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38205/
This is just to prepare the ground for when we have the new review state
key in the review extra_data.

Review commit: https://reviewboard.mozilla.org/r/38207/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38207/
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37411/diff/1-2/
Comment on attachment 8725302 [details]
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37413/diff/2-3/
Attachment #8725302 - Attachment description: MozReview Request: mozreview: [wip] show review status in the commits table (bug 1197879) → MozReview Request: mozreview: show review flag in the commits table (bug 1197879)
Attachment #8725301 - Flags: review?(mcote)
Without reading through all of the comments, will the patches in this bug allow me to submit some comments without clearing the review request? In my particular instance, I want to send some preliminary feedback before I am able to finish the full review, which is why I want to keep the r? active on the patch.
Flags: needinfo?(mdoglio)
jaws: Yes, you'll be able to choose whether to leave it as r? or set it to r- or r+, pretty much the same as with BMO attachments today.  I'm updating the summary to reflect that.
Flags: needinfo?(mdoglio)
Summary: mozreview has no way to r- a patch → use BMO-style review flag (r?/r+/r-) instead of "ship it"
Attachment #8695132 - Attachment is obsolete: true
Attachment #8695132 - Flags: review?(mdoglio)
Blocks: 1257229
Review commit: https://reviewboard.mozilla.org/r/45271/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45271/
Attachment #8726732 - Attachment description: MozReview Request: mozreview: add set_attachment_flag function to bugzilla client (bug 1197879) → MozReview Request: mozreview: add set_attachment_flag to bugzilla client (bug 1197879); r?smacleod r?mcote
Attachment #8726733 - Attachment description: MozReview Request: mozreview: use set_attachment_flag on review publishing (bug 1197879) → MozReview Request: mozreview: use set_attachment_flag on review publishing (bug 1197879) r?smacleod r?mcote
Attachment #8725301 - Attachment description: MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?mcote → MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley
Attachment #8725302 - Attachment description: MozReview Request: mozreview: show review flag in the commits table (bug 1197879) → MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod
Attachment #8739438 - Flags: review?(smacleod)
Attachment #8739439 - Flags: review?(smacleod)
Attachment #8739439 - Flags: review?(mcote)
Attachment #8726732 - Flags: review?(smacleod)
Attachment #8726732 - Flags: review?(mcote)
Attachment #8726733 - Flags: review?(smacleod)
Attachment #8726733 - Flags: review?(mcote)
Attachment #8725301 - Flags: review?(smacleod)
Attachment #8725301 - Flags: review?(mconley)
Attachment #8725302 - Flags: review?(smacleod)
Now that the reviewer can explicitly set review flags, we can stop clearing
those flags on comment. As a result, a flag set on mozreview is always mirrored to bugzilla.
In case of a r? -> r+ -> r? status transition, the final flag will have the reviewer set as
both the setter and the requestee.

Review commit: https://reviewboard.mozilla.org/r/45273/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45273/
Comment on attachment 8726732 [details]
MozReview Request: mozreview: add set_attachment_flag to bugzilla client (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38205/diff/1-2/
Comment on attachment 8726733 [details]
MozReview Request: mozreview: use set_attachment_flag on review publishing (bug 1197879) r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38207/diff/1-2/
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37411/diff/2-3/
Comment on attachment 8725302 [details]
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37413/diff/3-4/
Comment on attachment 8725302 [details]
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37413/diff/4-5/
Comment on attachment 8739438 [details]
MozReview Request: mozreview: show review flag in review request history (bug 1197879); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45271/diff/1-2/
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45273/diff/1-2/
https://reviewboard.mozilla.org/r/37411/#review41757

::: pylib/mozreview/mozreview/static/mozreview/js/parents.js:111
(Diff revision 3)
> +  },
> +
> +  render: function() {
> +    this.$el.html(this.template({
> +      states: this.states,
> +      val: this.model.get('extraData')[this.key] || 'r?'

As discussed, we should set the flag to the value in the most recent review from that user, if any (otherwise r?).
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

https://reviewboard.mozilla.org/r/37411/#review41737

::: pylib/mozreview/mozreview/extension.py:105
(Diff revision 3)
> +class RPlusAndMinusExtension(JSExtension):
> +    model_class = 'MRPlusAndMinus.Extension'

Probably should name this `class MRPlusAndMinusExtension` to be consistent with the model class. I know that is kind of a leftover fragment from my original patch - so it's my bad.

::: pylib/mozreview/mozreview/static/mozreview/js/parents.js:66
(Diff revision 3)
> +  /**
> +   * These are the possible states that will be shown in the dropdown,
> +   * in order. These are also the values that will be stored in the
> +   * extraData field.
> +   */
> +  states: [" ", "r?", "r+", "r-"],

There's a lot of mix between single and double-quotes. I see more single quotes being used elsewhere in this file, so maybe we should be consistent there.

::: pylib/mozreview/mozreview/static/mozreview/js/parents.js:96
(Diff revision 3)
> +  /**
> +  * Under some circumstances a model save will replace the comment textarea
> +  * with a 'Add text' link. This is a hack to circumvent that problem.
> +  */

I think I need some more information about this. Which comment text area? The header text input on the review dialog? Which "Add text" link?

::: pylib/mozreview/mozreview/static/mozreview/js/parents.js:134
(Diff revision 3)
> +    // TODO: move the MRPlusMinusExtension to the same bundle as init_rr.js
> +    // so that we can reuse Mozreview.isParent.

Can you please file a bug for this TODO and put the bug number into the comment?
Attachment #8725301 - Flags: review?(mconley)
Blocks: 1263591
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37411/diff/3-4/
Attachment #8725301 - Flags: review?(mconley)
Comment on attachment 8725302 [details]
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37413/diff/5-6/
Comment on attachment 8739438 [details]
MozReview Request: mozreview: show review flag in review request history (bug 1197879); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45271/diff/2-3/
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45273/diff/2-3/
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

https://reviewboard.mozilla.org/r/37411/#review42003

::: pylib/mozreview/mozreview/extension.py:105
(Diff revision 4)
>  class ParentJSExtension(JSExtension):
>      model_class = 'MRParents.Extension'
>      apply_to = review_request_url_names
>  
>  
> +class MRPlusAndMinusExtension(JSExtension):

Please see https://reviewboard.mozilla.org/r/27021/#comment32689
Attachment #8725301 - Flags: review?(smacleod)
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

https://reviewboard.mozilla.org/r/37411/#review42403

Still waiting on the issue I opened in my last review.

::: pylib/mozreview/mozreview/static/mozreview/js/parents.js:87
(Diff revision 4)
> +    * The first time a ReviewDialogHook is rendered, it will then retrieve
> +    * the model data off the server and update the extraData. We have to
> +    * wait for that update, and then re-render in order to show the value
> +    * that is being stored server-side.
> +    */

Nit: Busted alignment - please indent one more space.

::: pylib/mozreview/mozreview/static/mozreview/js/parents.js:97
(Diff revision 4)
> +  * Under some circumstances a model save will replace the comment textarea
> +  * with a 'Add text' link. This is a hack to circumvent that problem.
> +  */

Same as above, re: alignment
Attachment #8725301 - Flags: review?(mconley)
Comment on attachment 8726732 [details]
MozReview Request: mozreview: add set_attachment_flag to bugzilla client (bug 1197879); r?smacleod r?mcote

https://reviewboard.mozilla.org/r/38205/#review42417

::: pylib/mozreview/mozreview/bugzilla/client.py:383
(Diff revision 2)
>      @xmlrpc_to_bugzilla_errors
> -    def r_plus_attachment(self, bug_id, reviewer, rb_url, comment=None):
> -        """Set a review flag to "+".
> +    def set_attachment_flag(self, bug_id, flag, reviewer,
> +                            rb_url, comment=None):
> +        """Create, update or clear an attachment flag.
>  
> -        Does nothing if the reviewer has already r+ed the attachment.
> +        Does nothing if the reviewer has already set the given flag to the

Nit: "on the attachment".

::: pylib/mozreview/mozreview/bugzilla/client.py:386
(Diff revision 2)
>  
> -        Does nothing if the reviewer has already r+ed the attachment.
> -        Updates flag if a corresponding r? is found; otherwise creates a
> +        Does nothing if the reviewer has already set the given flag to the
> +        attachment.
> -        new flag.
>  
> -        We return a boolean indicating whether we r+ed the attachment.
> +        Returns a boolean indicating whether we flagged the attachment.

This also returns True if the attachment flag was cleared ("flagged the attachment" sounds like only if a flag was set).

::: pylib/mozreview/mozreview/bugzilla/client.py:428
(Diff revision 2)
> +                if flag == '?':
> +                    flag_obj['status'] = 'X'

Why are we always clearing r?s?  Should we not just leave it if it's currently set correctly?

::: pylib/mozreview/mozreview/bugzilla/client.py:445
(Diff revision 2)
> -            flag['requestee'] = reviewer
> +                # This should never happen, but if it does log the error.
> +                logger.error('No flag to clear for %s on %s' % (
> +                    reviewer, rb_attachment
> +                ))
> +                return False
> +            elif flag not in ('+', '-'):

I thought we were going to create a r? if it's missing, just from the reviewer to the reviewer (instead of from the author to the reviewer).

::: pylib/mozreview/mozreview/bugzilla/client.py:446
(Diff revision 2)
> +                logger.error('No flag to clear for %s on %s' % (
> +                    reviewer, rb_attachment
> +                ))
> +                return False
> +            elif flag not in ('+', '-'):
> +                # If the reviewer has no +/- set and he submits a comment

Nit: I think we have this elsewhere, but I'd prefer gender-neutral language in comments (e.g. "they submit a comment").
Attachment #8726732 - Flags: review?(mcote)
https://reviewboard.mozilla.org/r/38205/#review42565

::: pylib/mozreview/mozreview/bugzilla/client.py:479
(Diff revision 2)
> +        new flag.
> +
> +        We return a boolean indicating whether we r+ed the attachment.
> +        """
> +
> +        return self.set_attachment_flag(bug_id, '+', reviewer, rb_url, comment)

I see this (and below, in cancel_review_request) was fixed in the next commit, but it should be fixed here instead ('+' -> 'r+' and 'X' -> '').
Comment on attachment 8726733 [details]
MozReview Request: mozreview: use set_attachment_flag on review publishing (bug 1197879) r?smacleod r?mcote

https://reviewboard.mozilla.org/r/38207/#review42563

Giving this a ship-it because two of the issues are minor and the other will be addressed in the previous commit.

::: pylib/mozreview/mozreview/bugzilla/client.py:479
(Diff revision 2)
>          new flag.
>  
>          We return a boolean indicating whether we r+ed the attachment.
>          """
>  
> -        return self.set_attachment_flag(bug_id, '+', reviewer, rb_url, comment)
> +        return self.set_attachment_flag(bug_id, 'r+', reviewer, rb_url, comment)

As I noted in my review of the previous commit, this fix and the fix in the function below belong over there.

::: pylib/mozreview/mozreview/signal_handlers.py:567
(Diff revision 2)
> +    This is needed to give a default value to the REVIEW_STATE_KEY
> +    extra_data key.
> +    """
> +    review = kwargs["instance"]
> +
> +

Nit: too many blank lines.

::: pylib/rbbz/rbbz/extension.py:124
(Diff revision 2)
> +            # If for some reasons we don't have the flag set in extra_data,
> +            # fall back to ship_it

This is generally unexpected right?  I would log a warning.
Attachment #8726733 - Flags: review?(mcote) → review+
https://reviewboard.mozilla.org/r/38205/#review42417

> I thought we were going to create a r? if it's missing, just from the reviewer to the reviewer (instead of from the author to the reviewer).

Just dropped this and the previous issues as I see you address them in your final commit.  Perhaps you could amend the commit message here to say that the current behaviour, in which an r? is always either turned into an r+ or cancelled after a review, is being preserved here for now.
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

https://reviewboard.mozilla.org/r/45273/#review42577

::: pylib/mozreview/mozreview/bugzilla/client.py:444
(Diff revision 3)
> +
> +        # If the reviewer is setting the flag back to ?,
> +        # he will then be both the setter and the requestee
> +        if flag == '?':

We only need to do this if the r? isn't set correctly, right?  If the original r? is still present, with the correct setter, then don't modify the flag.
Attachment #8739439 - Flags: review?(mcote)
When the user opens up the review dialog they will see the review flag initially set to the
flag of the last review they gave on that review request or 'r?' if none is present.

Review commit: https://reviewboard.mozilla.org/r/46105/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46105/
Attachment #8740986 - Flags: review?(smacleod)
Attachment #8740986 - Flags: review?(mcote)
Attachment #8739439 - Flags: review?(mcote)
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45273/diff/3-4/
https://reviewboard.mozilla.org/r/37411/#review41757

> As discussed, we should set the flag to the value in the most recent review from that user, if any (otherwise r?).

This is fixed in the last commit of the series
https://reviewboard.mozilla.org/r/37411/#review41737

> I think I need some more information about this. Which comment text area? The header text input on the review dialog? Which "Add text" link?

Yes, that's the textarea. When you change the review status flag for the first time that textarea disappears and a preview ov its content is rendered instead. More or less like when you click the 'ok' button underneath it. The 'Add text' link shows up when do the same process and the textarea is empty. So again, it's as if you clicked the 'ok' button.
https://reviewboard.mozilla.org/r/37411/#review42003

> Please see https://reviewboard.mozilla.org/r/27021/#comment32689

I'll let :mconley reply to this, since he knows better than me what's the best practice here.
https://reviewboard.mozilla.org/r/45273/#review42577

> We only need to do this if the r? isn't set correctly, right?  If the original r? is still present, with the correct setter, then don't modify the flag.

This is to cover the case where the users give a r+, publish the review and then change their mind and revert it to r?.
Comment on attachment 8726732 [details]
MozReview Request: mozreview: add set_attachment_flag to bugzilla client (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38205/diff/2-3/
Attachment #8726732 - Flags: review?(mcote)
Attachment #8725301 - Flags: review?(smacleod)
Attachment #8725301 - Flags: review?(mconley)
Comment on attachment 8726733 [details]
MozReview Request: mozreview: use set_attachment_flag on review publishing (bug 1197879) r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38207/diff/2-3/
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37411/diff/4-5/
Comment on attachment 8725302 [details]
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37413/diff/6-7/
Comment on attachment 8739438 [details]
MozReview Request: mozreview: show review flag in review request history (bug 1197879); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45271/diff/3-4/
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45273/diff/4-5/
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46105/diff/1-2/
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

https://reviewboard.mozilla.org/r/37411/#review42631

Thanks mdog!

::: pylib/mozreview/mozreview/static/mozreview/js/parents.js:120
(Diff revision 5)
> +    if (val === "") {
> +        val = " ";

Single-quotes please
Attachment #8725301 - Flags: review?(mconley) → review+
https://reviewboard.mozilla.org/r/45273/#review42577

> This is to cover the case where the users give a r+, publish the review and then change their mind and revert it to r?.

Right, but it doesn't look like we preserve the original r? in the case where it doesn't need to be changed.  This would be nicer, rather than always replacing it.
https://reviewboard.mozilla.org/r/45273/#review42577

> Right, but it doesn't look like we preserve the original r? in the case where it doesn't need to be changed.  This would be nicer, rather than always replacing it.

The if clause at line 428 covers that case, doesn't it?
https://reviewboard.mozilla.org/r/45273/#review42577

> The if clause at line 428 covers that case, doesn't it?

Ah right!  Nevermind me.
https://reviewboard.mozilla.org/r/37411/#review41737

> Yes, that's the textarea. When you change the review status flag for the first time that textarea disappears and a preview ov its content is rendered instead. More or less like when you click the 'ok' button underneath it. The 'Add text' link shows up when do the same process and the textarea is empty. So again, it's as if you clicked the 'ok' button.

I'll mark this issue as fixed, feel free to reopen it if you think there's a better way to circumvent (or even fix!) the problem.
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37411/diff/5-6/
Comment on attachment 8725302 [details]
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37413/diff/7-8/
Comment on attachment 8739438 [details]
MozReview Request: mozreview: show review flag in review request history (bug 1197879); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45271/diff/4-5/
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45273/diff/5-6/
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46105/diff/2-3/
https://reviewboard.mozilla.org/r/37411/#review42003

> I'll let :mconley reply to this, since he knows better than me what's the best practice here.

On a second thought, I think we can eventually merge the 2 JSExtension classes in a separate patch. I'll ni mconley on the bug to understand if we need to, but I would prefer to not increase the scope of this patch. I'll drop this issue, feel free to reopen if you have objections.
:mconley do you have any opinion on this? https://reviewboard.mozilla.org/r/37411/#comment54633
Flags: needinfo?(mconley)
https://reviewboard.mozilla.org/r/37411/#review42003

> On a second thought, I think we can eventually merge the 2 JSExtension classes in a separate patch. I'll ni mconley on the bug to understand if we need to, but I would prefer to not increase the scope of this patch. I'll drop this issue, feel free to reopen if you have objections.

Let's file a follow-up and deal with potentially merging these later.
Flags: needinfo?(mconley)
Comment on attachment 8726732 [details]
MozReview Request: mozreview: add set_attachment_flag to bugzilla client (bug 1197879); r?smacleod r?mcote

https://reviewboard.mozilla.org/r/38205/#review43095
Attachment #8726732 - Flags: review?(mcote) → review+
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

https://reviewboard.mozilla.org/r/45273/#review43099

::: pylib/mozreview/mozreview/bugzilla/client.py:447
(Diff revision 6)
>  
>              # Flag not found, let's create a new one.
>              flag_obj['new'] = True
> +
> +        # If the reviewer is setting the flag back to ?,
> +        # he will then be both the setter and the requestee

Missed this the first time, but s/he/they.  This can be cleaned up after though.
Attachment #8739439 - Flags: review?(mcote) → review+
Attachment #8740986 - Flags: review?(mcote)
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

https://reviewboard.mozilla.org/r/46105/#review43101

At least some, if not all, of these tests belong in the commit before this one.

::: pylib/mozreview/mozreview/signal_handlers.py:577
(Diff revision 3)
> +        reviewers_status = get_reviewers_status(review.review_request)
> +
> +        try:
> +            flag = reviewers_status[review.user.username]['review_status']
> +        except KeyError:
> +            flag='r?'

Nit: spaces around =.

::: pylib/mozreview/mozreview/static/mozreview/js/parents.js
(Diff revision 3)
>      // the api endpoint will ignore it otherwise.
>      if (val === '') {
>          val = ' ';
>      }
>      this.model.get('extraData')[this.key] = val;
> -    $('#id_shipit').prop('checked', (val == 'r+'));

It's not clear to me what's in this commit that lets us delete this...
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45273/diff/6-7/
Attachment #8740986 - Flags: review?(mcote)
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46105/diff/3-4/
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45273/diff/7-8/
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46105/diff/4-5/
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46105/diff/5-6/
https://reviewboard.mozilla.org/r/46105/#review43101

> It's not clear to me what's in this commit that lets us delete this...

The value of ship_it is now set up in the pre_save_review signal handler, so we don't need that line anymore.
https://reviewboard.mozilla.org/r/46105/#review43101

> The value of ship_it is now set up in the pre_save_review signal handler, so we don't need that line anymore.

Ah right, gotcha.
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

https://reviewboard.mozilla.org/r/46105/#review43383
Attachment #8740986 - Flags: review?(mcote) → review+
Attachment #8726732 - Flags: review?(smacleod)
Comment on attachment 8726732 [details]
MozReview Request: mozreview: add set_attachment_flag to bugzilla client (bug 1197879); r?smacleod r?mcote

https://reviewboard.mozilla.org/r/38205/#review43945

::: pylib/mozreview/mozreview/bugzilla/client.py:381
(Diff revision 3)
>          return None
>  
>      @xmlrpc_to_bugzilla_errors
> -    def r_plus_attachment(self, bug_id, reviewer, rb_url, comment=None):
> -        """Set a review flag to "+".
> +    def set_attachment_flag(self, bug_id, flag, reviewer,
> +                            rb_url, comment=None):
> +        """Create, update or clear an attachment flag.

A lot of the wording around this makes it seem like it's a generic method to update an attachment flag, but the implementation is very specific to the review flag.

::: pylib/mozreview/mozreview/bugzilla/client.py:418
(Diff revision 3)
> -        for f in flags:
> -            if f['name'] == 'review' and f.get('requestee') == reviewer:
> +        # Filter out non-mozreview attachments
> +        review_flags = [f for f in flag_list if f['name'] == 'review']

This isn't filtering out non-mozreview attachments (it's not even filtering attachments tbh)

::: pylib/mozreview/mozreview/bugzilla/client.py:422
(Diff revision 3)
>  
> -        for f in flags:
> -            if f['name'] == 'review' and f.get('requestee') == reviewer:
> -                flag['id'] = f['id']
> -                break
> -            elif (f['name'] == 'review' and f.get('setter') == reviewer and
> +        # Filter out non-mozreview attachments
> +        review_flags = [f for f in flag_list if f['name'] == 'review']
> +
> +        for f in review_flags:
> +

No need for blank here

::: pylib/mozreview/mozreview/bugzilla/client.py:426
(Diff revision 3)
> -                break
> -            elif (f['name'] == 'review' and f.get('setter') == reviewer and
> -                  f['status'] == '+'):
> -                logger.info('r+ already set.')
> +        for f in review_flags:
> +
> +            # Bugzilla attachments have a requestee only if the status is `?`.
> +            # In the other cases requestee == setter.
> +            if ((reviewer == f.get('requestee') and f['status'] == '?')  or
> +                reviewer == f.get('setter')):

I believe you need to check that `f['status'] == '+'` in this case. It's possible there is a review flag, set by the reviwer, on someone else completely, which will end up messing up here.

::: pylib/mozreview/mozreview/bugzilla/client.py:428
(Diff revision 3)
> +                # Always clear the flag if the review status is r?
> +                if flag == '?':
> +                    flag_obj['status'] = 'X'

The current status isn't checked here though, and from above it could make it to this case if the status is `r+`. Is this comment misleading, or the code incorrect?

What we check is `flag`, but that's the desired end state, not the current state. So if they want an r?, we cancel the flag??

::: pylib/mozreview/mozreview/bugzilla/client.py:439
(Diff revision 3)
> +                    logger.info('r%s already set.' % flag)
> -                return False
> +                    return False
> +
> +                flag_obj['id'] = f['id']
> +                break
>          else:

Can you throw in a comment for what this means semantically (e.g. `# The reviewer did not have a flag on the attachment`)

::: pylib/mozreview/mozreview/bugzilla/client.py:453
(Diff revision 3)
> +                # don't create any flag.
> +                return False
> +
> +            # Flag not found, let's create a new one.
> +            flag_obj['new'] = True
> +            flag_obj['requestee'] = reviewer

In the case of `r+` here, `requestee` doesn't make sense? will BMO just ignore this in that case?
Comment on attachment 8726733 [details]
MozReview Request: mozreview: use set_attachment_flag on review publishing (bug 1197879) r?smacleod r?mcote

https://reviewboard.mozilla.org/r/38207/#review44157

::: pylib/mozreview/mozreview/extra_data.py:32
(Diff revision 3)
>  COMMIT_ID_KEY = MOZREVIEW_KEY + '.commit_id'
>  COMMITS_KEY = MOZREVIEW_KEY + '.commits'
>  DISCARD_ON_PUBLISH_KEY = MOZREVIEW_KEY + '.discard_on_publish_rids'
>  FIRST_PUBLIC_ANCESTOR_KEY = MOZREVIEW_KEY + '.first_public_ancestor'
>  IDENTIFIER_KEY = MOZREVIEW_KEY + '.identifier'
> +REVIEW_STATE_KEY = MOZREVIEW_KEY + '.review_state'

This isn't a CommitData field key, as the comment above these indicates, you should store this in its own block of constants.

Also, I think `REVIEW_FLAG_KEY` is more accurate, `STATE` will be especially nebulous if we add feedback flags etc down the road.

::: pylib/mozreview/mozreview/signal_handlers.py:567
(Diff revision 3)
> +    if (review.extra_data.get(REVIEW_STATE_KEY) is None
> +            and not is_parent(review.review_request)):
> +        review.extra_data[REVIEW_STATE_KEY] = 'r?'

```
    if (REVIEW_STATE_KEY not in review.extra_data and
            not is_parent(review.review_request)):
        review.extra_data[REVIEW_STATE_KEY] = 'r?'
```
Attachment #8726733 - Flags: review?(smacleod)
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

https://reviewboard.mozilla.org/r/37411/#review44165

::: pylib/mozreview/mozreview/extension.py:120
(Diff revision 6)
>  
>      default_settings = {}
>  
>      is_configurable = True
>  
> -    js_extensions = [ParentJSExtension]
> +    js_extensions = [ParentJSExtension, MRPlusAndMinusExtension]

Alphabetical please (while you're at it, please turn it into a multi-line 4 space indent list.

::: pylib/mozreview/mozreview/static/mozreview/css/review.less:126
(Diff revision 6)
>  .parent-request {
>    #id_shipit,
>    label[for="id_shipit"] {
>      display: none;
>    }
>  }

This rule is no longer needed now that we're hiding the checkbox on all review requests.

::: pylib/mozreview/mozreview/static/mozreview/js/parents.js:51
(Diff revision 6)
> +/**
> + * MRPlusAndMinus is responsible for showing the r?, r+, r- select dropdown
> + * in the review dialog. It will also do the work of setting the extraData
> + * in a Review model with the value that the user selects.
> + */
> +MRPlusAndMinus = {};

This really shouldn't be in `parents.js`, it actively does not apply to parent review requests, so this location is quite unintuitive heh.

::: pylib/mozreview/mozreview/static/mozreview/js/parents.js:56
(Diff revision 6)
> +   * If we save state, it'll be to a Review's extraData using this
> +   * key.
> +   */
> +  key: 'p2rb.review_state',

Can you add to the comment the corresponding python module path to the constant for this.

::: pylib/mozreview/mozreview/static/mozreview/js/parents.js:70
(Diff revision 6)
> +   */
> +  states: [' ', 'r?', 'r+', 'r-'],
> +
> +  template: _.template([
> +      '<label for="MRPlusAndMinus">Review state:</label>',
> +      '<select id="MRPlusAndMinus">',

something like `mr-review-flag` might make more sense, especially considering other flags being added in the future.

::: pylib/mozreview/mozreview/static/mozreview/js/parents.js:96
(Diff revision 6)
> +  /**
> +  *  Under some circumstances a model save will replace the comment textarea
> +  *  with a 'Add text' link. This is a hack to circumvent that problem.
> +  */
> +  save: function(){

Reminds me of this :)
![Useless Machine](https://i.imgur.com/FRflC.gif)
Attachment #8725301 - Flags: review?(smacleod)
Comment on attachment 8725302 [details]
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod

https://reviewboard.mozilla.org/r/37413/#review44171

::: pylib/mozreview/mozreview/review_helpers.py:9
(Diff revision 8)
>  from mozreview.models import get_profile
> +from mozreview.extra_data import REVIEW_STATE_KEY

Alphabetical

::: pylib/mozreview/mozreview/review_helpers.py:86
(Diff revision 8)
> +    # This is used in the templates to print a css class representing
> +    # the review status
> +    reviewer_status_class_map = {
> +        'r?': 'review-pending',
> +        'r+': 'review-granted',
> +        'r-': 'review-denied',
> +        ' ': 'review-cleared'
> +    }

I don't think we should send CSS classes out in the API responses. What I think you should do instead is write a template tag which will use this mapping to convert to CSS classes, and call that from the template where you need it.

::: pylib/mozreview/mozreview/review_helpers.py:100
(Diff revision 8)
>      for r in reviewers:
> -        reviewers_status[r.username] = {'ship_it': False}
> +        # The initial state is r?
> +        reviewers_status[r.username] = {
> +            'ship_it': False,
> +            'review_status': 'r?',
> +            'css_class': reviewer_status_class_map['r?']

nuke this, see other comment.

::: pylib/mozreview/mozreview/review_helpers.py:103
(Diff revision 8)
>      # Don't show any status on drafts.
>      if type(review_request) == ReviewRequestDraft:
> -        return reviewers_status
> +        return {}

huh... strange... we used to just output `False` for every reviewer ont he draft... definitely doesn't seem like "Don't show any status on drafts.". It weirds me out a bit that this change is inconsequential. I guess we just don't look at the draft stuff ever? Were there really no other test changes from this?

::: pylib/mozreview/mozreview/review_helpers.py:108
(Diff revision 8)
>      # Don't show any status on drafts.
>      if type(review_request) == ReviewRequestDraft:
> -        return reviewers_status
> +        return {}
>  
>      for review in gen_latest_reviews(review_request):
> -
> +        review_status = review.extra_data.get(REVIEW_STATE_KEY)

I'd call this "review_flag" or something. "review_status" is so close to "reviewers_status" at first glance it was throwing me off.

::: pylib/mozreview/mozreview/review_helpers.py:109
(Diff revision 8)
> +        user = review.user.username
> +        if user in reviewers_status:
> -        if review.user.username in reviewers_status:
> +            if review.user.username in reviewers_status:

Measure twice cut once? Have to really make sure the user is in the status ;)

::: pylib/mozreview/mozreview/static/mozreview/css/commits.less:88
(Diff revision 8)
> +        &.review-pending::after {
> +          content: " (r?)";
> +        }
> +        &.review-granted::after {
> +          content: " (r+)";
> +        }
> +        &.review-denied::after {
> +          content: " (r-)";
> +        }
> +        &.review-cleared{
> +          color: grey;
> +        }
> +        &.review-cleared::after {
> +          content: " (r? cleared)";
> +        }

I love how all this turned out looking, great ideas :)
Attachment #8725302 - Flags: review?(smacleod)
Comment on attachment 8739438 [details]
MozReview Request: mozreview: show review flag in review request history (bug 1197879); r?smacleod

https://reviewboard.mozilla.org/r/45271/#review44175

::: pylib/mozreview/mozreview/static/mozreview/js/review.js:32
(Diff revision 5)
> +
> +  RB.apiCall({
> +    type: 'GET',
> +    prefix: reviewRequest.get('sitePrefix'),
> +    noActivityIndicator: true,
> +    url: '/api/review-requests/'+reviewRequest.get('id')+'/reviews/',

This will break down if there has been more than 25 reviews (it will start to paginate).

::: pylib/mozreview/mozreview/static/mozreview/js/review.js:33
(Diff revision 5)
> +  RB.apiCall({
> +    type: 'GET',
> +    prefix: reviewRequest.get('sitePrefix'),
> +    noActivityIndicator: true,
> +    url: '/api/review-requests/'+reviewRequest.get('id')+'/reviews/',
> +    success: function(data){

space between "function()" and "{"

::: pylib/mozreview/mozreview/static/mozreview/js/review.js:34
(Diff revision 5)
> +    type: 'GET',
> +    prefix: reviewRequest.get('sitePrefix'),
> +    noActivityIndicator: true,
> +    url: '/api/review-requests/'+reviewRequest.get('id')+'/reviews/',
> +    success: function(data){
> +      _.forEach(data.reviews, function(item){

space between "function()" and "{"
Attachment #8739438 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/38207/#review44179

::: pylib/mozreview/mozreview/signal_handlers.py:569
(Diff revision 3)
> +    """
> +    review = kwargs["instance"]
> +
> +    if (review.extra_data.get(REVIEW_STATE_KEY) is None
> +            and not is_parent(review.review_request)):
> +        review.extra_data[REVIEW_STATE_KEY] = 'r?'

If the state isn't set, I think we should clear the flag rather than make it `r?`, so we preserve current behaviour.
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

https://reviewboard.mozilla.org/r/45273/#review44177

All of the test changes need to be examined to make sure they still indicate what they're testing, that they make sense, and any new conditions also need test cases rather than just updating the current ones.

::: hgext/reviewboard/tests/test-bugzilla-review-flag-cleared.t:75
(Diff revision 8)
>      product: TestProduct
>      resolution: ''
>      status: UNCONFIRMED
>      summary: First Bug
>  
>  Publishing a review will clear the r? flag

This functionality has changed, but the comments haven't been updated. Any of the changed tests need to be read through to make sure they aren't misleading / confusing.

::: hgext/reviewboard/tests/test-bugzilla-review-flag-cleared.t
(Diff revision 8)
> -      - Comment on attachment 1 [details] [diff] [review]
> -      - 'MozReview Request: Bug 1 - Initial commit to review'
> -      - ''

This change seems strange to me? what's going on here.

::: hgext/reviewboard/tests/test-bugzilla-review-flag-cleared.t:131
(Diff revision 8)
>      product: TestProduct
>      resolution: ''
>      status: UNCONFIRMED
>      summary: First Bug
>  
>  Posting a non Ship It review without a review flag adds a comment

Again, this is incorrect for what's actually going on here.

::: hgext/reviewboard/tests/test-bugzilla-review-interaction.t:925
(Diff revision 8)
>      product: TestProduct
>      resolution: ''
>      status: UNCONFIRMED
>      summary: Parent Reviews
>  
> -A non-ship-it review on a child should also clear the attachment's r?.
> +A r? review on a child should result in a r? flag.

"an"

::: hgext/reviewboard/tests/test-bugzilla-review-interaction.t:940
(Diff revision 8)
> +      - id: 8
> +        name: review
> +        requestee: reviewer2@example.com
> +        setter: author@example.com
> +        status: '?'

errr... how is the setter the author here? the setter should be the reviewer?? wat?
Attachment #8739439 - Flags: review?(smacleod)
Attachment #8740986 - Flags: review?(smacleod)
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

https://reviewboard.mozilla.org/r/46105/#review44183

Again, I really think these test changes need to be looked at more closely and either explained better in comments, or fixed up.

::: hgext/reviewboard/tests/test-review-request-approval.t
(Diff revision 6)
> -  approved: false
> -  approval_failure: A suitable reviewer has not given a "Ship It!"
> -  review_count: 3
> -  reviews:
> -  - id: 1
> -    public: true
> -    ship_it: true
> -    extra_data:

These test changes really don't make sense to me...

::: pylib/mozreview/mozreview/signal_handlers.py:574
(Diff revision 6)
> +        try:
> +            flag = reviewers_status[review.user.username]['review_status']
> +        except KeyError:
> +            flag = 'r?'

```
    flag = reviewers_stats.get(review.user.username, {}).get('review_status', 'r?')
```

::: pylib/mozreview/mozreview/signal_handlers.py:577
(Diff revision 6)
> +        reviewers_status = get_reviewers_status(review.review_request)
> +
> +        try:
> +            flag = reviewers_status[review.user.username]['review_status']
> +        except KeyError:
> +            flag = 'r?'

I'm not sure r? as the default makes sense, especially if someone does a drive by.

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:158
(Diff revision 6)
> +  {% for reviewer, status in review_request_details.get_review_request|reviewers_status %}
> +    {% if reviewer == user.username %}
> +      <div id="user-review-status" data-reviewer-status="{{status.review_status}}"></div>
> +    {% endif %}
> +  {% endfor %}

I think it makes more sense to handle this how scm_level is handled. It might be good to just do what glob has done in his change[1]


[1] https://reviewboard.mozilla.org/r/38103/diff/2#14
https://reviewboard.mozilla.org/r/38205/#review43945

> I believe you need to check that `f['status'] == '+'` in this case. It's possible there is a review flag, set by the reviwer, on someone else completely, which will end up messing up here.

This condition covers the following cases:
  
- {"requestee" : "reviewer", "setter" : "author", "status" : "?"}
- {"requestee" : "reviewer", "setter" : "reviewer", "status" : "?"}
- {"setter" : "reviewer", "status" : "+"}
- {"setter" : "reviewer", "status" : "-"}

Which case am I missing?

> The current status isn't checked here though, and from above it could make it to this case if the status is `r+`. Is this comment misleading, or the code incorrect?
> 
> What we check is `flag`, but that's the desired end state, not the current state. So if they want an r?, we cancel the flag??

I believe this keeps the current behaviour where a comment from a reviewer without a ship-it clears the r? flag on bugzilla.
In a later commit, once we have explicit r+/r-, this behaviour will change.
Maybe I can `s/review status is r?/desired status is not r+` in the comment?

> In the case of `r+` here, `requestee` doesn't make sense? will BMO just ignore this in that case?

I think it's an error indeed. Bugzilla will ignore it because it's a r+/r-. I'll remove it.
Comment on attachment 8726732 [details]
MozReview Request: mozreview: add set_attachment_flag to bugzilla client (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38205/diff/3-4/
Attachment #8726732 - Flags: review?(smacleod)
Attachment #8726733 - Flags: review?(smacleod)
Attachment #8725301 - Flags: review?(smacleod)
Attachment #8725302 - Flags: review?(smacleod)
Attachment #8739438 - Flags: review?(smacleod)
Attachment #8739439 - Flags: review?(smacleod)
Attachment #8740986 - Flags: review?(smacleod)
Comment on attachment 8726733 [details]
MozReview Request: mozreview: use set_attachment_flag on review publishing (bug 1197879) r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38207/diff/3-4/
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37411/diff/6-7/
Comment on attachment 8725302 [details]
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37413/diff/8-9/
Comment on attachment 8739438 [details]
MozReview Request: mozreview: show review flag in review request history (bug 1197879); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45271/diff/5-6/
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45273/diff/8-9/
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46105/diff/6-7/
https://reviewboard.mozilla.org/r/38205/#review43945

> This condition covers the following cases:
>   
> - {"requestee" : "reviewer", "setter" : "author", "status" : "?"}
> - {"requestee" : "reviewer", "setter" : "reviewer", "status" : "?"}
> - {"setter" : "reviewer", "status" : "+"}
> - {"setter" : "reviewer", "status" : "-"}
> 
> Which case am I missing?

{"requestee" : "someone-else", "setter": "reviewer", "status": "?"}

> I believe this keeps the current behaviour where a comment from a reviewer without a ship-it clears the r? flag on bugzilla.
> In a later commit, once we have explicit r+/r-, this behaviour will change.
> Maybe I can `s/review status is r?/desired status is not r+` in the comment?

Yes, but look at how it interacts with the case I indicate in the comment above this one.
https://reviewboard.mozilla.org/r/38205/#review43945

> This isn't filtering out non-mozreview attachments (it's not even filtering attachments tbh)

This was not fixed, re-opening.
https://reviewboard.mozilla.org/r/38205/#review43945

> No need for blank here

This was not fixed, re-opening.
https://reviewboard.mozilla.org/r/38205/#review43945

> {"requestee" : "someone-else", "setter": "reviewer", "status": "?"}

ah, right you are!
Comment on attachment 8726732 [details]
MozReview Request: mozreview: add set_attachment_flag to bugzilla client (bug 1197879); r?smacleod r?mcote

https://reviewboard.mozilla.org/r/38205/#review45581

::: pylib/mozreview/mozreview/bugzilla/client.py:381
(Diff revision 4)
>          return None
>  
>      @xmlrpc_to_bugzilla_errors
> -    def r_plus_attachment(self, bug_id, reviewer, rb_url, comment=None):
> -        """Set a review flag to "+".
> +    def set_attachment_flag(self, bug_id, flag, reviewer,
> +                            rb_url, comment=None):
> +        """Create, update or clear an attachment flag.

I would argue that the logging output is now more inline with this docstring, but the implementation is still extremely focused on review attachments (and wouldn't be trivial to modify in a way that isn't).

TBH, I was expecting you would update things to talk about review flags, rather than modify to the logging output. I think the logging is fine how it is and an update to call this `set_review_flag` as well as making this docstring "Create, update, or clear a review flag." should mostly be fine and pretty simple.

::: pylib/mozreview/mozreview/bugzilla/client.py:439
(Diff revision 4)
> +        # The reviewer did not have a flag on the attachment.
>          else:

Please stick this comment below the else, rather than above.
Attachment #8726732 - Flags: review?(smacleod)
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

Clearing until update
Attachment #8725301 - Flags: review?(smacleod)
Comment on attachment 8725302 [details]
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod

Clearing until update
Attachment #8725302 - Flags: review?(smacleod)
Comment on attachment 8726733 [details]
MozReview Request: mozreview: use set_attachment_flag on review publishing (bug 1197879) r?smacleod r?mcote

Clearing until update
Attachment #8726733 - Flags: review?(smacleod)
Comment on attachment 8739438 [details]
MozReview Request: mozreview: show review flag in review request history (bug 1197879); r?smacleod

Clearing until update
Attachment #8739438 - Flags: review?(smacleod)
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

Clearing until update
Attachment #8739439 - Flags: review?(smacleod)
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

Clearing until update.
Attachment #8740986 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/46105/#review44183

> I'm not sure r? as the default makes sense, especially if someone does a drive by.

What do you think would be a sane default here?

> I think it makes more sense to handle this how scm_level is handled. It might be good to just do what glob has done in his change[1]
> 
> 
> [1] https://reviewboard.mozilla.org/r/38103/diff/2#14

I'll handle it the same way as scm_level then. I'll then let glob decide whether to incorporate this piece into his work or not.
https://reviewboard.mozilla.org/r/46105/#review44183

> What do you think would be a sane default here?

I've been thinking about this.  I think the best would be defaulting to none (no flag/cancel review) if the person isn't currently flagged for review and hasn't previously left a review.  That's how Splinter works, anwyay.
Comment on attachment 8726732 [details]
MozReview Request: mozreview: add set_attachment_flag to bugzilla client (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38205/diff/4-5/
Attachment #8726732 - Flags: review?(smacleod)
Attachment #8726733 - Flags: review?(smacleod)
Attachment #8725301 - Flags: review?(smacleod)
Attachment #8725302 - Flags: review?(smacleod)
Attachment #8739438 - Flags: review?(smacleod)
Attachment #8739439 - Flags: review?(smacleod)
Attachment #8740986 - Flags: review?(smacleod)
Comment on attachment 8726733 [details]
MozReview Request: mozreview: use set_attachment_flag on review publishing (bug 1197879) r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38207/diff/4-5/
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37411/diff/7-8/
Comment on attachment 8725302 [details]
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37413/diff/9-10/
Comment on attachment 8739438 [details]
MozReview Request: mozreview: show review flag in review request history (bug 1197879); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45271/diff/6-7/
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45273/diff/9-10/
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46105/diff/7-8/
https://reviewboard.mozilla.org/r/38205/#review43945

> Yes, but look at how it interacts with the case I indicate in the comment above this one.

Now that that case is excluded by the updated condition, this should be not an issue anymore, right?
https://reviewboard.mozilla.org/r/45271/#review44175

> This will break down if there has been more than 25 reviews (it will start to paginate).

I added the max-results parameter to set the page size to 200.
https://reviewboard.mozilla.org/r/45273/#review44177

> This change seems strange to me? what's going on here.

This is due to the fact that we don't update the review flag if the flag has not changed. Instead we only add a comment to the bug. The comment on bugzilla contains a link to the comment on reviewboard anyway, so I think this side effect is not a bad one.
https://reviewboard.mozilla.org/r/46105/#review44183

> These test changes really don't make sense to me...

I checked and fixed all the tests, it should be fine now.
You have a number of open issues still.  I glanced at a couple and it seems you fixed them.  Can you close them if they're all resolved?
Flags: needinfo?(mdoglio)
https://reviewboard.mozilla.org/r/46105/#review44183

> I've been thinking about this.  I think the best would be defaulting to none (no flag/cancel review) if the person isn't currently flagged for review and hasn't previously left a review.  That's how Splinter works, anwyay.

Ok, I'll try to implement that.
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37411/diff/8-9/
Comment on attachment 8725302 [details]
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37413/diff/10-11/
Comment on attachment 8739438 [details]
MozReview Request: mozreview: show review flag in review request history (bug 1197879); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45271/diff/7-8/
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45273/diff/10-11/
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46105/diff/8-9/
mdoglio said he has resolved all the issues, so clearing his NI.
Flags: needinfo?(mdoglio)
Comment on attachment 8726732 [details]
MozReview Request: mozreview: add set_attachment_flag to bugzilla client (bug 1197879); r?smacleod r?mcote

https://reviewboard.mozilla.org/r/38205/#review47375

::: pylib/mozreview/mozreview/bugzilla/client.py:440
(Diff revision 5)
> +                # This should never happen, but if it does log the error.
> +                logger.error('No flag to clear for %s on %s' % (
> +                    reviewer, rb_attachment
> +                ))

This could totally happen if the action is causing a flag clear but someone had already cleared it manually in bugzilla. I think saying "never" here is a little too strong, it might also be better to do a `logger.info` rather than `logger.error`
Attachment #8726732 - Flags: review?(smacleod) → review+
Attachment #8726733 - Flags: review?(smacleod) → review+
Comment on attachment 8726733 [details]
MozReview Request: mozreview: use set_attachment_flag on review publishing (bug 1197879) r?smacleod r?mcote

https://reviewboard.mozilla.org/r/38207/#review47379

::: pylib/mozreview/mozreview/signal_handlers.py:573
(Diff revisions 3 - 5)
> -            and not is_parent(review.review_request)):
> -        review.extra_data[REVIEW_STATE_KEY] = 'r?'
> +            not is_parent(review.review_request)):
> +        if review.pk:
> +            # The review create endpoint calls save twice: the first time it
> +            # gets or creates the review and the second time it updates the
> +            # object retrieved/created. This condition let us execute the code
> +            # below only qwhen all the fields are set.

"qwhen" -> "when"

::: pylib/mozreview/mozreview/signal_handlers.py:135
(Diff revision 5)
> +    SignalHook(
> +        extension,
> +        pre_save,
> +        pre_save_review,
> +        sender=Review,
> +        sandbox_errors=False)

we should probably let sandbox_errors be true for this case -> if our signal handler has some bug in it which causes it to fail it will 500 the page with this. I'm not sure that's the best idea.

If we really want to block a review from being published without a flag set we should check that in the publish handler.
Attachment #8725301 - Flags: review?(smacleod) → review+
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

https://reviewboard.mozilla.org/r/37411/#review47381

::: pylib/mozreview/mozreview/static/mozreview/js/review_flag.js:89
(Diff revision 9)
> +    // TODO: move the MRPlusMinusExtension to the same bundle as init_rr.js
> +    // so that we can reuse Mozreview.isParent (Bug 1263591).

It *is* in the same bundle now.

::: pylib/mozreview/mozreview/templates/mozreview/review-scripts-js.html
(Diff revision 9)
> -{% if review_request|isPush %}
> -{%   ext_js_bundle extension "reviews" %}
> -

Excellent, you switched us to using the standard bundle inclusion :D
Comment on attachment 8725302 [details]
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod

https://reviewboard.mozilla.org/r/37413/#review47387

Looking good :D

::: pylib/mozreview/mozreview/review_helpers.py:86
(Diff revision 11)
>      """Returns the latest review status for each reviewer."""
>      if not reviewers:
>          reviewers = review_request.target_people.all()
>      reviewers_status = dict()
>  
> +

nuke this blank line.

::: pylib/mozreview/mozreview/review_helpers.py:94
(Diff revision 11)
>      # Don't show any status on drafts.
>      if type(review_request) == ReviewRequestDraft:
> -        return reviewers_status
> +        return {}

This should be at the top of the function to avoid any database calls or iterations of reviewers.
Attachment #8725302 - Flags: review?(smacleod) → review+
Attachment #8739438 - Flags: review?(smacleod) → review+
Comment on attachment 8739438 [details]
MozReview Request: mozreview: show review flag in review request history (bug 1197879); r?smacleod

https://reviewboard.mozilla.org/r/45271/#review47389

::: pylib/mozreview/mozreview/static/mozreview/js/review.js:32
(Diff revision 8)
> +  RB.apiCall({
> +    type: 'GET',
> +    prefix: reviewRequest.get('sitePrefix'),
> +    noActivityIndicator: true,
> +    url: '/api/review-requests/'+reviewRequest.get('id')+'/reviews/'
> +       + '?max-results=200',

I'm not the biggest fan of handling it this way, but this should be a temporary hack so it will hopefully be fine for now.
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

https://reviewboard.mozilla.org/r/45273/#review47393

::: hgext/reviewboard/tests/test-bugzilla-review-flag-cleared.t:75
(Diff revision 11)
>      product: TestProduct
>      resolution: ''
>      status: UNCONFIRMED
>      summary: First Bug
>  
> -Publishing a review will clear the r? flag
> +Publishing a non-'clear-the-flag' review will not clear the r? flag

(Not really a comment on this particular line)

It appears we don't have any coverage of an `r-` review? or am I missing something. Can you add testing for `r-` transitions somewhere.

::: hgext/reviewboard/tests/test-bugzilla-review-flag-cleared.t:131
(Diff revision 11)
>      product: TestProduct
>      resolution: ''
>      status: UNCONFIRMED
>      summary: First Bug
>  
> -Posting a non Ship It review without a review flag adds a comment
> +Posting a r? review with a comment only adds a comment

"an r?"
Attachment #8739439 - Flags: review?(smacleod)
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

https://reviewboard.mozilla.org/r/46105/#review47397

This is looking really good, just two small nits and the test stuff :)

::: hgext/reviewboard/tests/test-review-request-approval.t:208
(Diff revision 9)
>      body_top: Ship-it!
>      body_top_text_type: plain
>      diff_comments: []
>  
>  Posting a new review without r+ should cancel the previous approval
> -  $ rbmanage create-review 2 --body-top "Don't Land it!" --public
> +  $ rbmanage create-review 2 --body-top "Don't Land it!" --public --review-flag='r?'

This definitely feels like a case where we should `r-`

::: hgext/reviewboard/tests/test-review-request-approval.t:557
(Diff revision 9)
>      diff_comments: []
>  
>  Opening issues, even from an L1 user, should revoke approval until they're fixed
>  
>    $ exportbzauth l1b@example.com password
> -  $ rbmanage create-review 2 --body-top "I found issues"
> +  $ rbmanage create-review 2 --body-top "I found issues" --review-flag="r?"

should probably be `r-` or `r+` (i.e. fix it, then ship it)

::: hgext/reviewboard/tests/test-reviewer-flags.t:95
(Diff revision 9)
>  
>  There are warnings for reviewers who haved granted a non ship-it review when
>  using r=.
>  
>    $ exportbzauth cthulhu@example.com password
> -  $ rbmanage create-review 2 --body-top "No way you should ship-it!" --public
> +  $ rbmanage create-review 2 --body-top "No way you should ship-it!" --public --review-flag='r?'

sounds like `r-` territory

::: pylib/mozreview/mozreview/signal_handlers.py:579
(Diff revision 9)
> -            # below only when all the fields are set.
> -            review.extra_data[REVIEW_FLAG_KEY] = 'r?'
> +        # below only once.
> +
> +        if not is_parent(review.review_request):
> +
> +            if REVIEW_FLAG_KEY not in review.extra_data:
> +                reviewers_status = get_reviewers_status(review.review_request)

You should specify `reviewers` in this call so we don't fetch all the target people.

Arguably we should use a different query than going through *all* the reviews, which is what `get_reviewers_status` does. This can be handled as a followup though, could you just add a TODO comment here mentioning this and we'll take care of it if it matters.

::: pylib/mozreview/mozreview/templatetags/mozreview.py:116
(Diff revision 9)
> +def reviewers_status_with_drive_by(review_request):
> +    return get_reviewers_status(review_request, include_drive_by=True).items()

We should really pass in the reviewer to `get_reviewers_status` here as well so we don't have to loop over the entire status.
Attachment #8740986 - Flags: review?(smacleod)
Comment on attachment 8726732 [details]
MozReview Request: mozreview: add set_attachment_flag to bugzilla client (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38205/diff/5-6/
Attachment #8739439 - Flags: review?(smacleod)
Attachment #8740986 - Flags: review?(smacleod)
Comment on attachment 8726733 [details]
MozReview Request: mozreview: use set_attachment_flag on review publishing (bug 1197879) r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38207/diff/5-6/
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37411/diff/9-10/
Comment on attachment 8725302 [details]
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37413/diff/11-12/
Comment on attachment 8739438 [details]
MozReview Request: mozreview: show review flag in review request history (bug 1197879); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45271/diff/8-9/
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45273/diff/11-12/
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46105/diff/9-10/
I still have to go over the issues on the last commit, I'll do it first thing tomorrow.
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46105/diff/10-11/
https://reviewboard.mozilla.org/r/46105/#review47397

> You should specify `reviewers` in this call so we don't fetch all the target people.
> 
> Arguably we should use a different query than going through *all* the reviews, which is what `get_reviewers_status` does. This can be handled as a followup though, could you just add a TODO comment here mentioning this and we'll take care of it if it matters.

We still need to fetch the reviewers to check whether they are designated or drive-by. I slightly changed get_reviewers_status to make it clear.
All the issues were fixed.
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

https://reviewboard.mozilla.org/r/45273/#review49756
Attachment #8739439 - Flags: review?(smacleod) → review+
Attachment #8740986 - Flags: review?(smacleod) → review+
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

https://reviewboard.mozilla.org/r/46105/#review49760
Comment on attachment 8726732 [details]
MozReview Request: mozreview: add set_attachment_flag to bugzilla client (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38205/diff/6-7/
Comment on attachment 8726733 [details]
MozReview Request: mozreview: use set_attachment_flag on review publishing (bug 1197879) r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38207/diff/6-7/
Comment on attachment 8725301 [details]
MozReview Request: mozreview: add r?, r+, r- UI to the Review Dialog. (bug 1197879) r?smacleod r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37411/diff/10-11/
Comment on attachment 8725302 [details]
MozReview Request: mozreview: show review flag in the commits table (bug 1197879) r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37413/diff/12-13/
Comment on attachment 8739438 [details]
MozReview Request: mozreview: show review flag in review request history (bug 1197879); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45271/diff/9-10/
Comment on attachment 8739439 [details]
MozReview Request: mozreview: stop clearing bugzilla flags on comments (bug 1197879); r?smacleod r?mcote

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45273/diff/12-13/
Comment on attachment 8740986 [details]
MozReview Request: mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46105/diff/11-12/
This looks like it's ready to go. Are we looking to land this soon?
Yeah I'm planning to land it today.
Blocks: 1270951
No longer blocks: 1270951
https://reviewboard.mozilla.org/r/53386/#review50178

Unless there are any egregious errors here, please autoland this after deploying the flag stuff.  I can fix up any little problems later tomorrow.
found an issue: existing r- attachment flags in bugzilla are reset to r? when a reviewer is added via review board.

str:
1. author publishes a review requesting review from userA
   attachment flags: userA r?
2. userA leaves r-
   attachment flags: userA r-
3. author uses review board web ui to add userB as a reviewer

expected:
   review board ui : userA r-, userB r?
   attachment state: userA r-, userB r?

actual:
   review board ui : userA r-, userB r?
   attachment flags: userA r?, userB r?
one fix for the issue in comment 179 is to always leave r- flags untouched.
i've got a patch that does that which fixes this issue; working on adding a test case for this.
Depends on: 1273790
spun the issue out into bug 1273790 to make reviewing/tracking easier.
Depends on: 1273954
Depends on: 1273956
Depends on: 1274151
No longer depends on: 1273954
Attachment #8753601 - Flags: review?(smacleod) → review?(glob)
Comment on attachment 8753601 [details]
MozReview Request: docs: Update user docs to reflect new flag system (bug 1197879); r?glob

https://reviewboard.mozilla.org/r/53386/#review52094

::: docs/mozreview/bugzilla.rst:39
(Diff revision 1)
>  the Bugzilla user interface or following the link to the review will
>  redirect you to Review Board.
>  
> -If someone gives a *Ship It* on a review request, a ``review+`` will be
> -set in Bugzilla.
> +When someone leaves a review on a review request, the chosen flag
> +value will be set in Bugzilla (or cleared if the empty value is
> +chosen).

this looks great, however i don't see any documentation covering what happens when a new revision of a patch is pushed.

i expect the docs to mention:
- the flags on an existing bugzilla attachment will be updated (ie. a new attachment is not created for each review request)
- any flags set to r- or cleared during a prior review will be set to r?
- prior r+ flags will be automatically carried forward
Attachment #8753601 - Flags: review?(glob)
Comment on attachment 8753601 [details]
MozReview Request: docs: Update user docs to reflect new flag system (bug 1197879); r?glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53386/diff/1-2/
Attachment #8753601 - Attachment description: MozReview Request: docs: Update user docs to reflect new flag system (bug 1197879); r?smacleod → MozReview Request: docs: Update user docs to reflect new flag system (bug 1197879); r?glob
Attachment #8753601 - Flags: review?(glob)
Comment on attachment 8753601 [details]
MozReview Request: docs: Update user docs to reflect new flag system (bug 1197879); r?glob

https://reviewboard.mozilla.org/r/53386/#review52384

lgtm
Attachment #8753601 - Flags: review?(glob) → review+
This should be landed just before we deploy (Monday morning?).
Depends on: 1276847
Final fix-ups landed, and everything was deployed today.

Doc changes also landed (http://hg.mozilla.org/hgcustom/version-control-tools/rev/7b36487b5588) and docs at readthedocs.io have been rebuilt.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago → 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: