fix(incidents): Sync Slack participants before visibility check for private incidents

src/firetower/incidents/tests/test_views.py

@@ -344,10 +344,11 @@
             captain=other_user,
         )
 
-        self.client.force_authenticate(user=self.user)
-        response = self.client.post(
-            f"/api/incidents/{incident.incident_number}/sync-participants/"
-        )
+        with patch("firetower.incidents.views.sync_incident_participants_from_slack"):
+            self.client.force_authenticate(user=self.user)
+            response = self.client.post(
+                f"/api/incidents/{incident.incident_number}/sync-participants/"
+            )
 
         assert response.status_code == 404
 
@@ -359,6 +360,216 @@
         assert response.status_code == 400
 
 
+@pytest.mark.django_db
+class TestPrivateIncidentVisibilitySync:
+    """Tests that Slack channel members can access private incidents in the UI
+    even before their participant record has been synced to the DB."""
+
+    def setup_method(self):
+        self.client = APIClient()
+        self.user = User.objects.create_user(
+            username="channel_member@example.com",
+            email="channel_member@example.com",
+            password="testpass123",
+        )
+        self.captain = User.objects.create_user(
+            username="captain@example.com",
+            email="captain@example.com",
+            password="testpass123",
+        )
+
+    def _make_private_incident(self) -> Incident:
+        return Incident.objects.create(
+            title="Private Incident",
+            status=IncidentStatus.ACTIVE,
+            severity=IncidentSeverity.P1,
+            is_private=True,
+            captain=self.captain,
+        )
+
+    def test_detail_view_syncs_before_visibility_check(self):
+        """User in Slack channel but not yet synced gets access after force-sync."""
+        incident = self._make_private_incident()
+
+        def sync_adds_user(inc, force=False):
+            inc.participants.add(self.user)
+            return ParticipantsSyncStats(added=1)
+
+        with patch(
+            "firetower.incidents.views.sync_incident_participants_from_slack",
+            side_effect=sync_adds_user,
+        ):
+            self.client.force_authenticate(user=self.user)
+            response = self.client.get(f"/api/ui/incidents/{incident.incident_number}/")
+
+        assert response.status_code == 200
+        assert response.data["incident"]["id"] == incident.incident_number
+
+    def test_detail_view_404_when_sync_does_not_add_user(self):
+        """User NOT in Slack channel still gets 404 after sync attempt."""
+        incident = self._make_private_incident()
+
+        with patch(
+            "firetower.incidents.views.sync_incident_participants_from_slack",
+            return_value=ParticipantsSyncStats(),
+        ):
+            self.client.force_authenticate(user=self.user)
+            response = self.client.get(f"/api/ui/incidents/{incident.incident_number}/")
+
+        assert response.status_code == 404
+
+    def test_detail_view_404_when_sync_raises(self):
+        """Sync failure on the fallback path results in 404, not 500."""
+        incident = self._make_private_incident()
+
+        with patch(
+            "firetower.incidents.views.sync_incident_participants_from_slack",
+            side_effect=Exception("Slack API error"),
+        ):
+            self.client.force_authenticate(user=self.user)
+            response = self.client.get(f"/api/ui/incidents/{incident.incident_number}/")
+
+        assert response.status_code == 404
+
+    def test_detail_view_fast_path_for_existing_participant(self):
+        """User already in participants skips the fallback sync entirely."""
+        incident = self._make_private_incident()
+        incident.participants.add(self.user)
+
+        with patch(
+            "firetower.incidents.views.sync_incident_participants_from_slack"
+        ) as mock_sync:
+            self.client.force_authenticate(user=self.user)
+            response = self.client.get(f"/api/ui/incidents/{incident.incident_number}/")
+
+        assert response.status_code == 200
+        # Called once from the view's post-fetch sync, NOT from the helper.
+        mock_sync.assert_called_once_with(incident)
+
+    def test_public_incident_does_not_trigger_fallback_sync(self):
+        """Public incidents never hit the fallback sync path."""
+        incident = Incident.objects.create(
+            title="Public Incident",
+            status=IncidentStatus.ACTIVE,
+            severity=IncidentSeverity.P1,
+            is_private=False,
+        )
+
+        with patch(
+            "firetower.incidents.views.sync_incident_participants_from_slack"
+        ) as mock_sync:
+            self.client.force_authenticate(user=self.user)
+            response = self.client.get(f"/api/ui/incidents/{incident.incident_number}/")
+
+        assert response.status_code == 200
+        # Only the view's normal post-fetch sync, not a force sync.
+        mock_sync.assert_called_once_with(incident)
+
+    def test_action_items_view_syncs_before_visibility_check(self):
+        """Action items endpoint also syncs participants for private incidents."""
+        incident = self._make_private_incident()
+        ActionItem.objects.create(
+            incident=incident,
+            linear_issue_id="id-1",
+            linear_identifier="ENG-1",
+            title="Fix the bug",
+            status=ActionItemStatus.TODO,
+            url="https://linear.app/team/issue/ENG-1",
+        )
+
+        def sync_adds_user(inc, force=False):
+            inc.participants.add(self.user)
+            return ParticipantsSyncStats(added=1)
+
+        with (
+            patch(
+                "firetower.incidents.views.sync_incident_participants_from_slack",
+                side_effect=sync_adds_user,
+            ),
+            patch(
+                "firetower.incidents.views.sync_action_items_from_linear",
+            ),
+        ):
+            self.client.force_authenticate(user=self.user)
+            response = self.client.get(
+                f"/api/ui/incidents/{incident.incident_number}/action-items/"
+            )
+
+        assert response.status_code == 200
+        assert len(response.data) == 1
+        assert response.data[0]["title"] == "Fix the bug"
+
+    def test_action_items_view_404_when_not_in_channel(self):
+        """Action items endpoint returns 404 when user is not a channel member."""
+        incident = self._make_private_incident()
+
+        with patch(
+            "firetower.incidents.views.sync_incident_participants_from_slack",
+            return_value=ParticipantsSyncStats(),
+        ):
+            self.client.force_authenticate(user=self.user)
+            response = self.client.get(
+                f"/api/ui/incidents/{incident.incident_number}/action-items/"
+            )
+
+        assert response.status_code == 404
+
+    def test_nonexistent_incident_returns_404(self):
+        """Fallback path does not reveal whether an incident exists."""
+        with patch(
+            "firetower.incidents.views.sync_incident_participants_from_slack"
+        ) as mock_sync:
+            self.client.force_authenticate(user=self.user)
+            response = self.client.get(
+                f"/api/ui/incidents/{settings.PROJECT_KEY}-99999/"
+            )
+
+        assert response.status_code == 404
+        mock_sync.assert_not_called()
+
+    def test_sync_participants_endpoint_no_double_sync(self):
+        """Sync-participants endpoint does not re-sync when the visibility
+        check already performed a force sync."""
+        incident = self._make_private_incident()
+
+        def sync_adds_user(inc, force=False):
+            inc.participants.add(self.user)
+            return ParticipantsSyncStats(added=1)
+
+        with patch(
+            "firetower.incidents.views.sync_incident_participants_from_slack",
+            side_effect=sync_adds_user,
+        ) as mock_sync:
+            self.client.force_authenticate(user=self.user)
+            response = self.client.post(
+                f"/api/incidents/{incident.incident_number}/sync-participants/"
+            )
+
+        assert response.status_code == 200
+        assert response.data["success"] is True
+        assert response.data["stats"]["added"] == 1
+        mock_sync.assert_called_once_with(incident, force=True)
+
+    def test_sync_participants_endpoint_syncs_when_already_visible(self):
+        """Sync-participants endpoint performs sync when user is already
+        visible (no fallback path triggered)."""
+        incident = self._make_private_incident()
+        incident.participants.add(self.user)
+
+        with patch(
+            "firetower.incidents.views.sync_incident_participants_from_slack",
+            return_value=ParticipantsSyncStats(added=0, already_existed=5),
+        ) as mock_sync:
+            self.client.force_authenticate(user=self.user)
+            response = self.client.post(
+                f"/api/incidents/{incident.incident_number}/sync-participants/"
+            )
+
+        assert response.status_code == 200
+        assert response.data["stats"]["already_existed"] == 5
+        mock_sync.assert_called_once_with(incident, force=True)
+
+
 @pytest.mark.django_db
 class TestIncidentAPIViews:
     """Tests for service API endpoints (not UI)"""
@@ -1722,7 +1933,11 @@
             url="https://linear.app/team/issue/ENG-99",
         )
 
-        self.client.force_authenticate(user=other_user)
-        response = self.client.get(self._url(private_incident.incident_number))
+        with patch(
+            "firetower.incidents.views.sync_incident_participants_from_slack",
+            return_value=ParticipantsSyncStats(),
+        ):
+            self.client.force_authenticate(user=other_user)
+            response = self.client.get(self._url(private_incident.incident_number))
 
         assert response.status_code == 404

src/firetower/incidents/views.py

@@ -4,7 +4,9 @@
 from dataclasses import asdict
 
 from django.conf import settings
+from django.contrib.auth.models import User
 from django.db.models import Case, Count, F, QuerySet, When
+from django.http import Http404
 from django.shortcuts import get_object_or_404
 from django.utils import timezone
 from rest_framework import generics, serializers
@@ -77,6 +79,43 @@
     return int(match.group(1))
 
 
+def _get_visible_incident(
+    queryset: QuerySet[Incident], numeric_id: int, user: User
+) -> Incident:
+    """Get an incident, syncing Slack participants first if needed for private incidents.
+
+    Slack commands let channel members interact with private incidents, but
+    the UI requires DB participant records for visibility.  When a channel
+    member hasn't been synced yet, the normal visibility filter returns 404.
+    This helper force-syncs participants on the fallback path so that channel
+    members are recognised on their first UI visit.
+    """
+    visible_qs = filter_visible_to_user(queryset, user)
+    incident = visible_qs.filter(id=numeric_id).first()
+    if incident is not None:
+        return incident
+
+    # Not visible yet -- if the incident exists and is private, the user
+    # might be a Slack channel member who hasn't been synced to the DB.
+    incident = queryset.filter(id=numeric_id).first()
+    if incident is None or not incident.is_private:
+        raise Http404
+
+    try:
+        stats = sync_incident_participants_from_slack(incident, force=True)
+        incident._visibility_sync_stats = stats
+    except Exception:
+        logger.exception(
+            f"Failed to sync participants for incident {incident.id} during visibility check"
+        )
+        raise Http404
+
+    if not incident.is_visible_to_user(user):
+        raise Http404
+
+    return incident
+
+
 class IncidentListUIView(generics.ListAPIView):
     """
     List all incidents from database.
@@ -143,12 +182,9 @@
         """
         incident_id = self.kwargs["incident_id"]
         numeric_id = parse_incident_id(incident_id)
-
-        # Get incident by numeric ID
-        queryset = self.get_queryset()
-        queryset = filter_visible_to_user(queryset, self.request.user)
-
-        incident = get_object_or_404(queryset, id=numeric_id)
+        incident = _get_visible_incident(
+            self.get_queryset(), numeric_id, self.request.user
+        )
 
         try:
             sync_incident_participants_from_slack(incident)
@@ -322,16 +358,18 @@
         """
         incident_id = self.kwargs["incident_id"]
         numeric_id = parse_incident_id(incident_id)
-        queryset = filter_visible_to_user(self.get_queryset(), self.request.user)
-
-        obj = get_object_or_404(queryset, id=numeric_id)
+        obj = _get_visible_incident(self.get_queryset(), numeric_id, self.request.user)
         self.check_object_permissions(self.request, obj)
 
         return obj
 
     def post(self, request: Request, incident_id: str) -> Response:
         incident = self.get_object()
 
+        visibility_stats = getattr(incident, "_visibility_sync_stats", None)
+        if visibility_stats is not None:
+            return Response({"success": True, "stats": asdict(visibility_stats)})
+
         try:
             stats = sync_incident_participants_from_slack(incident, force=True)
             return Response({"success": True, "stats": asdict(stats)})
@@ -374,10 +412,11 @@
     def _get_incident(self) -> Incident:
         if not hasattr(self, "_incident"):
             numeric_id = parse_incident_id(self.kwargs["incident_id"])
-            queryset = filter_visible_to_user(Incident.objects.all(), self.request.user)
-            self._incident = get_object_or_404(queryset, id=numeric_id)
+            self._incident = _get_visible_incident(
+                Incident.objects.all(), numeric_id, self.request.user
+            )
             self.check_object_permissions(self.request, self._incident)
         return self._incident
 
     def list(self, request: Request, *args: object, **kwargs: object) -> Response:
         incident = self._get_incident()
@@ -401,8 +440,7 @@
 
     def get_object(self) -> Incident:
         numeric_id = parse_incident_id(self.kwargs["incident_id"])
-        queryset = filter_visible_to_user(self.get_queryset(), self.request.user)
-        obj = get_object_or_404(queryset, id=numeric_id)
+        obj = _get_visible_incident(self.get_queryset(), numeric_id, self.request.user)
         self.check_object_permissions(self.request, obj)
         return obj