feat(issue-views): Update endpoints to send and receive page filters #83937

MichaelSun48 · 2025-01-23T17:45:24Z

This PR updates the two Groupsearch view endpoints:

PUT ../group-search-view: updated to optionally accept the four new page filter parameters. The frontend will be updated shortly to send over these parameters.
GET ../group-search-view: updated to return the four new page filter parameters. The frontend will not do anything with these parameters until later.
- validating projects parameter in this endpoint will come in a later PR

MichaelSun48 · 2025-01-23T17:49:08Z

src/sentry/issues/endpoints/organization_group_search_views.py

+        gsv = GroupSearchView.objects.get(id=view["id"], user_id=user_id)
+        gsv.name = view["name"]
+        gsv.query = view["query"]
+        gsv.query_sort = view["querySort"]
+        gsv.position = position
+        gsv.is_all_projects = view.get("isAllProjects", False)
+
+        if "projects" in view:
+            gsv.projects.set(view["projects"])
+
+        if "environments" in view:
+            gsv.environments = view["environments"]
+
+        if "timeFilters" in view:
+            gsv.time_filters = view["timeFilters"]
+
+        gsv.save()


Can't set projects directly in the .update() since it's an M2M, so we have to do this shenanigans

codecov · 2025-01-23T18:13:00Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
23661	1	23660	289

View the top 1 failed tests by shortest run time

tests.sentry.issues.endpoints.test_organization_group_search_views.OrganizationGroupSearchViewsProjectsTransactionTest::test_invalid_project_ids

Stack Traces | 15.3s run time

#x1B[1m#x1B[.../issues/endpoints/test_organization_group_search_views.py#x1B[0m:584: in test_invalid_project_ids
    assert response.content == {"detail": "One or more projects do not exist"}
#x1B[1m#x1B[31mE   assert b'{"detail":"...o not exist"}' == {'detail': 'O...do not exist'}#x1B[0m
#x1B[1m#x1B[31mE     #x1B[0m
#x1B[1m#x1B[31mE     Full diff:#x1B[0m
#x1B[1m#x1B[31mE     - {#x1B[0m
#x1B[1m#x1B[31mE     -     'detail': 'One or more projects do not exist',#x1B[0m
#x1B[1m#x1B[31mE     ? ^^^^       ^ ^^                                  ^#x1B[0m
#x1B[1m#x1B[31mE     + (b'{"detail":"One or more projects do not exist"}')#x1B[0m
#x1B[1m#x1B[31mE     ? ^^ ++      ^ ^                                 ++ ^#x1B[0m
#x1B[1m#x1B[31mE     - }#x1B[0m

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

malwilley

@MichaelSun48 do you think there is value in feature flagging any of this logic? I'm just a bit worried about adding so much code that we can't easily roll back

malwilley · 2025-01-23T23:17:24Z

src/sentry/issues/endpoints/organization_group_search_views.py

+            try:
+                validate_projects(organization, request.user, view)
+            except ValidationError as e:
+                sentry_sdk.capture_message(e.args[0])


Normally we wouldn't want to capture the error since this is expected. Did you add this just to be certain nothing is wrong during rollout?

Yeah that's right, the frontend should never be sending invalid projects anyways, so capturing this is mostly just to verify that.

malwilley · 2025-01-23T23:17:55Z

src/sentry/issues/endpoints/organization_group_search_views.py

+def validate_projects(
+    org: Organization, user: User | AnonymousUser, view: GroupSearchViewValidatorResponse
+) -> None:
+    if "projects" in view and view["projects"] is not None:


nit: there are quite a few if statements here. IMO it's more readable if you add early returns instead, but this is just a style preference so feel free to ignore

src/sentry/issues/endpoints/organization_group_search_views.py

malwilley · 2025-01-24T00:01:28Z

src/sentry/issues/endpoints/organization_group_search_views.py

+            view["isAllProjects"] = True
+            view["projects"] = []
+        else:
+            projects = Project.objects.filter(


This might be a bit slow if you have many views since it does each in sequence. Do you think it's necessary to check this? The db transaction should fail if the project ID doesn't exist right?

I didn't consider that, great point! Yeah this definitely would have added a lot of unnecessary database queries

tests/sentry/issues/endpoints/test_organization_group_search_views.py

MichaelSun48 · 2025-01-24T17:34:56Z

do you think there is value in feature flagging any of this logic? I'm just a bit worried about adding so much code that we can't easily roll back

@malwilley Yeah I think I was initially adverse because it seemed pretty complicated ot split some of the serializer logic, but looking at this again, I don't think it should be too bad. I'll add that in in the next PR, good suggestion! fwiw, I think these changes should be pretty safe, assuming I correctly wrote it in a way that doesn't interfere with how the frontend works right now.

malwilley · 2025-01-24T18:08:58Z

@MichaelSun48 no need to split if the changes are indeed trivial. I'd just make sure to run this locally in the UI and make sure that things are working well (for global views permissions on/off) before merging since the unit tests can't catch everything

malwilley · 2025-01-24T23:58:32Z

tests/sentry/issues/endpoints/test_organization_group_search_views.py

+        response = self.client.put(
+            url, data={"views": views}, format="json", content_type="application/json"
+        )
+        assert response.status_code == 500


This should respond with a 4xx code since invalid project IDs are a bad request. Anytime I see a 500 I think it's an unexpected error

I'm actually not sure if this is possible since this check occurs when the transaction is committed and not when the view is actually updated, so it might be impossible to tell what caused the integrity violation (though I will dig deeper into this)

We could query the Projects table again and check, for each view, if it's projects exist in the QuerySet, but then the only purpose of that data query would be to make the error message more accurate...

Probably not worth making the extra request, but I hope there is a way to discern the type of violation because I'm sure we'll get some Sentry issues at some point and will want to know if it's expected behavior or not

malwilley · 2025-01-24T23:59:42Z

src/sentry/issues/endpoints/organization_group_search_views.py

@@ -129,6 +157,20 @@ def bulk_update_views(
            _update_existing_view(org, user_id, view, position=idx)


+def pick_default_project(org: Organization, user: User | AnonymousUser) -> int:


This isn't used anymore right?

Nope! But it will eventually be used for the get endpoint 😅

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

MichaelSun48 commented Jan 23, 2025

View reviewed changes

MichaelSun48 force-pushed the msun/issueViews/updatePUTWithPageFilterParams branch from 6abadbe to de738a2 Compare January 23, 2025 17:52

MichaelSun48 requested a review from malwilley January 23, 2025 17:53

vercel bot deployed to Preview January 23, 2025 17:55 View deployment

MichaelSun48 added 2 commits January 23, 2025 10:35

Update endpoints to send and receive page filters

13469a4

Fix type errors

c6e4f22

MichaelSun48 force-pushed the msun/issueViews/updatePUTWithPageFilterParams branch from de738a2 to c6e4f22 Compare January 23, 2025 19:02

vercel bot deployed to Preview January 23, 2025 19:06 View deployment

Fix broken regression test

5f27ee6

MichaelSun48 marked this pull request as ready for review January 23, 2025 21:17

MichaelSun48 requested a review from a team as a code owner January 23, 2025 21:17

vercel bot deployed to Preview January 23, 2025 21:20 View deployment

malwilley reviewed Jan 24, 2025

View reviewed changes

MichaelSun48 added 2 commits January 24, 2025 13:54

Query projects table once for validation

8e0a315

remove temporary test class

ed36997

vercel bot deployed to Preview January 24, 2025 22:01 View deployment

Update test to correctly fail in transaction

5c23dad

vercel bot deployed to Preview January 24, 2025 23:34 View deployment

MichaelSun48 requested a review from malwilley January 24, 2025 23:53

malwilley reviewed Jan 24, 2025

View reviewed changes

Distinguish between failure modes in integrity error

237054b

vercel bot deployed to Preview January 25, 2025 00:22 View deployment

Fix type error

a053fdd

vercel bot deployed to Preview January 25, 2025 00:38 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(issue-views): Update endpoints to send and receive page filters #83937

feat(issue-views): Update endpoints to send and receive page filters #83937

MichaelSun48 commented Jan 23, 2025 •

edited

Loading

MichaelSun48 Jan 23, 2025

codecov bot commented Jan 23, 2025 •

edited

Loading

malwilley left a comment

malwilley Jan 23, 2025

MichaelSun48 Jan 24, 2025

malwilley Jan 23, 2025

malwilley Jan 24, 2025

MichaelSun48 Jan 24, 2025

MichaelSun48 commented Jan 24, 2025

malwilley commented Jan 24, 2025

malwilley Jan 24, 2025

MichaelSun48 Jan 25, 2025 •

edited

Loading

malwilley Jan 25, 2025

malwilley Jan 24, 2025

MichaelSun48 Jan 25, 2025

		@@ -129,6 +157,20 @@ def bulk_update_views(
		_update_existing_view(org, user_id, view, position=idx)


		def pick_default_project(org: Organization, user: User \| AnonymousUser) -> int:

feat(issue-views): Update endpoints to send and receive page filters #83937

Are you sure you want to change the base?

feat(issue-views): Update endpoints to send and receive page filters #83937

Conversation

MichaelSun48 commented Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 23, 2025 • edited Loading

❌ 1 Tests Failed:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelSun48 commented Jan 24, 2025

malwilley commented Jan 24, 2025

Choose a reason for hiding this comment

MichaelSun48 Jan 25, 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

MichaelSun48 commented Jan 23, 2025 •

edited

Loading

codecov bot commented Jan 23, 2025 •

edited

Loading

MichaelSun48 Jan 25, 2025 •

edited

Loading