Invalid cart after removing line item with line item adjustment

[kristinalim]

Description

When shopping as customer, the cart becomes invalid when you remove a line item with a line item based adjustment, such as an enterprise fee.

Removing the line item results to an HTTP 500 error, then the line item adjustment is not deleted while the associated line item is already gone. These are some implications (there may be more):

• The order total still includes the line item adjustment that is no longer visible in the shopfront. This adjustment is still listed in the order page for admins.
• Clicking "Empty Cart" in the cart page fails.
• Clicking "Update" in the cart page fails.

The customer can still check out with the wrong order total.

Adding back the variant which was removed from the cart fixes this.

Expected Behavior

Deleting a line item from the cart should also remove the adjustment.

Actual Behaviour

Deleting a line item from the cart leaves behind the adjustment.

Steps to Reproduce

As customer:

1. Add two variants with associated enterprise fees in the order cycle.
2. Remove the first line item. (HTTP 500)
3. Go to the checkout page and notice the wrong order total.

As admin:

1. Go to the order page for the order that is still in "Cart" state.
2. Notice that the adjustment for the removed line item is still there.
3. Notice that the order total includes this adjustment.

As customer:

1. Go back to the cart page.
2. Click on "Empty Cart". (HTTP 500)
3. Change the quantity for the remaining line item, and click on "Update". (HTTP 500)

Context

I was looking into #4022.

Severity

bug-s2: a non-critical feature is broken, no workaround

[kristinalim]

This is related to #3969.

[kristinalim]

This is the error when deleting the line item:

Parameters for spree/orders#update:

{"utf8"=>"✓",
 "_method"=>"put",
 "authenticity_token"=>"AUTHENTICITY_TOKEN",
 "order"=>{"line_items_attributes"=>{"0"=>{"quantity"=>"3",
 "id"=>"10"},
 "1"=>{"quantity"=>"0",
 "id"=>"13"}}}}

Error:

NoMethodError (undefined method `price' for nil:NilClass):
  app/models/calculator/flat_percent_per_item.rb:26:in `block in compute'
  app/models/calculator/flat_percent_per_item.rb:25:in `compute'
  app/controllers/spree/orders_controller.rb:179:in `with_open_adjustments'
  app/controllers/spree/orders_controller.rb:79:in `update'

I'm not sure if this is related to #4022

[sauloperez]

I'm not sure if this is related to #4022

4022 might be related to #3969 as well.

[luisramos0]

Hi @kristinalim I tried to replicate locally but I couldn't see the 500 error. I think I see that an item with an order level fee (I put the fee on the income exchange), the fee is not removed if the item is removed. But I dont see the 500.

[kristinalim]

@luisramos0 Did you happen to be using an order-based calculator? Can you try one that is line-item based, so any calculator that's not in this list:

PER_ORDER_CALCULATORS = ['Spree::Calculator::FlatRate', 'Spree::Calculator::FlexiRate', 'Spree::Calculator::PriceSack'].freeze

I was using OC coordinator fees, but confirmed just now that it's also happens for incoming exchange fees. As long as the calculator is not order-based.

[luisramos0]

I can't replicate...

[kristinalim]

It seems I made a wrong assumption here, but still need to understand why this is not a problem for the PerItem calculator. So it is not all line item based calculators. This however is an issue for me for "Flat Percent (per item)" and "Weight". @luisramos0 Would you be able to verify?

[kristinalim]

Thank you very much, @luisramos0.

It turns out that Flat Rate (per item) is an exception to the rule: both our decorator and the calculator in Spree implement compute with the guard clause return 0 if object.nil? which avoids the error initially. Then the invalid adjustment is removed later on when all adjustments for the order are regenerated.

I think the solution in #4035 is appropriate. We should not rely on the guard clause because we should not be recomputing the adjustment when the line item is already nonexistent. As for Spree, 2.2 still has the guard clause, but 2.3 removes the calculator.

The PR is already here: #4035

[luisramos0]

yes, makes sense! I tested with flat percent per item and I can replicate. I also tested with the fix in 4035 and it works 👍