From 1e1e88fe51aa3f7de144ee7f135a0e97b5cca8a0 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 21 Jun 2019 16:05:54 +1000 Subject: [PATCH 1/3] Rescue checkout on shipping id conflict Helps to investigate https://github.com/openfoodfoundation/openfoodnetwork/issues/3924 A better solution would be to move the update of shipments out of the after_save callback and deal with it in controllers. Unfortunately, that's a big and tricky task. Since this exception is causing a lot of pain for some Australian farmers, I introduced more logging here to understand the problem better. The issue was observed in OFN v1 and may disappear with v2. But we don't know that and should monitor it. --- app/models/spree/order_decorator.rb | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index fec601be76..a723ce1198 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -166,7 +166,21 @@ Spree::Order.class_eval do shipments.each do |shipment| next if shipment.shipped? update_adjustment! shipment.adjustment if shipment.adjustment - shipment.save # updates included tax + begin + shipment.save # updates included tax + rescue ActiveRecord::RecordNotUnique => error + # This error was seen in production on `shipment.save` above. + # It caused lost payments and duplicate payments due to database rollbacks. + # While we don't understand the cause of this error yet, we rescue here + # because an outdated shipping fee is not as bad as a lost payment. + # And the shipping fee is already up-to-date when this error occurs. + # https://github.com/openfoodfoundation/openfoodnetwork/issues/3924 + Bugsnag.notify(error) do |report| + report.add_tab(:order, attributes) + report.add_tab(:shipment, shipment.attributes) + report.add_tab(:shipment_in_db, Spree::Shipment.find_by_id(shipment.id).attributes) + end + end end end From 980b4a86ab9161d5ace17a219d06a191943b2a08 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 26 Jun 2019 10:48:51 +1000 Subject: [PATCH 2/3] Move shipment rescue to its own method --- app/models/spree/order_decorator.rb | 32 +++++++++++++++-------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index a723ce1198..22a44e26de 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -166,21 +166,23 @@ Spree::Order.class_eval do shipments.each do |shipment| next if shipment.shipped? update_adjustment! shipment.adjustment if shipment.adjustment - begin - shipment.save # updates included tax - rescue ActiveRecord::RecordNotUnique => error - # This error was seen in production on `shipment.save` above. - # It caused lost payments and duplicate payments due to database rollbacks. - # While we don't understand the cause of this error yet, we rescue here - # because an outdated shipping fee is not as bad as a lost payment. - # And the shipping fee is already up-to-date when this error occurs. - # https://github.com/openfoodfoundation/openfoodnetwork/issues/3924 - Bugsnag.notify(error) do |report| - report.add_tab(:order, attributes) - report.add_tab(:shipment, shipment.attributes) - report.add_tab(:shipment_in_db, Spree::Shipment.find_by_id(shipment.id).attributes) - end - end + save_or_rescue_shipment(shipment) + end + end + + def save_or_rescue_shipment(shipment) + shipment.save # updates included tax + rescue ActiveRecord::RecordNotUnique => error + # This error was seen in production on `shipment.save` above. + # It caused lost payments and duplicate payments due to database rollbacks. + # While we don't understand the cause of this error yet, we rescue here + # because an outdated shipping fee is not as bad as a lost payment. + # And the shipping fee is already up-to-date when this error occurs. + # https://github.com/openfoodfoundation/openfoodnetwork/issues/3924 + Bugsnag.notify(error) do |report| + report.add_tab(:order, attributes) + report.add_tab(:shipment, shipment.attributes) + report.add_tab(:shipment_in_db, Spree::Shipment.find_by_id(shipment.id).attributes) end end From 5bbd63bcd80cf68ed918b98def41a266b775ff0f Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 26 Jun 2019 11:32:01 +1000 Subject: [PATCH 3/3] Add spec for rescuing order saving shipment --- spec/models/spree/order_spec.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index b39bad4ccc..c6d96c71be 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -756,6 +756,24 @@ describe Spree::Order do expect { order.next! }.to change { order.state }.from("delivery").to("payment") end + + it "advances to complete state despite error" do + advance_to_delivery_state(order) + # advance to payment state + order.next! + + create(:payment, order: order) + # https://github.com/openfoodfoundation/openfoodnetwork/issues/3924 + observed_error = ActiveRecord::RecordNotUnique.new( + "PG::UniqueViolation", + StandardError.new + ) + expect(order.shipment).to receive(:save).and_call_original + expect(order.shipment).to receive(:save).and_call_original + expect(order.shipment).to receive(:save).and_raise(observed_error) + + expect { order.next! }.to change { order.state }.from("payment").to("complete") + end end context "when the order is a subscription" do