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(issue-views): Update GET endpoint to validate view's projects #84338

Merged
merged 7 commits into from
Feb 6, 2025
Merged
Show file tree
Hide file tree
Changes from 5 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
21 changes: 19 additions & 2 deletions src/sentry/api/serializers/models/groupsearchview.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,32 @@ class GroupSearchViewSerializerResponse(TypedDict):

@register(GroupSearchView)
class GroupSearchViewSerializer(Serializer):
def __init__(self, *args, **kwargs):
self.has_global_views = kwargs.pop("has_global_views", None)
self.default_project = kwargs.pop("default_project", None)
super().__init__(*args, **kwargs)

def serialize(self, obj, attrs, user, **kwargs) -> GroupSearchViewSerializerResponse:
if self.has_global_views is False:
is_all_projects = False

projects = list(obj.projects.values_list("id", flat=True))
Copy link
Member

Choose a reason for hiding this comment

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

Does this make a query for each view? This would be slow for many views and would be better if we could prefetch them in the initial query since we know we'll need it anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

Added! (organization_group_search_views, line 75)

num_projects = len(projects)
if num_projects != 1:
projects = [projects[0] if num_projects > 1 else self.default_project]

else:
is_all_projects = obj.is_all_projects
projects = list(obj.projects.values_list("id", flat=True))

return {
"id": str(obj.id),
"name": obj.name,
"query": obj.query,
"querySort": obj.query_sort,
"position": obj.position,
"projects": list(obj.projects.values_list("id", flat=True)),
"isAllProjects": obj.is_all_projects,
"projects": projects,
"isAllProjects": is_all_projects,
"environments": obj.environments,
"timeFilters": obj.time_filters,
"dateCreated": obj.date_added,
Expand Down
44 changes: 37 additions & 7 deletions src/sentry/issues/endpoints/organization_group_search_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
GroupSearchViewValidator,
GroupSearchViewValidatorResponse,
)
from sentry.models.groupsearchview import GroupSearchView
from sentry.models.groupsearchview import DEFAULT_TIME_FILTER, GroupSearchView
from sentry.models.organization import Organization
from sentry.models.project import Project
from sentry.models.savedsearch import SortOptions
Expand All @@ -31,6 +31,9 @@
"query": "is:unresolved issue.priority:[high, medium]",
"querySort": SortOptions.DATE.value,
"position": 0,
"isAllProjects": False,
"environments": [],
"timeFilters": DEFAULT_TIME_FILTER,
"dateCreated": None,
"dateUpdated": None,
}
Expand Down Expand Up @@ -65,23 +68,53 @@ def get(self, request: Request, organization: Organization) -> Response:
):
return Response(status=status.HTTP_404_NOT_FOUND)

has_global_views = features.has("organizations:global-views", organization)

query = GroupSearchView.objects.filter(organization=organization, user_id=request.user.id)

# Return only the prioritized view if user has no custom views yet
# Return only the default view(s) if user has no custom views yet
if not query.exists():
return self.paginate(
request=request,
paginator=SequencePaginator(
[(idx, view) for idx, view in enumerate(DEFAULT_VIEWS)]
[
(
idx,
{
**view,
"projects": (
[]
if has_global_views
else [pick_default_project(organization, request.user)]
),
},
)
for idx, view in enumerate(DEFAULT_VIEWS)
]
),
on_results=lambda results: serialize(results, request.user),
)

default_project = None
if not has_global_views:
default_project = pick_default_project(organization, request.user)
Copy link
Member

Choose a reason for hiding this comment

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

Technically this query might not be necessary if all the views have a single project, so we can make this even better if we avoid it in that case

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is handled by Django's lazy loading of query sets - so if default project is never accessed in the for loop after this, the DB query isn't actually made.

if default_project is None:
return Response(
status=status.HTTP_400_BAD_REQUEST,
data={"detail": "You do not have access to any projects."},
)

return self.paginate(
request=request,
queryset=query,
order_by="position",
on_results=lambda x: serialize(x, request.user, serializer=GroupSearchViewSerializer()),
on_results=lambda x: serialize(
x,
request.user,
serializer=GroupSearchViewSerializer(
has_global_views=has_global_views, default_project=default_project
),
),
)

def put(self, request: Request, organization: Organization) -> Response:
Expand All @@ -90,7 +123,6 @@ def put(self, request: Request, organization: Organization) -> Response:
will delete any views that are not included in the request, add views if
they are new, and update existing views if they are included in the request.
This endpoint is explcititly designed to be used by our frontend.

"""
if not features.has(
"organizations:issue-stream-custom-views", organization, actor=request.user
Expand Down Expand Up @@ -175,8 +207,6 @@ def pick_default_project(org: Organization, user: User | AnonymousUser) -> int:
.values_list("id", flat=True)
.first()
)
if default_user_project is None:
raise ValidationError("You do not have access to any projects")
return default_user_project


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

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
from sentry.testutils.cases import APITestCase, TransactionTestCase
from sentry.testutils.helpers.features import with_feature
Expand Down Expand Up @@ -366,7 +367,7 @@ def create_base_data_with_page_filters(self) -> list[GroupSearchView]:
query_sort="date",
position=0,
time_filters={"period": "14d"},
environments=["production"],
environments=[],
)
first_custom_view_user_one.projects.set([self.project1])

Expand All @@ -378,7 +379,7 @@ def create_base_data_with_page_filters(self) -> list[GroupSearchView]:
query_sort="new",
position=1,
time_filters={"period": "7d"},
environments=["staging"],
environments=["staging", "production"],
)
second_custom_view_user_one.projects.set([self.project1, self.project2, self.project3])

Expand Down Expand Up @@ -413,7 +414,7 @@ def test_not_including_page_filters_does_not_reset_them_for_existing_views(self)
# Original Page filters
assert views[0]["timeFilters"] == {"period": "14d"}
assert views[0]["projects"] == [self.project1.id]
assert views[0]["environments"] == ["production"]
assert views[0]["environments"] == []

view = views[0]
# Change nothing but the name
Expand All @@ -428,7 +429,7 @@ def test_not_including_page_filters_does_not_reset_them_for_existing_views(self)
# Ensure these have not been changed
assert views[0]["timeFilters"] == {"period": "14d"}
assert views[0]["projects"] == [self.project1.id]
assert views[0]["environments"] == ["production"]
assert views[0]["environments"] == []

@with_feature({"organizations:issue-stream-custom-views": True})
@with_feature({"organizations:global-views": True})
Expand Down Expand Up @@ -584,6 +585,209 @@ def test_invalid_project_ids(self) -> None:
assert response.content == b'{"detail":"One or more projects do not exist"}'


class OrganizationGroupSearchViewsGetPageFiltersTest(APITestCase):
def create_base_data_with_page_filters(self) -> None:
self.team_1 = self.create_team(organization=self.organization, slug="team-1")
self.team_2 = self.create_team(organization=self.organization, slug="team-2")

# User 1 is on team 1 only
user_1 = self.user
self.create_team_membership(user=user_1, team=self.team_1)
# User 2 is on team 1 and team 2
self.user_2 = self.create_user()
self.create_member(
organization=self.organization, user=self.user_2, teams=[self.team_1, self.team_2]
)
# User 3 has no views and should get the default views
self.user_3 = self.create_user()
self.create_member(organization=self.organization, user=self.user_3, teams=[self.team_1])
# User 4 is part of no teams, should error out
self.user_4 = self.create_user()
self.create_member(organization=self.organization, user=self.user_4)

# This project should NEVER get chosen as a default since it does not belong to any teams
self.project1 = self.create_project(
organization=self.organization, slug="project-a", teams=[]
)
# This project should be User 2's default project since it's the alphabetically the first one
self.project2 = self.create_project(
organization=self.organization, slug="project-b", teams=[self.team_2]
)
# This should be User 1's default project since it's the only one that the user has access to
self.project3 = self.create_project(
organization=self.organization, slug="project-c", teams=[self.team_1, self.team_2]
)

first_issue_view_user_one = GroupSearchView.objects.create(
name="Issue View One",
organization=self.organization,
user_id=user_1.id,
query="is:unresolved",
query_sort="date",
position=0,
is_all_projects=False,
time_filters={"period": "14d"},
environments=[],
)
first_issue_view_user_one.projects.set([self.project3])

second_issue_view_user_one = GroupSearchView.objects.create(
name="Issue View Two",
organization=self.organization,
user_id=user_1.id,
query="is:resolved",
query_sort="new",
position=1,
is_all_projects=False,
time_filters={"period": "7d"},
environments=["staging", "production"],
)
second_issue_view_user_one.projects.set([])

third_issue_view_user_one = GroupSearchView.objects.create(
name="Issue View Three",
organization=self.organization,
user_id=user_1.id,
query="is:ignored",
query_sort="freq",
position=2,
is_all_projects=True,
time_filters={"period": "30d"},
environments=["development"],
)
third_issue_view_user_one.projects.set([])

first_issue_view_user_two = GroupSearchView.objects.create(
name="Issue View One",
organization=self.organization,
user_id=self.user_2.id,
query="is:unresolved",
query_sort="date",
position=0,
is_all_projects=False,
time_filters={"period": "14d"},
environments=[],
)
first_issue_view_user_two.projects.set([])

first_issue_view_user_four = GroupSearchView.objects.create(
name="Issue View One",
organization=self.organization,
user_id=self.user_4.id,
query="is:unresolved",
query_sort="date",
position=0,
is_all_projects=False,
time_filters={"period": "14d"},
environments=[],
)
first_issue_view_user_four.projects.set([])

def setUp(self) -> None:
self.create_base_data_with_page_filters()
self.url = reverse(
"sentry-api-0-organization-group-search-views",
kwargs={"organization_id_or_slug": self.organization.slug},
)

@with_feature({"organizations:issue-stream-custom-views": True})
@with_feature({"organizations:global-views": True})
def test_basic_get_page_filters_with_global_filters(self) -> None:
self.login_as(user=self.user)
response = self.client.get(self.url)

assert response.data[0]["timeFilters"] == {"period": "14d"}
assert response.data[0]["projects"] == [self.project3.id]
assert response.data[0]["environments"] == []
assert response.data[0]["isAllProjects"] is False

assert response.data[1]["timeFilters"] == {"period": "7d"}
assert response.data[1]["projects"] == []
assert response.data[1]["environments"] == ["staging", "production"]
assert response.data[1]["isAllProjects"] is False

assert response.data[2]["timeFilters"] == {"period": "30d"}
assert response.data[2]["projects"] == []
assert response.data[2]["environments"] == ["development"]
assert response.data[2]["isAllProjects"] is True

@with_feature({"organizations:issue-stream-custom-views": True})
@with_feature({"organizations:global-views": False})
def test_get_page_filters_without_global_filters(self) -> None:
self.login_as(user=self.user)
response = self.client.get(self.url)

assert response.data[0]["timeFilters"] == {"period": "14d"}
assert response.data[0]["projects"] == [self.project3.id]
assert response.data[0]["environments"] == []
assert response.data[0]["isAllProjects"] is False

assert response.data[1]["timeFilters"] == {"period": "7d"}
assert response.data[1]["projects"] == [self.project3.id]
assert response.data[1]["environments"] == ["staging", "production"]
assert response.data[1]["isAllProjects"] is False

assert response.data[2]["timeFilters"] == {"period": "30d"}
assert response.data[2]["projects"] == [self.project3.id]
assert response.data[2]["environments"] == ["development"]
assert response.data[2]["isAllProjects"] is False

@with_feature({"organizations:issue-stream-custom-views": True})
@with_feature({"organizations:global-views": False})
def test_get_page_filters_without_global_filters_user_2(self) -> None:
self.login_as(user=self.user_2)
response = self.client.get(self.url)

assert response.data[0]["timeFilters"] == {"period": "14d"}
assert response.data[0]["projects"] == [self.project2.id]
assert response.data[0]["environments"] == []
assert response.data[0]["isAllProjects"] is False

@with_feature({"organizations:issue-stream-custom-views": True})
@with_feature({"organizations:global-views": True})
def test_default_page_filters_with_global_views(self) -> None:
self.login_as(user=self.user_3)
response = self.client.get(self.url)

default_view_queries = {view["query"] for view in DEFAULT_VIEWS}
received_queries = {view["query"] for view in response.data}

assert default_view_queries == received_queries

for view in response.data:
assert view["timeFilters"] == {"period": "14d"}
# Global views means default project should be "My Projects"
assert view["projects"] == []
assert view["environments"] == []
assert view["isAllProjects"] is False

@with_feature({"organizations:issue-stream-custom-views": True})
@with_feature({"organizations:global-views": False})
def test_default_page_filters_without_global_views(self) -> None:
self.login_as(user=self.user_3)
response = self.client.get(self.url)

default_view_queries = {view["query"] for view in DEFAULT_VIEWS}
received_queries = {view["query"] for view in response.data}

assert default_view_queries == received_queries

for view in response.data:
assert view["timeFilters"] == {"period": "14d"}
# No global views means default project should be a single project
assert view["projects"] == [self.project3.id]
assert view["environments"] == []
assert view["isAllProjects"] is False

@with_feature({"organizations:issue-stream-custom-views": True})
@with_feature({"organizations:global-views": False})
def test_error_when_no_projects_found(self) -> None:
self.login_as(user=self.user_4)
response = self.client.get(self.url)
assert response.status_code == 400
assert response.data == {"detail": "You do not have access to any projects."}


class OrganizationGroupSearchViewsPutRegressionTest(APITestCase):
endpoint = "sentry-api-0-organization-group-search-views"
method = "put"
Expand Down
Loading