feat: Add webhook update method with isEnabled/statusChangeReason - #1806
feat: Add webhook update method with isEnabled/statusChangeReason#1806jacalata wants to merge 5 commits into
Conversation
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>
Code ReviewClean, well-structured PR. Here are the issues worth addressing before merge: IssuesConvention deviation —
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
Inconsistent
Missing CHANGELOG entry Per repo conventions, user-visible additions get a changelog bullet with the PR number. Minor
|
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
|
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
WebhookItemparsing/model fields foris_enabledandstatus_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.
| if not webhook_item.id: | ||
| error = "Webhook item missing ID. Webhook must be retrieved from server first." | ||
| raise MissingRequiredFieldError(error) |
| if webhook_item._event is not None: | ||
| source = ET.SubElement(webhook, "webhook-source") | ||
| ET.SubElement(source, webhook_item._event) |
| 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() |
| def test_event_setter_full_source_name() -> None: | ||
| """Full webhook-source-event- names should be accepted and stored as-is.""" |
Summary
Webhooks.update(webhook_item)(REST API v3.6) for modifying an existing webhook's name, event, destination URL, and enabled stateis_enabled(bool) andstatus_change_reason(str) fields toWebhookItem, parsed from theisEnabledandstatusChangeReasonXML attributes returned by the serverRequestFactory.Webhook.update_req()serializes all updatable fields;is_enabledis omitted whenNoneto support partial updates;statusChangeReasonis intentionally not serialized (server-set, read-only)MissingRequiredFieldErrorif the webhook item has no IDwebhook-source-event-*(legacy) andwebhook-event-*(newer) event name prefixes in the event setterCloses #1135
Schema compliance
isEnabledandstatusChangeReasonare both defined onwebhookTypein ts-api_3_29.xsd. Theupdate_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 increate_req().Test plan
python -m pytest test/test_webhook.py -v-- 22 tests, all passserver.webhooks.update(item)returns updatedWebhookItemwith correct fieldsis_enabled=Falseleaves name/url/event unchanged on serverMissingRequiredFieldErrorraised when item has no IDisEnabledattribute absent from request XML whenis_enabledisNone🤖 Generated with Claude Code