Summary

Correctly request and return FileDiffs with base FileDiff IDs

Review Request #10245 — Created Oct. 19, 2018 and submitted Oct. 25, 2018, 10:28 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

~~10233~~

Commit

1cd0436...

Reviewers

Groups

reviewboard

People

Description

The logic to request a diff fragment with a base FileDiff ID was never

actually implemented. The RB.DiffReviewable model generated a URL with

an invalid query string and, even if it were valid, the

DiffFragmentView did not know how to ask for a single FileDiff with a

base FileDiff.
The RB.DiffReviewable model now generates a correct URL and the

DiffFragmentView can parse this into a call into get_diff_files with

the appropriate parameters.
Additionally, the base FileDiff determination in get_diff_files was

incorrect: it returned an ancestor that was too old. This has been

corrected to return the least recent FileDiff from after the specified

base commit.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
E201 whitespace after '('	reviewbot	Oct. 23, 2018, 7:03 p.m.
Typo: "provded" -> "provided"	chipx86	Oct. 24, 2018, 3:49 p.m.
No blank line.	chipx86	Oct. 24, 2018, 3:50 p.m.
assertIsNone	chipx86	Oct. 24, 2018, 3:50 p.m.
assertIsNone	chipx86	Oct. 24, 2018, 3:50 p.m.
Do we have proper checks and test coverage here? It's crucial that nobody can get to other people's diffs through …	chipx86	Oct. 24, 2018, 7:40 p.m.
This doesn't seem like a suitable message.	chipx86	Oct. 24, 2018, 4:04 p.m.
Nor this.	chipx86	Oct. 24, 2018, 4:04 p.m.
Let's pull the index out. This is a lengthy expression to put into a template literal. Debugging is easier if …	chipx86	Oct. 24, 2018, 3:56 p.m.
No trailing comma here. (File isn't ES6).	chipx86	Oct. 24, 2018, 3:51 p.m.
E999 SyntaxError: invalid syntax	reviewbot	Oct. 25, 2018, 9:17 p.m.
E113 unexpected indentation	reviewbot	Oct. 25, 2018, 9:17 p.m.
W391 blank line at end of file	reviewbot	Oct. 25, 2018, 9:17 p.m.
Col: 48 Unnecessary semicolon.	reviewbot	Oct. 25, 2018, 9:17 p.m.
Col: 47 Missing semicolon.	reviewbot	Oct. 25, 2018, 9:17 p.m.
Col: 17 Missing semicolon.	reviewbot	Oct. 25, 2018, 9:17 p.m.
Col: 48 Unnecessary semicolon.	reviewbot	Oct. 25, 2018, 9:15 p.m.
Col: 47 Missing semicolon.	reviewbot	Oct. 25, 2018, 9:15 p.m.
Col: 17 Missing semicolon.	reviewbot	Oct. 25, 2018, 9:15 p.m.
W391 blank line at end of file	reviewbot	Oct. 25, 2018, 9:44 p.m.
Col: 17 Missing semicolon.	reviewbot	Oct. 25, 2018, 9:16 p.m.
Col: 47 Missing semicolon.	reviewbot	Oct. 25, 2018, 9:16 p.m.
Col: 48 Unnecessary semicolon.	reviewbot	Oct. 25, 2018, 9:16 p.m.
Let's use the IDs instead of %r, to avoid any chance of leaking details on filenames to those who shouldn't …	chipx86	Oct. 25, 2018, 9:58 p.m.
Typo: "Test sfor" -> "Tests for"	chipx86	Oct. 25, 2018, 9:58 p.m.
Alphabetical order.	chipx86	Oct. 25, 2018, 9:59 p.m.
Can you pass this with data= specifically?	chipx86	Oct. 25, 2018, 9:59 p.m.

flake8 passed.

JSHint passed.

Change Summary:

Fixed unit tests. Added JS test.

Summary:

-	WIP: Correctly request and return FileDiffs with base FileDiff IDs
+	Correctly request and return FileDiffs with base FileDiff IDs

Testing Done:

		Ran unit tests.
-
-		TODO: Add more unit tests
-		TODO: Add more JS tests.

Commit:

-	c948c3912e33581c77b79af82cdf365a46fb2663
+	fa0439896578d7f7c60c592d176d102ebcf92d15

Diff:

Revision 2 (+103 -52)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E201 whitespace after '('
```

Commit:

-	fa0439896578d7f7c60c592d176d102ebcf92d15
+	48fcd287b1fd201293b4d7895f5d7134198e2bb1

Diff:

Revision 3 (+103 -52)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/diffutils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Typo: "provded" -> "provided"
```
reviewboard/diffviewer/diffutils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
No blank line.
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
assertIsNone
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
assertIsNone
```

reviewboard/diffviewer/views.py (Diff revision 3)

The issue has been resolved. Show all issues

Do we have proper checks and test coverage here? It's crucial that nobody can get to other people's diffs through this.

brennie Oct. 24, 2018, 10:20 a.m.

We do have proper checks in place (line 619):

        elif base_filediff_id:
            interfilediff = None
            base_filediff = get_object_or_404(FileDiff, pk=base_filediff_id,
                                              diffset=diffset)


but I will add some test coverage.

reviewboard/diffviewer/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This doesn't seem like a suitable message.
```
1. brennie Oct. 24, 2018, 10:20 a.m.
  IDK, seems fine to me.
reviewboard/diffviewer/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Nor this.
```

reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 3)

The issue has been resolved. Show all issues

Let's pull the index out. This is a lengthy expression to put into a template literal. Debugging is easier if we keep the literals very small.

reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js (Diff revision 3)
The issue has been resolved. Show all issues
```
No trailing comma here. (File isn't ES6).
```

Commit:

-	48fcd287b1fd201293b4d7895f5d7134198e2bb1
+	330e7fe43a3b6628fa6aba4d35a1eb524d89e75a

Diff:

Revision 4 (+144 -52)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/tests/test_reviews_diff_viewer_view.py
	reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js

Checks run (2 failed)

flake8 failed.

JSHint failed.

flake8

reviewboard/reviews/tests/test_reviews_diff_viewer_view.py (Diff revision 4)
The issue has been dropped. Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/reviews/tests/test_reviews_diff_viewer_view.py (Diff revision 4)
The issue has been dropped. Show all issues
```
E113 unexpected indentation
```
reviewboard/reviews/tests/test_reviews_diff_viewer_view.py (Diff revision 4)
The issue has been dropped. Show all issues
```
W391 blank line at end of file
```

JSHint

reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 48
 Unnecessary semicolon.
```
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 47
 Missing semicolon.
```
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 17
 Missing semicolon.
```

Commit:

-	330e7fe43a3b6628fa6aba4d35a1eb524d89e75a
+	a789b9e03e6231f7bd3a1749348a05bc567ca5a6

Diff:

Revision 5 (+110 -52)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 48
 Unnecessary semicolon.
```
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 47
 Missing semicolon.
```
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 17
 Missing semicolon.
```

Commit:

-	a789b9e03e6231f7bd3a1749348a05bc567ca5a6
+	bd7adc68819449a9bf089dd1843d9a95a9601a93

Diff:

Revision 6 (+271 -52)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/tests/test_reviews_diff_fragment_view.py
	reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js

Checks run (2 failed)

flake8 failed.

JSHint failed.

flake8

reviewboard/reviews/tests/test_reviews_diff_fragment_view.py (Diff revision 6)
The issue has been resolved. Show all issues
```
W391 blank line at end of file
```

JSHint

reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 17
 Missing semicolon.
```
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 47
 Missing semicolon.
```
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 48
 Unnecessary semicolon.
```

Commit:

-	bd7adc68819449a9bf089dd1843d9a95a9601a93
+	57c9cc3419e02c21dea5b71df72b605eda99fe26

Diff:

Revision 7 (+261 -52)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/tests/test_reviews_diff_fragment_view.py
	reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

The issue has been resolved. Show all issues

Let's use the IDs instead of %r, to avoid any chance of leaking details on filenames to those who shouldn't know them.

reviewboard/reviews/tests/test_reviews_diff_fragment_view.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Typo: "Test sfor" -> "Tests for"
```
reviewboard/reviews/tests/test_reviews_diff_fragment_view.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Alphabetical order.
```
reviewboard/reviews/tests/test_reviews_diff_fragment_view.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Can you pass this with data= specifically?
```

Change Summary:

Addressed feedback.

Commit:

-	57c9cc3419e02c21dea5b71df72b605eda99fe26
+	1cd0436c4c197d7200f6136fdea8634d3ecfb8af

Diff:

Revision 8 (+258 -52)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/tests/test_reviews_diff_fragment_view.py
	reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (fcce703)