fix(listings): make bulk-wizard category resolve mapping-aware (#1522)#1523
Conversation
77f405b to
55e6099
Compare
piotrswierzy
left a comment
There was a problem hiding this comment.
Review — fix(listings): make bulk-wizard category resolve mapping-aware (#1522)
Verdict: APPROVE. Correct, well-scoped fix that closes a real preview/build-resolution disagreement by reusing the existing mapping seam rather than duplicating it. No blockers — one non-blocking latent-divergence note and two minor suggestions.
What it does
The bulk-offer wizard's Resolve step resolved the destination category by EAN catalogue-match only, blocking products whose EAN isn't in Allegro's catalogue but whose source category is mapped — even though OfferBuilderService builds those offers fine server-side. This threads the persisted source category ids (products.categories, #1034) FE → API DTO → core, and makes CategoryResolutionService.resolveCategoriesBatch consult the operator's configured per-source-category mapping (resolveDestinationCategory / findBySourceCategory) as a fallback on EAN no-match/no-ean, returning method: 'category_mapping' with an empty product card. EAN match stays primary.
Assessment
- Architecture — strong. The mapping fallback lives in core, reuses the same
IMappingConfigService.resolveDestinationCategorythatOfferBuilderServiceuses (no parallel path), and is capability-gated onisEanCategoryMatcher— noplatformTypeliterals. The adapter still receives only{ variantId, ean }(category-resolution.service.ts:141-143);sourceCategoryIdsis kept a core concern and never leaks across the CORE↔Integration boundary. Borrows-taxonomy destinations (Erli) degrade tono-matchand resolve server-side, unchanged. New types live in*.types.ts(BatchCategoryResolveInput/Item,EanMatchMethod), barrel exports updated, sharedLogger, noany, explicit types, DTO validated with class-validator. - Correctness — verified. The four-way branch (EAN hit ignores mapping / EAN no-match + mapping hit / no-EAN + mapping hit / no-match + no mapping) is sound, and the batch verdict now agrees with build-time resolution for every destination that exists today (see inline note for the one future-facing asymmetry). FE
resolvedProductCardIdnormalization tonullon the empty-card path is correct —selectBulkProductCardId(bulk-policy.ts:309) already treats''as falsy, so this is defensive-but-clearer. - Tests — thorough. Core spec covers all four branches and asserts the adapter receives only the EAN shape; controller spec verifies
sourceCategoryIdsforwarding +category_mappingsurfacing; FE spec verifies threading, non-blocked row, andresolutionMethod. AAA, good edge coverage. - Migrations / security — none needed. Read-only preview path; input validated at the DTO boundary. Idempotency is not in scope (this touches the pre-flight resolve query, not the bulk submit).
IMPORTANT (non-blocking)
category-resolution.service.ts:159— batch path passes empty opts ({}) totryCategoryMapping, unlike the single-offer path which threadssourceConnectionId+borrowedTaxonomy. Safe today (owns-only reachability), but a latent divergence if a future destination is both anEanCategoryMatcherand aborrowsdestination. Recommend documenting the invariant inline. See the line comment for the full trace.
SUGGESTION
- The
productCardId: ''sentinel on the mapping-fallback result relies on empty-string-as-falsy across FE (selectBulkProductCardId) and the normalization inbulk-resolve-step.tsx. It works, but a nullable/omitted card would express "no catalogue card" more directly. Minor; the empty-string convention is already established forEanMatchResult.matched. - Two spec files exist for this service (
category-resolution.service.spec.tsand__tests__/category-resolution.service.spec.ts) — pre-existing, not introduced here, but worth consolidating in a future cleanup.
Positives
Reused the existing resolution seam instead of a fork; kept the adapter contract EAN-only; capability-gated with no platform literals; preview now provably agrees with OfferBuilderService for current destinations; comprehensive tests across all three layers; clear, honest doc comments on the new interface/types explaining the fallback semantics.
| item.sourceCategoryIds && | ||
| item.sourceCategoryIds.length > 0 | ||
| ) { | ||
| const mapped = await this.tryCategoryMapping(connectionId, item.sourceCategoryIds, {}); |
There was a problem hiding this comment.
IMPORTANT (non-blocking) — latent divergence from the single-offer path. This calls tryCategoryMapping(connectionId, item.sourceCategoryIds, {}) with empty opts, whereas OfferBuilderService.resolveCategory (offer-builder.service.ts:298-307) threads sourceConnectionId: masterConnectionId and borrowedTaxonomy into the same resolveDestinationCategory.
I traced this and it is correct for every destination that exists today: the batch mapping fallback only runs past the isEanCategoryMatcher(adapter) gate (Allegro = owns taxonomy), and for an owns destination resolveDestinationCategory resolves entirely via step 1 (findBySourceCategory(destinationConnectionId, sourceCategoryId) — mapping-config.service.ts:156), which consults neither opt. The borrowed-taxonomy step 2 only fires when opts.borrowedTaxonomy is set, and borrows destinations (Erli) aren't EanCategoryMatchers so they degrade to no-match before reaching this loop.
The gap is future-facing: if a destination is ever both an EanCategoryMatcher and a borrows-taxonomy destination, this batch preview would silently skip the source-scoped borrowed reuse (#1045) and diverge from build-time resolution — reintroducing exactly the preview/build mismatch this PR fixes. The transport also can't carry sourceConnectionId, so the batch physically can't replicate the source-scoped fallback. Recommend a one-line comment here documenting why empty opts is safe (owns-only reachability), so the invariant is explicit for the next reader.
The bulk-offer wizard's Resolve step resolved the destination (Allegro) category by EAN catalogue-match only and ignored the operator's configured PS -> Allegro category mapping. A product whose EAN is not in Allegro's catalogue but whose source category IS mapped was blocked in the wizard (empty picker / "needs attention"), even though OfferBuilderService builds the offer correctly server-side using that same mapping. Make the batch resolve mapping-aware so the wizard preview agrees with build-time resolution: - Core: CategoryResolutionService.resolveCategoriesBatch now keeps EAN catalogue match as the PRIMARY path and, on an EAN no-match (or a variant with no EAN), consults the configured per-source-category mapping (the same mappingConfig.resolveDestinationCategory OfferBuilderService uses) when the item supplies sourceCategoryIds, returning a matched result with method="category_mapping" and no catalogue card. - Thread the source category through FE -> API DTO -> core: batch item gains sourceCategoryIds; ProductResponseDto/Product transport expose the persisted source category ids (#1034); the Resolve step attaches each row's product.categories and reports the mapping method. EAN match remains primary; mapping is the fallback only on no-match/no-ean. Closes #1522 Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
…ng fallback Addresses piotrswierzy's non-blocking review note on #1523: the batch resolve path calls tryCategoryMapping with empty opts, unlike the single-offer path which threads sourceConnectionId/borrowedTaxonomy. Documents why that's safe today and what would need to change if a future destination is both an EanCategoryMatcher and a borrows-taxonomy destination. Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
55e6099 to
e45eed5
Compare
|
Addressed the review feedback in e45eed5:
The two SUGGESTIONs were left as-is since you framed them as out of scope for this PR:
Also rebased onto latest |
What & why
The bulk-offer wizard's Resolve step resolved the destination (Allegro) category by EAN catalogue-match only and ignored the operator's configured PS -> Allegro category mapping. A product whose EAN is not in Allegro's catalogue but whose source category is mapped was blocked in the wizard (empty category picker, review row "needs attention"), even though
OfferBuilderServicebuilds the offer correctly server-side using that same mapping. The wizard preview and build-time resolution disagreed.This makes the batch Resolve step mapping-aware so the two agree.
Approach — mapping-aware resolve (the real fix)
Chosen over the FE-blocker-suppression fallback because the source category ids are already available end-to-end: OL persists them (
products.categories, #1034) and the domainProductcarries them; they were simply not exposed on the transport. Threading them lets the wizard show the resolved category, mirroringOfferBuilderService, rather than merely suppressing the blocker.CategoryResolutionService.resolveCategoriesBatchkeeps EAN catalogue match as the primary path. On an EANno-match(or a variant with no EAN) it consults the operator's configured per-source-category mapping via the sameIMappingConfigService.resolveDestinationCategory(findBySourceCategory) thatOfferBuilderServiceuses, when the item suppliessourceCategoryIds. A hit returns amatchedresult withmethod: 'category_mapping'and an emptyproductCardId(the offer self-links by barcode at build time). Borrows-taxonomy destinations (Erli) are unchanged — they degrade tono-matchand resolve server-side at submit.sourceCategoryIds;EanMatchResult.matchedgains an optionalmethod;ProductResponseDto/ FEProductexpose the persisted source category ids.product.categories, prefers the batch verdict (now covering both EAN and mapping paths), reportscategory_mapping, and normalizes the empty mapping card id tonull.EAN match stays primary; mapping is the fallback only on
no-match/no-ean. No CORE<->Integration boundary crossings; capability-gated, noplatformTypechecks.How tested
category-resolution.service.spec.ts): EAN hit still wins (mapping not consulted); EAN no-match + source-category mapping -> resolves viacategory_mapping; no-EAN + mapping ->category_mapping; EAN no-match + no mapping -> staysno-match.sourceCategoryIdsforwarded to the service and acategory_mappingresult surfaced.bulk-resolve-step.test.tsx): a mapped-category row (EAN no-match) is no longer blocked and carriesresolutionMethod: category_mapping.@openlinker/coretype-check + tests,@openlinker/apitype-check + affected controller specs,@openlinker/webtype-check + full test suite (one pre-existing, unrelatedsettings-page.test.tsxenv-default failure).pnpm check:invariantspasses.Closes #1522
🤖 Generated with Claude Code