fix(core): Assign new variants to all product channels#4699
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughProduct variant creation now assigns new variants to additional channels already linked to the parent product, including related option groups and options. The E2E suite verifies channel visibility, pricing, stock, assets, option relationships, and permission-denied rollback behavior. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/service/services/product-variant.service.ts (1)
506-542: ⚡ Quick winBatch the per-channel assignments instead of looping per channel.
channelService.assignToChannels(ctx, EntityType, entityId, channelIds)accepts an array of channel IDs and batches them in a single relation operation, so the variant / option-group / option assignments can each be issued once withadditionalChannelIdsinstead of looping. The method deduplicates automatically, making this safe. OnlycreateOrUpdateProductVariantPriceneeds to stay per-channel because each channel may have its owndefaultCurrencyCode. This reduces DB round-trips on variant creation when a product spans many channels and/or has many options.♻️ Proposed batched form
- for (const additionalChannelId of additionalChannelIds) { - const channel = await this.connection.getEntityOrThrow( - ctx, - Channel, - additionalChannelId, - ); - await this.channelService.assignToChannels( - ctx, - ProductVariant, - createdVariant.id, - [additionalChannelId], - ); - await this.createOrUpdateProductVariantPrice( - ctx, - createdVariant.id, - input.price, - additionalChannelId, - channel.defaultCurrencyCode, - ); - // Also assign option groups and options to the target channel - for (const groupId of optionGroupIds) { - await this.channelService.assignToChannels( - ctx, - ProductOptionGroup, - groupId, - [additionalChannelId], - ); - } - for (const optionId of optionIds) { - await this.channelService.assignToChannels( - ctx, - ProductOption, - optionId, - [additionalChannelId], - ); - } - } + // Assign the variant to all additional channels in one call + await this.channelService.assignToChannels( + ctx, + ProductVariant, + createdVariant.id, + additionalChannelIds, + ); + // Assign option groups and options to all additional channels in one call each + for (const groupId of optionGroupIds) { + await this.channelService.assignToChannels( + ctx, + ProductOptionGroup, + groupId, + additionalChannelIds, + ); + } + for (const optionId of optionIds) { + await this.channelService.assignToChannels( + ctx, + ProductOption, + optionId, + additionalChannelIds, + ); + } + // Per-channel price creation (each channel can have its own defaultCurrencyCode) + const additionalChannels = await this.connection + .getRepository(ctx, Channel) + .find({ where: { id: In(additionalChannelIds) } }); + for (const channel of additionalChannels) { + await this.createOrUpdateProductVariantPrice( + ctx, + createdVariant.id, + input.price, + channel.id, + channel.defaultCurrencyCode, + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/service/services/product-variant.service.ts` around lines 506 - 542, Replace the per-channel loops with batched assignments: call this.channelService.assignToChannels once for the createdVariant (ProductVariant) with the full additionalChannelIds array, once for all option groups (ProductOptionGroup) using optionGroupIds, and once for all options (ProductOption) using optionIds; keep the per-channel loop only for createOrUpdateProductVariantPrice since it needs each channel's defaultCurrencyCode (use this.connection.getEntityOrThrow to fetch each channel inside that loop to read channel.defaultCurrencyCode). This removes the repeated assignToChannels calls while preserving per-channel price creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/service/services/product-variant.service.ts`:
- Around line 506-542: Replace the per-channel loops with batched assignments:
call this.channelService.assignToChannels once for the createdVariant
(ProductVariant) with the full additionalChannelIds array, once for all option
groups (ProductOptionGroup) using optionGroupIds, and once for all options
(ProductOption) using optionIds; keep the per-channel loop only for
createOrUpdateProductVariantPrice since it needs each channel's
defaultCurrencyCode (use this.connection.getEntityOrThrow to fetch each channel
inside that loop to read channel.defaultCurrencyCode). This removes the repeated
assignToChannels calls while preserving per-channel price creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 608c97d3-9894-4aed-a313-89d4bb95593e
📒 Files selected for processing (2)
packages/core/e2e/product-channel.e2e-spec.tspackages/core/src/service/services/product-variant.service.ts
grolmus
left a comment
grolmus
left a comment
There was a problem hiding this comment.
Thanks for tackling this, @Ryrahul — the bug in #4532 is real and your e2e nicely captures the symptom. Before this can go into core I'd like to see it reworked to build on the existing machinery rather than adding a parallel path.
Reuse assignProductVariantsToChannel. ProductVariantService.assignProductVariantsToChannel already handles everything a new variant needs when it enters another channel: the permission check (UpdateCatalog on the target channel), correct price derivation from the channel's pricesIncludeTax plus priceFactor, defaultCurrencyCode, stock-level seeding, asset assignment, and the ProductVariantChannelEvent. ProductService.assignProductsToChannel is the model for also propagating option groups/options. The current inline implementation in createSingle reproduces a subset of this and misses several pieces:
- Pricing: it writes the raw
input.priceas the net price into every channel, ignoring tax-inclusive channels, per-channel currency, andpriceFactor. Please derive the price the same wayassignProductVariantsToChanneldoes. - Permissions: there's no check that the user may write to the other channels — please don't bypass
userHasPermissionOnChannel. As written, a user with catalog rights only on the current channel could cause writes into channels they can't administer. - Events: no
ProductVariantChannelEventis published, so search indexing / plugins won't learn the variant now exists in the other channels — meaning the variant may not actually become searchable there (i.e. the fix may not fully deliver on its goal). - Stock & assets: no stock-level seeding (see #4864) and no asset assignment for the new channels, so
stockLevelsresolves to[]and a featured asset is invisible in the newly-covered channel.
Concretely, after the variant is created you should be able to compute the product's "other" channel IDs and route them through the existing assign method (or a shared helper extracted from it) rather than hand-rolling the loop.
A couple of practical notes:
- The branch currently conflicts with
masterand needs a rebase —createSinglehas changed around the price-seeding block. - Once pricing goes through the canonical path, the
priceWithTax === 2000assertion will need recomputing, and please double-check the query fragment actually selectscurrencyCode. - Could you extend the e2e to cover stock levels + assets in the assigned channel, a
priceFactor/ tax-inclusive pricing case, and a negative-permission case?
Happy to help map out the refactor if useful — thanks again for the contribution.
|
Thanks @grolmus for the review !. Will address the mentioned changes 👐 |
Ryrahul
force-pushed
the
fix/channel-product-variant-assingment
branch
from
ac938da to
acf8a0c
Compare
Ryrahul
force-pushed
the
fix/channel-product-variant-assingment
branch
from
acf8a0c to
5d1e62d
Compare
grolmus
left a comment
grolmus
left a comment
There was a problem hiding this comment.
Excellent rework, @Ryrahul — this now builds on assignProductVariantsToChannel exactly as hoped, so pricing (incl. tax-inclusive channels), permissions, stock seeding, assets and ProductVariantChannelEvent all come for free and stay consistent with ProductService.assignProductsToChannel. The option-group/option propagation is the right call, and the @Transaction() wrapper means a ForbiddenError on a target channel cleanly rolls back the create — nicely covered by the new negative-permission test. The extended e2e (tax-inclusive price, stock, assets, options, permissions) is exactly what I was hoping for.
A few notes — one worth tightening before I approve, the rest optional:
- Stock test: it asserts
stockOnHand === 0, which is what you'd get whether or not aStockLevelrow was actually seeded for the channel. Could you assert on the channel-filteredstockLevelsarray (its length / entry) instead? That's what actually provesensureStockLevelsForChanneldid its job — the behaviour this test is meant to guard. - Optional: the comment around the price test says "priceWithTax (2000)" — 2000 is the net price; the gross is 2400 (which the assertion correctly checks). Just the wording.
- Optional: after the permission failure, assert that no orphaned
CHAN-VAR-GREENvariant exists. The@Transaction()already guarantees the rollback, but it makes the intent explicit.
Once the stock assertion is tightened I'm happy to approve — the implementation itself looks great.
Address review feedback on the new-variant channel-assignment tests: - Stock test now asserts on the channel-filtered `stockLevels` array (queried from the target channel) instead of `stockOnHand === 0`. This proves ensureStockLevelsForChannel actually seeded a StockLevel row — a check that `stockOnHand === 0` alone could not distinguish from "no row seeded". The test now assigns the default stock location to the target channel first, so seeding has a location to write to. - Add an explicit assertion that no orphaned variant remains after a permission-denied create, making the @transaction() rollback intent explicit. - Clarify the tax-inclusive price comment (2000 is net; 2400 is gross). Relates to vendurehq#4532
Hi @grolmus, I've made the requested changes. Could you please review it again when you have a moment? Thanks for your time! |
2 participants
Fixes #4532
Description
When a new product variant is created after a channel has already been assigned to the product, the new variant does not appear in that channel. This is because
createSingleinProductVariantServiceonly assigns the variant to the current channel + default channel viaassignToCurrentChannel(), without checking what other channels the parent product belongs to.Root Cause
There is an asymmetry in channel assignment:
assignProductsToChannelassigns the product AND all its existing variants, option groups, and options to the target channel.createSingle(variant creation) only assigns the new variant toctx.channelId+ default channel — ignoring any other channels the parent product is already assigned to.Fix
After creating the variant and its prices for the current/default channel, we now:
getRepository().findOne()withrelations: ['channels']andrelationLoadStrategy: 'query'— same pattern asassignProductsToChannel).channelService.assignToChannels().ProductVariantPriceviacreateOrUpdateProductVariantPrice().channelService.assignToChannels()— matching whatassignProductsToChanneldoes.All methods used are existing public APIs already called elsewhere in the same file. No new patterns introduced.
Price handling
The new variant's price in additional channels uses
input.pricedirectly (priceFactor = 1). The originalpriceFactorfrom the initialassignProductsToChannelcall is a one-time conversion factor that is not stored anywhere, so it cannot be retroactively applied to new variants. The admin can adjust channel-specific prices after creation. This is strictly better than the current behavior where the variant is completely invisible in the other channel.Option/OptionGroup handling
When a new option is created after the product was assigned to a channel (e.g. adding a new color),
ProductOptionService.create()only assigns it to the current + default channel viaassignToCurrentChannel(). The fix also assigns the new variant's options and their option groups to the additional channels, preventingCannot return null for non-nullable field ProductOption.grouperrors.Scope
This fix is specific to
ProductVariantcreation. The same gap exists for other parent-child relationships:All of these use the same
assignToCurrentChannel()pattern on creation without checking the parent's channels.Question for maintainers: Would you prefer a common helper/pattern that handles this for all parent-child relationships, or is scoping to
ProductVariantsufficient for now?Breaking changes
None. This is purely additive — new variants now get assigned to more channels than before. Existing behavior for variants created in products that are only in the default channel is unchanged (the additional channels list is empty, so no extra work is done).
Checklist
Need help on this PR? Tag
@codesmithwith what you need.