refactor(ssr): remove the EDN snapshot round-trip; transaction edit is a plain form (heuristic 2)
The wizard serialized the whole accumulating form state into a `snapshot` hidden field (pr-str EDN + custom readers), decoded it every request, and merged step-params back in. For this single-step modal the snapshot is pure redundancy: every value is either in the entity or the live posted form. Remove it: - render: EditWizard.render-wizard renders a plain form -- no snapshot / edit-path / current-step hidden fields; a single `db/id` hidden rides in the form instead. - middleware: wrap-derive-state rebuilds :multi-form-state per request from the entity (loaded by the db/id hidden) overlaid with the live step-params, replacing wrap-init-multi-form-state + wrap-entity. The ~34 :snapshot reads are unchanged -- :snapshot is now a derived map, not a round-tripped blob. - editable fields (accounts, vendor, memo, approval, action, mode, amount-mode) come ONLY from the posted form (absent = cleared) so removing all account rows doesn't resurrect the entity's persisted accounts; only entity-only fields (db/id, client, amount, ...) come from the entity. - delete the dead initial-edit-wizard-state and render-account-grid-body. - e2e: make removeAllAccounts re-query each iteration (whole-form swaps stale a captured row index) and restore the percentage test to type-then-add ordering. Scorecard: snapshot EDN round-trip + custom readers + merge-multi-form-state -> gone (snapshot-field renders 0). Verified on a fresh server: full suite 38 pass / 1 unrelated fail, swap 6/6, transaction-edit 8/8 -- same green as before, snapshot removed.
This commit is contained in:
@@ -108,12 +108,32 @@ should ride in the form, not a parallel snapshot). It bit the percentage-split e
|
|||||||
50% then adding a row reverted the first row to its snapshot value, yielding a 66.67/33.33
|
50% then adding a row reverted the first row to its snapshot value, yielding a 66.67/33.33
|
||||||
split. Two ways it shows up and how to handle until the snapshot is gone:
|
split. Two ways it shows up and how to handle until the snapshot is gone:
|
||||||
|
|
||||||
**Fixed (Stage 1 of the wizard→plain-form rewrite):** the operation handlers now read the
|
**Fixed (Stage 1):** the operation handlers read the live `:step-params` rows (already
|
||||||
live `:step-params` rows (already schema-decoded by `mm/wrap-wizard`, so typed) and fall
|
schema-decoded by `mm/wrap-wizard`) so typed values survive add/remove/toggle.
|
||||||
back to the snapshot only when absent — typed values survive add/remove/toggle. The
|
|
||||||
percentage e2e was restored to the realistic type-then-add ordering as a regression guard.
|
**Done (Stage 2 — the snapshot round-trip is gone).** The EDN `snapshot` hidden field +
|
||||||
The *full* removal (no snapshot at all; the posted form *is* the state) is the remaining
|
custom readers + `merge-multi-form-state` are removed. A `db/id` hidden rides in the form;
|
||||||
rewrite stages.
|
`wrap-derive-state` rebuilds `:multi-form-state` per request from `entity ∪ step-params`,
|
||||||
|
and `EditWizard.render-wizard` renders a plain form (no snapshot/edit-path/current-step
|
||||||
|
hiddens). The ~34 `:snapshot` reads still work — `:snapshot` is now a derived map, not a
|
||||||
|
round-tripped blob.
|
||||||
|
|
||||||
|
**Trap that cost hours — derive `entity ∪ step-params` correctly.** First cut was
|
||||||
|
`(merge base step-params)`. Bug: `base` always carries the entity's *persisted* accounts,
|
||||||
|
so after the user removes every row (step-params has no accounts key) the merge falls back
|
||||||
|
to base → the persisted accounts **resurrect** on the next operation. Fix: editable fields
|
||||||
|
(accounts, vendor, memo, approval, action, mode, amount-mode) come **only** from the live
|
||||||
|
form (absent = cleared); only entity-only fields (`db/id`, client, amount, description,
|
||||||
|
status, type) come from the entity. Lesson: with a posted form, "field absent" means
|
||||||
|
*cleared*, not "use the persisted value" — never merge the entity's editable fields back in.
|
||||||
|
|
||||||
|
**Verify the snapshot removal on a FRESH server, and don't trust a long-lived in-process
|
||||||
|
test server.** Protocol/defrecord (`EditWizard.render-wizard`) and middleware reloads do
|
||||||
|
**not** fully take in a running REPL — the server kept rendering the old snapshot field
|
||||||
|
after `:reload`, and an in-process server that isn't reseeded between `npx playwright`
|
||||||
|
invocations accumulates state that makes order-dependent tests flake. Both produced hours
|
||||||
|
of phantom failures. Restart the REPL clean (or reseed) before trusting an e2e result; CI
|
||||||
|
boots a fresh server per run, so the fresh-server number (38 pass / 1 unrelated) is the real one.
|
||||||
|
|
||||||
## Characterization tests rot against table order and removed wizard chrome
|
## Characterization tests rot against table order and removed wizard chrome
|
||||||
|
|
||||||
|
|||||||
@@ -39,7 +39,7 @@ Each migration appends one row (after-numbers), referencing the before in the di
|
|||||||
| Phase | Modal | LOC | Routes | no-cursor twins | faked roots | snapshot merges | OOB | mixed hx- | cookbook reused / added |
|
| Phase | Modal | LOC | Routes | no-cursor twins | faked roots | snapshot merges | OOB | mixed hx- | cookbook reused / added |
|
||||||
|-------|-------|-----|--------|-----------------|-------------|-----------------|-----|-----------|-------------------------|
|
|-------|-------|-----|--------|-----------------|-------------|-----------------|-----|-----------|-------------------------|
|
||||||
| 1 (baseline) | Transaction Edit `transaction/edit.clj` | 1608 | ~12 | 1 | 2 | ~75 | 0 | 8 | — / seeded 7 entries |
|
| 1 (baseline) | Transaction Edit `transaction/edit.clj` | 1608 | ~12 | 1 | 2 | ~75 | 0 | 8 | — / seeded 7 entries |
|
||||||
| 2 (in progress) | Transaction Edit `transaction/edit.clj` | 1545 | **~5** | **0** | **0** | ~75 | 0 | 8 | — / 0 |
|
| 2 | Transaction Edit `transaction/edit.clj` | 1584 | **~5** | **0** | **0** | **0 round-trip** | 0 | 8 | — / 0 |
|
||||||
|
|
||||||
> **Phase 2 progress.** Achieved with parity held (swap spec **6/6**, transaction-edit
|
> **Phase 2 progress.** Achieved with parity held (swap spec **6/6**, transaction-edit
|
||||||
> spec **8/8**, full suite **38 pass / 1 unrelated fail / 0 skip**, up from 30/2/7):
|
> spec **8/8**, full suite **38 pass / 1 unrelated fail / 0 skip**, up from 30/2/7):
|
||||||
|
|||||||
@@ -124,14 +124,13 @@ async function getAccountLocation(page: any, rowIndex: number): Promise<string>
|
|||||||
}
|
}
|
||||||
|
|
||||||
async function removeAllAccounts(page: any) {
|
async function removeAllAccounts(page: any) {
|
||||||
const accountRows = page.locator('#account-grid-body tbody tr.account-row');
|
// Re-query each iteration: every remove is a whole-form swap that re-renders the rows,
|
||||||
const rowCount = await accountRows.count();
|
// so a row index captured up front goes stale. Click the last remove button until none
|
||||||
|
// remain.
|
||||||
for (let i = rowCount - 1; i >= 0; i--) {
|
for (let guard = 0; guard < 20; guard++) {
|
||||||
const row = accountRows.nth(i);
|
const removeButtons = page.locator('#account-grid-body .account-remove-action');
|
||||||
const removeButton = row.locator('.account-remove-action');
|
if (await removeButtons.count() === 0) break;
|
||||||
await removeButton.click();
|
await removeButtons.last().click();
|
||||||
// Wait for the Alpine.js removal animation (500ms + buffer)
|
|
||||||
await page.waitForTimeout(700);
|
await page.waitForTimeout(700);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -773,7 +773,9 @@
|
|||||||
[:div.mt-4 {:hx-post (bidi/path-for ssr-routes/only-routes ::route/unlink-payment)
|
[:div.mt-4 {:hx-post (bidi/path-for ssr-routes/only-routes ::route/unlink-payment)
|
||||||
:hx-trigger "unlinkPayment"
|
:hx-trigger "unlinkPayment"
|
||||||
:hx-target "#payment-matches"
|
:hx-target "#payment-matches"
|
||||||
:hx-include "this"
|
;; include the whole form so the db/id hidden rides along (the plain
|
||||||
|
;; form derives state from db/id instead of a serialized snapshot)
|
||||||
|
:hx-include "closest form"
|
||||||
:hx-swap "outerHTML"
|
:hx-swap "outerHTML"
|
||||||
:hx-confirm "Are you sure you want to unlink this payment?"}
|
:hx-confirm "Are you sure you want to unlink this payment?"}
|
||||||
|
|
||||||
@@ -1327,15 +1329,20 @@
|
|||||||
(if current-step
|
(if current-step
|
||||||
(mm/get-step this current-step)
|
(mm/get-step this current-step)
|
||||||
(mm/get-step this :links)))
|
(mm/get-step this :links)))
|
||||||
(render-wizard [this {:keys [multi-form-state] :as request}]
|
(render-wizard [this {:keys [multi-form-state form-errors] :as request}]
|
||||||
(mm/default-render-wizard
|
;; Plain-form render: no snapshot / edit-path / current-step hidden fields. The entity
|
||||||
this request
|
;; id rides in the form; all other state is the live form (step-params) re-derived
|
||||||
:form-params
|
;; against the entity on each request (see wrap-derive-state).
|
||||||
(-> mm/default-form-props
|
(let [step (mm/get-current-step this)]
|
||||||
(assoc :hx-post
|
[:form#wizard-form (-> mm/default-form-props
|
||||||
(str (bidi/path-for ssr-routes/only-routes ::route/edit-submit))
|
(assoc :hx-post (str (bidi/path-for ssr-routes/only-routes ::route/edit-submit))
|
||||||
:hx-ext "response-targets"))
|
:hx-ext "response-targets"))
|
||||||
:render-timeline? false))
|
(fc/start-form multi-form-state (when form-errors {:step-params form-errors})
|
||||||
|
(list
|
||||||
|
(com/hidden {:name "db/id" :value (-> multi-form-state :snapshot :db/id)})
|
||||||
|
(fc/with-field :step-params
|
||||||
|
(com/modal {:id "wizardmodal"}
|
||||||
|
(mm/render-step step request)))))]))
|
||||||
(steps [_]
|
(steps [_]
|
||||||
[:links])
|
[:links])
|
||||||
(get-step [this step-key]
|
(get-step [this step-key]
|
||||||
@@ -1350,32 +1357,53 @@
|
|||||||
|
|
||||||
(def edit-wizard (->EditWizard nil nil))
|
(def edit-wizard (->EditWizard nil nil))
|
||||||
|
|
||||||
(defn initial-edit-wizard-state [request]
|
(defn entity->base
|
||||||
(let [tx-id (-> request :route-params :db/id)
|
"The persisted transaction, shaped like the form's base state (what the old snapshot was
|
||||||
entity (dc/pull (dc/db conn)
|
seeded with). The plain form derives its state fresh from this + the live posted form,
|
||||||
'[:db/id
|
instead of round-tripping an EDN snapshot hidden field."
|
||||||
:transaction/vendor
|
[tx-id]
|
||||||
:transaction/client
|
(-> (dc/pull (dc/db conn)
|
||||||
:transaction/description-original
|
'[:db/id
|
||||||
:transaction/status
|
:transaction/vendor
|
||||||
:transaction/type
|
:transaction/client
|
||||||
:transaction/memo
|
:transaction/description-original
|
||||||
{[:transaction/approval-status :xform iol-ion.query/ident] [:db/ident]}
|
:transaction/status
|
||||||
:transaction/amount
|
:transaction/type
|
||||||
:transaction/accounts]
|
:transaction/memo
|
||||||
tx-id)
|
{[:transaction/approval-status :xform iol-ion.query/ident] [:db/ident]}
|
||||||
entity (-> entity
|
:transaction/amount
|
||||||
(update :transaction/vendor :db/id)
|
:transaction/accounts]
|
||||||
(update :transaction/client :db/id))]
|
tx-id)
|
||||||
(mm/->MultiStepFormState entity
|
(update :transaction/vendor :db/id)
|
||||||
[]
|
(update :transaction/client :db/id)))
|
||||||
entity)))
|
|
||||||
|
|
||||||
(defn- render-account-grid-body [request]
|
(defn wrap-derive-state
|
||||||
(fc/start-form (:multi-form-state request) nil
|
"Plain-form replacement for the EDN-snapshot round-trip. Builds :multi-form-state from
|
||||||
(fc/with-field :step-params
|
the entity (loaded by the db/id hidden field, or the route on initial open) overlaid
|
||||||
(fc/with-field :transaction/accounts
|
with the live posted step-params -- no serialized snapshot. Runs after wrap-decode /
|
||||||
(account-grid-body* request)))))
|
wrap-wizard, which provide nested + schema-typed step-params. The 30-odd `:snapshot`
|
||||||
|
reads keep working: snapshot is now `entity ∪ step-params`, derived per request."
|
||||||
|
[handler]
|
||||||
|
(fn [request]
|
||||||
|
(let [tx-id (->db-id (or (some-> request :form-params (get "db/id"))
|
||||||
|
(-> request :route-params :db/id)))
|
||||||
|
base (entity->base tx-id)
|
||||||
|
posted (-> request :multi-form-state :step-params)
|
||||||
|
;; Fields the form does NOT edit always come from the entity. Everything else is
|
||||||
|
;; the live posted form, which is authoritative even when ABSENT -- an absent
|
||||||
|
;; field means the user cleared it (e.g. removed all account rows), not "fall
|
||||||
|
;; back to the entity's persisted value". Merging base's editable fields back in
|
||||||
|
;; would resurrect persisted accounts after a remove-all.
|
||||||
|
entity-only (select-keys base [:db/id :transaction/client :transaction/amount
|
||||||
|
:transaction/description-original
|
||||||
|
:transaction/status :transaction/type])
|
||||||
|
;; On initial open there is no posted form -> render the entity. On every post
|
||||||
|
;; the form is authoritative for the editable fields.
|
||||||
|
step-params (if (seq posted) posted base)
|
||||||
|
snapshot (if (seq posted) (merge entity-only posted) base)]
|
||||||
|
(handler (-> request
|
||||||
|
(assoc :entity (d-transactions/get-by-id tx-id))
|
||||||
|
(assoc :multi-form-state (mm/->MultiStepFormState snapshot [] step-params)))))))
|
||||||
|
|
||||||
(defn render-full-form
|
(defn render-full-form
|
||||||
"Helper to render the complete transaction edit form for whole-form re-rendering."
|
"Helper to render the complete transaction edit form for whole-form re-rendering."
|
||||||
@@ -1518,18 +1546,18 @@
|
|||||||
(apply-middleware-to-all-handlers
|
(apply-middleware-to-all-handlers
|
||||||
{::route/edit-wizard (-> mm/open-wizard-handler
|
{::route/edit-wizard (-> mm/open-wizard-handler
|
||||||
(wrap-must {:activity :edit :subject :transaction} (fn get-client [request] (-> request :entity :transaction/client)))
|
(wrap-must {:activity :edit :subject :transaction} (fn get-client [request] (-> request :entity :transaction/client)))
|
||||||
|
(wrap-derive-state)
|
||||||
(mm/wrap-wizard edit-wizard)
|
(mm/wrap-wizard edit-wizard)
|
||||||
(mm/wrap-init-multi-form-state initial-edit-wizard-state)
|
(mm/wrap-decode-multi-form-state)
|
||||||
(wrap-entity [:route-params :db/id] d-transactions/default-read)
|
|
||||||
(wrap-schema-enforce :route-schema [:map [:db/id entity-id]]))
|
(wrap-schema-enforce :route-schema [:map [:db/id entity-id]]))
|
||||||
::route/edit-wizard-navigate (-> mm/next-handler
|
::route/edit-wizard-navigate (-> mm/next-handler
|
||||||
(wrap-must {:activity :edit :subject :transaction} (fn get-client [request] (-> request :entity :transaction/client)))
|
(wrap-must {:activity :edit :subject :transaction} (fn get-client [request] (-> request :entity :transaction/client)))
|
||||||
(wrap-entity [:multi-form-state :snapshot :db/id] d-transactions/default-read)
|
(wrap-derive-state)
|
||||||
(mm/wrap-wizard edit-wizard)
|
(mm/wrap-wizard edit-wizard)
|
||||||
(mm/wrap-decode-multi-form-state))
|
(mm/wrap-decode-multi-form-state))
|
||||||
::route/edit-submit (-> mm/submit-handler
|
::route/edit-submit (-> mm/submit-handler
|
||||||
(wrap-must {:activity :edit :subject :transaction} (fn get-client [request] (-> request :entity :transaction/client)))
|
(wrap-must {:activity :edit :subject :transaction} (fn get-client [request] (-> request :entity :transaction/client)))
|
||||||
(wrap-entity [:multi-form-state :snapshot :db/id] d-transactions/default-read)
|
(wrap-derive-state)
|
||||||
(mm/wrap-wizard edit-wizard)
|
(mm/wrap-wizard edit-wizard)
|
||||||
(mm/wrap-decode-multi-form-state))
|
(mm/wrap-decode-multi-form-state))
|
||||||
::route/location-select (-> location-select
|
::route/location-select (-> location-select
|
||||||
@@ -1540,16 +1568,14 @@
|
|||||||
[:account-id {:optional true}
|
[:account-id {:optional true}
|
||||||
[:maybe entity-id]]]))
|
[:maybe entity-id]]]))
|
||||||
::route/edit-form-changed (-> edit-form-changed-handler
|
::route/edit-form-changed (-> edit-form-changed-handler
|
||||||
|
(wrap-derive-state)
|
||||||
(mm/wrap-wizard edit-wizard)
|
(mm/wrap-wizard edit-wizard)
|
||||||
(wrap-entity [:multi-form-state :snapshot :db/id] d-transactions/default-read)
|
|
||||||
(mm/wrap-decode-multi-form-state))
|
(mm/wrap-decode-multi-form-state))
|
||||||
::route/unlink-payment (-> unlink-payment
|
::route/unlink-payment (-> unlink-payment
|
||||||
(wrap-must {:activity :edit :subject :transaction} (fn get-client [request] (-> request :entity :transaction/client)))
|
(wrap-must {:activity :edit :subject :transaction} (fn get-client [request] (-> request :entity :transaction/client)))
|
||||||
(wrap-entity [:multi-form-state :snapshot :db/id] d-transactions/default-read)
|
(wrap-derive-state)
|
||||||
(mm/wrap-wizard edit-wizard)
|
(mm/wrap-wizard edit-wizard)
|
||||||
(mm/wrap-decode-multi-form-state)
|
(mm/wrap-decode-multi-form-state))}
|
||||||
#_(wrap-schema-enforce :form-schema
|
|
||||||
save-schema))}
|
|
||||||
(fn [h]
|
(fn [h]
|
||||||
(-> h
|
(-> h
|
||||||
(wrap-client-redirect-unauthenticated)))))
|
(wrap-client-redirect-unauthenticated)))))
|
||||||
|
|||||||
Reference in New Issue
Block a user