-
Notifications
You must be signed in to change notification settings - Fork 92
Add Object permissions to project and allow set add_check globally or per-project #653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
1990ac4
5ab5ddc
0736fc3
054de50
e331e0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,9 +108,27 @@ class CheckListCreate(CheckMixin, ListCreateAPIView): | |
| lookup_url_kwarg = 'patch_id' | ||
| ordering = 'id' | ||
|
|
||
| def is_editable(self, user, patch): | ||
| if not user.is_authenticated: | ||
| return False | ||
|
|
||
| # Only users with add_check permission can do it. | ||
| # Notice that this is a global permission: it allows | ||
| # adding checks to any project inside Patchwork. | ||
| if user.has_perm('patchwork.add_check'): | ||
| patch._edited_by = user | ||
| return True | ||
|
|
||
| if user.has_perm('patchwork.add_check', patch.project): | ||
| patch._edited_by = user | ||
| return True | ||
|
|
||
| # Being maintainer doesn't grant rights to create checks. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a behavioural change? If yes, could this be avoided not to make upgrade difficult? Especially on large instances with many users :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, after the change, enabling/disabling checks become an independent permission. You got a point though: it could be better to set it by default during migration. Not sure if is there a way to teach migrations to set it by default for maintainers to preserve existing behaviour. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or do it in 2 steps: one without a behavioural change that can be backported, then changing the permissions. But yes, ideally setting it by default during the migration would make upgrade smoother. |
||
| return False | ||
|
|
||
| def create(self, request, patch_id, *args, **kwargs): | ||
| p = get_object_or_404(Patch, id=patch_id) | ||
| if not p.is_editable(request.user): | ||
| if not self.is_editable(request.user, p): | ||
| raise PermissionDenied() | ||
| request.patch = p | ||
| return super(CheckListCreate, self).create(request, *args, **kwargs) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,4 +3,5 @@ djangorestframework~=3.17.1 | |
| django-filter~=25.2.0 | ||
| django-debug-toolbar~=6.3.0 | ||
| django-dbbackup~=5.3.0 | ||
| django-guardian~=3.3.1 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you move it to the test one, please? I guess that's why the CI is no longer passing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, will do at the next rebase. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I think you should keep it there, but add it in the |
||
| -r requirements-test.txt | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| Django~=6.0.0 | ||
| djangorestframework~=3.17.1 | ||
| django-filter~=25.2.0 | ||
| django-guardian~=3.3.1 | ||
| psycopg~=3.3.4 | ||
| sqlparse~=0.5.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(
patchis not defined in this first commit)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix on a next rebase (I'm currently in vacations - will likely do it next weekend).