Require validation for DiffCommits when posting

Review Request #9920 — Created May 9, 2018 and updated

brennie
Review Board
release-4.0.x
9905
9921
b91eaa8...
reviewboard

We now require that diffs for an entire series of commits be validated
before posting to the DraftDiffCommitResource. Validation is performed
by making a series of POST requests to the
ValidateDiffCommitResource, each of which returns validation data to
be included in the next request. The validation data from each request
to the validation resource must then be used to POST to the
DraftDiffCommitResource.

This ensures that we do not upload broken diffs and also allows us to
upload more complex commit series. Previously, only linear commit series
where each commit modified different files were capable of being
uploaded. Now all linear commit series can be uploaded.

Posted /r/9609/ through to this commit to a single review request.
Ran unit tests.

  • 0
  • 0
  • 34
  • 2
  • 36
Description From Last Updated
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
david
  1. 
      
  2. reviewboard/diffviewer/commitutils.py (Diff revision 4)
     
     

    This can wrap.

  3. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Extra period on this line.

  4. reviewboard/diffviewer/managers.py (Diff revision 4)
     
     

    This can be wrapped.

  5. 
      
brennie
chipx86
  1. Apparently I never published this review, which I made a while back. My apologies if comments are pretty stale now. I'll do another as I get through the chain back to this.

  2. Can you rename the file to "commit_utils.py"?

  3. reviewboard/diffviewer/commitutils.py (Diff revision 4)
     
     

    This is way too generic a name. I'd prefer get_file_exists_for_validation.

    Not sure this file is the right place, actually. This is about validation and not commits themselves.

    1. Except this isnt only used for validation. This is used for reviewboard/diffviewer/managers.py as well when we create the actual commits.

  4. reviewboard/diffviewer/commitutils.py (Diff revision 4)
     
     

    You can now split this across lines at a .

  5. reviewboard/diffviewer/filediff_creator.py (Diff revision 4)
     
     
     
     
     
     
     

    Did you mean to unindent the line? Doesn't seem like it would particularly matter in this change.

    1. Yes, I did. In the old behaviour, we only appended to filediffs when we weren't validating and therefore would return an empty list. This makes it so we append to the list so we can instantiate them during validation without creating them in teh db.

  6. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Can you import specifically waht you need from the module, instead of the module itself?

    1. So we are using get_file_exists from that module (and re: my other comment, I don't believe renaming it to what you suggested is suitable), but the code in this module does:

              get_file_exists = partial(commit_utils.get_file_exists,
                                        validation_info or {},
                                        self.repository,
                                        self.cleaned_data['parent_id'])
      
              # ...
      
              return create_filediffs(
                  # ...
                  get_file_exists=get_file_exists,
                  # ...
              )
      

      which would override the global name.

      I personally don't see a big deal in importing the module vs importing bits of it in this case.

    2. It could be _get_file_exists in this case. It's not a big deal, but it's worth noting that we used to import this way from diffutils all over the place and it made it a bit more annoying to maintain. We eventually did a cleanup. I feel like as commit_utils grows, we're likely going to do the same thing.

  7. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     
     
     
     
     

    I imagine this should be a HiddenInput? We don't want users mucking about in it if we display this anywhere.

  8. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    "Validation metadata"

  9. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     
     
     

    The text should specifically be self.fields['validation_info'].error_messages['required']. No need for ugettext.

  10. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Shouldn't include literals in summaries.

  11. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Too many periods.

  12. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    "exceptions"

  13. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Maybe better as:

    if not validation_info:
        return {}
    
    # .. rest of the method
    
  14. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     
     
     

    b64decode can also raise TypeError.

  15. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     
     
     
     
     
     

    The return can be inline.

  16. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Maybe "A form for validating DiffCommits."

  17. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    "Parent diff"

  18. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    "Validation metadata"

  19. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    "Base commit ID"

  20. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    No literals in summaries.

  21. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Too many periods.

  22. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     

    Same comments as above.

    Seems we have some stuff we could easily pull out into a base class?

  23. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     

    This seems like code we have elsewhere. I'd like to deduplicate this if possible.

  24. reviewboard/diffviewer/managers.py (Diff revision 4)
     
     

    We should pull out the specific functions we need.

    1. Again, see previous reply.

  25. reviewboard/diffviewer/managers.py (Diff revision 4)
     
     

    This can be broken into multiple lines now at any .

  26. No literals in summaries.

  27. reviewboard/diffviewer/tests/test_forms.py (Diff revision 4)
     
     
     
     

    Mixing import groups here.

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

    Shouldn't DiffCommit be handling this?

  29. Ordering issues.

  30. The DIFF_* should be in alphabetical order.

  31. 
      
brennie
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed
Loading...