Summary

Updating changenum with PUT call to ReviewRequest

Review Request #6774 — Created Jan. 15, 2015 and submitted Jan. 17, 2015, 12:10 p.m.

Information

Owner

VTL-Developer

Repository

Review Board

Branch

master

Bugs

3685

Depends On

Commit

7e4f538...

Reviewers

Groups

reviewboard, students

People

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
Col: 17 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2015, 4:36 p.m.
Col: 31 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2015, 4:36 p.m.
What happens if you have both extra_fields and the changenum was updated? I'm also not a big fan of adding …	david	Jan. 17, 2015, 9:20 a.m.
Col: 25 W292 no newline at end of file	reviewbot	Jan. 17, 2015, 11:36 a.m.
Col: 45 E502 the backslash is redundant between brackets	reviewbot	Jan. 17, 2015, 11:36 a.m.

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

reviewboard/testing/scmtool.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 17
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/resources/review_request.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 31
 E127 continuation line over-indented for visual indent
```

Change Summary:

Uploaded changes to address indentation issue.

Commit:

6e82df8bf4cc7f2727aecd925b8f287707c2093f

14b6718b36d57df0951c43f20862cbbb4ce6909d

Diff:

Revision 2 (+56 -3)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/testing/scmtool.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/tests/test_review_request.py

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

reviewboard/webapi/resources/review_request.py (Diff revision 2)

The issue has been resolved. 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)

Change Summary:

Made adjustments as suggested.

Commit:

14b6718b36d57df0951c43f20862cbbb4ce6909d

65641310121e478258488d5bd7ba9a80b295c459

Diff:

Revision 3 (+58 -4)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/testing/scmtool.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/tests/test_review_request.py

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

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

Change Summary:: Updated the description, and moved the test cases to the "Testing Done" section.
Description:: ~
Two Test cases were made
~
Making a PUT request to the webapi for ReviewRequest such as

~

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

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

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.

~
This lead to two modifications were made to the PUT 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 save the ReviewRequest in the event that it is still a pending change.
~
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.
Testing Done:: ~
The tests cases.
~
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.

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.

Change Summary:

Updated to separate the TestTool for SCM Tools to be two different separate classes, the base one (the normal one used before implementation) and a new one, TestToolSupportsPendingChangeSets, an SCM Test tool that supports pending change sets.

Commit:

65641310121e478258488d5bd7ba9a80b295c459

1e49630308ec5166aab4dfb835a2e3bde2fab296

Diff:

Revision 4 (+63 -5)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/testing/scmtool.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/tests/test_review_request.py

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

reviewboard/testing/scmtool.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 25
 W292 no newline at end of file
```
reviewboard/testing/testcase.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 45
 E502 the backslash is redundant between brackets
```

Change Summary:

Adjusted for ReviewBot styling

Commit:

1e49630308ec5166aab4dfb835a2e3bde2fab296

7e4f5380e1e76393e8d0843c4fb2ef1df0a02450

Diff:

Revision 5 (+63 -5)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/testing/scmtool.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/tests/test_review_request.py

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

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.0.x (6e485ac)

~		The tests cases.
	~	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.