Add a form for uploading DiffCommits

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

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

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

Ran unit tests.

  • 10
  • 0
  • 16
  • 1
  • 27
Description From Last Updated
Should be "Parent diff", to match labels below. chipx86 chipx86
You can remove the first "only." chipx86 chipx86
I want to strive for more friendly errors, and think we can improve on this just a tad. How about ... chipx86 chipx86
Summary shouldn't have literals. chipx86 chipx86
Let's group together repository and the spy, and then separate out the diffset. Actually, why not have a setUp() that ... chipx86 chipx86
Grouping should be consistent with the above test. Same for the next test. chipx86 chipx86
Do we modify the data passed in? chipx86 chipx86
Let's use update_fields, get just a bit of a speed boost. If we start to do that in more tests, ... chipx86 chipx86
Can you reverse these? chipx86 chipx86
Both of these are separate, even though they're both about dates. They're separate pieces of code with their own logic. ... 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
Review request changed

Change Summary:

Oops

Commit:

-833aa54a17549f6f22f41bf52e2c039381bcf087
+cdf686f830798bb31432b659981093427a51edba

Diff:

Revision 15 (+363 -12)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
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. 
      
Loading...