feat(shared-views): Add PUT /group-search-views/:viewId endpoint for updating a view
#88111
Conversation
github-actions
bot
added
the
Scope: Backend
| if "starred" in validated_data: | ||
| if validated_data["starred"]: |
There was a problem hiding this comment.
aren't these two if's doing the same thing?
There was a problem hiding this comment.
starred isn't a required query param, so the first one checks if it exists, and the second one checks if its true.
There was a problem hiding this comment.
in what case would we not pass starred in? i thought we had to pass the whole view in, so in the case where it's already starred and then we don't pass starred in, we keep it starred?
There was a problem hiding this comment.
This tangentially answers your question, but I think it might make sense to just remove this query param entirely since its redundant with the POST /group-search-view-starred/ endpoint, and I'm not sure if there will be an entrypoint to editing the filter params AND the starred status in the same flow
src/sentry/issues/endpoints/organization_group_search_view_details.py
Outdated
Show resolved
Hide resolved
| from sentry.api.serializers.base import serialize | ||
| from sentry.api.serializers.models.groupsearchview import GroupSearchViewSerializer | ||
| from sentry.api.serializers.rest_framework.groupsearchview import GroupSearchViewDetailsPutValidator | ||
| from sentry.issues.endpoints.organization_group_search_views import pick_default_project |
There was a problem hiding this comment.
it's not ideal to import a helper function from one endpoint to another. maybe pick_default_project could go either into the org member model, or group search view model? could also consider making a utils or "usecase" file for gsvs in the issues folder
There was a problem hiding this comment.
yeah I thought about this and I'm not exactly sure where the best place to put this is... I'm hesitant to add it to a foundational model (like OrgMember or Project) since that would imply that this "default project" logic should be used across the product, which makes me a little hesitant. On the other hand, putting it on the GroupSearchView model is a little weird since that logic doesn't even touch the GroupSearchView table. A utils folder might be the best place for it for now...
There was a problem hiding this comment.
^ I'm going to include this change in a separate PR to improve file structure, since there are other relevant changes I'd want to include in there (like moving things to an Issues folder, per your suggestion in a previous PR)
armenzg
left a comment
armenzg
left a comment
There was a problem hiding this comment.
I did not get a chance to look at the tests before my next meeting.
|
|
||
| validated_data = serializer.validated_data | ||
|
|
||
| view.name = validated_data["name"] |
There was a problem hiding this comment.
You may be able to add a method to view to read the parameters (e.g. view.read_data(validated_data)) or even store it as view.data = validated_data.
…for updating a view (#88111) Creates the `PUT` `/group-search-views/:viewId`, which updates a single view. This endpoint requires aa fully verbose view as its input — that is, all filter values must be populated, even if they have not been changed. In addition to the standard filter params, two non-required params are available to include: 1. `visibility: {"owner", "organization"}`: Whether or not to change the visibility of the view. Only org members with `team:admin`, `org:admin`, or `org:owner` permissions are allowed to change the visibility of a view from "owner" to "organization" 2. `starred: bool` : Whether or not to add or remove view from the starred list.
Creates the
PUT/group-search-views/:viewId, which updates a single view.This endpoint requires aa fully verbose view as its input — that is, all filter values must be populated, even if they have not been changed.
In addition to the standard filter params, two non-required params are available to include:
visibility: {"owner", "organization"}: Whether or not to change the visibility of the view. Only org members withteam:admin,org:admin, ororg:ownerpermissions are allowed to change the visibility of a view from "owner" to "organization"starred: bool: Whether or not to add or remove view from the starred list.