Summary

Display the selected revision's commits in the diffviewer

Review Request #10125 — Created Aug. 23, 2018 and submitted Oct. 16, 2018, 1:25 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

~~10094~~

Blocks

10126

Commit

c643e35...

Reviewers

Groups

reviewboard

People

Description

The diffviewer now displays the list of commit messages and authors and
they are updated when the selected revision changes. Currently the
commit list doesn't support interdiffs and an error message is
displayed instead. This will be addressed in a future patch.

Testing Done


Ran unit tests.
Ran JS tests.

Files

Issues

Description	From	Last Updated
Typo in the description: "commist" -> "commit"	chipx86	Aug. 27, 2018, 6:22 p.m.
F401 'reviewboard.diffviewer.features.dvcs_feature' imported but unused	reviewbot	Aug. 24, 2018, 2 p.m.
Col: 27 Missing semicolon.	reviewbot	Aug. 24, 2018, 2 p.m.
Col: 14 'testRows' was used before it was defined.	reviewbot	Aug. 24, 2018, 2 p.m.
Leftover debug output.	chipx86	Aug. 27, 2018, 6:22 p.m.
This can easily be in alphabetical order. (While that doesn't always make things better in CSS -- width and height, …	chipx86	Aug. 27, 2018, 6:22 p.m.
Swap these.	chipx86	Aug. 27, 2018, 6:23 p.m.
Alphabetical order for these selectors.	chipx86	Aug. 27, 2018, 6:23 p.m.
This is using a trailing comma, but this file isn't a .es6.js. Can you rename the file?	chipx86	Aug. 27, 2018, 6:24 p.m.
Can you add a constant for the max length, and then compute the two numbers from that?	chipx86	Aug. 27, 2018, 6:26 p.m.
I'd prefer to be explicit and not use the shorthand. Especially when we're having to be explicit for other values …	chipx86	Aug. 27, 2018, 6:37 p.m.
Missing docs.	chipx86	Aug. 27, 2018, 6:28 p.m.
Missing a trailing comma.	chipx86	Aug. 27, 2018, 6:28 p.m.
interview? :)	chipx86	Aug. 27, 2018, 6:28 p.m.
Wrong number of spaces for indentation.	chipx86	Aug. 27, 2018, 6:30 p.m.
Extra blank line.	chipx86	Aug. 27, 2018, 6:30 p.m.
Alphabetical order.	chipx86	Aug. 27, 2018, 6:30 p.m.
Can you be more explicit in the TODO?	chipx86	Aug. 27, 2018, 6:35 p.m.
Can you split out the template parameters so it's consistent with other calls in the codebase and easier to update? …	chipx86	Aug. 27, 2018, 6:36 p.m.
As above, I'd prefer being explicit with a key: value form.	chipx86	Aug. 27, 2018, 6:36 p.m.
Here, too.	chipx86	Aug. 27, 2018, 6:37 p.m.
Technically this is a jQuery.Event. Same below.	chipx86	Aug. 27, 2018, 6:37 p.m.
Extra blank line.	chipx86	Aug. 27, 2018, 6:37 p.m.
Extra blank line.	chipx86	Aug. 27, 2018, 6:37 p.m.
We need to explicitly use function() with Jasmine instead of =>, as this matters.	chipx86	Aug. 27, 2018, 6:39 p.m.
Blank line between these.	chipx86	Aug. 27, 2018, 6:39 p.m.
No blank line here.	chipx86	Aug. 27, 2018, 6:39 p.m.
function(), not () =>. Same with all others in the tests in this change.	chipx86	Aug. 27, 2018, 6:39 p.m.
No need for </div>.	chipx86	Aug. 27, 2018, 6:39 p.m.
No need to define to null.	chipx86	Aug. 27, 2018, 6:40 p.m.
Can you use the explicit expanded form? Same below.	chipx86	Aug. 27, 2018, 6:40 p.m.
No spaces within the {}. This file is inconsistent on this. Same with all below.	chipx86	Aug. 27, 2018, 6:41 p.m.
Missing trailing comma.	chipx86	Aug. 27, 2018, 6:41 p.m.
Missing trailing comma.	chipx86	Aug. 27, 2018, 6:41 p.m.
No need for the default value.	chipx86	Aug. 27, 2018, 6:41 p.m.
Comma before "if any."	chipx86	Aug. 27, 2018, 6:42 p.m.
Missing docs.	chipx86	Aug. 27, 2018, 6:44 p.m.
If we can avoid this form, we should. It ends up being unnecessarily expensive, since it doesn't really add anything …	chipx86	Aug. 27, 2018, 6:44 p.m.
Can you put this in alphabetical order?	chipx86	Aug. 27, 2018, 6:44 p.m.
In sync how? Given the length and need for more details, this can use {% comment %}...{% endcomment %}.	chipx86	Aug. 27, 2018, 6:46 p.m.
Two blank lines on either side.	chipx86	Aug. 28, 2018, 1:57 p.m.
Should be Model Attributes.	chipx86	Aug. 28, 2018, 1:57 p.m.
boolean	chipx86	Aug. 28, 2018, 1:57 p.m.
Model Attribute.	chipx86	Aug. 28, 2018, 1:57 p.m.
Can you explain what needs to be kept in sync?	chipx86	Aug. 28, 2018, 1:57 p.m.
Multi-line comments should be in /* .. */ format.	chipx86	Aug. 28, 2018, 1:57 p.m.
Kinda looks wrong. Let's do: $content = $(tableTemplate({ ... })) .append(...)	chipx86	Aug. 28, 2018, 1:57 p.m.
boolean	chipx86	Aug. 28, 2018, 1:57 p.m.
Typo: "attribtues" -> "attributes".	chipx86	Aug. 28, 2018, 1:57 p.m.
Needs to be .call instead of .apply if not passing a list of arguments.	chipx86	Aug. 28, 2018, 1:57 p.m.
Alphabetical order.	chipx86	Aug. 28, 2018, 1:57 p.m.
This defaults to an empty list, but the other view defaults to None. We should be consistent to avoid issues …	chipx86	Aug. 30, 2018, 7:37 p.m.
Still need two blank lines.	chipx86	Aug. 30, 2018, 7:37 p.m.
Let's be explicit, especially since we are in the two cases above.	chipx86	Aug. 30, 2018, 7:38 p.m.
No spaces immediately within {..}	chipx86	Aug. 30, 2018, 7:38 p.m.
No spaces immediately within {..}	chipx86	Aug. 30, 2018, 7:38 p.m.
No spaces immediately within {..}	chipx86	Aug. 30, 2018, 7:39 p.m.
These templates are all using 2 spaces for indentation in the HTML, but they should use 1 like all other …	chipx86	Sept. 4, 2018, 12:37 p.m.
Alphabetical order.	chipx86	Sept. 4, 2018, 12:38 p.m.
Alphabetical order.	chipx86	Sept. 4, 2018, 12:38 p.m.
Can you check isEmpty() instead?	chipx86	Sept. 4, 2018, 12:38 p.m.
Col: 34 Missing semicolon.	reviewbot	Sept. 18, 2018, 3:38 p.m.
Col: 34 Missing semicolon.	reviewbot	Sept. 18, 2018, 3:38 p.m.
Col: 34 Missing semicolon.	reviewbot	Sept. 18, 2018, 3:37 p.m.
Can we put the expression in parens?	david	Sept. 27, 2018, 9:33 a.m.
You don't need to use the $() around headerTemplate--you can just append the HTML.	david	Sept. 27, 2018, 9:33 a.m.
Same here.	david	Sept. 27, 2018, 9:33 a.m.
We should probably also do e.stopPropagation()	david	Sept. 27, 2018, 9:34 a.m.
We should probably also do e.stopPropagation()	david	Sept. 27, 2018, 9:34 a.m.
Needs "Args:" now.	david	Sept. 27, 2018, 9:36 a.m.

flake8 failed.

JSHint failed.

flake8

reviewboard/diffviewer/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'reviewboard.diffviewer.features.dvcs_feature' imported but unused
```

JSHint

reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 27
 Missing semicolon.
```
reviewboard/static/rb/js/diffviewer/views/tests/diffCommitListViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 14
 'testRows' was used before it was defined.
```

Change Summary:

Address issues

Commit:

-	78ce35f91ff7165bc95f2ff908bb67ede1ef4900
+	38af8a0737f0b5cc0afb2663cf5fe285aa8d1493

Diff:

Revision 2 (+692 -132)

Display the selected revision's commits in the diffviewer

Information

Reviewers

flake8

JSHint

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Description:

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 (2 succeeded)

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 (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Change Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Change Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Change Summary:

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: