Support filtering FileDiffs in the WebAPI by their commit

Review Request #9757 — Created March 7, 2018 and updated

brennie
Review Board
release-4.0.x
9642
9852
78fba15...
reviewboard

The FileDiff list resource now supports filtering by a commit ID via
passing the ?commit-id=<id> query parameter. Doing so will limit the
results to FileDiff objects who are associated with that commit.
Passing an invalid commit ID will result in no results being returned.

Ran unit tests.

  • 0
  • 0
  • 38
  • 1
  • 39
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
brennie
chipx86
  1. 
      
  2. reviewboard/testing/testcase.py (Diff revision 3)
     
     

    Can you expand this docstring for arguments?

  3. Double backticks here.

  4. HttpRequest

  5. Instead of this method, which must be opted into, I think I'd feel more comfortable if we inversed how these querysets were built.

    get_queryset() should build the initial queryset that handles diffset__revision (which is consistent between the implementations) and the commit filtering.

    It would then call a method defined by the class/subclass for filtering the review request association, like get_diffset_query. That handles the diffset__history__review_request= here, and the diffset__review_request_draft= in DraftFileDiffResource.

    This simplifies implementations and keeps the control in the base get_queryset.

  6. reviewboard/webapi/resources/filediff.py (Diff revision 3)
     
     
     
     
     
     

    Too many blank lines.

  7. reviewboard/webapi/tests/test_draft_filediff.py (Diff revision 3)
     
     
     
     
     
     

    Too many blank lines.

  8. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/testing/testcase.py (Diff revision 4)
     
     

    Diff contents should always be binary.

    This function should assert this.

  3. reviewboard/testing/testcase.py (Diff revision 4)
     
     
     

    Same here.

  4. reviewboard/testing/testcase.py (Diff revision 4)
     
     

    Should be a :py:class: reference.

  5. reviewboard/testing/testcase.py (Diff revision 4)
     
     
     

    Missing "Returns".

  6. reviewboard/testing/testcase.py (Diff revision 4)
     
     

    This should ideally be consistent in implicit vs. explicit comparisons.

    1. Yeah, probably. I was trying to work around an issue I thought was in datetime (but turns out was only in time: see). I'll change this.

  7. Most docs use "The HTTP request from the client."

    1. Every single doc I've written is this way. Also:

      (rb40) ~/W/p/b/r/b/r/s/reviewboard (dev/release-4.0.x/dvcs-webapi)>
      git grep "The current HTTP request." | wc -l
      47
      (rb40) ~/W/p/b/r/b/r/s/reviewboard (dev/release-4.0.x/dvcs-webapi)>
      git grep "The HTTP request from the client." | wc -l
      33
      
  8. reviewboard/webapi/resources/draft_filediff.py (Diff revision 4)
     
     
     
     

    Missing a description.

  9. reviewboard/webapi/resources/filediff.py (Diff revision 4)
     
     
     

    Same description as above.

  10. reviewboard/webapi/resources/filediff.py (Diff revision 4)
     
     
     

    Do we want to allow for empty strings? Maybe just check truthiness?

  11. reviewboard/webapi/resources/filediff.py (Diff revision 4)
     
     
     

    Same as above.

  12. reviewboard/webapi/resources/filediff.py (Diff revision 4)
     
     
     
     

    Missing a description.

  13. Been noticing lately that your test functions are all backwards. The basic case should come first. The variations come after. Can you reorder these to be consistent with all our other tests?

  14. Can you explicitly say "with commit-id=", so it's clear what this is testing?

  15. Should test that the results are what we expect.

  16. reviewboard/webapi/tests/test_filediff.py (Diff revision 4)
     
     
     
     
     
     

    All the same comments from the previous file, regarding docstrings and ordering, apply here as well.

  17. reviewboard/webapi/tests/test_filediff.py (Diff revision 4)
     
     
     

    Should check the results.

  18. 
      
brennie
Review request changed

Change Summary:

Addressed Christian's feedback.

Commit:

-ec58889ea2cc33b5b0b07ef7ef7e3f32084fdd9a
+f984e04689b2f1444ba22a461c4f2158f7c24492

Diff:

Revision 5 (+317 -9)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
david
  1. 
      
  2. "review request revision" is weird

  3. This isn't something the review request resource cares about.

  4. reviewboard/webapi/resources/filediff.py (Diff revision 7)
     
     
     
     

    Doesn't this break? It seems like we're no longer limiting the results to the correct diff revision.

    1. No. If you look above, this method (get_diffset_query) is called by get_queryset, which does the filtering based on the the revision.

      It is refactored out so that draft and non-draft filtering logic can be in get_diffset_query while the main filtering logic lives in get_queryset

  5. 
      
brennie
brennie
david
  1. 
      
  2. This is incorrect, because this method ignores the diffset revision (which is filtered for in the caller).

    Can we just remove the diff_revision parameter entirely from this (since it's not used) and fix the docstring here to explain that it's returning all diffsets linked to the review request?

  3. reviewboard/webapi/resources/filediff.py (Diff revision 9)
     
     
     

    "review request revision" isn't a thing

  4. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/testing/testcase.py (Diff revision 10)
     
     

    Missing a "the".

  3. reviewboard/testing/testcase.py (Diff revision 10)
     
     
     
     
     
     

    This should either be an assert statement or we should raise something like a ValueError. I'd prefer the latter.

  4. reviewboard/testing/testcase.py (Diff revision 10)
     
     
     

    Is the second part of this logic correct? That should trigger if neither is provided (which the assertion says is okay), but won't trigger if only one is provided. Seems this would be more correct/readable:

    if ((committer_name or commiter_email) and
        not (committer_name and committer_email)):
    

    >>> ((True or False) and not (True and False))
    True
    >>> ((False or True) and not (False and True))
    True
    >>> ((True or True) and not (True and True))
    False
    >>> ((False or False) and not (False and False))
    False
    

    Unit tests need to be fleshed out for all conditions.

    1. I fixed the logic. I dont see a need to unit test TestCase.create_x methods

  5. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/webapi/resources/filediff.py (Diff revision 11)
     
     
     

    This isn't in the function signature. It can be removed here.

  3. 
      
brennie
Review request changed

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...