bleroy / Nwazet.Commerce

Idiomatic commerce module for Orchard CMS.
BSD 3-Clause "New" or "Revised" License
26 stars 21 forks source link

Inventory improvement #92

Open MatteoPiovanelli-Laser opened 7 years ago

MatteoPiovanelli-Laser commented 7 years ago

Thoughts about inventory, caused by working on solving #51

Maybe I could add an "AvailableInventory" property to the ProductPart. When a customer hits Checkout:

All that behaviour is actually quite primitive, in that it only fits a subset of possible cases. I think the concept for inventory has room for improvement in the module.

Any input on this?

Down the line, I may work on a feature to add an interface for more advanced inventory management, but I fear that would require reworking quite a bit of what's in place already, or rather hacky workarounds.

bleroy commented 7 years ago

This is a complex problem, which explains why it hasn't been implemented yet. The main problem as I see it is that if you want to prevent the selling of out of stock products, you have to either have an approximate solution that will fail sometimes (pretty much the current situation, with some careful improvements), or you have to implement long-running transactions.

If you accept that you will sometimes sell more than is available, the process implementation can remain very simple. The worst that can happen is some additional work from your sales department on that rare occasion, as they'll have to contact the customer and find a solution.

Now if you wanted to be foolproof, you'd need to block sales on the product as soon as the available quantity is reached in any active cart, until it's cancelled or times out. That means that in some cases you'll lose a sale you could have done. There's even an attack vector where your competition can fill carts with products to make them appear out of stock, and let those carts expire, then do it again.

I would err on the side of caution, simplicity, and accidentally selling too much on rare cases. More specifically, I would implement the limitation on how much you can add to the cart (but if two customers add to their cart concurrently, we let it happen), and only decrement the stock when the sale transaction actually happens, and as part of that transaction. This is the moment when other carts would start giving warnings to their owners.

Of course, cancelling an order should also increment the stock.

As for where the work should happen, I don't think the payment service would have to do any of the work. It seems like this is a job for IOrderService implementations.

MatteoPiovanelli-Laser commented 7 years ago

I agree that the payment services should know nothing about what is going on in inventory logic. However, as things are in the module, once the ICheckoutService has provided me with the checkout button, things are in the hands of the specific implementation there.

I would need to think a bit more on all the other considerations. I admit I had never thought about possible attacks to the system like the one you described, but it makes sense to factor them in.

bleroy commented 7 years ago

I'm not sure why you'd say that about the checkout button: the checkout service does take over for the duration of the payment operation, but it yields back as soon as that's done and gives control to the order service, which is where it would make the most sense for the work to be done.

MatteoPiovanelli-Laser commented 7 years ago

I will double check what is going on in the code tomorrow. The way I remeber it, the button (in the specific case of the Stripe implementation) has a call to a StripeController, that gets stuff back to the OrderService once it's done. But this is off the top of my head, so I may be missing some of the steps there

bleroy commented 7 years ago

I think that's correct, yes, but I still don't understand why that's an issue.

MatteoPiovanelli-Laser commented 7 years ago

It's not necessarily an issue. I'd just rather implement the checks only once.