Updating changenum with PUT call to ReviewRequest

Review Request #6774 — Created Jan. 15, 2015 and submitted

Information

Review Board
master
7e4f538...

Reviewers

Making a PUT request to the webapi for ReviewRequest such as

curl http://reviewboard.example.com/api/review-requests/8/ -X PUT -d changenum=1234

for a perforce review may not save the commit_id properly. This lead to two modifications made to the PUT command regarding changenum. Conversion of changenum to six.text_type in the event when changenum is entered as an integer for variable type conservation. The other is to change the save logic, updating the fields that have been changed.

Two Test cases were made

  • Tests the initialization of ReviewRequest with changenum, and checks if the instance has the proper changenum and an empty
    commit_id.

  • Tests the PUT call with changenum, and ensures the payload and the ReviewRequest saved have their changenum and commit_id changed to match up. This includes the implementation for TestTool.get_changeset was made, for scmtools that support pending changesets.

Description From Last Updated

Col: 17 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 31 E127 continuation line over-indented for visual indent

reviewbotreviewbot

What happens if you have both extra_fields and the changenum was updated? I'm also not a big fan of adding …

daviddavid

Col: 25 W292 no newline at end of file

reviewbotreviewbot

Col: 45 E502 the backslash is redundant between brackets

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/testing/scmtool.py
        reviewboard/reviews/tests.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/testing/scmtool.py
        reviewboard/reviews/tests.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
  2. reviewboard/testing/scmtool.py (Diff revision 1)
     
     
    Show all issues
    Col: 17
     E127 continuation line over-indented for visual indent
    
  3. Show all issues
    Col: 31
     E127 continuation line over-indented for visual indent
    
  4. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/testing/scmtool.py
        reviewboard/reviews/tests.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/testing/scmtool.py
        reviewboard/reviews/tests.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/webapi/resources/review_request.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues

    What happens if you have both extra_fields and the changenum was updated?

    I'm also not a big fan of adding this new state variable.

    How about we define an changed_fields list, in which we can put 'extra_data' and 'commit_id'. Then we'd replace this with something like:

    ... (define list and add in commit_id above)
    
    if extra_fields:
        self.import_extra_data(review_request, review_request.extra_data,
                               extra_fields)
        changed_fields.append('extra_data')
    
    if changed_fields:
        review_request.save(update_fields=changed_fields)
    
  3. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/testing/scmtool.py
        reviewboard/reviews/tests.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/testing/scmtool.py
        reviewboard/reviews/tests.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
  2. 
      
david
  1. <p>The code looks good now.</p>
    <p>Can you make a change to your description to talk about the bugfix itself first, and the details of the tests second?</p>

  2. 
      
VT
david
  1. I just applied this patch but it broke several unit tests. I think the issue is your changes to TestTool. It might make sense to extract those into a subclass and use it only for your newly-added tests.

  2. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/testing/scmtool.py
        reviewboard/reviews/tests.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/testing/scmtool.py
        reviewboard/reviews/tests.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
  2. reviewboard/testing/scmtool.py (Diff revision 4)
     
     
    Show all issues
    Col: 25
     W292 no newline at end of file
    
  3. reviewboard/testing/testcase.py (Diff revision 4)
     
     
    Show all issues
    Col: 45
     E502 the backslash is redundant between brackets
    
  4. 
      
VT
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/testing/scmtool.py
        reviewboard/reviews/tests.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/testing/scmtool.py
        reviewboard/reviews/tests.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
VT
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (6e485ac)