Summary

Serialize primary keys in DiffCommits and history entries

Review Request #10128 — Created Aug. 25, 2018 and submitted Oct. 18, 2018, 9:39 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

~~10127~~

Blocks

10132

Commit

d57abd3...

Reviewers

Groups

reviewboard

People

Description

When doing an interdiff, it is possible that there could be two commits
in the RB.DiffCommitCollection that have the same commitId property.
However, they may have different properties (however unlikely), so we
now serialize commits with their primary keys and use them for lookup in
history entries instead of the commit ID. This was especially prevalent
in test cases where the commit IDs were not actually representative
(i.e., a hash) of the content of the commit.

Testing Done


Ran unit tests.
Ran JS tests.

Issues

Description	From	Last Updated
Col: 30 Expected '{' and instead saw 'debugger'.	reviewbot	Aug. 25, 2018, 2:33 a.m.
Col: 30 Forgotten 'debugger' statement?	reviewbot	Aug. 25, 2018, 2:33 a.m.
Col: 30 Expected '{' and instead saw 'debugger'.	reviewbot	Aug. 25, 2018, 10:49 a.m.
Col: 30 Forgotten 'debugger' statement?	reviewbot	Aug. 25, 2018, 10:49 a.m.
Unwanted "for" in the summary.	chipx86	Aug. 30, 2018, 7:49 p.m.
Can you put this in alphabetical order?	chipx86	Aug. 30, 2018, 7:49 p.m.
Too many blank lines.	chipx86	Aug. 30, 2018, 7:49 p.m.
This will easily fit on one line. It'll fit on the previous if you do: diffset = review_request_details.get_latest_diffset() editor_data['commits'] = …	chipx86	Aug. 30, 2018, 7:50 p.m.
Should be in alphabetical order.	chipx86	Aug. 30, 2018, 7:51 p.m.

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 30
 Expected '{' and instead saw 'debugger'.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 30
 Forgotten 'debugger' statement?
```

Commit:

0a0c09fc7c49aadf11d3a89469b5d1f4c618ec76

8560976f3a0f6d74d84af1fd5b220c45189f8b9b

Diff:

Revision 2 (+60 -36)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/models/diffcommit.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/js/diffviewer/models/diffCommit.es6.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js
	3 more

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 30
 Expected '{' and instead saw 'debugger'.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 30
 Forgotten 'debugger' statement?
```

Commit:

8560976f3a0f6d74d84af1fd5b220c45189f8b9b

e3ac6d579928e79a360e5693bbb96923b8818bff

Diff:

Revision 3 (+58 -36)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/models/diffcommit.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/js/diffviewer/models/diffCommit.es6.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/models/diffcommit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Unwanted "for" in the summary.
```
reviewboard/diffviewer/models/diffcommit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can you put this in alphabetical order?
```

reviewboard/diffviewer/models/diffcommit.py (Diff revision 3)

The issue has been resolved. Show all issues

Too many blank lines.

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 3)

The issue has been resolved. Show all issues

This will easily fit on one line.
It'll fit on the previous if you do:

diffset = review_request_details.get_latest_diffset()
editor_data['commits'] = [
    commit.serialize()
    for commit in diffset.commits.all()
]

reviewboard/static/rb/js/diffviewer/models/diffCommit.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Should be in alphabetical order.
```

Change Summary:

Addressed feedback.

Commit:

e3ac6d579928e79a360e5693bbb96923b8818bff

1a84a4b7cb60b889acea801c98114c7f64baad33

Diff:

Revision 4 (+58 -37)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/models/diffcommit.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/js/diffviewer/models/diffCommit.es6.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

```
Ship It!
```

Change Summary:

Address feedback, update js tests.

Commit:

1a84a4b7cb60b889acea801c98114c7f64baad33

8fa12e0c370f24d41d8efd6bd1367eec2056751c

Diff:

Revision 5 (+64 -37)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/models/diffcommit.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/tests/diffCommitModelTests.es6.js
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Unit test updates from /r/10125/ and /r/10126/

Commit:

8fa12e0c370f24d41d8efd6bd1367eec2056751c

d57abd3f7d3336649bad530b581551459f778668

Diff:

Revision 6 (+75 -42)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/models/diffcommit.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_builtin_fields.py
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/tests/diffCommitModelTests.es6.js
	5 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-4.0.x (fd27c65)