Fixed the issue of publishing from web API without reviewers or groups specified.

Review Request #9210 - Created Sept. 23, 2017 and updated

Mukhtar Alhejji
Review Board
master
4057
a73e6ad...
reviewboard, students
mukhtar

When creating a new review request with no reviewer or groups
assigned, attempting to publish it with PUT will be sucessful.
However, it shouldn't allow that as review requests must have a
reviewer. The issue is solved by adding conditions to validate the
review draft on the server.

Added a unit test, which passes (along with all other tests).

  • Created a draft test review that don't have a reviewer/group name.
  • Ran it using this command ./tests/runtests.py
    reviewboard.reviews.tests.test_review_request_draft
  • This will raise a publishingError since there isn't reviewer.
  • 0
  • 0
  • 69
  • 6
  • 75
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

Christian Hammond
  1. 
      
  2. The summary doesn't tell me anything. From the summary alone, I should be able to have a clear idea of the purpose of this change. As it is, I have to know what reviewers name issue you're talking about.

    The description also needs a lot more fleshing out. I want to see what the problem was, why it was a problem, how it was fixed. Look at the Writing Good Change Descriptions guide for details on what we expect.

    Content must also wrap to 70-75 characters in your description/testing.

    Testing Done's content needs some work as well. I'd suggest getting a mentor and having him go over ways to make the testing clear and succint.

  3. I'd like to see a little more here with this change. Right now, the validation happens client-side in JavaScript for this field. The client also validates the summary and description. Both of these should also move into the server-side validation in your change (and you should summarize the change appropriately, like "Move UI-side validation for required review request fields into the backend"). You should then get rid of the client-side versions of these so we only have one validation codepath to go through.

  4. Shouldn't add parenthesis here. We only use them if spanning lines.

  5. reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
     
     
     
     
     
     

    This blank line shouldn't be added.

  6. reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
     
     
     
     
     
     

    Too many blank lines.

  7. This should go away.

  8. The web UI currently displays an error message for this case, so we should use that exact string.

  9. reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
     
     
     
     
     
     

    You should remove the blank lines you added here.

  10. reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
     
     
     
     
     
     

    Too many blank lines. There should be 2.

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

    Just one blank line here.

  12. This should test for the error message as well.

  13. reviewboard/webapi/resources/review_request_draft.py (Diff revision 1)
     
     
     
     
     
     

    Too many blank lines. You should remove the ones you added.

  14. 
      
Mukhtar Alhejji
Review request changed

Summary:

-Fixed reviewers name issue
+Fixed the Issue of Publishing From Web API Without Reviewers or Groups Specified

Description:

~  

Fixed the Issue of Publishing From Web API Without Reviewers or Groups Specified

  ~

When creating a new review request with no reviewer or groups assigned, attempting to publish it with PUT .../api/review-requests/####/draft/ public=1 will be sucessful. However, it shouldn't allow that as review requests must have a reviewer. The issue is solved by adding conditions to validate the review draft on the server.

Testing Done:

-  

When user attempt to publish using PUT .../api/review-requests/####/draft/ public=1, the people or group names is not required. The user can submit the review request with no reviewers or groups assigned.

-   There weren't any condition to check for user or group name in the web API. This change checks for people or group name to see if they're entered or not.

-  
   

Added a unit test, which passes (along with all other tests).

   
~  

Tested by creating a draft review request without containing user/group names. After trying to publish it this will raise publish error.

  ~
  • Created a draft test review that don't have a reviewer/group name.
  +
  • Ran it using this command ./tests/runtests.py reviewboard.reviews.tests.test_review_request_draft
  +
  • This will raise a publishingError since there isn't reviewer.

Commit:

-07f33dbc16e7d628d28e8adb606a7c09edd819a5
+7b960f1130a660c49df6dc3086f46ae20a232740

Diff:

Revision 2 (+34 -18)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Mukhtar Alhejji
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Mukhtar Alhejji
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Mukhtar Alhejji
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Mukhtar Alhejji
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Mike Conley
  1. Looking good! Just a few more notes below.

  2. This newline should be removed.

  3. reviewboard/reviews/models/review_request_draft.py (Diff revision 5)
     
     
     
     
     
     

    Glad you added a test for the lack of reviewers, but can you add some tests for the lack of a summary or description too please, to round it out?

  4. These extra newlines should be removed.

  5. This extra newline should be removed.

  6. 
      
Mukhtar Alhejji
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Mukhtar Alhejji
Barret Rennie
  1. A few minor nitpicks.

  2. Please wrap your description at 72 characters.

  3. Instead of including a URL in the description you can just name the resource involved.

  4. Single quotes.

    This should be marked for translation with ugettext.

  5. Single quotes.

    This should be marked for translation with ugettext.

  6. Single quotes.

    This should be marked for translation with ugettext.

  7. 
      
Mukhtar Alhejji
Barret Rennie
  1. 
      
  2. Please wrap your description at 72 characters.

  3. reviewboard/reviews/models/review_request_draft.py (Diff revisions 8 - 9)
     
     
     
     
     
     
     
     
     
     
     

    These need to be marked for translation with ugettext.

  4. 
      
Christian Hammond
  1. 
      
  2. The summary should be in sentence casing.

    1. This was marked as fixed but wasn't done.

  3. This needs to be localized (using ugettext).

  4. reviewboard/reviews/models/review_request_draft.py (Diff revision 9)
     
     
     
     
     

    These as well.

  5. This can be one statement.

  6. Imports must be included alphabetically.

  7. "publish" isn't explicit enough when looking at a whole list of tests. We prefer including the class name. Can you use "ReviewRequestDraft.publish"?

    Also, no blank line separating these.

    These comments apply below as well.

  8. reviewboard/static/rb/js/resources/models/draftReviewRequestModel.es6.js (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    You don't need to define this function anymore, since it's just chaining up.

  9. 
      
Mukhtar Alhejji
Review request changed

Summary:

-Fixed the Issue of Publishing From Web API Without Reviewers or Groups Specified
+Fixed the Issue of Publishing From Web API Without Reviewers or Groups Specified.

Commit:

-7a68c52fdfc99b64b78f3b7863b932bd6994efea
+c0c86a0ca3728366e7cf736d563f466541e0d4ec

Diff:

Revision 10 (+71 -24)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Mukhtar Alhejji
Mukhtar Alhejji
Barret Rennie
  1. 
      
  2. You still need to wrap your description at 72 characters per line.

  3. These can go on the same line.

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

    Undo this.

  5. reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 11)
     
     
     
     
     

    Alphabetical order.

  6. Why are we no longer doing this client side? It save s a round trip.

    1. I used to have both server and client side, but Christian suggested doing this only on the server side.

  7. 
      
Mukhtar Alhejji
Mike Conley
  1. 
      
  2. reviewboard/datagrids/builtin_items.py (Diff revision 12)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    This looks like your project is starting to leak into your EasyFix. You'll want to separate them, and do project development on a separate branch. Ping me in Slack if you need help de-tangling this stuff.

  3. 
      
Mukhtar Alhejji
David Trowbridge
  1. 
      
  2. This doesn't need the parens around it (we only use them if the list of imports wraps to multiple lines).

  3. Parens can go on the same line as the last part of the text.

  4. Grammar isn't quite right--needs to be "isn't a reviewer"

  5. 
      
Brian LeBlanc
  1. 
      
  2. Moving this line doesn't seem to be changing functionality but it is making the imports no longer alphabetically sorted.

  3. This import is also not in alphabetical order (e before f).

  4. This is the test for not having a description but you're checking for the error about not having a summary. This should probably be changed to set a summary on the draft and then the error should be about the description not being there.

  5. 
      
Mukhtar Alhejji
Review request changed

Branch:

-release-3.0.x
+master

Commit:

-ae36187944ab976c1c3dfa85e173aa59f0adef1f
+6d1d668e854ef48bde23dca3a8b6aaa32f8a4d79

Diff:

Revision 14 (+66 -2)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Mukhtar Alhejji
Review request changed
Mukhtar Alhejji
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Mukhtar Alhejji
Mukhtar Alhejji
Review request changed

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...