Summary

Fix diff attachment and default reviewers for post-commit review.

Review Request #8539 — Created Nov. 15, 2016 and submitted Nov. 17, 2016, 6:14 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

Commit

b4dccff...

Reviewers

Groups

reviewboard

People

Description

We had a very, very old bug with post-commit review where the diffs from
the posted commit would be attached to the Review Request's diffset
history, rather than the draft. This made the diff accessible to
superusers who had access to the review request, which wasn't a big
deal, but more importantly it meant the diff wasn't accessible via the
draft API and default reviewers would not apply.

This change fixes this by redoing some of that logic. Formerly, the code
for updating was assuming it would be called on either the draft or
review request, but in practice now it's only ever called on the draft,
so that has moved into ReviewRequestDraft and has been updated to
associate there instead of the review request history.

That also required some new code that figured out a suitable revision
for the diffset. Since that's already been done in two other places, the
logic has been centralized (and optimized to reduce the data returned
from an SQL query).

Some unit tests were added and others moved around to test against the
draft instead of review request.

Testing Done

Unit tests pass.

Tested post-commit review with default reviewers. It worked.

Issues

Description	From	Last Updated
'DiffSet' imported but unused	reviewbot	Nov. 15, 2016, 2:30 p.m.
'InvalidChangeNumberError' imported but unused	reviewbot	Nov. 15, 2016, 2:31 p.m.
undefined name 'InvalidChangeNumberError'	reviewbot	Nov. 15, 2016, 2:31 p.m.
undefined name 'InvalidChangeNumberError'	reviewbot	Nov. 15, 2016, 2:31 p.m.
Move this into the try block?	david	Nov. 17, 2016, 2:36 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/tests/test_review_request_manager.py
    reviewboard/scmtools/models.py
    reviewboard/reviews/tests/test_review_request_draft.py
    reviewboard/diffviewer/models.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/diffviewer/tests.py
    reviewboard/reviews/forms.py
    reviewboard/reviews/managers.py
    reviewboard/reviews/models/base_review_request_details.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/tests/test_review_request_manager.py
    reviewboard/scmtools/models.py
    reviewboard/reviews/tests/test_review_request_draft.py
    reviewboard/diffviewer/models.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/diffviewer/tests.py
    reviewboard/reviews/forms.py
    reviewboard/reviews/managers.py
    reviewboard/reviews/models/base_review_request_details.py

reviewboard/reviews/models/base_review_request_details.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'DiffSet' imported but unused
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'InvalidChangeNumberError' imported but unused
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 undefined name 'InvalidChangeNumberError'
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 undefined name 'InvalidChangeNumberError'
```

Change Summary:

Fixed missing/unused imports.

Commit:

-	9ce7ad9ab5fd79bad0966dec169af1e9435094b6
+	992e08a2471ca7581e59618565ea3371051b0265

Diff:

Revision 2 (+387 -244)

Show changes

	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/tests/test_review_request_manager.py
    reviewboard/scmtools/models.py
    reviewboard/reviews/tests/test_review_request_draft.py
    reviewboard/diffviewer/models.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/diffviewer/tests.py
    reviewboard/reviews/forms.py
    reviewboard/reviews/managers.py
    reviewboard/reviews/models/base_review_request_details.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/tests/test_review_request_manager.py
    reviewboard/scmtools/models.py
    reviewboard/reviews/tests/test_review_request_draft.py
    reviewboard/diffviewer/models.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/diffviewer/tests.py
    reviewboard/reviews/forms.py
    reviewboard/reviews/managers.py
    reviewboard/reviews/models/base_review_request_details.py

Ship it!

reviewboard/diffviewer/models.py (Diff revision 2)

The issue has been resolved. Show all issues

Move this into the try block?

chipx86 Nov. 15, 2016, 6:44 p.m.

No need. It's not querying anything at that point. It's just an attribute access for the relation. The except wouldn't even cover it.

david Nov. 16, 2016, 1:42 p.m.

I understand that, but the variable assignment only exists to make the line that is within the try fit in our text width. Given that, I think we should keep it together with where it's used.

chipx86 Nov. 17, 2016, 2:36 p.m.

Turns out it fits fine at this indentation level, so I'm just merging the statements. This was nested further in the original code.

Change Summary:

Condensed a couple of statements.

Commit:

-	992e08a2471ca7581e59618565ea3371051b0265
+	b4dccffa8d331d96dbd873c95599e9a458daeed8

Diff:

Revision 3 (+385 -244)

Show changes

	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/tests/test_review_request_manager.py
    reviewboard/scmtools/models.py
    reviewboard/reviews/tests/test_review_request_draft.py
    reviewboard/diffviewer/models.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/diffviewer/tests.py
    reviewboard/reviews/forms.py
    reviewboard/reviews/managers.py
    reviewboard/reviews/models/base_review_request_details.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/tests/test_review_request_manager.py
    reviewboard/scmtools/models.py
    reviewboard/reviews/tests/test_review_request_draft.py
    reviewboard/diffviewer/models.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/diffviewer/tests.py
    reviewboard/reviews/forms.py
    reviewboard/reviews/managers.py
    reviewboard/reviews/models/base_review_request_details.py

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (3fa69a0)