feat(shared-views): Add groupsearchview starred endpoint for reordering #87347

MichaelSun48 · 2025-03-18T21:22:51Z

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 a PUT /group-search-views/:viewId endpoint), but does handle bulk adding, removing, and reordering starred views.

The logic at a high level is:

Accept a list of viewIds from the client, where the position of the viewId in the list is the new position
Run validation on the viewIds
Delete any GroupSearchViewStarred entries that are no longer in the request list (they must have been removed)
Update the positions for any GroupSearchViewStarred entries that exist in the list
Create new GroupSearchViewStarred entries for views that did not already have them.

codecov · 2025-03-18T21:55:07Z

Codecov Report

Attention: Patch coverage is 90.90909% with 15 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...st_organization_group_search_view_starred_order.py	87.50%	13 Missing ⚠️
...ts/organization_group_search_view_starred_order.py	95.34%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #87347       +/-   ##
===========================================
+ Coverage   33.16%   87.75%   +54.58%     
===========================================
  Files        8358     9881     +1523     
  Lines      466166   560388    +94222     
  Branches    22076    22075        -1     
===========================================
+ Hits       154616   491745   +337129     
+ Misses     311146    68239   -242907     
  Partials      404      404

JoshFerge · 2025-03-18T22:16:10Z

src/sentry/issues/endpoints/organization_group_search_view_starred_order.py

+        try:
+            gsvs = GroupSearchView.objects.filter(
+                organization=self.context["organization"], id__in=view_ids
+            )


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.

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

JoshFerge · 2025-03-18T22:26:27Z

src/sentry/issues/endpoints/organization_group_search_view_starred_order.py

+        return Response(status=status.HTTP_204_NO_CONTENT)
+
+
+def _update_view_positions(organization: Organization, user_id: int, view_positions: list[int]):


this method might make sense to live on the object manager of GroupSearchViewStarred.

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.

JoshFerge · 2025-03-18T22:27:31Z

src/sentry/issues/endpoints/organization_group_search_view_starred_order.py

+        starred_view.group_search_view_id: starred_view for starred_view in existing_starred_views
+    }
+
+    for idx, view_id in enumerate(view_positions):


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?

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.

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

malwilley · 2025-03-18T22:42:57Z

src/sentry/issues/endpoints/organization_group_search_view_starred_order.py

+            existing_starred_views_dict[view_id].save()
+        else:
+            try:
+                GroupSearchViewStarred.objects.create(


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

Reordering your starred views shouldn't cause this create to fire. This would only fire if someone an existing view is starred.

IMO reordering and starring/unstarring should be handled in separate endpoints. I think that what you've written here is great for reordering, but I'd rather have a separate endpoint that just stars/unstars a single view at a time (for a couple reasons):

The frontend may want to star/unstar a view from a page where we don't have the full list of starred views available

When reordering, you may be coming back to a tab where you can out of date data (for example, maybe you unstarred something in one tab, then went back to an old tab). If you reorder the views in this state, it would re-star views that you previously unstarred

Not saying I totally disagree with you or that I'm adamant about keeping it this way (I could actually see a world where we have both endpoints), but to your two points:

When would this happen, realistically? I imagined that starred views would be among the first things the frontend fetches, at least when visiting Issues. And I can't think of a place outside of Issues where you would star/unstar an Issue view, especially without context into what views are already starred.

To me at least, this seems like the expected behavior — if we only allow reordering, we'd have to probably throw an error if the list of view_ids doesn't match the list of already starred views. So in your secnario, if you reorder a stale view state in a separate browser tab, we'd probably have to revert the reorder and display an error message, which seems unexpected. Similar to the old PUT /group-search-view/ endpoint, I feel like this endpoint should be pretty forgiving.

You're right that in the current world we will always be starring/unstarring in places where the views should already be fetched. Ergonomically, it would be better though if clicking a star icon you could just call POST /views/<view_id/star and not need to grab the rest of the starred items from the cache (and need to assume that the data is not stale). It's also good to design APIs for usages you can't yet anticipate and not require additional information.

The reason that we made the old endpoint so forgiving was that we allowed the user to do things like rename, new tab, reorder, etc all without waiting for the previous request to complete. This isn't as necessary for the new experience since you can only star/unstar and reorder (and we could put a loading state on star/unstar). I do agree that throwing an error would be a bad experience, but we could probably mitigate that by only throwing the error when there is no way to assume the correct ordering (and in that case doing an immediate refetch).

I think my stance would be that we should at the very least add a "dumb" star/unstar endpoint (in a separate PR of course). I think locking down this endpoint to only ordering would overall save users from some unexpected behavior, but it's not the worst thing either

Fair enough, I do agree that needing every other starred views' id to simply star a view is a bad api design, even if we can get away with it for now. I'll update this to just reorder in that case, thanks for the discussion and helping me think through this!

malwilley

Thanks for being so receptive to feedback! Looks great 💯

malwilley · 2025-03-19T18:52:23Z

src/sentry/models/groupsearchviewstarred.py

+        self, organization: Organization, user_id: int, new_view_positions: list[int]
+    ):
+        """
+        Reorders the positions of starred views for a user in an organization.


Very nice explanatory comment!

malwilley · 2025-03-19T18:54:06Z

src/sentry/issues/endpoints/organization_group_search_view_starred_order.py

@@ -61,33 +61,14 @@ def put(self, request: Request, organization: Organization) -> Response:

        try:
            with transaction.atomic(using=router.db_for_write(GroupSearchViewStarred)):


Hmm I wonder if this transaction is necessary anymore since we are just doing a single bulk update

…rred_order.py Co-authored-by: Josh Ferge <[email protected]>

MichaelSun48 requested review from a team as code owners March 18, 2025 21:22

MichaelSun48 requested a review from a team March 18, 2025 21:22

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 18, 2025

JoshFerge reviewed Mar 18, 2025

View reviewed changes

src/sentry/issues/endpoints/organization_group_search_view_starred_order.py Outdated Show resolved Hide resolved

JoshFerge reviewed Mar 18, 2025

View reviewed changes

src/sentry/issues/endpoints/organization_group_search_view_starred_order.py Outdated Show resolved Hide resolved

malwilley reviewed Mar 18, 2025

View reviewed changes

MichaelSun48 requested review from JoshFerge and malwilley March 18, 2025 23:41

vercel bot deployed to Preview March 18, 2025 23:47 View deployment

MichaelSun48 requested a review from a team March 19, 2025 18:43

vercel bot deployed to Preview March 19, 2025 18:47 View deployment

malwilley approved these changes Mar 19, 2025

View reviewed changes

JoshFerge approved these changes Mar 19, 2025

View reviewed changes

MichaelSun48 force-pushed the msun/sharedViews/addStarredOrderEndpoint branch from b4a786c to 34d48df Compare March 19, 2025 20:28

vercel bot deployed to Preview March 19, 2025 20:33 View deployment

MichaelSun48 and others added 4 commits March 19, 2025 13:49

Add groupsearchview starred endpoint for reordering

01a8871

Update src/sentry/issues/endpoints/organization_group_search_view_sta…

a7bfe9d

…rred_order.py Co-authored-by: Josh Ferge <[email protected]>

Rename endpoints, remove unnecessary try catches

60e0d32

Refactor to ONLY reorder. Use base manager instead of ad-hoc logic

903e82e

MichaelSun48 force-pushed the msun/sharedViews/addStarredOrderEndpoint branch from 34d48df to 903e82e Compare March 19, 2025 20:49

vercel bot deployed to Preview March 19, 2025 20:53 View deployment

MichaelSun48 merged commit 18a7492 into master Mar 19, 2025
48 checks passed

MichaelSun48 deleted the msun/sharedViews/addStarredOrderEndpoint branch March 19, 2025 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(shared-views): Add groupsearchview starred endpoint for reordering #87347

feat(shared-views): Add groupsearchview starred endpoint for reordering #87347

MichaelSun48 commented Mar 18, 2025

codecov bot commented Mar 18, 2025 •

edited

Loading

JoshFerge Mar 18, 2025 •

edited

Loading

JoshFerge Mar 18, 2025

JoshFerge Mar 18, 2025

MichaelSun48 Mar 18, 2025

JoshFerge Mar 18, 2025

MichaelSun48 Mar 18, 2025

JoshFerge Mar 18, 2025

malwilley Mar 18, 2025

MichaelSun48 Mar 18, 2025

malwilley Mar 19, 2025 •

edited

Loading

MichaelSun48 Mar 19, 2025

malwilley Mar 19, 2025 •

edited

Loading

MichaelSun48 Mar 19, 2025

malwilley left a comment

malwilley Mar 19, 2025

malwilley Mar 19, 2025

		return Response(status=status.HTTP_204_NO_CONTENT)


		def _update_view_positions(organization: Organization, user_id: int, view_positions: list[int]):

		@@ -61,33 +61,14 @@ def put(self, request: Request, organization: Organization) -> Response:

		try:
		with transaction.atomic(using=router.db_for_write(GroupSearchViewStarred)):

feat(shared-views): Add groupsearchview starred endpoint for reordering #87347

feat(shared-views): Add groupsearchview starred endpoint for reordering #87347

Conversation

MichaelSun48 commented Mar 18, 2025

codecov bot commented Mar 18, 2025 • edited Loading

Codecov Report

JoshFerge Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malwilley Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malwilley Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malwilley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 18, 2025 •

edited

Loading

JoshFerge Mar 18, 2025 •

edited

Loading

malwilley Mar 19, 2025 •

edited

Loading

malwilley Mar 19, 2025 •

edited

Loading