feat(docs): add Order History section and update related documentation - #2681
Maciej D (mdanilowicz) wants to merge 1 commit into
Conversation
Patryk Tomczyk (patzick)
left a comment
Patryk Tomczyk (patzick)
left a comment
There was a problem hiding this comment.
🔍 Code Review — docs recipe
Good, well-grounded recipe: composable signatures, defu precedence, the limit-overwrite behaviour, the guest-auth criteria shape and all SchemaTypeTooltip type keys check out against the current schema. Two factual claims contradict the shipped code/schema, plus two smaller accuracy points — see inline comments.
Blockers: none · Major: 2 · Minor: 2 · Nits: 2
CI: all 9 checks green. No merge conflicts. PR is still a draft and the description is the unfilled template.
Side note (not this PR): apps/docs/src/.data/schema/storeApiSchema.json is pinned at 6.7.10.0 while packages/api-client ships 6.7.13.0 — the schema:update:store script copies to .data/schema/… relative to apps/docs, but the file lives in apps/docs/src/.data/schema/, so it never lands. That stale copy is what the tooltips on this page render.
|
|
||
| The Store API resolves the customer of `readOrder post /order`, `orderSetPayment post /order/payment`, and `cancelOrder post /order/state/cancel` from the `sw-context-token` header. Nothing in the request names the customer, so the order history changes with the session and not with a route parameter. | ||
|
|
||
| `handlePaymentMethod post /handle-payment` is the exception: it is secured with the access key alone and identifies the order by `orderId` in the body. |
There was a problem hiding this comment.
Major — factual error. /handle-payment is not secured with the access key alone. In packages/api-client/api-types/storeApiSchema.json (6.7.13.0):
/handle-payment → security: [{ ApiKey: [], ContextToken: [] }]
/order → security: [{ ApiKey: [], ContextToken: [] }]
/payment-method → security: [{ ApiKey: [] }]
So it is the same security as the other order routes, not an exception. The "access key alone" claim only holds in the stale docs copy apps/docs/src/.data/schema/storeApiSchema.json (6.7.10.0).
Suggest dropping the sentence, or rewriting it as: handlePaymentMethod still identifies the order explicitly via orderId in the body rather than relying on the session alone — which is the part that is actually useful here.
| ## Edge Cases | ||
|
|
||
| - `loadOrders` builds its body as `{ ...parameters, limit: limit.value }`. A `limit` in your criteria is overwritten, so page sizes are changed through `limit.value` before the call. | ||
| - The list request sends no associations. `Schemas["Order"]` declares `stateMachineState` and `documents` as required, but they are associations and stay undefined at runtime until you request them. |
There was a problem hiding this comment.
Major — wrong for documents. The claim holds for stateMachineState, but not for documents:
useDefaultOrderAssociations()never requestsdocuments.useOrderDetailscomputeshasDocumentsas!!_sharedOrder.value?.documents.length— the optional chain stops at_sharedOrder.value, so an undefineddocumentswould throw aTypeError.- Both starter templates render
<AccountOrderDownloads v-if="hasDocuments" …>on a defaultloadOrderDetails()(templates/vue-demo-store/app/components/account/order/detail/View.vue:208,templates/vue-starter-template/app/components/account/order/Details.vue:156), andpackages/composables/src/mocks/Order.ts:1124carriesdocuments: [].
The route returns documents regardless of the criteria. Please limit the bullet to stateMachineState.
|
|
||
| - `loadOrders` builds its body as `{ ...parameters, limit: limit.value }`. A `limit` in your criteria is overwritten, so page sizes are changed through `limit.value` before the call. | ||
| - The list request sends no associations. `Schemas["Order"]` declares `stateMachineState` and `documents` as required, but they are associations and stay undefined at runtime until you request them. | ||
| - The same applies to `lineItems`, `deliveries`, and `transactions`. A list row cannot show the ordered products, the shipping method, or the payment state unless the criteria asks for those associations. |
There was a problem hiding this comment.
Minor. "The same applies" loses the point of the previous bullet. lineItems, deliveries and transactions are declared optional (lineItems?:, deliveries?:, transactions?:) in Schemas["Order"], so there is no required-vs-undefined mismatch for them — the type already tells you they may be absent. Worth saying so explicitly, since that contrast is what makes stateMachineState a trap.
| - The list request sends no associations. `Schemas["Order"]` declares `stateMachineState` and `documents` as required, but they are associations and stay undefined at runtime until you request them. | ||
| - The same applies to `lineItems`, `deliveries`, and `transactions`. A list row cannot show the ordered products, the shipping method, or the payment state unless the criteria asks for those associations. | ||
| - `useCustomerOrders` reads only `data.orders` from the response and discards `paymentChangeable`. Sending `checkPromotion: true` on the list gains nothing through this composable; `paymentChangeable` is available on `useOrderDetails`, whose request always sets it. | ||
| - `totalPages` is computed as `orders.total` divided by the current `limit` ref. Writing `limit.value` after a load changes `totalPages` without reloading anything. |
There was a problem hiding this comment.
Minor. It is Math.ceil(totalOrderItemsCount.value / limit.value) (packages/composables/src/useCustomerOrders/useCustomerOrders.ts:79). "Divided by" drops the ceiling, which is exactly what makes the last partial page reachable in the v-if="currentPage < totalPages" example above.
| {{ order.orderNumber }} | ||
| </a> | ||
| <span>{{ formatDate(order.orderDate) }}</span> | ||
| <span>{{ order.amountTotal }}</span> |
There was a problem hiding this comment.
Nit. amountTotal is rendered raw, so the example prints e.g. 123.45 with no currency. Either format it with usePrice().getFormattedPrice(order.amountTotal) or add a short comment that formatting is left out on purpose — copy-paste recipes tend to be taken literally.
| loadFirstPage(); | ||
| } | ||
| }, | ||
| { immediate: true } |
There was a problem hiding this comment.
Nit. Missing trailing comma: the sibling wishlist.md recipe ends the same watcher with { immediate: true },. Cosmetic only — apps/docs has no lint script, so nothing enforces it.
|
Patryk Tomczyk (@patzick) this is a draft, not ready for review |
2 participants
Description
Type of change
ToDo's
Screenshots (if applicable)
Additional context