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

Information
mderose
Review Board
release-4.0.x
4896
Reviewers
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

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

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

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

.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

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

.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

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

.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

mderose
mderose
mderose 
  1. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




mderose




   

   

  

 



 















 



 

  

   




mderose




   

   

  

 



 















 



 

  

   




mderose




   

   

  

 



 















 



 

  

   




mderose




   

   

  

 



 















 



 

  

   




mderose




   

   

  

 



 















 



 

  

   




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

   		
  



 










  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Great start on this!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      .vscode/settings.json
      


       (Diff revision 6)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    This file shouldn't be part of your change.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    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.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    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
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    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. 



    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?
      
 
      

      3. 


 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



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

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



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

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Let's call the iterator variable user here too.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 






  8. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



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

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 






  9. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    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:

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    10. 






  10. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    This blank line isn't necessary.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    11. 






  11. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/webapi/resources/review_request.py
      


       (Diff revision 6)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Please revert unintentional indentation changes to this file.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    12. 






  12. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Please put this line back.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    13. 






  13. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Please put this line back.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    14. 






  14. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Let's add a blank line after this.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    15. 






  15. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/webapi/resources/review_request_draft.py
      


       (Diff revision 6)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    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?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    16. 




  16. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    17. 




  

 









     

    

   

  

  

   
   Loading...