diff --git a/src/sentry/api/serializers/models/groupactionlogentry.py b/src/sentry/api/serializers/models/groupactionlogentry.py index 6d71baa48e34..6aefd1668964 100644 --- a/src/sentry/api/serializers/models/groupactionlogentry.py +++ b/src/sentry/api/serializers/models/groupactionlogentry.py @@ -333,11 +333,9 @@ def serialize( ): data.pop("current_release_version", None) - comment_id = _serialized_comment_id(obj) return { - # TODO(shashjar): Preserve the legacy id until clients have switched to commentId. - "id": comment_id if comment_id is not None else str(obj.id), - "commentId": comment_id, + "id": str(obj.id), + "commentId": _serialized_comment_id(obj), "type": type_display, "user": attrs["user"], "sentry_app": attrs["sentry_app"], diff --git a/src/sentry/issues/endpoints/group_notes_details.py b/src/sentry/issues/endpoints/group_notes_details.py index 20fe10b7bc2c..340d9d9b7861 100644 --- a/src/sentry/issues/endpoints/group_notes_details.py +++ b/src/sentry/issues/endpoints/group_notes_details.py @@ -223,20 +223,15 @@ def put(self, request: Request, group: Group, note_id: str) -> Response: sender="put", ) - if serve_from_log: - if original_comment_log_action is not None: - # editing a note doesn't update its COMMENT entry (instead it - # appends a separate COMMENT_EDIT entry), so patch in the fresh - # text we just published to GALE. The serializer resolves `id` - # back to the Activity id from the entry's comment_id, matching - # the flag-off contract so clients can edit/delete via note_id. - original_comment_log_action.data = { - **original_comment_log_action.data, - "text": payload.get("text"), - } - return Response( - serialize(original_comment_log_action, request.user), status=200 - ) + if serve_from_log and original_comment_log_action is not None: + # Edits append a COMMENT_EDIT rather than updating the COMMENT. + # Return the original entry's identity with the fresh text; + # commentId remains the reference for subsequent edits/deletes. + original_comment_log_action.data = { + **original_comment_log_action.data, + "text": payload.get("text"), + } + return Response(serialize(original_comment_log_action, request.user), status=200) return Response(serialize(note, request.user), status=200) diff --git a/tests/sentry/api/serializers/test_groupactionlogentry.py b/tests/sentry/api/serializers/test_groupactionlogentry.py index 98b678054db3..27d274f856e3 100644 --- a/tests/sentry/api/serializers/test_groupactionlogentry.py +++ b/tests/sentry/api/serializers/test_groupactionlogentry.py @@ -326,7 +326,7 @@ def test_merge_entry_reshapes_issues(self) -> None: result = serialize(entry, user) assert result["data"] == {"issues": [{"id": "2"}, {"id": "3"}]} - def test_comment_entry_serializes_the_activity_id(self) -> None: + def test_comment_entry_serializes_its_own_id_and_comment_reference(self) -> None: user = self.create_user() group = self.create_group(status=GroupStatus.UNRESOLVED) @@ -340,7 +340,7 @@ def test_comment_entry_serializes_the_activity_id(self) -> None: ) result = serialize(entry, user) - assert result["id"] == "123" + assert result["id"] == "456" assert result["commentId"] == "123" assert result["type"] == "note" @@ -386,7 +386,7 @@ def test_comment_edit_replaces_the_comment_text(self) -> None: items = self._activity_items() assert [item["type"] for item in items] == ["note", "first_seen"] - assert items[0]["id"] == "123" + assert items[0]["id"] == str(comment.id) assert items[0]["commentId"] == "123" assert items[0]["data"]["text"] == "edited" assert items[1]["id"] == "0" @@ -415,7 +415,7 @@ def test_edit_at_window_boundary_preserves_comment(self) -> None: assert [item["id"] for item in items] == [ str(newer_action.id), str(older_action.id), - "123", + str(comment.id), "0", ] assert items[:2] == before[:2] @@ -441,7 +441,7 @@ def test_initial_headroom_absorbs_boundary_edit_without_refetch(self) -> None: str(newest_action.id), str(newer_action.id), str(older_action.id), - "123", + str(comment.id), "0", ] assert items[3]["data"]["text"] == "edited" @@ -465,12 +465,13 @@ def test_refetch_limit_returns_short_page(self) -> None: assert [call.args[1] for call in fetch.call_args_list] == [3, 6, 12, 24] assert [item["type"] for item in items] == ["note", "first_seen"] - assert items[0]["id"] == "123" + assert items[0]["id"] == str(kept.id) assert items[0]["data"]["text"] == "edited" def test_full_page_without_mutations_needs_only_one_fetch(self) -> None: - self._comment(123, "comment") - self.create_group_action_log_entry(type=GroupActionType.RESOLVE) + action = self.create_group_action_log_entry(type=GroupActionType.RESOLVE) + # A comment reference can match another action's id without colliding in the feed. + comment = self._comment(action.id, "comment") with patch.object( GroupActionLogEntry.objects, @@ -480,7 +481,9 @@ def test_full_page_without_mutations_needs_only_one_fetch(self) -> None: items = self._activity_items(limit=2) fetch.assert_called_once_with(self.group, 2) - assert [item["type"] for item in items] == ["set_resolved", "note", "first_seen"] + assert [item["type"] for item in items] == ["note", "set_resolved", "first_seen"] + assert [item["id"] for item in items] == [str(comment.id), str(action.id), "0"] + assert items[0]["commentId"] == str(action.id) def test_deleted_comment_is_replaced_by_older_action(self) -> None: oldest_action = self.create_group_action_log_entry(type=GroupActionType.RESOLVE) @@ -501,25 +504,25 @@ def test_deleted_comment_is_replaced_by_older_action(self) -> None: assert items[-1]["type"] == "first_seen" def test_deleted_comment_is_replaced_by_older_comment(self) -> None: - self._comment(123, "oldest") - self._comment(456, "middle") + oldest = self._comment(123, "oldest") + middle = self._comment(456, "middle") deleted = self._comment(789, "newest") self._comment_mutation(GroupActionType.COMMENT_DELETE, deleted) items = self._activity_items(limit=2) - assert [item["id"] for item in items] == ["456", "123", "0"] + assert [item["id"] for item in items] == [str(middle.id), str(oldest.id), "0"] def test_comment_history_is_truncated_after_folding(self) -> None: oldest = self._comment(123, "oldest") - self._comment(456, "middle") + middle = self._comment(456, "middle") newest = self._comment(789, "newest") self._comment_mutation(GroupActionType.COMMENT_EDIT, newest, text="edited") self._comment_mutation(GroupActionType.COMMENT_EDIT, oldest, text="still outside the page") items = self._activity_items(limit=2) - assert [item["id"] for item in items] == ["789", "456", "0"] + assert [item["id"] for item in items] == [str(newest.id), str(middle.id), "0"] assert items[0]["data"]["text"] == "edited" assert items[-1]["type"] == "first_seen" @@ -558,7 +561,7 @@ def test_comment_delete_wins_over_an_earlier_edit(self) -> None: assert [item["type"] for item in items] == ["first_seen"] def test_only_the_named_comment_is_folded(self) -> None: - self._comment(123, "untouched") + untouched = self._comment(123, "untouched") edited = self._comment(456, "original") self._comment_mutation(GroupActionType.COMMENT_EDIT, edited, text="edited") deleted = self._comment(789, "doomed") @@ -578,9 +581,9 @@ def test_only_the_named_comment_is_folded(self) -> None: "note", "first_seen", ] - assert items[1]["id"] == "456" + assert items[1]["id"] == str(edited.id) assert items[1]["data"]["text"] == "edited" - assert items[2]["id"] == "123" + assert items[2]["id"] == str(untouched.id) assert items[2]["data"]["text"] == "untouched" def test_comment_mutations_are_dropped_without_their_comment(self) -> None: diff --git a/tests/sentry/issues/endpoints/test_group_details.py b/tests/sentry/issues/endpoints/test_group_details.py index 5b2312a48896..4bcce6e93bff 100644 --- a/tests/sentry/issues/endpoints/test_group_details.py +++ b/tests/sentry/issues/endpoints/test_group_details.py @@ -177,7 +177,13 @@ def test_group_action_log_comment_is_addressable(self) -> None: comments_url = f"/api/0/issues/{group.id}/comments/" response = self.client.post(comments_url, format="json", data={"text": "original"}) assert response.status_code == 201, response.content - activity_id = response.data["data"]["comment_id"] + comment_id = response.data["commentId"] + entry = GroupActionLogEntry.objects.get( + group_id=group.id, type=GroupActionType.COMMENT.value + ) + entry_id = str(entry.id) + assert response.data["id"] == entry_id + assert int(comment_id) == entry.data["comment_id"] # Put the comment at the oldest edge of the 99-entry action-log window. for _ in range(98): @@ -190,37 +196,31 @@ def test_group_action_log_comment_is_addressable(self) -> None: notes = [item for item in response.data["activity"] if item["type"] == "note"] assert len(notes) == 1 - note_id = notes[0]["commentId"] - assert note_id == str(activity_id) - assert notes[0]["id"] == note_id - - entry = GroupActionLogEntry.objects.get( - group_id=group.id, type=GroupActionType.COMMENT.value - ) - assert entry.data["comment_id"] == activity_id + assert notes[0]["commentId"] == comment_id + assert notes[0]["id"] == entry_id # the comment reference served by the feed round-trips through edit ... response = self.client.put( - f"{comments_url}{note_id}/", format="json", data={"text": "edited"} + f"{comments_url}{comment_id}/", format="json", data={"text": "edited"} ) assert response.status_code == 200, response.content - assert response.data["id"] == note_id - assert response.data["commentId"] == note_id + assert response.data["id"] == entry_id + assert response.data["commentId"] == comment_id assert response.data["data"]["text"] == "edited" # ... the feed folds the appended COMMENT_EDIT back into the comment ... response = self.client.get(details_url, format="json") assert response.status_code == 200, response.content assert len(response.data["activity"]) == 100 - assert response.data["activity"][-2]["id"] == note_id + assert response.data["activity"][-2]["id"] == entry_id notes = [item for item in response.data["activity"] if item["type"] == "note"] assert len(notes) == 1 - assert notes[0]["id"] == note_id - assert notes[0]["commentId"] == note_id + assert notes[0]["id"] == entry_id + assert notes[0]["commentId"] == comment_id assert notes[0]["data"]["text"] == "edited" # ... and delete - response = self.client.delete(f"{comments_url}{note_id}/", format="json") + response = self.client.delete(f"{comments_url}{comment_id}/", format="json") assert response.status_code == 204, response.status_code # ... after which the COMMENT_DELETE drops the comment from the feed diff --git a/tests/sentry/issues/endpoints/test_group_notes.py b/tests/sentry/issues/endpoints/test_group_notes.py index 403796626c09..f8f4283f8bfe 100644 --- a/tests/sentry/issues/endpoints/test_group_notes.py +++ b/tests/sentry/issues/endpoints/test_group_notes.py @@ -110,7 +110,7 @@ def test_note_merge(self) -> None: def test_reads_from_gale(self) -> None: group = self.group - self.create_group_action_log_entry( + entry = self.create_group_action_log_entry( group=group, type=GroupActionType.COMMENT, actor_type=GroupActorType.USER, @@ -124,8 +124,7 @@ def test_reads_from_gale(self) -> None: response = self.client.get(url, format="json") assert response.status_code == 200, response.content assert len(response.data) == 1 - # `id` is the Activity id (comment_id), matching the flag-off contract - assert response.data[0]["id"] == "123" + assert response.data[0]["id"] == str(entry.id) assert response.data[0]["commentId"] == "123" assert response.data[0]["type"] == "note" assert response.data[0]["user"]["id"] == str(self.user.id) @@ -174,17 +173,18 @@ def test_reads_from_gale_with_edits(self) -> None: # edits collapse into the original comment, so there is one row per comment assert len(response.data) == 2 - # `id` is the Activity id (comment_id), matching the flag-off contract - unedited_id = str(unedited.data["comment_id"]) - edited_id = str(edited.data["comment_id"]) + unedited_id = str(unedited.id) + edited_id = str(edited.id) rows_by_id = {row["id"]: row for row in response.data} assert set(rows_by_id) == {unedited_id, edited_id} # the edited comment keeps type "note" and shows the latest edit text assert rows_by_id[edited_id]["type"] == "note" + assert rows_by_id[edited_id]["commentId"] == "2" assert rows_by_id[edited_id]["data"]["text"] == "latest edit" # the unedited comment is unaffected assert rows_by_id[unedited_id]["data"]["text"] == "unedited comment" + assert rows_by_id[unedited_id]["commentId"] == "1" class GroupNoteCreateTest(APITestCase): @@ -229,10 +229,11 @@ def test_returns_gale(self) -> None: activity = Activity.objects.get( group=group, type=ActivityType.NOTE.value, user_id=self.user.id ) - GroupActionLogEntry.objects.get(group_id=group.id, type=GroupActionType.COMMENT.value) + entry = GroupActionLogEntry.objects.get( + group_id=group.id, type=GroupActionType.COMMENT.value + ) - # `id` is the Activity id (comment_id), matching the flag-off contract - assert response.data["id"] == str(activity.id) + assert response.data["id"] == str(entry.id) assert response.data["commentId"] == str(activity.id) assert response.data["type"] == "note" assert response.data["source"] == "mcp:claude-code" diff --git a/tests/sentry/issues/endpoints/test_group_notes_details.py b/tests/sentry/issues/endpoints/test_group_notes_details.py index 62719de4958e..366c2e31ffbb 100644 --- a/tests/sentry/issues/endpoints/test_group_notes_details.py +++ b/tests/sentry/issues/endpoints/test_group_notes_details.py @@ -226,7 +226,7 @@ def test_put_ignore_mentions(self) -> None: } @action_log_activity_enabled() - def test_put_returns_gale(self) -> None: + def test_put_returns_original_comment_and_appends_edit(self) -> None: self.login_as(user=self.user) group = self.group @@ -234,45 +234,22 @@ def test_put_returns_gale(self) -> None: post_url = f"/api/0/issues/{group.id}/comments/" response = self.client.post(post_url, format="json", data={"text": "original"}) assert response.status_code == 201, response.content - activity_id = response.data["data"]["comment_id"] + comment_id = response.data["commentId"] + original_entry = GroupActionLogEntry.objects.get( + group_id=group.id, id=response.data["id"], type=GroupActionType.COMMENT.value + ) - put_url = f"/api/0/issues/{group.id}/comments/{activity_id}/" + put_url = f"{post_url}{comment_id}/" response = self.client.put(put_url, format="json", data={"text": "updated text"}) assert response.status_code == 200, response.content - GroupActionLogEntry.objects.get( - group_id=group.id, - type=GroupActionType.COMMENT.value, - data__comment_id=activity_id, - ) - # `id` is the Activity id (comment_id), matching the flag-off contract - assert response.data["id"] == str(activity_id) - assert response.data["commentId"] == str(activity_id) + assert response.data["id"] == str(original_entry.id) + assert response.data["commentId"] == comment_id assert response.data["type"] == "note" assert response.data["user"]["id"] == str(self.user.id) # the fresh text is re-derived from the edited activity, not the stale GALE entry assert response.data["data"]["text"] == "updated text" - assert response.data["data"]["comment_id"] == activity_id - - @action_log_activity_enabled() - def test_put_writes_comment_edit_entry(self) -> None: - self.login_as(user=self.user) - group = self.group - - post_url = f"/api/0/issues/{group.id}/comments/" - response = self.client.post(post_url, format="json", data={"text": "original"}) - assert response.status_code == 201, response.content - activity_id = response.data["data"]["comment_id"] - - original_entry = GroupActionLogEntry.objects.get( - group_id=group.id, - type=GroupActionType.COMMENT.value, - data__comment_id=activity_id, - ) - - put_url = f"/api/0/issues/{group.id}/comments/{activity_id}/" - response = self.client.put(put_url, format="json", data={"text": "updated text"}) - assert response.status_code == 200, response.content + assert response.data["data"]["comment_id"] == int(comment_id) edit_entry = GroupActionLogEntry.objects.get( group_id=group.id, type=GroupActionType.COMMENT_EDIT.value @@ -293,15 +270,13 @@ def test_delete_writes_comment_delete_entry(self) -> None: post_url = f"/api/0/issues/{group.id}/comments/" response = self.client.post(post_url, format="json", data={"text": "original"}) assert response.status_code == 201, response.content - activity_id = response.data["data"]["comment_id"] + comment_id = response.data["commentId"] original_entry = GroupActionLogEntry.objects.get( - group_id=group.id, - type=GroupActionType.COMMENT.value, - data__comment_id=activity_id, + group_id=group.id, id=response.data["id"], type=GroupActionType.COMMENT.value ) - delete_url = f"/api/0/issues/{group.id}/comments/{activity_id}/" + delete_url = f"{post_url}{comment_id}/" response = self.client.delete(delete_url, format="json") assert response.status_code == 204, response.status_code