Summary

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

Review Request #9210 — Created Sept. 23, 2017 and submitted Feb. 9, 2018, 4:46 p.m.

Information

Owner

maram

Repository

Review Board

Branch

master

Bugs

4057

Depends On

Commit

f5e16b3...

Reviewers

Groups

reviewboard, students

People

mukhtar

Description

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.

Testing Done

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.
Fixed other unit tests that failed because of the change.

Issues

Description	From	Last Updated
The summary doesn't tell me anything. From the summary alone, I should be able to have a clear idea of …	chipx86	Sept. 23, 2017, 12:28 p.m.
Please wrap your description at 72 characters.	brennie	Sept. 28, 2017, 2:09 p.m.
Instead of including a URL in the description you can just name the resource involved.	brennie	Sept. 28, 2017, 2:11 p.m.
Please wrap your description at 72 characters.	brennie	Sept. 28, 2017, 5:15 p.m.
The summary should be in sentence casing.	chipx86	Sept. 30, 2017, 7:32 a.m.
You still need to wrap your description at 72 characters per line.	brennie	Oct. 3, 2017, 8:14 a.m.
I'd like to see a little more here with this change. Right now, the validation happens client-side in JavaScript for …	chipx86	Dec. 4, 2017, 10:38 a.m.
Shouldn't add parenthesis here. We only use them if spanning lines.	chipx86	Sept. 23, 2017, 9:12 a.m.
This blank line shouldn't be added.	chipx86	Sept. 23, 2017, 9:12 a.m.
Too many blank lines.	chipx86	Sept. 23, 2017, 9:12 a.m.
E265 block comment should start with '# '	reviewbot	Sept. 23, 2017, 9:11 a.m.
E303 too many blank lines (2)	reviewbot	Sept. 23, 2017, 9:12 a.m.
This should go away.	chipx86	Sept. 23, 2017, 9:12 a.m.
The web UI currently displays an error message for this case, so we should use that exact string.	chipx86	Sept. 23, 2017, 9:24 a.m.
You should remove the blank lines you added here.	chipx86	Sept. 23, 2017, 9:13 a.m.
Too many blank lines. There should be 2.	chipx86	Sept. 23, 2017, 9:13 a.m.
E303 too many blank lines (2)	reviewbot	Sept. 23, 2017, 9:11 a.m.
F841 local variable 'old_summary' is assigned to but never used	reviewbot	Sept. 23, 2017, 9:11 a.m.
F841 local variable 'old_description' is assigned to but never used	reviewbot	Sept. 23, 2017, 9:10 a.m.
F841 local variable 'old_testing_done' is assigned to but never used	reviewbot	Sept. 23, 2017, 9:10 a.m.
F841 local variable 'old_branch' is assigned to but never used	reviewbot	Sept. 23, 2017, 9:10 a.m.
F841 local variable 'old_bugs' is assigned to but never used	reviewbot	Sept. 23, 2017, 9:10 a.m.
Just one blank line here.	chipx86	Sept. 23, 2017, 9:13 a.m.
E303 too many blank lines (3)	reviewbot	Sept. 23, 2017, 9:10 a.m.
This should test for the error message as well.	chipx86	Sept. 23, 2017, 12:28 p.m.
W391 blank line at end of file	reviewbot	Sept. 23, 2017, 9:10 a.m.
Too many blank lines. You should remove the ones you added.	chipx86	Sept. 23, 2017, 9:13 a.m.
W291 trailing whitespace	reviewbot	Sept. 23, 2017, 12:32 p.m.
E501 line too long (114 > 79 characters)	reviewbot	Sept. 23, 2017, 12:35 p.m.
W292 no newline at end of file	reviewbot	Sept. 23, 2017, 12:31 p.m.
E501 line too long (114 > 79 characters)	reviewbot	Sept. 23, 2017, 12:45 p.m.
W291 trailing whitespace	reviewbot	Sept. 23, 2017, 12:47 p.m.
E501 line too long (106 > 79 characters)	reviewbot	Sept. 23, 2017, 12:47 p.m.
E501 line too long (106 > 79 characters)	reviewbot	Sept. 23, 2017, 1:14 p.m.
This newline should be removed.	mike_conley	Sept. 23, 2017, 1:25 p.m.
Glad you added a test for the lack of reviewers, but can you add some tests for the lack of …	mike_conley	Sept. 24, 2017, 8:19 a.m.
These extra newlines should be removed.	mike_conley	Sept. 23, 2017, 1:27 p.m.
This extra newline should be removed.	mike_conley	Sept. 23, 2017, 1:27 p.m.
E999 SyntaxError: EOL while scanning string literal	reviewbot	Sept. 23, 2017, 1:16 p.m.
W291 trailing whitespace	reviewbot	Sept. 23, 2017, 1:16 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Sept. 23, 2017, 1:16 p.m.
E124 closing bracket does not match visual indentation	reviewbot	Sept. 23, 2017, 1:16 p.m.
W291 trailing whitespace	reviewbot	Sept. 23, 2017, 1:28 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Sept. 24, 2017, 6:32 a.m.
E124 closing bracket does not match visual indentation	reviewbot	Sept. 24, 2017, 6:30 a.m.
E128 continuation line under-indented for visual indent	reviewbot	Sept. 24, 2017, 8:33 a.m.
E124 closing bracket does not match visual indentation	reviewbot	Sept. 24, 2017, 8:33 a.m.
W292 no newline at end of file	reviewbot	Sept. 24, 2017, 8:23 a.m.
These need to be marked for translation with ugettext.	brennie	Sept. 28, 2017, 6:07 p.m.
Single quotes. This should be marked for translation with ugettext.	brennie	Sept. 28, 2017, 2:09 p.m.
Single quotes. This should be marked for translation with ugettext.	brennie	Sept. 28, 2017, 2:08 p.m.
Single quotes. This should be marked for translation with ugettext.	brennie	Sept. 28, 2017, 2:08 p.m.
Undo this.	brennie	Sept. 28, 2017, 2:07 p.m.
This needs to be localized (using ugettext).	chipx86	Sept. 28, 2017, 6:07 p.m.
These as well.	chipx86	Sept. 28, 2017, 6:07 p.m.
This can be one statement.	chipx86	Sept. 28, 2017, 6:20 p.m.
Imports must be included alphabetically.	chipx86	Sept. 28, 2017, 6:21 p.m.
"publish" isn't explicit enough when looking at a whole list of tests. We prefer including the class name. Can you …	chipx86	Sept. 28, 2017, 6:23 p.m.
You don't need to define this function anymore, since it's just chaining up.	chipx86	Sept. 28, 2017, 6:27 p.m.
E501 line too long (88 > 79 characters)	reviewbot	Sept. 28, 2017, 6:32 p.m.
These can go on the same line.	brennie	Oct. 3, 2017, 8:28 a.m.
Undo this.	brennie	Oct. 3, 2017, 8:38 a.m.
Alphabetical order.	brennie	Oct. 3, 2017, 8:38 a.m.
Why are we no longer doing this client side? It save s a round trip.	brennie	Sept. 30, 2017, 3:27 p.m.
This looks like your project is starting to leak into your EasyFix. You'll want to separate them, and do project …	mike_conley	Oct. 4, 2017, 10:03 a.m.
This doesn't need the parens around it (we only use them if the list of imports wraps to multiple lines).	david	Dec. 2, 2017, 7:26 a.m.
Parens can go on the same line as the last part of the text.	david	Dec. 2, 2017, 7:27 a.m.
Moving this line doesn't seem to be changing functionality but it is making the imports no longer alphabetically sorted.	bleblan2	Dec. 2, 2017, 7:44 a.m.
This import is also not in alphabetical order (e before f).	bleblan2	Dec. 2, 2017, 7:44 a.m.
Grammar isn't quite right--needs to be "isn't a reviewer"	david	Dec. 2, 2017, 7:46 a.m.
This is the test for not having a description but you're checking for the error about not having a summary. …	bleblan2	Dec. 2, 2017, 7:45 a.m.
W292 no newline at end of file	reviewbot	Dec. 2, 2017, 7:48 a.m.
W292 no newline at end of file	reviewbot	Dec. 2, 2017, 7:50 a.m.
W391 blank line at end of file	reviewbot	Dec. 2, 2017, 7:51 a.m.
W293 blank line contains whitespace	reviewbot	Dec. 2, 2017, 7:51 a.m.
E265 block comment should start with '# '	reviewbot	Feb. 2, 2018, 10:04 a.m.
E265 block comment should start with '# '	reviewbot	Feb. 2, 2018, 10:04 a.m.
E265 block comment should start with '# '	reviewbot	Feb. 2, 2018, 10:04 a.m.
E265 block comment should start with '# '	reviewbot	Feb. 2, 2018, 10:04 a.m.
E265 block comment should start with '# '	reviewbot	Feb. 2, 2018, 10:04 a.m.
E501 line too long (97 > 79 characters)	reviewbot	Feb. 2, 2018, 10:04 a.m.
E265 block comment should start with '# '	reviewbot	Feb. 2, 2018, 10:04 a.m.
E501 line too long (105 > 79 characters)	reviewbot	Feb. 2, 2018, 10:04 a.m.
E265 block comment should start with '# '	reviewbot	Feb. 2, 2018, 10:04 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E265 block comment should start with '# '
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'old_summary' is assigned to but never used
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'old_description' is assigned to but never used
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'old_testing_done' is assigned to but never used
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'old_branch' is assigned to but never used
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'old_bugs' is assigned to but never used
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E303 too many blank lines (3)
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W391 blank line at end of file
```

The issue has been resolved. Show all issues

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.

The issue has been resolved. Show all issues

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.

reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Shouldn't add parenthesis here. We only use them if spanning lines.
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This blank line shouldn't be added.
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Too many blank lines.
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This should go away.
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
The web UI currently displays an error message for this case, so we should use that exact string.
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
You should remove the blank lines you added here.
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Too many blank lines. There should be 2.
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Just one blank line here.
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This should test for the error message as well.
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Too many blank lines. You should remove the ones you added.
```

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)

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

Information

Reviewers

flake8

Summary:

Description:

Testing Done:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (2 succeeded)

Description:

Commit:

Diff:

Checks run (2 succeeded)

Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (2 succeeded)

Summary:

Description:

Testing Done:

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Branch:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Owner:

Change Summary:

Summary:

Testing Done:

Commit:

Diff: