Skip to content

fix: use canonical shipping destination address fields - #97

Open
FanouZeng-TT wants to merge 2 commits into
Universal-Commerce-Protocol:mainfrom
FanouZeng-TT:fix/shipping-destination-field-names
Open

fix: use canonical shipping destination address fields#97
FanouZeng-TT wants to merge 2 commits into
Universal-Commerce-Protocol:mainfrom
FanouZeng-TT:fix/shipping-destination-field-names

Conversation

@FanouZeng-TT

Copy link
Copy Markdown
Contributor

Description

ShippingDestination inherits the canonical address fields
address_locality and address_region. The conformance helpers instead passed
locality and region when creating destinations:

ShippingDestination(
    ...,
    locality=dest_data.get("locality", dest_data.get("city")),
    region=dest_data.get("region", dest_data.get("state")),
)

The model accepts extra fields, so those names are serialized as non-contract
keys while address_locality and address_region remain unset. The same
mismatch appears in the checkout-update lifecycle destination.

Fix: pass the canonical address field names in both construction paths.

A direct serialization regression confirms that the old keys omit the canonical
fields, while the corrected fields serialize exclusively as
address_locality and address_region.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Is this a Breaking Change or Removal?

N/A — fixes conformance test request construction without removing schema fields.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have run a direct serialization regression that proves the fix
  • New and existing integration tests pass locally (requires a configured merchant server)
  • Any dependent changes have been merged and published in downstream modules

@damaz91 damaz91 added the status:needs-triage Signal that the PR is ready for human triage label Aug 24, 2026
@vishkaty

Copy link
Copy Markdown
Contributor

Ran this branch against the reference with the python sdk at head: the 52 suite errors persist, because both construction sites still build shipping_destination.ShippingDestination while the post #83 destination unions accept only the request variants, so validation rejects the instance with a model_type error before the renamed fields are reached. On top of this branch, switching create_checkout_payload in integration_test_utils.py to ShippingDestinationCreateRequest and the test_update_checkout site in checkout_lifecycle_test.py to ShippingDestinationUpdateRequest takes the suite from 52 errors to 2. The remaining 2 are the OrderTest failures already red in the nightly since Aug 21, a server side Payment validation error in the reference, unrelated to these fields.

@FanouZeng-TT

Copy link
Copy Markdown
Contributor Author

Thanks for testing this against the SDK at head. I updated both construction sites to use the request-specific models: ShippingDestinationCreateRequest for checkout creation and ShippingDestinationUpdateRequest for checkout updates. I also reproduced the model_type failure with the generic model and verified both request variants against the post-#83 destination unions, then reran Ruff, formatting, full pre-commit, Python compilation, and git diff --check.

@carolinerg1 carolinerg1 added status:under-review and removed status:needs-triage Signal that the PR is ready for human triage labels Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants