1055021 - One attachment per commit

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[(no longer active)]

See <https://bugzilla.mozilla.org/show_bug.cgi?id=1054112#c22> for example.  I had to set the flag manually later on.

Comment 1

[Steven MacLeod [:smacleod]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #0)
> See <https://bugzilla.mozilla.org/show_bug.cgi?id=1054112#c22> for example. 
> I had to set the flag manually later on.

If you mark Ship It! on the squashed review request it will set the r+ on the attachment.

I still don't think I'm fully happy with this model, but we should probably ship the first release with it just to get things out the door. Do you have any better ideas for how the Ship-It! to r+ translation should work?

Comment 2

[(no longer active)]

(In reply to Steven MacLeod [:smacleod] from comment #1)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #0)
> > See <https://bugzilla.mozilla.org/show_bug.cgi?id=1054112#c22> for example. 
> > I had to set the flag manually later on.
> 
> If you mark Ship It! on the squashed review request it will set the r+ on
> the attachment.

Oh, I see.  That is entirely not obvious.  ;-)

> I still don't think I'm fully happy with this model, but we should probably
> ship the first release with it just to get things out the door.

That sounds fine.

> Do you have
> any better ideas for how the Ship-It! to r+ translation should work?

How about setting the flag if the requestee has given some kind of review on all of the children of a squashed review request?

Comment 3

[Gregory Szorc [:gps]]

This sounds like a non-obvious UX failure. It should be easy enough to change the behavior, especially now that we have comprehensive test coverage.

Comment 4

[Gregory Szorc [:gps]]

There is a workflow decision to be made here. I think this is an enhancement, but not a P1.

Comment 5

[Mark Côté [:mcote]]

We're going to fix this by using one review flag per commit (i.e. per child) and none for the parent.

Comment 6

[Mark Côté [:mcote]]

Attachment: MozReview Request: bz://1055021/mcote

/r/5723 - Bug 1055021 - Get all review-request summaries for a given bug. r=smacleod

Pull down this commit:

hg pull review -r 49138f0ae28de2c45f943db8243c05832e69fbb4

Comment 7

[Mark Côté [:mcote]]

Comment on attachment 8580138 [details]
MozReview Request: bz://1055021/mcote

/r/5723 - Bug 1055021 - Get all review-request summaries for a given bug. r=smacleod
/r/5983 - Bug 1055021 - Create attachments for all children. r=smacleod

Pull down these commits:

hg pull review -r 5505577e1e745e46ec6cc55b711804ac27772d88

Comment 8

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5983/#review4937

This looks mostly great!

::: pylib/rbbz/rbbz/bugzilla.py
(Diff revision 1)
> -            if f['name'] == 'review' and f.get('requestee') == reviewer:
> +            if f['name'] == 'review' and (f.get('requestee') == reviewer or
> +                                          f.get('setter') == reviewer and
> +                                          f.get('status') == '+'):

I actually have no clue how order of operations deals with `expr1 or expr2 and expr3`. Could you please put parenthesis around this to make it more readable?

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> +        for bug in bugs.difference(bugs_commented):

One of the nice things about Python is operators can be overloaded and the operators for sets "just work."

`for bug in bugs - bugs_commented`

You may want `for bug in sorted(bugs - bugs_commented)` so behavior is deterministic and tests don't get confused.

::: hgext/reviewboard/tests/test-bugzilla-review-interaction.t
(Diff revision 1)
> +  $ hg up -r 0 > /dev/null

$ hg -q up -r 0

::: hgext/reviewboard/tests/test-bugzilla-review-interaction.t
(Diff revision 1)
> +  $ rbmanage publish $HGPORT1 9
> +
> +  $ exportbzauth [e-mail] password
> +
> +Verify that a single ship-it r+s only that attachment.
> +
> +  $ rbmanage create-review $HGPORT1 11 --body-top 'land it!' --public --ship-it

Somewhere in here we should test that the state of bug 5 has all the flags it should. Otherwise, I don't believe we have explicit test coverage of state immediately after publish.

::: hgext/reviewboard/tests/test-bugzilla-review-interaction.t
(Diff revision 1)
> +A non-ship-it review on a child should clear only that attachment's r+.

This behavior doesn't seem right. Consider the following scenario:

1. I leave a ship it with an open issue
2. Author asks a question about the open issue
3. I reply to that question (by posting a new review)

I *think* this will result in r+ getting cleared. I could be wrong here (I can't remember how all the various primitives work in RB API land).

Comment 9

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review4945

> Somewhere in here we should test that the state of bug 5 has all the flags it should. Otherwise, I don't believe we have explicit test coverage of state immediately after publish.

I thought that was basically covered by other tests, but sure, it doesn't hurt.

> $ hg -q up -r 0

Heh I was just copying all the other uses of "hg up" in that test file...

> I actually have no clue how order of operations deals with `expr1 or expr2 and expr3`. Could you please put parenthesis around this to make it more readable?

"and" is always evaluated first in, I believe, every language I've used, but sure.

> This behavior doesn't seem right. Consider the following scenario:
> 
> 1. I leave a ship it with an open issue
> 2. Author asks a question about the open issue
> 3. I reply to that question (by posting a new review)
> 
> I *think* this will result in r+ getting cleared. I could be wrong here (I can't remember how all the various primitives work in RB API land).

smacleod and I (and others, I think) talked about this last week.  If we don't do this, there's no way to ever cancel an r+ without editing the attachment.  If I r+ with issues, and someone fixes it in a way that I don't like, I should be able to cancel that r+.  In your example, you should "ship it" again in your response.

However, in this case I believe you're dealing with a review reply, which we could (and maybe already) handle differently... but I'll have to check.  The best-case scenario might be that an r+ is cancelled only if there has been a new commit, which I think would case it to be a review and not a review reply.  I think.

> One of the nice things about Python is operators can be overloaded and the operators for sets "just work."
> 
> `for bug in bugs - bugs_commented`
> 
> You may want `for bug in sorted(bugs - bugs_commented)` so behavior is deterministic and tests don't get confused.

Good points.  And to think just the other day I was talking to someone about that exact thing, Python overriding operators where the situations are clear. :)

Comment 10

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review4947

> smacleod and I (and others, I think) talked about this last week.  If we don't do this, there's no way to ever cancel an r+ without editing the attachment.  If I r+ with issues, and someone fixes it in a way that I don't like, I should be able to cancel that r+.  In your example, you should "ship it" again in your response.
> 
> However, in this case I believe you're dealing with a review reply, which we could (and maybe already) handle differently... but I'll have to check.  The best-case scenario might be that an r+ is cancelled only if there has been a new commit, which I think would case it to be a review and not a review reply.  I think.

"cause it to be a review and not a review reply." that is.

Comment 11

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review4971

> "cause it to be a review and not a review reply." that is.

I confirmed that, as long as it is a reply to a review, it will not cancel the review.  This is corroborated by the fact that there is a separate signal for replies (reply_publishing vs. review_publishing).

I'd like to leave it as is, then, and not implement my suggested scenario about, since the current functionality allows a reviewer to change their mind and issue a follow-up, non-ship-it review on a commit to cancel their previous ship-it review.  I admit this is not totally obvious, though, but I think it's the best option we have now, so we should just be sure to document it and possibly provide hints in the UI.

Comment 12

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review4975

> I confirmed that, as long as it is a reply to a review, it will not cancel the review.  This is corroborated by the fact that there is a separate signal for replies (reply_publishing vs. review_publishing).
> 
> I'd like to leave it as is, then, and not implement my suggested scenario about, since the current functionality allows a reviewer to change their mind and issue a follow-up, non-ship-it review on a commit to cancel their previous ship-it review.  I admit this is not totally obvious, though, but I think it's the best option we have now, so we should just be sure to document it and possibly provide hints in the UI.

Gah all this overlapping terminology.  To be clear, a reply to a review, as opposed to a separate review, will not have any effect on BMO review flags at all; it will merely add a comment to the bug.

Comment 13

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5983/#review4977

> Gah all this overlapping terminology.  To be clear, a reply to a review, as opposed to a separate review, will not have any effect on BMO review flags at all; it will merely add a comment to the bug.

Cool. The current implementation is fine by me.

We should follow up with better UX around clearing the r+ flag. I reckon we can add a message to the review submission form/window-y thing that reflects the state of the Bugzilla review flag. e.g. "warning: submitting this review without a ship-it will **clear** your previously given r+ flag." But this work can be deferred until later.

Comment 14

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5721/#review4979

Do you think this series should include a backout of 5cbcd94a3995 / bug 1134285? IMO that functionality was paving over sub-par UX around r+ flag interaction and is no longer necessary once we have separate flags per commit.

Comment 15

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5983/#review4983

Something I just thought of that I'm too lazy to check at 22:00.

What's the behavior of this patch on existing, in-flight review requests? If we have a review flag on only a parent review and then roll out this server update, what happens? My recall of the patch is that it will "just work." But I'd feel better if we had explicit test coverage to confirm it.

Comment 16

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review5011

Hm, you raise a good point.  I don't think it will work properly.  First, a parent r? will be left around, and second, when someone hits ship it, it will fail to find the child attachments, unless the committer has since republished.

I hate adding lots of backwards-compatibility code that will be unnecessary soon after, but I'm not sure what else to do.  A migration script is another option but that will mess up attachment ownership.  Hm.

Comment 17

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5983/#review5031

I thought if you reviewed the parent that it would create r+ flags on all children? So the only concern is orphaned parent review flags, no?

How about this:

1) Review Board will remove any r? from the parent commit when a child is reviewed
2) Write and run a script that crawls Review Board for parent review requests with r? flag and replaces them with child review requests. Worst case we can generate a list of attachment IDs and hand this off to a BMO admin to do manual database muckery if we need attachment ownership foo that can't be done via the web services.
3) Remove the code doing item 1

Comment 18

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review5061

My main concern is that child attachments won't be created, as you pointed out in 2).  I am virtually certain there's no APIs to change attachment ownership--but maybe we could use a dummy user or something.  I'll chat with the BMO team about an approach.

Comment 20

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review5193

Okay glob says the best way is to write a migration script; if executed locally I can create the attachments with the correct owners.  It might as well do 1) and 2) together, so no code changes will be required in MozReview.

Comment 21

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5721/#review5515

Yes, I think that would be for the best.

Comment 22

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5723/#review5517

Overall looks good to me, just a few minor issues.

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> +from django.core.exceptions import ObjectDoesNotExist

It's usually convention to just catch the specific model exception you care about. So in this case I believe you'll want `ReviewRequest.DoesNotExist` rather than importing this base exception.

::: pylib/mozreview/mozreview/resources/errors.py
(Diff revision 2)
> -CHILD_DOES_NOT_EXIST = WebAPIError(
> -    1002,
> -    "Child review request does not exist",
> -    http_status=500)  # 500 Internal Server Error

Since this has already landed, I think we might as well just keep it so that 1002 is forever `CHILD_DOES_NOT_EXIST`. That error will probably useful somewhere at some point, and I think once we select an ID number for an exception it'd be best to just keep it forever.

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> +COMMITS_KEY = 'p2rb.commits'
> +COMMIT_ID_KEY = 'p2rb.commit_id'
> +MOZREVIEW_KEY = 'p2rb'

I'm thinking these constants might be better in a more central place (not extensions.py - circular import problems).

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> +                summarize_review_request(x, x.extra_data[COMMIT_ID_KEY])
> +                for x in family['children'].values()

I'd prefer more verbose `child` or something rather than `x`.

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> +def summarize_families(families):

This needs a docstring, including a description of what `families` is/can be. Might also be nice to show what the return value looks like in a typical case.

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> -                            INVALID_ATTRIBUTE, NOT_LOGGED_IN, NOT_PARENT,
> -                            PERMISSION_DENIED)
> +        """Return public MozReview review requests.
> +        Takes an optional 'bug' parameter to narrow down the list to only

Blank line between these:

> Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description [1]

[1] https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> +        those matching that bug ID.
> +        Note that this presumes that bugs_closed only ever contains one

Blank lines between paragraphs.

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> +                'description': 'Return summaries for all review requests '
> +                               'associated with this bug',

The whole resource returns the summaries, this particular parameter filters the results.

How about "The review request must have the provided bug number."?

That being said, I wonder if we should be allowing a comma-separated list of bug numbers here, rather than just a single number. What do you think?

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> +        q = Q(extra_data__contains=MOZREVIEW_KEY)
> +        if is_list:
> +            if 'bug' in request.GET:
> +                q = q & Q(bugs_closed=request.GET.get('bug'))

This is probably going to get quite slow as the DB grows... We'll probably want to start building our own tables with indexed bug numbers, specifications of which rrids are pushed requests etc.

How about we add a TODO here noting we should likely optimize this in the future.

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> +            queryset = self._get_queryset(request, is_list=True, *args,
> +                                          **kwargs)

I'd rather we just call our own `get_queryset` rather than go through the indirection of using the "private" `_get_queryset`.

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> +        Note that this presumes that bugs_closed only ever contains one
> +        bug, which in MozReview, at the moment, is always true.

That will probably change this quarter. I can see the parents being associated with many bugs once we allow just pushing up multiples at a time.

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> +                families[rr.get_blocks()[0].id]['children'][rr.id] = rr

I don't like using the blocks field here. I can see that getting repurposed in the future, it'd be better to use something in extra data.

I'm fine leaving this for now though if it's not trivial to change.

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> +        return 200, {
> +            'review_request_summaries': summaries,
> +            'total_results': len(summaries),
> +        }

Is the "links" dictionary automatically added by RB with this return value? You may have to generate the links and add them manually, please check this.

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> +            'review_request_summaries': summaries,

the `review_request_summaries` key should be coming from some attribute the resource class has - this value is changeable / used in other places.

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> +                            parent_review_request.depends_on.all())

Again, not a big fan of using the depends on relationship stuff - this new code is the only place we rely on it instead of the extra data info.

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> +        return 200, summarize_families(families)[0]

Check here that we get a proper links dictionary as well.

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 2)
> +    def _sort_families(self, request, families, rrs):

I think a docstring would be nice for this function.

Also, I think it would make some of the other code cleaner if the families argument was optional and if not provided a defaultdict would be created and returned. Then your first callsite can just call in to get the dictionary.

Comment 23

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5983/#review5673

::: pylib/rbbz/rbbz/bugzilla.py
(Diff revision 2)
> +        if comment:
> +            params['comment'] = comment

If comment is optional it should be made an optional argument to the function.

::: pylib/rbbz/rbbz/bugzilla.py
(Diff revision 2)
> -        """Cancel a r? flag on a Bugzilla attachment while maybe adding a comment.
> +        """Cancel an r? or r+ flag on a Bugzilla attachment while maybe adding a
> +        comment.

I think the "while maybe adding an attachment" can just be part of the larger description.

::: pylib/rbbz/rbbz/extension.py
(Diff revision 2)
>          if using_bugzilla:
>              b.post_rb_url(bug_id,
>                            review_request.id,
>                            review_request_draft.summary,
>                            comment,
>                            review_or_request_url(review_request),
>                            reviewers)

Hmmm, this is going to spam the crap out of the bug is it not? We'll get an attachment + separate comment for every commit. That could easily be > 20 comments for one push.

Is there any way we can do this as a single operation? or at least maybe just post one comment?

::: pylib/rbbz/rbbz/extension.py
(Diff revision 2)
> +    # FIXME: Update all attachments in one call.  This is not possible right
> +    # now because we have to potentially mix changing and creating flags.
> +
> +    if is_review_request_squashed(review_request):
> +        # Update all children, but only post the comment once per bug.
> +        # Assumes only one bug per child, but potentially multiple bugs in
> +        # one family.  Prefer to comment when modifying the review flag.

I'm torn on whether we should even allow this mass flag changing with operations on the parent request.

Personally I'd like to encourage people just to review the individual commits and use the parent to track work / view the high level picture with the whole diff. Also, it'd be easier to add this functionality in later rather than have people start relying on it and then remove it.

::: pylib/rbbz/rbbz/extension.py
(Diff revision 2)
> +        if not commented and comment:

I think writing this as `if comment and not commented:` is clearer (due to not always having memorized the precedence of operators).

::: pylib/rbbz/rbbz/extension.py
(Diff revision 2)
>  def on_review_request_closed_discarded(user, review_request, type, **kwargs):
> -    if (not is_review_request_squashed(review_request) or
> +    if type != ReviewRequest.DISCARDED:

We should really rename `type` here to not be a reserved word while you're changing things.

::: pylib/rbbz/rbbz/bugzilla.py
(Diff revision 2)
>              return

`None` is not a "boolean" (as described in the docstring). I feel like we should log this case as well.

Comment 24

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5983/#review5683

> I'm torn on whether we should even allow this mass flag changing with operations on the parent request.
> 
> Personally I'd like to encourage people just to review the individual commits and use the parent to track work / view the high level picture with the whole diff. Also, it'd be easier to add this functionality in later rather than have people start relying on it and then remove it.

Keep in mind that people doing append-at-the-end tend to only look at the parent.

That being said, I'm not a huge fan of append-at-the-end. It's not how we land patches (for Firefox at least) and I think reviewers should be looking at the final product that lands because it helps remove uncertainty.

This will likely become an issue when we import GitHub PRs. I feel Git[Hub] users aren't so accustomed to rewriting commits. I suppose we can tackle the problem later with added complexity, if needed. My vote is with Steven on this behavior.

Comment 25

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5723/#review5827

> I cribbed this from djblets.webapi.resources, but actually I don't think this exception can happen at all from get_queryset(); if there are no objects found, it just returns a queryset with 0 objects.  Regardless, I'll make that change, just in case.

Enh more I think about it, the less I like putting in this exception at all, since I don't *think* it can ever be raised--I think the corresponding code in Djblets is actually wrong.  What do you think?

Comment 27

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5723/#review5891

> That will probably change this quarter. I can see the parents being associated with many bugs once we allow just pushing up multiples at a time.

Sure, but I don't want to make that change until we need it, largely because we can't easily test it until that point.  Do you mind if I leave this as is until that time?

Comment 28

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5723/#review5893

> Sure, but I don't want to make that change until we need it, largely because we can't easily test it until that point.  Do you mind if I leave this as is until that time?

Bah sorry, I guess your next comment implies that it's okay to leave it for now... I guess?  Not sure how you want me to resolve this issue, since your preferred solution is more complex (a mapping table).

Comment 29

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5723/#review5895

> The whole resource returns the summaries, this particular parameter filters the results.
> 
> How about "The review request must have the provided bug number."?
> 
> That being said, I wonder if we should be allowing a comma-separated list of bug numbers here, rather than just a single number. What do you think?

I see no use case right now for multiple bugs in one call.  This interface is being added specifically for the Bugzilla MozReview extension.  We can extend it later if needed.

> I'd rather we just call our own `get_queryset` rather than go through the indirection of using the "private" `_get_queryset`.

Yeah I suppose we don't need any of the extra functionality in _get_queryset().

> Is the "links" dictionary automatically added by RB with this return value? You may have to generate the links and add them manually, please check this.

Ugh, do we really need this?  What's the use case?

Comment 30

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5723/#review5897

> I think a docstring would be nice for this function.
> 
> Also, I think it would make some of the other code cleaner if the families argument was optional and if not provided a defaultdict would be created and returned. Then your first callsite can just call in to get the dictionary.

Nice, I was trying to think of a good way of doing that.  Hope my docstring makes sense; it's a bit hard to describe what the function does, despite it not being terribly complciated. :)

Comment 31

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5723/#review5923

> Enh more I think about it, the less I like putting in this exception at all, since I don't *think* it can ever be raised--I think the corresponding code in Djblets is actually wrong.  What do you think?

Maybe django used to raise the exception is previous versions or something. If you confirm django just gives the empty queryset in this case I'm fine leaving it out.

> Bah sorry, I guess your next comment implies that it's okay to leave it for now... I guess?  Not sure how you want me to resolve this issue, since your preferred solution is more complex (a mapping table).

Ya that's fine, we can fix it up later. You can just drop this issue.

> Ugh, do we really need this?  What's the use case?

RBTools relies on the resources having a common form. So ya, probably should add it in (shouldn't be hard, just steal from djblets).

djblets web API stuff is built around having resources represent models, so when you do other funky things you might have a bit of boilerplate.

Comment 32

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review6017

> I think the "while maybe adding an attachment" can just be part of the larger description.

Actually it's pretty obvious that it's optional from the signature so I'll just take that out.

> Hmmm, this is going to spam the **** out of the bug is it not? We'll get an attachment + separate comment for every commit. That could easily be > 20 comments for one push.
> 
> Is there any way we can do this as a single operation? or at least maybe just post one comment?

We can't do this as a single operation, at least not without a special API.  With one comment would be possible but would require restructing our signals (publishing the parent triggers publishing of all children, and then signals for each individual child are acted upon).  I'm also not sure if just removing the comment would prevent bugmail.  Perhaps what's best is to use the new ability to quash bugmail, when that is backported to bmo (https://bugzilla.mozilla.org/show_bug.cgi?id=1156105)--although, I'll have to double check if that also applies to updating and creating attachments.

Comment 33

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review6019

> Keep in mind that people doing append-at-the-end tend to only look at the parent.
> 
> That being said, I'm not a huge fan of append-at-the-end. It's not how we land patches (for Firefox at least) and I think reviewers should be looking at the final product that lands because it helps remove uncertainty.
> 
> This will likely become an issue when we import GitHub PRs. I feel Git[Hub] users aren't so accustomed to rewriting commits. I suppose we can tackle the problem later with added complexity, if needed. My vote is with Steven on this behavior.

I'm torn as well, but I see your point.  I'm not sure how we're going to enforce this, though.  Can we/do we want to completely remove the ability to do reviews, or at least "ship it"s on the parent, then?  If we don't, we're going to get the reverse of the confusion we have right now--instead of nothing happening in bmo when you "ship it" a child, nothing will happen when you "ship it" a parent.

Comment 34

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5983/#review6049

> I'm torn as well, but I see your point.  I'm not sure how we're going to enforce this, though.  Can we/do we want to completely remove the ability to do reviews, or at least "ship it"s on the parent, then?  If we don't, we're going to get the reverse of the confusion we have right now--instead of nothing happening in bmo when you "ship it" a child, nothing will happen when you "ship it" a parent.

Regarding spam, people may get tons of bugmail when a multi-part review goes up for review. We already have a spam problem :/

I think retaining r+ on parent does an r+ on all child review requests is acceptable. If the behavior turns out to be a disaster, we can change it later.

Comment 35

[Mark Côté [:mcote]]

Comment on attachment 8580138 [details]
MozReview Request: bz://1055021/mcote

/r/5723 - Bug 1055021 - mozreview: Get all review-request summaries for a given bug. r=smacleod
/r/5983 - Bug 1055021 - Create attachments for all children. r=smacleod

Pull down these commits:

hg pull -r 04575f3fde411e75149870e502d4cacd89e5e91d https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 36

[Mark Côté [:mcote]]

Comment on attachment 8580138 [details]
MozReview Request: bz://1055021/mcote

/r/7337 - Bug 1055021 - mozreview: Move extra-data-related functions from rbbz to mozreview.
/r/5723 - Bug 1055021 - mozreview: Get all review-request summaries for a given bug. r=smacleod
/r/5983 - Bug 1055021 - Create attachments for all children. r=smacleod

Pull down these commits:

hg pull -r c569f51fd8796b500d961354377e5ca4c8876247 https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 37

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review6089

> Regarding spam, people may get tons of bugmail when a multi-part review goes up for review. We already have a spam problem :/
> 
> I think retaining r+ on parent does an r+ on all child review requests is acceptable. If the behavior turns out to be a disaster, we can change it later.

Sounds good; I'll drop this issue, for now at least.

Comment 38

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review6143

> We can't do this as a single operation, at least not without a special API.  With one comment would be possible but would require restructing our signals (publishing the parent triggers publishing of all children, and then signals for each individual child are acted upon).  I'm also not sure if just removing the comment would prevent bugmail.  Perhaps what's best is to use the new ability to quash bugmail, when that is backported to bmo (https://bugzilla.mozilla.org/show_bug.cgi?id=1156105)--although, I'll have to double check if that also applies to updating and creating attachments.

Okay, we can suppress the general email for these operations but keep the flag mail (so requestees will still get notified).  Do we want to do this?  We could also add a single comment from the parent so there's some notification of a new patch without lots of spam.  That might be the best option.

Comment 39

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review6221

> We should really rename `type` here to not be a reserved word while you're changing things.

Actually we can't rename this because 'type' is passed as a keyword argument from Review Board.  Fix coming.

Comment 40

[Mark Côté [:mcote]]

Comment on attachment 8580138 [details]
MozReview Request: bz://1055021/mcote

/r/7337 - Bug 1055021 - mozreview: Move extra-data-related functions from rbbz to mozreview.
/r/5723 - Bug 1055021 - mozreview: Get all review-request summaries for a given bug. r=smacleod
/r/5983 - Bug 1055021 - Create attachments for all children. r=smacleod

Pull down these commits:

hg pull -r c7dc7e7ebec86f37607281b8efb0dda97c76c30a https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 42

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/7337/#review6259

::: pylib/mozreview/mozreview/extra_data.py:23
(Diff revision 2)
> +    return ReviewRequest.objects.get(
> +        commit_id=review_request.extra_data[IDENTIFIER_KEY])

This is not quite correct - it's possible for two review requests to have the same commit id if they are on a different repo (we just got bit by this in production, we should probably do a pass of our code to find all the places we're making this incorrect assumption).

So, we need to also filter by the repository the passed in review request is for.

::: pylib/mozreview/mozreview/extra_data.py:34
(Diff revision 2)
> +    If a review request is not found for the listed ID, get_rr_for_id will
> +    log this, and we'll skip that ID.

I wonder if we should fail in a more spectacular fashion here? If one of the ids is invalid something has gone quite wrong. The more operations we allow after this, the further from normal state we might drift - making it harder to track down what happened / manually repair things.

I don't have too strong a feeling about handling this right now though, so if it's a pain to change don't worry about it - just give it a thought.

::: pylib/mozreview/mozreview/extra_data.py:32
(Diff revision 2)
> +    the rids in that field.

"rrids" or just spell it out completely here. (I know this is just copied code, but we might as well update it if we're moving it - same goes for my other comments on moved code).

::: pylib/mozreview/mozreview/extra_data.py:54
(Diff revision 2)
> +def gen_rrs_by_rids(rids):
> +    for rid in rids:
> +        review_request = get_rr_for_id(rid)
> +        if review_request:
> +            yield review_request

we should either use "rrid" or just "id" for the 'Review Request ID' here.

::: pylib/mozreview/mozreview/extra_data.py:66
(Diff revision 2)
> +                      'rid %s because it does not appear to exist.'

"rrid" or "id"

::: pylib/mozreview/mozreview/extra_data.py:61
(Diff revision 2)
> +def get_rr_for_id(rid):

"id"

::: pylib/mozreview/mozreview/extra_data.py:16
(Diff revision 2)
> +UNPUBLISHED_RIDS_KEY = MOZREVIEW_KEY + '.unpublished_rids'

"RRIDS" or "IDS" - the actual key itself can stay since that'd be a pain to fix.

Comment 43

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5723/#review6263

::: pylib/mozreview/mozreview/resources/review_request_summary.py:16
(Diff revision 5)
> -from mozreview.resources.errors import CHILD_DOES_NOT_EXIST, NOT_PARENT
> +from mozreview.extra_data import (COMMITS_KEY, COMMIT_ID_KEY, MOZREVIEW_KEY,
> +                                  gen_child_rrs, get_parent_rr)

I generally prefer the one per line style when importing like this... also alphabetical order - but I'm assuming you wanted to group the constants? Maybe two import statements?

I'm pretty meh on this one - drop if you prefer.

::: pylib/mozreview/mozreview/resources/review_request_summary.py:25
(Diff revision 5)
> +    _name = 'review_request_summary'
> +    _name_plural = 'review_request_summaries'

`name` and `name_plural` assignments preferred.

::: pylib/mozreview/mozreview/resources/review_request_summary.py:46
(Diff revision 5)
> +        # TO DO: Then may get slow as the db size increase, particularly

I don't usually see "TO DO"... usually "TODO". Lets use the no space form to follow all the other TODOs sprinkled around.

::: pylib/mozreview/mozreview/resources/review_request_summary.py:55
(Diff revision 5)
> +            user=request.user,

This will mean you can only query for review requests that belong to *yourself* - I don't think we want to make that restriction?

::: pylib/mozreview/mozreview/resources/review_request_summary.py:86
(Diff revision 5)
> +        # FIXME: The returned data should actually be a dict, with keys
> +        # 'stat' and self.item_result_key mapped to 'ok' and the
> +        # family-summary dict, respectively, to match the standard Review
> +        # Board web API format.
> +
> +        return 200, self._summarize_families(request, families)[0]

Review Board doesn't automatically add these for you? If not we should just be calling `WebAPIResponse(...)` and passing our data in.

::: pylib/mozreview/mozreview/resources/review_request_summary.py:164
(Diff revision 5)
> +            if not self.has_access_permissions(request, rr):
> +                continue

Ah okay, nice, you're checking the children :)
/me deletes his previous comment about it.

::: pylib/mozreview/mozreview/resources/review_request_summary.py:84
(Diff revision 5)
> -            'children': [],
> +                            families)

I generally prefer not to rely on the order of default arguments - I would put a `families=families` here. This might just be some crazy preference I've made up though so drop if you feel like it.

::: pylib/mozreview/mozreview/resources/review_request_summary.py:115
(Diff revision 5)
> -                return CHILD_DOES_NOT_EXIST, {
> -                    'mozreview': {
> -                        'children': 'data for child review request %s not '
> +        missing_rrids = set()
> +
> +        families = self._sort_families(request, queryset)

super nit: remove the blank line and flip the order of lines.

::: pylib/mozreview/mozreview/resources/review_request_summary.py:126
(Diff revision 5)
> +                for commit_tuple in commit_tuples:
> +                    child_rrid = int(commit_tuple[1])

`for sha, child_rrid in commit_tuples:`
or, go for the whole shebang:
`[missing_rrids.add(child_rrid) for sha, child_rrid in commit_tuples if child_rrid not in family['children']]`

::: pylib/mozreview/mozreview/resources/review_request_summary.py:196
(Diff revision 5)
> +            'summary': 'Bug 1 - Update README.md.'

Worth checking that the way we output this can't possibly XSS bugzilla with your summary table (not sure how you output things there or what you assume)

::: pylib/mozreview/mozreview/resources/review_request_summary.py:204
(Diff revision 5)
> +        d['submitter'] = review_request.submitter.username

Should we also send the bugzilla user id, so that bugzilla could use it to find the actual user?

Not a blocker, just a thought.

::: pylib/mozreview/mozreview/resources/review_request_summary.py:230
(Diff revision 5)
> +        for family in families.values():

`for family in families.itervalues():`

Comment 44

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5983/#review6273

> Okay, we can suppress the general email for these operations but keep the flag mail (so requestees will still get notified).  Do we want to do this?  We could also add a single comment from the parent so there's some notification of a new patch without lots of spam.  That might be the best option.

Sounds good to me.

> Sounds good; I'll drop this issue, for now at least.

I don't know if I agree with us just being able to change it later. Yes, code wise we could, but if people are using it and we take it away they will be pissed.

Comment 45

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5983/#review6277

::: pylib/mozreview/mozreview/resources/review_request_summary.py:55
(Diff revision 5)
> +            status=None,

Why do we have this (and why is it in this commit not the last)? Is Review Board automatically filtering all queries to only have a specific status?

::: pylib/rbbz/rbbz/bugzilla.py:251
(Diff revision 5)
>          new_flag = {'name': 'review', 'status': '+'}

isn't always a new flag, lets just call this `flag`

::: pylib/rbbz/rbbz/extension.py:301
(Diff revision 5)
> -    # numeric Bugzilla userid into an email address. This lookup could be
> -    # avoided if Bugzilla accepted a numeric userid in the requestee parameter
> -    # when modifying an attachment.
> +        # immutable numeric Bugzilla userid into an email address. This lookup
> +        # could be avoided if Bugzilla accepted a numeric userid in the
> +        # requestee parameter when modifying an attachment.

Just while we're here, is this something you could get the BMO folks to add for us? Think it's worth it?

Comment 46

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/7337/#review6289

> This is not quite correct - it's possible for two review requests to have the same commit id if they are on a different repo (we just got bit by this in production, we should probably do a pass of our code to find all the places we're making this incorrect assumption).
> 
> So, we need to also filter by the repository the passed in review request is for.

Good point.  I did a quick search and found (and fixed) a couple other instances.

> I wonder if we should fail in a more spectacular fashion here? If one of the ids is invalid something has gone quite wrong. The more operations we allow after this, the further from normal state we might drift - making it harder to track down what happened / manually repair things.
> 
> I don't have too strong a feeling about handling this right now though, so if it's a pain to change don't worry about it - just give it a thought.

That's a good point, but yeah, tricky, since this is called in a variety of contexts.  Going to drop but I'll add a FIXME.

Comment 47

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5723/#review6291

> I generally prefer the one per line style when importing like this... also alphabetical order - but I'm assuming you wanted to group the constants? Maybe two import statements?
> 
> I'm pretty meh on this one - drop if you prefer.

Yes, I wanted to group the constants, although technically this *is* sorted, ascii-wise at least. :P

> `name` and `name_plural` assignments preferred.

Hm name and name_plural are defined as properties in djblets.webapi.resources, which default to using _name and _name_plural.  So the latter sounded like the right things to override.

> This will mean you can only query for review requests that belong to *yourself* - I don't think we want to make that restriction?

Hm yeah I'm not sure why I added this.

> I generally prefer not to rely on the order of default arguments - I would put a `families=families` here. This might just be some crazy preference I've made up though so drop if you feel like it.

Heh sure why not. :)

> Review Board doesn't automatically add these for you? If not we should just be calling `WebAPIResponse(...)` and passing our data in.

Oh I didn't make the FIXME clear: I don't want to fix this right now because it will break the Bugzilla extension.  After we switch it to fetching review requests by bug, we can fix this.  I'll clarify the FIXME.

Comment 48

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5723/#review6303

> Hm name and name_plural are defined as properties in djblets.webapi.resources, which default to using _name and _name_plural.  So the latter sounded like the right things to override.

Generally I'd avoid touching "private" (leading underscore) properties when we can. I'm going by what's done in RB core: https://github.com/reviewboard/reviewboard/blob/3847571705353a82c5c7cb5b3adbf32bcfe4d0f5/reviewboard/webapi/resources/repository.py#L48

Comment 49

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5723/#review6319

> Should we also send the bugzilla user id, so that bugzilla could use it to find the actual user?
> 
> Not a blocker, just a thought.

I don't think it's particularly useful at the moment.  I'll add it later if I need it.

> Worth checking that the way we output this can't possibly XSS bugzilla with your summary table (not sure how you output things there or what you assume)

The extension writes using jquery text() calls, so no worries about HTML creeping in there and messing things up.

Comment 50

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review6321

> Why do we have this (and why is it in this commit not the last)? Is Review Board automatically filtering all queries to only have a specific status?

Yup.  I can't figure out where it's doing it, but it doesn't return closed review requests by default.  I hadn't noticed it the first time through.  I'll add a test for it actually.

> Just while we're here, is this something you could get the BMO folks to add for us? Think it's worth it?

Let me ask what the plans are around accepting user IDs in lieu of usernames.

Comment 51

[Mark Côté [:mcote]]

Comment on attachment 8580138 [details]
MozReview Request: bz://1055021/mcote

/r/7337 - Bug 1055021 - mozreview: Move extra-data-related functions from rbbz to mozreview. r=smacleod
/r/5723 - Bug 1055021 - mozreview: Get all review-request summaries for a given bug. r=smacleod
/r/5983 - Bug 1055021 - mozreview: Create attachments for all children. r=smacleod

Pull down these commits:

hg pull -r 6367a823a5147360b9617bdd680a1a6442ac533d https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 52

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5983/#review6325

> Yup.  I can't figure out where it's doing it, but it doesn't return closed review requests by default.  I hadn't noticed it the first time through.  I'll add a test for it actually.

Ah, here I think: https://github.com/reviewboard/reviewboard/blob/3847571705353a82c5c7cb5b3adbf32bcfe4d0f5/reviewboard/reviews/managers.py#L457

Comment 53

[Mark Côté [:mcote]]

Really this is about one attachment per commit, since you can have multiple reviewers, and hence flags, per commit.

Comment 54

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/7337/#review6701

::: pylib/rbbz/rbbz/extension.py:15
(Diff revision 3)
> +from mozreview.extra_data import (UNPUBLISHED_RRIDS_KEY,
> +                                  gen_child_rrs,
> +                                  gen_rrs_by_rids,
> +                                  gen_rrs_by_extra_data_key)

lets put the constant last.

Comment 55

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/7337/#review6703

> lets put the constant last.

Actually, I'm fine with how this is.

Comment 56

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5723/#review6705

::: pylib/mozreview/mozreview/resources/review_request_summary.py:49
(Diff revision 6)
> +        # TODO: Then may get slow as the db size increase, particularly

"Then" -> "This"

Comment 57

[Steven MacLeod [:smacleod]]

Comment on attachment 8580138 [details]
MozReview Request: bz://1055021/mcote

Just clearing the flag until the final issues are addressed.

Comment 58

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review6715

> Let me ask what the plans are around accepting user IDs in lieu of usernames.

So it's planned for a future version of the REST API, but there are no definite timelines.  We'll see about it later in the year.

Comment 60

[Mark Côté [:mcote]]

First two commits have landed:

http://hg.mozilla.org/hgcustom/version-control-tools/rev/811e4b081af0
http://hg.mozilla.org/hgcustom/version-control-tools/rev/cc0bd5af947e

Comment 61

[Mark Côté [:mcote]]

Comment on attachment 8580138 [details]
MozReview Request: bz://1055021/mcote

/r/7337 - Bug 1055021 - mozreview: Include reviewer BMO ids in summary API. r=smacleod
/r/5983 - Bug 1055021 - mozreview: Create attachments for all children. r=smacleod
/r/5723 - Bug 1055021 - Prevent ship-its in the UI from parent review requests. r?smacleod

Pull down these commits:

hg pull -r 3ccbd50b05882defa46e59c96e6494bc4ac54736 https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 62

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5721/#review7139

Sorry, I messed up the history by submitting the first two patches separately. :\  Let me know if you'd like me to redo this somehow.

Comment 63

[Mark Côté [:mcote]]

Comment on attachment 8580138 [details]
MozReview Request: bz://1055021/mcote

/r/7337 - Bug 1055021 - mozreview: Include reviewer BMO ids in summary API. r?smacleod
/r/5983 - Bug 1055021 - mozreview: Create attachments for all children. r?smacleod
/r/5723 - Bug 1055021 - mozreview: Prevent ship-its in the UI from parent review requests. r?smacleod

Pull down these commits:

hg pull -r 2c2ac71e292a49a99bdaedd05f15384874b50eb0 https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 66

[Mark Côté [:mcote]]

Comment on attachment 8580138 [details]
MozReview Request: bz://1055021/mcote

/r/7337 - Bug 1055021 - mozreview: Include reviewer BMO ids in summary API. r?smacleod
/r/5983 - Bug 1055021 - mozreview: Create attachments for all children. r?smacleod
/r/5723 - Bug 1055021 - mozreview: Prevent ship-its in the UI from parent review requests. r?smacleod

Pull down these commits:

hg pull -r 94134e6e8a9f98c4699d60813d477f120a333c7c https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 67

[Mark Côté [:mcote]]

Comment on attachment 8580138 [details]
MozReview Request: bz://1055021/mcote

/r/7337 - Bug 1055021 - mozreview: Include reviewer BMO ids in summary API. r?smacleod
/r/5983 - Bug 1055021 - mozreview: Create attachments for all children. r?smacleod
/r/5723 - Bug 1055021 - mozreview: Prevent ship-its in the UI from parent review requests. r?smacleod

Pull down these commits:

hg pull -r 7805613b1d0c375340f46d10b281ccc9486ca209 https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 68

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/7337/#review7349

::: pylib/mozreview/mozreview/resources/review_request_summary.py:12
(Diff revision 7)
> +

no blank here.

::: pylib/mozreview/mozreview/resources/review_request_summary.py:211
(Diff revision 7)
> -        d['reviewers'] = [child.username for child in
> -                          review_request.target_people.all()]
> +        d['reviewers'] = [reviewer.username for reviewer in reviewers]
> +        d['reviewers_bmo_ids'] = [bzuser.bugzilla_user_id for bzuser in
> +                                  BugzillaUserMap.objects.filter(id__in=[
> +                                      reviewer.id for reviewer in reviewers])]

This seems strange as two lists, I think we should have a single list where the entries are dicts with `username` and `bmo_id` keys.

Comment 69

[Mark Côté [:mcote]]

Comment on attachment 8580138 [details]
MozReview Request: bz://1055021/mcote

/r/7337 - Bug 1055021 - mozreview: Include reviewer BMO ids in summary API. r?smacleod
/r/5983 - Bug 1055021 - mozreview: Create attachments for all children. r?smacleod
/r/5723 - Bug 1055021 - mozreview: Prevent ship-its in the UI from parent review requests. r?smacleod

Pull down these commits:

hg pull -r 79a031e3e0cf3923166ba596f49ac6f23435f36c https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 70

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/7337/#review7411

> This seems strange as two lists, I think we should have a single list where the entries are dicts with `username` and `bmo_id` keys.

I agree; however, since the logical key for this list of dicts would be (imho) "reviewers", which would break the Bugzilla extension, I think we should switch over later.  I opened https://bugzilla.mozilla.org/show_bug.cgi?id=1164756 for this.

Comment 71

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review7413

> I don't know if I agree with us just being able to change it later. Yes, code wise we could, but if people are using it and we take it away they will be ****.

smacleod won me over; I dropped support for mass flag changes.  As he says, we could add it later.

> Sounds good to me.

We've since decided to not require this, in the interests of shipping it.  We can use the email-suppression feature later, when it becomes available on BMO.

However, smacleod also raised the issue that, if the posting of a single attachment fails for some reason, it will leave MozReview in a weird state, with some commit review requests published and some not.  I updated the patch to only publish the commits after all attachments have been created.  This might leave the Bugzilla representation in a weird state, but that should be fixed by just republishing the parent (which itself would have remained in draft).

Comment 72

[Mark Côté [:mcote]]

Comment on attachment 8580138 [details]
MozReview Request: bz://1055021/mcote

/r/7337 - Bug 1055021 - mozreview: Include reviewer BMO ids in summary API. r?smacleod
/r/5983 - Bug 1055021 - mozreview: Create attachments for all children. r?smacleod
/r/5723 - Bug 1055021 - mozreview: Prevent ship-its in the UI from parent review requests. r?smacleod

Pull down these commits:

hg pull -r d94a65b580df256bc7ab07465ce21829e58e4ebf https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 73

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5983/#review7433

::: pylib/rbbz/rbbz/errors.py:22
(Diff revision 12)
> +class ParentShipItError(PublishError):
> +    def __init__(self):
> +        PublishError.__init__(self, '"Ship it" reviews on parent review '
> +                              'requests are not allowed.  Please review '
> +                              'individual commits.')

This should be in MozReview not rbbz.

https://hg.mozilla.org/hgcustom/version-control-tools/file/35943d03d552/pylib/mozreview/mozreview/errors.py

::: hgext/reviewboard/tests/test-bugzilla-review-flag-cleared.t:26
(Diff revision 12)
> -  $ rbmanage add-reviewer 1 --user reviewer
> +  $ rbmanage add-reviewer 2 --user reviewer
>    1 people listed on review request
>    $ rbmanage publish 1

I'm confused why this is working - we're not carrying over the reviewers to the parent in python yet, just js? So the parent shouldn't have any reviewers and the publish should fail (Or do we have default reviewers setup for the tests repos now?)

::: pylib/rbbz/rbbz/extension.py:198
(Diff revision 12)
> +def post_bugzilla_attachments(bugzilla, bug_id, review_request_draft,

This only posts a single bugzilla attachment, drop the plural.

::: pylib/rbbz/rbbz/extension.py:333
(Diff revision 12)
> +            for child in gen_child_rrs(review_request_draft):
> +                child_draft = child.get_draft(user=user)
> +
> +                if child_draft:
> +                    post_bugzilla_attachments(b, bug_id, child_draft, child)
> +
> +        # Publish draft commits. This will already include items that are in
> +        # unpublished_rids, so we'll remove anything we publish out of
> +        # unpublished_rids.
>          for child in gen_child_rrs(review_request_draft):

`gen_child_rrs` makes a database query for every commit to be published. I think we should just store the children in a list rather than call it twice now: `child_rrs = list(gen_child_rrs(...))`

::: pylib/rbbz/rbbz/extension.py:332
(Diff revision 12)
> +        if using_bugzilla:

Should we also be trying to obsolete the discarded request attachments here rather than wait until discard?

::: pylib/rbbz/rbbz/extension.py:398
(Diff revision 12)
> +    Note that a reviewer *must* have editbugs to set an attachment flag on
> +    someone else's attachment (i.e. the standard BMO review process).

In these cases should we be completely blocking the review like now, or attempting to post just a comment before failing? Right now this will block publish because we'll get a bugzilla error, correct?

People without editbugs should still be able to give feedback on a patch, just not make decisions about landing. I'd be fine with this getting filed as a followup bug though.

::: pylib/rbbz/rbbz/extension.py:415
(Diff revision 12)
> +    # FIXME: Update all attachments in one call.  This is not possible right

I'd prefer `TODO` over `FIXME`, it's much more common throughout the codebase.

Comment 74

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/7337/#review7437

Ship It!

Comment 75

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5983/#review7439

Also, please modify your commit message to remove the part about ship-it UI being removed - that is not present in this commit.

Comment 76

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5723/#review7441

::: pylib/mozreview/mozreview/static/mozreview/css/review.less:89
(Diff revision 12)
> +.commit-request .actions {
> +  #shipit-link {
> +    display: block;
> +  }
> +}

Please move this up above near the other .actions stuff.

::: pylib/mozreview/mozreview/static/mozreview/js/common.js:21
(Diff revision 12)
> -      $("#review_request").addClass("parent-request");
> +      $("body").addClass("parent-request");
>    } else {
> -      $("#review_request").addClass("commit-request");
> +      $("body").addClass("commit-request");

Please note in your commit message why you do this.

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

```
.parent-request {
  #id_shipit,
  #label[for="id_shipit"] {
    display: none;
  }
}
```

::: hgext/reviewboard/tests/test-bugzilla-review-interaction.t:220
(Diff revision 12)
> +Emulate the JavaScript by setting the reviewers on both parent and commit.
> +TODO: Implement the JavaScript bits on the server so we don't need to do this
> +in the tests.
> +
> +  $ rbmanage add-reviewer 3 --user reviewer --user rev2
> +  2 people listed on review request

Why didn't your previous commit have to do this?

Comment 77

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5723/#review7453

> Why didn't your previous commit have to do this?

Yeah just realized I put these changes in the wrong commit.  I'll move them back.

> ```
> .parent-request {
>   #id_shipit,
>   #label[for="id_shipit"] {
>     display: none;
>   }
> }
> ```

I assume you didn't mean to include the "#" in "#label[...".

Comment 78

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5723/#review7459

> I assume you didn't mean to include the "#" in "#label[...".

your assumption is correct.

Comment 79

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5983/#review7455

> I'm confused why this is working - we're not carrying over the reviewers to the parent in python yet, just js? So the parent shouldn't have any reviewers and the publish should fail (Or do we have default reviewers setup for the tests repos now?)

Yes, that was failing, as I accidentally did the test changes in the UI commit.  I'll fix the tests in this commit instead.

> In these cases should we be completely blocking the review like now, or attempting to post just a comment before failing? Right now this will block publish because we'll get a bugzilla error, correct?
> 
> People without editbugs should still be able to give feedback on a patch, just not make decisions about landing. I'd be fine with this getting filed as a followup bug though.

I put a TODO item that this has been filed as bug 1119065.

Comment 80

[Mark Côté [:mcote]]

Comment on attachment 8580138 [details]
MozReview Request: bz://1055021/mcote

/r/7337 - Bug 1055021 - mozreview: Include reviewer BMO ids in summary API. r?smacleod
/r/5983 - Bug 1055021 - mozreview: Create attachments for all children. r?smacleod
/r/5723 - Bug 1055021 - mozreview: Prevent ship-its in the UI from parent review requests. r?smacleod

Pull down these commits:

hg pull -r 9a3f4c91b754f1c0959f0f76bb5977ba1a405b22 https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 81

[Mark Côté [:mcote]]

Comment on attachment 8580138 [details]
MozReview Request: bz://1055021/mcote

/r/7337 - Bug 1055021 - mozreview: Include reviewer BMO ids in summary API. r?smacleod
/r/5983 - Bug 1055021 - mozreview: Create attachments for all children. r?smacleod
/r/5723 - Bug 1055021 - mozreview: Prevent ship-its in the UI from parent review requests. r?smacleod

Pull down these commits:

hg pull -r 9c12dfb581a46cdc978fbcaa109d28fc98052a17 https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 82

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5983/#review7807

Ship It!

Comment 83

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5723/#review7811

Your commit message summary isn't following the vct convention - please change to:
`mozreview: Prevent ship-its in the UI from parent review requests (Bug 1055021). r?smacleod`

Also please update the form on the other commits before landing.

Comment 84

[Steven MacLeod [:smacleod]]

Comment on attachment 8580138 [details]
MozReview Request: bz://1055021/mcote

https://reviewboard.mozilla.org/r/5721/#review7813

LGTM! :D

Just the nits about the commit message summaries before landing.

Comment 85

[Mark Côté [:mcote]]

Landed the first patch: http://hg.mozilla.org/hgcustom/version-control-tools/rev/fc10070d3b23

I'm waiting on the latter two until just before we run the Bugzilla script (bug 1158513).

Comment 86

[Mark Côté [:mcote]]

Other two patches have landed:

http://hg.mozilla.org/hgcustom/version-control-tools/rev/3f29681798d5
http://hg.mozilla.org/hgcustom/version-control-tools/rev/8f6f616a78fd

Comment 88

[Mark Côté [:mcote]]

Attachment: MozReview Request: Bug 1055021 - mozreview: Include reviewer BMO ids in summary API. r?smacleod

Comment 89

[Mark Côté [:mcote]]

Attachment: MozReview Request: Bug 1055021 - mozreview: Prevent ship-its in the UI from parent review requests. r?smacleod

Comment 90

[Mark Côté [:mcote]]

Attachment: MozReview Request: Bug 1055021 - mozreview: Create attachments for all children. r?smacleod

Comment 91

[Mark Côté [:mcote]]

Conversion script was successfully run today.  Only failure was bug 1142779, which is in a strange state (review request appears to still have an unpublished draft, and the attachment in bugzilla appears to be manually added, for only one child commit).  We'll just skip that one.