feat(DF-830): conditional payment amounts editor#1422
Open
Conversation
…ow gaps Three real bugs surfaced during /review with codex + claude adversarial passes: 1. mergeConditionalAmountsIntoOptions previously bailed out when state had zero conditional amounts. If a user opened the editor for a form with existing conditionalAmounts and removed them all, the save merge skipped the write and the on-disk JSON kept the old values. Distinguish "no session knowledge" (state.conditionalAmounts === undefined → preserve on-disk) from "user emptied the list" (state.conditionalAmounts === [] → write empty array so the deletion persists). 2. The POST handler reads state directly via getQuestionSessionState. If the yar session was evicted between GET and POST (Redis TTL, server restart, cookie reissue), state would be empty and saveQuestion would write a PaymentField stripped of its conditionalAmounts. Re-hydrate state.conditionalAmounts from the persisted component before merging, using the existing idempotent helper. The hydrate is a no-op when the session already has knowledge. 3. handleEnhancedActionOnPost dispatches conditional-amount actions before any state rebuild, but did not stage the form payload's questionDetails into session. So if the user typed paymentAmount/paymentDescription/etc and then clicked "Add payment amount", the typed values were lost on the redirect. Stage questionDetails into session before each conditional- amount action, mirroring the existing AddItem flow for radios. Also tighten the helper guards to use ComponentType.PaymentField instead of the string literal "PaymentField" for consistency with the rest of the codebase.
If a user opens the editor in two tabs and removes a tile in tab A, then clicks Save on the open edit form for that same tile in tab B, the editRow.id points at an item that's no longer in state.conditionalAmounts. Previously this would fall through to upsertConditionalAmount which appends a new entry, silently turning what the user thought was an edit into a fresh insert under a misleading "Payment amount 1" label. Detect the stale id explicitly (matchedIndex === -1), and in that case mint a fresh UUID and use items.length + 1 for the dynamic error label, mirroring the Add path. Adds a test pinning the behaviour.
…faults Three issues observed in local QA: 1. Save inside the inline conditional-amount form errored with "Enter item text". The radioText payload validator was firing for any enhancedAction not in [add-item, re-order] - which now includes the new conditional- amount actions. Inverted the gate to require radioText only on save-item (the radios save action). The new logic is also clearer: radioText is required if and only if the user is saving a list item. 2. The default payment amount input rendered blank instead of "0" because getPaymentAmount used a truthy check on `options.amount`, dropping the value when amount === 0. Treat 0 as a valid number and default to "0" for new components, matching the Figma. Non-zero amounts still render with two decimals via toFixed(2). 3. The "Add payment amount" button was rendering with the secondary (grey) variant. The Figma uses the GDS primary (green) action. Drop the --secondary class so it inherits the primary style consistent with the editor's other "add" buttons.
… condition select handlePrecision in lib/utils.js used a falsy check (!value) to reject undefined inputs, which also rejected the legitimate value 0. The base paymentAmountSchema now allows min(0) so amount=0 is a valid Save and continue payload, but the custom precision validator was firing 'any.required' for it (rendering as "Enter a payment amount"). Tighten the guard to only reject undefined/non-numbers and add a regression test. Also visually-hide the "Select an existing condition" label on the inline conditional-amount form. The dropdown already shows that text as the placeholder option in its collapsed state, matching the Figma. The visible label was duplicating that copy. Keep it as a screen-reader label so the field stays accessible.
The page-overview listing iterated definition.pages in their natural array order, which put the PaymentField page above Check your answers if that's how the JSON happened to be saved. Per Figma node 2262-14406, the listing should mirror the runtime flow: question pages, then Summary (Check your answers), then Payment. Sort the listed pages by a display rank (questions=0, Summary=1, Payment=2, other end pages=3), with original index breaking ties so question pages keep their authored order. pageNum still comes from the original definition.pages index so the labels (Page 1..N) stay stable.
Previously, clicking Save and continue while the inline conditional-amount
form was open would discard the in-flight draft and only emit base PaymentField
validation errors (e.g. "Enter a payment description"). Figma node 2129-44794
shows the inline conditional fields should also be validated and their errors
surfaced alongside the base errors.
Three changes:
1. New helper buildInlineConditionalAmountError(request, stateId) in
payment-conditional-amount-actions.js. Returns a Joi.ValidationError for
the in-flight inline conditional fields (errors remapped to UI ids, with
the dynamic "Payment amount {N}" label) when the edit row is expanded and
the values fail validation. Returns null otherwise.
2. failAction (Hapi payload validation fails first, e.g. empty description)
augments the validation error with inline conditional errors so they all
show together in the error summary banner.
3. The POST handler (Hapi payload passes) calls handleSaveConditionalAmount
when the inline form is still expanded. Valid inline fields auto-save as
a new tile and the save flow proceeds; invalid inline fields flash the
inline errors and redirect back to the editor with the inline form open.
Also fixes two ancillary bugs surfaced during live QA:
- The selectattr filter in payment-conditional-amount-edit.njk is Jinja2,
not Nunjucks. It was silently no-op'ing and returning the first error in
errorList for every field, so both inline fields displayed the same
message. Switched to direct lookup on formErrors.
- overrideStateIfJsEnabled in question-details.js replaced the entire session
state with only {questionType, editRow, listItems} when client JS sets
jsEnabled=true (which application.js does unconditionally). That wiped
conditionalAmounts and conditionalAmountEditRow on every validation
redirect. Spread existing state so unrelated fields survive.
Three real issues surfaced by /review on this branch's polish phase: 1. pageDisplayRank had unreachable code and missed terminal pages. isAnEndPage in @defra/forms-model is summary || payment, both already checked above the third branch — that branch was dead code, AND pages with controller: ControllerType.Terminal fell through to question-page rank, mixing exit pages with question pages in the form overview. Replace with explicit ControllerType.Terminal check; introduce named DISPLAY_RANK_* constants so the order is documentable rather than four bare integers. 2. failAction did not persist the user's typed inline conditional-amount values into session before redirecting. If the inline form was open AND a base-field validation error fired (e.g. empty paymentDescription), the user's typed conditionalAmount/Condition values were lost on the redirect because handleSaveConditionalAmount (which DOES persist them) never ran. New persistInlineConditionalAmountDraft helper writes the payload values back to state.conditionalAmountEditRow before redirect. 3. overrideStateIfJsEnabled spread preserved unrelated session state correctly for same-type-question redirects, but if the user switched questionType mid-session (e.g. PaymentField → RadiosField), then triggered a JS-enabled validation redirect, conditionalAmounts and conditionalAmountEditRow leaked into the new question-type's session bag. Now we drop carry-over when existing.questionType differs from the payload's questionType.
Adds tests for the gaps surfaced by the testing specialist in /review: - pages.test.js: mapPageData renders Summary before Payment regardless of authored array order, and pageNum stays tied to the original definition.pages index after the sort - base-settings-fields.test.js: getFieldValue returns "0" for paymentAmount=0 (DF-832 zero-bypass) and for missing amount (new component default) - base-settings-fields.test.js: radioText required only on enhancedAction=save-item; not required on add-item, re-order, add/save/cancel-conditional-amount, or absent enhancedAction (regression guard for the conditional-amount actions that the previous over-broad gate was blocking) - payment-conditional-amount-actions.test.js: handleRemoveConditionalAmount no-op paths (undefined id, unknown id) - payment-conditional-amount-actions.test.js: buildInlineConditionalAmountError cases (collapsed → null; valid payload → null; invalid payload → remapped Joi error; stale id → labelIndex = items.length+1) - payment-conditional-amount-actions.test.js: persistInlineConditionalAmountDraft writes payload values to editRow when expanded; no-op when collapsed or absent - question-details.test.js: overrideStateIfJsEnabled preserves conditionalAmounts/conditionalAmountEditRow when same questionType, drops them when type changed, no-op when jsEnabled !== "true". Wires the real session-helper read/write into the auto-mocked module via jest.requireActual so the carry-over rule can be exercised.
PaymentDomElements.values must satisfy PaymentSettings, which now requires paymentConditionalAmounts. The client-side preview reads typeable DOM inputs (paymentAmount, paymentDescription) for live updates as the user types; conditional amounts are server-side session state, so the client returns [] to satisfy the type. The actual list flows through on the next server render.
Bugs surfaced by /review specialists, Codex adversarial, and Claude adversarial passes: - Hydrate state.conditionalAmounts from the persisted component before the inline Save and continue path so a session-evicted save can no longer overwrite on-disk tiles with a single new entry. - Stage the typed base fields (paymentAmount, paymentDescription, API keys) into session before the inline-error redirect so they survive the round trip and reappear in the editor. - handleRemoveConditionalAmount no longer rewrites session state when the requested id does not match an existing tile (prevents undefined -> [] flips that would wipe disk tiles on the next save). - Validate the chosen condition exists in the form's conditions list and reject duplicate conditions across tiles. Both errors land on the conditionalAmountCondition field with friendly messages. - Cascade-clean PaymentField references when a condition is deleted, so the manager's deletion succeeds and PaymentField components no longer hold dangling condition ids. - Page list and global summary fall back to the first conditional amount when the base amount is 0 and conditional amounts exist, matching the runtime preview behaviour. - Export PAYMENT_CONDITIONAL_AMOUNTS_ANCHOR and import it in the route to remove the duplicated literal. Adds integration tests covering the four POST behaviours: enhanced action staging, Save and continue with the inline form expanded (success and failure legs), failAction merge of base + inline errors, and hydration on session eviction.
…2-designer # Conflicts: # designer/client/src/javascripts/preview/payment.js
🧪 Acceptance Test Results❌ Some acceptance tests failed |
- Add braces to single-line ifs in pageDisplayRank. - Extract getDisplayAmount helper from getPaymentInfo. - Extract getEditRowContext shared between handleSaveConditionalAmount and buildInlineConditionalAmountError. - Extract dispatchConditionalAmountAction from handleEnhancedActionOnPost. - Extract buildOverridenState from overrideStateIfJsEnabled. - Extract processInlineConditionalAmountSave from the POST handler. No behaviour change; tests + lint + types pass.
mokhld
force-pushed
the
feat/df-830-payment-v2-designer
branch
from
5c8d3da to
ce3892e
Compare
- Extract dispatchRadiosListAction from handleEnhancedActionOnPost. - Extract handleSaveQuestionError from the POST handler catch block. Pure structural extraction. No behaviour change.
- removePaymentConditionalAmountReference: PUT path, missing component, no matching condition, no conditionalAmounts at all. - formatPaymentTotal: amount-zero with first conditional fallback, amount-zero with empty list. - pageDisplayRank: Terminal page sorts after Payment. - handleSaveConditionalAmount semantic validation: unknown condition, duplicate condition, same tile keeps its own condition on edit. - condition-delete POST: cascade-cleans PaymentField references before deleting the condition.
|
There was a problem hiding this comment.
Pull request overview
Adds support for configuring and persisting conditional payment amounts on PaymentField in Editor v2, including inline editor actions, validation, and ensuring references are cleaned up when related conditions are deleted.
Changes:
- Adds/updates Editor v2 routes and actions to create/edit/remove conditional payment amount “tiles”, including session hydration/merge behavior and failAction error merging.
- Updates models to display a meaningful payment amount when the base amount is £0.00 (fallback to first conditional amount where applicable) and adjusts form overview page display ordering.
- Adds cascade cleanup when deleting a condition so any
PaymentField.options.conditionalAmounts[]references to that condition are removed before deletion.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| designer/server/src/views/forms/editor-v2/custom-templates/payment-conditional-amounts.njk | Tweaks “Add payment amount” button styling in the conditional amounts UI. |
| designer/server/src/views/forms/editor-v2/custom-templates/payment-conditional-amount-edit.njk | Switches inline edit error wiring to formErrors and hides the select label visually. |
| designer/server/src/routes/forms/editor-v2/question-details.test.js | Adds coverage for JS-enabled state overrides and PaymentField conditional-amount POST integration. |
| designer/server/src/routes/forms/editor-v2/question-details.js | Implements inline conditional-amount save interception, state hydration/merge updates, and refactors error handling. |
| designer/server/src/routes/forms/editor-v2/question-details-helper.test.js | Extends enhanced-action tests to cover conditional-amount actions staging details into session. |
| designer/server/src/routes/forms/editor-v2/question-details-helper.js | Routes conditional-amount enhanced actions via a shared dispatcher and passes definition where needed. |
| designer/server/src/routes/forms/editor-v2/payment-conditional-amount-actions.test.js | Adds tests for stale edit ids, semantic validation, draft persistence, and inline error building. |
| designer/server/src/routes/forms/editor-v2/payment-conditional-amount-actions.js | Adds semantic (definition-aware) validation, draft persistence, and improved edit-context handling. |
| designer/server/src/routes/forms/editor-v2/condition-delete.test.js | Adds test ensuring PaymentField conditional-amount references are cleaned before deleting a condition. |
| designer/server/src/routes/forms/editor-v2/condition-delete.js | Performs cascade cleanup of PaymentField conditional-amount references before condition deletion. |
| designer/server/src/models/forms/editor-v2/preview-helpers.js | Displays a fallback payment amount when the base amount is zero. |
| designer/server/src/models/forms/editor-v2/pages.test.js | Adds tests for page display order sorting and payment total fallback behavior. |
| designer/server/src/models/forms/editor-v2/pages.js | Implements display-order sorting for pages and formats Payment totals with fallback logic. |
| designer/server/src/models/forms/editor-v2/base-settings-fields.test.js | Adds regression tests for zero payment amounts and radioText requiredness gating. |
| designer/server/src/models/forms/editor-v2/base-settings-fields.js | Adjusts radioText requiredness to save-item only; ensures paymentAmount value handling supports 0. |
| designer/server/src/lib/utils.test.js | Adds test ensuring handlePrecision accepts zero. |
| designer/server/src/lib/utils.js | Updates handlePrecision to treat 0 as a valid number input. |
| designer/server/src/lib/payment-conditional-amount-helpers.test.js | Updates merge semantics tests for conditionalAmounts undefined vs empty array. |
| designer/server/src/lib/payment-conditional-amount-helpers.js | Adds state hydration from persisted component and refines merge semantics for conditional amounts. |
| designer/server/src/lib/editor.test.js | Adds tests for removing PaymentField conditional-amount references during condition deletion. |
| designer/server/src/lib/editor.js | Adds removePaymentConditionalAmountReference to update components by removing a deleted condition reference. |
| .gitignore | Ignores .gstack/ directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setQuestionSessionState(yar, stateId, hydrated) | ||
| } | ||
| } | ||
| handleSaveConditionalAmount(request, stateId) |
| * @param {string | undefined} listItemsData | ||
| * @returns {QuestionSessionState} | ||
| */ | ||
| function buildOverridenState(existing, questionType, listItemsData) { |
Comment on lines
+76
to
+85
| for (const ref of paymentFields) { | ||
| await removePaymentConditionalAmountReference( | ||
| formId, | ||
| token, | ||
| definition, | ||
| ref.pageId, | ||
| ref.componentId, | ||
| conditionId | ||
| ) | ||
| } |
Comment on lines
+118
to
+132
| /** | ||
| * @param {PaymentFieldComponent} component | ||
| * @returns {string} | ||
| */ | ||
| function formatPaymentTotal(component) { | ||
| const amount = component.options.amount | ||
| if (amount > 0) { | ||
| return formatCurrency(amount) | ||
| } | ||
| const conditional = component.options.conditionalAmounts ?? [] | ||
| if (conditional.length > 0) { | ||
| return formatCurrency(conditional[0].amount) | ||
| } | ||
| return formatCurrency(0) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a conditional payment amounts editor to PaymentField so editors can configure different charges per matched condition. All ticket flows verified.