feat(shared-views): Add groupsearchview starred endpoint for reordering #87347
Conversation
github-actions
bot
added
the
Scope: Backend
❌ 5 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| try: | ||
| gsvs = GroupSearchView.objects.filter( | ||
| organization=self.context["organization"], id__in=view_ids | ||
| ) |
There was a problem hiding this comment.
could use .values(...) here to make the query a bit more efficient by returning less data.
it is also probably good to do validation here that all the gsvs exist. that is, check the length of view_ids against the returned query.
There was a problem hiding this comment.
oh i guess it not existing is valid input, since we create it below
src/sentry/issues/endpoints/organization_group_search_view_starred_order.py
Outdated
Show resolved
Hide resolved
| return Response(status=status.HTTP_204_NO_CONTENT) | ||
|
|
||
|
|
||
| def _update_view_positions(organization: Organization, user_id: int, view_positions: list[int]): |
There was a problem hiding this comment.
this method might make sense to live on the object manager of GroupSearchViewStarred.
There was a problem hiding this comment.
Agreed, but I don't want to add that abstraction if this will be the only place that uses this functionality, which I suspect will be the case. I'll definitely keep the model managers abstraction in mind in the future though, i agree they can be very useful.
| starred_view.group_search_view_id: starred_view for starred_view in existing_starred_views | ||
| } | ||
|
|
||
| for idx, view_id in enumerate(view_positions): |
There was a problem hiding this comment.
at some point we'll probably want to do bulk operations instead of creating / updating one by one, as if you have 100+ views this could end up being a bit slow. on a related note, is there a cap to how many starred views someone could have?
There was a problem hiding this comment.
The PUT groupsearchview endpoint enforces a max of 50 views, which I think should be well below the threshold at which this would cause slowdowns.
There was a problem hiding this comment.
worth thinking about what is considered "fast" or "snappy" and can observe endpoint to make sure it's within parameters after releasing!
src/sentry/issues/endpoints/organization_group_search_view_starred_order.py
Outdated
Show resolved
Hide resolved
| existing_starred_views_dict[view_id].save() | ||
| else: | ||
| try: | ||
| GroupSearchViewStarred.objects.create( |
There was a problem hiding this comment.
Adding rows here seems unexpected. If I reorder some starred views I wouldn't want to star anything due to that action. IMO in this case we should either respond with a 400 or filter out the starred rows that don't exist
There was a problem hiding this comment.
Reordering your starred views shouldn't cause this create to fire. This would only fire if someone an existing view is starred.
…rred_order.py Co-authored-by: Josh Ferge <[email protected]>
|
Renaming this to groupsearchviewstarred (rather than groupsearchviewstarredorder) since this can endpoint does more than just reordering |
This PR adds the
PUT/group-search-views-starred-order/endpoint.This endpoint is akin to a pared down version of the old
PUT/group-search-views/endpoint, which used to bulk update every aspect of the view, including filter parameters like query, sort, and projects. This new endpoint no longer handles any filter parameter changes (that will be delegated to aPUT/group-search-views/:viewIdendpoint), but does handle bulk adding, removing, and reordering starred views.The logic at a high level is: