Conversation
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6328 +/- ##
==========================================
- Coverage 42.76% 42.55% -0.21%
==========================================
Files 2517 2523 +6
Lines 43758 44035 +277
Branches 9952 10484 +532
==========================================
+ Hits 18715 18741 +26
- Misses 25005 25256 +251
Partials 38 38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR extends the orders/refunds/returns flows to capture and display refund/return “reason” and a “reason reference” (a referenced Model/Page), wiring these fields through form state, mutations, GraphQL fragments, and fixtures.
Changes:
- Add
reason/reasonReferencefields to refund and return payloads (including transaction refunds and granted refunds). - Fetch refund settings to determine the configured “reason reference” model type and expose it to pages/forms.
- Update GraphQL fragments/type policies plus fixtures/tests to include the new fields on fulfillments and granted refund lines.
Reviewed changes
Copilot reviewed 25 out of 27 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/orders/views/OrderTransactionRefundCreate/handlers.ts | Adds reasonReference when preparing refund line inputs. |
| src/orders/views/OrderReturn/utils.tsx | Includes global return reason and reasonReference in parsed input. |
| src/orders/views/OrderReturn/useRefundWithinReturn.ts | Sends reason and reasonReferenceId when granting refund within return flow. |
| src/orders/views/OrderReturn/OrderReturn.tsx | Fetches refund settings and passes model type id down to the return page. |
| src/orders/views/OrderGrantRefund/OrderGrantRefund.tsx | Fetches refund settings, passes model type id, and submits reasonReferenceId. |
| src/orders/views/OrderEditGrantRefund/OrderEditGrantRefund.tsx | Fetches refund settings, passes model type id, and submits reasonReferenceId. |
| src/orders/utils/data.test.ts | Updates test data to include new fulfillment line fields. |
| src/orders/mutations.ts | Adds reasonReferenceId variables and maps them to API reasonReference inputs. |
| src/orders/fixtures/OrderFixture.ts | Extends fixture fulfillment/granted refund objects with reason fields. |
| src/orders/fixtures.ts | Extends order/granted refund fixtures with reason fields. |
| src/orders/components/OrderTransactionRefundPage/utils.ts | Extends line-level reason update helper to include reasonReference. |
| src/orders/components/OrderTransactionRefundPage/utils.test.ts | Updates tests to account for reasonReference in form data. |
| src/orders/components/OrderTransactionRefundPage/formDefaults.ts | Adds reasonReference defaults for create/edit line entries. |
| src/orders/components/OrderTransactionRefundPage/components/OrderTransactionRefundTable/OrderTransactionRefundTableLine.tsx | Preserves reasonReference on blur/max updates. |
| src/orders/components/OrderTransactionRefundPage/components/OrderTransactionReasonModal/OrderTransactionReasonModal.tsx | Adds reason-reference selection to the per-line reason modal. |
| src/orders/components/OrderTransactionRefundPage/OrderTransactionRefundPage.tsx | Wires per-line modal confirm to include reasonReference. |
| src/orders/components/OrderReturnPage/form.tsx | Adds reason / reasonReference fields to return form state defaults/types. |
| src/orders/components/OrderReturnPage/OrderReturnPage.tsx | Adds a refund reason reference selector and helper links in the return sidebar. |
| src/orders/components/OrderGrantRefundPage/utils.ts | Maps granted refund data to include line/global reason references. |
| src/orders/components/OrderGrantRefundPage/form.ts | Adds reasonReference to grant-refund form data defaults/types. |
| src/orders/components/OrderGrantRefundPage/OrderGrantRefundPage.tsx | Adds a reason reference selector and helper links to grant refund UI. |
| src/orders/components/OrderFulfillmentCard/OrderFulfillmentCard.tsx | Displays fulfillment reason and referenced reason type in the UI. |
| src/graphql/typePolicies.generated.ts | Regenerates Apollo type policies to include new fields/types. |
| src/graphql/hooks.generated.ts | Regenerates hooks/fragments to query reasonReference { id title } fields. |
| src/fragments/orders.ts | Extends order fragments to fetch fulfillment/granted refund line reasons and references. |
| <Link href={pageListUrl()}> | ||
| <Text color="inherit" size={2}> | ||
| {intl.formatMessage(refundReasonSelectHelperMessages.manageReasons)} | ||
| </Text> | ||
| </Link> |
There was a problem hiding this comment.
The "Manage reasons" link goes to the general models list (pageListUrl()) without filtering to the configured refund reason page type. Passing a pageTypes filter based on modelForRefundReasonRefId would make this link land the user on the relevant subset of models.
| variables: { | ||
| pageTypeId: modelForRefundReasonRefId ?? "", | ||
| }, | ||
| skip: !modelForRefundReasonRefId, |
There was a problem hiding this comment.
useModelsOfTypeQuery will run even when the modal is closed (as long as modelForRefundReasonRefId is set), which adds an unnecessary network request on every page load. Consider skipping the query when open is false (e.g. include open in the skip condition) and only fetching models when the modal is actually visible.
| skip: !modelForRefundReasonRefId, | |
| skip: !modelForRefundReasonRefId || !open, |
| <Select | ||
| options={reasonRefOptions} | ||
| value={tempReasonReference} | ||
| onChange={value => setTempReasonReference(value as string)} |
There was a problem hiding this comment.
The new reason type <Select> has no accessible label (no label/aria-label), which makes it difficult to use with screen readers. Please provide a label (or at least an aria-label) consistent with other selects in the app.
| onChange={value => setTempReasonReference(value as string)} | |
| onChange={value => setTempReasonReference(value as string)} | |
| aria-label={intl.formatMessage({ | |
| defaultMessage: "Refund reason type", | |
| id: "t63u3A", | |
| })} |
| <Select | ||
| disabled={!modelForRefundReasonRefId || loading} | ||
| options={reasonRefOptions} | ||
| value={data.reasonReference} | ||
| name="reasonReference" | ||
| onChange={value => | ||
| change({ | ||
| target: { | ||
| name: "reasonReference", | ||
| value: value as string, | ||
| }, | ||
| }) | ||
| } | ||
| /> |
There was a problem hiding this comment.
The refund reason type <Select> is missing an explicit accessible label (label prop or aria-label). Even though the card title provides visual context, screen readers typically need an associated label on the control itself.
| <Link href={pageListUrl()}> | ||
| <Text color="inherit" size={2}> | ||
| {intl.formatMessage(refundReasonSelectHelperMessages.manageReasons)} | ||
| </Text> | ||
| </Link> |
There was a problem hiding this comment.
The "Manage reasons" link currently navigates to the unfiltered models list (pageListUrl()), which may make it hard to find the configured refund-reason models. Consider passing query params to pre-filter by the configured modelForRefundReasonRefId (e.g. using the pageTypes filter) so the link matches the feature context.
| <Select | ||
| disabled={!modelForRefundReasonRefId || loading} | ||
| options={reasonRefOptions} | ||
| value={data.reasonReference} | ||
| name={"reasonReference" as keyof OrderGrantRefundFormData} | ||
| onChange={value => | ||
| change({ | ||
| target: { | ||
| name: "reasonReference", | ||
| value: value as string, | ||
| }, | ||
| }) | ||
| } | ||
| /> | ||
| )} |
There was a problem hiding this comment.
The refund reason type <Select> is rendered without a label/aria-label. Please add an accessible label so assistive technologies can identify the control (this is especially important since this select is not wrapped by the shared @dashboard/components/Select which enforces form-style labeling patterns).
| // TODO: Missing pagination, will fail if more than 100 types | ||
| const modelTypesOptions = modelsList?.pageTypes?.edges | ||
| .map(edge => edge.node) | ||
| .map(node => { | ||
| return { | ||
| value: node.id, | ||
| label: node.name, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
The TODO comment indicates a known limitation: "Missing pagination, will fail if more than 100 types". This should be addressed before merging. The query uses first: 100 which creates a hard limit. Implement proper pagination with cursor-based navigation or increase awareness of this limitation in the code.
| useEffect(() => { | ||
| setValue("returnReasonReferenceType", returnRefModelTypeId ?? ""); | ||
| }, [returnRefModelTypeId]); |
There was a problem hiding this comment.
The useEffect has a missing dependency. The effect depends on setValue which should be included in the dependency array. While setValue from useForm is stable, it's best practice to include it or use ESLint disable comment with justification.
| acc.push({ | ||
| quantity: line.quantity, | ||
| reason: line.reason, | ||
| reasonReference: line.reasonReference || undefined, |
There was a problem hiding this comment.
The reasonReference field uses || undefined which is redundant. An empty string is falsy and would already become undefined with the OR operator. However, this pattern might cause issues if reasonReference is intentionally an empty string. Consider using nullish coalescing (?? undefined) instead to only convert null/undefined values, or remove the conversion entirely if empty strings are acceptable.
| ? { | ||
| ...item, | ||
| quantity: acc[item.id].quantity + item.quantity, | ||
| reason: acc[item.id].reason || item.reason, | ||
| reasonReference: acc[item.id].reasonReference || item.reasonReference, | ||
| } |
There was a problem hiding this comment.
In the squashLines function, the logic for preserving reason fields uses acc[item.id].reason || item.reason. This means if the accumulated reason is an empty string, it will be overwritten by the new item's reason. This might not be the intended behavior - if the first item explicitly has an empty reason, it should likely be preserved. Consider using nullish coalescing (??) instead of logical OR (||) to only fall back when the value is null or undefined.
| @@ -68,6 +70,8 @@ class ReturnFormDataParser { | |||
| amountToRefund: 0, | |||
| includeShippingCosts: refundShipmentCosts, // tood: remove once removed in API | |||
There was a problem hiding this comment.
There's a typo in the TODO comment on line 71: "tood" should be "todo". While this is a minor issue, consistent spelling helps maintain code quality.
| const onSubmit = (values: FormSchema) => { | ||
| if (values.returnReasonReferenceType) { | ||
| return updateSettings({ | ||
| variables: { | ||
| returnSettingsInput: { | ||
| returnReasonReferenceType: values.returnReasonReferenceType, | ||
| }, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| if (values.returnReasonReferenceType === "") { | ||
| return clearReferenceType(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The form submission logic has inconsistent behavior. When returnReasonReferenceType is an empty string (line 135), it calls clearReferenceType(), but when it's falsy in other ways, nothing happens. Consider handling all falsy values consistently or adding an else clause to handle unexpected states.
| ...(lineReason?.reason ? { reason: lineReason.reason } : {}), | ||
| ...(lineReason?.reasonReference ? { reasonReference: lineReason.reasonReference } : {}), | ||
| } as unknown as T, |
There was a problem hiding this comment.
The conditional spreading on lines 105-106 only includes reason and reasonReference fields when they are truthy. This means empty strings will be excluded. If the API expects these fields to be explicitly set to empty string or null in certain cases, this logic might cause issues. Consider whether null or empty string should be explicitly sent, or document why truthy-only inclusion is the correct behavior.
| {hasTransactions ? ( | ||
| <> | ||
| <TransactionSubmitCard | ||
| transactions={order.transactions} | ||
| grantRefundErrors={grantRefundErrors} | ||
| sendRefundErrors={sendRefundErrors} | ||
| customRefundValue={data.amount} | ||
| autoGrantRefund={data.autoGrantRefund} | ||
| autoSendRefund={data.autoSendRefund} | ||
| refundShipmentCosts={data.refundShipmentCosts} | ||
| canRefundShipping={canRefundShipping} | ||
| shippingCosts={order?.shippingPrice?.gross} | ||
| transactionId={data.transactionId} | ||
| amountData={getReturnProductsAmountValues(order, data)} | ||
| onChange={change} | ||
| disabled={isSaveDisabled} | ||
| onSubmit={submit} | ||
| submitStatus={submitStatus} | ||
| onAmountChange={handlers.handleAmountChange} | ||
| isAmountDirty={isAmountDirty} | ||
| refundReasonConfigured={refundReasonConfigured} | ||
| orderId={order?.id ?? ""} | ||
| /> | ||
| <Box marginTop={6}> | ||
| <DashboardCard> | ||
| <DashboardCard.Header> | ||
| <DashboardCard.Title> | ||
| {intl.formatMessage({ | ||
| defaultMessage: "Return reason", | ||
| id: "6nwJUr", | ||
| })} | ||
| </DashboardCard.Title> | ||
| </DashboardCard.Header> | ||
| <DashboardCard.Content> | ||
| {modelsLoading ? ( | ||
| <Skeleton /> | ||
| ) : ( | ||
| <Select | ||
| disabled={!modelForReturnReasonRefId || loading} | ||
| options={reasonRefOptions} | ||
| value={data.reasonReference} | ||
| name="reasonReference" | ||
| onChange={value => | ||
| change({ | ||
| target: { | ||
| name: "reasonReference", | ||
| value: value as string, | ||
| }, | ||
| }) | ||
| } | ||
| /> | ||
| )} | ||
| <Box marginTop={2}> | ||
| {canManageSettings && modelForReturnReasonRefId && ( | ||
| <Link href={pageListUrl()}> | ||
| <Text color="inherit" size={2}> | ||
| {intl.formatMessage(returnReasonSelectHelperMessages.manageReasons)} | ||
| </Text> | ||
| </Link> | ||
| )} | ||
| {canManageSettings && !modelForReturnReasonRefId && ( | ||
| <Link href={returnsSettingsPath}> | ||
| <Text color="inherit" size={2}> | ||
| {intl.formatMessage( | ||
| returnReasonSelectHelperMessages.enableReasonsInSettings, | ||
| )} | ||
| </Text> | ||
| </Link> | ||
| )} | ||
| {!canManageSettings && ( | ||
| <Text color="default2" size={2}> | ||
| {intl.formatMessage(returnReasonSelectHelperMessages.noPermissionsHint)} | ||
| </Text> | ||
| )} | ||
| </Box> | ||
| <Box marginTop={4}> | ||
| <Textarea | ||
| data-test-id="refund-reason-textarea" | ||
| rows={4} | ||
| value={data.reason} | ||
| name="reason" | ||
| onChange={event => | ||
| change({ | ||
| target: { | ||
| name: "reason", | ||
| value: event.target.value, | ||
| }, | ||
| }) | ||
| } | ||
| /> | ||
| </Box> | ||
| </DashboardCard.Content> | ||
| </DashboardCard> | ||
| </Box> | ||
| </> |
There was a problem hiding this comment.
The "Return reason" card is only shown when hasTransactions is true. This creates an inconsistency where return reasons are not available for orders without transactions. Consider whether return reasons should be available in all return flows, not just those with transactions, to maintain feature consistency across different order types.
| export const ReturnsSettingsPage = () => { | ||
| const navigate = useNavigator(); | ||
| const notify = useNotifier(); | ||
| const intl = useIntl(); | ||
|
|
||
| const { loading: settingsLoading, data: returnSettingsData } = useReturnSettingsQuery(); | ||
|
|
||
| const returnRefModelTypeId = returnSettingsData?.returnSettings.reasonReferenceType?.id ?? null; | ||
|
|
||
| const { loading: modelTypesLoading, data: modelsList } = useModelTypesQuery(); | ||
|
|
||
| // TODO: Missing pagination, will fail if more than 100 types | ||
| const modelTypesOptions = modelsList?.pageTypes?.edges | ||
| .map(edge => edge.node) | ||
| .map(node => { | ||
| return { | ||
| value: node.id, | ||
| label: node.name, | ||
| }; | ||
| }); | ||
|
|
||
| const modelTypesOptionsWithEmptyValue = [ | ||
| { | ||
| value: "", | ||
| label: intl.formatMessage({ | ||
| defaultMessage: "None", | ||
| id: "450Fty", | ||
| }), | ||
| }, | ||
| ].concat(modelTypesOptions ?? []); | ||
|
|
||
| const { handleSubmit, getValues, setValue, watch } = useForm<FormSchema>({ | ||
| resolver: zodResolver(formSchema), | ||
| values: { | ||
| returnReasonReferenceType: returnRefModelTypeId ?? "", | ||
| }, | ||
| }); | ||
|
|
||
| const currentReturnReasonReferenceType = getValues("returnReasonReferenceType"); | ||
|
|
||
| const { data: exampleModelData } = useModelsOfTypeQuery({ | ||
| variables: { | ||
| pageTypeId: currentReturnReasonReferenceType, | ||
| }, | ||
| skip: !currentReturnReasonReferenceType, | ||
| }); | ||
|
|
||
| const [updateSettings] = useReturnSettingsUpdateMutation({ | ||
| onCompleted() { | ||
| notify({ | ||
| status: "success", | ||
| text: "Return reason reference type updated successfully", | ||
| }); | ||
| }, | ||
| onError(error) { | ||
| notify({ | ||
| status: "error", | ||
| title: "Failed to update return reason reference type", | ||
| text: error.message, | ||
| }); | ||
| }, | ||
| }); | ||
| const [clearReferenceType] = useReturnReasonReferenceClearMutation({ | ||
| onCompleted() { | ||
| notify({ | ||
| status: "success", | ||
| text: "Return reason reference was cleared", | ||
| }); | ||
| }, | ||
| onError(error) { | ||
| notify({ | ||
| status: "error", | ||
| title: "Failed to clear return reason reference", | ||
| text: error.message, | ||
| }); | ||
| }, | ||
| }); | ||
|
|
||
| watch("returnReasonReferenceType"); | ||
|
|
||
| const selectedTypeLabel = modelsList?.pageTypes?.edges.find( | ||
| edge => edge.node.id === currentReturnReasonReferenceType, | ||
| )?.node.name; | ||
|
|
||
| const exampleModels = exampleModelData?.pages?.edges.map(edge => edge.node) ?? []; | ||
|
|
||
| const anythingIsLoading = settingsLoading || modelTypesLoading; | ||
|
|
||
| useEffect(() => { | ||
| setValue("returnReasonReferenceType", returnRefModelTypeId ?? ""); | ||
| }, [returnRefModelTypeId]); | ||
|
|
||
| const onSubmit = (values: FormSchema) => { | ||
| if (values.returnReasonReferenceType) { | ||
| return updateSettings({ | ||
| variables: { | ||
| returnSettingsInput: { | ||
| returnReasonReferenceType: values.returnReasonReferenceType, | ||
| }, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| if (values.returnReasonReferenceType === "") { | ||
| return clearReferenceType(); | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <DetailPageLayout gridTemplateColumns={1}> | ||
| <TopNav | ||
| href={configurationMenuUrl} | ||
| title={intl.formatMessage(returnsSettingsPageMessages.pageTitle)} | ||
| /> | ||
| <DetailPageLayout.Content> | ||
| <form onSubmit={handleSubmit(onSubmit)} id="return-reason-settings-form"> | ||
| <Box display="grid" __gridTemplateColumns="1fr 2fr 1fr" gap={6} paddingX={6}> | ||
| <PageSectionHeader | ||
| title={intl.formatMessage(returnsSettingsPageMessages.explainerTitle)} | ||
| description={intl.formatMessage(returnsSettingsPageMessages.explainerContent)} | ||
| /> | ||
| <Box marginTop={6} __maxWidth="700px"> | ||
| {anythingIsLoading ? ( | ||
| <Skeleton /> | ||
| ) : ( | ||
| <> | ||
| <Text fontWeight="medium" display="block" marginBottom={2}> | ||
| {intl.formatMessage(returnsSettingsPageMessages.selectLabel)} | ||
| </Text> | ||
| <Combobox | ||
| options={modelTypesOptionsWithEmptyValue} | ||
| value={currentReturnReasonReferenceType} | ||
| onChange={value => setValue("returnReasonReferenceType", value as string)} | ||
| /> | ||
| <Box marginTop={2}> | ||
| <Text color="default2"> | ||
| {intl.formatMessage(returnsSettingsPageMessages.selectHelper)}{" "} | ||
| </Text> | ||
| <Link target={"_blank"} href={pageTypeAddUrl}> | ||
| <Text color="inherit"> | ||
| {intl.formatMessage(returnsSettingsPageMessages.createModelTypeLink)} | ||
| </Text> | ||
| </Link> | ||
| </Box> | ||
| </> | ||
| )} | ||
| </Box> | ||
| {!!currentReturnReasonReferenceType && ( | ||
| <Box marginTop={6}> | ||
| <Text fontWeight="medium" display="block" marginBottom={2}> | ||
| {intl.formatMessage(returnsSettingsPageMessages.previewTitle)}{" "} | ||
| <Link href={pageTypeUrl(currentReturnReasonReferenceType)}> | ||
| {selectedTypeLabel} | ||
| </Link> | ||
| </Text> | ||
| {exampleModels.length > 0 && ( | ||
| <Box marginTop={4}> | ||
| <Box marginTop={6} display="flex" flexDirection="column"> | ||
| {exampleModels.map(model => ( | ||
| <Text disabled key={model.id}> | ||
| - {model.title} | ||
| </Text> | ||
| ))} | ||
| </Box> | ||
| </Box> | ||
| )} | ||
| {exampleModels.length === 0 && ( | ||
| <Text> | ||
| {intl.formatMessage(returnsSettingsPageMessages.emptyModels)}{" "} | ||
| <Link | ||
| href={pageCreateUrl({ "page-type-id": currentReturnReasonReferenceType })} | ||
| > | ||
| {intl.formatMessage(returnsSettingsPageMessages.createModelLink)} | ||
| </Link> | ||
| </Text> | ||
| )} | ||
| </Box> | ||
| )} | ||
| </Box> | ||
|
|
||
| <Savebar> | ||
| <Savebar.Spacer /> | ||
| <Savebar.CancelButton onClick={() => navigate(configurationMenuUrl)} /> | ||
| <Savebar.ConfirmButton | ||
| form="return-reason-settings-form" | ||
| transitionState={anythingIsLoading ? "loading" : "default"} | ||
| disabled={anythingIsLoading} | ||
| type="submit" | ||
| /> | ||
| </Savebar> | ||
| </form> | ||
| </DetailPageLayout.Content> | ||
| </DetailPageLayout> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
The new returnsSettings module has no test coverage. Given that this module has complex form handling, API interactions with mutations and queries, and critical business logic for configuring return reasons, test coverage should be added. At minimum, add tests for the ReturnsSettingsPage component covering form submission, validation, and error handling scenarios.
| export const canGrantRefundDuringReturn = ({ | ||
| refundReasonConfigured, | ||
| }: { | ||
| refundReasonConfigured: boolean | undefined; | ||
| }): CanGrantRefund => { | ||
| if (refundReasonConfigured) { | ||
| return { | ||
| value: false, | ||
| reason: submitCardMessages.cantGrantRefundReasonRequired, | ||
| }; | ||
| } | ||
|
|
||
| return { value: true, reason: null }; | ||
| }; |
There was a problem hiding this comment.
The canGrantRefundDuringReturn function only checks if refundReasonConfigured is true, but it doesn't validate if a reason is actually provided when required. This might allow users to bypass required reason fields. Consider adding validation to ensure reasons are provided when the configuration requires them.
1 participant
Scope of the change