Summary:

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

Review Request #9217 — Created Sept. 24, 2017 and submitted Nov. 25, 2017, 10:58 a.m.

Information
Owner:	Claudz
Repository:	Review Board
Branch:	release-3.0.x
Bugs:	4507
Depends On:
Commit:	8412471...
Reviewers
Groups:	reviewboard, students
People:

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

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.

Issues

Description	From	Last Updated
W293 blank line contains whitespace	reviewbot	Sept. 24, 2017, 2:54 p.m.
E231 missing whitespace after ','	reviewbot	Sept. 24, 2017, 2:54 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Sept. 24, 2017, 3:12 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Sept. 24, 2017, 3:12 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Sept. 24, 2017, 3:12 p.m.
E501 line too long (90 > 79 characters)	reviewbot	Sept. 24, 2017, 3:12 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Sept. 24, 2017, 3:16 p.m.
E127 continuation line over-indented for visual indent	reviewbot	Sept. 24, 2017, 3:16 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Sept. 24, 2017, 3:19 p.m.
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	Sept. 25, 2017, 11:49 a.m.
No blank line here.	brennie	Sept. 25, 2017, 11:49 a.m.
Single quotes.	brennie	Sept. 25, 2017, 11:58 a.m.
We like our imports in alphabetical order.	brennie	Sept. 25, 2017, 11:58 a.m.
How about "Testing view_invite_only_groups permission allows viewing invite-only groups?"	brennie	Sept. 25, 2017, 11:58 a.m.
I would prefer initial_group_count	brennie	Sept. 25, 2017, 11:58 a.m.
Having read the Slack conversation, I know why we're doing this. However, in the future, someone reading this code might …	brennie	Sept. 25, 2017, 11:58 a.m.
Combine these into one statement.	brennie	Sept. 25, 2017, 11:58 a.m.
We don't modify the user's permissions past this point, so I don't believe this is necessary.	brennie	Sept. 25, 2017, 11:59 a.m.
I would prefer new_group_count. I find (adjective) (noun) more readable than (noun), (adjective).	brennie	Sept. 25, 2017, 11:59 a.m.
E265 block comment should start with '# '	reviewbot	Sept. 29, 2017, 8:58 p.m.
E265 block comment should start with '# '	reviewbot	Sept. 29, 2017, 8:58 p.m.
E265 block comment should start with '# '	reviewbot	Sept. 29, 2017, 9 p.m.
E265 block comment should start with '# '	reviewbot	Sept. 29, 2017, 9 p.m.
Can you insert this so that it's still in alphabetical order?	david	Oct. 21, 2017, 4:04 p.m.
Remove this blank line.	david	Oct. 21, 2017, 4:04 p.m.
We should wrap the description in _() so it gets marked for translation.	david	Oct. 21, 2017, 4:06 p.m.
Sentences in comments should end in periods.	david	Oct. 21, 2017, 4:08 p.m.
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	Nov. 4, 2017, 8:37 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/managers.py (Diff revision 1)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/models/group.py (Diff revision 1)
Show all issues
```
E231 missing whitespace after ','
```

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:

Commit:

-	540abdf2b69b38e223a2d2a9953789f5af903d9a
+	3b9ceee237ef4e1e76f685df3cd17864dd1a12be

Diff:

Revision 2 (+39 -2)

Show changes

	reviewboard/accounts/backends.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models/group.py
	reviewboard/reviews/tests/test_policy.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_policy.py (Diff revision 2)
Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/reviews/tests/test_policy.py (Diff revision 2)
Show all issues
```
E501 line too long (82 > 79 characters)
```
reviewboard/reviews/tests/test_policy.py (Diff revision 2)
Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/reviews/tests/test_policy.py (Diff revision 2)
Show all issues
```
E501 line too long (90 > 79 characters)
```

Commit:

-	3b9ceee237ef4e1e76f685df3cd17864dd1a12be
+	c985ec4b410c17a7b6847816a7a0d2f44c7b6dd6

Diff:

Revision 3 (+42 -3)

Show changes

	reviewboard/accounts/backends.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models/group.py
	reviewboard/reviews/tests/test_policy.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_policy.py (Diff revision 3)
Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/reviews/tests/test_policy.py (Diff revision 3)
Show all issues
```
E127 continuation line over-indented for visual indent
```

Commit:

-	c985ec4b410c17a7b6847816a7a0d2f44c7b6dd6
+	55065662504c53c35ed0219d29a1e67987f231c2

Diff:

Revision 4 (+41 -2)

Show changes

	reviewboard/accounts/backends.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models/group.py
	reviewboard/reviews/tests/test_policy.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_policy.py (Diff revision 4)
Show all issues
```
E128 continuation line under-indented for visual indent
```

Commit:

-	55065662504c53c35ed0219d29a1e67987f231c2
+	e8afcf15e4ebf41a101bf49179f1fd6c07935503

Diff:

Revision 5 (+41 -2)

Show changes

	reviewboard/accounts/backends.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models/group.py
	reviewboard/reviews/tests/test_policy.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Bugs:

+	4507

Some minor nitpicks and style suggestions. Otherwise, this looks great!

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)

    # ...

reviewboard/reviews/managers.py (Diff revision 5)
Show all issues
```
No blank line here.
```
reviewboard/reviews/models/group.py (Diff revision 5)
Show all issues
```
Single quotes.
```
reviewboard/reviews/tests/test_policy.py (Diff revision 5)
Show all issues
```
We like our imports in alphabetical order.
```

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

reviewboard/reviews/tests/test_policy.py (Diff revision 5)
Show all issues
```
I would prefer initial_group_count
```

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

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.

reviewboard/reviews/tests/test_policy.py (Diff revision 5)
Show all issues
```
Combine these into one statement.
```

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

Show all issues

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

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

Show all issues

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

Change Summary:

addressed the suggestions made by Barret

Commit:

-	e8afcf15e4ebf41a101bf49179f1fd6c07935503
+	e72419eb0a1723665dd4ac088e8edf0afe08e4eb

Diff:

Revision 6 (+40 -2)

Show changes

	reviewboard/accounts/backends.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models/group.py
	reviewboard/reviews/tests/test_policy.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_policy.py (Diff revision 6)
Show all issues
```
E265 block comment should start with '# '
```
reviewboard/reviews/tests/test_policy.py (Diff revision 6)
Show all issues
```
E265 block comment should start with '# '
```

Change Summary:

fixed missing comment space

Diff:

Revision 7 (+40 -2)

Show changes

	reviewboard/accounts/backends.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models/group.py
	reviewboard/reviews/tests/test_policy.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_policy.py (Diff revision 7)
Show all issues
```
E265 block comment should start with '# '
```
reviewboard/reviews/tests/test_policy.py (Diff revision 7)
Show all issues
```
E265 block comment should start with '# '
```

Commit:

-	e72419eb0a1723665dd4ac088e8edf0afe08e4eb
+	fd39ff8596ab547403ffb0f9fb2dd172b5b10bcf

Diff:

Revision 8 (+40 -2)

Show changes

	reviewboard/accounts/backends.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models/group.py
	reviewboard/reviews/tests/test_policy.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/accounts/backends.py (Diff revision 8)

Show all issues

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

reviewboard/reviews/managers.py (Diff revision 8)
Show all issues
```
Remove this blank line.
```

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

Show all issues

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

reviewboard/reviews/tests/test_policy.py (Diff revision 8)
Show all issues
```
Sentences in comments should end in periods.
```

Change Summary:

fixed minor syntax issues raised by David

Commit:

-	fd39ff8596ab547403ffb0f9fb2dd172b5b10bcf
+	5965f384c75ec55f4d4e1398faa9965716c295eb

Diff:

Revision 9 (+39 -2)

Show changes

	reviewboard/accounts/backends.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models/group.py
	reviewboard/reviews/tests/test_policy.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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')),
)

Claudz Nov. 4, 2017, 8:37 p.m.

yeah it definately crashes. I forgot to run my test after adding the underscore. I'll fix it.

Commit:

-	5965f384c75ec55f4d4e1398faa9965716c295eb
+	8412471169ba9fcb9d5bbad71779e7cbd10517da

Diff:

Revision 10 (+39 -2)

Show changes

	reviewboard/accounts/backends.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models/group.py
	reviewboard/reviews/tests/test_policy.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

Ship It!

Claudz Nov. 6, 2017, 6:11 p.m.

Should I press the ship it tab/button? Or will one of the mentors handle it?

mike_conley Nov. 6, 2017, 6:39 p.m.

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.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (db307df)