Add a new permission for users to see invite-only groups in groups list.
Review Request #9217 — Created Sept. 24, 2017 and submitted
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 |
 reviewbot
|
|
|
E231 missing whitespace after ',' |
reviewbot
|
|
|
E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
E501 line too long (90 > 79 characters) |
reviewbot
|
|
|
E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
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) # ... |
 brennie
|
|
|
No blank line here. |
brennie
|
|
|
Single quotes. |
brennie
|
|
|
We like our imports in alphabetical order. |
brennie
|
|
|
How about "Testing view_invite_only_groups permission allows viewing invite-only groups?" |
brennie
|
|
|
I would prefer initial_group_count |
brennie
|
|
|
Having read the Slack conversation, I know why we're doing this. However, in the future, someone reading this code might … |
brennie
|
|
|
Combine these into one statement. |
brennie
|
|
|
We don't modify the user's permissions past this point, so I don't believe this is necessary. |
brennie
|
|
|
I would prefer new_group_count. I find (adjective) (noun) more readable than (noun), (adjective). |
brennie
|
|
|
E265 block comment should start with '# ' |
reviewbot
|
|
|
E265 block comment should start with '# ' |
reviewbot
|
|
|
E265 block comment should start with '# ' |
reviewbot
|
|
|
E265 block comment should start with '# ' |
reviewbot
|
|
|
Can you insert this so that it's still in alphabetical order? |
 david
|
|
|
Remove this blank line. |
david
|
|
|
We should wrap the description in _() so it gets marked for translation. |
david
|
|
|
Sentences in comments should end in periods. |
david
|
|
|
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 … |
david
|
- 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:
-
540abdf2b69b38e223a2d2a9953789f5af903d9a3b9ceee237ef4e1e76f685df3cd17864dd1a12be
- Commit:
-
55065662504c53c35ed0219d29a1e67987f231c2e8afcf15e4ebf41a101bf49179f1fd6c07935503
Checks run (2 succeeded)
- Bugs:
-
Some minor nitpicks and style suggestions. Otherwise, this looks great!
-
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) # ... -
-
-
-
-
-
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.
-
-
-
- Change Summary:
-
addressed the suggestions made by Barret
- Commit:
-
e8afcf15e4ebf41a101bf49179f1fd6c07935503e72419eb0a1723665dd4ac088e8edf0afe08e4eb
- Commit:
-
e72419eb0a1723665dd4ac088e8edf0afe08e4ebfd39ff8596ab547403ffb0f9fb2dd172b5b10bcf
Checks run (2 succeeded)
- Change Summary:
-
fixed minor syntax issues raised by David
- Commit:
-
fd39ff8596ab547403ffb0f9fb2dd172b5b10bcf5965f384c75ec55f4d4e1398faa9965716c295eb
Checks run (2 succeeded)
- Commit:
-
5965f384c75ec55f4d4e1398faa9965716c295eb8412471169ba9fcb9d5bbad71779e7cbd10517da
