Add an API for commenting on FileDiffs ranges

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

Information

Review Board
release-4.0.x
b959551...

Reviewers

The ReviewDiffCommentResource is now capable of creating comments on
FileDiffs with a base FileDiff ID. This allows us to comment on
FileDiffs in commit ranges and keep the context of the base FileDiff.

Ran unit tests.

Description From Last Updated

E128 continuation line under-indented for visual indent

reviewbotreviewbot

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

reviewbotreviewbot

Seems like this should be "The ID fo the base FileDiff" or "The base FileDiff ID"

chipx86chipx86

This would be more concise as: return (self.extra_data and self.extra_data.get(...))

chipx86chipx86

Mind putting :term: around "cumulative diff?" We'll still need to define it, but it's such an insider term that I …

chipx86chipx86

I don't understand what this means.

chipx86chipx86

We should handle the case of this not existing. I know it should exist, but we've seen cases like this …

chipx86chipx86

Alphabetical order.

chipx86chipx86

Same comments as above with the terms and referencing commit histories.

chipx86chipx86

This really should have a period at the end. I know it didn't before, but most of our errors do. …

chipx86chipx86

This gets pretty deeply nested, and the checking of base_filediff_id here and the setting in the else clause can initially …

chipx86chipx86

This should be one statement.

chipx86chipx86

Typo: "Comapare" -> "Compare" Also, I think the "with" before "response" shouldn't be there.

chipx86chipx86

Let's just compare for truthiness. This is a bit fragile otherwise.

chipx86chipx86

Missing a docstring.

chipx86chipx86

"commit history"

chipx86chipx86

E999 SyntaxError: invalid syntax

reviewbotreviewbot

F401 'reviewboard.diffviewer.models.FileDiff' imported but unused

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

Let's use :term: here for "cumulative diff," since we now have it.

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. 
      
  2. Show all issues

    Seems like this should be "The ID fo the base FileDiff" or "The base FileDiff ID"

  3. reviewboard/reviews/models/diff_comment.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    This would be more concise as:

    return (self.extra_data and
            self.extra_data.get(...))
    
  4. Show all issues

    Mind putting :term: around "cumulative diff?" We'll still need to define it, but it's such an insider term that I think we'll need to if we're going to at all reference it publicly.

    This should probably also mention that it's used for review requests with commit history.

  5. Show all issues

    I don't understand what this means.

    1. Unless I am mistaken, if we GET the list resource, each comment will be serialized and each one will do FileDiff.objects.get(...) which will result in N queries.

      We can't augment the queryset to select_related because the relationship is stored in the JSON field. Thoughts?

    2. Hmm. That is a problem, and will be an issue for larger customers and for RBCommons.

      I have a possible solution in mind, but it's more work. So two questions for now:

      1. Is this needed by consumers, or just nice info to have?
      2. Is this represented as data in the payload, or a link to a resource?
  6. Show all issues

    We should handle the case of this not existing. I know it should exist, but we've seen cases like this before with corruption/hand-modified tables.

  7. Show all issues

    Alphabetical order.

  8. Show all issues

    Same comments as above with the terms and referencing commit histories.

  9. Show all issues

    This really should have a period at the end. I know it didn't before, but most of our errors do.

    All the ones below should as well.

  10. Show all issues

    This gets pretty deeply nested, and the checking of base_filediff_id here and the setting in the else clause can initially be confusing. How about:

    if filediff is None or not dvcs_feaure.is_enabled(request=request):
        base_filediff_id = None
    
    if base_filediff_id is not None:
        ...
    
  11. reviewboard/webapi/resources/review_diff_comment.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This should be one statement.

  12. Show all issues

    Typo: "Comapare" -> "Compare"

    Also, I think the "with" before "response" shouldn't be there.

  13. Show all issues

    Let's just compare for truthiness. This is a bit fragile otherwise.

    1. How, exactly? If it is not None then it will be a dict. In which case we can do an assertNotIn.

      Am I missing something?

    2. Ah, I was reading that as an assertIn(), and was thinking this was assuming that if extra_data was converted to a dict, it would have this content. I felt the None vs. dict was an unsafe assumption. Disregard.

  14. Show all issues

    Missing a docstring.

  15. Show all issues

    "commit history"

  16. 
      
brennie
Review request changed
Change Summary:

Addressed feedback.

Commit:
2669689253ab702a180ed67ac95cc7ff2d7739bd
23bc09a1302c353543e317f1d2ea99b9a3590a8a

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. 
      
  2. Show all issues

    Let's use :term: here for "cumulative diff," since we now have it.

  3. 
      
brennie
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.0.x (d14df94)