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.

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)
     
     

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