From 0aead954ee10b1fff3ca958b2e9faa76d5a600b6 Mon Sep 17 00:00:00 2001 From: Spencer Murray Date: Tue, 30 Jun 2026 15:25:12 -0400 Subject: [PATCH 1/3] fix(linear): track only sub-issues as action items and re-enable parent reopen Only child (sub-)issues of the Linear parent are synced as action items; related/blocking/duplicate relations are no longer treated as action items. This removes the source of confusion behind #261, where loosely related issues dragged the parent's auto-status around. With action items now scoped to true sub-issues, re-enable the two-way parent status sync (revert #261's one-way change): the parent advances to Completed when the incident is resolved and all sub-issues are done, and reopens to Started when the incident is reopened or sub-issues remain. Restores the Started status-change comment and its config key. Captain-change assignee sync from #261 is left untouched. --- src/firetower/config.py | 6 + src/firetower/incidents/hooks.py | 8 +- src/firetower/incidents/services.py | 36 +-- .../incidents/tests/test_action_items.py | 287 +----------------- .../incidents/tests/test_services.py | 172 ++++++----- src/firetower/integrations/services/linear.py | 85 ------ .../integrations/tests/test_linear_service.py | 92 ------ src/firetower/settings.py | 1 + 8 files changed, 143 insertions(+), 544 deletions(-) diff --git a/src/firetower/config.py b/src/firetower/config.py index ea36fab5..59280e55 100644 --- a/src/firetower/config.py +++ b/src/firetower/config.py @@ -100,6 +100,12 @@ class LinearConfig: "{% else %}all {{ total_action_items }} action " "item{% if total_action_items != 1 %}s{% endif %} are complete.{% endif %}" ) + parent_status_comment_started: str = ( + "Firetower set this issue to **Started**. " + "Incident {{ incident.incident_number }} is {{ incident.status }}. " + "{{ completed_action_items }} of {{ total_action_items }} action " + "item{% if total_action_items != 1 %}s{% endif %} complete." + ) @deserialize diff --git a/src/firetower/incidents/hooks.py b/src/firetower/incidents/hooks.py index a2db11c0..5cd15f9b 100644 --- a/src/firetower/incidents/hooks.py +++ b/src/firetower/incidents/hooks.py @@ -998,11 +998,11 @@ def _sync_linear_assignee(incident: Incident) -> None: LINEAR_PARENT_DESCRIPTION = ( - "Relate action items to this ticket to have them tracked by Firetower. " - "Child issues or other relations (related, blocking, etc.) will all work. " + "Add action items as sub-issues (child issues) of this ticket to have " + "them tracked by Firetower. " "Do not update title, status or captain here, use Firetower for that.\n\n" - "Firetower will mark this ticket completed once the incident is resolved and " - "all action items are done. " + "Firetower will reopen this ticket if the incident is reopened, or if " + "there are still unfinished action items. " "If you have questions, please reach out to #team-sre." ) diff --git a/src/firetower/incidents/services.py b/src/firetower/incidents/services.py index f3767d3b..8956087d 100644 --- a/src/firetower/incidents/services.py +++ b/src/firetower/incidents/services.py @@ -189,15 +189,21 @@ def _resolve_assignees( _PARENT_STATUS_TEMPLATE_ENV = Environment(autoescape=False) -def _comment_parent_issue_completed( +def _comment_parent_issue_status_change( incident: Incident, linear_service: LinearService, + target_state: str, statuses: list[str], ) -> None: if not settings.LINEAR or not incident.linear_parent_issue_id: return - template_source = settings.LINEAR.get("PARENT_STATUS_COMMENT_COMPLETED", "") + template_key = ( + "PARENT_STATUS_COMMENT_COMPLETED" + if target_state == "completed" + else "PARENT_STATUS_COMMENT_STARTED" + ) + template_source = settings.LINEAR.get(template_key, "") if not template_source or not template_source.strip(): return @@ -209,6 +215,7 @@ def _comment_parent_issue_completed( incident=incident, total_action_items=len(statuses), completed_action_items=completed_action_items, + target_state=target_state, ) except TemplateError: logger.exception( @@ -239,22 +246,23 @@ def _update_parent_issue_status( not statuses or all(s in COMPLETED_STATUSES for s in statuses) ) - if not all_complete: - return - states = linear_service.get_workflow_states(team_id) if not states: return + target_state = "completed" if all_complete else "started" + parent_issue = linear_service.get_issue(incident.linear_parent_issue_id) - if not parent_issue or parent_issue.get("state_type") == "completed": + if not parent_issue or parent_issue.get("state_type") == target_state: return - state_id = states.get("completed") + state_id = states.get(target_state) if state_id and linear_service.update_issue( incident.linear_parent_issue_id, state_id=state_id ): - _comment_parent_issue_completed(incident, linear_service, statuses) + _comment_parent_issue_status_change( + incident, linear_service, target_state, statuses + ) def sync_action_items_from_linear( @@ -297,21 +305,9 @@ def sync_action_items_from_linear( incident.save(update_fields=["action_items_last_synced_at"]) return stats - related = linear_service.get_related_issues(parent_id) - if related is None: - error_msg = f"Failed to fetch related issues for incident {incident.id}" - logger.warning(error_msg) - stats.errors.append(error_msg) - incident.action_items_last_synced_at = timezone.now() - incident.save(update_fields=["action_items_last_synced_at"]) - return stats - all_issues: dict[str, dict] = {} for issue in children: all_issues[issue["id"]] = issue - for issue in related: - if issue["id"] not in all_issues: - all_issues[issue["id"]] = issue logger.info(f"Syncing {len(all_issues)} Linear issues to incident {incident.id}") diff --git a/src/firetower/incidents/tests/test_action_items.py b/src/firetower/incidents/tests/test_action_items.py index d504ee36..6f36ba66 100644 --- a/src/firetower/incidents/tests/test_action_items.py +++ b/src/firetower/incidents/tests/test_action_items.py @@ -88,7 +88,6 @@ def test_creates_action_items_from_children(self): with patch("firetower.incidents.services._get_linear_service") as mock_get: mock_get.return_value.get_child_issues.return_value = children - mock_get.return_value.get_related_issues.return_value = [] mock_get.return_value.get_workflow_states.return_value = None stats = sync_action_items_from_linear(incident, force=True) @@ -99,61 +98,25 @@ def test_creates_action_items_from_children(self): assert incident.action_items.count() == 2 assert incident.action_items_last_synced_at is not None - def test_creates_action_items_from_relations(self): - incident = self._make_incident() - - related = [ - _make_linear_issue( - id="id-r1", - identifier="ENG-10", - title="Related task", - relation_type="related", - ), - _make_linear_issue( - id="id-r2", - identifier="ENG-11", - title="Blocking task", - relation_type="blocks", - ), - ] - - with patch("firetower.incidents.services._get_linear_service") as mock_get: - mock_get.return_value.get_child_issues.return_value = [] - mock_get.return_value.get_related_issues.return_value = related - mock_get.return_value.get_workflow_states.return_value = None - - stats = sync_action_items_from_linear(incident, force=True) - - assert stats.created == 2 - item_r1 = incident.action_items.get(linear_issue_id="id-r1") - assert item_r1.relation_type == ActionItemRelationType.RELATED - item_r2 = incident.action_items.get(linear_issue_id="id-r2") - assert item_r2.relation_type == ActionItemRelationType.BLOCKS - - def test_combines_children_and_relations(self): + def test_ignores_related_issues(self): incident = self._make_incident() children = [ _make_linear_issue(id="id-1", identifier="ENG-1", title="Child task"), ] - related = [ - _make_linear_issue( - id="id-2", - identifier="ENG-2", - title="Related task", - relation_type="related", - ), - ] with patch("firetower.incidents.services._get_linear_service") as mock_get: - mock_get.return_value.get_child_issues.return_value = children - mock_get.return_value.get_related_issues.return_value = related - mock_get.return_value.get_workflow_states.return_value = None + mock_service = mock_get.return_value + mock_service.get_child_issues.return_value = children + mock_service.get_workflow_states.return_value = None stats = sync_action_items_from_linear(incident, force=True) - assert stats.created == 2 - assert incident.action_items.count() == 2 + assert stats.created == 1 + assert incident.action_items.count() == 1 + item = incident.action_items.get(linear_issue_id="id-1") + assert item.relation_type == ActionItemRelationType.CHILD + mock_service.get_related_issues.assert_not_called() def test_same_issue_on_multiple_incidents(self): incident_a = self._make_incident(linear_parent_issue_id="parent-a") @@ -165,7 +128,6 @@ def test_same_issue_on_multiple_incidents(self): with patch("firetower.incidents.services._get_linear_service") as mock_get: mock_get.return_value.get_child_issues.return_value = shared_issue - mock_get.return_value.get_related_issues.return_value = [] mock_get.return_value.get_workflow_states.return_value = None sync_action_items_from_linear(incident_a, force=True) @@ -175,32 +137,6 @@ def test_same_issue_on_multiple_incidents(self): assert incident_b.action_items.filter(linear_issue_id="shared-1").exists() assert ActionItem.objects.filter(linear_issue_id="shared-1").count() == 2 - def test_deduplicates_children_and_relations(self): - incident = self._make_incident() - - children = [ - _make_linear_issue(id="id-1", identifier="ENG-1", title="Task"), - ] - related = [ - _make_linear_issue( - id="id-1", - identifier="ENG-1", - title="Task", - relation_type="related", - ), - ] - - with patch("firetower.incidents.services._get_linear_service") as mock_get: - mock_get.return_value.get_child_issues.return_value = children - mock_get.return_value.get_related_issues.return_value = related - mock_get.return_value.get_workflow_states.return_value = None - - stats = sync_action_items_from_linear(incident, force=True) - - assert stats.created == 1 - item = incident.action_items.get(linear_issue_id="id-1") - assert item.relation_type == ActionItemRelationType.CHILD - def test_updates_existing_action_items(self): incident = self._make_incident() ActionItem.objects.create( @@ -223,7 +159,6 @@ def test_updates_existing_action_items(self): with patch("firetower.incidents.services._get_linear_service") as mock_get: mock_get.return_value.get_child_issues.return_value = children - mock_get.return_value.get_related_issues.return_value = [] mock_get.return_value.get_workflow_states.return_value = None stats = sync_action_items_from_linear(incident, force=True) @@ -251,7 +186,6 @@ def test_deletes_stale_action_items(self): with patch("firetower.incidents.services._get_linear_service") as mock_get: mock_get.return_value.get_child_issues.return_value = children - mock_get.return_value.get_related_issues.return_value = [] mock_get.return_value.get_workflow_states.return_value = None stats = sync_action_items_from_linear(incident, force=True) @@ -277,7 +211,6 @@ def test_force_bypasses_throttle(self): with patch("firetower.incidents.services._get_linear_service") as mock_get: mock_get.return_value.get_child_issues.return_value = [] - mock_get.return_value.get_related_issues.return_value = [] mock_get.return_value.get_workflow_states.return_value = None stats = sync_action_items_from_linear(incident, force=True) @@ -298,20 +231,6 @@ def test_handles_children_api_failure(self): incident.refresh_from_db() assert incident.action_items_last_synced_at is not None - def test_handles_relations_api_failure(self): - incident = self._make_incident() - - with patch("firetower.incidents.services._get_linear_service") as mock_get: - mock_get.return_value.get_child_issues.return_value = [] - mock_get.return_value.get_related_issues.return_value = None - - stats = sync_action_items_from_linear(incident, force=True) - - assert len(stats.errors) == 1 - assert "related issues" in stats.errors[0] - incident.refresh_from_db() - assert incident.action_items_last_synced_at is not None - def test_resolves_assignee_by_email(self): user = User.objects.create_user( username="dev@example.com", @@ -332,7 +251,6 @@ def test_resolves_assignee_by_email(self): with patch("firetower.incidents.services._get_linear_service") as mock_get: mock_get.return_value.get_child_issues.return_value = children - mock_get.return_value.get_related_issues.return_value = [] mock_get.return_value.get_workflow_states.return_value = None sync_action_items_from_linear(incident, force=True) @@ -359,7 +277,6 @@ def test_creates_user_for_unknown_assignee_email(self): with patch("firetower.incidents.services._get_linear_service") as mock_get: mock_get.return_value.get_child_issues.return_value = children - mock_get.return_value.get_related_issues.return_value = [] mock_get.return_value.get_workflow_states.return_value = None sync_action_items_from_linear(incident, force=True) @@ -396,7 +313,6 @@ def fake_create_parent(inc): patch("firetower.incidents.services._get_linear_service") as mock_get, ): mock_get.return_value.get_child_issues.return_value = children - mock_get.return_value.get_related_issues.return_value = [] mock_get.return_value.get_workflow_states.return_value = None stats = sync_action_items_from_linear(incident, force=True) @@ -422,7 +338,6 @@ def test_auto_completes_parent_when_all_done(self, settings): with patch("firetower.incidents.services._get_linear_service") as mock_get: mock_service = mock_get.return_value mock_service.get_child_issues.return_value = children - mock_service.get_related_issues.return_value = [] mock_service.get_workflow_states.return_value = { "completed": "state-done", "backlog": "state-backlog", @@ -435,7 +350,7 @@ def test_auto_completes_parent_when_all_done(self, settings): "parent-issue-id", state_id="state-done" ) - def test_does_not_change_parent_state_when_incomplete_items(self, settings): + def test_sets_parent_to_started_when_incomplete_items(self, settings): settings.LINEAR = {"TEAM_ID": "team-1"} incident = self._make_incident(status=IncidentStatus.DONE) @@ -451,7 +366,6 @@ def test_does_not_change_parent_state_when_incomplete_items(self, settings): with patch("firetower.incidents.services._get_linear_service") as mock_get: mock_service = mock_get.return_value mock_service.get_child_issues.return_value = children - mock_service.get_related_issues.return_value = [] mock_service.get_workflow_states.return_value = { "completed": "state-done", "started": "state-started", @@ -460,8 +374,9 @@ def test_does_not_change_parent_state_when_incomplete_items(self, settings): sync_action_items_from_linear(incident, force=True) - for call in mock_service.update_issue.call_args_list: - assert "state_id" not in call.kwargs + mock_service.update_issue.assert_any_call( + "parent-issue-id", state_id="state-started" + ) def test_completes_parent_when_no_action_items(self, settings): settings.LINEAR = {"TEAM_ID": "team-1"} @@ -470,7 +385,6 @@ def test_completes_parent_when_no_action_items(self, settings): with patch("firetower.incidents.services._get_linear_service") as mock_get: mock_service = mock_get.return_value mock_service.get_child_issues.return_value = [] - mock_service.get_related_issues.return_value = [] mock_service.get_workflow_states.return_value = { "completed": "state-done", "backlog": "state-backlog", @@ -499,7 +413,6 @@ def test_does_not_push_parent_assignee_on_sync(self, settings): with patch("firetower.incidents.services._get_linear_service") as mock_get: mock_service = mock_get.return_value mock_service.get_child_issues.return_value = [] - mock_service.get_related_issues.return_value = [] mock_service.get_workflow_states.return_value = None mock_service.update_issue.return_value = True @@ -626,180 +539,6 @@ def test_get_child_issues_maps_state_types(self): assert issues[1]["status"] == "Done" assert issues[1]["assignee_email"] == "dev@example.com" - def test_get_related_issues_maps_relation_types(self): - forward_response = { - "issue": { - "relations": { - "nodes": [ - { - "type": "related", - "relatedIssue": { - "id": "id-1", - "identifier": "ENG-1", - "title": "Related task", - "url": "https://linear.app/t/ENG-1", - "state": {"type": "unstarted"}, - "assignee": None, - }, - }, - { - "type": "blocks", - "relatedIssue": { - "id": "id-2", - "identifier": "ENG-2", - "title": "Blocking task", - "url": "https://linear.app/t/ENG-2", - "state": {"type": "started"}, - "assignee": None, - }, - }, - ], - "pageInfo": {"hasNextPage": False, "endCursor": None}, - } - } - } - inverse_response = { - "issue": { - "inverseRelations": { - "nodes": [], - "pageInfo": {"hasNextPage": False, "endCursor": None}, - } - } - } - - with patch("firetower.integrations.services.linear.settings") as mock_settings: - mock_settings.LINEAR = { - "CLIENT_ID": "test-id", - "CLIENT_SECRET": "test-secret", - } - service = LinearService() - - with patch.object( - service, "_graphql", side_effect=[forward_response, inverse_response] - ): - issues = service.get_related_issues("parent-id") - - assert issues is not None - assert len(issues) == 2 - assert issues[0]["relation_type"] == "related" - assert issues[1]["relation_type"] == "blocks" - - def test_get_related_issues_includes_inverse_relations(self): - forward_response = { - "issue": { - "relations": { - "nodes": [ - { - "type": "related", - "relatedIssue": { - "id": "id-1", - "identifier": "ENG-1", - "title": "Forward relation", - "url": "https://linear.app/t/ENG-1", - "state": {"type": "unstarted"}, - "assignee": None, - }, - }, - ], - "pageInfo": {"hasNextPage": False, "endCursor": None}, - } - } - } - inverse_response = { - "issue": { - "inverseRelations": { - "nodes": [ - { - "type": "related", - "issue": { - "id": "id-2", - "identifier": "INF-100", - "title": "Inverse relation", - "url": "https://linear.app/t/INF-100", - "state": {"type": "started"}, - "assignee": None, - }, - }, - ], - "pageInfo": {"hasNextPage": False, "endCursor": None}, - } - } - } - - with patch("firetower.integrations.services.linear.settings") as mock_settings: - mock_settings.LINEAR = { - "CLIENT_ID": "test-id", - "CLIENT_SECRET": "test-secret", - } - service = LinearService() - - with patch.object( - service, "_graphql", side_effect=[forward_response, inverse_response] - ): - issues = service.get_related_issues("parent-id") - - assert issues is not None - assert len(issues) == 2 - assert issues[0]["identifier"] == "ENG-1" - assert issues[1]["identifier"] == "INF-100" - - def test_get_related_issues_deduplicates_across_directions(self): - forward_response = { - "issue": { - "relations": { - "nodes": [ - { - "type": "related", - "relatedIssue": { - "id": "id-1", - "identifier": "ENG-1", - "title": "Same issue", - "url": "https://linear.app/t/ENG-1", - "state": {"type": "unstarted"}, - "assignee": None, - }, - }, - ], - "pageInfo": {"hasNextPage": False, "endCursor": None}, - } - } - } - inverse_response = { - "issue": { - "inverseRelations": { - "nodes": [ - { - "type": "related", - "issue": { - "id": "id-1", - "identifier": "ENG-1", - "title": "Same issue", - "url": "https://linear.app/t/ENG-1", - "state": {"type": "unstarted"}, - "assignee": None, - }, - }, - ], - "pageInfo": {"hasNextPage": False, "endCursor": None}, - } - } - } - - with patch("firetower.integrations.services.linear.settings") as mock_settings: - mock_settings.LINEAR = { - "CLIENT_ID": "test-id", - "CLIENT_SECRET": "test-secret", - } - service = LinearService() - - with patch.object( - service, "_graphql", side_effect=[forward_response, inverse_response] - ): - issues = service.get_related_issues("parent-id") - - assert issues is not None - assert len(issues) == 1 - def test_update_issue(self): with patch("firetower.integrations.services.linear.settings") as mock_settings: mock_settings.LINEAR = { diff --git a/src/firetower/incidents/tests/test_services.py b/src/firetower/incidents/tests/test_services.py index 27291dea..97c70042 100644 --- a/src/firetower/incidents/tests/test_services.py +++ b/src/firetower/incidents/tests/test_services.py @@ -16,7 +16,7 @@ IncidentStatus, ) from firetower.incidents.services import ( - _comment_parent_issue_completed, + _comment_parent_issue_status_change, _update_parent_issue_status, sync_incident_participants_from_slack, ) @@ -441,60 +441,62 @@ def _linear_settings(self, settings): "TEAM_ID": "team-1", "API_KEY": "key", "PARENT_STATUS_COMMENT_COMPLETED": "completed comment", + "PARENT_STATUS_COMMENT_STARTED": "started comment", } - def _add_item(self, incident, status, suffix="1"): - return ActionItem.objects.create( - incident=incident, - linear_issue_id=f"li-{suffix}", - linear_identifier=f"INC-{suffix}", - title=f"Item {suffix}", - status=status, - url=f"https://linear.app/issue/{suffix}", - ) - - def test_active_incident_no_action_items_does_nothing(self): + def test_active_incident_no_action_items_sets_started(self): incident = self._make_incident(status=IncidentStatus.ACTIVE) svc = self._make_linear_service() _update_parent_issue_status(incident, svc) - svc.update_issue.assert_not_called() - svc.create_comment.assert_not_called() + svc.update_issue.assert_called_once_with("lin-123", state_id="state-started") + svc.create_comment.assert_called_once() - def test_active_incident_all_items_done_does_nothing(self): + def test_active_incident_all_items_done_sets_started(self): incident = self._make_incident(status=IncidentStatus.ACTIVE) - self._add_item(incident, ActionItemStatus.DONE) - svc = self._make_linear_service() - - _update_parent_issue_status(incident, svc) - - svc.update_issue.assert_not_called() - svc.create_comment.assert_not_called() - - def test_mitigated_incident_all_items_done_does_nothing(self): - incident = self._make_incident(status=IncidentStatus.MITIGATED) - self._add_item(incident, ActionItemStatus.DONE) + ActionItem.objects.create( + incident=incident, + linear_issue_id="li-1", + linear_identifier="INC-1", + title="Item 1", + status=ActionItemStatus.DONE, + url="https://linear.app/issue/1", + ) svc = self._make_linear_service() _update_parent_issue_status(incident, svc) - svc.update_issue.assert_not_called() - svc.create_comment.assert_not_called() + svc.update_issue.assert_called_once_with("lin-123", state_id="state-started") + svc.create_comment.assert_called_once() - def test_done_incident_incomplete_items_does_nothing(self): + def test_done_incident_no_action_items_sets_completed(self): incident = self._make_incident(status=IncidentStatus.DONE) - self._add_item(incident, ActionItemStatus.DONE, "1") - self._add_item(incident, ActionItemStatus.IN_PROGRESS, "2") svc = self._make_linear_service() _update_parent_issue_status(incident, svc) - svc.update_issue.assert_not_called() - svc.create_comment.assert_not_called() + svc.update_issue.assert_called_once_with("lin-123", state_id="state-completed") + svc.create_comment.assert_called_once() - def test_done_incident_no_action_items_sets_completed(self): + def test_done_incident_all_items_done_sets_completed(self): incident = self._make_incident(status=IncidentStatus.DONE) + ActionItem.objects.create( + incident=incident, + linear_issue_id="li-1", + linear_identifier="INC-1", + title="Item 1", + status=ActionItemStatus.DONE, + url="https://linear.app/issue/1", + ) + ActionItem.objects.create( + incident=incident, + linear_issue_id="li-2", + linear_identifier="INC-2", + title="Item 2", + status=ActionItemStatus.CANCELED, + url="https://linear.app/issue/2", + ) svc = self._make_linear_service() _update_parent_issue_status(incident, svc) @@ -502,25 +504,46 @@ def test_done_incident_no_action_items_sets_completed(self): svc.update_issue.assert_called_once_with("lin-123", state_id="state-completed") svc.create_comment.assert_called_once() - def test_done_incident_all_items_done_sets_completed(self): + def test_done_incident_incomplete_items_sets_started(self): incident = self._make_incident(status=IncidentStatus.DONE) - self._add_item(incident, ActionItemStatus.DONE, "1") - self._add_item(incident, ActionItemStatus.CANCELED, "2") + ActionItem.objects.create( + incident=incident, + linear_issue_id="li-1", + linear_identifier="INC-1", + title="Item 1", + status=ActionItemStatus.DONE, + url="https://linear.app/issue/1", + ) + ActionItem.objects.create( + incident=incident, + linear_issue_id="li-2", + linear_identifier="INC-2", + title="Item 2", + status=ActionItemStatus.IN_PROGRESS, + url="https://linear.app/issue/2", + ) svc = self._make_linear_service() _update_parent_issue_status(incident, svc) - svc.update_issue.assert_called_once_with("lin-123", state_id="state-completed") + svc.update_issue.assert_called_once_with("lin-123", state_id="state-started") svc.create_comment.assert_called_once() - def test_canceled_incident_all_items_done_sets_completed(self): - incident = self._make_incident(status=IncidentStatus.CANCELED) - self._add_item(incident, ActionItemStatus.DONE) + def test_mitigated_incident_all_items_done_sets_started(self): + incident = self._make_incident(status=IncidentStatus.MITIGATED) + ActionItem.objects.create( + incident=incident, + linear_issue_id="li-1", + linear_identifier="INC-1", + title="Item 1", + status=ActionItemStatus.DONE, + url="https://linear.app/issue/1", + ) svc = self._make_linear_service() _update_parent_issue_status(incident, svc) - svc.update_issue.assert_called_once_with("lin-123", state_id="state-completed") + svc.update_issue.assert_called_once_with("lin-123", state_id="state-started") svc.create_comment.assert_called_once() def test_update_issue_failure_skips_comment(self): @@ -533,9 +556,9 @@ def test_update_issue_failure_skips_comment(self): svc.update_issue.assert_called_once_with("lin-123", state_id="state-completed") svc.create_comment.assert_not_called() - def test_skips_update_when_already_completed(self): - incident = self._make_incident(status=IncidentStatus.DONE) - svc = self._make_linear_service(current_state_type="completed") + def test_skips_update_when_already_in_target_state(self): + incident = self._make_incident(status=IncidentStatus.ACTIVE) + svc = self._make_linear_service(current_state_type="started") _update_parent_issue_status(incident, svc) @@ -552,7 +575,7 @@ def test_updates_when_in_different_state(self): svc.create_comment.assert_called_once() def test_skips_update_when_get_issue_fails(self): - incident = self._make_incident(status=IncidentStatus.DONE) + incident = self._make_incident(status=IncidentStatus.ACTIVE) svc = self._make_linear_service() svc.get_issue.return_value = None @@ -561,21 +584,10 @@ def test_skips_update_when_get_issue_fails(self): svc.update_issue.assert_not_called() svc.create_comment.assert_not_called() - def test_never_reopens_when_item_reopens_after_completion(self): - incident = self._make_incident(status=IncidentStatus.DONE) - self._add_item(incident, ActionItemStatus.DONE, "1") - self._add_item(incident, ActionItemStatus.IN_PROGRESS, "2") - svc = self._make_linear_service(current_state_type="completed") - - _update_parent_issue_status(incident, svc) - - svc.update_issue.assert_not_called() - svc.create_comment.assert_not_called() - @pytest.mark.django_db -class TestCommentParentIssueCompleted: - def _make_incident(self, status=IncidentStatus.DONE): +class TestCommentParentIssueStatusChange: + def _make_incident(self, status=IncidentStatus.ACTIVE): return Incident.objects.create( title="Test Incident", status=status, @@ -593,24 +605,46 @@ def _linear_settings(self, settings): "Incident {{ incident.incident_number }} is {{ incident.status }}. " "{{ completed_action_items }}/{{ total_action_items }} done." ), + "PARENT_STATUS_COMMENT_STARTED": ( + "Set to Started. " + "Incident {{ incident.incident_number }} is {{ incident.status }}. " + "{{ completed_action_items }}/{{ total_action_items }} done." + ), } def test_posts_completed_comment(self): incident = self._make_incident(status=IncidentStatus.DONE) svc = MagicMock() - _comment_parent_issue_completed(incident, svc, ["Done", "Done"]) + _comment_parent_issue_status_change( + incident, svc, "completed", ["Done", "Done"] + ) svc.create_comment.assert_called_once_with( "lin-123", f"Set to Completed. Incident {incident.incident_number} is Done. 2/2 done.", ) + def test_posts_started_comment_with_mixed_statuses(self): + incident = self._make_incident(status=IncidentStatus.ACTIVE) + svc = MagicMock() + + _comment_parent_issue_status_change( + incident, svc, "started", ["Done", "In Progress", "Todo"] + ) + + svc.create_comment.assert_called_once_with( + "lin-123", + f"Set to Started. Incident {incident.incident_number} is Active. 1/3 done.", + ) + def test_counts_canceled_as_completed(self): incident = self._make_incident(status=IncidentStatus.DONE) svc = MagicMock() - _comment_parent_issue_completed(incident, svc, ["Done", "Canceled"]) + _comment_parent_issue_status_change( + incident, svc, "completed", ["Done", "Canceled"] + ) svc.create_comment.assert_called_once_with( "lin-123", @@ -622,16 +656,16 @@ def test_empty_template_skips_comment(self, settings): incident = self._make_incident(status=IncidentStatus.DONE) svc = MagicMock() - _comment_parent_issue_completed(incident, svc, ["Done"]) + _comment_parent_issue_status_change(incident, svc, "completed", ["Done"]) svc.create_comment.assert_not_called() def test_whitespace_only_template_skips_comment(self, settings): - settings.LINEAR["PARENT_STATUS_COMMENT_COMPLETED"] = " " - incident = self._make_incident(status=IncidentStatus.DONE) + settings.LINEAR["PARENT_STATUS_COMMENT_STARTED"] = " " + incident = self._make_incident(status=IncidentStatus.ACTIVE) svc = MagicMock() - _comment_parent_issue_completed(incident, svc, ["Done"]) + _comment_parent_issue_status_change(incident, svc, "started", ["Todo"]) svc.create_comment.assert_not_called() @@ -639,7 +673,7 @@ def test_no_action_items(self): incident = self._make_incident(status=IncidentStatus.DONE) svc = MagicMock() - _comment_parent_issue_completed(incident, svc, []) + _comment_parent_issue_status_change(incident, svc, "completed", []) svc.create_comment.assert_called_once_with( "lin-123", @@ -651,7 +685,7 @@ def test_create_comment_failure_logs_and_continues(self): svc = MagicMock() svc.create_comment.return_value = False - _comment_parent_issue_completed(incident, svc, ["Done"]) + _comment_parent_issue_status_change(incident, svc, "completed", ["Done"]) svc.create_comment.assert_called_once() @@ -660,7 +694,7 @@ def test_create_comment_exception_logs_and_continues(self): svc = MagicMock() svc.create_comment.side_effect = Exception("API error") - _comment_parent_issue_completed(incident, svc, ["Done"]) + _comment_parent_issue_status_change(incident, svc, "completed", ["Done"]) svc.create_comment.assert_called_once() @@ -669,6 +703,6 @@ def test_template_render_error_logs_and_continues(self, settings): incident = self._make_incident(status=IncidentStatus.DONE) svc = MagicMock() - _comment_parent_issue_completed(incident, svc, ["Done"]) + _comment_parent_issue_status_change(incident, svc, "completed", ["Done"]) svc.create_comment.assert_not_called() diff --git a/src/firetower/integrations/services/linear.py b/src/firetower/integrations/services/linear.py index c89cf85d..a4b6de78 100644 --- a/src/firetower/integrations/services/linear.py +++ b/src/firetower/integrations/services/linear.py @@ -23,13 +23,6 @@ "canceled": "Canceled", } -LINEAR_RELATION_TYPE_MAP = { - "related": "related", - "blocks": "blocks", - "blocked": "blocked_by", - "duplicate": "duplicate", -} - TOKEN_LIFETIME = timedelta(days=30) TOKEN_REFRESH_BUFFER = timedelta(days=1) @@ -471,81 +464,3 @@ def get_child_issues(self, issue_id: str) -> list[dict[str, Any]] | None: break return issues - - def _fetch_relations( - self, - issue_id: str, - field: str, - issue_key: str, - seen_ids: set[str], - ) -> list[dict[str, Any]] | None: - query = f""" - query($issueId: String!, $after: String) {{ - issue(id: $issueId) {{ - {field}(first: 50, after: $after) {{ - nodes {{ - type - {issue_key} {{ - {ISSUE_FIELDS} - }} - }} - pageInfo {{ - hasNextPage - endCursor - }} - }} - }} - }} - """ - - issues: list[dict[str, Any]] = [] - cursor: str | None = None - max_pages = 25 - - for _ in range(max_pages): - variables: dict[str, Any] = {"issueId": issue_id} - if cursor is not None: - variables["after"] = cursor - - data = self._graphql(query, variables) - if data is None: - return None - - issue = data.get("issue") - if not issue: - return None - - relations = issue.get(field, {}) - for node in relations.get("nodes", []): - related_issue = node.get(issue_key) - if not related_issue or "id" not in related_issue: - continue - if related_issue["id"] in seen_ids: - continue - seen_ids.add(related_issue["id"]) - - linear_type = node.get("type", "").lower() - relation_type = LINEAR_RELATION_TYPE_MAP.get(linear_type, "related") - issues.append(self._parse_issue(related_issue, relation_type)) - - page_info = relations.get("pageInfo", {}) - if not page_info.get("hasNextPage"): - break - cursor = page_info.get("endCursor") - if cursor is None: - break - - return issues - - def get_related_issues(self, issue_id: str) -> list[dict[str, Any]] | None: - seen_ids: set[str] = set() - - forward = self._fetch_relations(issue_id, "relations", "relatedIssue", seen_ids) - if forward is None: - return None - - inverse = self._fetch_relations(issue_id, "inverseRelations", "issue", seen_ids) - if inverse is None: - return None - - return forward + inverse diff --git a/src/firetower/integrations/tests/test_linear_service.py b/src/firetower/integrations/tests/test_linear_service.py index 390ef7dd..dbeebf99 100644 --- a/src/firetower/integrations/tests/test_linear_service.py +++ b/src/firetower/integrations/tests/test_linear_service.py @@ -537,98 +537,6 @@ def test_paginates_through_multiple_pages(self, linear_service): assert second_call_vars["after"] == "cursor1" -class TestGetRelatedIssues: - def test_combines_forward_and_inverse_relations(self, linear_service): - forward_response = { - "issue": { - "relations": { - "nodes": [ - { - "type": "related", - "relatedIssue": { - "id": "r1", - "identifier": "E-1", - "title": "Related", - "url": "u1", - "state": {"type": "started"}, - "assignee": None, - }, - } - ], - "pageInfo": {"hasNextPage": False, "endCursor": None}, - } - } - } - inverse_response = { - "issue": { - "inverseRelations": { - "nodes": [ - { - "type": "blocks", - "issue": { - "id": "r2", - "identifier": "E-2", - "title": "Blocker", - "url": "u2", - "state": {"type": "unstarted"}, - "assignee": None, - }, - } - ], - "pageInfo": {"hasNextPage": False, "endCursor": None}, - } - } - } - - with patch.object( - linear_service, - "_graphql", - side_effect=[forward_response, inverse_response], - ): - result = linear_service.get_related_issues("issue-1") - - assert len(result) == 2 - assert result[0]["identifier"] == "E-1" - assert result[0]["relation_type"] == "related" - assert result[1]["identifier"] == "E-2" - assert result[1]["relation_type"] == "blocks" - - def test_deduplicates_across_forward_and_inverse(self, linear_service): - shared_issue = { - "id": "same-id", - "identifier": "E-1", - "title": "Same", - "url": "u", - "state": {"type": "started"}, - "assignee": None, - } - forward_response = { - "issue": { - "relations": { - "nodes": [{"type": "related", "relatedIssue": shared_issue}], - "pageInfo": {"hasNextPage": False, "endCursor": None}, - } - } - } - inverse_response = { - "issue": { - "inverseRelations": { - "nodes": [{"type": "related", "issue": shared_issue}], - "pageInfo": {"hasNextPage": False, "endCursor": None}, - } - } - } - - with patch.object( - linear_service, - "_graphql", - side_effect=[forward_response, inverse_response], - ): - result = linear_service.get_related_issues("issue-1") - - assert len(result) == 1 - - class TestGetUserByEmail: def test_returns_user_when_found(self, linear_service): mock_response = { diff --git a/src/firetower/settings.py b/src/firetower/settings.py index c675c6a8..9cc6da37 100644 --- a/src/firetower/settings.py +++ b/src/firetower/settings.py @@ -328,6 +328,7 @@ class StatuspageSettings(TypedDict): "ACTION_ITEM_NAG_COMMENT_HIGH_PRIORITY": config.linear.action_item_nag_comment_high_priority, "ACTION_ITEM_NAG_COMMENT_MEDIUM_PRIORITY": config.linear.action_item_nag_comment_medium_priority, "PARENT_STATUS_COMMENT_COMPLETED": config.linear.parent_status_comment_completed, + "PARENT_STATUS_COMMENT_STARTED": config.linear.parent_status_comment_started, } if config.linear else None From b6e6679cfefa5778afd1ab0395e4f08368b87ef0 Mon Sep 17 00:00:00 2001 From: Spencer Murray Date: Tue, 30 Jun 2026 15:44:56 -0400 Subject: [PATCH 2/3] fix(linear): polish parent-status docs, templates, and tests Dual-review follow-ups: - Document the auto-completion direction in the parent issue description (it was only describing reopen). - Handle the zero-action-items case in the Started status comment template, mirroring the Completed template. - Restore CANCELED-incident -> completed parent-status test coverage. - Reuse the COMPLETED_STATUSES constant instead of an inline literal. --- src/firetower/config.py | 5 ++-- src/firetower/incidents/hooks.py | 2 ++ src/firetower/incidents/services.py | 4 +-- .../incidents/tests/test_services.py | 25 +++++++++++++++++++ 4 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/firetower/config.py b/src/firetower/config.py index 59280e55..99194f8b 100644 --- a/src/firetower/config.py +++ b/src/firetower/config.py @@ -103,8 +103,9 @@ class LinearConfig: parent_status_comment_started: str = ( "Firetower set this issue to **Started**. " "Incident {{ incident.incident_number }} is {{ incident.status }}. " - "{{ completed_action_items }} of {{ total_action_items }} action " - "item{% if total_action_items != 1 %}s{% endif %} complete." + "{% if total_action_items == 0 %}There are no action items." + "{% else %}{{ completed_action_items }} of {{ total_action_items }} action " + "item{% if total_action_items != 1 %}s{% endif %} complete.{% endif %}" ) diff --git a/src/firetower/incidents/hooks.py b/src/firetower/incidents/hooks.py index 5cd15f9b..723a17d9 100644 --- a/src/firetower/incidents/hooks.py +++ b/src/firetower/incidents/hooks.py @@ -1001,6 +1001,8 @@ def _sync_linear_assignee(incident: Incident) -> None: "Add action items as sub-issues (child issues) of this ticket to have " "them tracked by Firetower. " "Do not update title, status or captain here, use Firetower for that.\n\n" + "Firetower will mark this ticket as completed once the incident is " + "resolved and all action items are done. " "Firetower will reopen this ticket if the incident is reopened, or if " "there are still unfinished action items. " "If you have questions, please reach out to #team-sre." diff --git a/src/firetower/incidents/services.py b/src/firetower/incidents/services.py index 8956087d..238e33ff 100644 --- a/src/firetower/incidents/services.py +++ b/src/firetower/incidents/services.py @@ -207,9 +207,7 @@ def _comment_parent_issue_status_change( if not template_source or not template_source.strip(): return - completed_action_items = sum( - 1 for s in statuses if s in {ActionItemStatus.DONE, ActionItemStatus.CANCELED} - ) + completed_action_items = sum(1 for s in statuses if s in COMPLETED_STATUSES) try: comment = _PARENT_STATUS_TEMPLATE_ENV.from_string(template_source).render( incident=incident, diff --git a/src/firetower/incidents/tests/test_services.py b/src/firetower/incidents/tests/test_services.py index 97c70042..8e8527fd 100644 --- a/src/firetower/incidents/tests/test_services.py +++ b/src/firetower/incidents/tests/test_services.py @@ -504,6 +504,31 @@ def test_done_incident_all_items_done_sets_completed(self): svc.update_issue.assert_called_once_with("lin-123", state_id="state-completed") svc.create_comment.assert_called_once() + def test_canceled_incident_all_items_done_sets_completed(self): + incident = self._make_incident(status=IncidentStatus.CANCELED) + ActionItem.objects.create( + incident=incident, + linear_issue_id="li-1", + linear_identifier="INC-1", + title="Item 1", + status=ActionItemStatus.DONE, + url="https://linear.app/issue/1", + ) + ActionItem.objects.create( + incident=incident, + linear_issue_id="li-2", + linear_identifier="INC-2", + title="Item 2", + status=ActionItemStatus.CANCELED, + url="https://linear.app/issue/2", + ) + svc = self._make_linear_service() + + _update_parent_issue_status(incident, svc) + + svc.update_issue.assert_called_once_with("lin-123", state_id="state-completed") + svc.create_comment.assert_called_once() + def test_done_incident_incomplete_items_sets_started(self): incident = self._make_incident(status=IncidentStatus.DONE) ActionItem.objects.create( From 6008d158f1d66c6a12110748ceccdad8ed9b3bd9 Mon Sep 17 00:00:00 2001 From: Spencer Murray Date: Tue, 30 Jun 2026 21:43:22 -0400 Subject: [PATCH 3/3] fix(linear): never override a manually-cancelled parent issue The re-enabled two-way parent status sync would force a parent whose Linear state was manually set to 'canceled' back to 'started' on every sync while the incident is active (and to 'completed' once resolved), because the skip guard only matched the exact target state. Firetower only ever drives the parent to 'started'/'completed', so treat 'canceled' as a terminal human decision and leave it untouched. Reported by Warden (MAV-TB5). --- src/firetower/incidents/services.py | 10 +++++++++- src/firetower/incidents/tests/test_services.py | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/firetower/incidents/services.py b/src/firetower/incidents/services.py index 238e33ff..ac7f9df8 100644 --- a/src/firetower/incidents/services.py +++ b/src/firetower/incidents/services.py @@ -251,7 +251,15 @@ def _update_parent_issue_status( target_state = "completed" if all_complete else "started" parent_issue = linear_service.get_issue(incident.linear_parent_issue_id) - if not parent_issue or parent_issue.get("state_type") == target_state: + if not parent_issue: + return + + # Never override a manually-cancelled parent issue. Firetower only ever + # drives the parent to "started" or "completed", so a "canceled" state + # reflects a deliberate human decision and must not be reopened to + # "started" (or forced to "completed") on subsequent syncs. + current_state_type = parent_issue.get("state_type") + if current_state_type in ("canceled", target_state): return state_id = states.get(target_state) diff --git a/src/firetower/incidents/tests/test_services.py b/src/firetower/incidents/tests/test_services.py index 8e8527fd..1cf8730e 100644 --- a/src/firetower/incidents/tests/test_services.py +++ b/src/firetower/incidents/tests/test_services.py @@ -599,6 +599,24 @@ def test_updates_when_in_different_state(self): svc.update_issue.assert_called_once_with("lin-123", state_id="state-completed") svc.create_comment.assert_called_once() + def test_does_not_reopen_manually_cancelled_parent(self): + incident = self._make_incident(status=IncidentStatus.ACTIVE) + svc = self._make_linear_service(current_state_type="canceled") + + _update_parent_issue_status(incident, svc) + + svc.update_issue.assert_not_called() + svc.create_comment.assert_not_called() + + def test_does_not_complete_manually_cancelled_parent(self): + incident = self._make_incident(status=IncidentStatus.DONE) + svc = self._make_linear_service(current_state_type="canceled") + + _update_parent_issue_status(incident, svc) + + svc.update_issue.assert_not_called() + svc.create_comment.assert_not_called() + def test_skips_update_when_get_issue_fails(self): incident = self._make_incident(status=IncidentStatus.ACTIVE) svc = self._make_linear_service()