Prevent review requests created with history from adding regular diffs

Review Request #9852 — Created April 3, 2018 and submitted

brennie
Review Board
release-4.0.x
9757, 9899
9883
0bd5e31...
reviewboard

We now have a notion of a review request that is created with history
support. A review request created with this flag cannot have a regular
diff attached to it. Instead, an empty DiffSet must be created and
commits attached to that. (Support for which is coming in another
patch.)

This option is now specified at review request creation time and cannot
be changed once the review request is created (i.e., after save() is
called and the primary key has been assigned). It can be specified via
a new field on the review request resources which defaults to the old
(i.e., no history) behaviour.

Ran unit tests.

Description From Last Updated

Needs to be the full path.

chipx86chipx86

Missing ugettext().

chipx86chipx86

Is this going to be seen by end users ever? If so, we shouldn't be talking DiffSets and DiffCommits, and ...

chipx86chipx86

Needs to be the full module path.

chipx86chipx86

This will be just a tad more clear with "... cannot both be set to True".

chipx86chipx86

Missing ending paren.

chipx86chipx86

This would probably be better as: return (self.extra_data and self.extra_data.get(..., False))

chipx86chipx86

Can you also have this check the resulting errors?

chipx86chipx86

This should check the message.

chipx86chipx86

Can you group those create_ flags together?

chipx86chipx86

Spaces around *.

chipx86chipx86

Should this be two tests?

chipx86chipx86

This should check the error.

chipx86chipx86

Here, too.

chipx86chipx86

Should have ~ at the beginning. These are annoyingly long, and means this must be kept up-to-date. Maybe just have ...

chipx86chipx86

We can probably just talk about publishing rather than have a long line for the function reference.

chipx86chipx86

Needs to be the full reference.

chipx86chipx86

Can you have this say explicitly that "This field cannot be set if create_with_history is set" ?

chipx86chipx86

And similar here.

chipx86chipx86

Can you put the create_ arguments together?

chipx86chipx86

Can you have this check the "reason" message?

chipx86chipx86

Two blank lines.

chipx86chipx86

Can you have this check the "reason" message?

chipx86chipx86

This can fit on one line.

chipx86chipx86

This doesn't need to be indented. item_rsp['extra_data'] can fit right after the assertEqual(.

chipx86chipx86

F401 'django.http.HttpRequest' imported but unused

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

F811 redefinition of unused 'test_get_dvcs_feature_disabled' from line 1569

reviewbotreviewbot

Should be ValueError

daviddavid

typo: mulitple

daviddavid

typo: mulitple

daviddavid

typo: mulitple

daviddavid

Missing localization.

chipx86chipx86

"STATUSES"

chipx86chipx86

Extra blank line.

chipx86chipx86

"associated"

chipx86chipx86

This needs to explicitly use ugettext().

chipx86chipx86

Missing a trailing comma. (So is the one above.)

chipx86chipx86

Should be assertFalse

chipx86chipx86

Typo: "creatd" -> "created"

chipx86chipx86

This fits on one line.

chipx86chipx86

Same here.

chipx86chipx86

This should say that one based on the local_site_name attr will be provided if not specified here.

chipx86chipx86

We should capitalize the field names.

chipx86chipx86

This can be split across lines.

chipx86chipx86

This should use :py:attr.

chipx86chipx86
chipx86
  1. 
      
  2. reviewboard/reviews/forms.py (Diff revision 1)
     
     

    Needs to be the full path.

  3. reviewboard/reviews/forms.py (Diff revision 1)
     
     
     
     
     

    Missing ugettext().

  4. reviewboard/reviews/forms.py (Diff revision 1)
     
     
     
     

    Is this going to be seen by end users ever? If so, we shouldn't be talking DiffSets and DiffCommits, and need to rethink what we're telling users.

  5. reviewboard/reviews/managers.py (Diff revision 1)
     
     

    Needs to be the full module path.

  6. reviewboard/reviews/managers.py (Diff revision 1)
     
     
     

    This will be just a tad more clear with "... cannot both be set to True".

  7. Missing ending paren.

  8. reviewboard/reviews/models/review_request.py (Diff revision 1)
     
     
     
     
     
     

    This would probably be better as:

    return (self.extra_data and
            self.extra_data.get(..., False))
    
  9. reviewboard/reviews/tests/test_forms.py (Diff revision 1)
     
     

    Can you also have this check the resulting errors?

  10. This should check the message.

  11. reviewboard/reviews/tests/test_review_request.py (Diff revision 1)
     
     
     
     

    Can you group those create_ flags together?

  12. Spaces around *.

  13. reviewboard/reviews/tests/test_review_request.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Should this be two tests?

  14. This should check the error.

  15. reviewboard/testing/testcase.py (Diff revision 1)
     
     
     
     

    Should have ~ at the beginning.

    These are annoyingly long, and means this must be kept up-to-date. Maybe just have this refer to the list of status on the class (with a reference to it).

    (Also, these would be :py:attr, not :py:data).

  16. reviewboard/testing/testcase.py (Diff revision 1)
     
     
     
     

    We can probably just talk about publishing rather than have a long line for the function reference.

  17. reviewboard/testing/testcase.py (Diff revision 1)
     
     

    Needs to be the full reference.

  18. Can you have this say explicitly that "This field cannot be set if create_with_history is set" ?

  19. And similar here.

  20. Can you put the create_ arguments together?

  21. reviewboard/webapi/tests/test_diff.py (Diff revision 1)
     
     
     

    Can you have this check the "reason" message?

  22. reviewboard/webapi/tests/test_diff.py (Diff revision 1)
     
     
     
     

    Two blank lines.

  23. Can you have this check the "reason" message?

  24. This can fit on one line.

  25. This doesn't need to be indented. item_rsp['extra_data'] can fit right after the assertEqual(.

  26. 
      
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
brennie
brennie
Review request changed

Change Summary:

Hide DVCS fields when the feature is disabled. Do not allow creation of DVCS review requests when feature is disabled.

Commit:

-7e0d7f02b6229b04bfa6847e85a327d5f485aac0
+bc63b1a7f058833236c9eeda538ab31e0dd15897

Diff:

Revision 6 (+573 -22)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
brennie
david
  1. 
      
  2. Should be ValueError

  3. reviewboard/webapi/resources/diff.py (Diff revision 9)
     
     

    typo: mulitple

  4. reviewboard/webapi/tests/test_diff.py (Diff revision 9)
     
     

    typo: mulitple

  5. typo: mulitple

  6. 
      
brennie
brennie
david
  1. Ship It!

  2. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/forms.py (Diff revision 11)
     
     
     
     
     

    Missing localization.

  3. reviewboard/testing/testcase.py (Diff revision 11)
     
     

    "STATUSES"

  4. reviewboard/webapi/tests/test_review_request.py (Diff revision 11)
     
     
     
     
     

    Extra blank line.

  5. 
      
brennie
brennie
brennie
chipx86
  1. 
      
  2. reviewboard/reviews/forms.py (Diff revision 14)
     
     

    "associated"

  3. reviewboard/reviews/forms.py (Diff revision 14)
     
     

    This needs to explicitly use ugettext().

  4. reviewboard/webapi/resources/diff.py (Diff revision 14)
     
     

    Missing a trailing comma. (So is the one above.)

  5. Should be assertFalse

  6. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/reviews/tests/test_forms.py (Diff revision 15)
     
     

    Typo: "creatd" -> "created"

  3. reviewboard/testing/testcase.py (Diff revision 15)
     
     
     

    This fits on one line.

  4. reviewboard/testing/testcase.py (Diff revision 15)
     
     
     

    Same here.

  5. reviewboard/testing/testcase.py (Diff revision 15)
     
     
     

    This should say that one based on the local_site_name attr will be provided if not specified here.

  6. reviewboard/testing/testcase.py (Diff revision 15)
     
     
     
     
     
     
     
     

    We should capitalize the field names.

  7. reviewboard/testing/testcase.py (Diff revision 15)
     
     

    This can be split across lines.

  8. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/testing/testcase.py (Diff revision 16)
     
     

    This should use :py:attr.

  3. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (88ddfca)
Loading...