Conversation
source/schemas/shopping/order.json
Outdated
| "properties": { | ||
| "id": { | ||
| "type": "string", | ||
| "description": "Checkout session identifier." |
There was a problem hiding this comment.
| "description": "Checkout session identifier." | |
| "description": ""Unique identifier of the checkout session." |
To be consistent with checkout docs / schema desc
| "type": "string", | ||
| "format": "date-time", | ||
| "description": "RFC 3339 timestamp when this checkout session was created." | ||
| } |
There was a problem hiding this comment.
is the purpose of created_at to identify the sequential order of multiple checkouts? If so, do we need to include updated_at as well?
There was a problem hiding this comment.
Or is the purpose here just to distinguish the original checkout and edit/exchange checkouts ?
There was a problem hiding this comment.
created_at is there so the sequence is clear. Checkouts are frozen after creation - they are never updated - so updated_at is not needed here.
There was a problem hiding this comment.
Should we just rely on order here instead? If we really want to add created_at, it feels like we should add it to checkout too, so there is no information you get about the checkout on order you could not get directly.
There was a problem hiding this comment.
I agree with @lemonmade here on the checkouts array structure feeling askew, I'll attempt to summarize my concerns:
- Denormalization: Including
created_atin the array items feels leaky. If the checkout resource is the source of truth, we should rely on a timestamp within. - Ordering Contract: I understand the path to including
created_atto imply an ordering but, even with it, there is no actual schema enforcement. If the client really cares about ordering they should double-check it, this is not expensive even if it's a 1:1000 relationship. - Naming:
created_atis misleading here. I assume we should only be referencing checkouts that reached a terminal (success) state. Should this befinalized_atorexecuted_atinstead? - Dereferencing: I'm not persuaded that we want to include the full checkout object rather than just references. An ordered array of checkout IDs provides the same lineage, and optionality to the client to fetch the full objects (or not).
| "totals": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "total.json" |
There was a problem hiding this comment.
I don't think the current total and amount schema allows negative - should update them as part of this PR?
There was a problem hiding this comment.
Good catch. Removed minimum: 0 from amount.json so negative values are allowed for signed adjustment totals.
| **Line Items** — what was purchased at checkout: | ||
|
|
||
| * Includes current quantity counts (total, fulfilled) | ||
| * Can change post-order (e.g. order edits, exchanges) |
There was a problem hiding this comment.
With order edits now in the picture, can we add a guideline around what line_items SHOULD include post these changes, is it:
- A comprehensive list of all the items that once existed in the order, even if they were altered/removed via an edit?
OR - Only contain remaining items that are present after the latest edit?
My hunch is that 1) is more comprehensive from a data/audit perspective given there may have been adjustments that reference back to these altered products?
There was a problem hiding this comment.
Option 1 - comprehensive list. Line items should include all items that ever existed on the order, even if altered/removed via an edit. This is why quantity.original exists on line items - it preserves what was originally ordered at checkout, even when quantity.total changes to 0 after an edit. Adjustments can then always reference back to these items.
There was a problem hiding this comment.
Thanks, makes sense, is it possible to actually add this on top of this bullet here?
i.e. Something along the lines of ; **MUST** include all line items that ever existed on the order regardless of edits or alterations?
docs/specification/order.md
Outdated
| ```json | ||
| { | ||
| "total": 3, // Current total quantity | ||
| "original": 3, // Quantity at checkout |
There was a problem hiding this comment.
nit: Maybe we should clarify a bit further that this is the checkout with the oldest created_at timestamp (and not just any checkout)?
There was a problem hiding this comment.
Good point. This refers to the original checkout (the one with the earliest created_at). Updated the description to clarify.
| "partial", | ||
| "fulfilled" | ||
| ], | ||
| "description": "Derived status: fulfilled if quantity.fulfilled == quantity.total, partial if quantity.fulfilled > 0, otherwise processing." |
There was a problem hiding this comment.
I think we will need some more status or at least add some more conditions to this described logic to cover the following 2 scenarios:
- If order is cancelled prior to fulfillment, then by this logic today, item-level status will be stuck in
processing. We should probably add some terminal state to catchcancelled. - If an item is fulfilled, then edited from an exchange, then I'd imagine the old item that got exchanged would have
totals = 0butfulfilled > 0, in that case, what status should it be in (maybe we should havefulfilled if quantity.fulfilled >= quantity.totalinstead)?
There was a problem hiding this comment.
Good observations. The status derivation documented here covers the common case - it is not meant to be exhaustive. The spec treats status as an open string, so businesses can use additional values like cancelled or exchanged for these scenarios. We will keep the derivation simple for now and can expand the documented examples in a follow-up if needed. Does that sound good?
There was a problem hiding this comment.
The explanation makes sense, the only nit-pick from me would be if we can represent this "open string is accepted" concept properly here? My understanding of JSON schema is that as soon as we have enum key, it restricts the value to a finite fixed value set where no further extension is accepted (which is the case here).
b3b2218 to
6039caf
Compare
|
@alexpark20 @jingyli Thanks for the thorough review! Responded to each comment inline and pushed updates for the ones that needed code changes. Please take another look when you get a chance. |
igrigorik
left a comment
There was a problem hiding this comment.
See comment on removing "minimum": 0 on amount.json.
| "title": "Amount", | ||
| "description": "Monetary amount in the currency's minor unit as defined by ISO 4217. Refer to the currency's exponent to determine minor-to-major ratio (e.g., 2 for USD, 0 for JPY, 3 for KWD).", | ||
| "type": "integer", | ||
| "minimum": 0 |
There was a problem hiding this comment.
@richmolj I think this is a footgun. I understand the motivation, but removing this constraint on shared type opens up a class of unexpected scenarios — e.g. negative prices on line items, etc.
If we move forward with allowing negative values, I think we need to be more surgical. PTAL @ #261 (commits), we can use similar approach here.
There was a problem hiding this comment.
Sure, I've updated the PR with signed_amounts.json.
That said, I disagree. The current field requires mental math to understand type = discount means negative, why not make this explicit? Negative line item prices, or negatives from other business-specific types, should be up to the business logic not the spec.
I don't feel strongly here, though, and it's a more significant breaking change. So went ahead and updated.
…emoving minimum on shared amount Restore minimum: 0 on amount.json and introduce signed_amount.json for places that genuinely need negative values. Adjustment totals now inline their schema with signed_amount refs, keeping total.json and all other amount.json consumers non-negative. Addresses review feedback from @igrigorik on #254.
…emoving minimum on shared amount Restore minimum: 0 on amount.json and introduce signed_amount.json for places that genuinely need negative values. Adjustment totals now inline their schema with signed_amount refs, keeping total.json and all other amount.json consumers non-negative. Addresses review feedback from @igrigorik on #254.
0983c3b to
66f6dbf
Compare
…unt override Align with the composition pattern in PR #261 (totals contract): allOf total.json base + signed_amount.json override, rather than inlining the schema.
lemonmade
left a comment
There was a problem hiding this comment.
Left a few nits, but the main parts of the change are looking good to me 👍
| "type": "string", | ||
| "format": "date-time", | ||
| "description": "RFC 3339 timestamp when this checkout session was created." | ||
| } |
There was a problem hiding this comment.
Should we just rely on order here instead? If we really want to add created_at, it feels like we should add it to checkout too, so there is no information you get about the checkout on order you could not get directly.
| "properties": { | ||
| "original": { | ||
| "type": "integer", | ||
| "minimum": 0, |
There was a problem hiding this comment.
Could all of the properties in here use the https://ucp.dev/schemas/shopping/types/amount.json shared type?
| "title": "Signed Amount", | ||
| "description": "Monetary amount in the currency's minor unit as defined by ISO 4217. Refer to the currency's exponent to determine minor-to-major ratio (e.g., 2 for USD, 0 for JPY, 3 for KWD). May be negative — the sign is intrinsic to the value (e.g., discounts are negative, charges are positive).", | ||
| "type": "integer" | ||
| } |
There was a problem hiding this comment.
I think this is in main now, a rebase should remove the diff.
There was a problem hiding this comment.
It looks like we haven't run this in quite awhile, I am not even sure why it is still here. Is there a reason you had to update it for this PR?
jingyli
left a comment
There was a problem hiding this comment.
Thanks for the responses, resolved some threads and added one more generic question on how we think about the representation of checkout_id within order capability.
| **Line Items** — what was purchased at checkout: | ||
|
|
||
| * Includes current quantity counts (total, fulfilled) | ||
| * Can change post-order (e.g. order edits, exchanges) |
There was a problem hiding this comment.
Thanks, makes sense, is it possible to actually add this on top of this bullet here?
i.e. Something along the lines of ; **MUST** include all line items that ever existed on the order regardless of edits or alterations?
| "checkout_id": { | ||
| "type": "string", | ||
| "description": "Associated checkout ID for reconciliation." | ||
| "checkouts": { |
There was a problem hiding this comment.
Looking at this a bit more, a naive QQ: What do we feel is the value by having this full array (especially as a required field)?
The reason I question this is 2-fold:
- This seems to suggest an additional dependency between UCP's checkout & order capabilities. What does it mean for a platform (like a post-order management agent) that only implements/supports order capability in UCP? To them, the concept of
checkout_idis fairly useless since they don't really need/require knowledge on the upstream process. - Per this guideline, there is no guarantee that business will persist the checkout session after completion so I'm unsure what can the platform truly do with this full list of checkout_ids (having the initial one makes sense to me for the reconciliation use case). It also feels a bit odd conceptually since we don't generally go back from order to checkout.
| "partial", | ||
| "fulfilled" | ||
| ], | ||
| "description": "Derived status: fulfilled if quantity.fulfilled == quantity.total, partial if quantity.fulfilled > 0, otherwise processing." |
There was a problem hiding this comment.
The explanation makes sense, the only nit-pick from me would be if we can represent this "open string is accepted" concept properly here? My understanding of JSON schema is that as soon as we have enum key, it restricts the value to a finite fixed value set where no further extension is accepted (which is the case here).
|
I think it may be work escalating this one to the TC, I think the change risks introducing a domain mismatch introduced by using the checkouts array to handle post-purchase modifications.
I would propose, we continue to lean on the Adjustments capability (which already supports signed quantities/amounts) to handle edits, returns, and exchanges. This keeps the Order fulfillment-centric. If we must link to a validation session for audit purposes, we should do so via a |
|
@gsmith85 Thanks for the feedback. I think this is simpler than it appears. Consider a concrete scenario: a buyer places an order through UCP, then calls the merchant and asks to add an item. The merchant sends the buyer a link where they go through checkout again — this time to pay for the additional item and add it to the existing order. The buyer literally goes through checkout twice, so the order references two checkout sessions. That's all the The This is all entirely optional, by the way. If the merchant can handle the edit themselves without the buyer re-entering checkout, they model it as a single checkout with an adjustment. Both are valid — the spec just doesn't preclude the scenario where the buyer goes through checkout again. Happy to escalate to the TC if you'd like further discussion. |
|
@richmolj, thanks for the concrete example, the add-on scenario makes the intent much clearer! I think where we're diverging is just how to best translate that type of flow into the base schema. In the example you give, the tension lives between:
Even though the add-on was in reference to a previous Is there a reason to not model add-ons as a new Order at the protocol layer, and let platforms handle linking them? Perhaps we're missing a layer of modeling around |
Description
Collection of updates to the Order capability to make the spec more flexible, robust, and clear.
checkoutsarray (order edits create new sessions)quantity.originalon line itemsamounttototalsfor consistency with Order and OrderLineItemType of change
functionality to not work as expected, including removal of schema files
or fields)
Is this a Breaking Change or Removal?
!to my PR title (e.g.,feat!: remove field).Breaking Changes / Removal Justification
checkout_id(string) replaced withcheckouts(array of{ id, created_at }objects) — orders can reference multiple checkout sessions via edits and exchangesamounton Adjustment replaced withtotals(array of Total objects) — aligns with the pattern used by Order and OrderLineItemminimum: 1removed from adjustment line item quantities — signed values needed for returns and exchangesminimum: 0removed fromamount.json— amounts can be negative for refunds, credits, and adjustment totals across all capabilitiesChecklist