Add a form for uploading DiffCommits

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

Information

Review Board
release-4.0.x
e50fa30...

Reviewers

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

Ran unit tests.

Description From Last Updated

This looks like an unintentional change.

daviddavid

Can you move this above the clean methods? The clean methods, while not private, are more akin to private methods …

chipx86chipx86

Other labels don't include periods.

daviddavid

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

reviewbotreviewbot

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

reviewbotreviewbot

Do we want to call this commit_message?

chipx86chipx86

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

chipx86chipx86

This and committer_date should probably use forms.DateTimeField.

chipx86chipx86

This needs to be the full path to the model.

chipx86chipx86

This needs to be the full path to the model.

chipx86chipx86

If using DateTimeField, we shouldn't need these.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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.

chipx86chipx86

Same comment here as above.

chipx86chipx86

And these.

chipx86chipx86

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

chipx86chipx86

You can remove the first "only."

chipx86chipx86

I want to strive for more friendly errors, and think we can improve on this just a tad. How about …

chipx86chipx86

Summary shouldn't have literals.

chipx86chipx86

Let's group together repository and the spy, and then separate out the diffset. Actually, why not have a setUp() that …

chipx86chipx86

Grouping should be consistent with the above test. Same for the next test.

chipx86chipx86

Do we modify the data passed in?

chipx86chipx86

Let's use update_fields, get just a bit of a speed boost. If we start to do that in more tests, …

chipx86chipx86

Can you reverse these?

chipx86chipx86

Both of these are separate, even though they're both about dates. They're separate pieces of code with their own logic. …

chipx86chipx86

E501 line too long (80 > 79 characters)

reviewbotreviewbot

datetime.datetime

chipx86chipx86

datetime.datetime

chipx86chipx86

Two blank lines.

chipx86chipx86

I mentioned this before, but I think we want to use list() here, since ordering matters and we want to …

chipx86chipx86
david
  1. 
      
  2. reviewboard/diffviewer/managers.py (Diff revision 1)
     
     
    Show all issues

    This looks like an unintentional change.

  3. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/forms.py (Diff revision 2)
     
     
    Show all issues

    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)
     
     
    Show all issues

    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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/forms.py (Diff revision 7)
     
     
     
     
    Show all issues

    Do we want to call this commit_message?

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

    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)
     
     
    Show all issues

    This and committer_date should probably use forms.DateTimeField.

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

    This needs to be the full path to the model.

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

    This needs to be the full path to the model.

  7. reviewboard/diffviewer/forms.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    If using DateTimeField, we shouldn't need these.

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

    Missing comma at the end.

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

  9. Show all issues

    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)
     
     
     
     
     
    Show all issues

    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)
     
     
     
     
     
     
    Show all issues

    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)
     
     
     
     
     
     
    Show all issues

    Same comment here as above.

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

    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)
     
     
    Show all issues

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

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

    You can remove the first "only."

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

    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)
     
     
    Show all issues

    Summary shouldn't have literals.

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

    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)
     
     
     
     
     
    Show all issues

    Grouping should be consistent with the above test.

    Same for the next test.

  8. Show all issues

    Do we modify the data passed in?

  9. Show all issues

    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. Show all issues

    Can you reverse these?

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

    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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    datetime.datetime

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

    datetime.datetime

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

    Two blank lines.

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

    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:
Completed
Change Summary:
Pushed to release-4.0.x (ea80fb3)