diff --git a/cards/merge_request.yaml.j2 b/cards/merge_request.yaml.j2 index 39a8e44..4723dfc 100644 --- a/cards/merge_request.yaml.j2 +++ b/cards/merge_request.yaml.j2 @@ -19,6 +19,7 @@ body: - details - showMore - showLess + - threadsCollapsed columns: - type: Column verticalContentAlignment: Center @@ -58,6 +59,15 @@ body: - details - showMore - showLess + - threadsCollapsed +{% if precalc.discussion_stats and precalc.discussion_stats.threads_unresolved > 0 %} +- type: TextBlock + id: threadsCollapsed + isVisible: {{ collapsed }} + spacing: Small + text: '**Threads** {{ precalc.discussion_stats.threads_resolved }}/{{ precalc.discussion_stats.threads_total }} resolved' + color: Warning +{% endif %} - type: RichTextBlock id: showMore isVisible: {{ collapsed }} @@ -72,6 +82,7 @@ body: - details - showMore - showLess + - threadsCollapsed {% endif %} - type: Container diff --git a/cards/render.py b/cards/render.py index 056dc82..19d6821 100644 --- a/cards/render.py +++ b/cards/render.py @@ -107,6 +107,7 @@ def render( and mri.merge_request_payload.object_attributes.action not in ("close", "merge") ): icon_color = Teams_Color.WARNING + icon_name = "CommentError" precalc = { "path_with_namespace": mri.merge_request_payload.project.path_with_namespace, diff --git a/db.py b/db.py index 24809a4..45f1a55 100644 --- a/db.py +++ b/db.py @@ -127,6 +127,11 @@ class MergeRequestExtraState(BaseModel): discussion_stats: DiscussionStats | None = None +def has_unresolved_threads(extra_state: MergeRequestExtraState) -> bool: + """Check if extra_state has unresolved threads.""" + return extra_state.discussion_stats is not None and extra_state.discussion_stats.threads_unresolved > 0 + + class MergeRequestInfos(BaseModel): merge_request_ref_id: int merge_request_payload: MergeRequestPayload @@ -144,6 +149,15 @@ def compute_mri_fingerprint(mri: MergeRequestInfos) -> str: return hashlib.sha256(json.dumps(datasource, sort_keys=True, default=str).encode()).hexdigest() +def make_mr_summary(mri: MergeRequestInfos) -> str: + """Create a summary string for Teams message fallback.""" + return ( + f"MR {mri.merge_request_payload.object_attributes.state}:" + f" {mri.merge_request_payload.object_attributes.title}\n" + f"on {mri.merge_request_payload.project.path_with_namespace}" + ) + + class DBHelper: def __init__(self, database: DatabaseLifecycleHandler): self.db: DatabaseLifecycleHandler = database diff --git a/periodic_cleanup.py b/periodic_cleanup.py index 78dd368..183e126 100644 --- a/periodic_cleanup.py +++ b/periodic_cleanup.py @@ -12,6 +12,8 @@ from db import MergeRequestInfos from db import compute_mri_fingerprint from db import dbh +from db import has_unresolved_threads +from db import make_mr_summary from gitlab_api import fetch_and_persist_discussion_stats from webhook.messaging import update_all_messages_transactional @@ -44,6 +46,8 @@ async def _process_pending_refreshes() -> int: head_pipeline_id=row["head_pipeline_id"], ) + had_unresolved_threads = has_unresolved_threads(mri.merge_request_extra_state) + updated_extra_state = await fetch_and_persist_discussion_stats( merge_request_ref_id=mri.merge_request_ref_id, project_url=mri.merge_request_payload.project.web_url, @@ -53,18 +57,9 @@ async def _process_pending_refreshes() -> int: if updated_extra_state is not None: mri.merge_request_extra_state = updated_extra_state - should_be_collapsed: bool = ( - mri.merge_request_payload.object_attributes.draft - or mri.merge_request_payload.object_attributes.work_in_progress - or mri.merge_request_payload.object_attributes.state in ("closed", "merged") - ) - card = render(mri, collapsed=should_be_collapsed, show_collapsible=should_be_collapsed) - datasource_fingerprint = compute_mri_fingerprint(mri) - summary = ( - f"MR {mri.merge_request_payload.object_attributes.state}:" - f" {mri.merge_request_payload.object_attributes.title}\n" - f"on {mri.merge_request_payload.project.path_with_namespace}" - ) + now_has_unresolved_threads = has_unresolved_threads(mri.merge_request_extra_state) + + is_closing_state = mri.merge_request_payload.object_attributes.state in ("closed", "merged") # Use GitLab's updated_at from stored payload (not local last_event_at) # to keep timestamps comparable for OOO detection @@ -72,10 +67,42 @@ async def _process_pending_refreshes() -> int: mri.merge_request_payload.object_attributes.updated_at.replace(" UTC", "+00:00") ) + if had_unresolved_threads and not now_has_unresolved_threads and not is_closing_state: + datasource_fingerprint = compute_mri_fingerprint(mri) + temp_card = render(mri, collapsed=False, show_collapsible=False) + await update_all_messages_transactional( + mri, + temp_card, + make_mr_summary(mri), + datasource_fingerprint, + payload_updated_at, + "threads-resolved", + schedule_deletion=True, + deletion_delay=datetime.timedelta(seconds=0), + ) + await dbh.delete_pending_refresh(mri.merge_request_ref_id) + processed += 1 + logger.info( + "pending refresh processed - threads resolved, message re-emitted", + merge_request_ref_id=mri.merge_request_ref_id, + payload_type=row["payload_type"], + fingerprint=datasource_fingerprint[:16], + ) + continue + + should_be_collapsed: bool = ( + mri.merge_request_payload.object_attributes.draft + or mri.merge_request_payload.object_attributes.work_in_progress + or is_closing_state + or now_has_unresolved_threads + ) + card = render(mri, collapsed=should_be_collapsed, show_collapsible=should_be_collapsed) + datasource_fingerprint = compute_mri_fingerprint(mri) + messages_updated = await update_all_messages_transactional( mri, card, - summary, + make_mr_summary(mri), datasource_fingerprint, payload_updated_at, row["payload_type"], diff --git a/tests/test_card_render.py b/tests/test_card_render.py index e9a9ee5..c5979fa 100644 --- a/tests/test_card_render.py +++ b/tests/test_card_render.py @@ -236,53 +236,109 @@ def find_icon_color(card: dict[str, Any]) -> str | None: return None -class TestIconColor: - """Tests for icon color based on MR state and unresolved threads.""" +def find_icon_name(card: dict[str, Any]) -> str | None: + """Find the icon name in the adaptive card body.""" + for item in card.get("body", []): + if item.get("type") == "ColumnSet": + for col in item.get("columns", []): + for inner in col.get("items", []): + if inner.get("type") == "Icon": + name = inner.get("name") + return str(name) if name is not None else None + return None - def test_default_icon_color_no_discussion_stats(self): - """Icon should be accent when no discussion stats.""" + +class TestIconColorAndName: + """Tests for icon color and name based on MR state and unresolved threads.""" + + def test_default_icon_no_discussion_stats(self): + """Icon should be BranchRequest with accent color when no discussion stats.""" mri = make_mri() result = render(mri) assert find_icon_color(result) == "accent" + assert find_icon_name(result) == "BranchRequest" - def test_default_icon_color_no_unresolved_threads(self): - """Icon should be accent when all threads resolved.""" + def test_default_icon_no_unresolved_threads(self): + """Icon should be BranchRequest with accent color when all threads resolved.""" stats = DiscussionStats(threads_total=3, threads_resolved=3, threads_unresolved=0) mri = make_mri(discussion_stats=stats) result = render(mri) assert find_icon_color(result) == "accent" + assert find_icon_name(result) == "BranchRequest" - def test_warning_icon_color_with_unresolved_threads(self): - """Icon should be warning (orange) when there are unresolved threads.""" + def test_chatbubbles_icon_with_unresolved_threads(self): + """Icon should be CommentError with warning color when unresolved threads.""" stats = DiscussionStats(threads_total=3, threads_resolved=1, threads_unresolved=2) mri = make_mri(discussion_stats=stats) result = render(mri) assert find_icon_color(result) == "warning" + assert find_icon_name(result) == "CommentError" def test_closed_mr_keeps_attention_regardless_of_threads(self): - """Closed MR should keep attention color, not be overridden by thread status.""" + """Closed MR should keep CodeTextOff icon with attention color.""" stats = DiscussionStats(threads_total=3, threads_resolved=1, threads_unresolved=2) mri = make_mri(action="close", discussion_stats=stats) result = render(mri) assert find_icon_color(result) == "attention" + assert find_icon_name(result) == "CodeTextOff" def test_merged_mr_keeps_good_regardless_of_threads(self): - """Merged MR should keep good color, not be overridden by thread status.""" + """Merged MR should keep Merge icon with good color.""" stats = DiscussionStats(threads_total=3, threads_resolved=1, threads_unresolved=2) mri = make_mri(action="merge", discussion_stats=stats) result = render(mri) assert find_icon_color(result) == "good" + assert find_icon_name(result) == "Merge" - def test_draft_mr_with_unresolved_threads_shows_warning(self): - """Draft MR with unresolved threads should show warning color.""" + def test_draft_mr_with_unresolved_threads_shows_chatbubbles(self): + """Draft MR with unresolved threads should show CommentError with warning.""" stats = DiscussionStats(threads_total=2, threads_resolved=0, threads_unresolved=2) mri = make_mri(draft=True, discussion_stats=stats) result = render(mri) assert find_icon_color(result) == "warning" + assert find_icon_name(result) == "CommentError" - def test_draft_mr_without_unresolved_threads_shows_default(self): - """Draft MR without unresolved threads should show default color.""" + def test_draft_mr_without_unresolved_threads_shows_drafts(self): + """Draft MR without unresolved threads should show Drafts icon with default color.""" stats = DiscussionStats(threads_total=2, threads_resolved=2, threads_unresolved=0) mri = make_mri(draft=True, discussion_stats=stats) result = render(mri) assert find_icon_color(result) == "default" + assert find_icon_name(result) == "Drafts" + + +def find_thread_count_block(card: dict[str, Any]) -> dict[str, Any] | None: + """Find the threadsCollapsed TextBlock in the card body.""" + for item in card.get("body", []): + if item.get("type") == "TextBlock" and item.get("id") == "threadsCollapsed": + return dict(item) + return None + + +class TestCollapsedWithThreads: + """Tests for collapsed state showing thread count.""" + + def test_collapsed_with_threads_shows_thread_count(self): + """Collapsed card with unresolved threads should show 'Threads X/Y resolved' on separate line.""" + stats = DiscussionStats(threads_total=5, threads_resolved=2, threads_unresolved=3) + mri = make_mri(discussion_stats=stats) + result = render(mri, collapsed=True, show_collapsible=True) + thread_block = find_thread_count_block(result) + assert thread_block is not None + assert "2/5 resolved" in thread_block.get("text", "") + assert thread_block.get("color") == "Warning" + + def test_collapsed_without_threads_no_thread_count(self): + """Collapsed card without unresolved threads should not show thread count block.""" + stats = DiscussionStats(threads_total=3, threads_resolved=3, threads_unresolved=0) + mri = make_mri(discussion_stats=stats) + result = render(mri, collapsed=True, show_collapsible=True) + thread_block = find_thread_count_block(result) + assert thread_block is None + + def test_collapsed_draft_no_thread_count(self): + """Collapsed draft without threads should not show thread count block.""" + mri = make_mri(draft=True) + result = render(mri, collapsed=True, show_collapsible=True) + thread_block = find_thread_count_block(result) + assert thread_block is None diff --git a/tests/test_integration_webhook_flow.py b/tests/test_integration_webhook_flow.py index dc0951a..7cc0aec 100644 --- a/tests/test_integration_webhook_flow.py +++ b/tests/test_integration_webhook_flow.py @@ -123,6 +123,7 @@ async def test_webhook_endpoint_merge_request_open( merge_request_extra_state=MagicMock( opener=MagicMock(id=1), approvers={}, + discussion_stats=None, ), ) @@ -180,6 +181,7 @@ async def test_webhook_endpoint_merge_request_merge( merge_request_extra_state=MagicMock( opener=MagicMock(id=1), approvers={}, + discussion_stats=None, ), ) @@ -268,6 +270,7 @@ async def test_webhook_endpoint_invalid_conversation_token( merge_request_extra_state=MagicMock( opener=MagicMock(id=1), approvers={}, + discussion_stats=None, ), ) @@ -316,6 +319,7 @@ async def test_webhook_endpoint_multiple_conversation_tokens( merge_request_extra_state=MagicMock( opener=MagicMock(id=1), approvers={}, + discussion_stats=None, ), ) @@ -365,6 +369,7 @@ async def test_webhook_endpoint_activity_api_error( merge_request_extra_state=MagicMock( opener=MagicMock(id=1), approvers={}, + discussion_stats=None, ), ) diff --git a/tests/test_merge_request_deletion.py b/tests/test_merge_request_deletion.py index e0055e1..9782c7e 100644 --- a/tests/test_merge_request_deletion.py +++ b/tests/test_merge_request_deletion.py @@ -35,7 +35,7 @@ async def test_merge_close_uses_transactional_update(mock_database, sample_merge sample_mri = AsyncMock( merge_request_ref_id=merge_request_ref_id, merge_request_payload=sample_merge_request_payload, - merge_request_extra_state=AsyncMock(opener=sample_merge_request_payload.user), + merge_request_extra_state=AsyncMock(opener=sample_merge_request_payload.user, discussion_stats=None), ) with ( @@ -82,7 +82,7 @@ async def test_merge_close_transactional_rollback_on_failure(mock_database, samp sample_mri = AsyncMock( merge_request_ref_id=merge_request_ref_id, merge_request_payload=sample_merge_request_payload, - merge_request_extra_state=AsyncMock(opener=sample_merge_request_payload.user), + merge_request_extra_state=AsyncMock(opener=sample_merge_request_payload.user, discussion_stats=None), ) with ( @@ -179,7 +179,7 @@ async def test_multiple_messages_all_deleted_transactionally(mock_database, samp sample_mri = AsyncMock( merge_request_ref_id=merge_request_ref_id, merge_request_payload=sample_merge_request_payload, - merge_request_extra_state=AsyncMock(opener=sample_merge_request_payload.user), + merge_request_extra_state=AsyncMock(opener=sample_merge_request_payload.user, discussion_stats=None), ) with ( @@ -223,7 +223,7 @@ async def test_draft_to_ready_uses_transaction(mock_database, sample_merge_reque sample_merge_request_payload.changes = {"draft": {"previous": True, "current": False}} connection.fetch.return_value = [{"merge_request_message_ref_id": 1, "message_id": uuid.uuid4()}] - connection.fetchrow.return_value = {"merge_request_extra_state": {}} + connection.fetchrow.return_value = {"merge_request_extra_state": MagicMock(discussion_stats=None)} mock_ref = MRMessRef( merge_request_message_ref_id=1, @@ -237,6 +237,7 @@ async def test_draft_to_ready_uses_transaction(mock_database, sample_merge_reque merge_request_extra_state=AsyncMock( opener=sample_merge_request_payload.user, approvers={}, + discussion_stats=None, ), ) @@ -286,7 +287,7 @@ async def test_merge_with_no_messages_doesnt_delete_mr_ref(mock_database, sample sample_mri = AsyncMock( merge_request_ref_id=100, merge_request_payload=sample_merge_request_payload, - merge_request_extra_state=AsyncMock(opener=sample_merge_request_payload.user), + merge_request_extra_state=AsyncMock(opener=sample_merge_request_payload.user, discussion_stats=None), ) with ( diff --git a/tests/test_merge_request_handler.py b/tests/test_merge_request_handler.py index 0348814..ab1a382 100644 --- a/tests/test_merge_request_handler.py +++ b/tests/test_merge_request_handler.py @@ -55,6 +55,7 @@ def sample_mri(): mri.merge_request_payload.assignees = [] mri.merge_request_payload.reviewers = [] mri.merge_request_extra_state.opener.id = 1 + mri.merge_request_extra_state.discussion_stats = None return mri @@ -251,7 +252,9 @@ async def test_mr_update_updates_pipeline_id(self, sample_mr_payload, sample_mri sample_mr_payload.object_attributes.head_pipeline_id = 999 mock_ref = make_mock_ref("token1", message_id=uuid.uuid4()) - mock_database.connection.fetchrow.return_value = {"merge_request_extra_state": {}} + mock_database.connection.fetchrow.return_value = { + "merge_request_extra_state": MagicMock(discussion_stats=None) + } with ( patch("webhook.merge_request.dbh.get_or_create_merge_request_ref_id", return_value=1), @@ -291,7 +294,9 @@ async def test_mr_update_with_new_commits_revokes_approvals( sample_mr_payload.object_attributes.oldrev = "abc123" mock_ref = make_mock_ref("token1", message_id=uuid.uuid4()) - mock_database.connection.fetchrow.return_value = {"merge_request_extra_state": {}} + mock_database.connection.fetchrow.return_value = { + "merge_request_extra_state": MagicMock(discussion_stats=None) + } with ( patch("webhook.merge_request.dbh.get_or_create_merge_request_ref_id", return_value=1), @@ -333,7 +338,9 @@ async def test_draft_to_ready_transition_deletes_and_creates_messages( sample_mr_payload.changes = {"draft": {"previous": True, "current": False}} new_mock_ref = make_mock_ref("token1", message_id=None) - mock_database.connection.fetchrow.return_value = {"merge_request_extra_state": {}} + mock_database.connection.fetchrow.return_value = { + "merge_request_extra_state": MagicMock(discussion_stats=None) + } with ( patch("webhook.merge_request.dbh.get_or_create_merge_request_ref_id", return_value=1), diff --git a/tests/test_message_operations.py b/tests/test_message_operations.py index 3bcaa4f..cee8d83 100644 --- a/tests/test_message_operations.py +++ b/tests/test_message_operations.py @@ -40,7 +40,7 @@ async def test_merge_creates_single_deletion_record(mock_database, sample_merge_ sample_mri = AsyncMock( merge_request_ref_id=100, merge_request_payload=sample_merge_request_payload, - merge_request_extra_state=AsyncMock(opener=sample_merge_request_payload.user), + merge_request_extra_state=AsyncMock(opener=sample_merge_request_payload.user, discussion_stats=None), ) with ( @@ -206,7 +206,7 @@ async def test_draft_transition_uses_for_update_lock(mock_database, sample_merge ] connection.fetch.return_value = messages - connection.fetchrow.return_value = {"merge_request_extra_state": {}} + connection.fetchrow.return_value = {"merge_request_extra_state": MagicMock(discussion_stats=None)} msg_ref = MRMessRef( merge_request_message_ref_id=1, @@ -220,6 +220,7 @@ async def test_draft_transition_uses_for_update_lock(mock_database, sample_merge merge_request_extra_state=AsyncMock( opener=sample_merge_request_payload.user, approvers={}, + discussion_stats=None, ), ) @@ -271,9 +272,10 @@ async def test_approval_updates_extra_state(mock_database, sample_merge_request_ message_id = uuid.uuid4() connection.fetchrow.return_value = { - "merge_request_extra_state": { - "approvers": {"1": {"id": 1, "username": "user1", "status": "approved"}} - } + "merge_request_extra_state": MagicMock( + approvers={"1": {"id": 1, "username": "user1", "status": "approved"}}, + discussion_stats=None, + ) } msg_ref = MRMessRef( @@ -288,6 +290,7 @@ async def test_approval_updates_extra_state(mock_database, sample_merge_request_ merge_request_extra_state=AsyncMock( opener=sample_merge_request_payload.user, approvers={"1": {"id": 1, "username": "user1", "status": "approved"}}, + discussion_stats=None, ), ) diff --git a/tests/test_mr_state_transitions.py b/tests/test_mr_state_transitions.py index 36058b3..e1e3051 100644 --- a/tests/test_mr_state_transitions.py +++ b/tests/test_mr_state_transitions.py @@ -34,6 +34,7 @@ def make_mock_mri(payload, ref_id=1): mri.merge_request_extra_state = MagicMock() mri.merge_request_extra_state.opener = payload.user mri.merge_request_extra_state.approvers = {} + mri.merge_request_extra_state.discussion_stats = None return mri @@ -258,7 +259,9 @@ async def test_mr_state_approved_updates_approvers(base_mr_payload, mock_databas connection = mock_database.connection connection.fetchrow.return_value = { - "merge_request_extra_state": {"approvers": {"1": {"id": 1, "status": "approved"}}} + "merge_request_extra_state": MagicMock( + approvers={"1": {"id": 1, "status": "approved"}}, discussion_stats=None + ) } sample_mri = make_mock_mri(base_mr_payload) @@ -314,7 +317,9 @@ async def test_mr_state_unapproved_updates_approvers(base_mr_payload, mock_datab connection = mock_database.connection connection.fetchrow.return_value = { - "merge_request_extra_state": {"approvers": {"1": {"id": 1, "status": "unapproved"}}} + "merge_request_extra_state": MagicMock( + approvers={"1": {"id": 1, "status": "unapproved"}}, discussion_stats=None + ) } sample_mri = make_mock_mri(base_mr_payload) @@ -370,7 +375,9 @@ async def test_mr_update_with_new_commits_resets_approvals(base_mr_payload, mock ) connection = mock_database.connection - connection.fetchrow.return_value = {"merge_request_extra_state": {"approvers": {}}} + connection.fetchrow.return_value = { + "merge_request_extra_state": MagicMock(approvers={}, discussion_stats=None) + } sample_mri = make_mock_mri(base_mr_payload) @@ -437,7 +444,7 @@ async def test_mr_draft_to_ready_deletes_old_messages(base_mr_payload, mock_data "message_id": msg_ref.message_id, } ] - connection.fetchrow.return_value = {"merge_request_extra_state": {}} + connection.fetchrow.return_value = {"merge_request_extra_state": MagicMock(discussion_stats=None)} sample_mri = make_mock_mri(base_mr_payload) @@ -590,3 +597,137 @@ async def test_mr_update_existing_message_uses_patch(base_mr_payload, mock_datab assert mock_client.request.call_count == 1 call_args = mock_client.request.call_args assert call_args[0][0] == "PATCH" + + +@pytest.mark.asyncio +async def test_threads_resolved_triggers_reemission(base_mr_payload, mock_database): + """Test that resolving all threads triggers message re-emission.""" + base_mr_payload.object_attributes.action = "update" + base_mr_payload.object_attributes.state = "opened" + base_mr_payload.object_attributes.draft = False + base_mr_payload.object_attributes.updated_at = "2025-01-01T12:00:00Z" + + conv_token = uuid.UUID("11111111-1111-1111-1111-111111111111") + msg_ref = MRMessRef( + merge_request_message_ref_id=1, + conversation_token=conv_token, + message_id=uuid.uuid4(), + ) + + connection = mock_database.connection + connection.fetch.return_value = [ + { + "merge_request_message_ref_id": 1, + "conversation_token": conv_token, + "message_id": msg_ref.message_id, + } + ] + + sample_mri = make_mock_mri(base_mr_payload) + + call_count = [0] + + def mock_has_unresolved(extra_state): + call_count[0] += 1 + return call_count[0] == 1 + + with ( + patch("webhook.merge_request.database", mock_database), + patch("webhook.messaging.database", mock_database), + patch("webhook.merge_request.dbh.get_or_create_merge_request_ref_id", return_value=1), + patch("webhook.merge_request.dbh.update_merge_request_ref_payload", return_value=sample_mri), + patch("webhook.merge_request.dbh.get_merge_request_ref_infos", return_value=sample_mri), + patch("webhook.merge_request.dbh.any_message_needs_update", return_value=True), + patch("webhook.merge_request.fetch_and_persist_discussion_stats", return_value=None), + patch("webhook.merge_request.has_unresolved_threads", side_effect=mock_has_unresolved), + patch("webhook.merge_request.render"), + patch("webhook.merge_request.compute_mri_fingerprint", return_value="test-fp"), + patch("webhook.merge_request.update_all_messages_transactional") as mock_update_transactional, + patch("webhook.merge_request.get_or_create_message_refs") as mock_get_or_create, + patch("webhook.merge_request.get_all_message_refs") as mock_get_all, + patch("webhook.merge_request.periodic_cleanup") as mock_cleanup, + patch("httpx.AsyncClient") as mock_client_class, + ): + mock_update_transactional.return_value = 1 + mock_get_or_create.return_value = {str(conv_token): msg_ref} + mock_get_all.return_value = [msg_ref] + + mock_client = AsyncMock() + mock_response = MagicMock() + mock_response.status_code = 200 + mock_response.json.return_value = {"message_id": str(msg_ref.message_id)} + mock_client.request = AsyncMock(return_value=mock_response) + mock_client_class.return_value.__aenter__.return_value = mock_client + + from webhook.merge_request import merge_request + + await merge_request( + mr=base_mr_payload, + conversation_tokens=[str(conv_token)], + participant_ids_filter=[], + new_commits_revoke_approvals=False, + ) + + assert mock_update_transactional.called + call_kwargs = mock_update_transactional.call_args[1] + assert call_kwargs["schedule_deletion"] is True + assert call_kwargs["deletion_delay"].total_seconds() == 0 + assert "threads-resolved" in str(mock_update_transactional.call_args) + + assert mock_cleanup.reschedule.called + + +@pytest.mark.asyncio +async def test_threads_not_resolved_no_reemission(base_mr_payload, mock_database): + """Test that keeping unresolved threads does not trigger re-emission.""" + base_mr_payload.object_attributes.action = "update" + base_mr_payload.object_attributes.state = "opened" + base_mr_payload.object_attributes.draft = False + base_mr_payload.object_attributes.updated_at = "2025-01-01T12:00:00Z" + + conv_token = uuid.UUID("11111111-1111-1111-1111-111111111111") + msg_ref = MRMessRef( + merge_request_message_ref_id=1, + conversation_token=conv_token, + message_id=uuid.uuid4(), + ) + + sample_mri = make_mock_mri(base_mr_payload) + + with ( + patch("webhook.merge_request.database", mock_database), + patch("webhook.messaging.database", mock_database), + patch("webhook.merge_request.dbh.get_or_create_merge_request_ref_id", return_value=1), + patch("webhook.merge_request.dbh.update_merge_request_ref_payload", return_value=sample_mri), + patch("webhook.merge_request.dbh.get_merge_request_ref_infos", return_value=sample_mri), + patch("webhook.merge_request.dbh.any_message_needs_update", return_value=False), + patch("webhook.merge_request.has_unresolved_threads", return_value=True), + patch("webhook.merge_request.render"), + patch("webhook.merge_request.compute_mri_fingerprint", return_value="test-fp"), + patch("webhook.merge_request.update_all_messages_transactional") as mock_update_transactional, + patch("webhook.merge_request.get_or_create_message_refs") as mock_get_or_create, + patch("webhook.merge_request.get_all_message_refs") as mock_get_all, + patch("httpx.AsyncClient") as mock_client_class, + ): + mock_get_or_create.return_value = {str(conv_token): msg_ref} + mock_get_all.return_value = [msg_ref] + + mock_client = AsyncMock() + mock_response = MagicMock() + mock_response.status_code = 200 + mock_response.json.return_value = {"message_id": str(msg_ref.message_id)} + mock_client.request = AsyncMock(return_value=mock_response) + mock_client_class.return_value.__aenter__.return_value = mock_client + + from webhook.merge_request import merge_request + + await merge_request( + mr=base_mr_payload, + conversation_tokens=[str(conv_token)], + participant_ids_filter=[], + new_commits_revoke_approvals=False, + ) + + # Transactional update may or may not be called, but never for "threads-resolved" + if mock_update_transactional.called: + assert "threads-resolved" not in str(mock_update_transactional.call_args) diff --git a/tests/test_participant_filtering.py b/tests/test_participant_filtering.py index 7bbfe92..b5839fd 100644 --- a/tests/test_participant_filtering.py +++ b/tests/test_participant_filtering.py @@ -44,6 +44,7 @@ def make_mock_mri(payload, ref_id=1): mri.merge_request_extra_state = MagicMock() mri.merge_request_extra_state.opener = payload.user mri.merge_request_extra_state.approvers = {} + mri.merge_request_extra_state.discussion_stats = None return mri diff --git a/webhook/merge_request.py b/webhook/merge_request.py index d7593db..8fdaec6 100644 --- a/webhook/merge_request.py +++ b/webhook/merge_request.py @@ -12,6 +12,8 @@ from db import compute_mri_fingerprint from db import database from db import dbh +from db import has_unresolved_threads +from db import make_mr_summary from gitlab_api import fetch_and_persist_discussion_stats from gitlab_model import MergeRequestPayload from webhook.messaging import create_or_update_message @@ -132,11 +134,7 @@ async def merge_request( temp_mri = await dbh.get_merge_request_ref_infos(mr) temp_card = render(temp_mri, collapsed=False, show_collapsible=False) temp_fingerprint = compute_mri_fingerprint(temp_mri) - temp_summary = ( - f"MR {temp_mri.merge_request_payload.object_attributes.state}:" - f" {temp_mri.merge_request_payload.object_attributes.title}\n" - f"on {temp_mri.merge_request_payload.project.path_with_namespace}" - ) + temp_summary = make_mr_summary(temp_mri) await update_all_messages_transactional( temp_mri, @@ -168,6 +166,8 @@ async def merge_request( mri = await dbh.get_merge_request_ref_infos(mr) datasource_fingerprint = compute_mri_fingerprint(mri) + had_unresolved_threads = has_unresolved_threads(mri.merge_request_extra_state) + if await dbh.any_message_needs_update(mri.merge_request_ref_id, datasource_fingerprint): updated_extra_state = await fetch_and_persist_discussion_stats( merge_request_ref_id=mri.merge_request_ref_id, @@ -179,21 +179,30 @@ async def merge_request( mri.merge_request_extra_state = updated_extra_state datasource_fingerprint = compute_mri_fingerprint(mri) + now_has_unresolved_threads = has_unresolved_threads(mri.merge_request_extra_state) + + if had_unresolved_threads and not now_has_unresolved_threads and not is_closing_action: + temp_card = render(mri, collapsed=False, show_collapsible=False) + await update_all_messages_transactional( + mri, + temp_card, + make_mr_summary(mri), + datasource_fingerprint, + payload_updated_at, + "threads-resolved", + schedule_deletion=True, + deletion_delay=datetime.timedelta(seconds=0), + ) + need_cleanup_reschedule = True + should_be_collapsed: bool = ( mr.object_attributes.draft or mr.object_attributes.work_in_progress - or mr.object_attributes.state - in ( - "closed", - "merged", - ) + or mr.object_attributes.state in ("closed", "merged") + or now_has_unresolved_threads ) card = render(mri, collapsed=should_be_collapsed, show_collapsible=should_be_collapsed) - summary = ( - f"MR {mri.merge_request_payload.object_attributes.state}:" - f" {mri.merge_request_payload.object_attributes.title}\n" - f"on {mri.merge_request_payload.project.path_with_namespace}" - ) + summary = make_mr_summary(mri) if is_closing_action: await update_all_messages_transactional(