feat(savedsearch): Use visibility in OrganizationSearchesEndpoint (getsentry#40942)

evanpurkhiser · web-flow · commit 0662646664c8 · 2022-11-04T22:58:42.000+11:00
diff --git a/src/sentry/api/endpoints/organization_searches.py b/src/sentry/api/endpoints/organization_searches.py
@@ -18,6 +18,14 @@ class OrganizationSearchSerializer(serializers.Serializer):
     sort = serializers.ChoiceField(
         choices=SortOptions.as_choices(), default=SortOptions.DATE, required=False
     )
+    # TODO(epurkhiser): Once the frontend is deployed we should change this to
+    # default to OWNER since that is a more sane default than organization
+    # visibile.
+    visibility = serializers.ChoiceField(
+        choices=Visibility.as_choices(include_pinned=False),
+        default=Visibility.ORGANIZATION,
+        required=False,
+    )
 
 
 @region_silo_endpoint
@@ -39,49 +47,58 @@ def get(self, request: Request, organization) -> Response:
             search_type = SearchType(int(request.GET.get("type", 0)))
         except ValueError as e:
             return Response({"detail": "Invalid input for `type`. Error: %s" % str(e)}, status=400)
-        org_searches_q = Q(Q(owner=request.user) | Q(owner__isnull=True), organization=organization)
-        global_searches_q = Q(is_global=True)
-        saved_searches = list(
-            SavedSearch.objects.filter(org_searches_q | global_searches_q, type=search_type).extra(
-                select={"has_owner": "owner_id is not null", "name__upper": "UPPER(name)"},
-                order_by=["-has_owner", "name__upper"],
+
+        query = (
+            SavedSearch.objects
+            # Do not include pinned or personal searches from other users in
+            # the same organization. DOES include the requesting users pinned
+            # search
+            .exclude(
+                ~Q(owner=request.user),
+                visibility__in=(Visibility.OWNER, Visibility.OWNER_PINNED),
             )
+            .filter(
+                Q(organization=organization) | Q(is_global=True),
+                type=search_type,
+            )
+            .extra(order_by=["name"])
         )
 
-        return Response(serialize(saved_searches, request.user))
+        return Response(serialize(list(query), request.user))
 
     def post(self, request: Request, organization) -> Response:
         serializer = OrganizationSearchSerializer(data=request.data)
 
-        if serializer.is_valid():
-            result = serializer.validated_data
-            # Prevent from creating duplicate queries
-            if SavedSearch.objects.filter(
-                Q(is_global=True) | Q(organization=organization, owner__isnull=True),
-                query=result["query"],
-            ).exists():
-                return Response(
-                    {"detail": "Query {} already exists".format(result["query"])}, status=400
-                )
+        if not serializer.is_valid():
+            return Response(serializer.errors, status=400)
 
-            saved_search = SavedSearch.objects.create(
-                organization=organization,
-                type=result["type"],
-                name=result["name"],
-                query=result["query"],
-                sort=result["sort"],
-                # NOTE: We have not yet exposed the API for setting the
-                # visibility of a saved search, but we don't want to use
-                # the model default of 'owner'. Existing is to be visible
-                # to the organization.
-                visibility=Visibility.ORGANIZATION,
-            )
-            analytics.record(
-                "organization_saved_search.created",
-                search_type=SearchType(saved_search.type).name,
-                org_id=organization.id,
-                query=saved_search.query,
+        result = serializer.validated_data
+
+        # Prevent from creating duplicate queries
+        if (
+            SavedSearch.objects
+            # Query duplication for pinned searches is fine, exlcuded these
+            .exclude(visibility=Visibility.OWNER_PINNED)
+            .filter(Q(is_global=True) | Q(organization=organization), query=result["query"])
+            .exists()
+        ):
+            return Response(
+                {"detail": "Query {} already exists".format(result["query"])}, status=400
             )
-            return Response(serialize(saved_search, request.user))
 
-        return Response(serializer.errors, status=400)
+        saved_search = SavedSearch.objects.create(
+            organization=organization,
+            owner=request.user,
+            type=result["type"],
+            name=result["name"],
+            query=result["query"],
+            sort=result["sort"],
+            visibility=result["visibility"],
+        )
+        analytics.record(
+            "organization_saved_search.created",
+            search_type=SearchType(saved_search.type).name,
+            org_id=organization.id,
+            query=saved_search.query,
+        )
+        return Response(serialize(saved_search, request.user))
diff --git a/src/sentry/models/savedsearch.py b/src/sentry/models/savedsearch.py
@@ -70,18 +70,12 @@ class SavedSearch(Model):
     # is_global does NOT have an associated organization_id
     is_global = models.NullBooleanField(null=True, default=False, db_index=True)
 
-    # XXX(epurkhiser): This is different from "creator". Owner is a misnomer
-    # for this column, as this actually indicates that the search is "pinned"
-    # by the user. A user may only have one pinned search epr (org, type)
-    #
-    # XXX(epurkhiser): Once the visibility column is correctly in use this
-    # column will be used essentially as "created_by"
+    # Creator of the saved search. When visibility is
+    # Visibility.{OWNER,OWNER_PINNED} this field is used to constrain who the
+    # search is visibile to.
     owner = FlexibleForeignKey("sentry.User", null=True)
 
     # Defines who can see the saved search
-    #
-    # NOTE: `owner_pinned` has special behavior in that the saved search will
-    # not appear in the user saved search list
     visibility = models.CharField(
         max_length=16, default=Visibility.OWNER, choices=Visibility.as_choices(include_pinned=True)
     )
diff --git a/tests/sentry/api/endpoints/test_organization_searches.py b/tests/sentry/api/endpoints/test_organization_searches.py
@@ -20,72 +20,106 @@ def get_response(self, *args, **params):
         return super().get_response(*args, **params)
 
     def create_base_data(self):
+        user_1 = self.user
+        user_2 = self.create_user()
+
+        self.create_member(organization=self.organization, user=user_2)
+
         # Depending on test we run migrations in Django 1.8. This causes
         # extra rows to be created, so remove them to keep this test working
         SavedSearch.objects.filter(is_global=True).delete()
 
-        SavedSearch.objects.create(
-            name="foo",
+        # Note names are prefixed with A-Z to make it easy to understand the sorting
+
+        savedsearch_global = SavedSearch.objects.create(
+            name="A Global Query",
+            query="is:unresolved",
+            sort=SortOptions.DATE,
+            is_global=True,
+            visibility=Visibility.ORGANIZATION,
+            date_added=timezone.now(),
+        )
+        savedsearch_org = SavedSearch.objects.create(
+            organization=self.organization,
+            owner=user_1,
+            name="B Simple SavedSearch 1",
             query="some test",
+            sort=SortOptions.NEW,
+            visibility=Visibility.ORGANIZATION,
+            date_added=timezone.now(),
+        )
+        savedsearch_org_diff_owner = SavedSearch.objects.create(
+            organization=self.organization,
+            owner=user_2,
+            name="C Simple SavedSearch for same org diff owner",
+            query="some other test",
+            sort=SortOptions.DATE,
+            visibility=Visibility.ORGANIZATION,
+            date_added=timezone.now(),
+        )
+        savedsearch_owner_me = SavedSearch.objects.create(
+            organization=self.organization,
+            owner=user_1,
+            name="D My personal search",
+            query="some other test",
             sort=SortOptions.DATE,
+            visibility=Visibility.OWNER,
             date_added=timezone.now(),
         )
-        SavedSearch.objects.create(
+        savedsearch_other_owner = SavedSearch.objects.create(
             organization=self.organization,
-            owner=self.create_user(),
-            name="foo",
-            query="some other user's query",
+            owner=user_2,
+            name="E Other user personal search",
+            query="whatever",
             sort=SortOptions.DATE,
+            visibility=Visibility.OWNER,
             date_added=timezone.now(),
         )
-        included = [
-            SavedSearch.objects.create(
-                name="Global Query",
-                query="is:unresolved",
-                sort=SortOptions.DATE,
-                is_global=True,
-                date_added=timezone.now(),
-            ),
-            SavedSearch.objects.create(
-                organization=self.organization,
-                name="foo",
-                query="some test",
-                sort=SortOptions.DATE,
-                date_added=timezone.now(),
-            ),
-            SavedSearch.objects.create(
-                organization=self.organization,
-                name="wat",
-                query="is:unassigned is:unresolved",
-                sort=SortOptions.NEW,
-                date_added=timezone.now(),
-            ),
-        ]
-        return included
+        savedsearch_my_pinned = SavedSearch.objects.create(
+            organization=self.organization,
+            owner=user_1,
+            name="F My pinned search",
+            query="whatever",
+            sort=SortOptions.DATE,
+            visibility=Visibility.OWNER_PINNED,
+            date_added=timezone.now(),
+        )
+        savedsearch_other_pinned = SavedSearch.objects.create(
+            organization=self.organization,
+            owner=user_2,
+            name="G Other user pinned search",
+            query="whatever",
+            sort=SortOptions.DATE,
+            visibility=Visibility.OWNER_PINNED,
+            date_added=timezone.now(),
+        )
+
+        return {
+            "savedsearch_global": savedsearch_global,
+            "savedsearch_org": savedsearch_org,
+            "savedsearch_org_diff_owner": savedsearch_org_diff_owner,
+            "savedsearch_owner_me": savedsearch_owner_me,
+            "savedsearch_other_owner": savedsearch_other_owner,
+            "savedsearch_my_pinned": savedsearch_my_pinned,
+            "savedsearch_other_pinned": savedsearch_other_pinned,
+        }
 
     def check_results(self, expected):
         self.login_as(user=self.user)
-        expected.sort(key=lambda search: (not search.is_pinned, search.name.lower()))
         response = self.get_success_response(self.organization.slug)
         assert response.data == serialize(expected)
 
     def test_simple(self):
-        included = self.create_base_data()
-        self.check_results(included)
-
-    def test_pinned(self):
-        included = self.create_base_data()
-        pinned_query = SavedSearch.objects.create(
-            organization=self.organization,
-            owner=self.user,
-            name="My Pinned Query",
-            query="pinned junk",
-            sort=SortOptions.NEW,
-            date_added=timezone.now(),
-            visibility=Visibility.OWNER_PINNED,
+        objs = self.create_base_data()
+        self.check_results(
+            [
+                objs["savedsearch_global"],
+                objs["savedsearch_org"],
+                objs["savedsearch_org_diff_owner"],
+                objs["savedsearch_owner_me"],
+                objs["savedsearch_my_pinned"],
+            ]
         )
-        included.append(pinned_query)
-        self.check_results(included)
 
 
 @region_silo_test
@@ -109,19 +143,30 @@ def test_simple(self):
         search_type = SearchType.ISSUE.value
         name = "test"
         query = "hello"
+        visibility = Visibility.ORGANIZATION
+
         self.login_as(user=self.manager)
         resp = self.get_success_response(
-            self.organization.slug, type=search_type, name=name, query=query
+            self.organization.slug,
+            type=search_type,
+            name=name,
+            query=query,
+            visibility=visibility,
         )
         assert resp.data["name"] == name
         assert resp.data["query"] == query
         assert resp.data["type"] == search_type
+        assert resp.data["visibility"] == visibility
         assert SavedSearch.objects.filter(id=resp.data["id"]).exists()
 
-    def test_perms(self):
+    def test_member_cannot_create_org_search(self):
         self.login_as(user=self.member)
         resp = self.get_response(
-            self.organization.slug, type=SearchType.ISSUE.value, name="hello", query="test"
+            self.organization.slug,
+            type=SearchType.ISSUE.value,
+            name="hello",
+            query="test",
+            Visibility=Visibility.ORGANIZATION,
         )
         assert resp.status_code == 403
 
@@ -131,6 +176,7 @@ def test_exists(self):
             name="Some global search",
             query="is:unresolved",
             is_global=True,
+            visibility=Visibility.ORGANIZATION,
         )
         self.login_as(user=self.manager)
         resp = self.get_response(
@@ -147,20 +193,26 @@ def test_exists(self):
             type=SearchType.ISSUE.value,
             name="Some org search",
             query="org search",
+            visibility=Visibility.ORGANIZATION,
         )
         resp = self.get_response(
             self.organization.slug,
             type=SearchType.ISSUE.value,
             name="hello",
             query=org_search.query,
+            visibility=Visibility.ORGANIZATION,
         )
         assert resp.status_code == 400
         assert "already exists" in resp.data["detail"]
 
     def test_empty(self):
         self.login_as(user=self.manager)
         resp = self.get_response(
-            self.organization.slug, type=SearchType.ISSUE.value, name="hello", query=""
+            self.organization.slug,
+            type=SearchType.ISSUE.value,
+            name="hello",
+            query="",
+            visibility=Visibility.ORGANIZATION,
         )
         assert resp.status_code == 400
         assert "This field may not be blank." == resp.data["query"][0]
