Summary

Prevent review requests created with history from adding regular diffs

Review Request #9852 — Created April 3, 2018 and submitted Aug. 27, 2018, 5:04 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

~~9757~~, ~~9899~~

Blocks

9883

Commit

0bd5e31...

Reviewers

Groups

reviewboard

People

Description

We now have a notion of a review request that is created with history
support. A review request created with this flag cannot have a regular
diff attached to it. Instead, an empty DiffSet must be created and
commits attached to that. (Support for which is coming in another
patch.)

This option is now specified at review request creation time and cannot
be changed once the review request is created (i.e., after save() is
called and the primary key has been assigned). It can be specified via
a new field on the review request resources which defaults to the old
(i.e., no history) behaviour.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
Needs to be the full path.	chipx86	April 18, 2018, 3:28 p.m.
Missing ugettext().	chipx86	April 18, 2018, 3:37 p.m.
Is this going to be seen by end users ever? If so, we shouldn't be talking DiffSets and DiffCommits, and …	chipx86	April 18, 2018, 3:32 p.m.
Needs to be the full module path.	chipx86	April 18, 2018, 3:33 p.m.
This will be just a tad more clear with "... cannot both be set to True".	chipx86	April 18, 2018, 3:33 p.m.
Missing ending paren.	chipx86	April 18, 2018, 3:33 p.m.
This would probably be better as: return (self.extra_data and self.extra_data.get(..., False))	chipx86	April 18, 2018, 3:34 p.m.
Can you also have this check the resulting errors?	chipx86	April 18, 2018, 3:38 p.m.
This should check the message.	chipx86	April 18, 2018, 3:42 p.m.
Can you group those create_ flags together?	chipx86	April 18, 2018, 3:43 p.m.
Spaces around *.	chipx86	April 18, 2018, 3:43 p.m.
Should this be two tests?	chipx86	April 18, 2018, 3:44 p.m.
This should check the error.	chipx86	April 18, 2018, 3:45 p.m.
Here, too.	chipx86	April 18, 2018, 3:45 p.m.
Should have ~ at the beginning. These are annoyingly long, and means this must be kept up-to-date. Maybe just have …	chipx86	April 18, 2018, 3:47 p.m.
We can probably just talk about publishing rather than have a long line for the function reference.	chipx86	April 18, 2018, 3:47 p.m.
Needs to be the full reference.	chipx86	April 18, 2018, 4:28 p.m.
Can you have this say explicitly that "This field cannot be set if create_with_history is set" ?	chipx86	April 18, 2018, 4:29 p.m.
And similar here.	chipx86	April 18, 2018, 4:29 p.m.
Can you put the create_ arguments together?	chipx86	April 18, 2018, 4:30 p.m.
Can you have this check the "reason" message?	chipx86	April 18, 2018, 4:36 p.m.
Two blank lines.	chipx86	April 18, 2018, 4:31 p.m.
Can you have this check the "reason" message?	chipx86	April 18, 2018, 4:36 p.m.
This can fit on one line.	chipx86	April 18, 2018, 4:32 p.m.
This doesn't need to be indented. item_rsp['extra_data'] can fit right after the assertEqual(.	chipx86	April 18, 2018, 4:32 p.m.
F401 'django.http.HttpRequest' imported but unused	reviewbot	April 19, 2018, 4:30 p.m.
E231 missing whitespace after ','	reviewbot	April 19, 2018, 4:30 p.m.
F811 redefinition of unused 'test_get_dvcs_feature_disabled' from line 1569	reviewbot	April 20, 2018, 5:02 p.m.
Should be ValueError	david	May 1, 2018, 5:04 p.m.
typo: mulitple	david	May 1, 2018, 5:04 p.m.
typo: mulitple	david	May 1, 2018, 5:04 p.m.
typo: mulitple	david	May 1, 2018, 5:04 p.m.
Missing localization.	chipx86	July 4, 2018, 3:55 p.m.
"STATUSES"	chipx86	July 4, 2018, 3:56 p.m.
Extra blank line.	chipx86	July 4, 2018, 3:56 p.m.
"associated"	chipx86	July 10, 2018, 8:05 p.m.
This needs to explicitly use ugettext().	chipx86	July 10, 2018, 8:06 p.m.
Missing a trailing comma. (So is the one above.)	chipx86	July 10, 2018, 8:07 p.m.
Should be assertFalse	chipx86	July 10, 2018, 8:08 p.m.
Typo: "creatd" -> "created"	chipx86	Aug. 17, 2018, 6:36 p.m.
This fits on one line.	chipx86	Aug. 17, 2018, 6:37 p.m.
Same here.	chipx86	Aug. 17, 2018, 6:37 p.m.
This should say that one based on the local_site_name attr will be provided if not specified here.	chipx86	Aug. 17, 2018, 6:38 p.m.
We should capitalize the field names.	chipx86	Aug. 17, 2018, 6:38 p.m.
This can be split across lines.	chipx86	Aug. 17, 2018, 6:39 p.m.
This should use :py:attr.	chipx86	Aug. 27, 2018, 4:53 p.m.

flake8 timed out.

JSHint timed out.

reviewboard/reviews/forms.py (Diff revision 1)
Show all issues
```
Needs to be the full path.
```
reviewboard/reviews/forms.py (Diff revision 1)
Show all issues
```
Missing ugettext().
```

reviewboard/reviews/forms.py (Diff revision 1)

Show all issues

Is this going to be seen by end users ever? If so, we shouldn't be talking DiffSets and DiffCommits, and need to rethink what we're telling users.

reviewboard/reviews/managers.py (Diff revision 1)
Show all issues
```
Needs to be the full module path.
```

reviewboard/reviews/managers.py (Diff revision 1)

Show all issues

This will be just a tad more clear with "... cannot both be set to True".

reviewboard/reviews/models/review_request.py (Diff revision 1)
Show all issues
```
Missing ending paren.
```

reviewboard/reviews/models/review_request.py (Diff revision 1)

Show all issues

This would probably be better as:

return (self.extra_data and
        self.extra_data.get(..., False))

reviewboard/reviews/tests/test_forms.py (Diff revision 1)
Show all issues
```
Can you also have this check the resulting errors?
```
reviewboard/reviews/tests/test_review_request.py (Diff revision 1)
Show all issues
```
This should check the message.
```
reviewboard/reviews/tests/test_review_request.py (Diff revision 1)
Show all issues
```
Can you group those create_ flags together?
```
reviewboard/reviews/tests/test_review_request.py (Diff revision 1)
Show all issues
```
Spaces around *.
```

reviewboard/reviews/tests/test_review_request.py (Diff revision 1)

Show all issues

Should this be two tests?

reviewboard/reviews/tests/test_review_request.py (Diff revision 1)
Show all issues
```
This should check the error.
```
reviewboard/reviews/tests/test_review_request.py (Diff revision 1)
Show all issues
```
Here, too.
```

reviewboard/testing/testcase.py (Diff revision 1)

Show all issues

Should have ~ at the beginning.
These are annoyingly long, and means this must be kept up-to-date. Maybe just have this refer to the list of status on the class (with a reference to it).
(Also, these would be :py:attr, not :py:data).

reviewboard/testing/testcase.py (Diff revision 1)

Show all issues

We can probably just talk about publishing rather than have a long line for the function reference.

reviewboard/testing/testcase.py (Diff revision 1)
Show all issues
```
Needs to be the full reference.
```

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

Show all issues

Can you have this say explicitly that "This field cannot be set if create_with_history is set" ?

reviewboard/webapi/resources/review_request.py (Diff revision 1)
Show all issues
```
And similar here.
```
reviewboard/webapi/resources/review_request.py (Diff revision 1)
Show all issues
```
Can you put the create_ arguments together?
```
reviewboard/webapi/tests/test_diff.py (Diff revision 1)
Show all issues
```
Can you have this check the "reason" message?
```
reviewboard/webapi/tests/test_diff.py (Diff revision 1)
Show all issues
```
Two blank lines.
```
reviewboard/webapi/tests/test_draft_diff.py (Diff revision 1)
Show all issues
```
Can you have this check the "reason" message?
```
reviewboard/webapi/tests/test_review_request.py (Diff revision 1)
Show all issues
```
This can fit on one line.
```

reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 1)

Show all issues

This doesn't need to be indented. item_rsp['extra_data'] can fit right after the assertEqual(.

Change Summary:

Addressed feedback.

Commit:

-	7ad09adc5caecf78c422f0357d746f3324e66bbc
+	ea37078bc487c3b48d91a06d821bea6491279336

Diff:

Revision 2 (+406 -22)

Prevent review requests created with history from adding regular diffs

Information

Reviewers

Change Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Depends On:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Status: Closed (submitted)

Change Summary: