Summary

Guarded against an inactive user being set as a reviewer or owner of a review request

Review Request #11422 — Created Jan. 30, 2021 and updated March 28, 2021, 8:35 p.m.

Information

Owner

mderose

Repository

Review Board

Branch

release-4.0.x

Bugs

4896

Depends On

Reviewers

Groups

reviewboard, students

People

Description

A change that prevents an inactive user to be assigned as a submitter or reviewer for a
unpublished review request. In addition to this, this change will prevent
a review request draft from being published if the submitter or reviewer(s) are inactive.

Testing Done

I've manually validated in the UI for draft review request that an error appears if a user
tries to update the owner/submitter or target people (reviewers) field with an innactive
user

-I've created unit tests for when a review request draft is published for the following
cases
1. A review request draft is published with an inactive reviewer
2. A review request draft is published with an inactive owner/submitter
3. A review request draft is published with multiple reviewers, one of which is inactive

-I've also created additional unit tests regarrding when a review request draft is updated
using a PUT request
5. A PUT API call is made to update a review request draft with an inactive user set as the
owner/submitter of the review request
7. A PUT API call is made to update a review request draft with an inactive user set as one
8. of the reviewers of the review request

Commits

Summary	ID	Author
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	77e175ff12c5c098ec93c8558d6dae2727f06eb6	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	89d6d608750c529824fc00cce880cf77cbe7ff6d	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	634e91d9a34c6939dfcbf2bf5c358be12b10067c	mderose123
Addendum Fixed Flake8 Issues Again	92d0dbb9c1deccdeb6a758b27e377048658e6509	mderose123
Added tests for handling put requests with for updating submitter or target people with inactive users	6511fc0ef5068540042210a266bdfe6cdd28859d	mderose123
Addendum removed whitespace as recommended by flake8	422334836d31a49b09658c3b4151cdb89960e428	mderose123
Addendum fixed flake8 code styling issues	1aca18a7075644c9043ae48365e7e720031f2c08	mderose123
Addendum added changes as perDavid's recommendations	60348fa225a045f5c33d56ff5ada4c303702af85	mderose123
Addendum made changes based off of flake8 recommendations	ec40c4934a492758547a73cf9c8bf8c18c765203	mderose123
Addendum removed autoindentations added from IDE	4b0ae5cdf93a629aab9d4b3a27fffb1eb0046d13	mderose123
Addendum fixed over-indentation error raised by flake8	f5f6c7ca54da7bc917503e4a2ecaef991b9558ea	mderose123

Issues

Description	From	Last Updated
W293 blank line contains whitespace	reviewbot	Feb. 6, 2021, 12:25 p.m.
E712 comparison to False should be 'if cond is False:' or 'if not cond:'	reviewbot	Feb. 6, 2021, 12:25 p.m.
E501 line too long (96 > 79 characters)	reviewbot	Feb. 6, 2021, 12:25 p.m.
F812 list comprehension redefines 'user' from line 341	reviewbot	Feb. 6, 2021, 12:25 p.m.
E202 whitespace before ')'	reviewbot	Feb. 6, 2021, 12:25 p.m.
W293 blank line contains whitespace	reviewbot	Feb. 6, 2021, 12:25 p.m.
E501 line too long (85 > 79 characters)	reviewbot	Feb. 6, 2021, 12:25 p.m.
E501 line too long (86 > 79 characters)	reviewbot	Feb. 6, 2021, 12:25 p.m.
W293 blank line contains whitespace	reviewbot	Feb. 6, 2021, 12:25 p.m.
E711 comparison to None should be 'if cond is not None:'	reviewbot	Feb. 6, 2021, 12:25 p.m.
E712 comparison to False should be 'if cond is False:' or 'if not cond:'	reviewbot	Feb. 6, 2021, 12:25 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Feb. 6, 2021, 12:25 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Feb. 6, 2021, 12:25 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Feb. 6, 2021, 12:25 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 12:25 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 12:25 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Feb. 6, 2021, 12:25 p.m.
E127 continuation line over-indented for visual indent	reviewbot	Feb. 6, 2021, 12:25 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 12:25 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Feb. 6, 2021, 12:25 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 12:26 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Feb. 6, 2021, 12:26 p.m.
E127 continuation line over-indented for visual indent	reviewbot	Feb. 6, 2021, 12:26 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Feb. 6, 2021, 12:26 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 12:26 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Feb. 6, 2021, 12:26 p.m.
E127 continuation line over-indented for visual indent	reviewbot	Feb. 6, 2021, 12:26 p.m.
W293 blank line contains whitespace	reviewbot	Feb. 6, 2021, 12:26 p.m.
W293 blank line contains whitespace	reviewbot	Feb. 6, 2021, 12:26 p.m.
F812 list comprehension redefines 'user' from line 1042	reviewbot	Feb. 6, 2021, 12:26 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 12:26 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Feb. 6, 2021, 12:26 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 12:26 p.m.
W293 blank line contains whitespace	reviewbot	Feb. 6, 2021, 12:26 p.m.
E999 SyntaxError: EOL while scanning string literal	reviewbot	Feb. 6, 2021, 12:26 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 12:26 p.m.
E126 continuation line over-indented for hanging indent	reviewbot	Feb. 6, 2021, 12:26 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 12:26 p.m.
E123 closing bracket does not match indentation of opening bracket's line	reviewbot	Feb. 6, 2021, 12:26 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 12:27 p.m.
E999 SyntaxError: EOL while scanning string literal	reviewbot	Feb. 6, 2021, 12:27 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Feb. 6, 2021, 12:27 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Feb. 6, 2021, 12:27 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 12:27 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Feb. 6, 2021, 12:27 p.m.
E127 continuation line over-indented for visual indent	reviewbot	Feb. 6, 2021, 12:27 p.m.
W293 blank line contains whitespace	reviewbot	Feb. 6, 2021, 12:27 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 12:27 p.m.
F812 list comprehension redefines 'user' from line 1042	reviewbot	Feb. 6, 2021, 12:27 p.m.
W293 blank line contains whitespace	reviewbot	Feb. 6, 2021, 12:33 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 12:33 p.m.
W293 blank line contains whitespace	reviewbot	Feb. 6, 2021, 12:33 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 12:33 p.m.
W293 blank line contains whitespace	reviewbot	Feb. 6, 2021, 12:33 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 12:34 p.m.
W293 blank line contains whitespace	reviewbot	Feb. 6, 2021, 12:34 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 5:41 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 5:41 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 5:41 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Feb. 6, 2021, 5:41 p.m.
W291 trailing whitespace	reviewbot	Feb. 6, 2021, 5:41 p.m.
F841 local variable 'draft' is assigned to but never used	reviewbot	Feb. 6, 2021, 5:41 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Feb. 6, 2021, 5:41 p.m.
This file shouldn't be part of your change.	david	March 27, 2021, 7:49 p.m.
I don't know if your editor did this automatically, but there are a bunch of indentation changes to this file …	david	March 27, 2021, 7:48 p.m.
Let's make the iterator variable named "user" rather than "inactive user", since this iterates over all users. It's probably also …	david	March 27, 2021, 7:48 p.m.
The outer parens aren't needed here. We can also just check truthiness on the list instead of checking the length.	david	March 27, 2021, 7:48 p.m.
Hmm. It might be nice to inform the user which users are inactive.	david	March 27, 2021, 7:48 p.m.
Let's call the iterator variable user here too.	david	March 27, 2021, 7:48 p.m.
We can just check truthiness on the list here instead of checking the length.	david	March 27, 2021, 7:48 p.m.
Outer parens aren't needed here. But we do need a space between the "if" and the conditional. These could actually …	david	March 27, 2021, 7:48 p.m.
This blank line isn't necessary.	david	March 27, 2021, 7:48 p.m.
Please revert unintentional indentation changes to this file.	david	March 27, 2021, 7:48 p.m.
Please put this line back.	david	March 27, 2021, 7:48 p.m.
Please put this line back.	david	March 27, 2021, 7:48 p.m.
Let's add a blank line after this.	david	March 27, 2021, 7:48 p.m.
Hmm. I think we want to keep this using found_usernames. Perhaps we can just add the is_active check into the …	david	Feb. 7, 2021, 3:49 p.m.
E501 line too long (96 > 79 characters)	reviewbot	March 27, 2021, 7:48 p.m.
E501 line too long (85 > 79 characters)	reviewbot	March 27, 2021, 7:48 p.m.
E703 statement ends with a semicolon	reviewbot	March 27, 2021, 7:48 p.m.
W293 blank line contains whitespace	reviewbot	March 27, 2021, 7:48 p.m.
E126 continuation line over-indented for hanging indent	reviewbot	March 27, 2021, 7:49 p.m.
We have a standard form for list comprehensions. This should look like: inactive_reviewer_list = [ reviewer for reviewer in reviewer_list …	chipx86	March 28, 2021, 8:35 p.m.
Blank line between statements and new blocks.	chipx86	March 28, 2021, 8:35 p.m.
Two important things here: We don't use .format(). It's not always better (technically it's slower than %-based formatting), and more …	chipx86	March 28, 2021, 8:35 p.m.
We're now iterating through the same list a second time. A better option, normally would be to iterate once and …	chipx86	March 28, 2021, 8:35 p.m.
Blank line between statements and blocks.	chipx86	March 28, 2021, 8:35 p.m.
Since we've now done that work of pulling out the active reviewers and checking them, we can avoid performing another …	chipx86	March 28, 2021, 8:35 p.m.
We've already covered the "active" bit above. I think we should leave this string as-is, since it's confusing when in …	chipx86	March 28, 2021, 8:35 p.m.
Blank line between statements and blocks.	chipx86	March 28, 2021, 8:35 p.m.
I think we can word this a bit better. How about: "The owner assigned to this review request cannot be …	chipx86	March 28, 2021, 8:35 p.m.
The """ must be on a separate line, for multi-line docstrings. Same with ones below.	chipx86	March 28, 2021, 8:35 p.m.
Can you use self.create_user() instead, and pass in keyword arguments for the values? Same for other tests.	chipx86	March 28, 2021, 8:35 p.m.
We're setting a whole lot more than we need for the test. This would be fine if we were testing …	chipx86	March 28, 2021, 8:35 p.m.
Blank line between statements and docs. Same for tests below.	chipx86	March 28, 2021, 8:35 p.m.
These changes aren't correct, and also aren't relevant to the rest of your change. Can you revert them?	chipx86	March 28, 2021, 8:35 p.m.
This will need to be in the style I outlined before, for list comprehensions.	chipx86	March 28, 2021, 8:35 p.m.
This just became expensive. Better to build a list of usernames up-front, and then pass that in. Otherwise, we're rebuilding …	chipx86	March 28, 2021, 8:35 p.m.
This blank line must remain.	chipx86	March 28, 2021, 8:35 p.m.
No trailing period for unit test docstrings. The test runner already puts a ... after the docstring, and it looks …	chipx86	March 28, 2021, 8:35 p.m.
It doesn't look like this is used anywhere.	chipx86	March 28, 2021, 8:35 p.m.
No need to pass None here. That's the default. Also, a bare None is not self-descriptive at all (it means …	chipx86	March 28, 2021, 8:35 p.m.
The last entry in a dictionary or list should always have a trailing comma, so that we don't have to …	chipx86	March 28, 2021, 8:35 p.m.
We have some newer kgb support for checking calls now. Can you replace these with: self.assertSpyNotCalled(ChangeDescription.save) self.assertSpyNotCalled(ReviewRequestDraft.save) Same below.	chipx86	March 28, 2021, 8:35 p.m.
Doesn't look like we're using this either.	chipx86	March 28, 2021, 8:35 p.m.
This should use self.assertIn instead.	chipx86	March 28, 2021, 8:35 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E712 comparison to False should be 'if cond is False:' or 'if not cond:'
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (96 > 79 characters)
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F812 list comprehension redefines 'user' from line 341
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E202 whitespace before ')'
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (85 > 79 characters)
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (86 > 79 characters)
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E711 comparison to None should be 'if cond is not None:'
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E712 comparison to False should be 'if cond is False:' or 'if not cond:'
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
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 1)
The issue has been resolved. Show all issues
```
E501 line too long (82 > 79 characters)
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
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 1)
The issue has been resolved. Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
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 1)
The issue has been resolved. Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 1)
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 1)
The issue has been resolved. Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/webapi/resources/review_request.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F812 list comprehension redefines 'user' from line 1042
```

Commits:

Summary	ID	Author
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	47a726d9344045966731e7d789ab5b469f96f547	mderose123
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	47a726d9344045966731e7d789ab5b469f96f547	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	55390df0128c8883c6c936d662c191ad1ed72763	mderose123

Diff:

Revision 2 (+264 -104)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/webapi/resources/review_request.py
	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 (82 > 79 characters)
```
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
```
W293 blank line contains whitespace
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E999 SyntaxError: EOL while scanning string literal
```
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
```
E126 continuation line over-indented for hanging indent
```
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
```
E123 closing bracket does not match indentation of opening bracket's line
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E999 SyntaxError: EOL while scanning string literal
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 2)
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 2)
The issue has been resolved. Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/webapi/resources/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F812 list comprehension redefines 'user' from line 1042
```

Testing Done:

-		-I've manually validated in the UI for unpublished or published review request that an error appears if a user tries to update the owner or people (reviewers) field with an innactive user
-
		-I've created unit tests for when a review request is published with the APIS for the following cases
		1. A review request draft is published with an inactive reviewer
		2. A review request draft is published with an inactive owner/submitter
		3. A review request draft is published with multiple reviewers, one of which is inactive

		Outstanding tests that I need to figure out how to write:
~		-Unit tests for checking if an API call is made to update a review request (or review request draft) with a user with any combination of an inactive reviewer and/or an inactive owner
	~	-Unit tests for checking if an API call is made to update a review request (or review request draft) with a user with any combination of an inactive reviewer and/or an inactive owner
	+	-I've manually validated in the UI for draft review request that an error appears if a user tries to update the owner or people (reviewers) field with an innactive user but need to iron out issues that appeared after making flake8 changes

Commits:

Summary	ID	Author
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	47a726d9344045966731e7d789ab5b469f96f547	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	55390df0128c8883c6c936d662c191ad1ed72763	mderose123
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	47a726d9344045966731e7d789ab5b469f96f547	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	55390df0128c8883c6c936d662c191ad1ed72763	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	dfa0445c86ed412ac9e12bdbccf6a2fe0e8ddb59	mderose123

Diff:

Revision 3 (+303 -125)

Show changes

	.vscode/settings.json
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/webapi/resources/review_request.py
	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
```
W293 blank line contains whitespace
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```

Commits:

Summary	ID	Author
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	47a726d9344045966731e7d789ab5b469f96f547	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	55390df0128c8883c6c936d662c191ad1ed72763	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	dfa0445c86ed412ac9e12bdbccf6a2fe0e8ddb59	mderose123
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	47a726d9344045966731e7d789ab5b469f96f547	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	55390df0128c8883c6c936d662c191ad1ed72763	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	dfa0445c86ed412ac9e12bdbccf6a2fe0e8ddb59	mderose123
Addendum Fixed Flake8 Issues Again	b2694e1d558f70d333d36587503859fa326e2821	mderose123

Diff:

Revision 4 (+308 -130)

Show changes

	.vscode/settings.json
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/resources/review_request_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/review_request_draft.py (Diff revision 4)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```

Commits:

Summary	ID	Author
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	47a726d9344045966731e7d789ab5b469f96f547	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	55390df0128c8883c6c936d662c191ad1ed72763	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	dfa0445c86ed412ac9e12bdbccf6a2fe0e8ddb59	mderose123
Addendum Fixed Flake8 Issues Again	b2694e1d558f70d333d36587503859fa326e2821	mderose123
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	47a726d9344045966731e7d789ab5b469f96f547	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	55390df0128c8883c6c936d662c191ad1ed72763	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	dfa0445c86ed412ac9e12bdbccf6a2fe0e8ddb59	mderose123
Addendum Fixed Flake8 Issues Again	b2694e1d558f70d333d36587503859fa326e2821	mderose123
Added tests for handling put requests with for updating submitter or target people with inactive users	07d9184b8ef78a6569c5bce98844af2355dc3d6f	mderose123
Addendum removed whitespace as recommended by flake8	3c55086ca8fbbf3b32c51afc75ac97ad5bd9bd16	mderose123

Diff:

Revision 5 (+433 -131)

Show changes

	.vscode/settings.json
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/review_request_draft.py (Diff revision 5)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 5)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 5)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 5)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 5)
The issue has been resolved. Show all issues
```
F841 local variable 'draft' is assigned to but never used
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 5)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```

Commits:

Summary	ID	Author
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	47a726d9344045966731e7d789ab5b469f96f547	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	55390df0128c8883c6c936d662c191ad1ed72763	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	dfa0445c86ed412ac9e12bdbccf6a2fe0e8ddb59	mderose123
Addendum Fixed Flake8 Issues Again	b2694e1d558f70d333d36587503859fa326e2821	mderose123
Added tests for handling put requests with for updating submitter or target people with inactive users	07d9184b8ef78a6569c5bce98844af2355dc3d6f	mderose123
Addendum removed whitespace as recommended by flake8	3c55086ca8fbbf3b32c51afc75ac97ad5bd9bd16	mderose123
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	47a726d9344045966731e7d789ab5b469f96f547	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	55390df0128c8883c6c936d662c191ad1ed72763	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	dfa0445c86ed412ac9e12bdbccf6a2fe0e8ddb59	mderose123
Addendum Fixed Flake8 Issues Again	b2694e1d558f70d333d36587503859fa326e2821	mderose123
Added tests for handling put requests with for updating submitter or target people with inactive users	07d9184b8ef78a6569c5bce98844af2355dc3d6f	mderose123
Addendum removed whitespace as recommended by flake8	3c55086ca8fbbf3b32c51afc75ac97ad5bd9bd16	mderose123
Addendum fixed flake8 code styling issues	e01be77030ddb4f5fe19e5b6db1cf67c9e58be87	mderose123

Diff:

Revision 6 (+440 -136)

Show changes

	.vscode/settings.json
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:: A change that prevents an inactive user to be assigned as a submitter or reviewer for a
~ published or unpublished review request. In addition to this, this change will prevent
~ unpublished review request. In addition to this, this change will prevent
a review request draft from being published if the submitter or reviewer(s) are inactive.
Testing Done:: +
I've manually validated in the UI for draft review request that an error appears if a user tries to update the owner/submitter or target people (reviewers) field with an innactive user
+

-I've created unit tests for when a review request is published with the APIS for the following cases
1. A review request draft is published with an inactive reviewer
2. A review request draft is published with an inactive owner/submitter
~ 3. A review request draft is published with multiple reviewers, one of which is inactive
~
~
Outstanding tests that I need to figure out how to write:
~ 3. A review request draft is published with multiple reviewers, one of which is inactive
~ 4. A PUT API call is made to update a review request draft with an inactive user set as the owner/submitter of the review request
~ 5. A PUT API call is made to update a review request draft with an inactive user set as one of the reviewers of the review request
- -Unit tests for checking if an API call is made to update a review request (or review request draft) with a user with any combination of an inactive reviewer and/or an inactive owner
- -I've manually validated in the UI for draft review request that an error appears if a user tries to update the owner or people (reviewers) field with an innactive user but need to iron out issues that appeared after making flake8 changes

Summary:: [WIP] Guarded against an inactive user or owner from being assigned to a review.
Guarded against an inactive user or owner from being assigned to a review.

Summary:: Guarded against an inactive user or owner from being assigned to a review.
Guarded against an inactive reviewer or owner from being assigned to a review.

Summary:: Guarded against an inactive reviewer or owner from being assigned to a review.
Guarded against an inactive user being set as a reviewer or owner of a review request

Groups:: students

Testing Done:: I've manually validated in the UI for draft review request that an error appears if a user tries to update the owner/submitter or target people (reviewers) field with an innactive user

~
-I've created unit tests for when a review request is published with the APIS for the following cases
~
-I've created unit tests for when a review request draft is published for the following cases
1. A review request draft is published with an inactive reviewer
2. A review request draft is published with an inactive owner/submitter
~ 3. A review request draft is published with multiple reviewers, one of which is inactive
~ 4. A PUT API call is made to update a review request draft with an inactive user set as the owner/submitter of the review request
~ 5. A PUT API call is made to update a review request draft with an inactive user set as one of the reviewers of the review request
~ 3. A review request draft is published with multiple reviewers, one of which is inactive
~
~
-I've also created additional unit tests regarrding when a review request draft is updated using a PUT request
+ 5. A PUT API call is made to update a review request draft with an inactive user set as the owner/submitter of the review request
+ 6. A PUT API call is made to update a review request draft with an inactive user set as one of the reviewers of the review request

Fix it!

```
Great start on this!
```
.vscode/settings.json (Diff revision 6)
The issue has been resolved. Show all issues
```
This file shouldn't be part of your change.
```

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

The issue has been resolved. Show all issues

I don't know if your editor did this automatically, but there are a bunch of indentation changes to this file that are unrelated to your fix. Please go through and revert these.

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

The issue has been resolved. Show all issues

Let's make the iterator variable named "user" rather than "inactive user", since this iterates over all users.

It's probably also more readable to have if not user.is_active

mderose Feb. 7, 2021, 4:41 p.m.

Thanks for the recommendation David, I had originally had "user" for these list comprehensions to check if a user is active or not but I was getting this issue from flake8 "F812 list comprehension redefines 'user' from line XXX". Do you know why this is happening? It was fixed when I changed the names to something other than user

david Feb. 8, 2021, 10:07 a.m.

That is a concern (python scoping doesn't isolate list comprehensions, so it will end up redefining the user in the rest of the function). Maybe something else unique like reviewer, or even just something short like u?

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

The issue has been resolved. Show all issues

The outer parens aren't needed here. We can also just check truthiness on the list instead of checking the length.

reviewboard/reviews/models/review_request_draft.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Hmm. It might be nice to inform the user which users are inactive.
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Let's call the iterator variable user here too.
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 6)
The issue has been resolved. Show all issues
```
We can just check truthiness on the list here instead of checking the length.
```

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

The issue has been resolved. Show all issues

Outer parens aren't needed here. But we do need a space between the "if" and the conditional. These could actually be combined together:

if owner is not None and not owner.is_active:

reviewboard/reviews/models/review_request_draft.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This blank line isn't necessary.
```
reviewboard/webapi/resources/review_request.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Please revert unintentional indentation changes to this file.
```
reviewboard/webapi/resources/review_request.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Please put this line back.
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Please put this line back.
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Let's add a blank line after this.
```

reviewboard/webapi/resources/review_request_draft.py (Diff revision 6)

An issue was opened. Show all issues

Hmm. I think we want to keep this using found_usernames. Perhaps we can just add the is_active check into the list compherension up on lines 1027-1030?

mderose March 20, 2021, 9:02 p.m.

When I add this check at liness 1027 - 1030 then I ran into an issue at line 1042 an inactive user can still be retrieved from there. The users list and found_usernames list are updated at the same time at line 1046 and 10947 respectively so the user list should match the found usernames and vice versa. How do you receommend I move forward with this given the issue I brought up?

Commits:

Summary	ID	Author
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	47a726d9344045966731e7d789ab5b469f96f547	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	55390df0128c8883c6c936d662c191ad1ed72763	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	dfa0445c86ed412ac9e12bdbccf6a2fe0e8ddb59	mderose123
Addendum Fixed Flake8 Issues Again	b2694e1d558f70d333d36587503859fa326e2821	mderose123
Added tests for handling put requests with for updating submitter or target people with inactive users	07d9184b8ef78a6569c5bce98844af2355dc3d6f	mderose123
Addendum removed whitespace as recommended by flake8	3c55086ca8fbbf3b32c51afc75ac97ad5bd9bd16	mderose123
Addendum fixed flake8 code styling issues	e01be77030ddb4f5fe19e5b6db1cf67c9e58be87	mderose123
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	47a726d9344045966731e7d789ab5b469f96f547	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	55390df0128c8883c6c936d662c191ad1ed72763	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	dfa0445c86ed412ac9e12bdbccf6a2fe0e8ddb59	mderose123
Addendum Fixed Flake8 Issues Again	b2694e1d558f70d333d36587503859fa326e2821	mderose123
Added tests for handling put requests with for updating submitter or target people with inactive users	07d9184b8ef78a6569c5bce98844af2355dc3d6f	mderose123
Addendum removed whitespace as recommended by flake8	3c55086ca8fbbf3b32c51afc75ac97ad5bd9bd16	mderose123
Addendum fixed flake8 code styling issues	e01be77030ddb4f5fe19e5b6db1cf67c9e58be87	mderose123
Addendum added changes as perDavid's recommendations	f3bfde108fe01cc5a0dbca39e94428da686c172b	mderose123

Diff:

Revision 7 (+520 -405)

Show changes

	reviewboard/accounts/backends/ad.py
	reviewboard/accounts/backends/ldap.py
	reviewboard/accounts/tests/test_ad_auth_backend.py
	reviewboard/accounts/tests/test_ldap_auth_backend.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/static/rb/js/newReviewRequest/views/commitView.es6.js
	reviewboard/static/rb/js/ui/views/dialogView.es6.js
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/models/review_request_draft.py (Diff revision 7)
The issue has been resolved. Show all issues
```
E501 line too long (96 > 79 characters)
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 7)
The issue has been resolved. Show all issues
```
E501 line too long (85 > 79 characters)
```
reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 7)
The issue has been resolved. Show all issues
```
E703 statement ends with a semicolon
```
reviewboard/webapi/resources/review_request.py (Diff revision 7)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```

Commits:

Summary	ID	Author
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	47a726d9344045966731e7d789ab5b469f96f547	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	55390df0128c8883c6c936d662c191ad1ed72763	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	dfa0445c86ed412ac9e12bdbccf6a2fe0e8ddb59	mderose123
Addendum Fixed Flake8 Issues Again	b2694e1d558f70d333d36587503859fa326e2821	mderose123
Added tests for handling put requests with for updating submitter or target people with inactive users	07d9184b8ef78a6569c5bce98844af2355dc3d6f	mderose123
Addendum removed whitespace as recommended by flake8	3c55086ca8fbbf3b32c51afc75ac97ad5bd9bd16	mderose123
Addendum fixed flake8 code styling issues	e01be77030ddb4f5fe19e5b6db1cf67c9e58be87	mderose123
Addendum added changes as perDavid's recommendations	f3bfde108fe01cc5a0dbca39e94428da686c172b	mderose123
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	77e175ff12c5c098ec93c8558d6dae2727f06eb6	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	89d6d608750c529824fc00cce880cf77cbe7ff6d	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	634e91d9a34c6939dfcbf2bf5c358be12b10067c	mderose123
Addendum Fixed Flake8 Issues Again	92d0dbb9c1deccdeb6a758b27e377048658e6509	mderose123
Added tests for handling put requests with for updating submitter or target people with inactive users	6511fc0ef5068540042210a266bdfe6cdd28859d	mderose123
Addendum removed whitespace as recommended by flake8	422334836d31a49b09658c3b4151cdb89960e428	mderose123
Addendum fixed flake8 code styling issues	1aca18a7075644c9043ae48365e7e720031f2c08	mderose123
Addendum added changes as perDavid's recommendations	60348fa225a045f5c33d56ff5ada4c303702af85	mderose123
Addendum made changes based off of flake8 recommendations	ec40c4934a492758547a73cf9c8bf8c18c765203	mderose123

Diff:

Revision 8 (+467 -159)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commits:

Summary	ID	Author
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	77e175ff12c5c098ec93c8558d6dae2727f06eb6	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	89d6d608750c529824fc00cce880cf77cbe7ff6d	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	634e91d9a34c6939dfcbf2bf5c358be12b10067c	mderose123
Addendum Fixed Flake8 Issues Again	92d0dbb9c1deccdeb6a758b27e377048658e6509	mderose123
Added tests for handling put requests with for updating submitter or target people with inactive users	6511fc0ef5068540042210a266bdfe6cdd28859d	mderose123
Addendum removed whitespace as recommended by flake8	422334836d31a49b09658c3b4151cdb89960e428	mderose123
Addendum fixed flake8 code styling issues	1aca18a7075644c9043ae48365e7e720031f2c08	mderose123
Addendum added changes as perDavid's recommendations	60348fa225a045f5c33d56ff5ada4c303702af85	mderose123
Addendum made changes based off of flake8 recommendations	ec40c4934a492758547a73cf9c8bf8c18c765203	mderose123
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	77e175ff12c5c098ec93c8558d6dae2727f06eb6	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	89d6d608750c529824fc00cce880cf77cbe7ff6d	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	634e91d9a34c6939dfcbf2bf5c358be12b10067c	mderose123
Addendum Fixed Flake8 Issues Again	92d0dbb9c1deccdeb6a758b27e377048658e6509	mderose123
Added tests for handling put requests with for updating submitter or target people with inactive users	6511fc0ef5068540042210a266bdfe6cdd28859d	mderose123
Addendum removed whitespace as recommended by flake8	422334836d31a49b09658c3b4151cdb89960e428	mderose123
Addendum fixed flake8 code styling issues	1aca18a7075644c9043ae48365e7e720031f2c08	mderose123
Addendum added changes as perDavid's recommendations	60348fa225a045f5c33d56ff5ada4c303702af85	mderose123
Addendum made changes based off of flake8 recommendations	ec40c4934a492758547a73cf9c8bf8c18c765203	mderose123
Addendum removed autoindentations added from IDE	4b0ae5cdf93a629aab9d4b3a27fffb1eb0046d13	mderose123

Diff:

Revision 9 (+468 -160)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/models/review_request_draft.py (Diff revision 9)
The issue has been resolved. Show all issues
```
E126 continuation line over-indented for hanging indent
```

Commits:

Summary	ID	Author
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	77e175ff12c5c098ec93c8558d6dae2727f06eb6	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	89d6d608750c529824fc00cce880cf77cbe7ff6d	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	634e91d9a34c6939dfcbf2bf5c358be12b10067c	mderose123
Addendum Fixed Flake8 Issues Again	92d0dbb9c1deccdeb6a758b27e377048658e6509	mderose123
Added tests for handling put requests with for updating submitter or target people with inactive users	6511fc0ef5068540042210a266bdfe6cdd28859d	mderose123
Addendum removed whitespace as recommended by flake8	422334836d31a49b09658c3b4151cdb89960e428	mderose123
Addendum fixed flake8 code styling issues	1aca18a7075644c9043ae48365e7e720031f2c08	mderose123
Addendum added changes as perDavid's recommendations	60348fa225a045f5c33d56ff5ada4c303702af85	mderose123
Addendum made changes based off of flake8 recommendations	ec40c4934a492758547a73cf9c8bf8c18c765203	mderose123
Addendum removed autoindentations added from IDE	4b0ae5cdf93a629aab9d4b3a27fffb1eb0046d13	mderose123
Guarded against an inactive user or owner from being assigned to a review. A change that prevents an inactive user to be assigned as a submitter or reviewer for a published or unpublished review request. In addition to this, this change will prevent a review request draft from being published if the submitter or reviewer(s) are inactive.	77e175ff12c5c098ec93c8558d6dae2727f06eb6	mderose123
Addendum Made code fixes based on flake8 code styling recommendations	89d6d608750c529824fc00cce880cf77cbe7ff6d	mderose123
Addendum Fixed Further Flake8 Code Styling Issues	634e91d9a34c6939dfcbf2bf5c358be12b10067c	mderose123
Addendum Fixed Flake8 Issues Again	92d0dbb9c1deccdeb6a758b27e377048658e6509	mderose123
Added tests for handling put requests with for updating submitter or target people with inactive users	6511fc0ef5068540042210a266bdfe6cdd28859d	mderose123
Addendum removed whitespace as recommended by flake8	422334836d31a49b09658c3b4151cdb89960e428	mderose123
Addendum fixed flake8 code styling issues	1aca18a7075644c9043ae48365e7e720031f2c08	mderose123
Addendum added changes as perDavid's recommendations	60348fa225a045f5c33d56ff5ada4c303702af85	mderose123
Addendum made changes based off of flake8 recommendations	ec40c4934a492758547a73cf9c8bf8c18c765203	mderose123
Addendum removed autoindentations added from IDE	4b0ae5cdf93a629aab9d4b3a27fffb1eb0046d13	mderose123
Addendum fixed over-indentation error raised by flake8	f5f6c7ca54da7bc917503e4a2ecaef991b9558ea	mderose123

Diff:

Revision 10 (+470 -162)

Show changes

	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request_draft.py
	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Testing Done:: ~
I've manually validated in the UI for draft review request that an error appears if a user tries to update the owner/submitter or target people (reviewers) field with an innactive user
~
I've manually validated in the UI for draft review request that an error appears if a user
+ tries to update the owner/submitter or target people (reviewers) field with an innactive
+ user

~
-I've created unit tests for when a review request draft is published for the following cases
~
-I've created unit tests for when a review request draft is published for the following
+ cases
1. A review request draft is published with an inactive reviewer
2. A review request draft is published with an inactive owner/submitter
3. A review request draft is published with multiple reviewers, one of which is inactive

~
-I've also created additional unit tests regarrding when a review request draft is updated using a PUT request
~ 5. A PUT API call is made to update a review request draft with an inactive user set as the owner/submitter of the review request
~ 6. A PUT API call is made to update a review request draft with an inactive user set as one of the reviewers of the review request
~
-I've also created additional unit tests regarrding when a review request draft is updated
~ using a PUT request
~ 5. A PUT API call is made to update a review request draft with an inactive user set as the
+ owner/submitter of the review request
+ 7. A PUT API call is made to update a review request draft with an inactive user set as one
+ 8. of the reviewers of the review request

Fix it!

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

An issue was opened. Show all issues

We have a standard form for list comprehensions. This should look like:

inactive_reviewer_list = [
    reviewer
    for reviewer in reviewer_list
    if not reviewer.is_active
]

Same applies to other sbelow.

reviewboard/reviews/models/review_request_draft.py (Diff revision 10)
An issue was opened. Show all issues
```
Blank line between statements and new blocks.
```

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

An issue was opened. Show all issues

Two important things here:

We don't use .format(). It's not always better (technically it's slower than %-based formatting), and more than anything, we just don't standardize around using it. So let's use %.
You're formatting before it goes into ugettext, which just won't work. You can't localize a dynamic string. Instead, ugettext must take a static string, and then you have to format the result of that call.

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

An issue was opened. Show all issues

We're now iterating through the same list a second time. A better option, normally would be to iterate once and define both lists.

However, note that we don't care at all about the contents of this list. We know how many inactive reviewers we have, and what the total is. So we can instead do:

has_active_reviewers = (len(reviewer_list) > len(inactive_reviewer_list)

reviewboard/reviews/models/review_request_draft.py (Diff revision 10)
An issue was opened. Show all issues
```
Blank line between statements and blocks.
```

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

An issue was opened. Show all issues

Since we've now done that work of pulling out the active reviewers and checking them, we can avoid performing another database query (target_groups.exists()) if we know we've failed that check. So, let's reverse the order of the things we check in this if statement.

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

An issue was opened. Show all issues

We've already covered the "active" bit above. I think we should leave this string as-is, since it's confusing when in the context of review groups.

reviewboard/reviews/models/review_request_draft.py (Diff revision 10)
An issue was opened. Show all issues
```
Blank line between statements and blocks.
```

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

An issue was opened. Show all issues

I think we can word this a bit better. How about:

"The owner assigned to this review request cannot be an inactive user."

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 10)
An issue was opened. Show all issues
```
The """ must be on a separate line, for multi-line docstrings.
Same with ones below.
```

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

An issue was opened. Show all issues

Can you use self.create_user() instead, and pass in keyword arguments for the values?

Same for other tests.

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

An issue was opened. Show all issues

We're setting a whole lot more than we need for the test. This would be fine if we were testing that draft publishing worked and that these values propagated to the review request, but we're not. Let's keep the test state minimal.

Same with tests below.

reviewboard/reviews/tests/test_review_request_draft.py (Diff revision 10)
An issue was opened. Show all issues
```
Blank line between statements and docs.
Same for tests below.
```

reviewboard/webapi/resources/review_request.py (Diff revision 10)

An issue was opened. Show all issues

These changes aren't correct, and also aren't relevant to the rest of your change. Can you revert them?

reviewboard/webapi/resources/review_request_draft.py (Diff revision 10)
An issue was opened. Show all issues
```
This will need to be in the style I outlined before, for list comprehensions.
```

reviewboard/webapi/resources/review_request_draft.py (Diff revision 10)

An issue was opened. Show all issues

This just became expensive.

Better to build a list of usernames up-front, and then pass that in. Otherwise, we're rebuilding the list of usernames for every loop of our list comprehension.

reviewboard/webapi/resources/review_request_draft.py (Diff revision 10)
An issue was opened. Show all issues
```
This blank line must remain.
```

reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 10)

An issue was opened. Show all issues

No trailing period for unit test docstrings. The test runner already puts a ... after the docstring, and it looks weird to have . ...

Same with test docstrings below.

reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 10)
An issue was opened. Show all issues
```
It doesn't look like this is used anywhere.
```

reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 10)

An issue was opened. Show all issues

No need to pass None here. That's the default. Also, a bare None is not self-descriptive at all (it means people have to go look up the function signature), so if we did want to pass this, we'd want to do so with a keyword argument.
Same below.

reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 10)

An issue was opened. Show all issues

The last entry in a dictionary or list should always have a trailing comma, so that we don't have to re-modify this line if we add another key.

Same below.

reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 10)

An issue was opened. Show all issues

We have some newer kgb support for checking calls now. Can you replace these with:

self.assertSpyNotCalled(ChangeDescription.save)
self.assertSpyNotCalled(ReviewRequestDraft.save)

Same below.

reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 10)
An issue was opened. Show all issues
```
Doesn't look like we're using this either.
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 10)
An issue was opened. Show all issues
```
This should use self.assertIn instead.
```

		A change that prevents an inactive user to be assigned as a submitter or reviewer for a
~		published or unpublished review request. In addition to this, this change will prevent
	~	unpublished review request. In addition to this, this change will prevent
		a review request draft from being published if the submitter or reviewer(s) are inactive.

~		I've manually validated in the UI for draft review request that an error appears if a user tries to update the owner/submitter or target people (reviewers) field with an innactive user
	~	I've manually validated in the UI for draft review request that an error appears if a user
	+	tries to update the owner/submitter or target people (reviewers) field with an innactive
	+	user

~		-I've created unit tests for when a review request draft is published for the following cases
	~	-I've created unit tests for when a review request draft is published for the following
	+	cases
		1. A review request draft is published with an inactive reviewer
		2. A review request draft is published with an inactive owner/submitter
		3. A review request draft is published with multiple reviewers, one of which is inactive

~		-I've also created additional unit tests regarrding when a review request draft is updated using a PUT request
~		5. A PUT API call is made to update a review request draft with an inactive user set as the owner/submitter of the review request
~		6. A PUT API call is made to update a review request draft with an inactive user set as one of the reviewers of the review request
	~	-I've also created additional unit tests regarrding when a review request draft is updated
	~	using a PUT request
	~	5. A PUT API call is made to update a review request draft with an inactive user set as the
	+	owner/submitter of the review request
	+	7. A PUT API call is made to update a review request draft with an inactive user set as one
	+	8. of the reviewers of the review request