Repository navigation
Make the CSV importer's variant-axis checks deterministic - #12
Open
eastagiletracker wants to merge 1 commit into
Open
eastagiletracker wants to merge 1 commit into
eastagiletracker wants to merge 1 commit into
Conversation
Checks 32 to 34 of test:csv expected a Size: Small/Medium import to be rejected as an unsupported variant axis value. On a clean database it succeeds: seed:pk creates the "size" axis through createProductAttributesWorkflow, so it carries a product_option_id, and Mercur 2.3's createProductAttributeValuesWorkflow mirrors any new value onto that option. The rejection only happened on databases whose Size values predated the mirroring, which is why the checks passed locally and failed from scratch. The condition that decides it is the axis, not the value: an attribute with no mirrored product option (one created through the module service) can never have a usable value, and the importer refuses those rows. The rejection checks now build exactly that axis per run, guard the precondition with its own check, and delete the axis afterwards. The assertions themselves are unchanged. The Size import is kept as the supported case and now asserts what it should: one product, two variants carrying the uploaded sizes, two offers at the CSV price. Proven able to fail: letting the importer use unmirrored values makes the product workflow throw "Product has 0 option values"; offering only the first variant turns the one-product and per-variant-offer checks red; swapping variant SKUs turns the size check red; creating the test axis through the workflow instead reproduces the old failure and the precondition check names it. With the suite deterministic it gates CI again, and the known issue and the assertion counts in the docs are updated to match.
This branch has not been deployed
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.
This PR proposes making the CSV importer's variant-axis checks deterministic, which closes known issue 3 in
docs/known-issues.mdand letstest:csvgate CI again. We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/1129. You can sign in with your GitHub ID to claim ownership of the project.What this changes
Checks 32 to 34 expected a
Size: Small/Mediumimport to be rejected as an unsupported axis value. On a clean database it succeeds, becauseseed:pkcreates thesizeaxis throughcreateProductAttributesWorkflow, so it carries aproduct_option_id, and Mercur 2.3'screateProductAttributeValuesWorkflowmirrors any new value onto that option. So whether a value can be used depends on the axis, not on the value. If the attribute has a mirrored product option, new values work. If it has none (the attribute was created through the module service), no value on it can ever be used, and the importer correctly refuses those rows. The old checks only passed on databases whoseSizevalues predated the mirroring.The rejection checks now set up that condition themselves. Each run creates its own unmirrored axis (
Fit <stamp>, not filterable) through the module service. A new check guards the precondition (product_option_idis null), the three original assertions run against that axis unchanged, and the axis is deleted in afinally. TheSizeimport stays in as the supported case and now asserts what actually happens: one product, two variants carrying the uploaded sizes, two offers for the seller at the CSV price. The importer's "KNOWN LIMITATION" comment is rewritten to describe the real condition (no logic change). Because the suite is now deterministic,continue-on-erroris removed from the CI step. The known-issues entry is moved to a one-line limitation under "Catalogue import", and the assertion counts (229 to 233, CSV 34 to 38) are updated wherever the docs quote them.How it was verified
Reproduced on
mainat e02290e against a fresh Postgres 16 + Redis 7, migrated and seeded exactly asci.ymldoes:With this branch I built a fresh database, ran migrate,
seed:pk,seed:commissionsandseed:digital, then ran every suite in CI order.test:settlement34,test:payout26,test:csv38,test:shopify29,test:otp22,test:confirmationandtest:digitalall passed.test:csvalso passed 38/38 on a second run against the same accumulated database.npx tsc --noEmitinpackages/apiis clean. The baseline failure set was exactly the three checks above, so nothing new went red.Proof the tests can fail
ensureVariantAxestreat unmirrored values as usable: the product workflow throwsProduct has 0 option values but there were 1 provided option values for the variant: Slim.and the suite goes red.offers=1).{"KRT-…-M":["Small"],"KRT-…-S":["Medium"]}).createProductAttributesWorkflowinstead: check 35 goes red (product_option_id=opt_…) and 36 to 38 reproduce the original failure. If the precondition ever drifts, the guard reports it.How this was managed
We imported your repo's pull requests and history into a board at https://eastagiletracker.com/projects/1129 (11 stories) and tracked this work as https://eastagiletracker.com/projects/1129/stories/1340816.
If you'd rather not receive contributions like this, reply
no-more-prson this pull request and we won't open any further ones on your repositories.Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com