1229468 - Review Board Web API to submit a series of review requests

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Gregory Szorc [:gps]]

Submitting review requests from Mercurial via the Review Board web API requires a gazillion HTTP requests. Series involving several commits take a long time to submit. Actions are performed non-atomically and the risk of failure partially through submission is higher than it should be. As we keep adding more and more actions to submission (such as LDAP checking), submitting review requests keeps getting slower.

Let's establish a single Review Board Web API to ingest a series of commits and turn them into a review request series.

This will be part of a larger effort to overhaul how commits are submitted to MozReview. This will enable the Git integration to be simpler.

Comment 1

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod

Currently, the Mercurial server makes several HTTP requests to Review
Board's Web API to turn the pushed commits into review requests. Series
with many commits take longer to submit than series with a single commit
because we make additional requests for each review request / commit.

This commit introduces a new Review Board Web API that aims to
eventually replace the several existing Web API requests with a unified
/ single API call. Right now, this new API simply creates/updates the
parent/squashed review request and diff. Eventually, more of the logic
from pushhooks.py will move into the new Web API.

I'm not an expert in the Review Board API. So there should be a thorough
code review, which likely involves reading the new code next to existing
code in Review Board's Web API.

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8694470 [details]
MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/1-2/

Comment 3

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor

mozreview: copy more read-only processing code; r?smacleod

This is basically copying a lot of code from pushhooks.py. We don't yet
make any database modification as part of this code. This just proves
that a rough 1:1 copy continues to work.

Comment 4

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/26757/#review24543

::: pylib/mozreview/mozreview/resources/batch_review_request.py:175
(Diff revision 2)
> +            squashed_draft = ReviewRequestDraft.create(squashed_rr)

I think this is buggy. ReviewBoard doesn't copy fields from rr to draft. Instead, the Web API has the code for this. I fix this up as part of a not-yet-published commit.

I may have to fold all these commits before final landing :/

Comment 5

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: print more diff info; r=smacleod

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

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8694470 [details]
MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/2-3/

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8694493 [details]
MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/1-2/

Comment 8

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

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

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8699069 [details]
MozReview Request: mozreview: print more diff info; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27509/diff/1-2/

Comment 10

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: log user events; r=smacleod

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

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8694470 [details]
MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/3-4/

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8694493 [details]
MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/2-3/

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/1-2/

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8694470 [details]
MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/4-5/

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8694493 [details]
MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/3-4/

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/2-3/

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/3-4/

Comment 18

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod

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

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/4-5/

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8699612 [details]
MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28311/diff/1-2/

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8699069 [details]
MozReview Request: mozreview: print more diff info; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27509/diff/2-3/

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8699565 [details]
MozReview Request: mozreview: log user events; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28277/diff/1-2/

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 8694470 [details]
MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

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

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8694493 [details]
MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/4-5/

Comment 25

[Gregory Szorc [:gps]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

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

Comment 26

[Gregory Szorc [:gps]]

Comment on attachment 8699612 [details]
MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28311/diff/2-3/

Comment 27

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod

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

Comment 28

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod

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

Comment 29

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod

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

Comment 30

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod

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

Comment 31

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod

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

Comment 32

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod

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

Comment 33

[Dan Minor [:dminor]]

Comment on attachment 8694470 [details]
MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

https://reviewboard.mozilla.org/r/26757/#review25415

::: pylib/mozreview/mozreview/resources/batch_review_request.py:37
(Diff revision 6)
> +

nit: please add a second empty line

::: pylib/mozreview/mozreview/resources/batch_review_request.py:57
(Diff revision 6)
> +                'description': 'Repository to submit reviews to',

nit: Repository to which to submit reviews

::: pylib/mozreview/mozreview/resources/batch_review_request.py:128
(Diff revision 6)
> +                    identifier, local_site, False)

nit: fix indentation

::: pylib/mozreview/mozreview/resources/batch_review_request.py:194
(Diff revision 6)
> +    Callers must call ``diffset.save()`` afterwards for changes to be preserved.

Is there a use case for calling this function and then not saving the changes?

Comment 34

[Dan Minor [:dminor]]

Comment on attachment 8694493 [details]
MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor

https://reviewboard.mozilla.org/r/26783/#review25425

::: pylib/mozreview/mozreview/resources/batch_review_request.py:101
(Diff revision 5)
> +                    logger.error('commit missing key %s' % key)

Please include the commit sha1 to make debugging easier.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:104
(Diff revision 5)
> +                            'commits': 'Individual commit missing key %s' % key}}

Please include the commit sha1 to make debugging easier.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:273
(Diff revision 5)
> +

nit: remove extra newline

Comment 35

[Dan Minor [:dminor]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

https://reviewboard.mozilla.org/r/28177/#review25391

::: pylib/mozreview/mozreview/resources/batch_review_request.py:236
(Diff revision 6)
> +                    'warnings': warnings,

nit: please place in alphabetical order

::: pylib/mozreview/mozreview/resources/batch_review_request.py:509
(Diff revision 6)
> +        # TODO update reviewers on squashed draft.

So I think the only reason we were setting reviewers on squashed review requests so they could be published

Since we now allow publishing requests without reviewers we might be able to skip doing this work.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:536
(Diff revision 6)
> +def previous_reviewers(rr):

Since you have to rewrite this anyway, you might as well use the helper function here: https://dxr.mozilla.org/hgcustom_version-control-tools/source/pylib/mozreview/mozreview/review_helpers.py#10

Comment 36

[Gregory Szorc [:gps]]

Comment on attachment 8699773 [details]
MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28445/diff/1-2/

Comment 37

[Gregory Szorc [:gps]]

Comment on attachment 8699069 [details]
MozReview Request: mozreview: print more diff info; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27509/diff/3-4/

Comment 38

[Gregory Szorc [:gps]]

Comment on attachment 8699565 [details]
MozReview Request: mozreview: log user events; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28277/diff/2-3/

Comment 39

[Gregory Szorc [:gps]]

Comment on attachment 8694470 [details]
MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/6-7/

Comment 40

[Gregory Szorc [:gps]]

Comment on attachment 8694493 [details]
MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor

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

Comment 41

[Gregory Szorc [:gps]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/6-7/

Comment 42

[Gregory Szorc [:gps]]

Comment on attachment 8699612 [details]
MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28311/diff/3-4/

Comment 43

[Gregory Szorc [:gps]]

Comment on attachment 8699769 [details]
MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28437/diff/1-2/

Comment 44

[Gregory Szorc [:gps]]

Comment on attachment 8699770 [details]
MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28439/diff/1-2/

Comment 45

[Gregory Szorc [:gps]]

Comment on attachment 8699771 [details]
MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28441/diff/1-2/

Comment 46

[Gregory Szorc [:gps]]

Comment on attachment 8699772 [details]
MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28443/diff/1-2/

Comment 47

[Gregory Szorc [:gps]]

Comment on attachment 8699774 [details]
MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28447/diff/1-2/

Comment 48

[Gregory Szorc [:gps]]

Comment on attachment 8694470 [details]
MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/7-8/

Comment 49

[Gregory Szorc [:gps]]

Comment on attachment 8694493 [details]
MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/6-7/

Comment 50

[Gregory Szorc [:gps]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/7-8/

Comment 51

[Gregory Szorc [:gps]]

Comment on attachment 8699612 [details]
MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28311/diff/4-5/

Comment 52

[Gregory Szorc [:gps]]

Comment on attachment 8699769 [details]
MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28437/diff/2-3/

Comment 53

[Gregory Szorc [:gps]]

Comment on attachment 8699770 [details]
MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28439/diff/2-3/

Comment 54

[Gregory Szorc [:gps]]

Comment on attachment 8699771 [details]
MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28441/diff/2-3/

Comment 55

[Gregory Szorc [:gps]]

Comment on attachment 8699772 [details]
MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28443/diff/2-3/

Comment 56

[Gregory Szorc [:gps]]

Comment on attachment 8699774 [details]
MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28447/diff/2-3/

Comment 57

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/26755/#review26493

I've still got a single failure in test-auth.t in this series. But I think it's at the point where it can receive some serious review love. Hopefully the interdiffs will be small.

Comment 58

[Dan Minor [:dminor]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

https://reviewboard.mozilla.org/r/28177/#review26543

::: pylib/mozreview/mozreview/resources/batch_review_request.py:658
(Diff revision 8)
> +        logger.info('recognized reviewer: %s' % reviewer)

unrecognized reviewer?

Comment 59

[Gregory Szorc [:gps]]

Comment on attachment 8699773 [details]
MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28445/diff/2-3/

Comment 60

[Gregory Szorc [:gps]]

Comment on attachment 8699069 [details]
MozReview Request: mozreview: print more diff info; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27509/diff/4-5/

Comment 61

[Gregory Szorc [:gps]]

Comment on attachment 8699565 [details]
MozReview Request: mozreview: log user events; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28277/diff/3-4/

Comment 62

[Gregory Szorc [:gps]]

Comment on attachment 8694470 [details]
MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/8-9/

Comment 63

[Gregory Szorc [:gps]]

Comment on attachment 8694493 [details]
MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/7-8/

Comment 64

[Gregory Szorc [:gps]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/8-9/

Comment 65

[Gregory Szorc [:gps]]

Comment on attachment 8699612 [details]
MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod

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

Comment 66

[Gregory Szorc [:gps]]

Comment on attachment 8699769 [details]
MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28437/diff/3-4/

Comment 67

[Gregory Szorc [:gps]]

Comment on attachment 8699770 [details]
MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28439/diff/3-4/

Comment 68

[Gregory Szorc [:gps]]

Comment on attachment 8699771 [details]
MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28441/diff/3-4/

Comment 69

[Gregory Szorc [:gps]]

Comment on attachment 8699772 [details]
MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28443/diff/3-4/

Comment 70

[Gregory Szorc [:gps]]

Comment on attachment 8699774 [details]
MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28447/diff/3-4/

Comment 71

[Steven MacLeod [:smacleod]]

Comment on attachment 8699773 [details]
MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod

https://reviewboard.mozilla.org/r/28445/#review26729

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:525
(Diff revision 3)
> -                               password=privileged_password):
> +                               password=privileged_password) as rbc:

So strange... I've added some more testing around the security of our ldap association stuff just to be safe, it's in your review queue.

Comment 72

[Steven MacLeod [:smacleod]]

Comment on attachment 8699069 [details]
MozReview Request: mozreview: print more diff info; r=smacleod

https://reviewboard.mozilla.org/r/27509/#review26781

Comment 73

[Steven MacLeod [:smacleod]]

Comment on attachment 8699565 [details]
MozReview Request: mozreview: log user events; r=smacleod

https://reviewboard.mozilla.org/r/28277/#review26783

Comment 74

[Steven MacLeod [:smacleod]]

Comment on attachment 8694470 [details]
MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

https://reviewboard.mozilla.org/r/26757/#review27041

::: pylib/mozreview/mozreview/resources/batch_review_request.py:74
(Diff revision 9)
> +        logger.info('processing post_series request for %s' % user)

We call this "BatchReviewRequestResource" but refer to it as a "post_series" request here, we should be consistent with the naming.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:102
(Diff revision 9)
> +            repo = Repository.objects.get(pk=repo_id)

You should be passing the `local_site` here as well. It's not strictly needed in our case but if we ever start taking advantage of local sites we might as well have things working (especially since you've already fetched it above).

Local sites in Review Board are a way to host multiple Review Board's from the same database and web head (at separate paths I believe). When writing new stuff in the extensions it's probably a good idea to sprinkle the local site stuff around where Review Board does it itself.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:102
(Diff revision 9)
> +            repo = Repository.objects.get(pk=repo_id)
> +        except ObjectDoesNotExist:

We might as well call `int()` on the id and catch a `ValueErorr` to return `INVALID_FORM_DATA` as well since we're only supporting the integer repo id here (On the Review Request endpoint you can use things like the repos name as well)

::: pylib/mozreview/mozreview/resources/batch_review_request.py:103
(Diff revision 9)
> +        except ObjectDoesNotExist:

Please catch the specific Repository.DoesNotExist

::: pylib/mozreview/mozreview/resources/batch_review_request.py:108
(Diff revision 9)
> +            return self._no_access_error(user)

Now that we're on 2.5.2 I'm pretty sure this method is gone and we should be using `self.get_no_access_erorr()` which takes the request object rather than the user object.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:123
(Diff revision 9)
> +            squashed_rr = ReviewRequest.objects.get(commit_id=identifier)

You also need to specify the repository here or we're not guaranteed unique (we've been bitten by this bug before and had to fix it).

::: pylib/mozreview/mozreview/resources/batch_review_request.py:126
(Diff revision 9)
> +                return self._no_access_error(user)

Same, update this

::: pylib/mozreview/mozreview/resources/batch_review_request.py:127
(Diff revision 9)
> +        except ObjectDoesNotExist:

Please use `ReviewRequest.DoesNotExist`

::: pylib/mozreview/mozreview/resources/batch_review_request.py:148
(Diff revision 9)
> +            diffset = DiffSet.objects.create_from_data(
> +                repository=repo,
> +                diff_file_name='diff',
> +                diff_file_contents=commits['squashed']['diff'],
> +                parent_diff_file_name=None,
> +                parent_diff_file_contents=None,
> +                diffset_history=None,
> +                basedir='',
> +                request=request,
> +                base_commit_id=commits['squashed'].get('base_commit_id'),
> +                save=True
> +                )

In the future I think a big performance gain could be realized by moving all of the diffset creation code outside of the transaction. Diff parsing and processing can take quite a while for Review Board and once we are creating all of the commit requests in this transaction we could end up locking things for quite a while (with large diffs).

Since diffsets can just hang around and not show up in the UI without being attached to a history it shouldn't cause any problems - and in the case of an error we could just manually clean them up.

Maybe worth adding a TODO about this for the future?

::: pylib/mozreview/mozreview/resources/batch_review_request.py:161
(Diff revision 9)
> +            update_diffset_history(squashed_rr, diffset)

Rather than do this you should be able to just pass the `diffset_history=squashed_rr.diffset_history` to `create_from_data()` above. You won't need to call save afterwards either.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:164
(Diff revision 9)
> +        except Exception:
> +            logger.exception('error processing parent diff')
> +            return INVALID_FORM_DATA, {
> +                'fields': {'commits': 'error processing parent diff'}}

I believe this breaks our transaction handling - Django transactions rely on an exception to roll things back. It appears to me you're going to end up with diffsets and review requests hanging around by doing this.

What we'll want to do is throw exceptions up out of the transacation and catch them outside of it, then return our error response.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:165
(Diff revision 9)
> +            logger.exception('error processing parent diff')
> +            return INVALID_FORM_DATA, {
> +                'fields': {'commits': 'error processing parent diff'}}

Please refer to the "parent" diff as the "squashed" diff in code and logs. Review Board has a specific thing referred to as a parent diff and I want to avoid them ever getting confused.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:169
(Diff revision 9)
> +        discarded_diffset = None
> +
> +        try:
> +            squashed_draft = squashed_rr.draft.get()
> +            if squashed_draft.diffset and squashed_draft.diffset != diffset:
> +                discarded_diffset = squashed_draft.diffset
> +        except ReviewRequestDraft.DoesNotExist:
> +            squashed_draft = ReviewRequestDraft.create(squashed_rr)
> +
> +        squashed_draft.diffset = diffset
> +        squashed_draft.save()
> +
> +        if discarded_diffset:
> +            discarded_diffset.delete()

Please refactor this into a standalone function, it's definitely something reusable and as complexity grows it'll keep everything cleaner

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:137
(Diff revision 9)
> +    batch_request_resource = api_root.get_extension(
> +        extension_name='mozreview.extension.MozReviewExtension')\
> +        .get_batch_review_requests()

I *think* this now can throw a bunch of `APIError`s that we don't catch anywhere and will spit a traceback to the pusher?

Comment 75

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/26757/#review27041

> Rather than do this you should be able to just pass the `diffset_history=squashed_rr.diffset_history` to `create_from_data()` above. You won't need to call save afterwards either.

Ah, okay, I realize what's going on here (You need to delete the current draft diffset before calculating the diff revision). In that case we should just pass `save=False` to the `create_from_data`, then delete the drafts diffset, then call save to perform the revision calculation for us.

Comment 76

[Steven MacLeod [:smacleod]]

Comment on attachment 8694493 [details]
MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor

https://reviewboard.mozilla.org/r/26783/#review27061

::: pylib/mozreview/mozreview/resources/batch_review_request.py:214
(Diff revision 8)
> +        # API object.

No API objects here

::: pylib/mozreview/mozreview/resources/batch_review_request.py:301
(Diff revision 8)
> +    """Obtain a dictionary containing review metadata.

When we're in RB extension land I'd **really** like to keep the distinction between "review" and "review request" crystal clear. I'd be very happy if you could fix any problems the pushhooks code has while porting it over.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:314
(Diff revision 8)
> +        rd['public'] = False
> +    except ReviewRequestDraft.DoesNotExist:
> +        rd['public'] = True

I'm pretty sure this isn't always correct - a review requests public state is not `True` in all cases where there is no draft. I'm pretty sure you can have a review request where the draft doesn't exist but `reviewrequest.public == False`.

I believe what you'll want when a draft doesn't exist is to set `rd['public'] = rr.public`. In the case of the draft existing, it *is* always `False` like you have:
> Whether or not the draft is public. This will always be false up until the time it is first made public. At that point, the draft is deleted.

This actually comes from the Web API though, since the draft model has no `public` field. So we're fine keeping the explicit False in the draft case.

Comment 77

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/26757/#review27089

::: pylib/mozreview/mozreview/resources/batch_review_request.py:123
(Diff revision 9)
> +            squashed_rr = ReviewRequest.objects.get(commit_id=identifier)
> +            if not squashed_rr.is_mutable_by(user):
> +                logger.warn('%s not mutable by %s' % (squashed_rr.id, user))

Since we have direct access and are manipulating the review request directly instead of through the API we won't be prevented from updating requests which are closed / discarded etc.

I think we need to have checks for these types of cases (I'm not sure if we test them all, and I know we have at least one case where the request is submitted that right now we just send back a traceback about the commit id being used.)

Comment 78

[Steven MacLeod [:smacleod]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

https://reviewboard.mozilla.org/r/28177/#review27091

::: pylib/mozreview/mozreview/resources/batch_review_request.py:254
(Diff revision 9)
> -                    user, repo, identifier, local_site, False)
> +                    user, repo, identifier, local_site, True)

This change shouldn't actually do anything except cause Review Board to run extra code, throw a `NotImplemenetedError` exception and then fallback and just set the `commit_id` field. If you're getting different behaviour with `True` vs `False` here I'm quite confused.

We should stick with `False`.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:384
(Diff revision 9)
> -                #update_review_request(rr, commit)
> +                squashed_reviewers |= set(u for u in draft.target_people.all())

Personally I prefer calling the set method rather than the operator, plus the methods can take any iterable so we could just do `squashed_reviewers.update(u for u in draft.target_people.all())`

::: pylib/mozreview/mozreview/resources/batch_review_request.py:417
(Diff revision 9)
> +                assumed_old_rid = unclaimed_rids[0]
> +                unclaimed_rids.pop(0)

IIRC `pop()` returns what it removes, we should just combine these two? Might as well do it now.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:437
(Diff revision 9)
> +            rr = ReviewRequest.objects.create(user, repo, None, local_site, True)

It does nothing if you don't pass a `commit_id` (3rd argument) but pass `create_from_commit_id=True` (5th argument). We should stop passing `create_from_commit_id` here, it defaults to `False`.

(Also, can we start specifying the argument names, I have to keep looking up the order / names).

::: pylib/mozreview/mozreview/resources/batch_review_request.py:447
(Diff revision 9)
> +            assert isinstance(rr.id, int)

Why is this here? What else could it be? Can we get `None` here or something??

::: pylib/mozreview/mozreview/resources/batch_review_request.py:513
(Diff revision 9)
> +        # TODO update reviewers on squashed draft.

You do this above? and the post save hook we have on the draft updates our special field. Am I missing something or is this just a leftover comment?

::: pylib/mozreview/mozreview/resources/batch_review_request.py:548
(Diff revision 9)
> +    # TODO use review_helpers.gen_latest_reviews?

Yes, do this, I consider it a blocker - I'd really like to keep these things consistent throughout.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:602
(Diff revision 9)
> +def create_review_request_draft(rr):

I really don't see what `ReviewRequestDraft.create()` isn't doing for us if we call it instead? The only variable I see things important hidden behind is `draft_is_new`, which if we're sure we're creating a draft and not updating one it will always be `True`.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:624
(Diff revision 9)
> +    draft.extra_data.update(rr.extra_data)

This could cause problems with dictionaries within the dictionary. You should definitely deep copy.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:659
(Diff revision 9)
> +        logger.info('recognized reviewer: %s' % reviewer)

unrecognized

::: pylib/mozreview/mozreview/resources/batch_review_request.py:685
(Diff revision 9)
> +        diffset_history=None,
> +        basedir='',
> +        request=request,
> +        base_commit_id=commit.get('base_commit_id'),
> +        save=True,

We want `diffset_history` set properly here, then set `save=False`, then immediately after remove the draft diffset if it exists (which I think should be its own function), then finally save the diffset.

Comment 79

[Steven MacLeod [:smacleod]]

Comment on attachment 8699612 [details]
MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod

https://reviewboard.mozilla.org/r/28311/#review27117

Comment 80

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/26757/#review27041

> I *think* this now can throw a bunch of `APIError`s that we don't catch anywhere and will spit a traceback to the pusher?

It was like this before. That's why there are bugs with people seeing stacks from the server when doing `hg push`. This gets fixed as part of the refactor to switch the wire protocol to use JSON. I'm dropping this issue because it is existing.

Comment 81

[Steven MacLeod [:smacleod]]

Comment on attachment 8699769 [details]
MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod

https://reviewboard.mozilla.org/r/28437/#review27121

Comment 82

[Steven MacLeod [:smacleod]]

Comment on attachment 8699770 [details]
MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod

https://reviewboard.mozilla.org/r/28439/#review27129

The reason for the old RBTools version was unicode problems which should all have been fixed now.

Comment 83

[Steven MacLeod [:smacleod]]

Comment on attachment 8699771 [details]
MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod

https://reviewboard.mozilla.org/r/28441/#review27133

Comment 84

[Steven MacLeod [:smacleod]]

Comment on attachment 8699772 [details]
MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod

https://reviewboard.mozilla.org/r/28443/#review27135

Comment 85

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/26757/#review27041

> It was like this before. That's why there are bugs with people seeing stacks from the server when doing `hg push`. This gets fixed as part of the refactor to switch the wire protocol to use JSON. I'm dropping this issue because it is existing.

I moreso meant exceptions due to conditions which we used to handle, but now they'll be coming down from Review Board as an HTTP code that will throw on us. I don't have anything concrete though, and if the tests pass I guess we can just patch them up case by case.

Comment 86

[Steven MacLeod [:smacleod]]

Comment on attachment 8699774 [details]
MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod

https://reviewboard.mozilla.org/r/28447/#review27147

Comment 87

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/28177/#review27091

> Why is this here? What else could it be? Can we get `None` here or something??

I think this is left over from client-side foo and RBTools. We're obviously not hitting it or we'd have a bug report on file. I'll remove it.

Comment 88

[Gregory Szorc [:gps]]

Comment on attachment 8699773 [details]
MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28445/diff/3-4/

Comment 89

[Gregory Szorc [:gps]]

Comment on attachment 8699069 [details]
MozReview Request: mozreview: print more diff info; r=smacleod

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

Comment 90

[Gregory Szorc [:gps]]

Comment on attachment 8699565 [details]
MozReview Request: mozreview: log user events; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28277/diff/4-5/

Comment 91

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: add test for pushing to submitted review request; r=smacleod

We swallow output because it currently spews an exception stack, which
is a bug already on file.

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

Comment 92

[Gregory Szorc [:gps]]

Comment on attachment 8694470 [details]
MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/9-10/

Comment 93

[Gregory Szorc [:gps]]

Comment on attachment 8694493 [details]
MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/8-9/

Comment 94

[Gregory Szorc [:gps]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/9-10/

Comment 95

[Gregory Szorc [:gps]]

Comment on attachment 8699612 [details]
MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28311/diff/6-7/

Comment 96

[Gregory Szorc [:gps]]

Comment on attachment 8699769 [details]
MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28437/diff/4-5/

Comment 97

[Gregory Szorc [:gps]]

Comment on attachment 8699770 [details]
MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28439/diff/4-5/

Comment 98

[Gregory Szorc [:gps]]

Comment on attachment 8699771 [details]
MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28441/diff/4-5/

Comment 99

[Gregory Szorc [:gps]]

Comment on attachment 8699772 [details]
MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28443/diff/4-5/

Comment 100

[Gregory Szorc [:gps]]

Comment on attachment 8699774 [details]
MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28447/diff/4-5/

Comment 101

[Steven MacLeod [:smacleod]]

Comment on attachment 8706702 [details]
MozReview Request: reviewboard: add test for pushing to submitted review request; r=smacleod

https://reviewboard.mozilla.org/r/30437/#review27323

Comment 102

[Gregory Szorc [:gps]]

Comment on attachment 8699773 [details]
MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28445/diff/4-5/

Comment 103

[Gregory Szorc [:gps]]

Comment on attachment 8699069 [details]
MozReview Request: mozreview: print more diff info; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27509/diff/6-7/

Comment 104

[Gregory Szorc [:gps]]

Comment on attachment 8699565 [details]
MozReview Request: mozreview: log user events; r=smacleod

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

Comment 105

[Gregory Szorc [:gps]]

Comment on attachment 8706702 [details]
MozReview Request: reviewboard: add test for pushing to submitted review request; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30437/diff/1-2/

Comment 106

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: add test for pushing to a discarded series; r?smacleod

We didn't have explicit test coverage of this before.

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

Comment 107

[Gregory Szorc [:gps]]

Comment on attachment 8694470 [details]
MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

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

Comment 108

[Gregory Szorc [:gps]]

Comment on attachment 8694493 [details]
MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26783/diff/9-10/

Comment 109

[Gregory Szorc [:gps]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

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

Comment 110

[Gregory Szorc [:gps]]

Comment on attachment 8699612 [details]
MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28311/diff/7-8/

Comment 111

[Gregory Szorc [:gps]]

Comment on attachment 8699769 [details]
MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod

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

Comment 112

[Gregory Szorc [:gps]]

Comment on attachment 8699770 [details]
MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod

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

Comment 113

[Gregory Szorc [:gps]]

Comment on attachment 8699771 [details]
MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod

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

Comment 114

[Gregory Szorc [:gps]]

Comment on attachment 8699772 [details]
MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod

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

Comment 115

[Gregory Szorc [:gps]]

Comment on attachment 8699774 [details]
MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod

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

Comment 116

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/26755/#review27405

No matter how much I try, I can't figure out how to handle the diffset manipulation in the way smacleod suggests. I've tried a ton of different combinations of API calls and nothing seems to work. I'm inclined to push what I have working and leave refactoring the diffset code to a follow-up bug.

Comment 117

[Gregory Szorc [:gps]]

Comment on attachment 8699773 [details]
MozReview Request: reviewboard: use privileged RBTools client when setting LDAP association; r=smacleod

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

Comment 118

[Gregory Szorc [:gps]]

Comment on attachment 8699069 [details]
MozReview Request: mozreview: print more diff info; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27509/diff/7-8/

Comment 119

[Gregory Szorc [:gps]]

Comment on attachment 8699565 [details]
MozReview Request: mozreview: log user events; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28277/diff/6-7/

Comment 120

[Gregory Szorc [:gps]]

Comment on attachment 8706702 [details]
MozReview Request: reviewboard: add test for pushing to submitted review request; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30437/diff/2-3/

Comment 121

[Gregory Szorc [:gps]]

Comment on attachment 8707190 [details]
MozReview Request: reviewboard: add test for pushing to a discarded series; r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30607/diff/1-2/

Comment 122

[Gregory Szorc [:gps]]

Comment on attachment 8694470 [details]
MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26757/diff/11-12/

Comment 123

[Gregory Szorc [:gps]]

Comment on attachment 8694493 [details]
MozReview Request: mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor

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

Comment 124

[Gregory Szorc [:gps]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28177/diff/11-12/

Comment 125

[Gregory Szorc [:gps]]

Comment on attachment 8699612 [details]
MozReview Request: mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28311/diff/8-9/

Comment 126

[Gregory Szorc [:gps]]

Comment on attachment 8699769 [details]
MozReview Request: reviewboard: move review submission logic out of pushhooks.py; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28437/diff/6-7/

Comment 127

[Gregory Szorc [:gps]]

Comment on attachment 8699770 [details]
MozReview Request: mozreview: use RBTools 0.7.5 everywhere; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28439/diff/6-7/

Comment 128

[Gregory Szorc [:gps]]

Comment on attachment 8699771 [details]
MozReview Request: reviewboard: use built-in mechanism to disable caching; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28441/diff/6-7/

Comment 129

[Gregory Szorc [:gps]]

Comment on attachment 8699772 [details]
MozReview Request: reviewboard: use built-in mechanism to not save cookies; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28443/diff/6-7/

Comment 130

[Gregory Szorc [:gps]]

Comment on attachment 8699774 [details]
MozReview Request: reviewboard: don't use ReviewBoardClient as a context manager; r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28447/diff/6-7/

Comment 131

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: use demandimport.deactivated; r?smacleod

This is the proper way to temporarily deactivate demandimport, as it
will restore the previous state instead of assuming it was enabled to
begin with.

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

Comment 132

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/26757/#review27505

lgtm

Comment 133

[Dan Minor [:dminor]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

https://reviewboard.mozilla.org/r/28177/#review27507

lgtm

::: pylib/mozreview/mozreview/resources/batch_review_request.py:165
(Diff revision 12)
> +    * TODO Bug 1047465

Bug 1047465 is resolved fixed.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:581
(Diff revision 12)
> +    # consistent. For now, we duplicate that logic here.

I think you've fixed this TODO :)

Comment 134

[Steven MacLeod [:smacleod]]

Comment on attachment 8707190 [details]
MozReview Request: reviewboard: add test for pushing to a discarded series; r?smacleod

https://reviewboard.mozilla.org/r/30607/#review27533

Comment 135

[Steven MacLeod [:smacleod]]

Comment on attachment 8694470 [details]
MozReview Request: mozreview: add web API to submit an entire series of commits (bug 1229468); r?smacleod, dminor

https://reviewboard.mozilla.org/r/26757/#review27535

Ya, it's not a huge deal the way you have the diffset processing. I agree, we can just poke at it after we've landed this stuff.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:38
(Diff revision 12)
> +class SquashedDiffProcessingException(Exception):
> +    pass

In future iterations of your series you don't special case any other diffset processing errors? We should probably have a genric one for diffsets and actually check for all of them.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:128
(Diff revision 12)
> +        except SquashedDiffProcessingException:
> +            return INVALID_FORM_DATA, {
> +                'fields': {'commits': 'error processing squashed diff'}}

There might be other exceptions we should special case to return proper errors instead of just a 500, we can iterate on that though.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:167
(Diff revision 12)
> +        # Calling create_from_data() instead of create_from_upload() skips
> +        # diff size validation. We allow unlimited diff sizes, so no biggie.
> +        try:

Please move this comment inside the try block to the actaul `create_from_data` call.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:189
(Diff revision 12)
> +            logger.exception('error processing squashed diff')

I'd prefer if we logged a little more information about what the exception actually was.

Comment 136

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/26757/#review27041

> Ah, okay, I realize what's going on here (You need to delete the current draft diffset before calculating the diff revision). In that case we should just pass `save=False` to the `create_from_data`, then delete the drafts diffset, then call save to perform the revision calculation for us.

Dropping this, we can iterate.

Comment 137

[Steven MacLeod [:smacleod]]

Comment on attachment 8699070 [details]
MozReview Request: mozreview: create child review requests from batch API (bug 1229468); r?smacleod, dminor

https://reviewboard.mozilla.org/r/28177/#review27541

::: pylib/mozreview/mozreview/resources/batch_review_request.py:42
(Diff revision 12)
> +from ..review_helpers import (
> +    gen_latest_reviews,
> +)

I'd much rather we just call out the entire import path, I don't think we're doing this anywhere else.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:670
(Diff revision 12)
> +    diffset = DiffSet.objects.create_from_data(
> +        repository=rr.repository,
> +        diff_file_name='diff',
> +        diff_file_contents=commit['diff'],
> +        parent_diff_file_name='diff',
> +        parent_diff_file_contents=commit.get('parent_diff'),
> +        diffset_history=None,
> +        basedir='',
> +        request=request,
> +        base_commit_id=commit.get('base_commit_id'),
> +        save=True,
> +    )

Catch / Throw a diffset processing exception so we can 400 instead of 500.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:685
(Diff revision 12)
> +    discarded_diffset = None
> +    if draft.diffset and draft.diffset != diffset:
> +        discarded_diffset = draft.diffset
> +
> +    draft.diffset = diffset
> +    draft.save()
> +
> +    if discarded_diffset:
> +        discarded_diffset.delete()

This logic should really be merged into `update_review_request_draft_diffset` in a way that you can just re-use / call it here too.

Comment 138

[Steven MacLeod [:smacleod]]

Comment on attachment 8707257 [details]
MozReview Request: reviewboard: use demandimport.deactivated; r?smacleod

https://reviewboard.mozilla.org/r/30625/#review27547

Comment 139

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/44901a8bbcd8e06252578b171b2c6c5271b6023a
mozreview: add web API to submit an entire series of commits (bug 1229468); r=smacleod, dminor

https://hg.mozilla.org/hgcustom/version-control-tools/rev/64bc74f4b91a7730c7d03c4fcbe9f4a723813a75
mozreview: copy more read-only processing code (bug 1229468); r=smacleod, dminor

https://hg.mozilla.org/hgcustom/version-control-tools/rev/252a96904fba6c1d1bc11caf6a40bf6d09dfe3d7
mozreview: create child review requests from batch API (bug 1229468); r=smacleod, dminor

https://hg.mozilla.org/hgcustom/version-control-tools/rev/1cc42ef4b24f5b9e25f737ceba51ddc0ef588ea1
mozreview: cache reviewer to User lookups (bug 1229468); r=smacleod

Comment 140

[Gregory Szorc [:gps]]

Deployed to prod.