Openfoodnetwork: Two customers order one item at the same time and both successful

[lin-d-hop]

Reported by Sara at Tamar Grow Local. Confirmed by Lynne.

When two customers order the same item at the same time both are able to check out and pay for the item despite there not being enough stock to do so.

This behaviour is a known issue, originally found in the building of #1031 with a fix attempted in #1767, though we had never had a user report the issue. As TGL grows Sara has now reported this twice.

Description

Stock levels are checked before an item is added to a cart, then they are reduced during checkout. Stock levels of items added to the cart are not checked between cart creation and checkout.

Expected Behavior

UserA puts an item in a cart.
Either UserB does not see the item because it is now out of stock.
Or UserB puts the same item in the cart before UserA checks out. UserA then checks out and pays for the item. When UserB checks out they are told that the item is no longer in stock and they are not charged for the item.

Actual Behavior

UserA puts an item in a cart, then UserB puts the same item in the cart before UserA checks out. The system will assume enough stock for both UserA and UserB. UserA then checks out and pays for the item. Then UserB checks out and pays for the item. There is not enough stock for both users but both have been able to buy the item.

Steps to Reproduce

1. UserA puts an item with stock=1 in cart.
2. UserB puts the same item in cart.
3. UserA goes to checkout, and successfully orders and pays for the item.
4. UserB goes to checkout, and successfully orders and pays for the item.

Context

For customers, this bug means that some customers order and pay for a product that they cannot receive. For hub managers they need to explain to customers, process refunds, update accounts and manage producers who become stressed when products have been over-ordered.

Severity

S2 - Although this does not affect many users, it is affecting a critical flow (order and checkout) and there is no workaround as no one will know this has happened until packing day. At this point customers need to be refunded. We are going to see an increase in prevalence and we need a solution to this issue before it becomes a bug that results in customers not using OFN.\

This was not experienced (knowingly) in 2017 but already it has been reported twice by TGL in 2018.

Your Environment

Not environment specific.

Possible Fix

Two options:
1) We have a timeout on a cart that means that items are held in a cart for a specific length of time and stock levels are reduced while the items are in the cart. At the end of the timeout the cart items are 'released' and stock levels are replenished such that other users can buy the item. Users should be notified of this timeout when they are ordering. Ideally items can still remain in cart but cannot be guaranteed after the timeout.
2) There is a final check before the order is completed that confirms that all of the items in the cart are still available for purchase before the payment is taken. Users are notified if some items have been removed from the cart as they are no longer available.

I actually think the best solution is both 1 and 2. This is what Amazon does ;-)

[HugsDaniel]

Someone taking care of this one ? I can dig into it.

[myriamboure]

From some previous investigation @HugsDaniel , inputs that might help you:

1031 is blocked by #1491 .

PR #1698 solves 1031, in code review since August.
PR #1676 solves bug 1491, but it seems the solution proposed is not working, so needs new inception.
So PR 1698 couldn't be moved forward anyway.
I don't know if that impacts the resolution of this bug, but might be.
You are tackling a more complex one now ;-)

[sauloperez]

[sigmundpetersen]

The Last Jedi?

[sauloperez]

www.giphy.com :joy:

[lin-d-hop]

Hi @HugsDaniel,
This is indeed quite a complex one. I would think you might want to scope a solution first then ask one of the senior devs (is that the word? master devs? gatekeeper devs?) for input before you begin on implementation....

[sauloperez]

agree with @lin-d-hop let's all discuss together before jumping into implementation. This is a whole feature more than a bug.

We might need to investigate state-of-the-art solutions first. It feels like changing the transaction's isolation level (See https://www.postgresql.org/docs/9.1/static/transaction-iso.html) might be of help here.

[sauloperez]

Just to highlight it, the actual problem seems to be

There is not enough stock for both users but both have been able to buy the item.

so that we don't miss the point of this issue.

[lin-d-hop]

It's a bug but the solution is no less than a feature :-(

[HugsDaniel]

State-of-the-art investigations give two main solutions to this problem :

• Implement an Amazon-style "pending order" logic (see) : the product's stock is updated when the order is placed and order is held during a few minutes, allowing the user to checkout. If the order is cancelled, the transaction rollbacks. (it is also used for plane or cinema tickets example). It doesn't go well with #1031 though.
• Use psql transaction isolation as suggested by @sauloperez or stuff like Optimistic Offline Lock, which is a native ActiveRecord module. The flow would be something like : a user add products to his cart and their availability is re-checked right before checkout to make sure they're still available. If not, the user gets an error or a message like "This product is missing, etc...".

Whole books have been written on concurrency, it's a major issue we're tackling here. What do you think ? The user-friendly solution would be the first one I think, it avoids users to go through an order and have an error on checkout, which is pretty annoying.

[sauloperez]

Whole books have been written on concurrency

Yes, I don't wish even my worst enemy having to solve a concurrency problem :trollface:

The Amazon-style seems like a whole big feature that may bring lots of complexity that we'll likely need to manage as we do with delayed_job, for instance.

On the other hand, Optimistic Locking although not ideal seems a bit more reasonable and as it's built in Rails already, makes it much faster to implement (I assume).

The user-friendly solution would be the first one I think, it avoids users to go through an order and have an error on checkout, which is pretty annoying.

I do see the benefit of fixing this but I don't think we need a silver-bullet solution as it's not our top-most priority / annoying bug. Also, we do have very limited resource and we should carefully assess the impact of both solutions.

[HugsDaniel]

It appears that optimistic locking wouldn't be helpful here. Here are the steps I reproduced :

• User 1 puts a product in his cart then proceed to checkout, fills the fields (address....) and then goes for coffee;
• User 2 does the same thing, except he doesn't go for coffee;
• User 2 places his order successfully;
• User 1 comes back and places his order, also successfully.

After this, the variant's count_on_hand equals -1, since they both ordered the last one in stock.

If a variant is available when User 1 hits the "Checkout" button and goes to checkout#edit page, he will be able to buy it, even if another user bought the last available variant while User 1 was checking out. Methods used to check a variant's availability are called before the checkout.

The thing is that before the order is placed, no DB transaction is made. The transactions begin when the user hits the "Place order" button, but at this point it's already "too late". So optimistic locking doesn't solve the issue here.

Instead of optimistic locking, I was thinking about implementing a method doing a final check when the user hits the "Place order" button. In case of unavailability, it could redirect to the cart or checkout and return an error message telling which of the products are out of stock. It seems to work well.
What do you think ?

[sauloperez]

Methods used to check a variant's availability are called before the checkout.

Only at that step? aren't they checked right when placing the order? that's a bit silly from Spree... When is the count_on_hand actually decremented? When the order is placed?

Instead of optimistic locking, I was thinking about implementing a method doing a final check when the user hits the "Place order" button. In case of unavailability, it could redirect to the cart or checkout and return an error message telling which of the products are out of stock. It seems to work well.
What do you think?

Have you checked if this is solved by any of the Spree plugins? Check out https://github.com/spree-contrib/

I must say that I'm afraid we might invest too much time and resource in solving a problem that is far from our top-most priorities. The investigation is taking time but an implementation could take much longer. Also, considering that the part of the codebase we're talking about is the worst in the codebase due to amount of Spree overrides and callbacks involved. So, let's discuss and see if it's feasiable to work on it.

[myriamboure]

Do you mean @sauloperez that it would make more sense to plan that for after the Spree upgrade maybe? The upgrade might impact that part of the code right? @lin-d-hop you placed the issue as s-2 so might be good also to have your opinion on it. We know it's bad but those cases happen to be very seldom, so the number of users affected seems to be pretty low, no? Could be s3 maybe and we could delay solving it for after Spree upgrade in terms of priority?

[HugsDaniel]

The count_on_hand is decremented when the order is placed. I didn't see any inventory check between checkout and "place order", and the bug happens every time for me. It seems silly indeed :/ I'll check the plugins.

Even though it's a solid bug leading to poor user experience for both customers and suppliers, I would agree with @myriamboure on waiting for the Spree upgrade. In the worst case, implementing a method to final check the availability of a variant wouldn't take too long (I wrote most of it already, I'd just have to write the rescuing part).

[sauloperez]

This part of the codebase might change a bit after the upgrade, but I don't remember many changes on that end in Spree 2.0, off the top of my head. It's more about the limited resources we have in the priorities we agreed on.

This issue might as well be s2 (or s3 I'm not sure) but severity doesn't necessarily mean that we have the resources to start working on it right away.

I just wanted us to be aware of the tons of things that we need to do besides this issue.

[myriamboure]

So if I sum up:
1- either there are plugins that can solve this easily
2- either Hugo keeps on finishing the fix he has started to write and commit (means it will require some reviewing time, test, merge, might not be straightforward)
3- either we stop here for now and re-prioritize later and Hugo can move on something else
Let's see what Hugo says about 1 and if not possible then decide.

About the priorities we agreed on though, we said that fixing s1 and s2 bug was our very first priority in the "make what we have great" part of the roadmap ;-) So those s1 and s2 bugs for me should keep going as a side of other projects like Spree upgrade, products chain, etc. So for me if we decide to stop that it means we consider it switch to s3 as doesn't concern so many users for now.

[HugsDaniel]

Actually it doesn't seem to be a bug at all. There is an option in Configuration -> Inventory Settings called Allow Backorders. Disabling it does the job.

The Spree guide says :
"Backorderable Default - Controls whether inventory items at this location can be backordered when they run out. You can still change this on an item-by-item basis as needed."
The item-by-item option seems to be a new Spree feature though, I can't find it anywhere.

The screenshot shows what I get when I place the order with the Allow Backorders option disabled.

EDIT : I just remember @Matt-Yorkley suggested this on #1491, @lin-d-hop you said it didn't solve the issue. Your next comment spells a slightly different issue though.

[kirstenalarsen]

If this was what the problem was we NEED to document this somewhere as this would not be the first time confusion has been caused by people not knowing about this option. Does this link to the need for a clear and coherent super-admin user guide?

On 16 Feb 2018, at 7:16 AM, HugsDaniel [e-mail] wrote:

Actually it doesn't seem to be a bug at all. There is an option in Configuration -> Inventory Settings called Allow Backorders. Disabling it does the job.

The Spree guide https://guides.spreecommerce.org/user/configuring_inventory.html says :
"Backorderable Default - Controls whether inventory items at this location can be backordered when they run out. You can still change this on an item-by-item basis as needed."
The item-by-item option seems to be a new Spree feature though, I can't find it anywhere.

The screenshot shows what I get when I place the order with the Allow Backorders option disabled.
https://user-images.githubusercontent.com/28786869/36278873-70c5f01a-1295-11e8-973e-a329eebbe87c.png
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub https://github.com/openfoodfoundation/openfoodnetwork/issues/2067#issuecomment-366048487, or mute the thread https://github.com/notifications/unsubscribe-auth/ACxryBtPSyKL2gzeIVxxJnDTb7KUdtQjks5tVJC2gaJpZM4R3YY-.

[myriamboure]

Hum... the case was discussed here https://github.com/openfoodfoundation/openfoodnetwork/issues/1335 @HugsDaniel but it seemed that #1031 was not the same issue.
But reading again this issue, @lin-d-hop can you check if it's not the same case that in #1335 rather than #1031?? Can you check if "allow backorders" is disabled on the uk instance?

[HugsDaniel]

It's not exactly the same issue as #1335 but both come from allow_backorders. I re-did all the scenarii from #2067, #1491 and #1335 and all three of them are solved when disabling allow_backorders.

@myriamboure you suggested in #1335 that allow_backorders should be at lower levels, it seems the Spree upgrade will bring that.

@kirstenalarsen IMO we should definitely document it as it seems to be causing redundant issues.

[daniellemoorhead]

@sstead / @myriamboure is this something to be added to the userguide?

[sstead]

We don't have a Super Admin guide at the moment. There are a collection of pages in discourse (under Using OFN - As Super Admin) that describe some parts of the 'Configuration' settings that SuperAdmin can see in the backend. I've added a page to that category describing the Inventory Settings tab, which is where this 'allow backorders' setting is controlled.

[daniellemoorhead]

@myriamboure ☝️ 🎉

[myriamboure]

@daniellemoorhead happy to create a super admin gitbook, WIP :-) @lin-d-hop if you confirm that's an issue with "allow backorder" config please close then. Well spotted @HugsDaniel ;-)

[lin-d-hop]

This feels like groundhog day now :-)

No, this is not an issue with "allow backorders". We have never allowed back orders in UK production and i have just confirmed that it is definitely disabled.

This issue is the issue from #1031. It is the issue that stops us from saving shopfronts for reloading when a user returns.

This issue only crops up when two people order the same product at the same time and you must recreate it with different browser sessions to represent User A and User B in the original description.

There is a real issue here. The only person to ever recreate it successfully is @sstead while testing the solution for #1031. @HugsDaniel Perhaps you could ask @sstead to help you spot it? Though I imagine Sally with have forgotten by now because it was a while ago...?

[myriamboure]

I'm suspecting there is a problem with the way this issue is described. I reproduced exactly the steps you describe @lin-d-hop with two different users using different browsers and it reacts as expected. (I tried 2 different ways)

• https://www.useloom.com/share/6d28d9c5d5c243548a3372c8b2d8a6ac?focus_title=1&muted=1
• https://www.useloom.com/share/2acdf64a25a84da6bc2d86ad73429715

The issue spotted by @sstead was on a different case (but that you hit while working on build the persistent cart feature 1031): https://github.com/openfoodfoundation/openfoodnetwork/issues/1031#issuecomment-296052774
And yourself @lin-d-hop describe it here: https://github.com/openfoodfoundation/openfoodnetwork/issues/1491#issuecomment-321501063
_"If I had an item in my cart, then I logged out, then the item was removed from the OC (either just that item or the supplier), then I log in and go to the shopfront- my cart hangs and I'm unable to proceed through checkout. If I instead login and click straight on the edit my cart dropdown link, I can proceed through checkout and purchase something that's no longer in the OC."_

I tried to do exactly this to reproduce this error but if I log out and log in again my cart is lost (I guess it was while working on persistent cart wishlist #1031 that you spotted this so this is normal I guess as this feature is not implemented yet.)

I tried to reproduce it in another way by reducing stock while the item is already in a cart but before checkout completion, working perfectly:
https://www.useloom.com/share/547041b0bb754470b2dc23e434cd4274

I tried as well by directly removing the producer from the OC: here is a clear bug ! I manage to checkout even if the product has been taken out of the order cycle.
https://www.useloom.com/share/4dcbfb6a0025435ea4014a0780921050
That is the bug mentionned at the bottom of #1491 (not the main description) that was blocking #1031. (It's not clear even in #1491 that there is no connexion at all with "disable allow backorder"...) that bug was the one being fixed in the attempted #1767, not the one you are describing above.
SO I just opened an issue #2091 for that specific bug (which is the real bug blocking #1031 from my unerstanding) to separate it from #1491 and start sorting out the mess.

So @lin-d-hop I'm sorry but if we can't provide clear instructions on how to reproduce the bug described here, I don't really see how we can move forward on that issue. Are you able yourself to reproduce the error you mentionned above? If yes please give more precise instruction as I couldn't reproduce it. Did anyone else could? @sstead did you?
In the description you give "Stock levels of items added to the cart are not checked between cart creation and checkout." I don't think this is true. Any test I did (and @HugsDaniel as well) confirm it does when allow backorders is disabled. So if there is a specific case where it doesn't work we should find it and precise it.

Also would be great to avoid mixing bugs in issues as here is a clear example of the mess it becomes, pretty hard to sort out...

[enricostano]

Is anybody able to consistently reproduce this bug?

If not, we can start solving https://github.com/openfoodfoundation/openfoodnetwork/issues/2091 and open a new issue once we are able to reproduce it again.

@sstead @lin-d-hop if you can reproduce it please rise your hand now :joy:

[lin-d-hop]

Sure, when I have time I'll pick this up again.
In the meantime I'll talk to Sara to see if she can document it in a different way next time.