Add initial review request UI for commit histories.
Review Request #6931 — Created Feb. 10, 2015 and submitted
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
BuiltinLocalFieldMixinhas been modified to allow subclasses to
override its default behaviour (where thereview_request_detailsare
replaced if it is a draft and the field does not exist on the draft).
This was changed because the newDiffCommitListFieldneeds to display
the draft commits and the default behaviour would prevent that.
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.
| Description | From | Last Updated |
|---|---|---|
|
Hmm. It would be really nice to have a single list of commits: Commit Change Author Add foo Updated ... … |
 david
|
|
|
<p>That is pretty great. I love it.</p> |
 chipx86
|
|
|
This would be good for the docstring. |
chipx86
|
|
|
This would be good info in the docstring. |
chipx86
|
|
|
We need to return something else if this doesn't match. (Okay, technically, in Python, we don't, but it'd best to … |
chipx86
|
|
|
You can reduce the verbosity of the code (and reduce lookups) by starting off with a local variable for the … |
chipx86
|
|
|
To avoid an extra query, can we fetch both up-front into a list, and then fetch out of that list? … |
chipx86
|
|
|
Ideally, we'd pull out of the locals, but I know we don't have that. Maybe add a # TODO for … |
chipx86
|
|
|
Since the indentation is weird, how about pulling out the list into a variable first, then iterating that? |
chipx86
|
|
|
Selectors should be in alphabetical order. |
chipx86
|
|
|
Here too. |
chipx86
|
|
|
One space indentation. |
chipx86
|
|
|
Template tags and HTML tags should be indented independently of each other, with indentation for template tags happening inside the … |
chipx86
|
|
|
Col: 9 E303 too many blank lines (2) |
 reviewbot
|
|
|
Col: 9 E303 too many blank lines (2) |
reviewbot
|
|
|
This will evaluate by pulling in all matches, deserializing them, and then checking the resulting list for truthiness. Instead, add … |
chipx86
|
|
|
"ID" |
chipx86
|
|
|
You can use .exists() here as well. Basically, it will do a SELECT 1 FROM ... LIMIT 1, which is … |
chipx86
|
|
|
This seems a bit oddly formatted. I'd have expected the pep8 checker to yell about it. How about: commit_list = … |
chipx86
|
|
|
Small thing, but just for readability, can you move the j < len(...) onto the next line so there's only … |
chipx86
|
|
|
You can simplify this just a bit with a generator expression to a dictionary. |
chipx86
|
|
|
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
|
|
|
Ideally, we'd reference some consatnts for any colors. Here and below. |
chipx86
|
|
|
Needs localization. |
chipx86
|
|
|
No spaces inside the {{}} |
chipx86
|
|
|
Needs localization. |
chipx86
|
|
|
No spaces inside the {{}}. Here and below. |
chipx86
|
|
|
Add a blank line after this. |
david
|
- People:
- Change Summary:
-
Remove diffviewer dynamic content placeholders. Upload screenshots.
- Summary:
-
[WIP] Add the initial DiffCommitListField UI and styleAdd 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
BuiltinLocalFieldMixinhas been modified to allow subclasses to+ override its default behaviour (where the review_request_detailsare+ replaced if it is a draft and the field does not exist on the draft). + This was changed because the new DiffCommitListFieldneeds 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.
- Created a review request with commit history. The commit list field
-
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
-
-
-
-
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.)
-
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
changedescafter. -
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 :)
-
Ideally, we'd pull out of the locals, but I know we don't have that. Maybe add a
# TODOfor that, and an entry in Asana? -
Since the indentation is weird, how about pulling out the list into a variable first, then iterating that?
-
-
-
-
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.
- Summary:
-
Add initial review request UI for commit historiesAdd initial review request UI for commit histories.
- Commit:
-
24c629e6cdcfec5821f40f486e44a287f7c171e2
- Diff:
-
Revision 3 (+1475 -67)
-
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
-
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
-
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 -
-
-
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.
-
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
-
-
-
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. -
-
You can use
.exists()here as well. Basically, it will do aSELECT 1 FROM ... LIMIT 1, which is faster even than counting. -
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(...)) -
Small thing, but just for readability, can you move the
j < len(...)onto the next line so there's only one expression per line? -
-
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) -
-
-
-
-
-
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
-
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
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