Summary

Add a form for uploading DiffCommits

Review Request #9642 — Created Feb. 15, 2018 and submitted Aug. 17, 2018, 6:32 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

~~9631~~, ~~9898~~, ~~9638~~

Blocks

9757

Commit

e50fa30...

Reviewers

Groups

reviewboard

People

Description

A new form has been added to upload a DiffCommit and attach it to a
DiffSet.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
This looks like an unintentional change.	david	Feb. 22, 2018, 3:16 p.m.
Can you move this above the clean methods? The clean methods, while not private, are more akin to private methods …	chipx86	Feb. 27, 2018, 8:12 p.m.
Other labels don't include periods.	david	March 27, 2018, 6:09 p.m.
F841 local variable 'diff_file' is assigned to but never used	reviewbot	April 13, 2018, 4:07 p.m.
F841 local variable 'diffset_history' is assigned to but never used	reviewbot	April 13, 2018, 4:07 p.m.
Do we want to call this commit_message?	chipx86	April 16, 2018, 6:02 p.m.
Can we use forms.EmailField? (Might hit some issue with values that aren't e-mail fields, so maybe not...) If not, maybe …	chipx86	April 16, 2018, 6:08 p.m.
This and committer_date should probably use forms.DateTimeField.	chipx86	April 16, 2018, 6 p.m.
This needs to be the full path to the model.	chipx86	April 16, 2018, 6:01 p.m.
This needs to be the full path to the model.	chipx86	April 16, 2018, 6:01 p.m.
If using DateTimeField, we shouldn't need these.	chipx86	April 16, 2018, 6:01 p.m.
Missing comma at the end. Can you move the SimpleUplodaedFile out into its own variable? That'll simplify this dictionary.	chipx86	April 16, 2018, 5:56 p.m.
No need for a set(), since there's only one item. Probably no need to cast to anything. I imagine it'll …	chipx86	April 16, 2018, 6:21 p.m.
You should be able to pull these out too, and reuse them for both forms.	chipx86	April 16, 2018, 5:59 p.m.
Can you update this to be: data=dict({ 'parent_id': 'r1', 'commit_id': 'r2', }, **self._default_form_data) This follows the pattern we use elsewhere.	chipx86	April 16, 2018, 5:55 p.m.
Same comment here as above.	chipx86	April 16, 2018, 6:01 p.m.
And these.	chipx86	April 16, 2018, 6:01 p.m.
Should be "Parent diff", to match labels below.	chipx86	July 3, 2018, 4:02 p.m.
You can remove the first "only."	chipx86	July 3, 2018, 4:02 p.m.
I want to strive for more friendly errors, and think we can improve on this just a tad. How about …	chipx86	July 3, 2018, 4:02 p.m.
Summary shouldn't have literals.	chipx86	July 3, 2018, 4:02 p.m.
Let's group together repository and the spy, and then separate out the diffset. Actually, why not have a setUp() that …	chipx86	July 3, 2018, 4:05 p.m.
Grouping should be consistent with the above test. Same for the next test.	chipx86	July 3, 2018, 4:05 p.m.
Do we modify the data passed in?	chipx86	July 3, 2018, 4:06 p.m.
Let's use update_fields, get just a bit of a speed boost. If we start to do that in more tests, …	chipx86	July 3, 2018, 4:07 p.m.
Can you reverse these?	chipx86	July 3, 2018, 4:07 p.m.
Both of these are separate, even though they're both about dates. They're separate pieces of code with their own logic. …	chipx86	July 3, 2018, 4:08 p.m.
E501 line too long (80 > 79 characters)	reviewbot	July 5, 2018, 3:59 p.m.
datetime.datetime	chipx86	July 10, 2018, 8:02 p.m.
datetime.datetime	chipx86	July 10, 2018, 8:02 p.m.
Two blank lines.	chipx86	July 10, 2018, 8:03 p.m.
I mentioned this before, but I think we want to use list() here, since ordering matters and we want to …	chipx86	Aug. 17, 2018, 3:26 p.m.

flake8 passed.

JSHint passed.

reviewboard/diffviewer/managers.py (Diff revision 1)
Show all issues
```
This looks like an unintentional change.
```

Commit:

-	fe4eb70deb54c0ea1681351770c4a8f9eab82ed3
+	c873a131535fe9c303f969fa7e0aa7696db28b64

Diff:

Revision 2 (+161 -7)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

Show all issues

Can you move this above the clean methods? The clean methods, while not private, are more akin to private methods than clean (which is meant to be used by consumers and should be more prominent).

Change Summary:

Fix depends-on

Depends On:

-	~~9609 - Add a model representation of a commit in a series~~
+	~~9631 - Add a manager for the DiffCommit model~~

Change Summary:

Addressed Christian's issues.

Commit:

-	c873a131535fe9c303f969fa7e0aa7696db28b64
+	1bfa88fb446b72d0de60c0fe51749ca06c896f72

Diff:

Revision 3 (+161 -7)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Rebased.

Commit:

-	1bfa88fb446b72d0de60c0fe51749ca06c896f72
+	9acb842bd8af5b3f09b052ba3b2e04742c8ed2c4

Diff:

Revision 4 (+163 -7)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/forms.py (Diff revision 4)
Show all issues
```
Other labels don't include periods.
```

Change Summary:

Addressed David's issues.

Commit:

-	9acb842bd8af5b3f09b052ba3b2e04742c8ed2c4
+	95922560dcbd2e158ef6419c871263969d94f3c1

Diff:

Revision 5 (+163 -7)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:


Move diffset argument into constructor.
Handle published diffsets (form validation fails).
Handle parent diffs on subsequent commits (form validation fails).
Add unit tests for above.

Commit:

-	95922560dcbd2e158ef6419c871263969d94f3c1
+	3aa1b16cf2bee17e41599f656544ab9ce37999bc

Diff:

Revision 6 (+323 -8)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_forms.py (Diff revision 6)
Show all issues
```
F841 local variable 'diff_file' is assigned to but never used
```

reviewboard/diffviewer/tests/test_forms.py (Diff revision 6)

Show all issues

F841 local variable 'diffset_history' is assigned to but never used

Change Summary:

Fix flake8 issues.

Commit:

-	3aa1b16cf2bee17e41599f656544ab9ce37999bc
+	88e25fecc47035bb3bd4b23b8eaad77c9616716c

Diff:

Revision 7 (+319 -8)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

		A new form has been added to upload a `DiffCommit` and attach it to a
		`DiffSet`. The common functionality (almost all of the implementation)
~		with the `UploadDiffForm has been refactored out into the`BaseUploadDiffForm`.
	~	with the `UploadDiffForm` has been refactored out into the
	+	`BaseUploadDiffForm`.

reviewboard/diffviewer/forms.py (Diff revision 7)
Show all issues
```
Do we want to call this commit_message?
```

reviewboard/diffviewer/forms.py (Diff revision 7)

Show all issues

Can we use forms.EmailField? (Might hit some issue with values that aren't e-mail fields, so maybe not...)

If not, maybe we should at least still use EmailInput as the widget.

Here and the other form below.

reviewboard/diffviewer/forms.py (Diff revision 7)

Show all issues

This and committer_date should probably use forms.DateTimeField.

reviewboard/diffviewer/forms.py (Diff revision 7)
Show all issues
```
This needs to be the full path to the model.
```
reviewboard/diffviewer/forms.py (Diff revision 7)
Show all issues
```
This needs to be the full path to the model.
```

reviewboard/diffviewer/forms.py (Diff revision 7)

Show all issues

If using DateTimeField, we shouldn't need these.

reviewboard/diffviewer/tests/test_forms.py (Diff revision 7)

Show all issues

Missing comma at the end.

Can you move the SimpleUplodaedFile out into its own variable? That'll simplify this dictionary.

reviewboard/diffviewer/tests/test_forms.py (Diff revision 7)

Show all issues

No need for a set(), since there's only one item. Probably no need to cast to anything. I imagine it'll iterate.

brennie April 16, 2018, 6:21 p.m.

We do, because it will compare querysets otherwise, not the actual items.

chipx86 April 30, 2018, 2:45 p.m.

list() is better then. We shouldn't use set() unless order truly never matters. If we were to update this to test with multiple files, we'd want to know if order ever somehow changed.

reviewboard/diffviewer/tests/test_forms.py (Diff revision 7)

Show all issues

You should be able to pull these out too, and reuse them for both forms.

reviewboard/diffviewer/tests/test_forms.py (Diff revision 7)

Show all issues

Can you update this to be:

data=dict({
    'parent_id': 'r1',
    'commit_id': 'r2',
}, **self._default_form_data)


This follows the pattern we use elsewhere.

brennie April 16, 2018, 6:08 p.m.

This won't overwrite the defaults in data.

>>> defaults = {'foo': 'foo'}
>>> dict({'foo': 'bar'}, **defaults)
{'foo': 'foo'}


It should be dict(defaults, **{overrides here})

reviewboard/diffviewer/tests/test_forms.py (Diff revision 7)
Show all issues
```
Same comment here as above.
```

reviewboard/diffviewer/tests/test_forms.py (Diff revision 7)

Show all issues

And these.

Change Summary:

Addressed Christian's issues.

Commit:

-	88e25fecc47035bb3bd4b23b8eaad77c9616716c
+	e7dc1f410e117ef41f5a4ba7da2062c35df8d46c

Diff:

Revision 8 (+320 -9)

Show changes

	reviewboard/settings.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

catch datetime validation errors

Commit:

-	e7dc1f410e117ef41f5a4ba7da2062c35df8d46c
+	63cecf6cc5e32e72b3a78b7ed892b77971e4b20b

Diff:

Revision 9 (+328 -9)

Show changes

	reviewboard/settings.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

-	63cecf6cc5e32e72b3a78b7ed892b77971e4b20b
+	9ae2988be66d334ff0796da4da00faeee7833388

Diff:

Revision 10 (+328 -9)

Show changes

	reviewboard/settings.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Rebase.
Remove BaseUploadDiffForm since implementation has diverged enough.

Description:

		A new form has been added to upload a `DiffCommit` and attach it to a
~		`DiffSet`. The common functionality (almost all of the implementation)
	~	`DiffSet`.
-		with the `UploadDiffForm` has been refactored out into the
-		`BaseUploadDiffForm`.

Commit:

-	9ae2988be66d334ff0796da4da00faeee7833388
+	6a97d4171e4466564daac3ed1a7e307cb9a95749

Diff:

Revision 11 (+336 -12)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Rename some fields

Commit:

-	6a97d4171e4466564daac3ed1a7e307cb9a95749
+	9e137434c663fb494c60b9aacb976ac3a3bfbc9b

Diff:

Revision 12 (+336 -12)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
Ship It!
```

Change Summary:

Require ISO 8601 dates

Depends On:

+	~~9898 - Drop dependency on python-dateutil~~

Commit:

-	9e137434c663fb494c60b9aacb976ac3a3bfbc9b
+	746d4ac6c83fa088ca4332463363a4ae511c1cbb

Diff:

Revision 13 (+363 -12)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Summary:

-	Add a form for uploading DiffCommits and refactor UploadDiffForm
+	Add a form for uploading DiffCommits

Change Summary:

diff_commit_count -> diffcommit_count

Commit:

-	746d4ac6c83fa088ca4332463363a4ae511c1cbb
+	833aa54a17549f6f22f41bf52e2c039381bcf087

Diff:

Revision 14 (+363 -12)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Oops

Commit:

-	833aa54a17549f6f22f41bf52e2c039381bcf087
+	cdf686f830798bb31432b659981093427a51edba

Diff:

Revision 15 (+363 -12)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/forms.py (Diff revision 15)
Show all issues
```
Should be "Parent diff", to match labels below.
```
reviewboard/diffviewer/forms.py (Diff revision 15)
Show all issues
```
You can remove the first "only."
```

reviewboard/diffviewer/forms.py (Diff revision 15)

Show all issues

I want to strive for more friendly errors, and think we can improve on this just a tad. How about simply: "This date must be in ISO 8601 format."

reviewboard/diffviewer/forms.py (Diff revision 15)
Show all issues
```
Summary shouldn't have literals.
```

reviewboard/diffviewer/tests/test_forms.py (Diff revision 15)

Show all issues

Let's group together repository and the spy, and then separate out the diffset.

Actually, why not have a setUp() that handles these objects? We're repeating ourselves a lot.

reviewboard/diffviewer/tests/test_forms.py (Diff revision 15)

Show all issues

Grouping should be consistent with the above test.

Same for the next test.

reviewboard/diffviewer/tests/test_forms.py (Diff revision 15)
Show all issues
```
Do we modify the data passed in?
```

reviewboard/diffviewer/tests/test_forms.py (Diff revision 15)

Show all issues

Let's use update_fields, get just a bit of a speed boost. If we start to do that in more tests, we'll speed up the test suite.

reviewboard/diffviewer/tests/test_forms.py (Diff revision 15)
Show all issues
```
Can you reverse these?
```

reviewboard/diffviewer/tests/test_forms.py (Diff revision 15)

Show all issues

Both of these are separate, even though they're both about dates. They're separate pieces of code with their own logic. The docstring even really acknowledges this. We should break this into two tests.

Commit:

-	cdf686f830798bb31432b659981093427a51edba
+	f5385c32f09db3759c50cecd4aa6a419422bd114

Diff:

Revision 16 (+362 -12)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_forms.py (Diff revision 16)
Show all issues
```
E501 line too long (80 > 79 characters)
```

Change Summary:

flake8 fixups

Commit:

-	f5385c32f09db3759c50cecd4aa6a419422bd114
+	7d05e96119044a8eec793c072ac977a389e0533c

Diff:

Revision 17 (+363 -12)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/forms.py (Diff revision 17)
Show all issues
```
datetime.datetime
```
reviewboard/diffviewer/forms.py (Diff revision 17)
Show all issues
```
datetime.datetime
```
reviewboard/diffviewer/tests/test_forms.py (Diff revision 17)
Show all issues
```
Two blank lines.
```

Change Summary:

Addressed feedback.

Commit:

-	7d05e96119044a8eec793c072ac977a389e0533c
+	a68c157419c4c8a6a2712be7bce7ef43cf85a252

Diff:

Revision 18 (+364 -12)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Allow parent diff in subsequent commits

Commit:

-	a68c157419c4c8a6a2712be7bce7ef43cf85a252
+	e50fa3063f074d7649fa4dcdadcc6ddebc98f77e

Diff:

Revision 19 (+340 -12)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/tests/test_forms.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it! Fix it!

reviewboard/diffviewer/tests/test_forms.py (Diff revision 19)

Show all issues

I mentioned this before, but I think we want to use list() here, since ordering matters and we want to know if that ever breaks.

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (ea80fb3)