Auto-populating branch for new post-commit review requests

Review Request #7659 — Created Sept. 25, 2015 and submitted

Information

Review Board
release-2.5.x

Reviewers

When creating a new post-commit review request, the branch name was left blank rather than being pre-populated. Since we have the data available, this should be populated.
Added branch info to the request url when creating the new ReviewRequest, webAPI takes care of the rest.

Corrected API breaking issue. When no branch was provided in API request, None would be attempt to be saved to the database but branch had a Not Null constraint.
Added additional tests to cover this case.

Ran all reviewboard tests.
Added new API tests for post with and without branch info.

Description From Last Updated

I would prefer if we just provided the branch: this.model.collection.options.branch key-value-pair in the createFromCommit call. Its cleaner. Also, the only …

brenniebrennie

Context is the this object that the callbacks will be bound to. You don't really want to be modifying it.

brenniebrennie

'pdb' imported but unused

reviewbotreviewbot

'pdb' imported but unused

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

redefinition of unused 'pdb' from line 4

reviewbotreviewbot

Col: 23 E231 missing whitespace after ';'

reviewbotreviewbot

Col: 23 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

Col: 39 E703 statement ends with a semicolon

reviewbotreviewbot

Col: 13 E122 continuation line missing indentation or outdented

reviewbotreviewbot

Col: 13 E122 continuation line missing indentation or outdented

reviewbotreviewbot

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

reviewbotreviewbot

Col: 28 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

Do you mean as a keyword parameter for the create function? Because that's already the case. Is this suggestion so …

AH ahache

You can add branch=None as a keyword argument to create.

brenniebrennie

Col: 34 E225 missing whitespace around operator

reviewbotreviewbot

Col: 41 E703 statement ends with a semicolon

reviewbotreviewbot

You should be able to just include branch=branch, in the self.model() call above.

daviddavid

To keep this consistent with the repository, please use branch = this.model.get('branch') and then pass in branch.id below. Although, perhaps …

daviddavid

Remove this commented out code.

brenniebrennie

No blank line here.

brenniebrennie

Can you format this as: review_request = ReviewRequest.objects.get( pkg=rsp['review_request']['id'])

brenniebrennie

Can you put this on the previous line?

brenniebrennie

Can you format this as: review_request = ReviewRequest.objects.get( pkg=rsp['review_request']['id'])

brenniebrennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/resources/repository_commits.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/repositoryCommitModel.js
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
        reviewboard/static/rb/js/resources/collections/repositoryCommitsCollection.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
        reviewboard/static/rb/js/newReviewRequest/views/commitView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/resources/repository_commits.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/repositoryCommitModel.js
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
        reviewboard/static/rb/js/resources/collections/repositoryCommitsCollection.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
        reviewboard/static/rb/js/newReviewRequest/views/commitView.js
    
    
  2.  'pdb' imported but unused
    
  3.  'pdb' imported but unused
    
  4. Col: 9
     E265 block comment should start with '# '
    
  5.  redefinition of unused 'pdb' from line 4
    
  6. Col: 23
     E231 missing whitespace after ';'
    
  7. Col: 23
     E702 multiple statements on one line (semicolon)
    
  8. Col: 39
     E703 statement ends with a semicolon
    
  9. 
      
brennie
  1. 
      
  2. reviewboard/static/rb/js/newReviewRequest/views/commitView.js (Diff revision 1)
     
     
     
     
     
     
     
     

    I would prefer if we just provided the branch: this.model.collection.options.branch key-value-pair in the createFromCommit call. Its cleaner.

    Also, the only time the branch will be set is when creating it via the post-commit UI. In that case, it makes sense to set() the data in reviewRequestModel.js:createFromCommit.

    1. Seems that we don't have the collection anymore at that point "Uncaught TypeError: Cannot read property 'collection' of undefined".
      And if I try doing it in _onCreateReviewRequest in postCommitView.js I get the error on options instead "Uncaught TypeError: Cannot read property 'options' of undefined" (even though collection itself is undefined there".

  3. Context is the this object that the callbacks will be bound to. You don't really want to be modifying it.

  4. 
      
AH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/repositoryCommitModel.js
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
        reviewboard/static/rb/js/resources/collections/repositoryCommitsCollection.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
        reviewboard/static/rb/js/newReviewRequest/views/commitView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/repositoryCommitModel.js
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
        reviewboard/static/rb/js/resources/collections/repositoryCommitsCollection.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
        reviewboard/static/rb/js/newReviewRequest/views/commitView.js
    
    
  2. Col: 13
     E122 continuation line missing indentation or outdented
    
  3. Col: 13
     E122 continuation line missing indentation or outdented
    
  4. 
      
AH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/repositoryCommitModel.js
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
        reviewboard/static/rb/js/resources/collections/repositoryCommitsCollection.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
        reviewboard/static/rb/js/newReviewRequest/views/commitView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/repositoryCommitModel.js
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
        reviewboard/static/rb/js/resources/collections/repositoryCommitsCollection.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
        reviewboard/static/rb/js/newReviewRequest/views/commitView.js
    
    
  2. Col: 80
     E501 line too long (81 > 79 characters)
    
  3. Col: 28
     E131 continuation line unaligned for hanging indent
    
  4. 
      
AH
AH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/repositoryCommitModel.js
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
        reviewboard/static/rb/js/resources/collections/repositoryCommitsCollection.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
        reviewboard/static/rb/js/newReviewRequest/views/commitView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/repositoryCommitModel.js
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
        reviewboard/static/rb/js/resources/collections/repositoryCommitsCollection.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
        reviewboard/static/rb/js/newReviewRequest/views/commitView.js
    
    
  2. 
      
AH
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/commitView.js
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/commitView.js
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
  2. 
      
AH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
  2. 
      
brennie
  1. Can you add the bug number to the bugs field?

  2. You can add branch=None as a keyword argument to create.

    1. Do you mean as a keyword parameter for the create function? Because that's already the case. Is this suggestion so that kwargs.get('branch', None) becomes kwargs.get('branch') ? If so, I had it that way at first but then changed it to have the default None value incase an API call was made to create the post-commit RR without passing branch information.

    2. If you have def create(..., branch=None, ...) then you don't need to do branch = kwargs.get(branch).

      It currently isnt the case beause the signaure is:

          def create(self, request, repository=None, submit_as=None, changenum=None,
                     commit_id=None, local_site_name=None,
                     create_from_commit_id=False, extra_fields={}, *args, **kwargs):
      

      You need to add branch=None to that before extra_fields={}

    3. Ohh I see, I was thinking of the create function in managers.py for some reason. Will do!
  3. 
      
AH
AH
AH
  1. 
      
  2. Do you mean as a keyword parameter for the create function? Because that's already the case. Is this suggestion so that kwargs.get('branch', None) becomes kwargs.get('branch') ? If so, I had it that way at first but then changed it to have the default None value incase an API call was made to create the post-commit RR without passing branch information.

  3. 
      
AH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
  2. 
      
brennie
  1. The description field doesn't need to describe exactly what changed in the code -- thats what the diff is for :)

    Instead, it should outline the problem that this patch fixes and how it is solved (in a very high-level way).

    Otherwise this patch looks great! Clean that up and we'll get it landed :)

    1. Awesome! I was unsure if any new tests would be needed for this change. After running the entire test suite, a few tests were failing based on the fact that the branch field was defaulted to None when no branch info was found, and that RR's have a Not Null constraint for the branch. I moved a line or two around to correct this.

  2. 
      
AH
AH
AH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
  2. reviewboard/reviews/managers.py (Diff revision 8)
     
     
    Col: 34
     E225 missing whitespace around operator
    
  3. reviewboard/reviews/managers.py (Diff revision 8)
     
     
    Col: 41
     E703 statement ends with a semicolon
    
  4. 
      
AH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/reviews/managers.py (Diff revision 9)
     
     
     

    You should be able to just include branch=branch, in the self.model() call above.

  3. To keep this consistent with the repository, please use branch = this.model.get('branch') and then pass in branch.id below.

    Although, perhaps we should be using branch.name instead?

  4. 
      
AH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
  2. 
      
david
  1. This looks good now, but I'm going to hold off on pushing it because we're on the cusp of releasing 2.5. I'll get it in for 2.5.1

  2. 
      
AH
AH
chipx86
  1. Unfortunately, this ended up breaking our API, so we're having to revert it and re-release.

    Can you update this to inclue a set of unit tests that test this addition?

    The specific breakage is documented on https://hellosplat.com/s/beanbag/tickets/4009/

    1. Oh no :(
      I'll get right on that!

    2. Out of curiosity, is there a reason we have a Not Null constraint for branch if branch isn't a required field on a review request and we just save it as an empty string when it isn't provided?

  2. 
      
AH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
  2. 
      
AH
brennie
  1. 
      
  2. reviewboard/webapi/resources/review_request.py (Diff revision 11)
     
     
     
     

    Remove this commented out code.

  3. No blank line here.

  4. Can you format this as:

            review_request = ReviewRequest.objects.get(
                pkg=rsp['review_request']['id'])
    
  5. Can you put this on the previous line?

  6. Can you format this as:

            review_request = ReviewRequest.objects.get(
                pkg=rsp['review_request']['id'])
    
  7. 
      
AH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    
    
  2. 
      
AH
AH
david
  1. Just a note, this was backed out due to bug 4009.

  2. 
      
AH
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to ucosp/dkus/dnd-inline-images (c6d6295)
Loading...