Multi-commit Pull Request Diff Support

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

finaiized
Review Board
dvcs
8463
cc31765...
reviewboard, students

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)
     
     
     'datetime' imported but unused
    
  3. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (96 > 79 characters)
    
  4. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    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)
     
     
     'datetime' imported but unused
    
  3. reviewboard/hostingsvcs/github.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  4. reviewboard/hostingsvcs/github.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (91 > 79 characters)
    
  5. 
      
brennie
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 2)
     
     

    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)
     
     
     'datetime' imported but unused
    
  3. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
     'ipdb' imported but unused
    
  4. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  5. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
    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)
     
     
     '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)
     
     
     
     
     

    Alphabetical order.

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

    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)
     
     

    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)
     
     

    Can you outline what the status means?

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

    "The repository that the commit belongs to."

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

    Missing period.

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

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

    Paragraphs should have a blank line between them.

    "The status to set for the commit."

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

    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)
     
     

    "The description ..."

    The defaults of params show up in documentation.

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

    "unicode, optional"

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

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

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

    No blank line here.

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

    This needs to be a constant on the class.

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

    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)
     
     

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

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

    This should probably be a constant in the siteconfig

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

    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)
     
     

    .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)
     
     

    We should log this.

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

    "Create"

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

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

  23. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
  24. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     

    django.http.HttpRequest, optional

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

    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)
     
     

    No blank line here.

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

    an SCMError is probably more appropriate here.

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

    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)
     
     
     

    Blank line between these.

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

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

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

    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. No blank line here.

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

    See previous comment re: number of queries.

  34. Single quotes.
    Capitilization.

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

  36. No blank line.

  37. Missing docstring.

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

    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)
     
     

    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)
     
     

    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)
     
     
    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

Diff:

Revision 7 (+659 -23)

Show changes

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. 
      
Loading...