flake8
-
reviewboard/reviews/managers.py (Diff revision 1) Show all issues -
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 |
reviewboard/reviews/managers.py (Diff revision 1) |
---|
Added a unit test for the new permission.
Summary: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||
Testing Done: |
|
||||||||||||||||||
Commit: |
|
||||||||||||||||||
Diff: |
Revision 2 (+39 -2) |
reviewboard/reviews/tests/test_policy.py (Diff revision 2) |
---|
E128 continuation line under-indented for visual indent
reviewboard/reviews/tests/test_policy.py (Diff revision 2) |
---|
E128 continuation line under-indented for visual indent
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+42 -3) |
reviewboard/reviews/tests/test_policy.py (Diff revision 3) |
---|
E128 continuation line under-indented for visual indent
reviewboard/reviews/tests/test_policy.py (Diff revision 3) |
---|
E127 continuation line over-indented for visual indent
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+41 -2) |
reviewboard/reviews/tests/test_policy.py (Diff revision 4) |
---|
E128 continuation line under-indented for visual indent
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+41 -2) |
Some minor nitpicks and style suggestions. Otherwise, this looks great!
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) # ...
reviewboard/reviews/tests/test_policy.py (Diff revision 5) |
---|
We like our imports in alphabetical order.
reviewboard/reviews/tests/test_policy.py (Diff revision 5) |
---|
How about "Testing view_invite_only_groups permission allows viewing invite-only groups?"
reviewboard/reviews/tests/test_policy.py (Diff revision 5) |
---|
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) |
---|
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) |
---|
I would prefer
new_group_count
. I find (adjective) (noun) more readable than (noun), (adjective).
addressed the suggestions made by Barret
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+40 -2) |
reviewboard/reviews/tests/test_policy.py (Diff revision 6) |
---|
E265 block comment should start with '# '
reviewboard/reviews/tests/test_policy.py (Diff revision 6) |
---|
E265 block comment should start with '# '
reviewboard/reviews/tests/test_policy.py (Diff revision 7) |
---|
E265 block comment should start with '# '
reviewboard/reviews/tests/test_policy.py (Diff revision 7) |
---|
E265 block comment should start with '# '
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+40 -2) |
reviewboard/accounts/backends.py (Diff revision 8) |
---|
Can you insert this so that it's still in alphabetical order?
reviewboard/reviews/models/group.py (Diff revision 8) |
---|
We should wrap the description in
_()
so it gets marked for translation.
reviewboard/reviews/tests/test_policy.py (Diff revision 8) |
---|
Sentences in comments should end in periods.
fixed minor syntax issues raised by David
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+39 -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')), )
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+39 -2) |