Correctly request and return FileDiffs with base FileDiff IDs

Review Request #10245 — Created Oct. 19, 2018 and submitted

Information

Review Board
release-4.0.x
1cd0436...

Reviewers

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.

Ran unit tests.

Description From Last Updated

E201 whitespace after '('

reviewbotreviewbot

Typo: "provded" -> "provided"

chipx86chipx86

No blank line.

chipx86chipx86

assertIsNone

chipx86chipx86

assertIsNone

chipx86chipx86

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

chipx86chipx86

This doesn't seem like a suitable message.

chipx86chipx86

Nor this.

chipx86chipx86

Let's pull the index out. This is a lengthy expression to put into a template literal. Debugging is easier if …

chipx86chipx86

No trailing comma here. (File isn't ES6).

chipx86chipx86

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E113 unexpected indentation

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

Col: 48 Unnecessary semicolon.

reviewbotreviewbot

Col: 47 Missing semicolon.

reviewbotreviewbot

Col: 17 Missing semicolon.

reviewbotreviewbot

Col: 48 Unnecessary semicolon.

reviewbotreviewbot

Col: 47 Missing semicolon.

reviewbotreviewbot

Col: 17 Missing semicolon.

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

Col: 17 Missing semicolon.

reviewbotreviewbot

Col: 47 Missing semicolon.

reviewbotreviewbot

Col: 48 Unnecessary semicolon.

reviewbotreviewbot

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

chipx86chipx86

Typo: "Test sfor" -> "Tests for"

chipx86chipx86

Alphabetical order.

chipx86chipx86

Can you pass this with data= specifically?

chipx86chipx86
brennie
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 3)
     
     

    Typo: "provded" -> "provided"

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

    No blank line.

  4. 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.

    1. 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.

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

    This doesn't seem like a suitable message.

    1. IDK, seems fine to me.

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

    Nor this.

  7. 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.

  8. No trailing comma here. (File isn't ES6).

  9. 
      
brennie
Review request changed
brennie
Review request changed
brennie
Review request changed
brennie
chipx86
  1. 
      
  2. 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.

  3. Typo: "Test sfor" -> "Tests for"

  4. Alphabetical order.

  5. Can you pass this with data= specifically?

  6. 
      
brennie
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (fcce703)
Loading...