feat: Add webhook update method with isEnabled/statusChangeReason - #1806
feat: Add webhook update method with isEnabled/statusChangeReason#1806jacalata wants to merge 8 commits into
Conversation
Sorry, something went wrong.
Fixes #1135 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Sorry, something went wrong.
Code ReviewClean, well-structured PR. Here are the issues worth addressing before merge: IssuesConvention deviation — update uses from_response()[0] instead of copy.copy + _parse_common_tags webhooks_endpoint.py:150 — The project's documented update pattern (used in users_endpoint.py:208, datasources_endpoint.py:353) is: updated = copy.copy(item)
return updated._parse_common_tags(server_response.content, ns)This preserves locally-set fields that the server's partial response omits. Using from_response()[0] works here because the server always returns a complete webhook and WebhookItem has no _parse_common_tags, but it deviates from convention. Either add _parse_common_tags to WebhookItem for consistency, or leave a short comment explaining why the simpler approach is safe. status_change_reason is a mutable public attribute webhook_item.py:61 — It's stored as a plain self.status_change_reason: str | None = None and can be set freely by callers. Since it's server-assigned and not serialized by update_req, a caller who sets it won't get an error but also won't get the intended behavior. Per the project's property pattern for server-assigned/read-only fields (id, created_at, etc.), this should be a getter-only property (no setter), or the docstring should explicitly say "server-set, do not write." test_update doesn't assert the request body test/test_webhook.py:184 — The test only validates response parsing. There's no m.last_request.text assertion confirming the correct XML was sent in the PUT body. The test_update_request_factory_* tests cover serialization independently, so coverage isn't missing, but an integration-level check tying the endpoint to its request body would be more complete. Inconsistent -> None annotation sweep test_webhook.py:91–120 — The PR adds -> None to exactly 4 of the existing test functions while leaving others without it. Either annotate all test functions in the file or none (the file had none before this PR). Missing CHANGELOG entry Per repo conventions, user-visible additions get a changelog bullet with the PR number. server.webhooks.update() is a new public method that qualifies. Minor
|
Sorry, something went wrong.
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.
|
Valid. Landed the convention fix in 3f4b08a:
|
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds webhook update support and extends webhook parsing/serialization to include enablement state and status change reason.
Changes:
- Introduces webhooks.update() endpoint (API v3.6) that preserves locally-set fields when server responses are partial.
- Adds request factory support for webhook update requests.
- Extends WebhookItem parsing/model fields for is_enabled and status_change_reason, plus new tests and XML asset.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file| File | Description |
|---|---|
| test/test_webhook.py | Adds tests for webhook update behavior, request serialization, and parsing isEnabled/statusChangeReason. |
| test/assets/webhook_update.xml | Adds fixture XML representing an updated webhook response. |
| tableauserverclient/server/request_factory.py | Adds Webhook.update_req to build update request bodies. |
| tableauserverclient/server/endpoint/webhooks_endpoint.py | Adds WebhooksEndpoint.update implementation with merge semantics. |
| tableauserverclient/models/webhook_item.py | Adds new fields and parsing/merge helper for webhook update responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)tableauserverclient/server/request_factory.py:1433
- WebhookRequest.update_req accepts partially-populated items, but it currently doesn't validate the types of the provided fields (name/url/_event/is_enabled). If a caller accidentally sets (for example) name=123 or _event to a non-string, ElementTree will raise a low-level TypeError during serialization. create_req already raises clear ValueErrors for invalid types; update_req should do the same for consistency and debuggability.
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()
Sorry, something went wrong.
Two fresh-eyes findings on #1806: 1. `WebhookRequest.update_req` used to happily serialize `<tsRequest><webhook/></tsRequest>` when every updatable field on the WebhookItem was None. The server rejected that with a generic 400 the caller couldn't act on. Now raises ValueError up front with a message naming the four fields that would resolve it. 2. `test_update` verified the response-derived WebhookItem but never inspected the PUT body, so removing the `update_req` call in the endpoint would keep the test green. Added `m.last_request` assertions on the wire: name, isEnabled, url, and event tag all present in the PUT payload. This is the load-bearing endpoint<->factory contract check. Also added test_update_rejects_empty_payload confirming the new guard fires when nothing is set on the item. 28 passed on test_webhook.py. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the remaining unresolved Copilot review threads on #1806. - Add `WebhookItem.event_tag` public property returning the raw internal event string (e.g. `webhook-source-event-datasource-created`). This is distinct from `event`, which strips the `webhook-source-event-` prefix for backward-compat with callers expecting the short form. Both `WebhookRequest.create_req` and `WebhookRequest.update_req` now use `event_tag` instead of the private `_event` attribute; behavior is identical, we just stop cross-module private access. Also updated the create-side "must be provided" error to say `event` instead of `_event`, since callers set that via the public setter. Skipped: - Fix 1 (error-message reword on `webhooks_endpoint.update` missing id) was already applied in c2a4c1f: current text names both paths ("Set webhook_item.id directly, or fetch the webhook via webhooks.get_by_id() / webhooks.get() before updating"). No change. - Fix 3 (empty-update-payload guard in `update_req`) was already applied in 8eba015, along with `test_update_rejects_empty_payload`. The Copilot thread can be resolved with a reply pointing at that commit. No change. - Fix 4 (`test_event_setter_full_source_name` docstring vs behavior) was already applied in c2a4c1f: docstring now says "_event stores the full name; the public event getter strips the webhook-source-event- prefix for backwards compatibility". Matches the assertion pair. No change. 28 tests in test_webhook.py pass. mypy: no issues found in 104 files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Closes #1135
Schema compliance
isEnabled and statusChangeReason are both defined on webhookType in ts-api_3_29.xsd. The update_req() child element structure (webhook-source, webhook-destination) matches the schema. webhook-event-* style event names are a live API extension not yet reflected in the published XSD; handling them is consistent with the pre-existing behavior in create_req().
Test plan
🤖 Generated with Claude Code