Skip to content

Cart doesn't always recalculate when the underlying data changes #6206

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

Open
fthobe opened this issue Apr 7, 2025 · 9 comments
Open

Cart doesn't always recalculate when the underlying data changes #6206

fthobe opened this issue Apr 7, 2025 · 9 comments

Comments

@fthobe
Copy link
Contributor

fthobe commented Apr 7, 2025

The cart does not recalculate automatically when the underlying data such prices changes or the cart is particularly old.

Solidus Version:
All

To Reproduce

  1. insert a product in the basket
  2. modify the price of a product in the admin interface or via API
  3. try to complete an order that was initiated

Current behavior
The cart is not recalculated and therefore the cart still contains the product with the old price.

Expected behavior
Credits for expected behaviour: @forkata
The general idea would be to re-assign the price on line item from the variant on the line item and call order.recalculate.

Clearing the LineItem#price and calling LineItem#set_pricing_attributes would do it as well -

def set_pricing_attributes
self.cost_price ||= variant.cost_price
self.money_price = variant.price_for_options(pricing_options)&.money if price.nil?
true
end

Additional context

@mickenorlen would you be up to make a PR related to this?

Slack Discussion

User Story:
The client updated the product price a couple of months back. Many orders have bought the product at new price. But all of a sudden there is a customer that buys the product with the old price. This has been reported twice now. What could be the issue here? Are old prices stored somewhere in the application or is it something to do with client specific caching who maybe loaded the product before the price change?

@mickenorlen
Copy link
Contributor

Hi, I want to solve the problem for my client asap, thinking to look into it more tomorrow. Was considering a quicker solution for now. But I could try to create a PR if it seems feasible withing my time frame. However, I have some concerns that might complicate the solution.

Please let me know if you have any thoughts on these solutions.

Quick solution draft

Easy, but probably more resource heavy (following suggestion by @forkata)

  1. Before rendering cart/checkout page. Run following code:
    order.line_items.each do |line_item|
      line_item.price.delete # Clear old price
      line_item.set_pricing_attributes # Set new price
    end
    order.recalculate # Update overall order 

Core solution draft

More efficient bulk operations - but concerns for complications.

  1. Find the method where admin/api updates prices.
  2. Find all incomplete orders (order.state != completed?) containing the updated variants and call something like the code again suggested by @forkata
    LineItem.price.delete
    LineItem.set_pricing_attributes
    order.recalculate

Concerns

I don't remember the exact checkout/payment flows right now so please enlighten me if this makes little sense.

But I'm worried if this could somehow impact a customer in the middle of a payment that would accidentally pay a new price without realizing it. It seems almost like a legal problem if they just got presented and confirmed another price.

Could it be necessary to revert the customer to the cart/checkout page with a message notifying the price change?

Or could it be an idea to only apply the price change to orders in specific states like status=cart to avoid those in the middle of checkout/payment? But what if the customer does not follow through with the payment - could their order be reverted to cart state again without having their prices updated?

@fthobe
Copy link
Contributor Author

fthobe commented Apr 8, 2025

Hi, I want to solve the problem for my client asap, thinking to look into it more tomorrow. Was considering a quicker solution for now. But I could try to create a PR if it seems feasible withing my time frame. However, I have some concerns that might complicate the solution.

Everybody here appreciates upstream contributions.

Could it be necessary to revert the customer to the cart/checkout page with a message notifying the price change?

Your thought is actually correct. It would be correct and ethical to inform the customer about a change in price.

I don't remember the exact checkout/payment flows right now so please enlighten me if this makes little sense.

You can find more information about the state machine here: https://guides.solidus.io/advanced-solidus/state-machines/

I will add some more infos later today.

@jarednorman
Copy link
Member

There are a lot of different cases we need to handle. I think we should provide the initial building blocks and go from there.

I think the first step is providing the code that does the pricing updates for an order. People can then run that on all active carts, or carts that haven't been touched in X hours/days/weeks, or whatever they decide as a business. (Changing a price on a cart mid-checkout is risky and a good way to get yourself a spike in support tickets.)

With that, we could later provide mechanisms for different strategies for handling these:

  • We can provide callbacks/jobs to update carts when the price of a variant is changed.
  • We can provide a way a step to notify customers of pricing changes when they try to checkout.
  • We can do other things.

All of that (and more) is contingent on providing that basic code to update the prices on an order, so that's the first step.


I will note that in the conversation on Slack, @fthobe said

ok don’t worry about performance, you should be fine recalculating on checkout events [...]

That's not true for many stores. Recalculation can be expensive on high-volume stores that use auto-applied promotions. There's work going in to make Spree::Order#recalculate more efficient (#5872), but it's important to monitor the performance of a change like that with your own data/customizations/optimizations.

@fthobe
Copy link
Contributor Author

fthobe commented Apr 8, 2025

That's not true for many stores. Recalculation can be expensive on high-volume stores that use auto-applied promotions. There's work going in to make Spree::Order#recalculate more efficient (#5872), but it's important to monitor the performance of a change like that with your own data/customizations/optimizations.

I agree with that, my comment was more related to the situation he personally had not as a general approach to something mergable, sorry if I was not precise enough. Obviously this can have performance implications on larger platforms.

@mickenorlen
Copy link
Contributor

mickenorlen commented Apr 9, 2025

This is my solution.

Please let me know if you see some problem with simply adding a before_action in the checkouts_controller to recalculate prices and redirect back to cart if prices changed. From my testing it does not seem to break any important state flow.


Add new order column to track last recalculation time

So I can recalculate only if prices older than X

db/migrate/20250409084258_add_prices_recalculated_at_to_spree_order.rb

class AddPricesRecalculatedAtToSpreeOrder < ActiveRecord::Migration[7.0]
  def change
    add_column :spree_orders, :prices_recalculated_at, :timestamp
  end
end

Add price recalculation at cart and checkout page load

Revert to cart if prices recalculated in the checkout process

app/controllers/carts_controller.rb

  def show
    @order = current_order(build_order_if_necessary: true)

    if @order.recalculate_prices
      flash[:warning] = t(:order_prices_recalculated)
    end
    ...
  end

app/controllers/checkouts_controller.rb

  before_action :recalculate_prices

  def recalculate_prices
    return unless request.method == "GET"

    if @order.recalculate_prices
      flash[:warning] = t(:order_prices_recalculated)
      redirect_to(cart_path)
    end
  end

Extend order with price recalculation logic

app/extensions/solidus/spree_order_extension.rb

  # Recalculate order and line_items prices if calculation older than given time
  # returns true if prices changed so you can notify your customer
  def recalculate_prices(older_than = 20.minutes.ago) 
    return false if prices_calculated_since?(older_than)
    before_prices = price_comparison_map

    self.line_items.each do |line_item|
      line_item.price = nil # Clear old price
      line_item.send(:set_pricing_attributes) # Set new price
    end
    self.recalculate
    self.update(prices_recalculated_at: Time.now)

    price_comparison_map != before_prices
  end

  def prices_calculated_since?(time)
    return true if self.created_at.nil? # if nothing to calculate
    return true if self.created_at > time # if calculated at creation
    return false if self.prices_recalculated_at.nil? # if never recalculated
    self.prices_recalculated_at > time # if recalculated since time
  end
  
  def price_comparison_map
    [self.total] + self.line_items.map(&:price)
  end

config/locales/en.yml

  order_prices_recalculated: "The price of your order has changed. Please take a moment to review your cart"

@fthobe
Copy link
Contributor Author

fthobe commented Apr 9, 2025

@mickenorlen nice one, could you throw that in a PR so that I can be properly commented?

In that way we can see also if testing passes.

@fthobe
Copy link
Contributor Author

fthobe commented Apr 9, 2025

Also some testing would have to be written. I am sure Thomas can help you out there, it's a complex scenario.

@tvdeyen @mickenorlen stumbled over an issue that we also encountered with the no vat / vat issue and pricelists. The cart does not necessarily recalculate after a change in price as there is no loopback to update the cart.

It's surely not the most sophisticated solution (micken recalculates the cart after 20 minutes) but it could work.

I would put this in a configuration and allow to set this globally. What do you think?

@fthobe fthobe changed the title Cart does always recalculate when the underlying data changes Cart doesn't always recalculate when the underlying data changes Apr 9, 2025
@mickenorlen
Copy link
Contributor

mickenorlen commented Apr 10, 2025

Hi, sorry I don't think I'll be taking this to a PR as I feel like this solution probably isn't really what you are looking for and I don't really have time for further improvements. As @jarednorman said I think there are a lot of edge cases to consider to arrive at something more complete and I don't even have a test app running to work with this atm. At least you have the first step with some code examples. Btw, I added 2 updates to above code to hopefully avoid casuing any problems:

In checkouts controller, to prevent breaking any patch calls

return unless request.method == "GET"

For a new customer visiting the cart order.created_at == nil so had to escape that as well

    return true if self.created_at.nil? 

I published the change to our site now as well. So I can get back to you in a bit and let you know if everything is still working for us.

Another thought that came up working on this issue is whether or not changes in promotions or sale_prices would automatically be picked up in the carts. If this issue is only related to variant prices, or if other price related updates in the same way could cause outdated cart pricing. I think some more research might be needed there, either to understand how this issue is handled, or integrate a solution to handle them into this work as well.

The more simple solution i chose would probably catch any type of price changes, but it probably isn't the best way to handle this for larger sites as discussed previously.

@fthobe
Copy link
Contributor Author

fthobe commented Apr 10, 2025

@tvdeyen we want to solve this as part of the pricelists PR we would like to make. Is there any preferred road to achieve cart recalculation? I think a time aspect is a reasonable factor.

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

No branches or pull requests

3 participants