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

Fix dynamically applying discounts #1243

Merged
merged 7 commits into from
Sep 21, 2023
Merged

Fix dynamically applying discounts #1243

merged 7 commits into from
Sep 21, 2023

Conversation

repl6669
Copy link
Contributor

@repl6669 repl6669 commented Sep 8, 2023

I came across a situation when discounts were not applied, because the calculate method was already called on cart. It is called during CartSession::use($cart) and also during CartSession::current(). This ended up with a state in DiscountManager, when the $discounts property was set to empty collection. Then the check in DiscountManager became true every time, because it only checks for null value and not empty collection.

After changing the check to count with empty collection as follows: ! $this->discounts || $this->discounts?->isEmpty()), there were multiple instances of the discounts added to $cart->discounts property. I mitigated this behavior by only adding discounts which are not already present in that property.

I also added a test which calls the $cart->calculate() method multiple times to make sure that discounts are added properly.

All tests are running green now, but this implementation is ofc open for discussion. Maybe there is a better solution.

@vercel
Copy link

vercel bot commented Sep 8, 2023

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

Name Status Preview Comments Updated (UTC)
lunar-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 20, 2023 9:05am

@glennjacobs
Copy link
Contributor

This doesn't seem correct.

CartSession::use($cart) does NOT run the calculate method.

The only issue I can see, which probably needs fixing, is the Cart discounts property is not being reset when calculating, which can lead to the same discount being logged twice. I believe it probably needs adding here https://github.com/lunarphp/lunar/blob/0.5/packages/core/src/Pipelines/Cart/ApplyDiscounts.php#L18

E.g.

    public function handle(Cart $cart, Closure $next)
    {
        $cart->discounts = collect([]);  // <---------- this is new
        $cart->discountBreakdown = collect([]);

You probably need to update your test as follows

        // Calculate method called for the first time
        CartSession::use($cart)->calculate();

Copy link
Contributor

@glennjacobs glennjacobs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

@repl6669
Copy link
Contributor Author

Hello @glennjacobs, I updated the test.

And yes, there were 2 problems.

  1. Discounts could not be applied after first calculate call on Cart.
  2. Discounts were added to $cart->discounts multiple times.

Let me know if I should do some more changes. Thank you.

@ryanmitchell
Copy link
Contributor

Sorry for spamming the comments, but I would love if if this could be merged for 0.6. The lack of a count()/empty() check on discounts is biting us at the moment so we have to manually add it.

@alecritson alecritson merged commit 559cdf1 into lunarphp:0.5 Sep 21, 2023
12 checks passed
alecritson pushed a commit that referenced this pull request Sep 21, 2023
# Conflicts:
#	packages/core/tests/Unit/DiscountTypes/AmountOffTest.php
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.

4 participants