Skip to content

[Core] fix price calculation with immutables and adjustments - #2573

Merged
dpfaffenbauer merged 7 commits into
coreshop:3.2from
dpfaffenbauer:issue/2572
Mar 5, 2024
Merged

[Core] fix price calculation with immutables and adjustments#2573
dpfaffenbauer merged 7 commits into
coreshop:3.2from
dpfaffenbauer:issue/2572

Conversation

@dpfaffenbauer

Copy link
Copy Markdown
Member
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets #2572

@dpfaffenbauer dpfaffenbauer added this to the 3.2.6 milestone Mar 4, 2024
@dpfaffenbauer
dpfaffenbauer requested a review from a team March 4, 2024 15:47
@dpfaffenbauer dpfaffenbauer self-assigned this Mar 4, 2024
Comment thread src/CoreShop/Component/Core/Order/Processor/CartSubtotalProcessor.php Outdated
Comment thread src/CoreShop/Component/Core/Order/Processor/CartItemsImmutableProcessor.php Outdated
dpfaffenbauer and others added 2 commits March 4, 2024 17:11
…Processor.php

Co-authored-by: Jacob Dreesen <jacob@hdreesen.de>
…sor.php

Co-authored-by: Jacob Dreesen <jacob@hdreesen.de>
@solverat

solverat commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@dpfaffenbauer thanks for the quick response. Unfortunately, this does not work when using additional adjustments on cart items. In other words: The total value of the order items are getting completely ignored now.

To reproduce: You may want to add a simple adjustment to an item (not cart) in your test to validate the total value.

I'm not sure why this PR is not failing, but I guess it's because CartRuleApplier is adding an adjustment on item- AND cart layer:

$item->addAdjustment($this->adjustmentFactory->createWithData(
AdjustmentInterface::CART_PRICE_RULE,

$cart->addAdjustment(
$this->adjustmentFactory->createWithData(
AdjustmentInterface::CART_PRICE_RULE,

@dpfaffenbauer

Copy link
Copy Markdown
Member Author

@solverat adding an adjustment to the item without the order is not supported and is something we cannot make work easily without breaking other things. as you pointed out, cart-rules are applied on cart-item and cart directly. and that is the reason it is applied on both.

@dpfaffenbauer

Copy link
Copy Markdown
Member Author

@solverat one solution could be that we set the adjustment on the cart itself as neutral and use them from the items. not sure what side-effects that can have.

@dpfaffenbauer

Copy link
Copy Markdown
Member Author

@solverat please check the latest changes

@solverat

solverat commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

@dpfaffenbauer This looks VERY promising!

In my Test, I've

  • added a 2% global cart discount rule
  • added a global surcharge cart rule with fix amount
  • added a custom adjustment (just as before, only on item layer but with an isImmutable check)

…and all calculations are correct now!

The only thing I don't understand in your PR:

9e742c8#diff-ad51a1ab22ce6a03fe767ee679b9ccfd41a00b017606df746e1baf50b4c5f490R61-R72

I removed that part, and I wasn't able to spot the difference. Maybe a hint to test it would be great!

@dpfaffenbauer

Copy link
Copy Markdown
Member Author

@solverat If you have already have items with a total of 0, without that, it would lead to a division by 0 error.

I did some other changes as well, can you test them as well? Since we are now changing which discounts apply to what, we need to make sure to always have the right calculations.

@solverat

solverat commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

@dpfaffenbauer Just adopted your changes: Still valid!

@dpfaffenbauer

Copy link
Copy Markdown
Member Author

I will also test it in one of our projects first. It might have unconsidered side-effects

@solverat

solverat commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

I also think that this PR makes the neutral adjustment calculation more solid. 👍

It's quite clear now, what's happening when adding custom adjustments - especially if they have to be added to the order item subtotal to allow them to be discountable by cart rules!

@dpfaffenbauer

Copy link
Copy Markdown
Member Author

Yeah, so basically what this changes is:

  • Cart Price Rules are applied to the Cart Total, and added to the Cart Item as Neutral Adjustment
  • Cart Price Item Rules are applied to the Cart Item Total, and added to the Cart as Neutral Adjustment

@solverat

solverat commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

@dpfaffenbauer Btw, I was not able to test cart items action (coreshop.cart_item_price_rule.action). They never get applied (When using discount amount for example).

I don't know where the cart item rule processor should get triggered?

@dpfaffenbauer

Copy link
Copy Markdown
Member Author

@solverat They do work though for me. They are triggered as Cart Item Rule Action.

@solverat

solverat commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

On my debug marker, only CoreShop\Component\Core\Cart\Rule\Action\DiscountAmountActionProcessor gets dispatched, but never CoreShop\Component\Core\CartItem\Rule\Action\DiscountAmountActionProcessor.

@dpfaffenbauer

dpfaffenbauer commented Mar 5, 2024

Copy link
Copy Markdown
Member Author

@solverat
Screenshot 2024-03-05 at 10 40 26
Screenshot 2024-03-05 at 10 40 31
Screenshot 2024-03-05 at 10 42 11

@solverat

solverat commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

🙈 I'm so sorry, I'm very new to coreshop...

@dpfaffenbauer
dpfaffenbauer merged commit 6618d6c into coreshop:3.2 Mar 5, 2024
@dpfaffenbauer
dpfaffenbauer deleted the issue/2572 branch March 5, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants