Fix empty review can be published bug.

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

Information

Review Board
release-4.0.x

Reviewers

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 ID
Fix empty review can be published bug.
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. Testing Done: Ran runtests.py, failed #788 Testing GitHub.authorize. Tested on browser to ensure the fix works on Web API.
70a42fa7cb8c3452ee7c300742c4a8ca5f8539aa
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. Show all issues

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

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

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

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

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

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

    These can be combined into one conditional.

  8. Show all issues

    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 ID
Hotfix for GitHub Auth token when adding repo.
ee3f1f85814ea025b3a6dff616eacab7b6dc5c3f
Add validate_fields to check for empty reviews when publishing.
88eb60a7bbb0e8bf98bf3c12da079154a53bb56f
Set review.model.public to false when PublishError.
90fb63ac2d03a030874c8020a460783975abc827
Populate review text as it cannot be empty.
ea074c9d6f6d69b44cdcdab535b012b87bb08aa1
Modify validate to check for comments, emails and Ship It.
4a12eb1e06d440ba1ba8bab71436871f1904a4eb
Update indent for Review Bot indent issue.
47e132043bf149a56df183d1af490e0a5532ef78
Fix empty review can be published bug.
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. Testing Done: Ran runtests.py, failed #788 Testing GitHub.authorize. Tested on browser to ensure the fix works on Web API.
95f5466dc4edf1d491d61ad91b0819b62b42796f

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

keanweng
Review request changed
Change Summary:

Jasmine trigger error change.

Commits:
Summary ID
Fix empty review can be published bug.
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. Testing Done: Ran runtests.py, failed #788 Testing GitHub.authorize. Tested on browser to ensure the fix works on Web API.
95f5466dc4edf1d491d61ad91b0819b62b42796f
Fix empty review can be published bug.
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. Testing Done: Ran runtests.py, failed #788 Testing GitHub.authorize. Tested on browser to ensure the fix works on Web API.
95f5466dc4edf1d491d61ad91b0819b62b42796f
Jasmine trigger error on calling publish.
2d3b5ca4a165fe0646f366a182e93d9d100d12e1

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

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

  3. Show all issues

    There's an extra blank line here.

  4. 
      
keanweng
Review request changed
Change Summary:

Address feedback.

Commits:
Summary ID
Fix empty review can be published bug.
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. Testing Done: Ran runtests.py, failed #788 Testing GitHub.authorize. Tested on browser to ensure the fix works on Web API.
158738ba093ee9380d001634020de30675d9ecc6
Fix empty review can be published bug.
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. Testing Done: Ran runtests.py, failed #788 Testing GitHub.authorize. Tested on browser to ensure the fix works on Web API.
70a42fa7cb8c3452ee7c300742c4a8ca5f8539aa

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
chipx86
  1. 
      
  2.