Lint, test, fix mongo helper partial permissions #4776
Conversation
Add mypy and ruff pyproject settings Refactor mongo helper default args, fix documented types Add mongo helper unit test Add pertial perm unit tests (not all pass here, marked to not run) Improve validation on asset permission assignment
| raise serializers.ValidationError( | ||
| {'user': t(ASSIGN_OWNER_ERROR_MESSAGE)} | ||
| ) | ||
| raise serializers.ValidationError({'user': t(ASSIGN_OWNER_ERROR_MESSAGE)}) |
There was a problem hiding this comment.
It's going over the 80 characters limit (but below 90*). Did you do this in purpose or your linter is configured this way?
* Old Kobo Python coding convention discussion: We usually break at 80 but allow up to 90 when it's only for few characters.
There was a problem hiding this comment.
It appears screwed up my local config to use default 88 when it was already set to 80. Using the formatter with 80 is OK right? I will update. Note the formatter greatly helps in the (soon to be) large permission dict (") and json (') that we need in docs and tests.
| ASSIGN_OWNER_ERROR_MESSAGE = "Owner's permissions cannot be assigned explicitly" | ||
|
|
||
|
|
||
| class AssetPermissionAssignmentSerializer(serializers.ModelSerializer): | ||
|
|
There was a problem hiding this comment.
💔 . Your linter is going to be crazy on all my code. I do add an extra line in purpose.
IMHO, It's more compliant with separated blocks of docstring and variables declaration.
There was a problem hiding this comment.
It happens several times in this PR. I would keep the extra line at the beginning of classes (and methods).
| return asset.get_label_for_permission( | ||
| object_permission.permission.codename | ||
| ) | ||
| return asset.get_label_for_permission(object_permission.permission.codename) |
There was a problem hiding this comment.
Same comment as #4776 (comment)
| 'url': url, | ||
| 'filters': filters | ||
| }) | ||
| hyperlinked_partial_perms.append({'url': url, 'filters': filters}) |
| for partial_permission, filters_ in self.__get_partial_permissions_generator( | ||
| partial_permissions | ||
| ): |
There was a problem hiding this comment.
What about
for (
partial_permission,
filters_,
) in self.__get_partial_permissions_generator(partial_permissions):instead. It's Black compliant and does not go over the 80 characters limit.
|
|
||
|
|
||
| class PartialPermissionField(serializers.Field): | ||
|
|
| assignments = serializers.ListField(child=PermissionAssignmentSerializer()) | ||
|
|
||
| @dataclass(frozen=True) | ||
| class PermissionAssignment: | ||
| """ A more-explicit alternative to a simple tuple """ | ||
| """A more-explicit alternative to a simple tuple""" |
There was a problem hiding this comment.
Your linter really don't like when the code breaths a little :-P
I would leave the extra space alone or even better I would put the docstrings on separated lines.
"""
A more-explicit alternative to a simple tuple
"""I prefer this way even if the sentence is short. Moreover if there is a diff at a later time, it does not involve the triple quotes.
| @@ -1,30 +1,29 @@ | |||
| # coding: utf-8 | |||
| from copy import deepcopy | |||
|
|
|||
| from django.contrib.auth.models import User, Permission | |||
| from django.contrib.auth.models import Permission, User | |||
| ASSIGN_OWNER_ERROR_MESSAGE = "Owner's permissions cannot be assigned explicitly" | ||
|
|
||
|
|
||
| class AssetPermissionAssignmentSerializer(serializers.ModelSerializer): | ||
|
|
There was a problem hiding this comment.
It happens several times in this PR. I would keep the extra line at the beginning of classes (and methods).
| @@ -659,3 +634,209 @@ def test_no_assignments_saved_on_error(self): | |||
| # than the one they already had, i.e.: 'view_submissions'. | |||
| self.assertFalse(self.asset.has_perm(self.someuser, PERM_DELETE_SUBMISSIONS)) | |||
| self.assertFalse(self.asset.has_perm(self.anotheruser, PERM_CHANGE_SUBMISSIONS)) | |||
|
|
|||
| # TODO | |||
There was a problem hiding this comment.
We could also use @pytest.mark.skip(reason=...)
| assert expected == returned_partial_perms | ||
|
|
||
| def test_partial_permission_invalid(self): | ||
| """Poorly formatted json example""" |
| [tool.ruff.format] | ||
| quote-style = "single" | ||
| [tool.ruff.lint] | ||
| extend-select = ["I"] |
There was a problem hiding this comment.
I would add a comment to explain what's the purpose of this line.
|
Important - squash and merge this otherwise the commit history will be contain contradictory changes that will make understanding history harder. |
Notes
This PR should contain no user facing changes. We need to rewrite a sizable portion of the affected files to support AND and OR in partial permissions. To make changes manageable, I split up the PR. This PR contains only lint, minor bug fixes, tests, and type corrections. All things that are only tangentially related to partial permissions. The partial permissions, partial rewrite is complex and functional types are key to successful implementation. Getting the linting done here will greatly simplify the job while keeping our PR diffs very readable.
While the majority of the changes are cosmetic, it contains some fixes and simplifications which could in theory introduce bugs that we don't have test coverage on.
Testing done: I created some shared partial permission in the existing UI and saw no error.
Related issues