diff --git a/products/signals/backend/test/test_signal_report_api.py b/products/signals/backend/test/test_signal_report_api.py index 62760ea7ff3e..b181095d1801 100644 --- a/products/signals/backend/test/test_signal_report_api.py +++ b/products/signals/backend/test/test_signal_report_api.py @@ -1,10 +1,13 @@ import json +import uuid from datetime import timedelta +from types import SimpleNamespace from urllib.parse import urlencode from posthog.test.base import APIBaseTest from unittest.mock import patch +from django.core.cache import cache from django.utils import timezone from parameterized import parameterized @@ -1209,3 +1212,96 @@ def test_restore_preserves_title_and_summary(self): assert report.status == SignalReport.Status.READY assert report.title == "Original title" assert report.summary == "Original summary" + + +class TestAvailableReviewersAPI(APIBaseTest): + """GET signals/reports/available_reviewers/: returns every eligible org member (no cap), with server-side search.""" + + def setUp(self): + super().setUp() + # The over-threshold report is throttled via the cache; clear it so each test starts fresh. + cache.clear() + + def _url(self, **query) -> str: + base = f"/api/projects/{self.team.id}/signals/reports/available_reviewers/" + if not query: + return base + return f"{base}?{urlencode(query)}" + + def _fake_user(self, n: int) -> SimpleNamespace: + return SimpleNamespace( + uuid=uuid.UUID(int=n), + first_name=f"User{n:04d}", + last_name="Tester", + email=f"user{n:04d}@example.com", + ) + + def _login_map(self, count: int) -> dict[str, SimpleNamespace]: + return {f"gh{n}": self._fake_user(n) for n in range(count)} + + @patch("products.signals.backend.views.get_org_member_github_login_to_user_map") + def test_returns_all_members_without_cap(self, mock_map): + # 250 > the old hard cap of 100: every member must come back now. + mock_map.return_value = self._login_map(250) + response = self.client.get(self._url()) + assert response.status_code == status.HTTP_200_OK + assert len(response.json()) == 250 + + @patch("products.signals.backend.views.get_org_member_github_login_to_user_map") + def test_search_query_filters_server_side(self, mock_map): + mock_map.return_value = self._login_map(250) + response = self.client.get(self._url(query="User0123")) + assert response.status_code == status.HTTP_200_OK + body = response.json() + assert len(body) == 1 + assert next(iter(body.values()))["email"] == "user0123@example.com" + + @patch("products.signals.backend.views.capture_exception") + @patch("products.signals.backend.views.get_org_member_github_login_to_user_map") + def test_no_exception_captured_under_threshold(self, mock_map, mock_capture): + mock_map.return_value = self._login_map(50) + response = self.client.get(self._url()) + assert response.status_code == status.HTTP_200_OK + mock_capture.assert_not_called() + + @patch("products.signals.backend.views.capture_exception") + @patch("products.signals.backend.views.get_org_member_github_login_to_user_map") + def test_exception_captured_over_threshold(self, mock_map, mock_capture): + mock_map.return_value = self._login_map(1201) + response = self.client.get(self._url()) + assert response.status_code == status.HTTP_200_OK + assert len(response.json()) == 1201 + mock_capture.assert_called_once() + + @patch("products.signals.backend.views.capture_exception") + @patch("products.signals.backend.views.get_org_member_github_login_to_user_map") + def test_threshold_capture_deduplicated_across_requests(self, mock_map, mock_capture): + # Repeated popover opens for the same over-threshold org must report at most once. + mock_map.return_value = self._login_map(1201) + for _ in range(3): + assert self.client.get(self._url()).status_code == status.HTTP_200_OK + mock_capture.assert_called_once() + + @patch("products.signals.backend.views.capture_exception") + @patch("products.signals.backend.views.get_org_member_github_login_to_user_map") + def test_threshold_not_triggered_by_search_requests(self, mock_map, mock_capture): + # A search-as-you-type request must not spam the threshold capture. + mock_map.return_value = self._login_map(1201) + response = self.client.get(self._url(query="User0001")) + assert response.status_code == status.HTTP_200_OK + mock_capture.assert_not_called() + + @patch("products.signals.backend.views.get_org_member_github_login_to_user_map") + def test_empty_org_returns_empty(self, mock_map): + mock_map.return_value = {} + response = self.client.get(self._url()) + assert response.status_code == status.HTTP_200_OK + assert response.json() == {} + + @patch("products.signals.backend.views.get_org_member_github_login_to_user_map") + def test_missing_team_map_returns_empty(self, mock_map): + # The helper returns None for an unknown team; the view coalesces it to an empty result. + mock_map.return_value = None + response = self.client.get(self._url()) + assert response.status_code == status.HTTP_200_OK + assert response.json() == {} diff --git a/products/signals/backend/views.py b/products/signals/backend/views.py index 36a7a52d9f7b..624ec694a4ca 100644 --- a/products/signals/backend/views.py +++ b/products/signals/backend/views.py @@ -4,6 +4,7 @@ from typing import cast from django.conf import settings +from django.core.cache import cache from django.db import IntegrityError, transaction from django.db.models import ( BooleanField, @@ -30,6 +31,7 @@ from django_filters.rest_framework import DjangoFilterBackend from drf_spectacular.types import OpenApiTypes from drf_spectacular.utils import OpenApiParameter, extend_schema, extend_schema_view +from opentelemetry import trace from rest_framework import exceptions, mixins, serializers, status, viewsets from rest_framework.authentication import SessionAuthentication from rest_framework.decorators import action @@ -43,6 +45,7 @@ from posthog.api.routing import TeamAndOrgViewSetMixin from posthog.auth import InternalAPIAuthentication, OAuthAccessTokenAuthentication, PersonalAPIKeyAuthentication +from posthog.exceptions_capture import capture_exception from posthog.models import Team, User from posthog.models.integration import Integration from posthog.models.team.extensions import get_or_create_team_extension @@ -100,6 +103,14 @@ from products.warehouse_sources.backend.models.external_data_schema import ExternalDataSchema logger = structlog.get_logger(__name__) +tracer = trace.get_tracer(__name__) + +# `available_reviewers` returns every eligible org member in a single unpaginated payload. +# Org membership is tiny in practice, so even the biggest +# org today serialises to well under 100 KB. If an org ever exceeds this threshold we want a +# signal that it's time to add real pagination, rather than silently truncating the list (the +# old behaviour, which capped at 100 and dropped everyone alphabetically after ~"M"). +REVIEWER_PAGINATION_THRESHOLD = 1200 class EmitSignalSerializer(serializers.Serializer): @@ -949,36 +960,65 @@ def list(self, request, *args, **kwargs): @extend_schema(exclude=True) @action(detail=False, methods=["get"], url_path="available_reviewers", required_scopes=["task:read"]) def available_reviewers(self, request, **kwargs): - login_to_user = get_org_member_github_login_to_user_map(self.team.id) or {} - query = (request.query_params.get("query") or "").strip().lower() - - users_by_uuid = {str(user.uuid): user for user in login_to_user.values()} - - filtered_users = [ - (user_uuid, user) - for user_uuid, user in users_by_uuid.items() - if not query - or query in f"{user.first_name} {user.last_name}".strip().lower() - or query in (user.email or "").lower() - ] + with tracer.start_as_current_span("signals.available_reviewers") as span: + login_to_user = get_org_member_github_login_to_user_map(self.team.id) or {} + query = (request.query_params.get("query") or "").strip().lower() + + users_by_uuid = {str(user.uuid): user for user in login_to_user.values()} + + candidate_count = len(users_by_uuid) + span.set_attribute("signals.available_reviewers.candidate_count", candidate_count) + + # The full candidate list is returned unpaginated. If an org grows past the threshold, + # report it (non-blocking) so we know to add pagination before the payload gets large. + # `capture_exception` logs an exception and is not deduplicated, so we throttle to at + # most one report per org per day via the cache — otherwise a >threshold org would log + # on every popover open. (The span's candidate_count attribute is recorded every request + # regardless, if a metric-based alert is preferred later.) + if ( + not query + and candidate_count > REVIEWER_PAGINATION_THRESHOLD + and cache.add(f"signals:available_reviewers_over_threshold:{self.team.id}", True, 60 * 60 * 24) + ): + capture_exception( + Exception( + f"available_reviewers exceeded pagination threshold: {candidate_count} " + f"candidates > {REVIEWER_PAGINATION_THRESHOLD}; this endpoint should be paginated." + ), + additional_properties={ + "team_id": self.team.id, + "candidate_count": candidate_count, + "threshold": REVIEWER_PAGINATION_THRESHOLD, + }, + ) - reviewers = { - user_uuid: { - "name": f"{user.first_name} {user.last_name}".strip(), - "email": user.email or "", + filtered_users = [ + (user_uuid, user) + for user_uuid, user in users_by_uuid.items() + if not query + or query in f"{user.first_name} {user.last_name}".strip().lower() + or query in (user.email or "").lower() + ] + + reviewers = { + user_uuid: { + "name": f"{user.first_name} {user.last_name}".strip(), + "email": user.email or "", + } + for user_uuid, user in sorted( + filtered_users, + key=lambda item: ( + (item[1].first_name or "").lower(), + (item[1].last_name or "").lower(), + (item[1].email or "").lower(), + item[0], + ), + ) } - for user_uuid, user in sorted( - filtered_users, - key=lambda item: ( - (item[1].first_name or "").lower(), - (item[1].last_name or "").lower(), - (item[1].email or "").lower(), - item[0], - ), - )[:100] - } - return Response(reviewers) + span.set_attribute("signals.available_reviewers.result_count", len(reviewers)) + + return Response(reviewers) def destroy(self, request, *args, **kwargs): """Soft-delete a report and its signals via the deletion workflow."""