vendure-ecommerce / vendure

The commerce platform with customization in its DNA.
https://www.vendure.io
MIT License
5.39k stars 943 forks source link

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

Closed mschipperheyn closed 1 week ago

mschipperheyn commented 2 weeks ago

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

Checklist

📌 Always:

👍 Most of the time:

vercel[bot] commented 2 weeks ago

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
netlify[bot] commented 2 weeks ago

Deploy Preview for effervescent-donut-4977b2 canceled.

Name Link
Latest commit d122f34187aa33e1fdb611ccedd8d1ce3c3f987a
Latest deploy log https://app.netlify.com/sites/effervescent-donut-4977b2/deploys/666d95fb2429a200088978cd
mschipperheyn commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

True, everything ok! Mistake on my side 😁

michaelbromley commented 2 weeks ago

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 commented 2 weeks ago

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

mschipperheyn commented 1 week ago

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

michaelbromley commented 1 week ago

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