From 755f7b0e8b0b5df4d89ea8062790682b3e71e9a6 Mon Sep 17 00:00:00 2001 From: Asuka Minato Date: Mon, 27 Jul 2026 17:22:45 +0900 Subject: [PATCH] test: use sqlite3 session in test_workflow_comment_service (#38700) --- .../services/test_workflow_comment_service.py | 1036 +++++++++-------- 1 file changed, 576 insertions(+), 460 deletions(-) diff --git a/api/tests/unit_tests/services/test_workflow_comment_service.py b/api/tests/unit_tests/services/test_workflow_comment_service.py index e6db068e07c..0478351b073 100644 --- a/api/tests/unit_tests/services/test_workflow_comment_service.py +++ b/api/tests/unit_tests/services/test_workflow_comment_service.py @@ -1,35 +1,129 @@ -from unittest.mock import MagicMock, Mock, patch +"""Persistence-focused tests for :mod:`services.workflow_comment_service`. + +The service opens its own sessions for most operations, so these tests bind it to the +same disposable SQLite engine used for fixture setup and assert committed database +state. External task dispatch and the clock remain mocked at their I/O boundaries. +""" + +from datetime import datetime +from types import SimpleNamespace +from unittest.mock import Mock, patch import pytest +from sqlalchemy import event, func, select +from sqlalchemy.orm import Session from werkzeug.exceptions import Forbidden, NotFound +from models import App, TenantAccountJoin, WorkflowComment, WorkflowCommentMention, WorkflowCommentReply +from models.account import Account, TenantAccountRole +from models.model import AppMode from services import workflow_comment_service as service_module from services.workflow_comment_service import WorkflowCommentService +TENANT_ID = "11111111-1111-1111-1111-111111111111" +OTHER_TENANT_ID = "11111111-1111-1111-1111-111111111112" +APP_ID = "22222222-2222-2222-2222-222222222222" +OTHER_APP_ID = "22222222-2222-2222-2222-222222222223" +OWNER_ID = "33333333-3333-3333-3333-333333333333" +USER_2_ID = "33333333-3333-3333-3333-333333333334" +USER_3_ID = "33333333-3333-3333-3333-333333333335" +USER_4_ID = "33333333-3333-3333-3333-333333333336" +OUTSIDER_ID = "33333333-3333-3333-3333-333333333337" + @pytest.fixture -def mock_session(monkeypatch: pytest.MonkeyPatch) -> Mock: - session = Mock() - context_manager = MagicMock() - context_manager.__enter__.return_value = session - context_manager.__exit__.return_value = False - mock_db = MagicMock() - mock_db.engine = Mock() - empty_scalars = Mock() - empty_scalars.all.return_value = [] - session.scalars.return_value = empty_scalars - monkeypatch.setattr(service_module, "Session", Mock(return_value=context_manager)) - monkeypatch.setattr(service_module, "db", mock_db) - monkeypatch.setattr(service_module.send_workflow_comment_mention_email_task, "delay", Mock()) - return session +def delay_mock(monkeypatch: pytest.MonkeyPatch) -> Mock: + mock = Mock() + monkeypatch.setattr(service_module.send_workflow_comment_mention_email_task, "delay", mock) + return mock -def _mock_scalars(result_list: list[object]) -> Mock: - scalars = Mock() - scalars.all.return_value = result_list - return scalars +@pytest.fixture(autouse=True) +def bind_service_database( + sqlite_session: Session, + monkeypatch: pytest.MonkeyPatch, + delay_mock: Mock, +) -> None: + """Bind service-owned sessions to the engine prepared by the shared SQLite fixture.""" + monkeypatch.setattr(service_module, "db", SimpleNamespace(engine=sqlite_session.get_bind())) +def _account( + account_id: str, + *, + name: str = "Test User", + email: str = "user@example.com", + interface_language: str | None = "en-US", +) -> Account: + account = Account(name=name, email=email, interface_language=interface_language) + account.id = account_id + return account + + +def _app(*, app_id: str = APP_ID, tenant_id: str = TENANT_ID, name: str = "My App") -> App: + app = App( + tenant_id=tenant_id, + name=name, + mode=AppMode.WORKFLOW, + enable_site=False, + enable_api=False, + created_by=OWNER_ID, + ) + app.id = app_id + return app + + +def _comment( + *, + tenant_id: str = TENANT_ID, + app_id: str = APP_ID, + created_by: str = OWNER_ID, + content: str = "hello", + resolved: bool = False, + resolved_at: datetime | None = None, + resolved_by: str | None = None, +) -> WorkflowComment: + return WorkflowComment( + tenant_id=tenant_id, + app_id=app_id, + position_x=1.0, + position_y=2.0, + content=content, + created_by=created_by, + resolved=resolved, + resolved_at=resolved_at, + resolved_by=resolved_by, + ) + + +def _membership(account_id: str, *, tenant_id: str = TENANT_ID) -> TenantAccountJoin: + return TenantAccountJoin( + tenant_id=tenant_id, + account_id=account_id, + role=TenantAccountRole.NORMAL, + ) + + +def _persist(session: Session, *objects: object) -> None: + session.add_all(objects) + session.commit() + + +@pytest.mark.usefixtures("sqlite_session") +@pytest.mark.parametrize( + "sqlite_session", + [ + ( + Account, + TenantAccountJoin, + App, + WorkflowComment, + WorkflowCommentReply, + WorkflowCommentMention, + ) + ], + indirect=True, +) class TestWorkflowCommentService: def test_validate_content_rejects_empty(self) -> None: with pytest.raises(ValueError): @@ -39,42 +133,43 @@ class TestWorkflowCommentService: with pytest.raises(ValueError): WorkflowCommentService._validate_content("a" * 1001) - def test_filter_valid_mentioned_user_ids_filters_by_tenant_and_preserves_order(self, mock_session: Mock) -> None: - tenant_member_1 = "123e4567-e89b-12d3-a456-426614174000" - tenant_member_2 = "123e4567-e89b-12d3-a456-426614174002" - non_tenant_member = "123e4567-e89b-12d3-a456-426614174001" - mock_session.scalars.return_value = _mock_scalars([tenant_member_1, tenant_member_2]) + def test_filter_valid_mentioned_user_ids_filters_by_tenant_and_preserves_order( + self, sqlite_session: Session + ) -> None: + _persist( + sqlite_session, + _membership(OWNER_ID), + _membership(USER_2_ID), + _membership(USER_3_ID, tenant_id=OTHER_TENANT_ID), + ) result = WorkflowCommentService._filter_valid_mentioned_user_ids( [ - tenant_member_1, + OWNER_ID, "", 123, # type: ignore[list-item] - tenant_member_1, - non_tenant_member, - tenant_member_2, + OWNER_ID, + USER_3_ID, + USER_2_ID, ], - session=mock_session, - tenant_id="tenant-1", + session=sqlite_session, + tenant_id=TENANT_ID, ) - assert result == [ - tenant_member_1, - tenant_member_2, - ] + assert result == [OWNER_ID, USER_2_ID] def test_format_comment_excerpt_handles_short_and_long_limits(self) -> None: assert WorkflowCommentService._format_comment_excerpt(" hello ", max_length=10) == "hello" assert WorkflowCommentService._format_comment_excerpt("abcdefghijk", max_length=3) == "abc" assert WorkflowCommentService._format_comment_excerpt(" abcdefghijk ", max_length=8) == "abcde..." - def test_build_mention_email_payloads_returns_empty_for_no_candidates(self, mock_session: Mock) -> None: + def test_build_mention_email_payloads_returns_empty_for_no_candidates(self, sqlite_session: Session) -> None: assert ( WorkflowCommentService._build_mention_email_payloads( - session=mock_session, - tenant_id="tenant-1", - app_id="app-1", - mentioner_id="user-1", + session=sqlite_session, + tenant_id=TENANT_ID, + app_id=APP_ID, + mentioner_id=OWNER_ID, mentioned_user_ids=[], content="hello", ) @@ -82,11 +177,11 @@ class TestWorkflowCommentService: ) assert ( WorkflowCommentService._build_mention_email_payloads( - session=mock_session, - tenant_id="tenant-1", - app_id="app-1", - mentioner_id="user-1", - mentioned_user_ids=["user-1"], + session=sqlite_session, + tenant_id=TENANT_ID, + app_id=APP_ID, + mentioner_id=OWNER_ID, + mentioned_user_ids=[OWNER_ID], content="hello", ) == [] @@ -104,29 +199,26 @@ class TestWorkflowCommentService: assert delay_mock.call_count == 2 - def test_build_mention_email_payloads_skips_accounts_without_email(self, mock_session: Mock) -> None: - account_without_email = Mock() - account_without_email.email = None - account_without_email.name = "No Email" - account_without_email.interface_language = "en-US" - - account_with_email = Mock() - account_with_email.email = "user@example.com" - account_with_email.name = "" - account_with_email.interface_language = None - - mock_session.scalar.side_effect = ["My App", "Commenter"] - mock_session.scalars.return_value = _mock_scalars([account_without_email, account_with_email]) + def test_build_mention_email_payloads_skips_accounts_without_email(self, sqlite_session: Session) -> None: + _persist( + sqlite_session, + _app(), + _account(OWNER_ID, name="Commenter", email="commenter@example.com"), + _account(USER_2_ID, name="No Email", email=""), + _account(USER_3_ID, name="", email="user@example.com", interface_language=None), + _membership(USER_2_ID), + _membership(USER_3_ID), + ) payloads = WorkflowCommentService._build_mention_email_payloads( - session=mock_session, - tenant_id="tenant-1", - app_id="app-1", - mentioner_id="user-1", - mentioned_user_ids=["user-2"], + session=sqlite_session, + tenant_id=TENANT_ID, + app_id=APP_ID, + mentioner_id=OWNER_ID, + mentioned_user_ids=[USER_2_ID, USER_3_ID], content="hello", ) - expected_app_url = f"{service_module.dify_config.CONSOLE_WEB_URL.rstrip('/')}/app/app-1/workflow" + expected_app_url = f"{service_module.dify_config.CONSOLE_WEB_URL.rstrip('/')}/app/{APP_ID}/workflow" assert payloads == [ { @@ -140,439 +232,463 @@ class TestWorkflowCommentService: } ] - def test_create_comment_creates_mentions(self, mock_session: Mock) -> None: - comment = Mock() - comment.id = "comment-1" - comment.created_at = "ts" + def test_create_comment_creates_mentions(self, sqlite_session: Session) -> None: + _persist(sqlite_session, _membership(USER_2_ID)) - with ( - patch.object(service_module, "WorkflowComment", return_value=comment), - patch.object(service_module, "WorkflowCommentMention", return_value=Mock()), - patch.object(WorkflowCommentService, "_filter_valid_mentioned_user_ids", return_value=["user-2"]), - ): - result = WorkflowCommentService.create_comment( - tenant_id="tenant-1", - app_id="app-1", - created_by="user-1", - content="hello", - position_x=1.0, - position_y=2.0, - mentioned_user_ids=["user-2", "bad-id"], - ) + result = WorkflowCommentService.create_comment( + tenant_id=TENANT_ID, + app_id=APP_ID, + created_by=OWNER_ID, + content="hello", + position_x=1.0, + position_y=2.0, + mentioned_user_ids=[USER_2_ID, OUTSIDER_ID], + ) - assert result == {"id": "comment-1", "created_at": "ts"} - assert mock_session.add.call_args_list[0].args[0] is comment - assert mock_session.add.call_count == 2 - mock_session.commit.assert_called_once() - - def test_update_comment_raises_not_found(self, mock_session: Mock) -> None: - mock_session.scalar.return_value = None + comment = sqlite_session.get(WorkflowComment, result["id"]) + assert comment is not None + assert comment.content == "hello" + assert comment.created_at == result["created_at"] + mentions = sqlite_session.scalars( + select(WorkflowCommentMention).where(WorkflowCommentMention.comment_id == comment.id) + ).all() + assert [mention.mentioned_user_id for mention in mentions] == [USER_2_ID] + def test_update_comment_raises_not_found(self, sqlite_session: Session) -> None: with pytest.raises(NotFound): WorkflowCommentService.update_comment( - tenant_id="tenant-1", - app_id="app-1", - comment_id="comment-1", - user_id="user-1", + tenant_id=TENANT_ID, + app_id=APP_ID, + comment_id="missing-comment", + user_id=OWNER_ID, content="hello", ) - def test_update_comment_raises_forbidden(self, mock_session: Mock) -> None: - comment = Mock() - comment.created_by = "owner" - mock_session.scalar.return_value = comment + def test_update_comment_raises_forbidden(self, sqlite_session: Session) -> None: + comment = _comment() + _persist(sqlite_session, comment) with pytest.raises(Forbidden): WorkflowCommentService.update_comment( - tenant_id="tenant-1", - app_id="app-1", - comment_id="comment-1", - user_id="intruder", + tenant_id=TENANT_ID, + app_id=APP_ID, + comment_id=comment.id, + user_id=OUTSIDER_ID, content="hello", ) - def test_update_comment_replaces_mentions(self, mock_session: Mock) -> None: - comment = Mock() - comment.id = "comment-1" - comment.created_by = "owner" - mock_session.scalar.return_value = comment + def test_update_comment_replaces_mentions(self, sqlite_session: Session) -> None: + comment = _comment() + _persist(sqlite_session, comment) + _persist( + sqlite_session, + WorkflowCommentMention(comment_id=comment.id, mentioned_user_id=USER_3_ID), + WorkflowCommentMention(comment_id=comment.id, mentioned_user_id=USER_4_ID), + _membership(USER_2_ID), + ) - existing_mentions = [Mock(), Mock()] - mock_session.scalars.return_value = _mock_scalars(existing_mentions) + result = WorkflowCommentService.update_comment( + tenant_id=TENANT_ID, + app_id=APP_ID, + comment_id=comment.id, + user_id=OWNER_ID, + content="updated", + mentioned_user_ids=[USER_2_ID, OUTSIDER_ID], + ) - with patch.object(WorkflowCommentService, "_filter_valid_mentioned_user_ids", return_value=["user-2"]): - result = WorkflowCommentService.update_comment( - tenant_id="tenant-1", - app_id="app-1", - comment_id="comment-1", - user_id="owner", - content="updated", - mentioned_user_ids=["user-2", "bad-id"], - ) + sqlite_session.expire_all() + persisted_comment = sqlite_session.get(WorkflowComment, comment.id) + assert persisted_comment is not None + assert persisted_comment.content == "updated" + assert result == {"id": comment.id, "updated_at": persisted_comment.updated_at} + mentions = sqlite_session.scalars( + select(WorkflowCommentMention).where(WorkflowCommentMention.comment_id == comment.id) + ).all() + assert [mention.mentioned_user_id for mention in mentions] == [USER_2_ID] - assert result == {"id": "comment-1", "updated_at": comment.updated_at} - assert mock_session.delete.call_count == 2 - assert mock_session.add.call_count == 1 - mock_session.commit.assert_called_once() - - def test_update_comment_preserves_mentions_when_mentioned_user_ids_omitted(self, mock_session: Mock) -> None: - comment = Mock() - comment.id = "comment-1" - comment.created_by = "owner" - mock_session.scalar.return_value = comment - - with ( - patch.object(WorkflowCommentService, "_filter_valid_mentioned_user_ids") as filter_mentions_mock, - patch.object(WorkflowCommentService, "_build_mention_email_payloads") as build_payloads_mock, - patch.object(WorkflowCommentService, "_dispatch_mention_emails") as dispatch_mock, - ): - result = WorkflowCommentService.update_comment( - tenant_id="tenant-1", - app_id="app-1", - comment_id="comment-1", - user_id="owner", - content="updated", - ) - - assert result == {"id": "comment-1", "updated_at": comment.updated_at} - mock_session.delete.assert_not_called() - mock_session.add.assert_not_called() - filter_mentions_mock.assert_not_called() - build_payloads_mock.assert_not_called() - dispatch_mock.assert_called_once_with([]) - mock_session.commit.assert_called_once() - - def test_update_comment_clears_mentions_when_empty_list_provided(self, mock_session: Mock) -> None: - comment = Mock() - comment.id = "comment-1" - comment.created_by = "owner" - mock_session.scalar.return_value = comment - - existing_mentions = [Mock(), Mock()] - mock_session.scalars.return_value = _mock_scalars(existing_mentions) - - with patch.object(WorkflowCommentService, "_filter_valid_mentioned_user_ids", return_value=[]): - result = WorkflowCommentService.update_comment( - tenant_id="tenant-1", - app_id="app-1", - comment_id="comment-1", - user_id="owner", - content="updated", - mentioned_user_ids=[], - ) - - assert result == {"id": "comment-1", "updated_at": comment.updated_at} - assert mock_session.delete.call_count == 2 - mock_session.add.assert_not_called() - mock_session.commit.assert_called_once() - - def test_update_comment_notifies_only_new_mentions(self, mock_session: Mock) -> None: - comment = Mock() - comment.id = "comment-1" - comment.created_by = "owner" - mock_session.scalar.return_value = comment - - existing_mention = Mock() - existing_mention.mentioned_user_id = "user-2" - mock_session.scalars.return_value = _mock_scalars([existing_mention]) - - with ( - patch.object( - WorkflowCommentService, - "_filter_valid_mentioned_user_ids", - return_value=["user-2", "user-3"], - ), - patch.object( - WorkflowCommentService, - "_build_mention_email_payloads", - return_value=[], - ) as build_payloads_mock, - patch.object(WorkflowCommentService, "_dispatch_mention_emails") as dispatch_mock, - ): - WorkflowCommentService.update_comment( - tenant_id="tenant-1", - app_id="app-1", - comment_id="comment-1", - user_id="owner", - content="updated", - mentioned_user_ids=["user-2", "user-3"], - ) - - assert build_payloads_mock.call_args.kwargs["mentioned_user_ids"] == ["user-3"] - dispatch_mock.assert_called_once_with([]) - - def test_get_comments_preloads_related_accounts(self, mock_session: Mock) -> None: - comment = Mock() - comment.created_by = "user-1" - comment.resolved_by = "user-2" - reply = Mock() - reply.created_by = "user-3" - mention = Mock() - mention.mentioned_user_id = "user-4" - comment.replies = [reply] - comment.mentions = [mention] - comment.cache_created_by_account = Mock() - comment.cache_resolved_by_account = Mock() - reply.cache_created_by_account = Mock() - mention.cache_mentioned_user_account = Mock() - - account_1 = Mock() - account_1.id = "user-1" - account_2 = Mock() - account_2.id = "user-2" - account_3 = Mock() - account_3.id = "user-3" - account_4 = Mock() - account_4.id = "user-4" - - mock_session.scalars.side_effect = [ - _mock_scalars([comment]), - _mock_scalars([account_1, account_2, account_3, account_4]), - ] - - result = WorkflowCommentService.get_comments("tenant-1", "app-1") - - assert result == [comment] - comment.cache_created_by_account.assert_called_once_with(account_1) - comment.cache_resolved_by_account.assert_called_once_with(account_2) - reply.cache_created_by_account.assert_called_once_with(account_3) - mention.cache_mentioned_user_account.assert_called_once_with(account_4) - - def test_preload_accounts_returns_early_for_empty_comments(self, mock_session: Mock) -> None: - WorkflowCommentService._preload_accounts(mock_session, []) - - mock_session.scalars.assert_not_called() - - def test_get_comment_raises_not_found_with_provided_session(self) -> None: - session = Mock() - session.scalar.return_value = None - - with pytest.raises(NotFound): - WorkflowCommentService.get_comment("tenant-1", "app-1", "comment-1", session=session) - - def test_get_comment_uses_context_manager_when_session_not_provided(self, mock_session: Mock) -> None: - comment = Mock() - comment.created_by = "user-1" - comment.resolved_by = None - comment.replies = [] - comment.mentions = [] - comment.cache_created_by_account = Mock() - comment.cache_resolved_by_account = Mock() - mock_session.scalar.return_value = comment - mock_session.scalars.return_value = _mock_scalars([]) - - result = WorkflowCommentService.get_comment("tenant-1", "app-1", "comment-1") - - assert result is comment - comment.cache_created_by_account.assert_called_once() - comment.cache_resolved_by_account.assert_called_once_with(None) - - def test_delete_comment_raises_forbidden(self, mock_session: Mock) -> None: - comment = Mock() - comment.created_by = "owner" - - with patch.object(WorkflowCommentService, "get_comment", return_value=comment): - with pytest.raises(Forbidden): - WorkflowCommentService.delete_comment("tenant-1", "app-1", "comment-1", "intruder") - - def test_delete_comment_removes_related_entities(self, mock_session: Mock) -> None: - comment = Mock() - comment.created_by = "owner" - - mentions = [Mock(), Mock()] - replies = [Mock()] - mock_session.scalars.side_effect = [_mock_scalars(mentions), _mock_scalars(replies)] - - with patch.object(WorkflowCommentService, "get_comment", return_value=comment): - WorkflowCommentService.delete_comment("tenant-1", "app-1", "comment-1", "owner") - - assert mock_session.delete.call_count == 4 - mock_session.commit.assert_called_once() - - def test_resolve_comment_sets_fields(self, mock_session: Mock) -> None: - comment = Mock() - comment.resolved = False - comment.resolved_at = None - comment.resolved_by = None - - with ( - patch.object(WorkflowCommentService, "get_comment", return_value=comment), - patch.object(service_module, "naive_utc_now", return_value="now"), - ): - result = WorkflowCommentService.resolve_comment("tenant-1", "app-1", "comment-1", "user-1") - - assert result is comment - assert comment.resolved is True - assert comment.resolved_at == "now" - assert comment.resolved_by == "user-1" - mock_session.commit.assert_called_once() - - def test_resolve_comment_noop_when_already_resolved(self, mock_session: Mock) -> None: - comment = Mock() - comment.resolved = True - - with patch.object(WorkflowCommentService, "get_comment", return_value=comment): - result = WorkflowCommentService.resolve_comment("tenant-1", "app-1", "comment-1", "user-1") - - assert result is comment - mock_session.commit.assert_not_called() - - def test_create_reply_requires_comment(self, mock_session: Mock) -> None: - mock_session.get.return_value = None - - with pytest.raises(NotFound): - WorkflowCommentService.create_reply("comment-1", "hello", "user-1") - - def test_create_reply_creates_mentions(self, mock_session: Mock) -> None: - mock_session.get.return_value = Mock() - reply = Mock() - reply.id = "reply-1" - reply.created_at = "ts" - - with ( - patch.object(service_module, "WorkflowCommentReply", return_value=reply), - patch.object(service_module, "WorkflowCommentMention", return_value=Mock()), - patch.object(WorkflowCommentService, "_filter_valid_mentioned_user_ids", return_value=["user-2"]), - ): - result = WorkflowCommentService.create_reply( - comment_id="comment-1", - content="hello", - created_by="user-1", - mentioned_user_ids=["user-2", "bad-id"], - ) - - assert result == {"id": "reply-1", "created_at": "ts"} - assert mock_session.add.call_count == 2 - mock_session.commit.assert_called_once() - - def test_update_reply_raises_not_found(self, mock_session: Mock) -> None: - mock_session.scalar.return_value = None - - with pytest.raises(NotFound): - WorkflowCommentService.update_reply( - tenant_id="tenant-1", - app_id="app-1", - comment_id="comment-1", - reply_id="reply-1", - user_id="user-1", - content="hello", - ) - - def test_update_reply_raises_forbidden(self, mock_session: Mock) -> None: - reply = Mock() - reply.created_by = "owner" - mock_session.scalar.return_value = reply - - with pytest.raises(Forbidden): - WorkflowCommentService.update_reply( - tenant_id="tenant-1", - app_id="app-1", - comment_id="comment-1", - reply_id="reply-1", - user_id="intruder", - content="hello", - ) - - def test_update_reply_replaces_mentions(self, mock_session: Mock) -> None: - reply = Mock() - reply.id = "reply-1" - reply.comment_id = "comment-1" - reply.created_by = "owner" - reply.updated_at = "updated" - mock_session.scalar.return_value = reply - mock_session.scalars.return_value = _mock_scalars([Mock()]) - comment = Mock() - comment.tenant_id = "tenant-1" - comment.app_id = "app-1" - mock_session.get.return_value = comment - - with patch.object(WorkflowCommentService, "_filter_valid_mentioned_user_ids", return_value=["user-2"]): - result = WorkflowCommentService.update_reply( - tenant_id="tenant-1", - app_id="app-1", - comment_id="comment-1", - reply_id="reply-1", - user_id="owner", - content="new", - mentioned_user_ids=["user-2", "bad-id"], - ) - - assert result == {"id": "reply-1", "updated_at": "updated"} - assert mock_session.delete.call_count == 1 - assert mock_session.add.call_count == 1 - mock_session.commit.assert_called_once() - mock_session.refresh.assert_called_once_with(reply) - - def test_update_comment_updates_position_coordinates_when_provided(self, mock_session: Mock) -> None: - comment = Mock() - comment.id = "comment-1" - comment.created_by = "owner" - comment.position_x = 1.0 - comment.position_y = 2.0 - mock_session.scalar.return_value = comment - mock_session.scalars.return_value = _mock_scalars([]) + def test_update_comment_preserves_mentions_when_mentioned_user_ids_omitted( + self, sqlite_session: Session, delay_mock: Mock + ) -> None: + comment = _comment() + _persist(sqlite_session, comment) + mention = WorkflowCommentMention(comment_id=comment.id, mentioned_user_id=USER_2_ID) + _persist(sqlite_session, mention) WorkflowCommentService.update_comment( - tenant_id="tenant-1", - app_id="app-1", - comment_id="comment-1", - user_id="owner", + tenant_id=TENANT_ID, + app_id=APP_ID, + comment_id=comment.id, + user_id=OWNER_ID, + content="updated", + ) + + sqlite_session.expire_all() + persisted_comment = sqlite_session.get(WorkflowComment, comment.id) + assert persisted_comment is not None + assert persisted_comment.content == "updated" + assert sqlite_session.get(WorkflowCommentMention, mention.id) is not None + delay_mock.assert_not_called() + + def test_update_comment_clears_mentions_when_empty_list_provided(self, sqlite_session: Session) -> None: + comment = _comment() + _persist(sqlite_session, comment) + _persist( + sqlite_session, + WorkflowCommentMention(comment_id=comment.id, mentioned_user_id=USER_2_ID), + WorkflowCommentMention(comment_id=comment.id, mentioned_user_id=USER_3_ID), + ) + + WorkflowCommentService.update_comment( + tenant_id=TENANT_ID, + app_id=APP_ID, + comment_id=comment.id, + user_id=OWNER_ID, + content="updated", + mentioned_user_ids=[], + ) + + mention_count = sqlite_session.scalar( + select(func.count()) + .select_from(WorkflowCommentMention) + .where(WorkflowCommentMention.comment_id == comment.id) + ) + assert mention_count == 0 + + def test_update_comment_notifies_only_new_mentions(self, sqlite_session: Session, delay_mock: Mock) -> None: + comment = _comment() + _persist( + sqlite_session, + _app(), + _account(OWNER_ID, name="Owner", email="owner@example.com"), + _account(USER_2_ID, name="Existing", email="existing@example.com"), + _account(USER_3_ID, name="New User", email="new@example.com"), + _membership(USER_2_ID), + _membership(USER_3_ID), + comment, + ) + _persist(sqlite_session, WorkflowCommentMention(comment_id=comment.id, mentioned_user_id=USER_2_ID)) + + WorkflowCommentService.update_comment( + tenant_id=TENANT_ID, + app_id=APP_ID, + comment_id=comment.id, + user_id=OWNER_ID, + content="updated", + mentioned_user_ids=[USER_2_ID, USER_3_ID], + ) + + delay_mock.assert_called_once() + assert delay_mock.call_args.kwargs["to"] == "new@example.com" + mentions = sqlite_session.scalars( + select(WorkflowCommentMention).where(WorkflowCommentMention.comment_id == comment.id) + ).all() + assert {mention.mentioned_user_id for mention in mentions} == {USER_2_ID, USER_3_ID} + + def test_get_comments_preloads_related_accounts(self, sqlite_session: Session) -> None: + comment = _comment(resolved=True, resolved_by=USER_2_ID) + _persist( + sqlite_session, + _account(OWNER_ID, name="Owner"), + _account(USER_2_ID, name="Resolver"), + _account(USER_3_ID, name="Replier"), + _account(USER_4_ID, name="Mentioned"), + comment, + ) + reply = WorkflowCommentReply(comment_id=comment.id, content="reply", created_by=USER_3_ID) + mention = WorkflowCommentMention(comment_id=comment.id, mentioned_user_id=USER_4_ID) + _persist(sqlite_session, reply, mention) + + result = WorkflowCommentService.get_comments(TENANT_ID, APP_ID) + + assert len(result) == 1 + loaded_comment = result[0] + assert loaded_comment.id == comment.id + assert loaded_comment.created_by_account.id == OWNER_ID + assert loaded_comment.resolved_by_account.id == USER_2_ID + assert loaded_comment.replies[0].created_by_account.id == USER_3_ID + assert loaded_comment.mentions[0].mentioned_user_account.id == USER_4_ID + + def test_preload_accounts_returns_early_for_empty_comments(self, sqlite_session: Session) -> None: + statements: list[str] = [] + bind = sqlite_session.get_bind() + + def record_sql(*args: object) -> None: + statements.append(str(args[2])) + + event.listen(bind, "before_cursor_execute", record_sql) + try: + WorkflowCommentService._preload_accounts(sqlite_session, []) + finally: + event.remove(bind, "before_cursor_execute", record_sql) + + assert statements == [] + + def test_get_comment_raises_not_found_with_provided_session(self, sqlite_session: Session) -> None: + with pytest.raises(NotFound): + WorkflowCommentService.get_comment(TENANT_ID, APP_ID, "missing-comment", session=sqlite_session) + + def test_get_comment_uses_context_manager_when_session_not_provided(self, sqlite_session: Session) -> None: + comment = _comment() + _persist(sqlite_session, _account(OWNER_ID, name="Owner"), comment) + + result = WorkflowCommentService.get_comment(TENANT_ID, APP_ID, comment.id) + + assert result.id == comment.id + assert result.created_by_account.id == OWNER_ID + assert result.resolved_by_account is None + + def test_delete_comment_raises_forbidden(self, sqlite_session: Session) -> None: + comment = _comment() + _persist(sqlite_session, comment) + + with pytest.raises(Forbidden): + WorkflowCommentService.delete_comment(TENANT_ID, APP_ID, comment.id, OUTSIDER_ID) + + assert sqlite_session.get(WorkflowComment, comment.id) is not None + + def test_delete_comment_removes_related_entities(self, sqlite_session: Session) -> None: + comment = _comment() + _persist(sqlite_session, comment) + comment_id = comment.id + reply = WorkflowCommentReply(comment_id=comment.id, content="reply", created_by=USER_2_ID) + _persist(sqlite_session, reply) + _persist( + sqlite_session, + WorkflowCommentMention(comment_id=comment.id, mentioned_user_id=USER_3_ID), + WorkflowCommentMention(comment_id=comment.id, reply_id=reply.id, mentioned_user_id=USER_4_ID), + ) + + WorkflowCommentService.delete_comment(TENANT_ID, APP_ID, comment_id, OWNER_ID) + + sqlite_session.expire_all() + assert sqlite_session.get(WorkflowComment, comment_id) is None + assert ( + sqlite_session.scalar( + select(func.count()) + .select_from(WorkflowCommentReply) + .where(WorkflowCommentReply.comment_id == comment_id) + ) + == 0 + ) + assert ( + sqlite_session.scalar( + select(func.count()) + .select_from(WorkflowCommentMention) + .where(WorkflowCommentMention.comment_id == comment_id) + ) + == 0 + ) + + def test_resolve_comment_sets_fields(self, sqlite_session: Session) -> None: + comment = _comment() + _persist(sqlite_session, comment) + now = datetime(2026, 7, 10, 12, 0, 0) + + with patch.object(service_module, "naive_utc_now", return_value=now): + result = WorkflowCommentService.resolve_comment(TENANT_ID, APP_ID, comment.id, USER_2_ID) + + assert result.resolved is True + assert result.resolved_at == now + assert result.resolved_by == USER_2_ID + sqlite_session.expire_all() + persisted_comment = sqlite_session.get(WorkflowComment, comment.id) + assert persisted_comment is not None + assert persisted_comment.resolved is True + assert persisted_comment.resolved_at == now + assert persisted_comment.resolved_by == USER_2_ID + + def test_resolve_comment_noop_when_already_resolved(self, sqlite_session: Session) -> None: + resolved_at = datetime(2026, 7, 9, 12, 0, 0) + comment = _comment(resolved=True, resolved_at=resolved_at, resolved_by=USER_2_ID) + _persist(sqlite_session, comment) + + result = WorkflowCommentService.resolve_comment(TENANT_ID, APP_ID, comment.id, USER_3_ID) + + assert result.resolved_at == resolved_at + assert result.resolved_by == USER_2_ID + + def test_create_reply_requires_comment(self, sqlite_session: Session) -> None: + with pytest.raises(NotFound): + WorkflowCommentService.create_reply("missing-comment", "hello", OWNER_ID) + + def test_create_reply_creates_mentions(self, sqlite_session: Session) -> None: + comment = _comment() + _persist(sqlite_session, comment, _membership(USER_2_ID)) + + result = WorkflowCommentService.create_reply( + comment_id=comment.id, + content="hello", + created_by=OWNER_ID, + mentioned_user_ids=[USER_2_ID, OUTSIDER_ID], + ) + + reply = sqlite_session.get(WorkflowCommentReply, result["id"]) + assert reply is not None + assert reply.content == "hello" + assert reply.created_at == result["created_at"] + mentions = sqlite_session.scalars( + select(WorkflowCommentMention).where(WorkflowCommentMention.reply_id == reply.id) + ).all() + assert [mention.mentioned_user_id for mention in mentions] == [USER_2_ID] + + def test_update_reply_raises_not_found(self, sqlite_session: Session) -> None: + with pytest.raises(NotFound): + WorkflowCommentService.update_reply( + tenant_id=TENANT_ID, + app_id=APP_ID, + comment_id="missing-comment", + reply_id="missing-reply", + user_id=OWNER_ID, + content="hello", + ) + + def test_update_reply_raises_forbidden(self, sqlite_session: Session) -> None: + comment = _comment() + _persist(sqlite_session, comment) + reply = WorkflowCommentReply(comment_id=comment.id, content="reply", created_by=OWNER_ID) + _persist(sqlite_session, reply) + + with pytest.raises(Forbidden): + WorkflowCommentService.update_reply( + tenant_id=TENANT_ID, + app_id=APP_ID, + comment_id=comment.id, + reply_id=reply.id, + user_id=OUTSIDER_ID, + content="hello", + ) + + def test_update_reply_replaces_mentions(self, sqlite_session: Session) -> None: + comment = _comment() + _persist(sqlite_session, comment, _membership(USER_2_ID)) + reply = WorkflowCommentReply(comment_id=comment.id, content="reply", created_by=OWNER_ID) + _persist(sqlite_session, reply) + _persist( + sqlite_session, + WorkflowCommentMention(comment_id=comment.id, reply_id=reply.id, mentioned_user_id=USER_3_ID), + ) + + result = WorkflowCommentService.update_reply( + tenant_id=TENANT_ID, + app_id=APP_ID, + comment_id=comment.id, + reply_id=reply.id, + user_id=OWNER_ID, + content="new", + mentioned_user_ids=[USER_2_ID, OUTSIDER_ID], + ) + + sqlite_session.expire_all() + persisted_reply = sqlite_session.get(WorkflowCommentReply, reply.id) + assert persisted_reply is not None + assert persisted_reply.content == "new" + assert result == {"id": reply.id, "updated_at": persisted_reply.updated_at} + mentions = sqlite_session.scalars( + select(WorkflowCommentMention).where(WorkflowCommentMention.reply_id == reply.id) + ).all() + assert [mention.mentioned_user_id for mention in mentions] == [USER_2_ID] + + def test_update_comment_updates_position_coordinates_when_provided(self, sqlite_session: Session) -> None: + comment = _comment() + _persist(sqlite_session, comment) + + WorkflowCommentService.update_comment( + tenant_id=TENANT_ID, + app_id=APP_ID, + comment_id=comment.id, + user_id=OWNER_ID, content="updated", position_x=10.5, position_y=20.5, mentioned_user_ids=[], ) - assert comment.position_x == 10.5 - assert comment.position_y == 20.5 + sqlite_session.expire_all() + persisted_comment = sqlite_session.get(WorkflowComment, comment.id) + assert persisted_comment is not None + assert persisted_comment.position_x == 10.5 + assert persisted_comment.position_y == 20.5 - def test_delete_reply_raises_forbidden(self, mock_session: Mock) -> None: - reply = Mock() - reply.created_by = "owner" - mock_session.scalar.return_value = reply + def test_delete_reply_raises_forbidden(self, sqlite_session: Session) -> None: + comment = _comment() + _persist(sqlite_session, comment) + reply = WorkflowCommentReply(comment_id=comment.id, content="reply", created_by=OWNER_ID) + _persist(sqlite_session, reply) with pytest.raises(Forbidden): WorkflowCommentService.delete_reply( - tenant_id="tenant-1", - app_id="app-1", - comment_id="comment-1", - reply_id="reply-1", - user_id="intruder", + tenant_id=TENANT_ID, + app_id=APP_ID, + comment_id=comment.id, + reply_id=reply.id, + user_id=OUTSIDER_ID, ) - def test_delete_reply_raises_not_found(self, mock_session: Mock) -> None: - mock_session.scalar.return_value = None - + def test_delete_reply_raises_not_found(self, sqlite_session: Session) -> None: with pytest.raises(NotFound): WorkflowCommentService.delete_reply( - tenant_id="tenant-1", - app_id="app-1", - comment_id="comment-1", - reply_id="reply-1", - user_id="owner", + tenant_id=TENANT_ID, + app_id=APP_ID, + comment_id="missing-comment", + reply_id="missing-reply", + user_id=OWNER_ID, ) - def test_delete_reply_removes_mentions(self, mock_session: Mock) -> None: - reply = Mock() - reply.created_by = "owner" - mock_session.scalar.return_value = reply - mock_session.scalars.return_value = _mock_scalars([Mock(), Mock()]) - - WorkflowCommentService.delete_reply( - tenant_id="tenant-1", - app_id="app-1", - comment_id="comment-1", - reply_id="reply-1", - user_id="owner", + def test_delete_reply_removes_mentions(self, sqlite_session: Session) -> None: + comment = _comment() + _persist(sqlite_session, comment) + reply = WorkflowCommentReply(comment_id=comment.id, content="reply", created_by=OWNER_ID) + _persist(sqlite_session, reply) + reply_id = reply.id + _persist( + sqlite_session, + WorkflowCommentMention(comment_id=comment.id, reply_id=reply.id, mentioned_user_id=USER_2_ID), + WorkflowCommentMention(comment_id=comment.id, reply_id=reply.id, mentioned_user_id=USER_3_ID), ) - assert mock_session.delete.call_count == 3 - mock_session.commit.assert_called_once() + WorkflowCommentService.delete_reply( + tenant_id=TENANT_ID, + app_id=APP_ID, + comment_id=comment.id, + reply_id=reply_id, + user_id=OWNER_ID, + ) - def test_validate_comment_access_delegates_to_get_comment(self) -> None: - comment = Mock() - with patch.object(WorkflowCommentService, "get_comment", return_value=comment) as get_comment_mock: - result = WorkflowCommentService.validate_comment_access("comment-1", "tenant-1", "app-1") + sqlite_session.expire_all() + assert sqlite_session.get(WorkflowCommentReply, reply_id) is None + assert ( + sqlite_session.scalar( + select(func.count()) + .select_from(WorkflowCommentMention) + .where(WorkflowCommentMention.reply_id == reply_id) + ) + == 0 + ) - assert result is comment - get_comment_mock.assert_called_once_with("tenant-1", "app-1", "comment-1") + def test_validate_comment_access_delegates_to_get_comment(self, sqlite_session: Session) -> None: + comment = _comment() + _persist(sqlite_session, comment) + + result = WorkflowCommentService.validate_comment_access(comment.id, TENANT_ID, APP_ID) + + assert result.id == comment.id + + def test_reply_lookup_is_scoped_to_tenant_app_and_comment(self, sqlite_session: Session) -> None: + comment = _comment() + other_comment = _comment(app_id=OTHER_APP_ID) + _persist(sqlite_session, comment, other_comment) + reply = WorkflowCommentReply(comment_id=comment.id, content="reply", created_by=OWNER_ID) + _persist(sqlite_session, reply) + + with pytest.raises(NotFound): + WorkflowCommentService.update_reply( + tenant_id=TENANT_ID, + app_id=OTHER_APP_ID, + comment_id=other_comment.id, + reply_id=reply.id, + user_id=OWNER_ID, + content="cross-thread update", + ) + + sqlite_session.refresh(reply) + assert reply.content == "reply"