-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Comments
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 draftEasy, but probably more resource heavy (following suggestion by @forkata)
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 draftMore efficient bulk operations - but concerns for complications.
LineItem.price.delete
LineItem.set_pricing_attributes
order.recalculate ConcernsI 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 |
Everybody here appreciates upstream contributions.
Your thought is actually correct. It would be correct and ethical to inform the customer about a change in price.
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. |
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:
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
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 |
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. |
This is my solution. Please let me know if you see some problem with simply adding a Add new order column to track last recalculation timeSo I can recalculate only if prices older than X
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 loadRevert to cart if prices recalculated in the checkout process
def show
@order = current_order(build_order_if_necessary: true)
if @order.recalculate_prices
flash[:warning] = t(:order_prices_recalculated)
end
...
end
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
# 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
order_prices_recalculated: "The price of your order has changed. Please take a moment to review your cart" |
@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. |
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? |
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 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. |
@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. |
The cart does not recalculate automatically when the underlying data such prices changes or the cart is particularly old.
Solidus Version:
All
To Reproduce
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 -
solidus/core/app/models/spree/line_item.rb
Lines 168 to 172 in fc44726
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?
The text was updated successfully, but these errors were encountered: