Add an API for commenting on FileDiffs ranges

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

brennie
Review Board
release-4.0.x
10136
10242
b959551...
reviewboard

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

    This would be more concise as:

    return (self.extra_data and
            self.extra_data.get(...))
    
  4. 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. 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. 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. Alphabetical order.

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

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

    This should be one statement.

  12. Typo: "Comapare" -> "Compare"

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

  13. 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. Missing a docstring.

  15. "commit history"

  16. 
      
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

  3. 
      
brennie
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

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