From 21b2cdd5f60c048f2fd2c85a57303642bb80ff33 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Wed, 17 Jun 2026 17:54:34 -0700 Subject: [PATCH 1/5] feat: add webhook update method and isEnabled/statusChangeReason fields Fixes #1135 Co-Authored-By: Claude Sonnet 4.6 --- tableauserverclient/models/webhook_item.py | 25 +++++++- .../server/endpoint/webhooks_endpoint.py | 28 +++++++++ tableauserverclient/server/request_factory.py | 20 +++++++ test/assets/webhook_update.xml | 12 ++++ test/test_webhook.py | 60 +++++++++++++++++-- 5 files changed, 137 insertions(+), 8 deletions(-) create mode 100644 test/assets/webhook_update.xml diff --git a/tableauserverclient/models/webhook_item.py b/tableauserverclient/models/webhook_item.py index a8dfe3ef0..b39ca7e8a 100644 --- a/tableauserverclient/models/webhook_item.py +++ b/tableauserverclient/models/webhook_item.py @@ -44,6 +44,12 @@ class WebhookItem: owner_id : str | None The identifier (luid) of the user who owns the webhook. + + is_enabled : bool | None + Whether the webhook is enabled. Disabled webhooks do not fire. + + status_change_reason : str | None + The reason the webhook status last changed (e.g. why it was disabled). """ def __init__(self): @@ -52,8 +58,10 @@ def __init__(self): self.url: str | None = None self._event: str | None = None self.owner_id: str | None = None + self.is_enabled: bool | None = None + self.status_change_reason: str | None = None - def _set_values(self, id, name, url, event, owner_id): + def _set_values(self, id, name, url, event, owner_id, is_enabled=None, status_change_reason=None): if id is not None: self._id = id if name: @@ -64,6 +72,10 @@ def _set_values(self, id, name, url, event, owner_id): self.event = event if owner_id: self.owner_id = owner_id + if is_enabled is not None: + self.is_enabled = is_enabled + if status_change_reason is not None: + self.status_change_reason = status_change_reason @property def id(self) -> str | None: @@ -116,7 +128,14 @@ def _parse_element(webhook_xml: ET.Element, ns) -> tuple: if owner_tag is not None: owner_id = owner_tag.get("id", None) - return id, name, url, event, owner_id + is_enabled = None + is_enabled_str = webhook_xml.get("isEnabled", None) + if is_enabled_str is not None: + is_enabled = is_enabled_str.lower() == "true" + + status_change_reason = webhook_xml.get("statusChangeReason", None) + + return id, name, url, event, owner_id, is_enabled, status_change_reason def __repr__(self) -> str: - return f"" + return f"" diff --git a/tableauserverclient/server/endpoint/webhooks_endpoint.py b/tableauserverclient/server/endpoint/webhooks_endpoint.py index 449fdbdf5..00729544f 100644 --- a/tableauserverclient/server/endpoint/webhooks_endpoint.py +++ b/tableauserverclient/server/endpoint/webhooks_endpoint.py @@ -1,6 +1,7 @@ import logging from .endpoint import Endpoint, api +from .exceptions import MissingRequiredFieldError from tableauserverclient.server import RequestFactory from tableauserverclient.models import WebhookItem, PaginationItem @@ -118,6 +119,33 @@ def create(self, webhook_item: WebhookItem) -> WebhookItem: logger.info(f"Created new webhook (ID: {new_webhook.id})") return new_webhook + @api(version="3.6") + def update(self, webhook_item: WebhookItem) -> WebhookItem: + """ + Modifies an existing webhook. + + REST API: https://help.tableau.com/current/api/rest_api/en-us/REST/rest_api_ref.htm#update_webhook + + Parameters + ---------- + webhook_item : WebhookItem + The webhook item to update. Must have a valid id. + + Returns + ------- + WebhookItem + An object containing information about the updated webhook. + """ + if not webhook_item.id: + error = "Webhook item missing ID. Webhook must be retrieved from server first." + raise MissingRequiredFieldError(error) + url = f"{self.baseurl}/{webhook_item.id}" + update_req = RequestFactory.Webhook.update_req(webhook_item) + server_response = self.put_request(url, update_req) + updated_webhook = WebhookItem.from_response(server_response.content, self.parent_srv.namespace)[0] + logger.info(f"Updated webhook (ID: {webhook_item.id})") + return updated_webhook + @api(version="3.6") def test(self, webhook_id: str): """ diff --git a/tableauserverclient/server/request_factory.py b/tableauserverclient/server/request_factory.py index fc4694c01..502aed06d 100644 --- a/tableauserverclient/server/request_factory.py +++ b/tableauserverclient/server/request_factory.py @@ -1412,6 +1412,26 @@ def create_req(self, xml_request: ET.Element, webhook_item: "WebhookItem") -> by return ET.tostring(xml_request) + @_tsrequest_wrapped + def update_req(self, xml_request: ET.Element, webhook_item: "WebhookItem") -> bytes: + webhook = ET.SubElement(xml_request, "webhook") + if webhook_item.name is not None: + webhook.attrib["name"] = webhook_item.name + if webhook_item.is_enabled is not None: + webhook.attrib["isEnabled"] = str(webhook_item.is_enabled).lower() + + if webhook_item._event is not None: + source = ET.SubElement(webhook, "webhook-source") + ET.SubElement(source, webhook_item._event) + + if webhook_item.url is not None: + destination = ET.SubElement(webhook, "webhook-destination") + post = ET.SubElement(destination, "webhook-destination-http") + post.attrib["method"] = "POST" + post.attrib["url"] = webhook_item.url + + return ET.tostring(xml_request) + class MetricRequest: @_tsrequest_wrapped diff --git a/test/assets/webhook_update.xml b/test/assets/webhook_update.xml new file mode 100644 index 000000000..c001710e9 --- /dev/null +++ b/test/assets/webhook_update.xml @@ -0,0 +1,12 @@ + + + + + + + + + + + + diff --git a/test/test_webhook.py b/test/test_webhook.py index 4fa011da0..63d8bcd45 100644 --- a/test/test_webhook.py +++ b/test/test_webhook.py @@ -13,6 +13,7 @@ GET_NEW_EVENT_XML = TEST_ASSET_DIR / "webhook_get_new_event.xml" CREATE_XML = TEST_ASSET_DIR / "webhook_create.xml" CREATE_REQUEST_XML = TEST_ASSET_DIR / "webhook_create_request.xml" +UPDATE_XML = TEST_ASSET_DIR / "webhook_update.xml" @pytest.fixture(scope="function") @@ -90,8 +91,7 @@ def test_request_factory(): assert webhook_request_expected.replace("\r", "") == webhook_request_actual -def test_event_setter_none(): - """Setting event to None should store None without crashing.""" +def test_event_setter_none() -> None: item = WebhookItem() item.event = "datasource-updated" assert item.event == "datasource-updated" @@ -100,7 +100,7 @@ def test_event_setter_none(): assert item.event is None -def test_event_setter_short_name(): +def test_event_setter_short_name() -> None: """Short event names should be stored with the webhook-source-event- prefix.""" item = WebhookItem() item.event = "datasource-updated" @@ -108,7 +108,7 @@ def test_event_setter_short_name(): assert item.event == "datasource-updated" -def test_event_setter_full_source_name(): +def test_event_setter_full_source_name() -> None: """Full webhook-source-event- names should be accepted and stored as-is.""" item = WebhookItem() item.event = "webhook-source-event-datasource-updated" @@ -116,7 +116,7 @@ def test_event_setter_full_source_name(): assert item.event == "datasource-updated" -def test_event_setter_new_style_event_name(): +def test_event_setter_new_style_event_name() -> None: """New-style event names (webhook-event-*) should be stored as-is and not mangled.""" item = WebhookItem() item.event = "webhook-event-user-promoted-admin" @@ -167,3 +167,53 @@ def test_create_with_source_event_name(server: TSC.Server) -> None: new_webhook = server.webhooks.create(webhook_model) assert new_webhook.id is not None + + +def test_get_parses_is_enabled_and_status_change_reason(server: TSC.Server) -> None: + response_xml = UPDATE_XML.read_text() + with requests_mock.mock() as m: + m.get(server.webhooks.baseurl + "/webhook-id", text=response_xml) + webhook = server.webhooks.get_by_id("webhook-id") + + assert webhook.is_enabled is True + assert webhook.status_change_reason == "" + assert webhook.name == "webhook-name-updated" + assert webhook.url == "https://updated-url.example.com/hook" + + +def test_update(server: TSC.Server) -> None: + response_xml = UPDATE_XML.read_text() + with requests_mock.mock() as m: + m.put(server.webhooks.baseurl + "/webhook-id", text=response_xml) + webhook_item = WebhookItem() + webhook_item._set_values( + "webhook-id", "webhook-name-updated", "https://updated-url.example.com/hook", "datasource-created", None + ) + webhook_item.is_enabled = True + + updated_webhook = server.webhooks.update(webhook_item) + + assert updated_webhook.id == "webhook-id" + assert updated_webhook.name == "webhook-name-updated" + assert updated_webhook.url == "https://updated-url.example.com/hook" + assert updated_webhook.is_enabled is True + + +def test_update_missing_id(server: TSC.Server) -> None: + webhook_item = WebhookItem() + webhook_item.name = "some-webhook" + with pytest.raises(Exception): + server.webhooks.update(webhook_item) + + +def test_update_request_factory_is_enabled() -> None: + webhook_item = WebhookItem() + webhook_item._set_values( + "webhook-id", "webhook-name", "https://example.com/hook", "datasource-created", None, is_enabled=False + ) + + request_bytes = RequestFactory.Webhook.update_req(webhook_item) + request_str = request_bytes.decode("utf-8") + + assert 'isEnabled="false"' in request_str + assert "webhook-name" in request_str From 64f68fdcd3b794bf662ecba17b58c71b2e81b554 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Fri, 26 Jun 2026 02:19:54 -0700 Subject: [PATCH 2/5] test: strengthen webhook update test coverage Tighten test_update_missing_id to assert MissingRequiredFieldError specifically. Add tests for update_req serializing url and event, omitting isEnabled when None, partial (name-only) updates, and correct parsing of isEnabled="false" from XML. Co-Authored-By: Claude Sonnet 4.6 --- test/test_webhook.py | 59 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/test/test_webhook.py b/test/test_webhook.py index 63d8bcd45..18f46141f 100644 --- a/test/test_webhook.py +++ b/test/test_webhook.py @@ -202,7 +202,7 @@ def test_update(server: TSC.Server) -> None: def test_update_missing_id(server: TSC.Server) -> None: webhook_item = WebhookItem() webhook_item.name = "some-webhook" - with pytest.raises(Exception): + with pytest.raises(TSC.MissingRequiredFieldError): server.webhooks.update(webhook_item) @@ -217,3 +217,60 @@ def test_update_request_factory_is_enabled() -> None: assert 'isEnabled="false"' in request_str assert "webhook-name" in request_str + + +def test_update_request_factory_url_and_event() -> None: + """update_req should serialize url and event into the request body.""" + webhook_item = WebhookItem() + webhook_item._set_values("webhook-id", "webhook-name", "https://example.com/hook", "datasource-created", None) + + request_bytes = RequestFactory.Webhook.update_req(webhook_item) + request_str = request_bytes.decode("utf-8") + + assert "https://example.com/hook" in request_str + assert "webhook-source-event-datasource-created" in request_str + assert 'method="POST"' in request_str + + +def test_update_request_factory_partial_update_name_only() -> None: + """update_req with only name set should omit url, event, and isEnabled.""" + webhook_item = WebhookItem() + webhook_item.name = "new-name" + + request_bytes = RequestFactory.Webhook.update_req(webhook_item) + request_str = request_bytes.decode("utf-8") + + assert "new-name" in request_str + assert "isEnabled" not in request_str + assert "webhook-source" not in request_str + assert "webhook-destination" not in request_str + + +def test_update_request_factory_omits_is_enabled_when_none() -> None: + """update_req should not emit isEnabled when is_enabled is None.""" + webhook_item = WebhookItem() + webhook_item._set_values("webhook-id", "webhook-name", "https://example.com/hook", "datasource-created", None) + # is_enabled is None by default + + request_bytes = RequestFactory.Webhook.update_req(webhook_item) + request_str = request_bytes.decode("utf-8") + + assert "isEnabled" not in request_str + + +def test_parse_is_enabled_false() -> None: + """isEnabled='false' in XML should parse to boolean False.""" + xml = ( + b"" + b'' + b' ' + b" " + b' ' + b" " + b" " + b"" + ) + ns = {"t": "http://tableau.com/api"} + webhooks = WebhookItem.from_response(xml, ns) + assert len(webhooks) == 1 + assert webhooks[0].is_enabled is False From 9bd222ff8f328694ea1a75e1df24ba2ac5107799 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Fri, 26 Jun 2026 14:14:32 -0700 Subject: [PATCH 3/5] fix: restore missing docstring on test_event_setter_none The return-type annotation added in the branch accidentally dropped the docstring that was on test_event_setter_none. Restore it. Co-Authored-By: Claude Sonnet 4.6 --- test/test_webhook.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/test_webhook.py b/test/test_webhook.py index 18f46141f..4afd0c389 100644 --- a/test/test_webhook.py +++ b/test/test_webhook.py @@ -92,6 +92,7 @@ def test_request_factory(): def test_event_setter_none() -> None: + """Setting event to None should store None without crashing.""" item = WebhookItem() item.event = "datasource-updated" assert item.event == "datasource-updated" From 3f4b08ac7c386ffcf3d20939ad2e3b2a6a9e4e13 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Tue, 28 Jul 2026 00:38:44 -0700 Subject: [PATCH 4/5] refactor: use copy.copy + _parse_common_tags in Webhooks.update Match the convention used by users_endpoint.update and datasources_endpoint.update so that fields set locally on a WebhookItem are preserved when the server's update response omits them. Previously the endpoint used WebhookItem.from_response(...)[0], which returned a fresh item populated only from server-supplied fields. - Adds WebhookItem._parse_common_tags matching the pattern in UserItem and WorkbookItem (name refers to XML common tags, not user-facing tags). - Adds test_update_preserves_locally_set_fields_omitted_by_server exercising the local-preservation semantics against a partial server response. Feedback from bcantoni on #1806. --- tableauserverclient/models/webhook_item.py | 15 +++++++ .../server/endpoint/webhooks_endpoint.py | 5 ++- test/test_webhook.py | 40 +++++++++++++++++++ 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/tableauserverclient/models/webhook_item.py b/tableauserverclient/models/webhook_item.py index b39ca7e8a..717abbfca 100644 --- a/tableauserverclient/models/webhook_item.py +++ b/tableauserverclient/models/webhook_item.py @@ -109,6 +109,21 @@ def from_response(cls: type["WebhookItem"], resp: bytes, ns) -> list["WebhookIte all_webhooks_items.append(webhook_item) return all_webhooks_items + def _parse_common_tags(self, webhook_xml, ns) -> "WebhookItem": + """Merge fields from a server response into this item. + + Used by the update endpoint (matching the convention in users_endpoint + and datasources_endpoint) so that locally-set fields are preserved + when the server's response omits them. + """ + if not isinstance(webhook_xml, ET.Element): + parsed = fromstring(webhook_xml) + webhook_xml = parsed.find(".//t:webhook", namespaces=ns) + if webhook_xml is not None: + values = self._parse_element(webhook_xml, ns) + self._set_values(*values) + return self + @staticmethod def _parse_element(webhook_xml: ET.Element, ns) -> tuple: id = webhook_xml.get("id", None) diff --git a/tableauserverclient/server/endpoint/webhooks_endpoint.py b/tableauserverclient/server/endpoint/webhooks_endpoint.py index 00729544f..7526d11e1 100644 --- a/tableauserverclient/server/endpoint/webhooks_endpoint.py +++ b/tableauserverclient/server/endpoint/webhooks_endpoint.py @@ -1,3 +1,4 @@ +import copy import logging from .endpoint import Endpoint, api @@ -142,9 +143,9 @@ def update(self, webhook_item: WebhookItem) -> WebhookItem: url = f"{self.baseurl}/{webhook_item.id}" update_req = RequestFactory.Webhook.update_req(webhook_item) server_response = self.put_request(url, update_req) - updated_webhook = WebhookItem.from_response(server_response.content, self.parent_srv.namespace)[0] logger.info(f"Updated webhook (ID: {webhook_item.id})") - return updated_webhook + updated_webhook = copy.copy(webhook_item) + return updated_webhook._parse_common_tags(server_response.content, self.parent_srv.namespace) @api(version="3.6") def test(self, webhook_id: str): diff --git a/test/test_webhook.py b/test/test_webhook.py index 4afd0c389..ab3778f34 100644 --- a/test/test_webhook.py +++ b/test/test_webhook.py @@ -207,6 +207,46 @@ def test_update_missing_id(server: TSC.Server) -> None: server.webhooks.update(webhook_item) +def test_update_preserves_locally_set_fields_omitted_by_server(server: TSC.Server) -> None: + """update should preserve locally-set fields that the server response omits. + + Matches the pattern used by users_endpoint.update and datasources_endpoint.update: + fields set on the input item should survive when the server returns a partial + response. + """ + # Server response omits is_enabled, owner, and url; only id and name are returned. + partial_response = ( + "" + '' + ' ' + " " + " " + " " + " " + "" + ) + with requests_mock.mock() as m: + m.put(server.webhooks.baseurl + "/webhook-id", text=partial_response) + webhook_item = WebhookItem() + webhook_item._set_values( + "webhook-id", + "webhook-name-original", + "https://local-url.example.com/hook", + "datasource-created", + "local-owner-luid", + ) + webhook_item.is_enabled = False + + updated_webhook = server.webhooks.update(webhook_item) + + # Fields returned by the server should be updated. + assert updated_webhook.name == "webhook-name-updated" + # Fields omitted by the server should retain their locally-set values. + assert updated_webhook.url == "https://local-url.example.com/hook" + assert updated_webhook.owner_id == "local-owner-luid" + assert updated_webhook.is_enabled is False + + def test_update_request_factory_is_enabled() -> None: webhook_item = WebhookItem() webhook_item._set_values( From c2a4c1f0d0ebcf387ae7440a1fa6131ab9a85fc1 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Fri, 7 Aug 2026 00:45:35 -0700 Subject: [PATCH 5/5] Address code-review findings on #1806 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Combined findings from the June 17 pre-merge code review and the recent Copilot pass: - **Missing test: isEnabled-only partial update** — a webhook update with only `is_enabled` set (no name, url, event) should produce an XML body that emits `isEnabled` but omits every other element/attr. Added `test_update_request_factory_partial_update_is_enabled_only`. - **Missing test: status_change_reason absent from response** — verify that when the server omits the `statusChangeReason` attribute entirely, `WebhookItem.status_change_reason` parses to None (not empty string, no crash). Added `test_status_change_reason_absent_from_response_is_none`. - **Missing test: update() on server < 3.6** — the `@api(version="3.6")` decorator should block, not proceed to the underlying PUT. Added `test_update_raises_on_server_below_3_6`. - **Missing test: new-style webhook-event-* through update_req** — round trip the newer event-name prefix through the update payload builder. Added `test_update_request_factory_new_style_event_name`. - **Copilot: docstring "stored as-is" is misleading** — the setter test for `webhook-source-event-*` names had a one-line docstring that implied the public `.event` getter also returns the full name. It doesn't: it strips the prefix for backward compat. Expanded the docstring to say so explicitly. - **Copilot: misleading error message on update() missing id** — the current text says "Webhook must be retrieved from server first", implying `webhooks.get()` is the only way. Callers can also set the id directly; broadened the message to mention both paths. No production-code behaviour change beyond the error-message text. 27 tests in test_webhook.py pass (23 existing + 4 new). Full suite: 879 passed, 1 skipped. --- .../server/endpoint/webhooks_endpoint.py | 5 +- test/test_webhook.py | 74 ++++++++++++++++++- 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/tableauserverclient/server/endpoint/webhooks_endpoint.py b/tableauserverclient/server/endpoint/webhooks_endpoint.py index 7526d11e1..c0cf611fd 100644 --- a/tableauserverclient/server/endpoint/webhooks_endpoint.py +++ b/tableauserverclient/server/endpoint/webhooks_endpoint.py @@ -138,7 +138,10 @@ def update(self, webhook_item: WebhookItem) -> WebhookItem: An object containing information about the updated webhook. """ if not webhook_item.id: - error = "Webhook item missing ID. Webhook must be retrieved from server first." + error = ( + "Webhook item missing ID. Set webhook_item.id directly, or fetch the " + "webhook via webhooks.get_by_id() / webhooks.get() before updating." + ) raise MissingRequiredFieldError(error) url = f"{self.baseurl}/{webhook_item.id}" update_req = RequestFactory.Webhook.update_req(webhook_item) diff --git a/test/test_webhook.py b/test/test_webhook.py index ab3778f34..dfcad04c9 100644 --- a/test/test_webhook.py +++ b/test/test_webhook.py @@ -110,7 +110,10 @@ def test_event_setter_short_name() -> None: def test_event_setter_full_source_name() -> None: - """Full webhook-source-event- names should be accepted and stored as-is.""" + """Full webhook-source-event-* names: _event stores the full name; the + public event getter strips the webhook-source-event- prefix for backwards + compatibility with callers that expect the short form. + """ item = WebhookItem() item.event = "webhook-source-event-datasource-updated" assert item._event == "webhook-source-event-datasource-updated" @@ -315,3 +318,72 @@ def test_parse_is_enabled_false() -> None: webhooks = WebhookItem.from_response(xml, ns) assert len(webhooks) == 1 assert webhooks[0].is_enabled is False + + +def test_update_request_factory_partial_update_is_enabled_only() -> None: + """update_req with only is_enabled set (no name, url, event) should emit + only the isEnabled attribute. Server should accept this per Update Webhook + REST spec, though the actual accepted-fields matrix must be validated e2e. + """ + webhook_item = WebhookItem() + webhook_item._set_values("wh-1", None, None, None, None) + webhook_item.is_enabled = False + + request_bytes = RequestFactory.Webhook.update_req(webhook_item) + request_str = request_bytes.decode("utf-8") + + assert 'isEnabled="false"' in request_str + assert "webhook-source" not in request_str + assert "webhook-destination" not in request_str + # name attribute is only emitted when webhook_item.name is set + assert 'name="' not in request_str + + +def test_status_change_reason_absent_from_response_is_none() -> None: + """A webhook response without a statusChangeReason attribute should leave + the parsed item.status_change_reason as None (not empty string, not raise). + """ + xml = ( + b"" + b'' + b' ' + b" " + b' ' + b" " + b" " + b"" + ) + ns = {"t": "http://tableau.com/api"} + webhooks = WebhookItem.from_response(xml, ns) + assert len(webhooks) == 1 + assert webhooks[0].status_change_reason is None + + +def test_update_raises_on_server_below_3_6(server: TSC.Server) -> None: + """webhooks.update is decorated @api(version="3.6"); calling on an older + server must raise EndpointUnavailableError, not silently attempt the PUT. + """ + from tableauserverclient.server.exceptions import EndpointUnavailableError + + server.version = "3.5" + webhook_item = WebhookItem() + webhook_item._set_values("wh-1", "new-name", None, None, None) + + with pytest.raises(EndpointUnavailableError): + server.webhooks.update(webhook_item) + + +def test_update_request_factory_new_style_event_name() -> None: + """Webhooks created with new-style webhook-event-* event names should + round-trip through update_req: the full event name is emitted as an + XML element inside and stored verbatim in _event. + """ + webhook_item = WebhookItem() + webhook_item._set_values("wh-3", None, None, None, None) + webhook_item.event = "webhook-event-user-promoted-admin" + + request_bytes = RequestFactory.Webhook.update_req(webhook_item) + request_str = request_bytes.decode("utf-8") + + assert "webhook-source" in request_str + assert "webhook-event-user-promoted-admin" in request_str