Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(product-duplicator): Copy prices in product duplicator #2900

Merged

Conversation

mschipperheyn
Copy link
Collaborator

Description

When you copy products using the product-duplicator all variants get a price of 0. This PR fixes that by also copying the prices.

I'm not sure if the original behavior was intentional or a bug. I considered it the latter. But if it was intentional, I can create an additional flag to ensure backward. compatibility.

Breaking changes

  • Copied product variant prices are no longer 0

Checklist

📌 Always:

  • [ x] I have set a clear title
  • [ x] My PR is small and contains a single feature
  • [ x] I have checked my own PR

👍 Most of the time:

  • [ x] I have added or updated test cases
  • I have updated the README if needed

Copy link

vercel bot commented Jun 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Jun 18, 2024 0:21am

Copy link

netlify bot commented Jun 15, 2024

Deploy Preview for effervescent-donut-4977b2 canceled.

Name Link
🔨 Latest commit d122f34
🔍 Latest deploy log https://app.netlify.com/sites/effervescent-donut-4977b2/deploys/666d95fb2429a200088978cd

@mschipperheyn
Copy link
Collaborator Author

mschipperheyn commented Jun 15, 2024

I'm seeing some issues in the files I committed. Some changes occured in files I didn't touch. I will review and update the PR

Done

@davidhoeck
Copy link
Collaborator

As far as I can see, this would set the price of all variants to the price of the first variant in the array. I don't think that is good, as we are already looping through the variants at that point.

@mschipperheyn
Copy link
Collaborator Author

I don't think that is the case. It's taking the price from the first price in the price array per variant. But I'll have a look.

@davidhoeck
Copy link
Collaborator

True, everything ok! Mistake on my side 😁

@michaelbromley
Copy link
Member

Yes, the current behaviour is a bug indeed, thanks for picking this up! I'm wondering is we can make this even smarter: in this change, we take the 1st price in the productVariantPrices array. If we have a multi-channel setup, that price might not be associated with the current active channel, which may lead to an unexpected result.

We could do something like

const price = variant.productVariantPrices.find(p => idsAreEqual(p.channelId, ctx.channelId)?.price ?? /* fallback to 1st item */

Going even further, it could make sense to allow copying all prices in the current channel (for the case that there are multiple currencies set up for this channel). But this is out-of-scope for this bugfix, and could be considered as a potential feature for a future release.

@mschipperheyn
Copy link
Collaborator Author

I'll review and update. Thanks for the feedback!

@mschipperheyn
Copy link
Collaborator Author

I've added the requested changes and added a test for channel specific price copy

@michaelbromley michaelbromley merged commit 18d200c into vendure-ecommerce:master Jun 18, 2024
13 checks passed
@michaelbromley
Copy link
Member

@mschipperheyn Excellent work, especially on the e2e tests 👍 Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants