Fix bug on calculator_decorator line_items_for where input is line_item with a nil order

[luisramos0]

What? Why?

Take 2 on weight calculator (#2932), this time for a more fundamental change (remove line_items_for method all together).

The specific issue we address in the calculator is that the passed "object" could be a line_item that would "respond? :order" but with a nil order. In this case, the option to return [line_item] is the correct option.

What should we test?

Functional: we changed the way calculators work internally so we need to review all the points where calculators are used, not just weight. We need to review shipping fees, enterprise fees, and weight line items. I dont think it's possible these changes will break the calculations (the numbers), if something is wrong, things will just not work, no fees calculated, for example.

Tech: there could be a class loading issue with this PR, we need to watch in staging if there are any class loading error messages.

Specs: All specs related to calculators are green in the 2-0 branch. Fixes tests in spree upgrade issue #3025

Release Notes

Changelog Category: Fixed
Fixed bug in fees calculated based on weight. Bug introduced in previous release (#3072). This bug was applicable to orders with multiple line items with weight in order cycles with enterprise fees that use a weight calculator (this was validated in live environments and we didn't detect any case where this was applicable)

[luisramos0]

I have tested this locally, buying 3 items of a product of 5 kgs and paying enterprise fees per kg and shipping fees also per kg and with a payment method with "flexible rate" (first item one price, following items another price).

All the calculations are correct as far as I understand these things:

• 3 items x 5 kgs x 8.7eur enterprise fee = 130.5eur (I added the fee 3 times to the order cycle)
• 3 items x 5 kgs x 5.6 eur shipping fee = 84eur
• 10eur payment fee first item + 100eur x 2 payment fee 2nd and 3rd items = 210eur payment fees

[kristinalim]

Do I understand correctly that both #2932 and #3072 are not supposed to change the current calculator behaviour?

Considering the following:

• OC exchange with Weight calculator
• More than one line item in the order

It seems that #2932 changed the calculation for this type of order and then #3072 changes it back.

With #2932:

• EnterpriseFee::PER_ORDER_CALCULATORS does not include the weight calculator
• The weight-based fee is calculated per line item for which the fee is applicable
• But the calculation per line item uses the total weight of the order, not just the line item, so the fee is bigger

I think the current behaviour is the correct one, but if I'm correct, we need to know what should be done regarding #2932.

[luisramos0]

nice @kristinalim, thanks for your review!
You hit jackpot on this code review: finding a bug that is in production and was not detected previously 🎉 😭

I went back to reading the enterprise fees code and validated that #3072 does introduce a bug on orders with multiple line items with weight in order cycles with enterprise fees that use a weight calculator.
(the method line_items_for is called with object = line_item, the current live code will return the line_item.order which is wrong, it should return the line_item itself). This problem is solved in this PR. So, I understand what you are saying and agree.

I immediately went to the main live instances to see how many cases we have, lucky we are, this is only used in FR live in the "Hub demo Open Food France" (I checked AUS, UK, FR and ES).

This was the SQL I used:
select count(*) from spree_orders where state = 'complete' and order_cycle_id in (select distinct(order_cycle_id) from exchanges ex, exchange_fees xfee, spree_calculators c where ex.id = xfee.exchange_id and xfee.enterprise_fee_id = c.calculable_id and c.calculable_type = 'EnterpriseFee' and c.type = 'Calculator::Weight');

In @myriamboure 's test on #3072, here:
#2932 (comment)
the OC test order only has one line item with a kilo (the second item is only 10g). It explains why this was not seen.

I tested locally in master and this PR and it's obviously broken in master and it is working in this PR.
The test is an order in a OC with a weight calculator and the order must have at least two line items with weight: the prices will get mixed up on cart/checkout (the prices show correctly on the product list).

So, the only bad news is the bug in production.
The rest is good news:

• it's not happening to anyone
• we just need to test this PR and put it live

[luisramos0]

I had to rebase the PR to get the google maps pending specs out of the way...

[kristinalim]

@luisramos0 Could you also check for coordinator fees that use Calculator::Weight? And maybe, even just for active order cycles, we should check any order status and not only completed orders. Crossing fingers that there are no matches.

I think these should be also be done:

1. Create an issue to add specs that would test for this bug, because IMO the line_items_for method is difficult to visualize and we might make the same mistake.
2. Does the SQL check need to be done again right before deploying to each of the instances?
3. Update release notes to mention that this bug has been fixed.

What do you think?

[luisramos0]

thanks @kristinalim I agree. I will do that.

[luisramos0]

So, updating the checks:

• Removing order state from the first check: select count(*) from spree_orders where order_cycle_id in (select distinct(order_cycle_id) from exchanges ex, exchange_fees xfee, spree_calculators c where ex.id = xfee.exchange_id and xfee.enterprise_fee_id = c.calculable_id and c.calculable_type = 'EnterpriseFee' and c.type = 'Calculator::Weight');
• And creating a second check for coordinator fees: select count(*) from spree_orders where order_cycle_id in (select order_cycle_id from coordinator_fees cfee, spree_calculators c where cfee.enterprise_fee_id = c.calculable_id and c.calculable_type = 'EnterpriseFee' and c.type = 'Calculator::Weight');

Checked UK, AUS, ES and FR. Nothing detected except in France where there is one enterprise using this in 2016/2017 and then the only 2 test enterprises using it as well.
So, nothing to worry here.

I am trying to write the automated test to validate this case, in progress.

[luisramos0]

ok @kristinalim
new checks done in live, release notes updated and a new commit with the spec (I validated that this test is failing in master and passing in this branch).
ready for re-review.
thanks!

[luisramos0]

@kristinalim can you please re-review this one? thanks!

[kristinalim]

Sorry about the late response here - I didn't notice this mention among the recent notifications. (Need to update my work process.)

@luisramos0 This looks great. I wrote comments on very minor issues, but happy with this PR.

[kristinalim]

The quantity of 5 here really confused me because the cart total was only computing for 3 units. If the product factory's on_hand value of 3 for product_tax is important in this test, what do you think of specifying this attribute when defining product_tax?

[luisramos0]

oh, good catch. I dont see (and cant remember either) any reason to have that quantity capped in this test. I just changed it to 3 in this line! Maybe it was just a typo in the first place.
I fixed the double quotes as well.
I force-pushed the last commit with these fixes.
also, agree, we should have a extra re-review on this tricky PR.
thanks!

[kristinalim]

@sauloperez There have been important changes since your approval. I'll request another review from you to unmark it, and you or another dev can do the second review.

[sauloperez]

Nice! now it's way easier to understand.

[sstead]

Testing Notes

• All of the fee calculators are functioning correctly - both when an order is first made, and dealing with any recalculations. I checked fees on shipping/payment methods and enterprise fees.

One possibly related glitch I noticed:

• There seemed to be a cacheing error (my guess) where a $1/kg fee was getting applied to an 'item' product which was previously a 500g product. The fee shouldn't get applied to products which are items. Seems the calculator was seeing the product as 500g still.

Pre-existing glitch I noticed:

• Got a snail page when creating a new enterprise fee when I left a field blank. We need to add a 'please fill in this field' error message instead of snail page. New bug here - If I try to create a new Enterprise Fee but forget to fill the 'Fee Type' field, I get snail page. #3307

https://docs.google.com/document/d/178DpKby-lpQMkMX_AF2UCzfXejiMlzHUVmzFU14XdFY/edit#

[mkllnk]

I didn't see any errors while Sally was testing. Sally found that glitch is pre-existing. So this is ready to go.

[luisramos0]

great, thanks @sstead and @mkllnk