Summary

Require validation for DiffCommits when posting

Review Request #9920 — Created May 9, 2018 and submitted Sept. 2, 2018, 12:04 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

~~10119~~, ~~9905~~

Blocks

9921

Commit

1e5bd7d...

Reviewers

Groups

reviewboard

People

Description

We now require that diffs for an entire series of commits be validated
before posting to the DraftDiffCommitResource. Validation is performed
by making a series of POST requests to the
ValidateDiffCommitResource, each of which returns validation data to
be included in the next request. The validation data from each request
to the validation resource must then be used to POST to the
DraftDiffCommitResource.

This ensures that we do not upload broken diffs and also allows us to
upload more complex commit series. Previously, only linear commit series
where each commit modified different files were capable of being
uploaded. Now all linear commit series can be uploaded.

Testing Done

Posted /r/9609/ through to this commit to a single review request.
Ran unit tests.

Issues

Description	From	Last Updated
Can you rename the file to "commit_utils.py"?	chipx86	July 10, 2018, 12:03 p.m.
F401 'djblets.webapi.errors.INVALID_FORM_DATA' imported but unused	reviewbot	May 9, 2018, 10:47 p.m.
This is way too generic a name. I'd prefer get_file_exists_for_validation. Not sure this file is the right place, actually. This …	chipx86	July 10, 2018, 11:51 a.m.
This can wrap.	david	July 3, 2018, 4:12 p.m.
You can now split this across lines at a .	chipx86	July 10, 2018, 12:03 p.m.
bool	chipx86	July 10, 2018, 12:03 p.m.
Did you mean to unindent the line? Doesn't seem like it would particularly matter in this change.	chipx86	July 10, 2018, 11:54 a.m.
Can you import specifically waht you need from the module, instead of the module itself?	chipx86	July 10, 2018, 8:14 p.m.
I imagine this should be a HiddenInput? We don't want users mucking about in it if we display this anywhere.	chipx86	July 10, 2018, 12:10 p.m.
"Validation metadata"	chipx86	July 10, 2018, 12:10 p.m.
The text should specifically be self.fields['validation_info'].error_messages['required']. No need for ugettext.	chipx86	July 10, 2018, 12:12 p.m.
Shouldn't include literals in summaries.	chipx86	July 10, 2018, 12:12 p.m.
Extra period on this line.	david	July 3, 2018, 4:12 p.m.
Too many periods.	chipx86	July 10, 2018, 12:12 p.m.
"exceptions"	chipx86	July 10, 2018, 12:12 p.m.
Maybe better as: if not validation_info: return {} # .. rest of the method	chipx86	July 10, 2018, 12:13 p.m.
b64decode can also raise TypeError.	chipx86	July 10, 2018, 12:13 p.m.
The return can be inline.	chipx86	July 10, 2018, 12:13 p.m.
Maybe "A form for validating DiffCommits."	chipx86	July 10, 2018, 12:13 p.m.
"Parent diff"	chipx86	July 10, 2018, 12:14 p.m.
"Validation metadata"	chipx86	July 10, 2018, 12:14 p.m.
"Base commit ID"	chipx86	July 10, 2018, 12:14 p.m.
No literals in summaries.	chipx86	July 10, 2018, 12:14 p.m.
Too many periods.	chipx86	July 10, 2018, 12:14 p.m.
Same comments as above. Seems we have some stuff we could easily pull out into a base class?	chipx86	July 10, 2018, 12:18 p.m.
This seems like code we have elsewhere. I'd like to deduplicate this if possible.	chipx86	July 10, 2018, 12:24 p.m.
We should pull out the specific functions we need.	chipx86	July 10, 2018, 8:14 p.m.
This can be wrapped.	david	July 3, 2018, 4:13 p.m.
This can be broken into multiple lines now at any .	chipx86	July 10, 2018, 12:24 p.m.
No literals in summaries.	chipx86	July 10, 2018, 12:25 p.m.
Mixing import groups here.	chipx86	July 10, 2018, 12:27 p.m.
Shouldn't DiffCommit be handling this?	chipx86	July 10, 2018, 12:11 p.m.
Ordering issues.	chipx86	July 10, 2018, 12:27 p.m.
The DIFF_* should be in alphabetical order.	chipx86	July 10, 2018, 12:27 p.m.
E501 line too long (90 > 79 characters)	reviewbot	July 11, 2018, 12:38 p.m.
E501 line too long (84 > 79 characters)	reviewbot	July 11, 2018, 12:38 p.m.
E126 continuation line over-indented for hanging indent	reviewbot	July 24, 2018, 5:42 p.m.
Can we include the error details in this? That'd help with debugging.	chipx86	July 31, 2018, 2:27 p.m.
Trailing comma.	chipx86	July 31, 2018, 2:27 p.m.
Can we use keyword arguments for all calls to this, here and everywhere else in the change? It's a complex …	chipx86	July 31, 2018, 2:31 p.m.
This will create a tuple of strings.	chipx86	July 31, 2018, 2:32 p.m.
This creates a tuple.	chipx86	July 31, 2018, 2:32 p.m.
This too.	chipx86	July 31, 2018, 2:32 p.m.
Can you use references throughout the docstrings whenever you're referencing another resource? They're in the form of webapi2.0-<blah>-resource or webapi2.0-<blah>-list-resource.	chipx86	Aug. 13, 2018, 4:17 p.m.
It doesn't really return "OK." I feel that can be misleading. We probably can remove the whole "either OK or …	chipx86	Aug. 13, 2018, 4:16 p.m.
Double backticks for validation_info. Also, "field" and not "parameter."	chipx86	Aug. 13, 2018, 4:17 p.m.
Can you switch to a named logger for the module?	chipx86	Aug. 13, 2018, 4:18 p.m.
Missing trailing comma.	chipx86	Aug. 13, 2018, 4:18 p.m.
F841 local variable 'validation_info' is assigned to but never used	reviewbot	Aug. 13, 2018, 4:30 p.m.
F821 undefined name 'on_info'	reviewbot	Aug. 13, 2018, 4:30 p.m.
E501 line too long (120 > 79 characters)	reviewbot	Aug. 13, 2018, 4:30 p.m.
F841 local variable 'validation_info' is assigned to but never used	reviewbot	Aug. 13, 2018, 4:30 p.m.
F821 undefined name 'on_info'	reviewbot	Aug. 13, 2018, 4:30 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Aug. 16, 2018, 8:14 p.m.
E501 line too long (91 > 79 characters)	reviewbot	Aug. 16, 2018, 8:14 p.m.
W391 blank line at end of file	reviewbot	Aug. 16, 2018, 8:14 p.m.
F841 local variable 'parent_diff' is assigned to but never used	reviewbot	Aug. 17, 2018, 2:57 p.m.
Newlines in exceptions are generally not going to be handled well, either in HTML or in logs. Maybe: Could not …	chipx86	Aug. 17, 2018, 7:05 p.m.
Missing trailing comma.	chipx86	Aug. 17, 2018, 7:06 p.m.
You can consolidate these.	chipx86	Aug. 17, 2018, 7:13 p.m.
Missing a trailing period.	chipx86	Aug. 17, 2018, 7:07 p.m.
Name should be GetFileExistsInHistoryTests.	chipx86	Aug. 17, 2018, 7:09 p.m.
For here and any others, can we use called_with() and check the arguments?	chipx86	Aug. 17, 2018, 7:08 p.m.
Extra blank line here.	chipx86	Aug. 17, 2018, 7:09 p.m.
Missing a period.	chipx86	Aug. 17, 2018, 7:09 p.m.
Extra blank line.	chipx86	Aug. 17, 2018, 7:09 p.m.
You can align these starting on the initial call. There should be plenty of room.	chipx86	Aug. 17, 2018, 7:09 p.m.
Can you include the repository ID? That'll help with RBCommons.	chipx86	Aug. 17, 2018, 7:10 p.m.
This line's too long. You can break at a .	chipx86	Aug. 17, 2018, 7:10 p.m.
Can you explain why here?	chipx86	Aug. 17, 2018, 7:11 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Aug. 17, 2018, 7:33 p.m.
F821 undefined name 'revision'	reviewbot	Aug. 17, 2018, 7:33 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Aug. 17, 2018, 7:39 p.m.
This sounds confusing. Can you say: The "%s" repository does not support review requests with commit history.	chipx86	Aug. 27, 2018, 5:18 p.m.
Blank line between these.	chipx86	Aug. 27, 2018, 5:19 p.m.

flake8 passed.

JSHint passed.

Change Summary:

Fix unit test failures.

Depends On:

+	~~9905 - Add a DVCS capability gated behind the DVCS feature~~

Commit:

-	c73a8395ee5a8c463733f9933cd1693ef85a84a2
+	26ef4844d2fe1de202a5e98cb3e484ea1f1e2bd2

Diff:

Revision 2 (+1703 -9)

Require validation for DiffCommits when posting

Information

Reviewers

Change Summary:

Depends On:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

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 (1 failed, 1 succeeded)

flake8

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 (1 failed, 1 succeeded)

flake8

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)

Depends On:

Change Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8