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

mderose
Review Board
release-4.0.x
4896
reviewboard, students

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.

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

Summary Author
Guarded against an inactive user or owner from being assigned to a review.
mderose123
Addendum Made code fixes based on flake8 code styling recommendations
mderose123
Addendum Fixed Further Flake8 Code Styling Issues
mderose123
Addendum Fixed Flake8 Issues Again
mderose123
Added tests for handling put requests
mderose123
Addendum removed whitespace as recommended by flake8
mderose123
Addendum fixed flake8 code styling issues
mderose123
Addendum added changes as perDavid's recommendations
mderose123
Addendum made changes based off of flake8 recommendations
mderose123
Addendum removed autoindentations added from IDE
mderose123
Addendum fixed over-indentation error raised by flake8
mderose123
Description From Last Updated

W293 blank line contains whitespace

reviewbotreviewbot

E712 comparison to False should be 'if cond is False:' or 'if not cond:'

reviewbotreviewbot

E501 line too long (96 > 79 characters)

reviewbotreviewbot

F812 list comprehension redefines 'user' from line 341

reviewbotreviewbot

E202 whitespace before ')'

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (85 > 79 characters)

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E711 comparison to None should be 'if cond is not None:'

reviewbotreviewbot

E712 comparison to False should be 'if cond is False:' or 'if not cond:'

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

F812 list comprehension redefines 'user' from line 1042

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E999 SyntaxError: EOL while scanning string literal

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E999 SyntaxError: EOL while scanning string literal

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

F812 list comprehension redefines 'user' from line 1042

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

F841 local variable 'draft' is assigned to but never used

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

This file shouldn't be part of your change.

daviddavid

I don't know if your editor did this automatically, but there are a bunch of indentation changes to this file ...

daviddavid

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

daviddavid

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

daviddavid

Hmm. It might be nice to inform the user which users are inactive.

daviddavid

Let's call the iterator variable user here too.

daviddavid

We can just check truthiness on the list here instead of checking the length.

daviddavid

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

daviddavid

This blank line isn't necessary.

daviddavid

Please revert unintentional indentation changes to this file.

daviddavid

Please put this line back.

daviddavid

Please put this line back.

daviddavid

Let's add a blank line after this.

daviddavid

Hmm. I think we want to keep this using found_usernames. Perhaps we can just add the is_active check into the ...

daviddavid

E501 line too long (96 > 79 characters)

reviewbotreviewbot

E501 line too long (85 > 79 characters)

reviewbotreviewbot

E703 statement ends with a semicolon

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

We have a standard form for list comprehensions. This should look like: inactive_reviewer_list = [ reviewer for reviewer in reviewer_list ...

chipx86chipx86

Blank line between statements and new blocks.

chipx86chipx86

Two important things here: We don't use .format(). It's not always better (technically it's slower than %-based formatting), and more ...

chipx86chipx86

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

chipx86chipx86

Blank line between statements and blocks.

chipx86chipx86

Since we've now done that work of pulling out the active reviewers and checking them, we can avoid performing another ...

chipx86chipx86

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

chipx86chipx86

Blank line between statements and blocks.

chipx86chipx86

I think we can word this a bit better. How about: "The owner assigned to this review request cannot be ...

chipx86chipx86

The """ must be on a separate line, for multi-line docstrings. Same with ones below.

chipx86chipx86

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

chipx86chipx86

We're setting a whole lot more than we need for the test. This would be fine if we were testing ...

chipx86chipx86

Blank line between statements and docs. Same for tests below.

chipx86chipx86

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

chipx86chipx86

This will need to be in the style I outlined before, for list comprehensions.

chipx86chipx86

This just became expensive. Better to build a list of usernames up-front, and then pass that in. Otherwise, we're rebuilding ...

chipx86chipx86

This blank line must remain.

chipx86chipx86

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

chipx86chipx86

It doesn't look like this is used anywhere.

chipx86chipx86

No need to pass None here. That's the default. Also, a bare None is not self-descriptive at all (it means ...

chipx86chipx86

The last entry in a dictionary or list should always have a trailing comma, so that we don't have to ...

chipx86chipx86

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.

chipx86chipx86

Doesn't look like we're using this either.

chipx86chipx86

This should use self.assertIn instead.

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

mderose
Review request changed

Commits:

Summary Author
-
Guarded against an inactive user or owner from being assigned to a review.
mderose123
+
Guarded against an inactive user or owner from being assigned to a review.
mderose123
+
Addendum Made code fixes based on flake8 code styling recommendations
mderose123

Diff:

Revision 2 (+264 -104)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mderose
Review request changed

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 Author
-
Guarded against an inactive user or owner from being assigned to a review.
mderose123
-
Addendum Made code fixes based on flake8 code styling recommendations
mderose123
+
Guarded against an inactive user or owner from being assigned to a review.
mderose123
+
Addendum Made code fixes based on flake8 code styling recommendations
mderose123
+
Addendum Fixed Further Flake8 Code Styling Issues
mderose123

Diff:

Revision 3 (+303 -125)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mderose
Review request changed

Commits:

Summary Author
-
Guarded against an inactive user or owner from being assigned to a review.
mderose123
-
Addendum Made code fixes based on flake8 code styling recommendations
mderose123
-
Addendum Fixed Further Flake8 Code Styling Issues
mderose123
+
Guarded against an inactive user or owner from being assigned to a review.
mderose123
+
Addendum Made code fixes based on flake8 code styling recommendations
mderose123
+
Addendum Fixed Further Flake8 Code Styling Issues
mderose123
+
Addendum Fixed Flake8 Issues Again
mderose123

Diff:

Revision 4 (+308 -130)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mderose
Review request changed

Commits:

Summary Author
-
Guarded against an inactive user or owner from being assigned to a review.
mderose123
-
Addendum Made code fixes based on flake8 code styling recommendations
mderose123
-
Addendum Fixed Further Flake8 Code Styling Issues
mderose123
-
Addendum Fixed Flake8 Issues Again
mderose123
+
Guarded against an inactive user or owner from being assigned to a review.
mderose123
+
Addendum Made code fixes based on flake8 code styling recommendations
mderose123
+
Addendum Fixed Further Flake8 Code Styling Issues
mderose123
+
Addendum Fixed Flake8 Issues Again
mderose123
+
Added tests for handling put requests
mderose123
+
Addendum removed whitespace as recommended by flake8
mderose123

Diff:

Revision 5 (+433 -131)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mderose
mderose
  1. 
      
  2. 
      
mderose
mderose
mderose
mderose
mderose
mderose
david
  1. Great start on this!

  2. .vscode/settings.json (Diff revision 6)
     
     
     
     

    This file shouldn't be part of your change.

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

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

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

    2. 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?

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

  6. Hmm. It might be nice to inform the user which users are inactive.

  7. Let's call the iterator variable user here too.

  8. We can just check truthiness on the list here instead of checking the length.

  9. 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:
    
  10. This blank line isn't necessary.

  11. reviewboard/webapi/resources/review_request.py (Diff revision 6)
     
     
     
     
     

    Please revert unintentional indentation changes to this file.

  12. Please put this line back.

  13. Please put this line back.

  14. Let's add a blank line after this.

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

    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?

    1. 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?

  16. 
      
mderose
Review request changed

Commits:

Summary Author
-
Guarded against an inactive user or owner from being assigned to a review.
mderose123
-
Addendum Made code fixes based on flake8 code styling recommendations
mderose123
-
Addendum Fixed Further Flake8 Code Styling Issues
mderose123
-
Addendum Fixed Flake8 Issues Again
mderose123
-
Added tests for handling put requests
mderose123
-
Addendum removed whitespace as recommended by flake8
mderose123
-
Addendum fixed flake8 code styling issues
mderose123
+
Guarded against an inactive user or owner from being assigned to a review.
mderose123
+
Addendum Made code fixes based on flake8 code styling recommendations
mderose123
+
Addendum Fixed Further Flake8 Code Styling Issues
mderose123
+
Addendum Fixed Flake8 Issues Again
mderose123
+
Added tests for handling put requests
mderose123
+
Addendum removed whitespace as recommended by flake8
mderose123
+
Addendum fixed flake8 code styling issues
mderose123
+
Addendum added changes as perDavid's recommendations
mderose123

Diff:

Revision 7 (+520 -405)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mderose
mderose
Review request changed

Commits:

Summary Author
-
Guarded against an inactive user or owner from being assigned to a review.
mderose123
-
Addendum Made code fixes based on flake8 code styling recommendations
mderose123
-
Addendum Fixed Further Flake8 Code Styling Issues
mderose123
-
Addendum Fixed Flake8 Issues Again
mderose123
-
Added tests for handling put requests
mderose123
-
Addendum removed whitespace as recommended by flake8
mderose123
-
Addendum fixed flake8 code styling issues
mderose123
-
Addendum added changes as perDavid's recommendations
mderose123
-
Addendum made changes based off of flake8 recommendations
mderose123
+
Guarded against an inactive user or owner from being assigned to a review.
mderose123
+
Addendum Made code fixes based on flake8 code styling recommendations
mderose123
+
Addendum Fixed Further Flake8 Code Styling Issues
mderose123
+
Addendum Fixed Flake8 Issues Again
mderose123
+
Added tests for handling put requests
mderose123
+
Addendum removed whitespace as recommended by flake8
mderose123
+
Addendum fixed flake8 code styling issues
mderose123
+
Addendum added changes as perDavid's recommendations
mderose123
+
Addendum made changes based off of flake8 recommendations
mderose123
+
Addendum removed autoindentations added from IDE
mderose123

Diff:

Revision 9 (+468 -160)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mderose
mderose
  1. 
      
  2. 
      
mderose
Review request changed

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

chipx86
  1. 
      
  2. 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.

  3. Blank line between statements and new blocks.

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

    Two important things here:

    1. 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 %.
    2. 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.
  5. 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)
    
  6. Blank line between statements and blocks.

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

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

  9. Blank line between statements and blocks.

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

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

  11. The """ must be on a separate line, for multi-line docstrings.

    Same with ones below.

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

    Same for other tests.

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

    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.

  14. Blank line between statements and docs.

    Same for tests below.

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

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

  16. This will need to be in the style I outlined before, for list comprehensions.

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

    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.

  18. This blank line must remain.

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

  20. It doesn't look like this is used anywhere.

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

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

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

  24. Doesn't look like we're using this either.

  25. This should use self.assertIn instead.

  26. 
      
Loading...