Correctly request and return FileDiffs with base FileDiff IDs
Review Request #10245 — Created Oct. 19, 2018 and submitted
The logic to request a diff fragment with a base FileDiff ID was never
actually implemented. TheRB.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 intoget_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.
Ran unit tests.
Description | From | Last Updated |
---|---|---|
E201 whitespace after '(' |
![]() |
|
Typo: "provded" -> "provided" |
|
|
No blank line. |
|
|
assertIsNone |
|
|
assertIsNone |
|
|
Do we have proper checks and test coverage here? It's crucial that nobody can get to other people's diffs through … |
|
|
This doesn't seem like a suitable message. |
|
|
Nor this. |
|
|
Let's pull the index out. This is a lengthy expression to put into a template literal. Debugging is easier if … |
|
|
No trailing comma here. (File isn't ES6). |
|
|
E999 SyntaxError: invalid syntax |
![]() |
|
E113 unexpected indentation |
![]() |
|
W391 blank line at end of file |
![]() |
|
Col: 48 Unnecessary semicolon. |
![]() |
|
Col: 47 Missing semicolon. |
![]() |
|
Col: 17 Missing semicolon. |
![]() |
|
Col: 48 Unnecessary semicolon. |
![]() |
|
Col: 47 Missing semicolon. |
![]() |
|
Col: 17 Missing semicolon. |
![]() |
|
W391 blank line at end of file |
![]() |
|
Col: 17 Missing semicolon. |
![]() |
|
Col: 47 Missing semicolon. |
![]() |
|
Col: 48 Unnecessary semicolon. |
![]() |
|
Let's use the IDs instead of %r, to avoid any chance of leaking details on filenames to those who shouldn't … |
|
|
Typo: "Test sfor" -> "Tests for" |
|
|
Alphabetical order. |
|
|
Can you pass this with data= specifically? |
|
Change Summary:
Fixed unit tests. Added JS test.
Summary: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||
Commit: |
|
||||||||||||
Diff: |
Revision 2 (+103 -52) |
Checks run (1 failed, 1 succeeded)
flake8
-
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 2) Show all issues
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+103 -52) |
Checks run (2 succeeded)
-
-
-
-
-
-
reviewboard/diffviewer/views.py (Diff revision 3) Do we have proper checks and test coverage here? It's crucial that nobody can get to other people's diffs through this.
-
-
-
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 3) 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.
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+144 -52) |
Checks run (2 failed)
flake8
-
reviewboard/reviews/tests/test_reviews_diff_viewer_view.py (Diff revision 4) E999 SyntaxError: invalid syntax
-
reviewboard/reviews/tests/test_reviews_diff_viewer_view.py (Diff revision 4) E113 unexpected indentation
-
reviewboard/reviews/tests/test_reviews_diff_viewer_view.py (Diff revision 4) W391 blank line at end of file
JSHint
-
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 4) Col: 48 Unnecessary semicolon.
-
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 4) Col: 47 Missing semicolon.
-
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 4) Col: 17 Missing semicolon.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+110 -52) |
Checks run (1 failed, 1 succeeded)
JSHint
-
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 5) Col: 48 Unnecessary semicolon.
-
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 5) Col: 47 Missing semicolon.
-
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 5) Col: 17 Missing semicolon.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+271 -52) |
Checks run (2 failed)
flake8
-
reviewboard/reviews/tests/test_reviews_diff_fragment_view.py (Diff revision 6) W391 blank line at end of file
JSHint
-
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 6) Col: 17 Missing semicolon.
-
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 6) Col: 47 Missing semicolon.
-
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js (Diff revision 6) Col: 48 Unnecessary semicolon.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+261 -52) |
Checks run (2 succeeded)
-
-
reviewboard/diffviewer/diffutils.py (Diff revision 7) 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) Typo: "Test sfor" -> "Tests for"
-
-
reviewboard/reviews/tests/test_reviews_diff_fragment_view.py (Diff revision 7) Can you pass this with
data=
specifically?
Change Summary:
Addressed feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+258 -52) |