From d67838bac7bf4791f9f7cd9e211e668f3637afd2 Mon Sep 17 00:00:00 2001 From: Jamyn Shanley <696574+Jamyn@users.noreply.github.com> Date: Thu, 20 Aug 2026 21:18:11 +0000 Subject: [PATCH 1/2] fix(confluence): copy the template the caller asked for copy_file ignored file_id and read one configured page instead, so every executive report and form export was a copy of the incident template. Dispatch resolves a different template per document type and passes it in; which one it is was never the plugin's decision. That configured page was the only reader of `template_id`, so the field goes with it. An existing instance keeps working: the key is simply ignored. --- .../configuring-atlassian-confluence.mdx | 4 --- .../dispatch_atlassian_confluence/plugin.py | 14 +++++---- .../fake_confluence.py | 1 - .../test_confluence_live.py | 29 ++++++++++++++++++- .../test_confluence_storage_plugin.py | 26 ++++++++++++----- 5 files changed, 54 insertions(+), 20 deletions(-) diff --git a/docs/docs/administration/settings/plugins/configuring-atlassian-confluence.mdx b/docs/docs/administration/settings/plugins/configuring-atlassian-confluence.mdx index aca269cc..c96ef792 100644 --- a/docs/docs/administration/settings/plugins/configuring-atlassian-confluence.mdx +++ b/docs/docs/administration/settings/plugins/configuring-atlassian-confluence.mdx @@ -26,10 +26,6 @@ Dispatch ships with Atlassian Confluence storage and document plugins. This page > API token to access the confluence instance. Please refer to the [link](https://support.atlassian.com/atlassian-account/docs/manage-api-tokens-for-your-atlassian-account/) on creating a new API token. -### `Incident template ID` \[Required\] - -> This is the page id of the template that contains the post incident review document details. The plugin uses this template to create a new page and then replace the supported variables with the incident details. - ### `Root Incident Storage Page ID` \[Required\] > Page id of the page each incident's own page is created under, found in the page URL: `/pages//`. diff --git a/src/dispatch/plugins/dispatch_atlassian_confluence/plugin.py b/src/dispatch/plugins/dispatch_atlassian_confluence/plugin.py index 4f46c043..4a603f92 100644 --- a/src/dispatch/plugins/dispatch_atlassian_confluence/plugin.py +++ b/src/dispatch/plugins/dispatch_atlassian_confluence/plugin.py @@ -35,9 +35,6 @@ def describe(exception: Exception) -> str: class ConfluenceConfiguration(ConfluenceConfigurationBase): """Confluence configuration description.""" - template_id: str = Field( - title="Incident template ID", description="This is the page id of the template." - ) root_id: str = Field( title="Root Incident Storage Page ID", description=( @@ -105,11 +102,16 @@ def create_file( ) def copy_file(self, folder_id: str, file_id: str, name: str): - # TODO : This is the function that is responsible for making the incident documents. + """Copies the page it is given into the page it is given. + + `file_id` is the template Dispatch resolved for this document -- the + incident type's own template, the executive report's, or the form + export's. They are different pages, and which one this is is the + caller's decision to make. + """ try: api = confluence_api(self.configuration) - logger.info(f"Copy_file function with args {folder_id}, {file_id}, {name}") - template_content = api.get_page(self.configuration.template_id) + template_content = api.get_page(file_id) page_details = api.create_page( parent_id=folder_id, title=name, diff --git a/tests/plugins/dispatch_atlassian_confluence/fake_confluence.py b/tests/plugins/dispatch_atlassian_confluence/fake_confluence.py index e55663f8..0850df3c 100644 --- a/tests/plugins/dispatch_atlassian_confluence/fake_confluence.py +++ b/tests/plugins/dispatch_atlassian_confluence/fake_confluence.py @@ -71,7 +71,6 @@ def build_configuration(hosting_type: str, **overrides): "hosting_type": hosting_type, "username": USERNAME, "password": PASSWORD, - "template_id": TEMPLATE_ID, "root_id": ROOT_PAGE_ID, } values.update(overrides) diff --git a/tests/plugins/dispatch_atlassian_confluence/test_confluence_live.py b/tests/plugins/dispatch_atlassian_confluence/test_confluence_live.py index 10056ace..2658cb48 100644 --- a/tests/plugins/dispatch_atlassian_confluence/test_confluence_live.py +++ b/tests/plugins/dispatch_atlassian_confluence/test_confluence_live.py @@ -93,7 +93,6 @@ def __init__(self, platform, settings): username=settings["username"], password=settings["password"], root_id=settings["root_id"], - template_id=settings["template_id"], ) self.storage = ConfluencePagePlugin() self.storage.configuration = self.configuration @@ -214,6 +213,34 @@ def test_the_template_is_copied_under_the_incident_page(live): ) +def test_each_document_copies_the_template_it_was_given(live): + """Dispatch resolves a different template per document type. The plugin + used to ignore `file_id` and copy one configured page, so an executive + report was a copy of the incident template.""" + incident = live.track( + live.storage.create_file(parent_id=live.root_id, name=live.title("incident")) + ) + other_source = live.track( + live.storage.create_file(parent_id=live.root_id, name=live.title("other template")) + ) + + from_template = live.track( + live.storage.copy_file( + folder_id=incident["id"], file_id=live.settings["template_id"], name=live.title("a") + ) + ) + from_other = live.track( + live.storage.copy_file( + folder_id=incident["id"], file_id=other_source["id"], name=live.title("b") + ) + ) + + body = lambda page_id: live.api.get_page(page_id)["body"]["storage"]["value"] # noqa: E731 + assert body(from_template["id"]) == body(live.settings["template_id"]) + assert body(from_other["id"]) == body(other_source["id"]) + assert body(from_template["id"]) != body(from_other["id"]) + + def test_the_copied_document_can_be_substituted_into(live): """The template needs `{{commander}}` in it, per this module's docstring.""" incident = live.track( diff --git a/tests/plugins/dispatch_atlassian_confluence/test_confluence_storage_plugin.py b/tests/plugins/dispatch_atlassian_confluence/test_confluence_storage_plugin.py index 7c5e5128..9cb287b7 100644 --- a/tests/plugins/dispatch_atlassian_confluence/test_confluence_storage_plugin.py +++ b/tests/plugins/dispatch_atlassian_confluence/test_confluence_storage_plugin.py @@ -207,26 +207,33 @@ def test_copy_file_sends_the_template_storage_body_not_the_body_object(confluenc object nests a dict under `body.storage.value`, which both APIs reject.""" plugin = storage_plugin(hosting_type) - plugin.copy_file(folder_id=ROOT_PAGE_ID, file_id="ignored", name="Incident Review") + plugin.copy_file(folder_id=ROOT_PAGE_ID, file_id=TEMPLATE_ID, name="Incident Review") created = confluence.last("POST").body["body"]["storage"]["value"] assert created == TEMPLATE_BODY assert isinstance(created, str) -def test_copy_file_reads_the_configured_template(confluence, hosting_type): +def test_copy_file_copies_the_template_it_was_given(confluence, hosting_type): + """Dispatch resolves a different template per document -- the incident + type's, the executive report's, the form export's -- and passes it as + `file_id`. The plugin used to ignore it and copy one configured page, so + every executive report was a copy of the incident template.""" plugin = storage_plugin(hosting_type) - plugin.copy_file(folder_id=ROOT_PAGE_ID, file_id="ignored", name="Incident Review") + plugin.copy_file(folder_id=ROOT_PAGE_ID, file_id=ROOT_PAGE_ID, name="Executive Report") - assert any(TEMPLATE_ID in r.path for r in confluence.requests if r.method == "GET") + read = [r.path for r in confluence.requests if r.method == "GET"] + assert any(ROOT_PAGE_ID in path for path in read) + assert not any(TEMPLATE_ID in path for path in read) + assert confluence.last("POST").body["body"]["storage"]["value"] == "<p>Incident home</p>" def test_copy_file_creates_the_page_under_the_given_folder(confluence, hosting_type): plugin = storage_plugin(hosting_type) incident = plugin.create_file(parent_id=ROOT_PAGE_ID, name="Dispatch Incident") - result = plugin.copy_file(folder_id=incident["id"], file_id="ignored", name="Incident Review") + result = plugin.copy_file(folder_id=incident["id"], file_id=TEMPLATE_ID, name="Incident Review") request = confluence.last("POST") if hosting_type == "cloud": @@ -244,7 +251,7 @@ def test_copy_file_links_to_the_new_page_not_the_template(confluence, hosting_ty persisted as the document's own link.""" plugin = storage_plugin(hosting_type) - result = plugin.copy_file(folder_id=ROOT_PAGE_ID, file_id="ignored", name="Incident Review") + result = plugin.copy_file(folder_id=ROOT_PAGE_ID, file_id=TEMPLATE_ID, name="Incident Review") assert result["id"] in result["weblink"] or "Incident+Review" in result["weblink"] assert TEMPLATE_ID not in result["weblink"] @@ -258,7 +265,7 @@ def test_copy_file_needs_no_second_call_to_place_the_page(confluence, hosting_ty raise on 4xx, so the move only ever failed silently.""" plugin = storage_plugin(hosting_type) - plugin.copy_file(folder_id=ROOT_PAGE_ID, file_id="ignored", name="Incident Review") + plugin.copy_file(folder_id=ROOT_PAGE_ID, file_id=TEMPLATE_ID, name="Incident Review") assert all("/move/" not in request.path for request in confluence.requests) assert all(request.method != "PUT" for request in confluence.requests) @@ -284,7 +291,10 @@ def test_copy_file_returns_none_and_logs_when_confluence_fails( plugin = storage_plugin(hosting_type) confluence.fail_with(status) - assert plugin.copy_file(folder_id=ROOT_PAGE_ID, file_id="x", name="Incident Review") is None + assert ( + plugin.copy_file(folder_id=ROOT_PAGE_ID, file_id=TEMPLATE_ID, name="Incident Review") + is None + ) assert "Exception happened while creating page" in caplog.text From af0b35b97fe669e629f5f2411c7a464554621696 Mon Sep 17 00:00:00 2001 From: Jamyn Shanley <696574+Jamyn@users.noreply.github.com> Date: Thu, 20 Aug 2026 21:34:02 +0000 Subject: [PATCH 2/2] feat(confluence): delete an incident's pages when its storage is deleted delete_file was never implemented, so delete_storage logged NotImplementedError and the pages stayed. Neither client removes a subtree on its own. Cloud trashes the page it is given and re-parents the children onto their grandparent, leaving an incident's Logs, Screengrabs and documents on the storage root -- observed against a real site, and the reason both paths walk the tree depth-first. Server's remove_page issues its DELETE in advanced mode, which skips raise_for_status, so its own error handling never runs and a refusal arrives as an unread status code. The wrapper reads it and raises. --- .../dispatch_atlassian_confluence/client.py | 36 +++++++++++ .../dispatch_atlassian_confluence/plugin.py | 8 +++ .../fake_confluence.py | 46 +++++++++++++- .../test_confluence_live.py | 29 +++++++++ .../test_confluence_storage_plugin.py | 63 +++++++++++++++++++ 5 files changed, 179 insertions(+), 3 deletions(-) diff --git a/src/dispatch/plugins/dispatch_atlassian_confluence/client.py b/src/dispatch/plugins/dispatch_atlassian_confluence/client.py index 322a002f..415ca897 100644 --- a/src/dispatch/plugins/dispatch_atlassian_confluence/client.py +++ b/src/dispatch/plugins/dispatch_atlassian_confluence/client.py @@ -26,6 +26,10 @@ ) +class ConfluenceError(Exception): + """A refusal the 5.x client reports without raising one of its own.""" + + class ConfluenceApi: """The Confluence operations Dispatch performs, over one 5.x client.""" @@ -88,6 +92,10 @@ def _parent(self, page_id: str) -> dict: def get_page(self, page_id: str) -> dict: raise NotImplementedError + def delete_page(self, page_id: str) -> None: + """Removes a page and everything filed beneath it.""" + raise NotImplementedError + def update_page(self, *, page: dict, body: str) -> dict: """Rewrites the body of a page already read with ``get_page``. @@ -144,6 +152,22 @@ def _parent(self, page_id: str) -> dict: def get_page(self, page_id: str) -> dict: return self.client.get_page_by_id(page_id, body_format="storage") + def delete_page(self, page_id: str) -> None: + # Cloud trashes only the page it is given and re-parents the children + # onto their grandparent, so an incident's sub-pages would resurface on + # the storage root. Depth-first, and each level is listed in full + # before any of it is removed. + for child_id in self._child_ids(page_id): + self.delete_page(child_id) + self.client.delete_page(page_id) + + def _child_ids(self, page_id: str) -> list[str]: + # Not get_child_pages: it addresses `/children/page`, which is not an + # endpoint, and asks for the body-format the API rejects. Its paging + # helper is sound, though, and cursors are not worth restating. + children = self.client._get_paged(f"api/v2/pages/{page_id}/children", params={"limit": 250}) + return [child["id"] for child in children] + def update_page(self, *, page: dict, body: str) -> dict: # PageUpdateRequest requires id, status, title, body and version, and # the client sends `status` only when asked to. Supplying `version` is @@ -194,6 +218,18 @@ def _parent(self, page_id: str) -> dict: def get_page(self, page_id: str) -> dict: return self.client.get_page_by_id(page_id, expand="body.storage", status=None, version=None) + def delete_page(self, page_id: str) -> None: + # Not remove_page(recursive=True): it issues the DELETE in advanced + # mode, which skips raise_for_status, so its own error handling never + # runs and a refusal comes back as a status code nobody reads. Each + # level is listed in full before any of it goes -- the collection + # shifts under a paged read otherwise. + for child in list(self.client.get_page_child_by_type(page_id)): + self.delete_page(child["id"]) + status = self.client.remove_page(page_id) + if not 200 <= status < 300: + raise ConfluenceError(f"Confluence refused to delete page {page_id}: HTTP {status}") + def update_page(self, *, page: dict, body: str) -> dict: # v1 reads the version from the page's history itself. return self.client.update_page( diff --git a/src/dispatch/plugins/dispatch_atlassian_confluence/plugin.py b/src/dispatch/plugins/dispatch_atlassian_confluence/plugin.py index 4a603f92..4f3c2e80 100644 --- a/src/dispatch/plugins/dispatch_atlassian_confluence/plugin.py +++ b/src/dispatch/plugins/dispatch_atlassian_confluence/plugin.py @@ -125,6 +125,14 @@ def copy_file(self, folder_id: str, file_id: str, name: str): except Exception as e: logger.error(f"Exception happened while creating page {name!r}: {describe(e)}") + def delete_file(self, file_id: str, **kwargs): + """Removes a page and everything filed beneath it. + + Failures are left to `delete_storage`, which logs them: swallowing one + here would report storage as cleaned up when it is still there. + """ + confluence_api(self.configuration).delete_page(file_id) + def move_file(self, new_folder_id: str, file_id: str, **kwargs): """Moves a file from one place to another. Not used in the plugin, keeping the body as the interface is needed to avoid exceptions. diff --git a/tests/plugins/dispatch_atlassian_confluence/fake_confluence.py b/tests/plugins/dispatch_atlassian_confluence/fake_confluence.py index 0850df3c..98320cbc 100644 --- a/tests/plugins/dispatch_atlassian_confluence/fake_confluence.py +++ b/tests/plugins/dispatch_atlassian_confluence/fake_confluence.py @@ -175,8 +175,10 @@ class FakeConfluence: def __init__(self): self.requests = [] self.failure = None + self.failing_methods = () self.omit_webui = False self.next_id = 900001 + self.deleted = [] self.pages = { ROOT_PAGE_ID: Page(ROOT_PAGE_ID, "Incidents", "<p>Incident home</p>"), TEMPLATE_ID: Page(TEMPLATE_ID, "Incident Template", TEMPLATE_BODY), @@ -184,9 +186,14 @@ def __init__(self): # -- what comes back --------------------------------------------------- - def fail_with(self, status: int, body: dict | None = None): - """Every subsequent request fails, in Confluence's error envelope.""" + def fail_with(self, status: int, body: dict | None = None, methods: tuple = ()): + """Subsequent requests fail, in Confluence's error envelope. + + ``methods`` narrows it to those verbs, which is the only way to reach + a refusal of the delete itself rather than of the read before it. + """ self.failure = (status, body if body is not None else {"message": "boom"}) + self.failing_methods = methods def page(self, page_id: str) -> Page: """The stored page, for asserting on where it ended up.""" @@ -219,11 +226,20 @@ def _render(self, page, **kwargs): # -- the routes -------------------------------------------------------- def handle(self, request: Request): - if self.failure: + if self.failure and request.method in (self.failing_methods or (request.method,)): return self.failure path, method = request.path, request.method + if path.endswith("/children") and method == "GET": + return self._children(path.split("/")[-2]) + if path.endswith("/child/page") and method == "GET": + return self._children(path.split("/")[-3]) + if "/api/v2/pages/" in path and method == "DELETE": + return self._delete(path.rsplit("/", 1)[1], reparent=True) + if "/rest/api/content/" in path and method == "DELETE": + return self._delete(path.rsplit("/", 1)[1], reparent=False) + # v2 (Cloud). Matched on the API path alone: the context path in front # of it is /wiki for a tenant URL and /ex/confluence/{cloudId} for a # gateway one, and getting that wrong is the bug under test. @@ -250,6 +266,30 @@ def handle(self, request: Request): return 404, {"message": f"no route for {method} {path}"} + def _children(self, page_id): + if page_id not in self.pages: + return 404, {"message": "page not found"} + kids = [page for page in self.pages.values() if page.parent_id == page_id] + return 200, {"results": [{"id": page.id, "title": page.title} for page in kids]} + + def _delete(self, page_id, *, reparent: bool): + """Removes one page, as Confluence removes one page. + + Cloud re-parents the children onto the grandparent and leaves them + current -- observed against a real site -- so a delete that does not + walk the tree strands an incident's sub-pages on the storage root. + Server's client walks the tree itself. + """ + page = self.pages.pop(page_id, None) + if page is None: + return 404, {"message": "page not found"} + if reparent: + for orphan in self.pages.values(): + if orphan.parent_id == page_id: + orphan.parent_id = page.parent_id + self.deleted.append(page_id) + return 204, {} + # -- v2 ---------------------------------------------------------------- def _v2_create(self, request): diff --git a/tests/plugins/dispatch_atlassian_confluence/test_confluence_live.py b/tests/plugins/dispatch_atlassian_confluence/test_confluence_live.py index 2658cb48..a8215a4e 100644 --- a/tests/plugins/dispatch_atlassian_confluence/test_confluence_live.py +++ b/tests/plugins/dispatch_atlassian_confluence/test_confluence_live.py @@ -269,6 +269,35 @@ def test_a_page_cannot_be_created_under_a_page_that_does_not_exist(live): assert live.storage.create_file(parent_id="1", name=live.title("orphan")) is None +def test_deleting_storage_takes_the_whole_incident_with_it(live): + """Cloud trashes only the page named and re-parents its children onto the + grandparent, leaving them current -- observed here, and the reason + `delete_page` walks the tree rather than calling the API once.""" + incident = live.track( + live.storage.create_file(parent_id=live.root_id, name=live.title("incident")) + ) + logs = live.track(live.storage.create_file(parent_id=incident["id"], name="Logs")) + document = live.track( + live.storage.copy_file( + folder_id=logs["id"], file_id=live.settings["template_id"], name=live.title("doc") + ) + ) + + live.storage.delete_file(file_id=incident["id"]) + + for page_id in (document["id"], logs["id"], incident["id"]): + assert _is_gone(live, page_id), f"{page_id} survived the delete" + assert live.api.get_page(live.root_id)["id"] == live.root_id + + +def _is_gone(live, page_id: str) -> bool: + """Trashed counts as gone: Cloud's delete is a soft delete.""" + try: + return live.api.get_page(page_id).get("status") == "trashed" + except HTTPError as exception: + return exception.response is not None and exception.response.status_code in (404, 400) + + # -- inferences from the schema that only a real instance can settle --------- diff --git a/tests/plugins/dispatch_atlassian_confluence/test_confluence_storage_plugin.py b/tests/plugins/dispatch_atlassian_confluence/test_confluence_storage_plugin.py index 9cb287b7..25615286 100644 --- a/tests/plugins/dispatch_atlassian_confluence/test_confluence_storage_plugin.py +++ b/tests/plugins/dispatch_atlassian_confluence/test_confluence_storage_plugin.py @@ -1,7 +1,10 @@ """What ConfluencePagePlugin actually sends and returns, per platform (#214, #242).""" import pytest +from atlassian.errors import ApiError +from requests import HTTPError +from dispatch.plugins.dispatch_atlassian_confluence.client import ConfluenceError from tests.plugins.dispatch_atlassian_confluence.conftest import storage_plugin from tests.plugins.dispatch_atlassian_confluence.fake_confluence import ( REPORTED_BASE, @@ -437,3 +440,63 @@ def test_cloud_failures_are_logged_with_something_actionable(confluence, caplog) assert plugin.create_file(parent_id=ROOT_PAGE_ID, name="Dispatch Incident") is None assert "403" in caplog.text + + +# -- deleting storage ------------------------------------------------------- + + +def test_delete_file_removes_the_page(confluence, hosting_type): + plugin = storage_plugin(hosting_type) + incident = plugin.create_file(parent_id=ROOT_PAGE_ID, name="Dispatch Incident") + + plugin.delete_file(file_id=incident["id"]) + + assert incident["id"] not in confluence.pages + + +def test_delete_file_takes_the_whole_incident_with_it(confluence, hosting_type): + """Cloud re-parents a deleted page's children onto its grandparent rather + than removing them, so a delete that does not walk the tree leaves Logs, + Screengrabs and every document sitting on the storage root.""" + plugin = storage_plugin(hosting_type) + incident = plugin.create_file(parent_id=ROOT_PAGE_ID, name="Dispatch Incident") + logs = plugin.create_file(parent_id=incident["id"], name="Logs") + document = plugin.copy_file(folder_id=logs["id"], file_id=TEMPLATE_ID, name="Incident Document") + + plugin.delete_file(file_id=incident["id"]) + + for page_id in (incident["id"], logs["id"], document["id"]): + assert page_id not in confluence.pages + assert ROOT_PAGE_ID in confluence.pages + + +def test_delete_file_leaves_the_rest_of_the_space_alone(confluence, hosting_type): + plugin = storage_plugin(hosting_type) + keep = plugin.create_file(parent_id=ROOT_PAGE_ID, name="Another Incident") + keep_logs = plugin.create_file(parent_id=keep["id"], name="Logs") + remove = plugin.create_file(parent_id=ROOT_PAGE_ID, name="Dispatch Incident") + + plugin.delete_file(file_id=remove["id"]) + + assert keep["id"] in confluence.pages + assert keep_logs["id"] in confluence.pages + assert TEMPLATE_ID in confluence.pages + + +@pytest.mark.parametrize("methods", [(), ("DELETE",)]) +def test_delete_file_reports_a_failure_rather_than_swallowing_it(confluence, hosting_type, methods): + """`delete_storage` logs what this raises. Returning quietly would report + storage as cleaned up while the pages are still there. + + Refusing only the DELETE is the case that matters: the v1 client issues it + in a mode that skips raise_for_status, so the refusal arrives as a status + code rather than an exception.""" + plugin = storage_plugin(hosting_type) + incident = plugin.create_file(parent_id=ROOT_PAGE_ID, name="Dispatch Incident") + confluence.fail_with(403, methods=methods) + + # Cloud raises from the client, Server returns a status the wrapper turns + # into one, and a refused listing raises before either. + with pytest.raises((HTTPError, ApiError, ConfluenceError)): + plugin.delete_file(file_id=incident["id"]) + assert incident["id"] in confluence.pages