flake8
-
reviewboard/reviews/managers.py (Diff revision 1) Show all issues -
Review Request #9217 — Created Sept. 24, 2017 and submitted
Information | |
---|---|
Claudz | |
Review Board | |
release-3.0.x | |
4507 | |
8412471... | |
Reviewers | |
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 |
![]() |
|
E231 missing whitespace after ',' |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E501 line too long (82 > 79 characters) |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E501 line too long (90 > 79 characters) |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
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) # ... |
|
|
No blank line here. |
|
|
Single quotes. |
|
|
We like our imports in alphabetical order. |
|
|
How about "Testing view_invite_only_groups permission allows viewing invite-only groups?" |
|
|
I would prefer initial_group_count |
|
|
Having read the Slack conversation, I know why we're doing this. However, in the future, someone reading this code might … |
|
|
Combine these into one statement. |
|
|
We don't modify the user's permissions past this point, so I don't believe this is necessary. |
|
|
I would prefer new_group_count. I find (adjective) (noun) more readable than (noun), (adjective). |
|
|
E265 block comment should start with '# ' |
![]() |
|
E265 block comment should start with '# ' |
![]() |
|
E265 block comment should start with '# ' |
![]() |
|
E265 block comment should start with '# ' |
![]() |
|
Can you insert this so that it's still in alphabetical order? |
|
|
Remove this blank line. |
|
|
We should wrap the description in _() so it gets marked for translation. |
|
|
Sentences in comments should end in periods. |
|
|
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 … |
|
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) |