Add a form for uploading DiffCommits and refactor UploadDiffForm

Review Request #9642 — Created Feb. 15, 2018 and updated

Barret Rennie
Review Board
release-4.0.x
9631, 9638
9757
6a97d41...
reviewboard

A new form has been added to upload a DiffCommit and attach it to a
DiffSet.

Ran unit tests.

  • 0
  • 0
  • 16
  • 1
  • 17
Description From Last Updated
David Trowbridge
  1. 
      
  2. reviewboard/diffviewer/managers.py (Diff revision 1)
     
     

    This looks like an unintentional change.

  3. 
      
Barret Rennie
Christian Hammond
  1. 
      
  2. reviewboard/diffviewer/forms.py (Diff revision 2)
     
     

    Can you move this above the clean methods? The clean methods, while not private, are more akin to private methods than clean (which is meant to be used by consumers and should be more prominent).

  3. 
      
Barret Rennie
Barret Rennie
Barret Rennie
David Trowbridge
  1. 
      
  2. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Other labels don't include periods.

  3. 
      
Barret Rennie
Barret Rennie
Review request changed

Change Summary:

  • Move diffset argument into constructor.
  • Handle published diffsets (form validation fails).
  • Handle parent diffs on subsequent commits (form validation fails).
  • Add unit tests for above.

Commit:

-95922560dcbd2e158ef6419c871263969d94f3c1
+3aa1b16cf2bee17e41599f656544ab9ce37999bc

Diff:

Revision 6 (+323 -8)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Barret Rennie
Barret Rennie
Christian Hammond
  1. 
      
  2. reviewboard/diffviewer/forms.py (Diff revision 7)
     
     
     
     

    Do we want to call this commit_message?

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

    Can we use forms.EmailField? (Might hit some issue with values that aren't e-mail fields, so maybe not...)

    If not, maybe we should at least still use EmailInput as the widget.

    Here and the other form below.

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

    This and committer_date should probably use forms.DateTimeField.

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

    This needs to be the full path to the model.

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

    This needs to be the full path to the model.

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

    If using DateTimeField, we shouldn't need these.

  8. reviewboard/diffviewer/tests/test_forms.py (Diff revision 7)
     
     
     
     

    Missing comma at the end.

    Can you move the SimpleUplodaedFile out into its own variable? That'll simplify this dictionary.

  9. No need for a set(), since there's only one item. Probably no need to cast to anything. I imagine it'll iterate.

    1. We do, because it will compare querysets otherwise, not the actual items.

  10. reviewboard/diffviewer/tests/test_forms.py (Diff revision 7)
     
     
     
     
     

    You should be able to pull these out too, and reuse them for both forms.

  11. reviewboard/diffviewer/tests/test_forms.py (Diff revision 7)
     
     
     
     
     
     

    Can you update this to be:

    data=dict({
        'parent_id': 'r1',
        'commit_id': 'r2',
    }, **self._default_form_data)
    

    This follows the pattern we use elsewhere.

    1. This won't overwrite the defaults in data.

      >>> defaults = {'foo': 'foo'}
      >>> dict({'foo': 'bar'}, **defaults)
      {'foo': 'foo'}
      

      It should be dict(defaults, **{overrides here})

  12. reviewboard/diffviewer/tests/test_forms.py (Diff revision 7)
     
     
     
     
     
     

    Same comment here as above.

  13. reviewboard/diffviewer/tests/test_forms.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     

    And these.

  14. 
      
Barret Rennie
Barret Rennie
Barret Rennie
Barret Rennie
Review request changed

Change Summary:

Rebase.
Remove BaseUploadDiffForm since implementation has diverged enough.

Description:

   

A new form has been added to upload a DiffCommit and attach it to a

~   DiffSet. The common functionality (almost all of the implementation)
  ~ DiffSet.

-   with the UploadDiffForm has been refactored out into the
-   BaseUploadDiffForm.

Commit:

-9ae2988be66d334ff0796da4da00faeee7833388
+6a97d4171e4466564daac3ed1a7e307cb9a95749

Diff:

Revision 11 (+336 -12)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...