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)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/models/review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (114 > 79 characters)
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
W292 no newline at end of file
```

Commit:

7b960f1130a660c49df6dc3086f46ae20a232740

1d24398b63fcb1d14899bde171dc5b0040ea813c

Diff:

Revision 3 (+40 -18)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/models/review_request_draft.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (114 > 79 characters)
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (106 > 79 characters)
```

Commit:

1d24398b63fcb1d14899bde171dc5b0040ea813c

b0b96286e45e8057e1b1636b0ec8c4f2ee9540b7

Diff:

Revision 4 (+44 -19)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 4)

The issue has been resolved. Show all issues

E501 line too long (106 > 79 characters)

mike_conley Sept. 23, 2017, 12:52 p.m.

Hey Mukhtar, you can address this with something like:

expected_message = (
    'There must be at least one reviewer before this review '
    'request can be published.'
)

Commit:

b0b96286e45e8057e1b1636b0ec8c4f2ee9540b7

a77089f54749bc327013b8d511d78056a6a1db0b

Diff:

Revision 5 (+46 -19)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 5)
The issue has been dropped. Show all issues
```
E999 SyntaxError: EOL while scanning string literal
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 5)
The issue has been dropped. Show all issues
```
W291 trailing whitespace
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 5)
The issue has been dropped. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 5)
The issue has been dropped. Show all issues
```
E124 closing bracket does not match visual indentation
```

Commit:

a77089f54749bc327013b8d511d78056a6a1db0b

be52cc8e8e01c6d95edcd01aa090b273e79cee0d

Diff:

Revision 6 (+46 -19)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 6)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 6)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 6)
The issue has been resolved. Show all issues
```
E124 closing bracket does not match visual indentation
```

Looking good! Just a few more notes below.

reviewboard/reviews/models/review_request_draft.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This newline should be removed.
```

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

The issue has been resolved. Show all issues

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?

reviewboard/reviews/models/review_request_draft.py (Diff revision 5)
The issue has been resolved. Show all issues
```
These extra newlines should be removed.
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This extra newline should be removed.
```

Commit:

be52cc8e8e01c6d95edcd01aa090b273e79cee0d

b6dffa2eeed963a1e942526f08d5a75b7a8270b8

Diff:

Revision 7 (+73 -19)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 7)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 7)
The issue has been resolved. Show all issues
```
E124 closing bracket does not match visual indentation
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 7)
The issue has been resolved. Show all issues
```
W292 no newline at end of file
```

Commit:

b6dffa2eeed963a1e942526f08d5a75b7a8270b8

765f712c52834a891bee580974e60e8a4a913db3

Diff:

Revision 8 (+74 -19)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
A few minor nitpicks.
```

The issue has been resolved. Show all issues

Please wrap your description at 72 characters.

The issue has been resolved. Show all issues

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

reviewboard/reviews/models/review_request_draft.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Single quotes.
This should be marked for translation with ugettext.
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Single quotes.
This should be marked for translation with ugettext.
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Single quotes.
This should be marked for translation with ugettext.
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Undo this.
```

Description:

~		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.
	~	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.

Commit:

765f712c52834a891bee580974e60e8a4a913db3

7a68c52fdfc99b64b78f3b7863b932bd6994efea

Diff:

Revision 9 (+72 -19)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Please wrap your description at 72 characters.

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

The issue has been resolved. Show all issues

These need to be marked for translation with ugettext.

The issue has been resolved. Show all issues

The summary should be in sentence casing.

chipx86 Sept. 29, 2017, 9:48 p.m.

This was marked as fixed but wasn't done.

reviewboard/reviews/models/review_request_draft.py (Diff revision 9)
The issue has been resolved. Show all issues
```
This needs to be localized (using ugettext).
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 9)
The issue has been resolved. Show all issues
```
These as well.
```
reviewboard/reviews/tests/test_review_request.py (Diff revision 9)
The issue has been resolved. Show all issues
```
This can be one statement.
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Imports must be included alphabetically.
```

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 9)

The issue has been resolved. Show all issues

"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.

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

The issue has been resolved. Show all issues

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

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

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 10)
The issue has been resolved. Show all issues
```
E501 line too long (88 > 79 characters)
```

Commit:

c0c86a0ca3728366e7cf736d563f466541e0d4ec

ec6d4d6c7e3f24a2496e0254b428148e29883289

Diff:

Revision 11 (+72 -24)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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.

The issue has been resolved. Show all issues

You still need to wrap your description at 72 characters per line.

reviewboard/reviews/models/review_request_draft.py (Diff revision 11)
The issue has been resolved. Show all issues
```
These can go on the same line.
```
reviewboard/reviews/tests/test_review_request.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Undo this.
```

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

The issue has been resolved. Show all issues

Alphabetical order.

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

The issue has been dropped. Show all issues

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

mukhtar Sept. 30, 2017, 10:33 a.m.

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

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.
	~	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
	~	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:

ec6d4d6c7e3f24a2496e0254b428148e29883289

3d85f20ea55eee23162169c3eca7699558845175

Diff:

Revision 12 (+99 -24)

Show changes

	reviewboard/datagrids/builtin_items.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/datagrids/builtin_items.py (Diff revision 12)

The issue has been resolved. Show all issues

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.

Commit:

3d85f20ea55eee23162169c3eca7699558845175

ae36187944ab976c1c3dfa85e173aa59f0adef1f

Diff:

Revision 13 (+71 -24)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/models/review_request_draft.py (Diff revision 13)

The issue has been resolved. Show all issues

This doesn't need the parens around it (we only use them if the list of imports wraps to multiple lines).

reviewboard/reviews/models/review_request_draft.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Parens can go on the same line as the last part of the text.
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Grammar isn't quite right--needs to be "isn't a reviewer"
```

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

The issue has been resolved. Show all issues

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

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 13)
The issue has been resolved. Show all issues
```
This import is also not in alphabetical order (e before f).
```

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 13)

The issue has been resolved. Show all issues

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.

Branch:

release-3.0.x

master

Commit:

ae36187944ab976c1c3dfa85e173aa59f0adef1f

6d1d668e854ef48bde23dca3a8b6aaa32f8a4d79

Diff:

Revision 14 (+66 -2)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 14)
The issue has been resolved. Show all issues
```
W292 no newline at end of file
```

Diff:

Revision 15 (+66 -2)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 15)
The issue has been resolved. Show all issues
```
W292 no newline at end of file
```

Commit:

6d1d668e854ef48bde23dca3a8b6aaa32f8a4d79

305520b7f15c7d40a6f2056107c9ce642e706804

Diff:

Revision 16 (+67 -2)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 16)
The issue has been resolved. Show all issues
```
W391 blank line at end of file
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 16)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```

Commit:

305520b7f15c7d40a6f2056107c9ce642e706804

21fe322f8e9c810d15f83db8e18c9f02ca5fc1de

Diff:

Revision 17 (+66 -2)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

21fe322f8e9c810d15f83db8e18c9f02ca5fc1de

a73e6ade4f583b68118f4c59560a6b62df3ae35d

Diff:

Revision 18 (+66 -21)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:: Handing off to Maram to drive this over the line. Thanks for your work, Mukhtar, and thanks for taking over, Maram!
Owner:: mukhtar
maram

Change Summary:

Many of the unit tests were failing because they were not adding reviewers, summary, or description to review requests.

Fixed that by adding a boolean parameter in review request creator that defaults to adding reviewers. [WIP]

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. [WIP]

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.

Commit:

a73e6ade4f583b68118f4c59560a6b62df3ae35d

95fefbd0d72c7bbc7128746aaee026123955e539

Diff:

Revision 19 (+98 -27)

Show changes

	reviewboard/attachments/tests.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_counters.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.es6.js
	reviewboard/testing/testcase.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/attachments/tests.py (Diff revision 19)
The issue has been resolved. Show all issues
```
E265 block comment should start with '# '
```
reviewboard/attachments/tests.py (Diff revision 19)
The issue has been resolved. Show all issues
```
E265 block comment should start with '# '
```
reviewboard/attachments/tests.py (Diff revision 19)
The issue has been resolved. Show all issues
```
E265 block comment should start with '# '
```
reviewboard/attachments/tests.py (Diff revision 19)
The issue has been resolved. Show all issues
```
E265 block comment should start with '# '
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 19)
The issue has been resolved. Show all issues
```
E265 block comment should start with '# '
```
reviewboard/reviews/tests/test_counters.py (Diff revision 19)
The issue has been resolved. Show all issues
```
E501 line too long (97 > 79 characters)
```
reviewboard/reviews/tests/test_counters.py (Diff revision 19)
The issue has been resolved. Show all issues
```
E265 block comment should start with '# '
```
reviewboard/reviews/tests/test_counters.py (Diff revision 19)
The issue has been resolved. Show all issues
```
E501 line too long (105 > 79 characters)
```
reviewboard/reviews/tests/test_counters.py (Diff revision 19)
The issue has been resolved. Show all issues
```
E265 block comment should start with '# '
```

Change Summary:

Fixed more unit tests.

Summary:

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

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

Commit:

95fefbd0d72c7bbc7128746aaee026123955e539

297a5d737825968a5bb4baa4451d578188424abc

Diff:

Revision 20 (+152 -80)

Show changes

	reviewboard/attachments/tests.py
	reviewboard/extensions/tests.py
	reviewboard/notifications/tests.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_counters.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/reviews/tests/test_views.py
	6 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Summary:: [WIP] 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.

Summary:: Fixed the issue of publishing from web API without reviewers or groups specified.
[WIP] Fixed the issue of publishing from web API without reviewers or groups specified.

Summary:

[WIP] 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.

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

Commit:

297a5d737825968a5bb4baa4451d578188424abc

f5e16b32982abd8c1c7812c78595cf206945a421

Diff:

Revision 21 (+149 -78)

Show changes

	reviewboard/attachments/tests.py
	reviewboard/extensions/tests.py
	reviewboard/notifications/tests.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_counters.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/reviews/tests/test_views.py
	6 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-3.0.x (2857ab8)