Support filtering FileDiffs in the WebAPI by their commit

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

Information

Review Board
release-4.0.x
78fba15...

Reviewers

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.

Description From Last Updated

E501 line too long (87 > 79 characters)

reviewbotreviewbot

Can you expand this docstring for arguments?

chipx86chipx86

HttpRequest

chipx86chipx86

Double backticks here.

chipx86chipx86

HttpRequest

chipx86chipx86

Instead of this method, which must be opted into, I think I'd feel more comfortable if we inversed how these …

chipx86chipx86

Too many blank lines.

chipx86chipx86

Too many blank lines.

chipx86chipx86

Diff contents should always be binary. This function should assert this.

chipx86chipx86

Same here.

chipx86chipx86

Should be a :py:class: reference.

chipx86chipx86

Missing "Returns".

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Missing a description.

chipx86chipx86

Same description as above.

chipx86chipx86

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

chipx86chipx86

Same as above.

chipx86chipx86

Missing a description.

chipx86chipx86

Been noticing lately that your test functions are all backwards. The basic case should come first. The variations come after. …

chipx86chipx86

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

chipx86chipx86

Here, too.

chipx86chipx86

Should test that the results are what we expect.

chipx86chipx86

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

chipx86chipx86

Should check the results.

chipx86chipx86

F841 local variable 'pk' is assigned to but never used

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

F841 local variable 'pk' is assigned to but never used

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

"review request revision" is weird

daviddavid

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

daviddavid

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

daviddavid

This is incorrect, because this method ignores the diffset revision (which is filtered for in the caller). Can we just …

daviddavid

"review request revision" isn't a thing

daviddavid

Missing a "the".

chipx86chipx86

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

chipx86chipx86

Is the second part of this logic correct? That should trigger if neither is provided (which the assertion says is …

chipx86chipx86

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

chipx86chipx86
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
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (4ca418f)
Loading...