Openfoodnetwork: Not possible anymore to manually capture a payment on production on some edge cases

[myriamboure]

Description

From general order page, or if I'm in edit mode of a specific order, when I click on "capture payment", I got the snail page.

Expected Behavior

I can capture a payment so the order is marked as paid.

Actual Behaviour

It's not

Steps to Reproduce

1. Go to https://www.openfoodfrance.org/admin/orders?q%5Bs%5D=completed_at+desc and try to capture a payment on an order.
2. Go into an order, sub menu "payments", same, try to capture a payment.

Animated Gif/Screenshot

Context

On French production, was documenting how to manage refunds on OFN in the user guide...

Severity

S2

Your Environment

• Version used: 1.27
• Browser name and version: Firefox
• Operating System and version (desktop or mobile): Ubuntu 1.18
• OFN Platform instance where you discovered the bug, and which version of the software they are using : French production

[kristinalim]

I ran the following on the OFF production server:

tail -n 10000 log/production.log | grep PaymentsController\#fire --after-context 150 --before-context 10

And spotted the following:

Started PUT "/admin/orders/R476265115/payments/32851/fire?e=capture" for 176.167.237.217 at 2019-02-27 20:25:13 +0100
Processing by Spree::Admin::PaymentsController#fire as HTML
  Parameters: {"authenticity_token"=>"TOKEN", "e"=>"capture", "order_id"=>"ORDER_ID", "id"=>"PAYMENT_ID"}
Redirected by /home/openfoodnetwork/apps/openfoodnetwork/current/app/controllers/spree/admin/payments_controller_decorator.rb:21:in `ensure in fire'
Redirected to https://www.openfoodfrance.org/admin/orders/ORDER_ID/payments
Completed 500 Internal Server Error in 458.3ms (ActiveRecord: 194.7ms)
** [Bugsnag] No API key configured, couldn't notify

NameError (undefined local variable or method `inventory_units' for #<Spree::Payment:...>):
  app/models/spree/payment_decorator.rb:36:in `block in line_items'
  app/models/spree/payment_decorator.rb:36:in `line_items'
  app/models/spree/calculator_decorator.rb:12:in `line_items_for'
  app/models/spree/calculator/flat_percent_item_total_decorator.rb:10:in `compute'
  app/models/spree/payment_decorator.rb:24:in `ensure_correct_adjustment'
  app/controllers/spree/admin/payments_controller_decorator.rb:13:in `public_send'
  app/controllers/spree/admin/payments_controller_decorator.rb:13:in `fire'

Still investigating.

[luisramos0]

yeah, I recognise this code. I removed it for v2!
https://github.com/openfoodfoundation/openfoodnetwork/commit/2ccbb87adcb3174bb5a64b57b6fc9dd4ab2b3f02#diff-e2ebcdd38cd6a972dc29886b9cad8ff1

I am not sure if, at the time, I checked if it was a bug in v1 as well... this is the explanation:
https://github.com/openfoodfoundation/openfoodnetwork/pull/2823#discussion_r225642696

UPDATE: this means this bug will most probably not happen in v2

[luisramos0]

I read the v1 code now and I dont see where payment.inventory_units could come from... but it's weird this was not seen before... but it all depends on how calculator.line_items_for is called. maybe order.line_items is called instead of payment.line_items for most cases...

[mkllnk]

Can we just revert the payment decorator for now to fix it?

[luisramos0]

My change/fix is in v2 Maikel, this bug is for v1. I just shared the context of my change in v2 because it's the same line of code, but in v2.

[kristinalim]

This happens with a particular sequence of order sequence of order creation, payment creation, and order completion when the payment method uses an order-based calculator (e.g. "Flat Percent"). Actually, I wasn't able to reproduce this from the UI using the usual checkout flow nor from the admin section. But there is a code path for this.

Already working on a PR.

[myriamboure]

Ok as it seems to be an edge case I suggest to reclass this bug as s3 bug. If this is fixed in v2 maybe we can wait then and retest after v2. I have a case that we know for sure is bugging in French production, order number # R476265115 if you want to check if same issue or not.

[myriamboure]

@kristinalim I would suggest you don't spend too much time on this now as we understand from your first spike on it it is only concerning some edge case... Don't hesitate in those kind of bug, if when spiking you realise it might not be s2 as only concerning few specific rare cases, to review the severity ;-)

[myriamboure]

I added the network info in the main post.

[kristinalim]

@myriamboure I found an obvious bug in the code that could cause this, but didn't know how to get there through the UI. There is already a quick fix PR #3563 for this earlier today, and it's already in Test Ready. (@RachL In case you have time to validate. :grin:)

BUT I confirmed just now that adding (not capturing) a payment to a completed order fails with the same error. I'll just update the testing notes to also test adding a payment.