Save customized search if user has member level permissions #4947
Conversation
|
Hi, |
| @@ -17,6 +17,8 @@ class SavedSearchSerializer(serializers.Serializer): | |||
|
|
|||
|
|
|||
| class ProjectSearchesEndpoint(ProjectEndpoint): | |||
| permission_classes = (ProjectSearchPermission,) | |||
There was a problem hiding this comment.
So we likely dont want to introduce new permissions for this. Searches are a low impact item. We'd need to either:
- Make the UI respect the existing permissions (e.g. members cant add new saved searches, and definitely cant set team defaults).
- OR create some unshared saved search behavior, where members can create their own saved searches but they cant share them with the team without having permissions.
I would side with #1, which is generally what the original API implementation was intended to support.
There was a problem hiding this comment.
@dcramer As the issue stated "members can't add new saved search", I tried to resolve it by adding new permission keeping in mind that a member would be allowed to add and change his default search view. Also API should be robust enough to prevent deleting of search view by members.
Existing permissions shouldn't mix with search permissions as both serve different purpose thats the reason I created separate permissions.
If still we don't require separate permissions, I will remove that and simply hide the button in UI depending on existing permissions.
There was a problem hiding this comment.
@nimishsinghal we already have permissions for this though. We dont really want/need granular per-search permissions (which is what these would suggest).
We could do something like:
- project:read => can set their own default saved search
- project:write => can set default team saved search
- project:write => can remove saved searches
Now if we want to allow them to create new saved searches, we'd have to augment the data model, because we wouldn't really want those exposed to the team by default. IMO those are two diff concerns from defaulting a saved search and exposing member-only saved searches.
If we allowed member level saved searches, it'd be something along the lines of:
- an optional owner field on SavedSearch model (must be nullable)
- an option to "share this saved search with the team"
- members with project:write can share with team
- members with project:read can only create private searches
- private searches show up differently in the UI to suggest they're private
There was a problem hiding this comment.
@dcramer as suggested by you I have tried to implement "allowing member level saved search". I have tried to follow above points.
Wasn't sure about following
=> private searches show up differently in the UI to suggest they're private
Should I add P label aligned with private searches to show that they are private?
|
@dcramer I made the requested changes. Please review and let me know if anything else needs to be changed. :) |
There was a problem hiding this comment.
- Updating the migration is absolutely mandatory since it can't be merged without that.
- Some tests would be useful, especially since we're talking about security and permissions.
Note: I haven't looked at any of the frontend changes, so will need to get eyes on that from @benvinegar or @macqueen.
| @@ -32,4 +32,5 @@ def serialize(self, obj, attrs, user): | |||
| 'isDefault': obj.is_default, | |||
| 'isUserDefault': attrs['isUserDefault'], | |||
| 'dateCreated': obj.date_added, | |||
| 'isPrivate': True if obj.owner else False, | |||
There was a problem hiding this comment.
This can/should just be bool(obj.owner)
| @@ -0,0 +1,730 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
So.... this is kinda shitty, but I can't merge this without this migration being recreated off of master. If merged as-is, the migration won't run since it's one in the past and the world will not be happy. So this needs to be deleted, then recreated off of master to give the right id. You can't just rename the file either since the frozen state at the bottom needs to be correct.
There was a problem hiding this comment.
@mattrobenolt Yup, It's a very valid point I have created a new migration.
nimishsinghal
force-pushed
the
feature-saved-role-member-level
branch
from
117882f
to
16f9888
Compare
|
Backend stuff looks reasonable, but want a frontend person to double check over those changes to make sure nothing is bad there. @benvinegar @macqueen |
|
@benvinegar @macqueen Can you please review the changes will resolve the conflicts post that and update the migration again :) |
fixup: Save customized search if user has member level permissions fixup: make new migration
macqueen
force-pushed
the
feature-saved-role-member-level
branch
from
16f9888
to
893b530
Compare
Generated by 🚫 danger |
|
@nimishsinghal thanks! |
|
Thanks @macqueen :) |
No description provided.