Add a new permission for users to see invite-only groups in groups list.

Review Request #9217 - Created Sept. 24, 2017 and updated

Claudia Chen
Review Board
release-3.0.x
4507
8412471...
reviewboard, students

When getting the groups list via the API, only super users can get the group list including invite-only groups.

This change adds a new permission under groups that can be granted to any user that includes all invite-only groups (except hidden groups) into the groups list requested by the user. This, however, does not allow the user to access all invite-only groups; only that they show up on the groups list page and related API requests.

Added a unit test, which passes (along with all other tests). The test checks that an invite-only group that a user is not a part of becomes viewable to the user once they are given the new permission. The test also checks that the permission does not give the user default access to invite-only hidden groups.

  • 0
  • 0
  • 28
  • 0
  • 28
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

Claudia Chen
Review request changed

Change Summary:

Added a unit test for the new permission.

Summary:

-[WIP] Add a new permission for users to see invite-only groups in groups list.
+Add a new permission for users to see invite-only groups in groups list.

Description:

   

When getting the groups list via the API, only super users can get the group list including invite-only groups.

   
~  

This change adds a new permission under groups that can be granted to any user that includes all invite-only groups into the groups list requested by the user.

  ~

This change adds a new permission under groups that can be granted to any user that includes all invite-only groups (except hidden groups) into the groups list requested by the user. This, however, does not allow the user to access all invite-only groups; only that they show up on the groups list page and related API requests.

-  
-  

This is a work in progress because unit tests are being written for this new permission.

Testing Done:

  +

Added a unit test, which passes (along with all other tests). The test checks that an invite-only group that a user is not a part of becomes viewable to the user once they are given the new permission. The test also checks that the permission does not give the user default access to invite-only hidden groups.

Commit:

-540abdf2b69b38e223a2d2a9953789f5af903d9a
+3b9ceee237ef4e1e76f685df3cd17864dd1a12be

Diff:

Revision 2 (+39 -2)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Claudia Chen
Review request changed

Commit:

-3b9ceee237ef4e1e76f685df3cd17864dd1a12be
+c985ec4b410c17a7b6847816a7a0d2f44c7b6dd6

Diff:

Revision 3 (+42 -3)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Claudia Chen
Review request changed

Commit:

-c985ec4b410c17a7b6847816a7a0d2f44c7b6dd6
+55065662504c53c35ed0219d29a1e67987f231c2

Diff:

Revision 4 (+41 -2)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Claudia Chen
Claudia Chen
Barret Rennie
  1. Some minor nitpicks and style suggestions. Otherwise, this looks great!

  2. reviewboard/reviews/managers.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     

    How about:

    else:
        q = Q()
    
        if not user.has_perm('reviews.can_view_invite_only_groups'):
            q &= Q(invite_only=False)
    
        if visible_only:
            q &= Q(visible=True)
    
        # ...
    
  3. reviewboard/reviews/managers.py (Diff revision 5)
     
     

    No blank line here.

  4. reviewboard/reviews/models/group.py (Diff revision 5)
     
     

    Single quotes.

  5. We like our imports in alphabetical order.

  6. reviewboard/reviews/tests/test_policy.py (Diff revision 5)
     
     
     
     

    How about "Testing view_invite_only_groups permission allows viewing invite-only groups?"

  7. I would prefer initial_group_count

  8. Having read the Slack conversation, I know why we're doing this. However, in the future, someone reading this code might not know about the permissions cache, so we should add a comment explaining why this is here.

  9. reviewboard/reviews/tests/test_policy.py (Diff revision 5)
     
     
     

    Combine these into one statement.

  10. We don't modify the user's permissions past this point, so I don't believe this is necessary.

  11. I would prefer new_group_count. I find (adjective) (noun) more readable than (noun), (adjective).

  12. 
      
Claudia Chen
Review request changed

Change Summary:

addressed the suggestions made by Barret

Commit:

-e8afcf15e4ebf41a101bf49179f1fd6c07935503
+e72419eb0a1723665dd4ac088e8edf0afe08e4eb

Diff:

Revision 6 (+40 -2)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Claudia Chen
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Claudia Chen
David Trowbridge
  1. 
      
  2. reviewboard/accounts/backends.py (Diff revision 8)
     
     
     
     
     
     
     
     

    Can you insert this so that it's still in alphabetical order?

  3. reviewboard/reviews/managers.py (Diff revision 8)
     
     

    Remove this blank line.

  4. reviewboard/reviews/models/group.py (Diff revision 8)
     
     

    We should wrap the description in _() so it gets marked for translation.

  5. reviewboard/reviews/tests/test_policy.py (Diff revision 8)
     
     
     

    Sentences in comments should end in periods.

  6. 
      
Claudia Chen
David Trowbridge
  1. 
      
  2. reviewboard/reviews/models/group.py (Diff revision 9)
     
     

    This syntax isn't quite right--instead of defining a 2-tuple, you're passing two arguments into ugettext_lazy. I'm surprised this isn't crashing (did you test it?)

    This should probably look like:

    permissions = (
        ('can_view_invite_only_groups', _('Can view invite-only groups')),
    )
    
    1. yeah it definately crashes. I forgot to run my test after adding the underscore. I'll fix it.

  3. 
      
Claudia Chen
Review request changed

Commit:

-5965f384c75ec55f4d4e1398faa9965716c295eb
+8412471169ba9fcb9d5bbad71779e7cbd10517da

Diff:

Revision 10 (+39 -2)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Barret Rennie
  1. Ship It!
    1. Should I press the ship it tab/button? Or will one of the mentors handle it?

    2. At this point, unless there's additional review feedback or changes requested, there's nothing you need to do here. You've got one Ship It - and I think if and when you get a second, we'll land your patch. 😄

  2. 
      
Loading...