Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(shared-views): Remove position usage in groupsearchview serializer #87367

Merged
merged 2 commits into from
Mar 19, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/sentry/api/serializers/models/groupsearchview.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ class GroupSearchViewSerializerResponse(TypedDict):
name: str
query: str
querySort: SORT_LITERALS
position: int
projects: list[int]
isAllProjects: bool
environments: list[str]
Expand All @@ -23,6 +22,10 @@ class GroupSearchViewSerializerResponse(TypedDict):
dateUpdated: str


class GroupSearchViewStarredSerializerResponse(GroupSearchViewSerializerResponse):
position: int


@register(GroupSearchView)
class GroupSearchViewSerializer(Serializer):
def __init__(self, *args, **kwargs):
Expand Down Expand Up @@ -67,7 +70,6 @@ def serialize(self, obj, attrs, user, **kwargs) -> GroupSearchViewSerializerResp
"name": obj.name,
"query": obj.query,
"querySort": obj.query_sort,
"position": obj.position,
"projects": projects,
"isAllProjects": is_all_projects,
"environments": obj.environments,
Expand All @@ -86,7 +88,7 @@ def __init__(self, *args, **kwargs):
self.organization = kwargs.pop("organization", None)
super().__init__(*args, **kwargs)

def serialize(self, obj, attrs, user, **kwargs) -> GroupSearchViewSerializerResponse:
def serialize(self, obj, attrs, user, **kwargs) -> GroupSearchViewStarredSerializerResponse:
serialized_view: GroupSearchViewSerializerResponse = serialize(
obj.group_search_view,
user,
Expand Down
61 changes: 45 additions & 16 deletions src/sentry/issues/endpoints/organization_group_search_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,13 @@
from sentry.api.helpers.group_index.validators import ValidationError
from sentry.api.paginator import SequencePaginator
from sentry.api.serializers import serialize
from sentry.api.serializers.models.groupsearchview import (
GroupSearchViewSerializer,
GroupSearchViewStarredSerializer,
)
from sentry.api.serializers.models.groupsearchview import GroupSearchViewStarredSerializer
from sentry.api.serializers.rest_framework.groupsearchview import (
GroupSearchViewValidator,
GroupSearchViewValidatorResponse,
)
from sentry.models.groupsearchview import DEFAULT_TIME_FILTER, GroupSearchView
from sentry.models.groupsearchviewlastvisited import GroupSearchViewLastVisited
from sentry.models.groupsearchviewstarred import GroupSearchViewStarred
from sentry.models.organization import Organization
from sentry.models.project import Project
Expand Down Expand Up @@ -157,7 +155,9 @@ def put(self, request: Request, organization: Organization) -> Response:

try:
with transaction.atomic(using=router.db_for_write(GroupSearchView)):
bulk_update_views(organization, request.user.id, validated_data["views"])
new_view_state = bulk_update_views(
organization, request.user.id, validated_data["views"]
)
except IntegrityError as e:
if (
len(e.args) > 0
Expand All @@ -171,13 +171,38 @@ def put(self, request: Request, organization: Organization) -> Response:
)
return Response(status=status.HTTP_500_INTERNAL_SERVER_ERROR)

query = GroupSearchView.objects.filter(organization=organization, user_id=request.user.id)
last_visited_views = GroupSearchViewLastVisited.objects.filter(
organization=organization,
user_id=request.user.id,
group_search_view_id__in=[view.id for view in new_view_state],
)
last_visited_map = {lv.group_search_view_id: lv.last_visited for lv in last_visited_views}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was the issue: I was sending over an entire GroupSearchViewLastVisited entry rather than just the last_visited timestamp, which was causing the serializability error.


return self.paginate(
request=request,
queryset=query,
order_by="position",
on_results=lambda x: serialize(x, request.user, serializer=GroupSearchViewSerializer()),
paginator=SequencePaginator(
[
(
idx,
{
"id": str(view.id),
"name": view.name,
"query": view.query,
"querySort": view.query_sort,
"projects": list(view.projects.values_list("id", flat=True)),
"isAllProjects": view.is_all_projects,
"environments": view.environments,
"timeFilters": view.time_filters,
"dateCreated": view.date_added,
"dateUpdated": view.date_updated,
"lastVisited": last_visited_map.get(view.id, None),
"position": idx,
},
)
for idx, view in enumerate(new_view_state)
]
),
on_results=lambda results: serialize(results, request.user),
)


Expand All @@ -198,16 +223,18 @@ def validate_projects(

def bulk_update_views(
org: Organization, user_id: int, views: list[GroupSearchViewValidatorResponse]
) -> None:
) -> list[GroupSearchView]:
existing_view_ids = [view["id"] for view in views if "id" in view]

_delete_missing_views(org, user_id, view_ids_to_keep=existing_view_ids)

created_views = []
for idx, view in enumerate(views):
if "id" not in view:
_create_view(org, user_id, view, position=idx)
created_views.append(_create_view(org, user_id, view, position=idx))
else:
_update_existing_view(org, user_id, view, position=idx)
created_views.append(_update_existing_view(org, user_id, view, position=idx))

return created_views


def pick_default_project(org: Organization, user: User | AnonymousUser) -> int | None:
Expand All @@ -230,7 +257,7 @@ def _delete_missing_views(org: Organization, user_id: int, view_ids_to_keep: lis

def _update_existing_view(
org: Organization, user_id: int, view: GroupSearchViewValidatorResponse, position: int
) -> None:
) -> GroupSearchView:
try:
gsv = GroupSearchView.objects.get(id=view["id"], user_id=user_id)
gsv.name = view["name"]
Expand All @@ -255,17 +282,18 @@ def _update_existing_view(
group_search_view=gsv,
defaults={"position": position},
)
return gsv
except GroupSearchView.DoesNotExist:
# It is possible – though unlikely under normal circumstances – for a view to come in that
# doesn't exist anymore. If, for example, the user has the issue stream open in separate
# windows, deletes a view in one window, then updates it in the other before refreshing.
# In this case, we decide to recreate the tab instead of leaving it deleted.
_create_view(org, user_id, view, position)
return _create_view(org, user_id, view, position)


def _create_view(
org: Organization, user_id: int, view: GroupSearchViewValidatorResponse, position: int
) -> None:
) -> GroupSearchView:
gsv = GroupSearchView.objects.create(
organization=org,
user_id=user_id,
Expand All @@ -286,3 +314,4 @@ def _create_view(
group_search_view=gsv,
position=position,
)
return gsv
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
from django.utils import timezone
from rest_framework.exceptions import ErrorDetail

from sentry.api.serializers.base import serialize
from sentry.api.serializers.rest_framework.groupsearchview import GroupSearchViewValidatorResponse
from sentry.issues.endpoints.organization_group_search_views import DEFAULT_VIEWS
from sentry.models.groupsearchview import GroupSearchView
Expand Down Expand Up @@ -136,7 +135,12 @@ def test_get_user_one_custom_views(self) -> None:
self.login_as(user=self.user)
response = self.get_success_response(self.organization.slug)

assert response.data == serialize(objs["user_one_views"])
assert response.data[0]["id"] == str(objs["user_one_views"][0].id)
assert response.data[0]["position"] == 0
assert response.data[1]["id"] == str(objs["user_one_views"][1].id)
assert response.data[1]["position"] == 1
assert response.data[2]["id"] == str(objs["user_one_views"][2].id)
assert response.data[2]["position"] == 2

@with_feature({"organizations:issue-stream-custom-views": True})
@with_feature({"organizations:global-views": True})
Expand Down Expand Up @@ -169,7 +173,10 @@ def test_get_user_two_custom_views(self) -> None:
self.login_as(user=self.user_2)
response = self.get_success_response(self.organization.slug)

assert response.data == serialize(objs["user_two_views"])
assert response.data[0]["id"] == str(objs["user_two_views"][0].id)
assert response.data[0]["position"] == 0
assert response.data[1]["id"] == str(objs["user_two_views"][1].id)
assert response.data[1]["position"] == 1

@with_feature({"organizations:issue-stream-custom-views": True})
@with_feature({"organizations:global-views": True})
Expand Down Expand Up @@ -278,6 +285,23 @@ def test_adds_view_with_no_id(self) -> None:
assert starred_views[idx].position == view["position"]
assert str(starred_views[idx].group_search_view.id) == view["id"]

@with_feature({"organizations:issue-stream-custom-views": True})
@with_feature({"organizations:global-views": True})
@freeze_time("2025-03-07T00:00:00Z")
def test_response_with_last_visited(self) -> None:
GroupSearchViewLastVisited.objects.create(
user_id=self.user.id,
organization=self.organization,
group_search_view=self.base_data["user_one_views"][0],
last_visited=timezone.now(),
)

views = self.client.get(self.url).data
response = self.get_success_response(self.organization.slug, views=views)

assert response.data[0]["lastVisited"] == timezone.now()
assert response.data[1]["lastVisited"] is None
Comment on lines +288 to +303
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test correctly reproduced the error and now passes with the fix.


@with_feature({"organizations:issue-stream-custom-views": True})
@with_feature({"organizations:global-views": True})
def test_reorder_views(self) -> None:
Expand Down
Loading