Fix empty review can be published bug.

Review Request #11185 — Created Sept. 19, 2020 and updated

keanweng
Review Board
release-4.0.x
4756
reviewboard, students

When publishing a review, an empty review can be published and it will
show as a blank review.

The publish() function of review has been updated to check for empty
text in a review's Header, Comment and Footer. When a review with empty
Header, Comment and Footer is being published, the function throws a
PublishError. A review with only Ship It and no text is considered
to be valid and will not raise an error. Updated #982 Testing sending
e-mails and filtering out users not on a local site to use a non-empty
Review as we now check for empty reviews.

Ran runtests.py, failed #788 Testing GitHub.authorize. Tested on
browser to ensure the fix works on Web API.

Summary
Fix empty review can be published bug.
Description From Last Updated

We'll want to see two Python-side unit tests that checks the "empty review" state, one with and one without validate_fields …

chipx86chipx86

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

Looks like this was included in this change, unintentionally. You'll want to make sure your commit only has your desired …

chipx86chipx86

Looks like you're setting these because we otherwise have a blank review which can't be published. This code is pretty …

chipx86chipx86

Can you put PublishError before RevokeShipItError? We like to keep things in alphabetical order.

chipx86chipx86

Since you're introducing new arguments, you'll need to update this to reflect our modern doc standards, like "Publish" instead of …

chipx86chipx86

These can be combined into one conditional.

chipx86chipx86

Ah interesting! Looks like you caught an issue we didn't catch before. That usually means "time to write a unit …

chipx86chipx86

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

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

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

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

This isn't part of your change, and should be removed. We have a better fix for this in the code …

daviddavid

There's an extra blank line here.

daviddavid
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

keanweng
chipx86
  1. 
      
  2. We'll want to see two Python-side unit tests that checks the "empty review" state, one with and one without validate_fields set.

  3. reviewboard/hostingsvcs/github.py (Diff revision 2)
     
     

    Looks like this was included in this change, unintentionally. You'll want to make sure your commit only has your desired changes, and post only that commit for review (rbt post HEAD, usually).

  4. reviewboard/notifications/tests/test_email_sending.py (Diff revision 2)
     
     
     
     
     
     
     
     

    Looks like you're setting these because we otherwise have a blank review which can't be published.

    This code is pretty old, and fortunately we have some modern test object creaation helpers that provide standard default data. Would you be up for switching to those instead? It works like:

    self.create_review(review_request=review_request,
                       user=site_user4,
                       publish=True)
    

    Note that publish=True: You can ditch the publish() calls with this.

  5. reviewboard/reviews/models/review.py (Diff revision 2)
     
     

    Can you put PublishError before RevokeShipItError? We like to keep things in alphabetical order.

  6. reviewboard/reviews/models/review.py (Diff revision 2)
     
     
     
     
     
     

    Since you're introducing new arguments, you'll need to update this to reflect our modern doc standards, like "Publish" instead of "Publishes," and an Args and Raises.

    See our doc standards.

  7. reviewboard/reviews/models/review.py (Diff revision 2)
     
     
     
     
     
     
     
     
     

    These can be combined into one conditional.

  8. Ah interesting! Looks like you caught an issue we didn't catch before.

    That usually means "time to write a unit test!" There's a test file for this in the tree that you'll want to ensure is asserting the state of this flag on error.

  9. 
      
keanweng
Review request changed

Change Summary:

Fix 6 issues from Revision 2.

Commits:

Summary
-
Hotfix for GitHub Auth token when adding repo.
-
Add validate_fields to check for empty reviews when publishing.
-
Set review.model.public to false when PublishError.
-
Populate review text as it cannot be empty.
-
Modify validate to check for comments, emails and Ship It.
-
Update indent for Review Bot indent issue.
+
Fix empty review can be published bug.

Diff:

Revision 3 (+158 -24)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

keanweng
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

keanweng
david
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     

    This isn't part of your change, and should be removed. We have a better fix for this in the code now.

  3. There's an extra blank line here.

  4. 
      
keanweng
Review request changed

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
chipx86
  1. 
      
  2. 
      
Loading...