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

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

Information

Review Board
release-3.0.x
8412471...

Reviewers

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

CL
Review request changed
Commit:
3b9ceee237ef4e1e76f685df3cd17864dd1a12be
c985ec4b410c17a7b6847816a7a0d2f44c7b6dd6

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

CL
Review request changed
Commit:
c985ec4b410c17a7b6847816a7a0d2f44c7b6dd6
55065662504c53c35ed0219d29a1e67987f231c2

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)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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)
     
     
    Show all issues

    No blank line here.

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

    Single quotes.

  5. Show all issues

    We like our imports in alphabetical order.

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

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

  7. Show all issues

    I would prefer initial_group_count

  8. Show all issues

    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)
     
     
     
    Show all issues

    Combine these into one statement.

  10. Show all issues

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

  11. Show all issues

    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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

CL
Review request changed
Change Summary:

fixed missing comment space

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

CL
david
  1. 
      
  2. reviewboard/accounts/backends.py (Diff revision 8)
     
     
     
     
     
     
     
     
    Show all issues

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

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

    Remove this blank line.

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

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

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

    Sentences in comments should end in periods.

  6. 
      
CL
david
  1. 
      
  2. reviewboard/reviews/models/group.py (Diff revision 9)
     
     
    Show all issues

    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:
Completed
Change Summary:
Pushed to release-3.0.x (db307df)