Add a form for uploading DiffCommits

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

brennie
Review Board
release-4.0.x
9631, 9898, 9638
9757
e50fa30...
reviewboard

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

Ran unit tests.

  • 1
  • 0
  • 30
  • 1
  • 32
Description From Last Updated
I mentioned this before, but I think we want to use list() here, since ordering matters and we want to ... chipx86 chipx86
david
  1. 
      
  2. reviewboard/diffviewer/managers.py (Diff revision 1)
     
     

    This looks like an unintentional change.

  3. 
      
brennie
chipx86
  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. 
      
brennie
brennie
brennie
david
  1. 
      
  2. reviewboard/diffviewer/forms.py (Diff revision 4)
     
     

    Other labels don't include periods.

  3. 
      
brennie
brennie
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

brennie
brennie
chipx86
  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.

    2. list() is better then. We shouldn't use set() unless order truly never matters. If we were to update this to test with multiple files, we'd want to know if order ever somehow changed.

  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. 
      
brennie
brennie
brennie
brennie
brennie
chipx86
  1. Ship It!

  2. 
      
brennie
brennie
brennie
brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/forms.py (Diff revision 15)
     
     

    Should be "Parent diff", to match labels below.

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

    You can remove the first "only."

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

    I want to strive for more friendly errors, and think we can improve on this just a tad. How about simply: "This date must be in ISO 8601 format."

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

    Summary shouldn't have literals.

  6. reviewboard/diffviewer/tests/test_forms.py (Diff revision 15)
     
     
     
     
     
     

    Let's group together repository and the spy, and then separate out the diffset.

    Actually, why not have a setUp() that handles these objects? We're repeating ourselves a lot.

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

    Grouping should be consistent with the above test.

    Same for the next test.

  8. Do we modify the data passed in?

  9. Let's use update_fields, get just a bit of a speed boost. If we start to do that in more tests, we'll speed up the test suite.

  10. Can you reverse these?

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

    Both of these are separate, even though they're both about dates. They're separate pieces of code with their own logic. The docstring even really acknowledges this. We should break this into two tests.

  12. 
      
brennie
Review request changed

Commit:

-cdf686f830798bb31432b659981093427a51edba
+f5385c32f09db3759c50cecd4aa6a419422bd114

Diff:

Revision 16 (+362 -12)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/forms.py (Diff revision 17)
     
     

    datetime.datetime

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

    datetime.datetime

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

    Two blank lines.

  5. 
      
brennie
brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/tests/test_forms.py (Diff revision 19)
     
     
     

    I mentioned this before, but I think we want to use list() here, since ordering matters and we want to know if that ever breaks.

  3. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

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