[upstream #4916] feat(dashboard): Send only changed fields on detail page updates - #27
Open
ayim wants to merge 60 commits into
Open
[upstream #4916] feat(dashboard): Send only changed fields on detail page updates#27ayim wants to merge 60 commits into
ayim wants to merge 60 commits into
Conversation
The form engine (useGeneratedForm) submitted the entire mapped payload on every save, so replace-semantics fields (facetValueIds, assetIds, scalar stock config, etc.) resent their stale page-load value and could silently overwrite a concurrent edit made by another admin or via the API. Compute the set of top-level fields the user actually changed by deep-comparing the submitted values against the form baseline (plus any non-nullable input field, which is always sent), and prune update-mutation payloads to those fields in useDetailPage. RHF dirtyFields is not used as the source (array-item removal reads as clean; setValue-driven fields have no Controller). Update inputs are patch-style, so omitting untouched fields leaves them unchanged. Adds a sendAllFieldsOnUpdate opt-out for update mutations that need the full payload. Relates to OSS-567
Adds an e2e payload assertion on the collection detail page (configurable- operation filters array, no update transform) confirming that editing only the name submits just id + translations and omits the filters replace-array. Relates to OSS-567
…hboard-form-engine-resubmits-full-payload-allowing
Co-authored-by: vendure-ci-automation-bot[bot] <221402278+vendure-ci-automation-bot[bot]@users.noreply.github.com>
… data Relates to OSS-567
…low (vendurehq#4974) Co-authored-by: vendure-ci-automation-bot[bot] <221402278+vendure-ci-automation-bot[bot]@users.noreply.github.com>
…-instance, config, filters, new widgets) (vendurehq#4976) Co-authored-by: vendure-ci-automation-bot[bot] <221402278+vendure-ci-automation-bot[bot]@users.noreply.github.com>
…vements (vendurehq#4983) Co-authored-by: vendure-ci-automation-bot[bot] <221402278+vendure-ci-automation-bot[bot]@users.noreply.github.com>
Co-authored-by: vendure-ci-automation-bot[bot] <221402278+vendure-ci-automation-bot[bot]@users.noreply.github.com>
Co-authored-by: vendure-ci-automation-bot[bot] <221402278+vendure-ci-automation-bot[bot]@users.noreply.github.com>
…bmits-full-payload-allowing `minor` rewrote both conflicted e2e specs (Design System v2 migration vendurehq#4957, plus vendurehq#4983 / vendurehq#4984 / vendurehq#4993), while this branch had added test blocks to them. Took the `minor` side of each file and re-applied this branch's OSS-567 blocks on top; dropped the now-duplicate `VendureAdminClient` import, which `minor` already has. The collections block needed adapting: its hand-rolled name-field locator no longer resolved against the reworked collection detail page, so it now uses the `BaseDetailPage` page object (`formItem` / `fillInput` / `updateButton`) like every other test in that file. The assertions are unchanged. Verified: all 68 tests across collections.spec.ts, products.spec.ts and product-variants.spec.ts pass against a clean install of the merged tree.
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.
Mirrored from vendurehq#4916 for the Overwatch review demo.
Original author: @grolmus
Summary
The dashboard detail-page form engine now submits only the fields the user actually changed on update, instead of resending the entire form payload. This closes a silent data-loss vector: previously, a field left untouched by admin A would be resubmitted at its stale page-load value and could overwrite a concurrent change made by admin B (or an API integration) — no error, success toast.
Design was signed off by @michaelbromley on OSS-567 (targeting
minor). This is Tier 1 of his framing (don't send untouched fields); Tier 2 (touched replace-set element conflicts, beyond the merged stock fix) and Tier 3 (optimistic concurrency) remain separate.Root cause
useGeneratedFormmapped the whole entity into the form and calledonSubmitwith the entire payload on every save;useDetailPageforwarded it to the update mutation. For replace-semantics fields (facetValueIds,assetIds, scalar stock config,channelIds, prices, promotion conditions/actions, …) the stale value replaced whatever was there.Change
form-engine/utils.ts—deepEqual(primitives incl. NaN, Date, arrays, plain objects) andgetChangedTopLevelFields(submitted, baseline, fields): the top-level keys that differ from the baseline, plus any input field that is non-nullable (FieldInfo.nullable === false, e.g.id,UpdateShippingMethodInput.translations) which is always sent.useGeneratedForm— computeschangedFieldsby deep-comparing the raw submitted values against the form baseline (thevaluesmemo) and passes it via a new, backward-compatible 2ndmetaarg toonSubmit. RHFdirtyFieldsis deliberately not used (array-item removal reads as clean → would drop the user's own deletion;assetIds/featuredAssetIdare written viasetValuewith no Controller).useDetailPage— on the update path only, prunes the payload tochangedFieldsbeforetransformUpdateInput. Create path is unchanged. Adds asendAllFieldsOnUpdate?: booleanopt-out. Composes with the existinggetChangedStockLevelstransform (fix(dashboard): only send edited stock levels on variant update vendurehq/vendure#4834).transformUpdateInput's signature (avoids the generic-inference hazard from the OSS-554 fix).Whole-key granularity: a changed top-level key is sent in full (the entire
translationsarray,facetValueIds,customFieldsobject), preserving the "full translation objects per language" invariant.Behaviour change / extensions
Update mutations move from full-snapshot to patch-of-changed-fields for all detail pages, including third-party dashboard extensions built on
useDetailPage. Core update inputs are patch-style by design, so this is safe for core. Extensions whose custom update mutation relies on receiving unchanged fields can setsendAllFieldsOnUpdate: true.Test plan
Unit (
form-engine/utils.spec.ts, 48 pass): deepEqual (NaN, Date, nested arrays/objects); getChangedTopLevelFields — changed scalar kept, untouched pruned, array add/remove, key deletion, nested translation/customFields, non-nullable always kept, undefined-baseline↔value transitions, missing non-nullable field doesn't throw.E2E (payload assertions, the strongest proof):
catalog/product-variants.spec.ts— edit only the SKU → update input is exactly{ id, sku }(facetValueIds/assetIds/trackInventory/translations/price omitted). 11/11.catalog/products.spec.ts— edit only the name → update input is exactly{ id, translations }. 16/16.Typecheck clean; existing catalog suites green (no regression).
Relates to OSS-567.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
Tests
Chores