Order data inconsistency between Line Items and Inventory Items (was: snail in admin edit orders)

[mkllnk]

Description

Some orders can't be edited by an admin. The Bugsnag error suggests that a variant got removed from an order (I guess it was out of stock) but the order's manifest is still referencing the variant. The view displaying the edit order screen fails to find the line item for the missing variant and crashes trying to access it.

Expected Behavior

You can always edit an order as admin.

Actual Behaviour

We can't edit at least one order.

Steps to Reproduce

1. Visit https://openfoodnetwork.org.au/admin/orders/R451317181/edit

Context

Customer says they tried to order 3 of a product, but there were only 2 available, so they rectified this in the final screen before paying, then 'got kicked out' and had to start again. When they started again they found that the first part of the order was still there, finished the order, made payment and thought it went through fine. Didn't receive confirmation email though, and payment didn't go through. (Went to pick up box that week and it wasn't there).

That order is in address state and can't be edited.

I also think that variants removed from the cart because they are out of stock, causes this error: https://app.bugsnag.com/yaycode/openfoodnetwork/errors/5c625e38b659cb00191b1307

Severity

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

Your Environment

• Version used: v2.2.2
• OFN Platform instance: OFN Australia

Possible Fix

Bugsnag

ActionView::Template::Error in spree/admin/orders#edit
undefined method `single_money' for nil:NilClass

View on Bugsnag

Stacktrace

app/views/spree/admin/orders/_shipment_manifest.html.haml:10 - block in _0864f0f33beb8e759403eadb8313605c
app/views/spree/admin/orders/_shipment_manifest.html.haml:1 - each
app/views/spree/admin/orders/_shipment_manifest.html.haml:1 - _0864f0f33beb8e759403eadb8313605c
app/views/spree/admin/orders/_shipment.html.haml:34 - _bc7e946539bd48bcf6974a1142286d17
app/views/spree/admin/orders/_form.html.haml:5 - _9fb4e2d06260a01ce10cc4614bd31893
app/views/spree/admin/orders/edit.html.haml:30 - _aa3ef3a74b75f7d5a0de15cf8b658c89

[Matt-Yorkley]

Any more info on this @mklink ? I tried looking at replicating the bug, but didn't have much success.

I see we have some specs covering the case where a variant is deleted and the orders page will still display, but this is obviously a slightly different issue, caused by a discrepancy between the shipment's items and the order's line_items.

I can see the order in Aus production has no address and no line items at all, which makes this part of the bug report confusing:

finished the order, made payment and thought it went through fine

The order and payment went through without an address?

[mkllnk]

I don't have any more info either.

[Matt-Yorkley]

I've added a pending spec in #4205.

The order edit page loops through the items in the shipment's manifest and checks them against the order's line_items. If a line_item has been removed we get a fatal error.

I can add a callback when destroying a line_item that checks for and removes the related item in the shipment, but I'm wondering what implications this might have, for instance if the order has already been shipped?

[luisramos0]

@Matt-Yorkley I think this maybe the fix (spree 2.2) :-)
spree/spree@607b87d#diff-d4464f8dddd86dd147739f1fcf6d589b

[Matt-Yorkley]

@luisramos0 we already have a similar callback when destroying a line_item:

openfoodnetwork/app/models/spree/line_item_decorator.rb

Lines 156 to 167 in fa1becb

	def update_inventory_before_destroy
	# This is necessary before destroying the line item
	# so that update_inventory will restore stock to the variant
	self.quantity = 0
	update_inventory
	# This is necessary after updating inventory
	# because update_inventory may delete the last shipment in the order
	# and that makes update_order fail if we don't reload the shipments
	order.shipments.reload
	end

It looks like if I call order.line_items.first.destroy, the proper callbacks are used and the associated item in the shipment is correctly removed. If I remove the line item from the order without calling destroy, for example with order.line_items.first.delete, the callbacks are bypassed, the corresponding item in the shipment is not removed, and the error occurs.

I haven't been able to actually replicate this via the UI (only in an abstract test), and I haven't been able to create a reasonable test that shows how this error actually occurs in the app.

I thought this might be the cause:

openfoodnetwork/app/controllers/spree/orders_controller.rb

Lines 189 to 191 in fa1becb

	def discard_empty_line_items
	@order.line_items = @order.line_items.select { |li| li.quantity > 0 }
	end

...but the above only seems to be called from the edit basket page when working with an order in the cart state, and at that point it doesn't have a shipment yet, so it doesn't trigger the error.

I think I'll stick this back in dev ready for now and let someone else take a look.

[luisramos0]

this is a brain twister! I just spent 2 hours on this without a single conclusion 😭

[RachL]

@luisramos0 shall we move it back to s3 and out of dev ready?

[luisramos0]

I dont know. @mkllnk what do you think?

[mkllnk]

We don't have a workaround which makes it an s2 by definition. But it affects very few users, maybe it affected only one real user. I'll degrade it to s3 for now but we may have to bump it up again if it occurs again.

[mkllnk]

We had one more case of this: https://app.bugsnag.com/yaycode/openfoodnetwork/errors/5e7c2499183e6e0018839df6

[mkllnk]

This issue has been linked to an error in Bugsnag
ActionView::Template::Error in spree/admin/orders#edit

[emilyjeanrogers]

Shopper contacted us to say that their order to Sugarloaf Produce doesn't appear in their account. The shopper has paid by EFT but the order hasn't been processed (it's incomplete) and can't be viewed by anyone (superadmin, customer or enterprise) due to this issue. There appear to be two failed orders in admin, both in address state, both with balance due. They can't be edited/opened (snail) so It's therefore not possible to investigate the order, find out what they ordered, provide a refund or work with the order in any other way. The shopper has been advised to attempt to re-order.

[luisramos0]

Hello @emilyjeanrogers I think it's important with dont share customer data in this public issue in github. I removed the user email from your comment above.

[luisramos0]

It wouldnt fix the root cause, but it will be very easy to make the view _shipment_manifest.html ignore the nil line_items. That would fix the snail.

[luisramos0]

I am working on a work around PR for this, I'll make the order edit page work when the order has inventory_items in the shipment but the respective line_item as been removed.

[luisramos0]

#5253 solves the problem of the broken order edit page, it doesnt fix the data problem in the order.
I think that after #5253 this issue becomes an S3 as the manager will be able to look at the order and make changes.

So, I am moving to Test Ready and marking this prod-test and blocked so that we can verify how the situation is in live after #5253 is released.
The prod-test will be unblocked when 5253 is released.

[daniellemoorhead]

Hey @filipefurtad0 and @luisramos0 #5253 is in the latest release so this issue can be tested on production as soon as it's deployed. 🎉

[RachL]

@kirstenalarsen are you able to test in production? The issues give a link to a specific example in AU prod.

[luisramos0]

I clicked that link and I see a snail. The order does not exist in the database!? The issue is from Aug 2019, what happened to this order!?
link:
https://openfoodnetwork.org.au/admin/orders/R451317181/edit
SQL:
select * from spree_orders where number = 'R451317181'
no results

I went to bugsnag and tried one of the errors there, for this second order, the page now loads:
https://openfoodnetwork.org.au/admin/orders/R655304771/edit
It doesnt show any line items and the order is in the address state. I can see the email on the customer details.

Maybe we can keep the issue open as the root problem is not solved but it's not an S2 anylonger, right? Maybe s3?

[luisramos0]

I think this problem is now an S3 as the users can see the order. The data inconsistency is still there and the root cause is still unknown.
Maybe we should close this issue and open a more specific "data inconsistency" issue. For now I'll just move this to "All the things".