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

Review Request #9217 — Created Sept. 24, 2017 and submitted

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

Description From Last Updated

W293 blank line contains whitespace

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

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

brenniebrennie

No blank line here.

brenniebrennie

Single quotes.

brenniebrennie

We like our imports in alphabetical order.

brenniebrennie

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

brenniebrennie

I would prefer initial_group_count

brenniebrennie

Having read the Slack conversation, I know why we're doing this. However, in the future, someone reading this code might ...

brenniebrennie

Combine these into one statement.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

E265 block comment should start with '# '

reviewbotreviewbot

E265 block comment should start with '# '

reviewbotreviewbot

E265 block comment should start with '# '

reviewbotreviewbot

E265 block comment should start with '# '

reviewbotreviewbot

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

daviddavid

Remove this blank line.

daviddavid

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

daviddavid

Sentences in comments should end in periods.

daviddavid

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

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

flake8

CL
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

CL
Review request changed

Commit:

-3b9ceee237ef4e1e76f685df3cd17864dd1a12be
+c985ec4b410c17a7b6847816a7a0d2f44c7b6dd6

Diff:

Revision 3 (+42 -3)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

CL
Review request changed

Commit:

-c985ec4b410c17a7b6847816a7a0d2f44c7b6dd6
+55065662504c53c35ed0219d29a1e67987f231c2

Diff:

Revision 4 (+41 -2)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

CL
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

CL
david
  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. 
      
CL
david
  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. 
      
CL
brennie
  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. 
      
david
  1. Ship It!
  2. 
      
CL
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (db307df)
Loading...