Summary

Add initial review request UI for commit histories.

Review Request #6931 — Created Feb. 10, 2015 and submitted March 3, 2015, 2:08 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

dvcs

Bugs

Depends On

~~6816~~, ~~6815~~

Commit

39bc55a...

Reviewers

Groups

reviewboard

People

chipx86, mike_conley, smacleod

Description

Review requests that contain commit history will now show the list of
commits on the current diff revision. The changes to the commits in
diff revisions are now reflected in change descriptions.

The BuiltinLocalFieldMixin has been modified to allow subclasses to
override its default behaviour (where the review_request_details are
replaced if it is a draft and the field does not exist on the draft).
This was changed because the new DiffCommitListField needs to display
the draft commits and the default behaviour would prevent that.

Testing Done

Tested the following manually:

Created a review request with commit history. The commit list field
was displayed in the draft view.
Published the review request. The commit list field was displayed in
the public review request.
Uploaded a new diff with history. The draft view reflected the
changes in the commit list field.
Published the review request. The change description reflected the
changes to the commits on the review request.
Published the review request without uploading a new diff. The commit
list field did not change and it did not appear in the change
description.
Updated the review request using a new diff with history. The change
description was rendered properly.
Updated the review request using a squashed diff. The commit list
field disappeared from the review request and the change description
showed the removed commits.
Updated the review request using a a diff with history. THe commit
list field re-appeared in the review request and the chagne
description showed the added commits.

Files

Issues

Description	From	Last Updated
Hmm. It would be really nice to have a single list of commits: Commit Change Author Add foo Updated ... …	david	Feb. 25, 2015, 9:02 a.m.
<p>That is pretty great. I love it.</p>	chipx86	Feb. 27, 2015, 9:44 a.m.
This would be good for the docstring.	chipx86	Feb. 12, 2015, 1:44 p.m.
This would be good info in the docstring.	chipx86	Feb. 12, 2015, 1:45 p.m.
We need to return something else if this doesn't match. (Okay, technically, in Python, we don't, but it'd best to …	chipx86	Feb. 12, 2015, 1:45 p.m.
You can reduce the verbosity of the code (and reduce lookups) by starting off with a local variable for the …	chipx86	Feb. 12, 2015, 1:46 p.m.
To avoid an extra query, can we fetch both up-front into a list, and then fetch out of that list? …	chipx86	Feb. 12, 2015, 2:36 p.m.
Ideally, we'd pull out of the locals, but I know we don't have that. Maybe add a # TODO for …	chipx86	Feb. 12, 2015, 2:14 p.m.
Since the indentation is weird, how about pulling out the list into a variable first, then iterating that?	chipx86	Feb. 12, 2015, 2:15 p.m.
Selectors should be in alphabetical order.	chipx86	Feb. 12, 2015, 2:19 p.m.
Here too.	chipx86	Feb. 12, 2015, 2:19 p.m.
One space indentation.	chipx86	Feb. 12, 2015, 2:17 p.m.
Template tags and HTML tags should be indented independently of each other, with indentation for template tags happening inside the …	chipx86	Feb. 12, 2015, 2:17 p.m.
Col: 9 E303 too many blank lines (2)	reviewbot	Feb. 25, 2015, 9:04 a.m.
Col: 9 E303 too many blank lines (2)	reviewbot	Feb. 25, 2015, 9:04 a.m.
This will evaluate by pulling in all matches, deserializing them, and then checking the resulting list for truthiness. Instead, add …	chipx86	Feb. 27, 2015, 9:17 a.m.
"ID"	chipx86	Feb. 27, 2015, 9:18 a.m.
You can use .exists() here as well. Basically, it will do a SELECT 1 FROM ... LIMIT 1, which is …	chipx86	Feb. 27, 2015, 9:18 a.m.
This seems a bit oddly formatted. I'd have expected the pep8 checker to yell about it. How about: commit_list = …	chipx86	Feb. 27, 2015, 9:22 a.m.
Small thing, but just for readability, can you move the j < len(...) onto the next line so there's only …	chipx86	Feb. 27, 2015, 9:22 a.m.
You can simplify this just a bit with a generator expression to a dictionary.	chipx86	Feb. 27, 2015, 9:24 a.m.
If there's 20 diffsets, we'll make 20 queries here, each with a join. How about doing: diff_commits = DiffCommit.objects.filter(diffset__in=diffsets) for …	chipx86	Feb. 27, 2015, 9:42 a.m.
Ideally, we'd reference some consatnts for any colors. Here and below.	chipx86	Feb. 27, 2015, 9:42 a.m.
Needs localization.	chipx86	Feb. 27, 2015, 9:38 a.m.
No spaces inside the {{}}	chipx86	Feb. 27, 2015, 9:38 a.m.
Needs localization.	chipx86	Feb. 27, 2015, 9:38 a.m.
No spaces inside the {{}}. Here and below.	chipx86	Feb. 27, 2015, 9:38 a.m.
Add a blank line after this.	david	March 3, 2015, 2:06 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/static/rb/css/pages/reviews.less

People:

+	smacleod
+	mike_conley

Change Summary:

Remove diffviewer dynamic content placeholders. Upload screenshots.

Summary:

-	[WIP] Add the initial DiffCommitListField UI and style
+	Add initial review request UI for commit histories

Description:

~		This is very much a work in progress.
	~	Review requests that contain commit history will now show the list of
	+	commits on the current diff revision. The changes to the commits in
	+	diff revisions are now reflected in change descriptions.
	+
	+	The `BuiltinLocalFieldMixin` has been modified to allow subclasses to
	+	override its default behaviour (where the `review_request_details` are
	+	replaced if it is a draft and the field does not exist on the draft).
	+	This was changed because the new `DiffCommitListField` needs to display
	+	the draft commits and the default behaviour would prevent that.

Testing Done:

	+	Tested the following manually:
	+
	+	Created a review request with commit history. The commit list field was displayed in the draft view.
	+	Published the review request. The commit list field was displayed in the public review request.
	+	Uploaded a new diff with history. The draft view reflected the changes in the commit list field.
	+	Published the review request. The change description reflected the changes to the commits on the review request.
	+	Published the review request without uploading a new diff. The commit list field did not change and it did not appear in the change description.
	+	Updated the review request using a new diff with history. The change description was rendered properly.
	+	Updated the review request using a squashed diff. The commit list field disappeared from the review request and the change description showed the removed commits.
	+	Updated the review request using a a diff with history. THe commit list field re-appeared in the review request and the chagne description showed the added commits.

People:

+	chipx86

Diff:

Revision 2 (+183 -3)

Show changes

	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/boxes/commit_list.html

Added Files:

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/static/rb/css/pages/reviews.less

reviewboard/reviews/builtin_fields.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This would be good for the docstring.
```
reviewboard/reviews/builtin_fields.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This would be good info in the docstring.
```

reviewboard/reviews/builtin_fields.py (Diff revision 2)

The issue has been resolved. Show all issues

We need to return something else if this doesn't match.
(Okay, technically, in Python, we don't, but it'd best to be explicit.)

reviewboard/reviews/builtin_fields.py (Diff revision 2)

The issue has been resolved. Show all issues

You can reduce the verbosity of the code (and reduce lookups) by starting off with a local variable for the dictionary, populating it, and then setting that into changedesc after.

reviewboard/reviews/builtin_fields.py (Diff revision 2)

The issue has been resolved. Show all issues

To avoid an extra query, can we fetch both up-front into a list, and then fetch out of that list? (Database queries are expensive and, historically, the cause of most of our performance issues on large servers, so I try to point this out whenever possible :)

reviewboard/reviews/builtin_fields.py (Diff revision 2)

The issue has been resolved. Show all issues

Ideally, we'd pull out of the locals, but I know we don't have that. Maybe add a # TODO for that, and an entry in Asana?

reviewboard/reviews/builtin_fields.py (Diff revision 2)

The issue has been resolved. Show all issues

Since the indentation is weird, how about pulling out the list into a variable first, then iterating that?

reviewboard/static/rb/css/pages/reviews.less (Diff revision 2)
The issue has been resolved. Show all issues
```
Selectors should be in alphabetical order.
```
reviewboard/static/rb/css/pages/reviews.less (Diff revision 2)
The issue has been resolved. Show all issues
```
Here too.
```
reviewboard/templates/reviews/boxes/commit_list.html (Diff revision 2)
The issue has been resolved. Show all issues
```
One space indentation.
```

reviewboard/templates/reviews/boxes/commit_list.html (Diff revision 2)

The issue has been resolved. Show all issues

Template tags and HTML tags should be indented independently of each other, with indentation for template tags happening inside the {% ... %}, like:

{% for ... %}
{%  if ... %}

The template language has its own set of indentation, and the formatted HTML is its own set of indentation.

Change Summary:

master -> dvcs

Branch:

-	master
+	dvcs

Summary:

-	Add initial review request UI for commit histories
+	Add initial review request UI for commit histories.

Commit:

+	24c629e6cdcfec5821f40f486e44a287f7c171e2

Diff:

Revision 3 (+1475 -67)

Show changes

	reviewboard/diffviewer/forms.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/boxes/commit_list.html
	reviewboard/testing/testcase.py
	reviewboard/webapi/server_info.py
	12 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/resources/draft_diff_commit.py
    reviewboard/webapi/server_info.py
    reviewboard/webapi/resources/filediff.py
    reviewboard/webapi/tests/test_draft_diff_commit.py
    reviewboard/webapi/resources/draft_diff.py
    reviewboard/webapi/tests/test_diff_commit.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/forms.py
    reviewboard/webapi/tests/urls.py
    reviewboard/webapi/tests/test_filediff.py
    reviewboard/webapi/tests/mimetypes.py
    reviewboard/webapi/resources/diff.py
    reviewboard/webapi/resources/diff_commit.py
    reviewboard/diffviewer/forms.py
    reviewboard/webapi/resources/draft_filediff.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/resources/draft_diff_commit.py
    reviewboard/webapi/server_info.py
    reviewboard/webapi/resources/filediff.py
    reviewboard/webapi/tests/test_draft_diff_commit.py
    reviewboard/webapi/resources/draft_diff.py
    reviewboard/webapi/tests/test_diff_commit.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/forms.py
    reviewboard/webapi/tests/urls.py
    reviewboard/webapi/tests/test_filediff.py
    reviewboard/webapi/tests/mimetypes.py
    reviewboard/webapi/resources/diff.py
    reviewboard/webapi/resources/diff_commit.py
    reviewboard/diffviewer/forms.py
    reviewboard/webapi/resources/draft_filediff.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/static/rb/css/pages/reviews.less

Change Summary:

Wrong branch

Diff:

Revision 4 (+199 -3)

Show changes

	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/boxes/commit_list.html

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/static/rb/css/pages/reviews.less

A change description showing removed and added commits. (Revision 1)

The issue has been resolved. Show all issues

Hmm. It would be really nice to have a single list of commits:

Commit	Change	Author
Add foo	Updated	...
Add bar	Updated	...
Call foo and bar in main	Updated	...
Add shebang to run.py	Updated	...
Add gitignore for pyc files	Updated	...
Add baz module	Added	...

brennie Feb. 25, 2015, 9:03 a.m.

The current revision has support for unmodified, added, and removed commits. It does not currently support detecting modified commits.

Diff:

Revision 5 (+252 -3)

Show changes

	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/boxes/commit_list.html

Added Files:

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/static/rb/css/pages/reviews.less

reviewboard/reviews/builtin_fields.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 9
 E303 too many blank lines (2)
```
reviewboard/reviews/builtin_fields.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 9
 E303 too many blank lines (2)
```

Diff:

Revision 6 (+250 -3)

Show changes

	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/boxes/commit_list.html

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/static/rb/css/pages/reviews.less

File Captions:

changedesc.png:
- A change description showing removed and added commits.
+ A change description showing unmodified and added commits.

Change Summary:

forgot template

Diff:

Revision 7 (+278 -3)

Show changes

	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/boxes/commit_list.html
	reviewboard/templates/reviews/boxes/commit_list_change.html

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/templates/reviews/boxes/commit_list_change.html
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/templates/reviews/boxes/commit_list_change.html
    reviewboard/static/rb/css/pages/reviews.less

A change description showing unmodified and added commits. (Revision 2)
The issue has been resolved. Show all issues
```
<p>That is pretty great. I love it.</p>
```

reviewboard/reviews/builtin_fields.py (Diff revision 7)

The issue has been resolved. Show all issues

This will evaluate by pulling in all matches, deserializing them, and then checking the resulting list for truthiness.

Instead, add .exists() to the end of that query.

reviewboard/reviews/builtin_fields.py (Diff revision 7)
The issue has been resolved. Show all issues
```
"ID"
```

reviewboard/reviews/builtin_fields.py (Diff revision 7)

The issue has been resolved. Show all issues

You can use .exists() here as well. Basically, it will do a SELECT 1 FROM ... LIMIT 1, which is faster even than counting.

reviewboard/reviews/builtin_fields.py (Diff revision 7)

The issue has been resolved. Show all issues

This seems a bit oddly formatted. I'd have expected the pep8 checker to yell about it. How about:

commit_list = list(
    DiffCommit.objects
        .select_related('diffset')
        .filter(...))

brennie Feb. 27, 2015, 9:22 a.m.

As it turns out, this code you suggested is a PEP8 violation on the .selected_realted('diffset') line:

E131 continutation line unaligned for hanging indent.

reviewboard/reviews/builtin_fields.py (Diff revision 7)

The issue has been resolved. Show all issues

Small thing, but just for readability, can you move the j < len(...) onto the next line so there's only one expression per line?

reviewboard/reviews/builtin_fields.py (Diff revision 7)

The issue has been resolved. Show all issues

You can simplify this just a bit with a generator expression to a dictionary.

reviewboard/reviews/views.py (Diff revision 7)

The issue has been resolved. Show all issues

If there's 20 diffsets, we'll make 20 queries here, each with a join.
How about doing:

diff_commits = DiffCommit.objects.filter(diffset__in=diffsets)

for diff_commit in diff_commits:
    diff_commits_by_id.setdefault(
        diff_commit.diffset_id, []).append(diff_commit)

brennie Feb. 27, 2015, 9:33 a.m.

Is there any benefit in using setdefault over collections.defaultdict, i.e.:

from collections import defaultdict

# ...

diff_commits = DiffCommit.objects.filter(diffset__in=diffsets)
diff_commits_by_id = defaultdict(list)

for diff_commit in diff_commits:
    diff_commits_by_id[diff_commit.id].append(diff_commit)

reviewboard/static/rb/css/pages/reviews.less (Diff revision 7)
The issue has been resolved. Show all issues
```
Ideally, we'd reference some consatnts for any colors. Here and below.
```
reviewboard/templates/reviews/boxes/commit_list.html (Diff revision 7)
The issue has been resolved. Show all issues
```
Needs localization.
```
reviewboard/templates/reviews/boxes/commit_list.html (Diff revision 7)
The issue has been resolved. Show all issues
```
No spaces inside the {{}}
```
reviewboard/templates/reviews/boxes/commit_list_change.html (Diff revision 7)
The issue has been resolved. Show all issues
```
Needs localization.
```
reviewboard/templates/reviews/boxes/commit_list_change.html (Diff revision 7)
The issue has been resolved. Show all issues
```
No spaces inside the {{}}. Here and below.
```

Change Summary:

Addressed Christrian's issues.

Diff:

Revision 8 (+284 -6)

Show changes

	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/boxes/commit_list.html
	reviewboard/templates/reviews/boxes/commit_list_change.html

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/templates/reviews/boxes/commit_list_change.html
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/templates/reviews/boxes/commit_list_change.html
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/pages/reviews.less

Ship it!

reviewboard/reviews/builtin_fields.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Add a blank line after this.
```

Commit:

-	24c629e6cdcfec5821f40f486e44a287f7c171e2
+	39bc55ae634715260baa780004c27291d5745188

Diff:

Revision 9 (+285 -6)

Show changes

	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/boxes/commit_list.html
	reviewboard/templates/reviews/boxes/commit_list_change.html

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/templates/reviews/boxes/commit_list_change.html
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/reviews/boxes/commit_list.html
    reviewboard/templates/reviews/boxes/commit_list_change.html
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/pages/reviews.less

Status: Closed (submitted)

Change Summary:

Pushed to dvcs (241be47)

Add initial review request UI for commit histories.

Information

Reviewers

People:

Change Summary:

Summary:

Description:

Testing Done:

People:

Diff:

Added Files:

Change Summary:

Branch:

Summary:

Commit:

Diff:

Change Summary:

Diff:

Diff:

Added Files:

Diff:

File Captions:

Change Summary:

Diff:

Change Summary:

Diff:

Commit:

Diff:

Status: Closed (submitted)

Change Summary: