Repository navigation
feat(replay-vision): add internal notes with @-mentions to observations #122359
+816
−45
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import uuid | ||
| from contextlib import nullcontext | ||
| from datetime import datetime, timedelta | ||
| from typing import TYPE_CHECKING, Any, cast | ||
|
|
@@ -32,6 +33,7 @@ | |
| from posthog.models.comment.comment import ( | ||
| CANVAS_COMMENT_SCOPES, | ||
| COMMENT_SCOPES_BLOCKED_FROM_GENERIC_API, | ||
| OBSERVATION_COMMENT_SCOPES, | ||
| TICKET_COMMENT_SCOPES, | ||
| activity_log_scope_for, | ||
| canonical_comment_scope, | ||
|
|
@@ -51,6 +53,11 @@ | |
| from posthog.tasks.email import send_discussions_mentioned | ||
|
|
||
| from products.conversations.backend import reply_dedupe | ||
| from products.replay_vision.backend.facade.api import ( | ||
| can_view_observation, | ||
| observation_note_write_rejection, | ||
| observation_viewer_user_ids, | ||
| ) | ||
|
|
||
| if TYPE_CHECKING: | ||
| from products.access_control.backend.facade.user_access_control import UserAccessControl | ||
|
|
@@ -90,6 +97,18 @@ | |
| raise exceptions.PermissionDenied("You do not have access to this ticket") | ||
|
|
||
|
|
||
| def _can_view_observation(*, team_id: int, item_id: str | None, user_access_control: "UserAccessControl") -> bool: | ||
| return bool(item_id) and can_view_observation(user_access_control, team_id, cast(str, item_id)) | ||
|
|
||
|
|
||
| def _canonical_observation_id(item_id: Any) -> Any: | ||
| """Store observation ids in one spelling, so lookups and cleanup that compare strings find every note.""" | ||
| try: | ||
| return str(uuid.UUID(str(item_id))) | ||
| except ValueError: | ||
| return item_id | ||
|
|
||
|
|
||
| # A reservation with no posted root older than this is a crashed send — safe to retry. | ||
| STALE_SLACK_RESERVATION_GRACE = timedelta(minutes=2) | ||
|
|
||
|
|
@@ -248,6 +267,11 @@ | |
| def _mentions_allowed_for_comment_target( | ||
| *, team_id: int, scope: str, item_id: str | None, item_context: dict | None, mentioned_user_ids: list[int] | ||
| ) -> list[int]: | ||
| if scope in OBSERVATION_COMMENT_SCOPES: | ||
| if not item_id: | ||
| return [] | ||
| viewer_ids = observation_viewer_user_ids(team_id=team_id, observation_id=item_id, user_ids=mentioned_user_ids) | ||
| return [user_id for user_id in mentioned_user_ids if user_id in viewer_ids] | ||
|
veria-ai[bot] marked this conversation as resolved.
|
||
| if scope not in DESKTOP_COMMENT_SCOPES: | ||
| return mentioned_user_ids | ||
| if scope in CANVAS_COMMENT_SCOPES: | ||
|
|
@@ -370,7 +394,7 @@ | |
| def validate_scope(self, value: str) -> str: | ||
| return canonical_comment_scope(value) | ||
|
|
||
| def validate(self, data): | ||
|
Check warning on line 397 in posthog/api/comments.py
|
||
| request = self.context["request"] | ||
| instance = cast(Comment, self.instance) | ||
|
|
||
|
|
@@ -441,6 +465,19 @@ | |
| scope = root.scope | ||
| item_id = root.item_id | ||
|
|
||
| if scope in OBSERVATION_COMMENT_SCOPES and "item_id" in data: | ||
| data["item_id"] = item_id = _canonical_observation_id(item_id) | ||
|
|
||
| # A note's replies are read through its thread, which checks only the root. Fixing where an observation | ||
| # note sits, and which thread it is in, keeps a whole thread under one access check. | ||
| if instance and ({instance.scope, scope, getattr(source_comment, "scope", None)} & OBSERVATION_COMMENT_SCOPES): | ||
| if (scope, item_id) != (instance.scope, instance.item_id): | ||
| raise exceptions.ValidationError({"item_id": "An observation note cannot move to another item."}) | ||
| if getattr(source_comment, "id", None) != instance.source_comment_id: | ||
| raise exceptions.ValidationError( | ||
| {"source_comment": "An observation note cannot move to another thread."} | ||
| ) | ||
|
|
||
| scopes_and_items = {(scope, item_id)} | ||
| if instance: | ||
| scopes_and_items.add((instance.scope, instance.item_id)) | ||
|
|
@@ -453,6 +490,18 @@ | |
| item_id=target_item_id, | ||
| user_access_control=self.context["get_user_access_control"](), | ||
| ) | ||
| elif target_scope in OBSERVATION_COMMENT_SCOPES: | ||
| rejection = ( | ||
| observation_note_write_rejection( | ||
| self.context["get_user_access_control"](), self.context["get_team"]().id, target_item_id | ||
| ) | ||
| if target_item_id | ||
| else "forbidden" | ||
| ) | ||
| if rejection == "forbidden": | ||
| raise exceptions.PermissionDenied("You do not have access to this observation") | ||
| if rejection == "not_finished": | ||
| raise exceptions.ValidationError({"item_id": "You can add notes after the scan succeeds."}) | ||
|
|
||
| target_scope = data.get("scope", instance.scope if instance else None) | ||
| target_item_id = data.get("item_id", instance.item_id if instance else None) | ||
|
|
@@ -697,7 +746,7 @@ | |
| scope_object = "comment" | ||
| scope_object_read_actions = ["list", "retrieve", "thread", "count"] | ||
|
|
||
| def dangerously_get_required_scopes(self, request: Request, view: Any) -> list[str] | None: | ||
|
Check warning on line 749 in posthog/api/comments.py
|
||
| """Ticket-scoped comments require ticket API scope access instead of comment access. | ||
|
|
||
| Candidate scopes are the union of every scope that determines what the request can | ||
|
|
@@ -734,12 +783,17 @@ | |
| except (ValueError, django_exceptions.ValidationError): | ||
| return None | ||
| candidate_scopes.discard(None) | ||
| is_ticket = bool(candidate_scopes & TICKET_COMMENT_SCOPES) | ||
| is_observation = bool(candidate_scopes & OBSERVATION_COMMENT_SCOPES) | ||
| if not is_ticket and not is_observation: | ||
| return None | ||
| access = "read" if self.action in self.scope_object_read_actions else "write" | ||
| required: list[str] = [f"ticket:{access}"] if is_ticket else [] | ||
| if candidate_scopes - TICKET_COMMENT_SCOPES: | ||
| required.append(f"comment:{access}") | ||
| if is_observation: | ||
| # Observation notes are recording-derived content, so they need the observation API's read scopes too. | ||
| required.extend(["replay_scanner:read", "session_recording:read"]) | ||
| return required | ||
|
|
||
| @extend_schema(parameters=[CommentListQueryParamsSerializer]) | ||
|
|
@@ -878,13 +932,7 @@ | |
| Ticket, | ||
| ) | ||
|
|
||
| target = self._pk_target() | ||
| if not target: | ||
| return | ||
| scope, item_id = target | ||
|
|
@@ -904,6 +952,28 @@ | |
| # Match the list path, where a denied ticket's comments are simply absent. | ||
| raise exceptions.NotFound() | ||
|
|
||
| def _pk_target(self) -> tuple[str, str | None] | None: | ||
| """The (scope, item_id) of the detail action's comment, read once for every scope gate.""" | ||
| if not hasattr(self, "_pk_target_cache"): | ||
| target = None | ||
| if pk := self.kwargs.get("pk"): | ||
| try: | ||
| target = Comment.objects.filter(team_id=self.team_id, pk=pk).values_list("scope", "item_id").first() | ||
| except (ValueError, django_exceptions.ValidationError): | ||
| target = None | ||
| self._pk_target_cache = target | ||
| return self._pk_target_cache | ||
|
|
||
| def _require_observation_viewer_access_for_pk(self) -> None: | ||
| """Gate a detail action on the observation its target comment belongs to, as the list path does.""" | ||
| target = self._pk_target() | ||
| if not target or target[0] not in OBSERVATION_COMMENT_SCOPES: | ||
| return | ||
| if not _can_view_observation( | ||
| team_id=self.team_id, item_id=target[1], user_access_control=self.user_access_control | ||
| ): | ||
|
Comment on lines
+972
to
+974
Contributor
There was a problem hiding this comment. P1 · Keep observation threads within one authorization boundary Changing a thread root's
Contributor
Author
There was a problem hiding this comment. Fixed in 0d2bd31. Instead of authorizing each reply in |
||
| raise exceptions.NotFound() | ||
|
|
||
| def _require_task_comment_viewer_access_for_pk(self) -> None: | ||
| pk = self.kwargs.get("pk") | ||
| if not pk: | ||
|
|
@@ -970,7 +1040,7 @@ | |
| ).values_list("id", flat=True) | ||
| return queryset.filter(item_id__in=[str(ticket_id) for ticket_id in visible_ticket_ids]) | ||
|
|
||
| def safely_get_queryset(self, queryset: QuerySet) -> QuerySet: | ||
|
Check warning on line 1043 in posthog/api/comments.py
|
||
| params = self.request.GET.dict() | ||
| queryset = queryset.exclude(scope__in=COMMENT_SCOPES_BLOCKED_FROM_GENERIC_API) | ||
|
|
||
|
|
@@ -991,6 +1061,11 @@ | |
| ) | ||
| if scope in TICKET_COMMENT_SCOPES: | ||
| queryset = self._filter_ticket_scoped_queryset(queryset, params.get("item_id")) | ||
| elif scope in OBSERVATION_COMMENT_SCOPES: | ||
| if not _can_view_observation( | ||
| team_id=self.team_id, item_id=params.get("item_id"), user_access_control=self.user_access_control | ||
| ): | ||
| return queryset.none() | ||
| elif scope in DESKTOP_COMMENT_SCOPES: | ||
| task_id = params.get("task_id") | ||
| item_id = params.get("item_id") | ||
|
|
@@ -1011,9 +1086,12 @@ | |
| elif self.action in ("list", "count"): | ||
| # Product-owned scopes require their own object-level access checks and must | ||
| # never leak through an unscoped generic comments query. | ||
| queryset = queryset.exclude( | ||
| scope__in=[*TICKET_COMMENT_SCOPES, *DESKTOP_COMMENT_SCOPES, *OBSERVATION_COMMENT_SCOPES] | ||
| ) | ||
| else: | ||
| self._require_ticket_viewer_access_for_pk() | ||
| self._require_observation_viewer_access_for_pk() | ||
| self._require_task_comment_viewer_access_for_pk() | ||
|
|
||
| if params.get("item_id"): | ||
|
|
@@ -1150,7 +1228,7 @@ | |
| ), | ||
| ) | ||
| @action(methods=["POST"], detail=True) | ||
| def send_to_slack(self, request: Request, *args: Any, **kwargs: Any) -> Response: | ||
|
Check warning on line 1231 in posthog/api/comments.py
|
||
| team = self.team | ||
| if not self._slack_mirror_flag_enabled(): | ||
| raise exceptions.NotFound() | ||
|
|
@@ -1160,6 +1238,9 @@ | |
| raise exceptions.ValidationError("Only a top-level comment (a thread root) can be sent to Slack") | ||
| if comment.scope == "conversations_ticket": | ||
| raise exceptions.ValidationError("Conversations tickets sync to Slack through the support product") | ||
| if comment.scope in OBSERVATION_COMMENT_SCOPES: | ||
| # A Slack channel has no observation access check, in either direction. | ||
| raise exceptions.ValidationError("Replay Vision observation notes cannot be sent to Slack") | ||
|
|
||
| params = SendCommentToSlackSerializer(data=request.data) | ||
| params.is_valid(raise_exception=True) | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1 · Recheck observation access when reading mention notifications
After a mentioned member loses scanner, recording, or experiment access, the notifications API still returns the stored note body. This filter checks access only before fan-out;
send_mention_notificationsstoresresource_type=comment, whichNotificationsViewSet._filter_by_access_controlskips. Gate notification reads on current observation access.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 18486b0. A mention notification for an observation note no longer stores the note text. Its body is a fixed placeholder that tells the reader to open the observation, and the comments API checks observation access each time the note is read. So a member who loses access later sees no note text in their notifications.