diff --git a/src/firetower/incidents/tasks/action_items.py b/src/firetower/incidents/tasks/action_items.py index 2eebad10..08c24fcf 100644 --- a/src/firetower/incidents/tasks/action_items.py +++ b/src/firetower/incidents/tasks/action_items.py @@ -1,6 +1,7 @@ import logging from dataclasses import dataclass from datetime import timedelta +from uuid import NAMESPACE_URL, UUID, uuid5 from django.conf import settings from django.utils import timezone @@ -71,6 +72,79 @@ def _build_nag_tiers(linear: dict) -> dict[int, NagTier]: _NAG_TEMPLATE_ENV = Environment(autoescape=False) +MISFILED_ACTION_ITEM_REMINDER_MARKER = "Firetower action-item team reminder" + + +def _remind_misfiled_action_item_creators(linear: dict) -> None: + if not linear.get("SYNC_IDENTIFIERS", False): + return + + team_id = linear.get("TEAM_ID") + if not team_id: + return + + try: + service = LinearService() + issues = service.get_misfiled_action_items( + team_id, MISFILED_ACTION_ITEM_REMINDER_MARKER + ) + except Exception: + logger.info( + "Failed to fetch misfiled action items from Linear; skipping team reminders", + exc_info=True, + ) + return + if issues is None: + logger.info( + "Failed to fetch misfiled action items from Linear; skipping team reminders" + ) + return + + logger.info( + "Found %d misfiled action items without a team reminder in Linear team %s", + len(issues), + team_id, + ) + for issue in issues: + issue_id = None + try: + issue_id = issue["id"] + creator = issue.get("creator") + greeting = ( + f"Hi @[{creator['displayName']}]({creator['id']}), " + if creator + else "Hi, " + ) + comment = ( + f"**{MISFILED_ACTION_ITEM_REMINDER_MARKER}**\n\n" + f"{greeting}this action item was created in the " + f"{settings.PROJECT_KEY} team. Please move it to the relevant " + "actioning team, keeping it as a child of the incident ticket. " + "The incident team is reserved for incident tickets, not action items." + ) + # A stable UUID in Linear's required v4 format also prevents duplicate + # comments from overlapping runs or a successful but timed-out request. + comment_id = str( + UUID( + bytes=uuid5( + NAMESPACE_URL, f"firetower:inc-team-reminder:{issue_id}" + ).bytes, + version=4, + ) + ) + success = service.create_comment(issue_id, comment, comment_id=comment_id) + except Exception: + logger.info( + "Failed to post team reminder for action item %s", + issue_id, + exc_info=True, + ) + continue + if not success: + logger.info("Failed to post team reminder for action item %s", issue_id) + else: + logger.info("Posted team reminder for action item %s", issue_id) + @datadog_log def send_action_item_reminder() -> None: @@ -81,7 +155,8 @@ def send_action_item_reminder() -> None: for priority, tier in nag_tiers.items() } if not any(comments_by_priority.values()): - logger.warning("No Linear nag comments configured, skipping job") + logger.warning("No Linear nag comments configured, skipping SLO reminders") + _remind_misfiled_action_item_creators(linear) return now = timezone.now() @@ -209,3 +284,5 @@ def _nag(action_item: ActionItem, incident: Incident) -> None: for action_item in eligible: _nag(action_item, incident) + + _remind_misfiled_action_item_creators(linear) diff --git a/src/firetower/incidents/tests/test_action_item_team_reminders.py b/src/firetower/incidents/tests/test_action_item_team_reminders.py new file mode 100644 index 00000000..262b5b00 --- /dev/null +++ b/src/firetower/incidents/tests/test_action_item_team_reminders.py @@ -0,0 +1,223 @@ +from unittest.mock import MagicMock, patch +from uuid import UUID + +import pytest + +from firetower.incidents.tasks.action_items import ( + MISFILED_ACTION_ITEM_REMINDER_MARKER, + _remind_misfiled_action_item_creators, + send_action_item_reminder, +) + +LINEAR = {"TEAM_ID": "incident-team", "SYNC_IDENTIFIERS": True} +ISSUE = { + "id": "issue-1", + "identifier": "INC-2508", + "creator": {"id": "creator-id", "displayName": "Leander Rodrigues"}, +} + + +@pytest.fixture +def service(settings): + settings.PROJECT_KEY = "INC" + settings.LINEAR = LINEAR + service = MagicMock() + service.get_misfiled_action_items.return_value = [ISSUE] + service.create_comment.return_value = True + with patch( + "firetower.incidents.tasks.action_items.LinearService", return_value=service + ): + yield service + + +def test_mentions_creator_and_explains_move(service): + _remind_misfiled_action_item_creators(LINEAR) + + service.get_misfiled_action_items.assert_called_once_with( + "incident-team", MISFILED_ACTION_ITEM_REMINDER_MARKER + ) + args, kwargs = service.create_comment.call_args + assert args[0] == ISSUE["id"] + assert "@[Leander Rodrigues](creator-id)" in args[1] + assert MISFILED_ACTION_ITEM_REMINDER_MARKER in args[1] + assert "created in the INC team" in args[1] + assert "move it to the relevant actioning team" in args[1] + assert "keeping it as a child" in args[1] + assert UUID(kwargs["comment_id"]).version == 4 + + +@pytest.mark.parametrize("issues", [[], [ISSUE]]) +def test_logs_candidate_count(service, caplog, issues): + service.get_misfiled_action_items.return_value = issues + + with caplog.at_level("INFO", logger="firetower.incidents.tasks.action_items"): + _remind_misfiled_action_item_creators(LINEAR) + + assert ( + f"Found {len(issues)} misfiled action items without a team reminder " + "in Linear team incident-team" + ) in caplog.text + + +@pytest.mark.parametrize("success", [True, False]) +def test_logs_posted_reminder_only_on_success(service, caplog, success): + service.create_comment.return_value = success + + with caplog.at_level("INFO", logger="firetower.incidents.tasks.action_items"): + _remind_misfiled_action_item_creators(LINEAR) + + assert ("Posted team reminder for action item issue-1" in caplog.text) is success + + +def test_skips_issues_with_existing_reminder_on_later_runs(service): + service.get_misfiled_action_items.side_effect = [[ISSUE], []] + + _remind_misfiled_action_item_creators(LINEAR) + _remind_misfiled_action_item_creators(LINEAR) + + service.create_comment.assert_called_once() + + +def test_comment_id_is_stable_across_retries(service): + service.create_comment.side_effect = [TimeoutError("response lost"), True] + + _remind_misfiled_action_item_creators(LINEAR) + _remind_misfiled_action_item_creators(LINEAR) + + calls = service.create_comment.call_args_list + assert len(calls) == 2 + assert calls[0].kwargs["comment_id"] == calls[1].kwargs["comment_id"] + + +def test_comment_ids_differ_between_issues(service): + service.get_misfiled_action_items.return_value = [ + ISSUE, + {**ISSUE, "id": "issue-2", "identifier": "INC-2509"}, + ] + + _remind_misfiled_action_item_creators(LINEAR) + + calls = service.create_comment.call_args_list + assert calls[0].kwargs["comment_id"] != calls[1].kwargs["comment_id"] + + +def test_handles_missing_creator(service): + service.get_misfiled_action_items.return_value = [{**ISSUE, "creator": None}] + + _remind_misfiled_action_item_creators(LINEAR) + + body = service.create_comment.call_args.args[1] + assert "Hi, this action item" in body + assert "@[" not in body + + +@pytest.mark.parametrize("config", [{}, {"TEAM_ID": ""}]) +def test_skips_when_team_is_not_configured(service, config): + _remind_misfiled_action_item_creators({**config, "SYNC_IDENTIFIERS": True}) + + service.get_misfiled_action_items.assert_not_called() + service.create_comment.assert_not_called() + + +@pytest.mark.parametrize("config", [{}, {"SYNC_IDENTIFIERS": False}]) +def test_skips_when_identifier_sync_is_disabled(service, config): + with patch("firetower.incidents.tasks.action_items.LinearService") as client: + _remind_misfiled_action_item_creators({"TEAM_ID": "incident-team", **config}) + + client.assert_not_called() + service.get_misfiled_action_items.assert_not_called() + service.create_comment.assert_not_called() + + +@pytest.mark.parametrize("failure", [None, RuntimeError("Linear unavailable")]) +def test_does_not_post_when_lookup_fails(service, failure, caplog): + if isinstance(failure, Exception): + service.get_misfiled_action_items.side_effect = failure + else: + service.get_misfiled_action_items.return_value = failure + + with caplog.at_level("INFO", logger="firetower.incidents.tasks.action_items"): + _remind_misfiled_action_item_creators(LINEAR) + + service.create_comment.assert_not_called() + assert "Failed to fetch misfiled action items from Linear" in caplog.text + assert all(record.levelname == "INFO" for record in caplog.records) + + +@pytest.mark.parametrize("failure", [False, RuntimeError("Linear unavailable")]) +def test_continues_after_a_comment_failure(service, failure, caplog): + service.get_misfiled_action_items.return_value = [ + ISSUE, + {**ISSUE, "id": "issue-2", "identifier": "INC-2509"}, + ] + service.create_comment.side_effect = [failure, True] + + with caplog.at_level("INFO", logger="firetower.incidents.tasks.action_items"): + _remind_misfiled_action_item_creators(LINEAR) + + assert service.create_comment.call_count == 2 + assert "Failed to post team reminder for action item issue-1" in caplog.text + assert all(record.levelname == "INFO" for record in caplog.records) + + +@pytest.mark.parametrize( + "malformed", + [ + None, + {}, + {**ISSUE, "creator": {"id": "creator-id"}}, + {**ISSUE, "creator": {"displayName": "Leander Rodrigues"}}, + ], +) +def test_continues_after_malformed_issue_data(service, malformed): + service.get_misfiled_action_items.return_value = [malformed, ISSUE] + + _remind_misfiled_action_item_creators(LINEAR) + + service.create_comment.assert_called_once() + assert service.create_comment.call_args.args[0] == ISSUE["id"] + + +def test_initialization_failure_does_not_escape(service): + with patch( + "firetower.incidents.tasks.action_items.LinearService", + side_effect=RuntimeError("initialization failed"), + ): + _remind_misfiled_action_item_creators(LINEAR) + + service.create_comment.assert_not_called() + + +def test_malformed_issue_does_not_block_slo_sweep(service, settings): + settings.LINEAR = {**LINEAR, "ACTION_ITEM_NAG_COMMENT_HIGH_PRIORITY": "SLO nag"} + service.get_misfiled_action_items.return_value = [ + {**ISSUE, "creator": {"id": "creator-id"}} + ] + incident = MagicMock() + incident.action_items.filter.return_value = [] + with ( + patch("firetower.incidents.tasks.action_items.Incident") as incident_model, + patch( + "firetower.incidents.tasks.action_items.sync_action_items_from_linear" + ) as sync, + ): + incident_model.objects.filter.return_value.exclude.return_value = [incident] + + send_action_item_reminder() + + sync.assert_called_once_with(incident) + service.create_comment.assert_not_called() + + +def test_uses_configured_incident_team_key(service, settings): + settings.PROJECT_KEY = "TESTINC" + + _remind_misfiled_action_item_creators(LINEAR) + + assert "created in the TESTINC team" in service.create_comment.call_args.args[1] + + +def test_cron_sweeps_even_without_slo_nag_configuration(service): + send_action_item_reminder() + + service.create_comment.assert_called_once() diff --git a/src/firetower/incidents/tests/test_tasks.py b/src/firetower/incidents/tests/test_tasks.py index 13c2140b..f9f53ed2 100644 --- a/src/firetower/incidents/tests/test_tasks.py +++ b/src/firetower/incidents/tests/test_tasks.py @@ -838,6 +838,13 @@ def _configure_linear(self): with patch.object(settings, "LINEAR", linear_settings): yield + @pytest.fixture(autouse=True) + def mock_team_reminders(self): + with patch( + "firetower.incidents.tasks.action_items._remind_misfiled_action_item_creators" + ) as mock: + yield mock + @pytest.fixture(autouse=True) def mock_sync(self): with patch( @@ -902,6 +909,19 @@ def _make_action_item(self, incident: Incident, **kwargs) -> ActionItem: defaults.update(kwargs) return ActionItem.objects.create(incident=incident, **defaults) + def test_sends_slo_nags_before_team_reminders( + self, mock_linear, mock_team_reminders + ): + incident = self._make_incident() + self._make_action_item(incident) + calls = MagicMock() + calls.attach_mock(mock_linear.create_comment, "slo_comment") + calls.attach_mock(mock_team_reminders, "team_reminders") + + send_action_item_reminder() + + assert [c[0] for c in calls.mock_calls] == ["slo_comment", "team_reminders"] + def test_processes_incident_in_age_window(self, mock_sync): incident = self._make_incident(days_old=45) send_action_item_reminder() diff --git a/src/firetower/integrations/services/linear.py b/src/firetower/integrations/services/linear.py index d980a10d..74ccf0f5 100644 --- a/src/firetower/integrations/services/linear.py +++ b/src/firetower/integrations/services/linear.py @@ -545,7 +545,65 @@ def create_attachment(self, issue_id: str, url: str, title: str) -> bool: return data.get("attachmentCreate", {}).get("success", False) - def create_comment(self, issue_id: str, body: str) -> bool: + def get_misfiled_action_items( + self, team_id: str, reminder_marker: str + ) -> list[dict[str, Any]] | None: + """Find unnotified child issues in the incident team, in any status.""" + query = """ + query($filter: IssueFilter!, $after: String) { + issues( + filter: $filter, includeArchived: true, first: 50, after: $after + ) { + nodes { + id + identifier + creator { + id + displayName + } + } + pageInfo { + hasNextPage + endCursor + } + } + } + """ + issue_filter = { + "team": {"id": {"eq": team_id}}, + "number": {"gte": 2000}, + "parent": {"team": {"id": {"eq": team_id}}}, + "or": [ + {"comments": {"length": {"eq": 0}}}, + {"comments": {"every": {"body": {"notContains": reminder_marker}}}}, + ], + } + issues: list[dict[str, Any]] = [] + cursor: str | None = None + for _ in range(25): + data = self._graphql(query, {"filter": issue_filter, "after": cursor}) + if not data or not data.get("issues"): + return None + page = data["issues"] + issues.extend(page["nodes"]) + if not page["pageInfo"]["hasNextPage"]: + return issues + next_cursor = page["pageInfo"]["endCursor"] + if not next_cursor or next_cursor == cursor: + logger.info("Incomplete Linear misfiled action item pagination") + return None + cursor = next_cursor + + logger.info( + "Reached Linear misfiled action item page limit; returning %d issues " + "and leaving the rest for the next run", + len(issues), + ) + return issues + + def create_comment( + self, issue_id: str, body: str, *, comment_id: str | None = None + ) -> bool: mutation = """ mutation($input: CommentCreateInput!) { commentCreate(input: $input) { @@ -553,11 +611,10 @@ def create_comment(self, issue_id: str, body: str) -> bool: } } """ - data = self._graphql( - mutation, - {"input": {"issueId": issue_id, "body": body}}, - retryable=False, - ) + input_data = {"issueId": issue_id, "body": body} + if comment_id is not None: + input_data["id"] = comment_id + data = self._graphql(mutation, {"input": input_data}, retryable=False) if not data: return False diff --git a/src/firetower/integrations/tests/test_linear_team_reminders.py b/src/firetower/integrations/tests/test_linear_team_reminders.py new file mode 100644 index 00000000..be922e33 --- /dev/null +++ b/src/firetower/integrations/tests/test_linear_team_reminders.py @@ -0,0 +1,134 @@ +from unittest.mock import patch + +import pytest + +from firetower.integrations.services.linear import LinearService + + +@pytest.fixture +def service(settings): + settings.LINEAR = {"API_KEY": "test"} + return LinearService() + + +def page(issues, has_next=False, cursor=None): + return { + "issues": { + "nodes": issues, + "pageInfo": {"hasNextPage": has_next, "endCursor": cursor}, + } + } + + +def test_filters_by_child_team_parent_team_number_and_existing_comments(service): + with patch.object(service, "_graphql", return_value=page([])) as graphql: + assert service.get_misfiled_action_items("team-id", "reminder-marker") == [] + + query, variables = graphql.call_args.args + assert variables == { + "filter": { + "team": {"id": {"eq": "team-id"}}, + "number": {"gte": 2000}, + "parent": {"team": {"id": {"eq": "team-id"}}}, + "or": [ + {"comments": {"length": {"eq": 0}}}, + {"comments": {"every": {"body": {"notContains": "reminder-marker"}}}}, + ], + }, + "after": None, + } + assert "includeArchived: true" in query + assert "creator {" in query + assert "displayName" in query + + +def test_paginates_all_candidates_before_posting(service): + first = {"id": "issue-1", "creator": None} + second = {"id": "issue-2", "creator": {"id": "user-1", "displayName": "Name"}} + with patch.object( + service, + "_graphql", + side_effect=[page([first], True, "cursor-1"), page([second])], + ) as graphql: + assert service.get_misfiled_action_items("team-id", "marker") == [first, second] + + assert graphql.call_args_list[1].args[1]["after"] == "cursor-1" + + +@pytest.mark.parametrize("response", [None, {}, {"issues": None}]) +def test_returns_none_when_query_fails(service, response): + with patch.object(service, "_graphql", return_value=response): + assert service.get_misfiled_action_items("team-id", "marker") is None + + +def test_does_not_return_partial_results_on_failure(service): + with patch.object( + service, + "_graphql", + side_effect=[page([{"id": "issue-1"}], True, "cursor-1"), None], + ): + assert service.get_misfiled_action_items("team-id", "marker") is None + + +@pytest.mark.parametrize("cursor", [None, "cursor-1"]) +def test_fails_closed_when_pagination_cannot_advance(service, cursor, caplog): + with ( + patch.object( + service, + "_graphql", + side_effect=[page([], True, "cursor-1"), page([], True, cursor)], + ), + caplog.at_level("INFO", logger="firetower.integrations.services.linear"), + ): + assert service.get_misfiled_action_items("team-id", "marker") is None + + assert "Incomplete Linear misfiled action item pagination" in caplog.text + assert all(record.levelname == "INFO" for record in caplog.records) + + +def test_returns_fetched_batch_at_page_limit(service): + issues = [{"id": f"issue-{i}"} for i in range(1250)] + with patch.object( + service, + "_graphql", + side_effect=[ + page(issues[i * 50 : (i + 1) * 50], True, f"cursor-{i}") for i in range(25) + ], + ) as graphql: + assert service.get_misfiled_action_items("team-id", "marker") == issues + + assert graphql.call_count == 25 + + +def test_starts_a_fresh_sweep_on_next_call(service): + first_batch = [{"id": f"issue-{i}"} for i in range(25)] + remaining = [{"id": "remaining-issue"}] + with patch.object( + service, + "_graphql", + side_effect=[ + *[ + page([issue], True, f"cursor-{i}") + for i, issue in enumerate(first_batch) + ], + page(remaining), + ], + ) as graphql: + assert service.get_misfiled_action_items("team-id", "marker") == first_batch + assert service.get_misfiled_action_items("team-id", "marker") == remaining + + assert graphql.call_args_list[25].args[1]["after"] is None + + +@pytest.mark.parametrize("comment_id", [None, "stable-comment-id"]) +def test_passes_optional_comment_id_without_retrying_mutation(service, comment_id): + with patch.object( + service, "_graphql", return_value={"commentCreate": {"success": True}} + ) as graphql: + assert service.create_comment("issue-1", "body", comment_id=comment_id) + + expected_input = {"issueId": "issue-1", "body": "body"} + if comment_id is not None: + expected_input["id"] = comment_id + assert graphql.call_args.args[1] == {"input": expected_input} + assert graphql.call_args.kwargs == {"retryable": False}