Multi-commit Pull Request Diff Support

Review Request #8547 — Created Nov. 19, 2016 and updated

Information

Review Board
dvcs
cc31765...

Reviewers

Reviewboard 4 introduces the concept of a DiffCommit, where a list of them can be used to represent a series of commits. This is especially helpful for pull requests, as they are often created and updated with multiple commits at a time.

All the commits in the pull request when the review request is created will be added to a revision. Further updates to the pull request will cause all the commits (not just the commits since then) to be added to the next revision, and so on.

This integrates DiffCommit with the current pull request implementation; the aim is to support pull requests created with multiple commits.

Added unit tests
- to verify the pull request hook is called, and only works with pull_request events and the right signature
- to verify pull requests are created and updated when a pull request is created
- to verify the pull request's status starts off with 'pending', is 'error' when there's open issues, and 'success' when approved

Manual testing
- Created a pull request on a repository linked to ReviewBoard through webhooks
- Verified the review request was created with 'pending' status
- Verified that opening an issue on the review request sets the 'error' status on the pull request
- Verified that having no open issue and adding a 'Ship it!' sets the 'success' status on the pull request
- Verified that pull requests with multiple commits get put into a single DiffSet and get rendered together.

Description From Last Updated

'datetime' imported but unused

reviewbotreviewbot

Col: 80 E501 line too long (96 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (117 > 79 characters)

reviewbotreviewbot

'datetime' imported but unused

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (91 > 79 characters)

reviewbotreviewbot

This should be non-None if len(d['parents']) > 1.

brenniebrennie

'datetime' imported but unused

reviewbotreviewbot

'ipdb' imported but unused

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (91 > 79 characters)

reviewbotreviewbot

'DiffSet' imported but unused

reviewbotreviewbot

Alphabetical order.

brenniebrennie

Missing 'Args' in docstring. Also this needs to go into deeper detail about what it actually does i.e. publishes the …

brenniebrennie

If these are constants that GitHub expects, make them constants on the class. e.g. PR_STATUS_SUCCESS etc.

brenniebrennie

Can you outline what the status means?

brenniebrennie

"The repository that the commit belongs to."

brenniebrennie

Missing period. "The full ID of the commit for which to set the status."

brenniebrennie

Paragraphs should have a blank line between them. "The status to set for the commit."

brenniebrennie

When these are constants you should outline them with, e.g. This must be one of the following values: * :py:attr:`CONSTANT_ONE` …

brenniebrennie

"The description ..." The defaults of params show up in documentation.

brenniebrennie

"unicode, optional"

brenniebrennie

The defaults of params show up in documentation."The HTTP(S) URL..."

brenniebrennie

No blank line here.

brenniebrennie

This needs to be a constant on the class.

brenniebrennie

Missing "args" and "returns" sections. This must be written in the imperitive mood, i.e., "Create or update ...."

brenniebrennie

Return a 400 bad request and log that the user doesn't exist?

brenniebrennie

This should probably be a constant in the siteconfig

brenniebrennie

Since this can fail, we should surround this with a try/except. In the event of a failure, we should log …

brenniebrennie

.split('=', 1) will ensure we only split on the first = which is what we're going to want to do …

brenniebrennie

We should log this.

brenniebrennie

"Create"

brenniebrennie

"The repository that the review request is associated with."

brenniebrennie

dict

brenniebrennie

django.http.HttpRequest, optional

brenniebrennie

Regarding my comment that this should throw SCMError, this must document that fact: Raises: reviewboard.scmtools.errors.SCMError: Raised when ....

brenniebrennie

No blank line here.

brenniebrennie

an SCMError is probably more appropriate here.

brenniebrennie

Ths should pass the exception up the call stack and should log it in the method that calls this.

brenniebrennie

In comments from mentors, I've been asked to provide the fully qualified path to the class type in documentation.

RS rswanson

Super small nit - but this line should be aligned with the one above it.

RS rswanson

Blank line between these.

brenniebrennie

This should probably render a link to the external pull request.

brenniebrennie

This is going to do up to three database queries: get the review request; get the repository; and get the …

brenniebrennie

No blank line here.

brenniebrennie

See previous comment re: number of queries.

brenniebrennie

Single quotes. Capitilization.

brenniebrennie

Use the imperitive mood ("return") and also missing "Returns"

brenniebrennie

No blank line.

brenniebrennie

Missing docstring.

brenniebrennie

Can you reformat this as: unique_together = ( (..., ...), (..., ...), ... )

brenniebrennie

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/evolutions/pull_request.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/models/review.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/evolutions/pull_request.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/models/review.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
    
    
  2. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
     'datetime' imported but unused
    
  3. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (96 > 79 characters)
    
  4. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (117 > 79 characters)
    
  5. 
      
FI
FI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/evolutions/pull_request.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/models/review.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/evolutions/pull_request.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/models/review.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/reviews/models/review_request.py
    
    
  2. reviewboard/hostingsvcs/github.py (Diff revision 2)
     
     
    Show all issues
     'datetime' imported but unused
    
  3. reviewboard/hostingsvcs/github.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  4. reviewboard/hostingsvcs/github.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (91 > 79 characters)
    
  5. 
      
brennie
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 2)
     
     
    Show all issues

    This should be non-None if len(d['parents']) > 1.

  3. 
      
FI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/evolutions/pull_request.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/models/review.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/evolutions/pull_request.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/models/review.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/reviews/models/review_request.py
    
    
  2. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
    Show all issues
     'datetime' imported but unused
    
  3. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
    Show all issues
     'ipdb' imported but unused
    
  4. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  5. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (91 > 79 characters)
    
  6. 
      
FI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/evolutions/pull_request.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/models/review.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/evolutions/pull_request.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/models/review.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/reviews/models/review_request.py
    
    
  2. reviewboard/hostingsvcs/github.py (Diff revision 4)
     
     
    Show all issues
     'DiffSet' imported but unused
    
  3. 
      
FI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/evolutions/pull_request.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/models/review.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/evolutions/pull_request.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/models/review.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/reviews/models/review_request.py
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
     
     
     
    Show all issues

    Alphabetical order.

  3. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    Missing 'Args' in docstring. Also this needs to go into deeper detail about what it actually does i.e. publishes the status to github.

  4. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    If these are constants that GitHub expects, make them constants on the class. e.g. PR_STATUS_SUCCESS etc.

  5. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    Can you outline what the status means?

  6. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    "The repository that the commit belongs to."

  7. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    Missing period.

    "The full ID of the commit for which to set the status."

  8. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
     
    Show all issues

    Paragraphs should have a blank line between them.

    "The status to set for the commit."

  9. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    When these are constants you should outline them with, e.g.

    This must be one of the following values:
    
    * :py:attr:`CONSTANT_ONE`
    * :py:attr:`CONSTANT_TWO`
    

    That will render as a list with links to the relevant constants in the documentation.

  10. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    "The description ..."

    The defaults of params show up in documentation.

  11. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    "unicode, optional"

  12. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    The defaults of params show up in documentation."The HTTP(S) URL..."

  13. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    No blank line here.

  14. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    This needs to be a constant on the class.

  15. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    Missing "args" and "returns" sections.

    This must be written in the imperitive mood, i.e., "Create or update ...."

  16. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    Return a 400 bad request and log that the user doesn't exist?

  17. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    This should probably be a constant in the siteconfig

  18. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
     
    Show all issues

    Since this can fail, we should surround this with a try/except. In the event of a failure, we should log the error and return a 400.

  19. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    .split('=', 1) will ensure we only split on the first = which is what we're going to want to do here. This will simplify the logic of the following conditional.

  20. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    We should log this.

  21. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    "Create"

  22. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    "The repository that the review request is associated with."

  23. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    dict

  24. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    django.http.HttpRequest, optional

  25. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    Regarding my comment that this should throw SCMError, this must document that fact:

    Raises:
        reviewboard.scmtools.errors.SCMError:
            Raised when ....
    
  26. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    No blank line here.

  27. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    an SCMError is probably more appropriate here.

  28. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    Ths should pass the exception up the call stack and should log it in the method that calls this.

  29. reviewboard/reviews/builtin_fields.py (Diff revision 5)
     
     
     
    Show all issues

    Blank line between these.

  30. reviewboard/reviews/builtin_fields.py (Diff revision 5)
     
     
    Show all issues

    This should probably render a link to the external pull request.

  31. reviewboard/reviews/models/base_comment.py (Diff revision 5)
     
     
     
    Show all issues

    This is going to do up to three database queries:

    1. get the review request;
    2. get the repository; and
    3. get the hosting service account.

    This will make this method very slow. To counteract this, we can just query for that hosting service account and review_request and from there get the hosting service as follows:

    review_request = review.review_request
    hosting_svc_account = HostingServiceAccount.objects.get(repository__review_requests__reviews=review)
    hosting_svc_account.service.update_pull_request(review_request)
    

    Then you can use review_request in the update call below as well.

  32. Show all issues

    No blank line here.

  33. reviewboard/reviews/models/review.py (Diff revision 5)
     
     
     
    Show all issues

    See previous comment re: number of queries.

  34. Show all issues

    Single quotes.
    Capitilization.

  35. Show all issues

    Use the imperitive mood ("return") and also missing "Returns"

  36. Show all issues

    No blank line.

  37. Show all issues

    Missing docstring.

  38. reviewboard/reviews/models/review_request.py (Diff revision 5)
     
     
     
     
     
    Show all issues

    Can you reformat this as:

    unique_together = (
        (..., ...),
        (..., ...),
        ...
    )
    
  39. 
      
RS
  1. Looks really good! Had a few musings below.

  2. reviewboard/hostingsvcs/service.py (Diff revision 5)
     
     
    Show all issues

    In comments from mentors, I've been asked to provide the fully qualified path to the class type in documentation.

  3. reviewboard/reviews/builtin_fields.py (Diff revision 5)
     
     
    Show all issues

    Super small nit - but this line should be aligned with the one above it.

  4. 
      
FI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/evolutions/pull_request.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/hostingsvcs/tests/test_github.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/models/review.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/evolutions/pull_request.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/hostingsvcs/tests/test_github.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/models/review.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/reviews/models/review_request.py
    
    
  2. reviewboard/hostingsvcs/github.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
FI
Review request changed
Change Summary:
  • Fix overrunning line
  • Removed WIP tag and added testing info
Summary:
[WIP] Multi-commit Pull Request Diff Support
Multi-commit Pull Request Diff Support
Testing Done:
  +

Added unit tests

  + - to verify the pull request hook is called, and only works with pull_request events and the right signature
  + - to verify pull requests are created and updated when a pull request is created
  + - to verify the pull request's status starts off with 'pending', is 'error' when there's open issues, and 'success' when approved

  +
  +

Manual testing

  + - Created a pull request on a repository linked to ReviewBoard through webhooks
  + - Verified the review request was created with 'pending' status
  + - Verified that opening an issue on the review request sets the 'error' status on the pull request
  + - Verified that having no open issue and adding a 'Ship it!' sets the 'success' status on the pull request
  + - Verified that pull requests with multiple commits get put into a single DiffSet and get rendered together.

Commit:
5a1cc4722171b2348beb04a158ca3af14c5ea117
cc317652e72738b87723a92d37e150b3f7f66d98
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/evolutions/pull_request.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/hostingsvcs/tests/test_github.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/models/review.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/evolutions/pull_request.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/hostingsvcs/tests/test_github.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/models/review.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/reviews/models/review_request.py
    
    
  2.