openfoodfoundation / openfoodnetwork

Connect suppliers, distributors and consumers to trade local produce.
https://www.openfoodnetwork.org
GNU Affero General Public License v3.0
1.09k stars 708 forks source link

ActiveRecord::RecordInvalid in checkout#update #12586

Open mkllnk opened 1 week ago

mkllnk commented 1 week ago

This is not a current bug. It appeared after deploying a pull request which has been reverted since:

Error in OpenFoodNetwork Aus

ActiveRecord::RecordInvalid in checkout#update Validation failed: Count on hand must be greater than or equal to 0

View on Bugsnag

Stacktrace

app/models/spree/stock_item.rb:26 - block in adjust_count_on_hand
app/models/spree/stock_item.rb:22 - adjust_count_on_hand
app/models/spree/stock_movement.rb:17 - update_stock_item_quantity
app/models/concerns/variant_stock.rb:124 - move
lib/open_food_network/scope_variant_to_hub.rb:50 - move
app/models/spree/stock_location.rb:49 - move
app/models/spree/stock_location.rb:45 - unstock
app/models/spree/shipment.rb:316 - manifest_unstock
app/models/spree/shipment.rb:208 - block in finalize!
app/models/spree/shipment.rb:208 - each
app/models/spree/shipment.rb:208 - finalize!
app/models/spree/order.rb:382 - block in finalize!
app/models/spree/order.rb:380 - finalize!
app/controllers/checkout_controller.rb:82 - confirm_order
app/controllers/checkout_controller.rb:39 - update
app/services/current_order_locker.rb:26 - block in lock_order_and_variants
app/services/current_order_locker.rb:24 - lock_order_and_variants
app/services/current_order_locker.rb:13 - around

View full stacktrace

Created by Maikel via Bugsnag

dacook commented 1 week ago

If this resulted in payments not being captured, we should list each one of these cases so we can let the affected enterprises know. And the same for all other bugsnag instances.. 😬

mkllnk commented 1 week ago

If this resulted in payments not being captured, we should list each one of these cases so we can let the affected enterprises know.

I went through Australian Bugsnag and send all affected order ids to @amidaOFN. But we should check others as well. Oh, and some subscriptions were affected bug I can't see the ids in Bugsnag.

mkllnk commented 1 week ago

Potential causing change reverted.

Comment added in Bugsnag by Maikel ActiveRecord::RecordInvalid in checkout#update

mkllnk commented 1 week ago

An error linked to this issue has been marked as fixed in Bugsnag ActiveRecord::RecordInvalid in checkout#update

mkllnk commented 3 days ago

Looking at the code, I have a suspicion:

A distributor with inventory can set a variant to on demand while not overriding the stock level. In the old model, orders would then not affect the stock level because on demand would disable stock tracking. But the offending pull request starting tracking stock for on demand products as well. I didn't consider this and thought that tracking stock for in this case was hidden to the user but it's actually a big change in logic. Now a sale of a distributor can reduce stock levels of the producer even though the distributor chose on demand.

I think that the solution is to enforce stock tracking on variant overrides when on demand is set. So the sales don't affect the producers stock level and the distributor has its own hidden stock tracking.