Feat(core): variant price calculator #1614
Conversation
this commit moves the previously added variant price calculation helper into its own service for easier testing
There was a problem hiding this comment.
Hi!
Thanks for the PR! I've left a few comments but my overall comment is: I'm not sure whether or not I want to have this in the core yet, because I'm not sure whether it is the best approach.
This is a really tricky problem to solve, but the nice thing is that all of this code can live as a plugin without requiring any changes to core, right?
Are you already using this in a project? If so, I'd be interested to hear about:
- How do you use it? Do you have a new query which is called on the product detail page to show discount prices? Or do you have a field resolver on ProductVariant?
- What's the performance like? There's quite a bit of computation going on here to get that final price - especially if the activeOrder already has many OrderLines. Do you notice any performance impact from querying this?
| const customerId = initialOrder?.customer?.id ?? ctx.activeUserId; | ||
| if (!customerId) | ||
| // FIXME use the appropriate Error type, instead of generic Error | ||
| throw new Error('no active user or order customer'); |
There was a problem hiding this comment.
You can use InternalServerError here.
| initialOrder?: Order, | ||
| ) { | ||
| const customerId = initialOrder?.customer?.id ?? ctx.activeUserId; | ||
| if (!customerId) |
There was a problem hiding this comment.
Use curly braces even on single-line if clauses, just to fit in with the code style of the rest of the project.
|
|
||
| let order: Order; | ||
| if (initialOrder) { | ||
| order = new Order(initialOrder); |
There was a problem hiding this comment.
If the initialOrder is of type Order, you should not need to construct a new one here right? Or is there some reason you need to?
There was a problem hiding this comment.
wouldn't adding orderLines to initialOrder mutate the order that is passed to the function, since its members are references and not copied values
| order = new Order(initialOrder); | ||
| } else if (ctx.session?.activeOrderId) { | ||
| const found = await this.orderService.findOne(ctx, ctx.session?.activeOrderId); | ||
| order = new Order(found); |
There was a problem hiding this comment.
Same - the orderService.findOne call will return an Order so no need to instantiate a new one, unless there is a specific reason I missed.
|
I will do some benchmarking with really large orders to see how it impacts performance |
|
and yes all of this can live inside a plugin, currently we have a custom query to get discounted prices for variants, it is used to show discounted prices right in the product display grid, since without this calculation the discounts are only shown after adding an item to your cart/order |
|
We will close this because of in-activity and missing information. As everything can live within a plugin at this point, it is not really necessary to change it in the core. |
Description
This PR adds a new helper service that can calculate the price of a variant, including promotions, without having to manually create the order, orderLines, orderItems... every time.
You can either supply a base order where it adds the variant and executes the promotions on, or use the currently active order for the session customer (default)
There are still some open fixme and todos that need to be adressed, i was hoping that @michaelbromley can give some input on those (what errors should be used and if it should just return the regular price in case it can't be calculated)