OrchardCore.Commerce icon indicating copy to clipboard operation
OrchardCore.Commerce copied to clipboard

Inventory management (OCC-37)

Open sarahelsaig opened this issue 2 years ago • 4 comments

Concept

Content items which have ProductPart may also have an InventoryPart. If a type has InventoryPart, then additional conditions apply before the product can be added to the cart.

Fields or Properties

Should this use OrchardCore.ContentFields? It would be nicer, I think.

  • Inventory (int/NumericField) The number of items in stock.
  • AllowsBackOrder (bool/BooleanField) If true, ordering is allowed even if Inventory is less or equal to zero.
  • OutOfStockMessage (string/MarkdownField/HtmlField) Enables a specific message for an out of stock product (can be used to give an ETA).
  • IgnoreInventory (bool/BooleanField) If true, inventory is ignored same as not having an InventoryPart at all. This is useful for digital products for example.
  • MinimumOrderQuantity (int/NumericField) The minimum number of this item one can order; ignored if zero or negative.
  • MaximumOrderQuantity (int/NumericField) The maximum number of this item one can order (per day? ever?); ignored if zero or negative.

I have changed the ConsiderInventory from the original proposal into IgnoreInventory so it can be default false.

Events

Should have an IInventoryEvents interface with at least

  • AddingToCart: before item is added to cart; add custom logic e.g. authentication check and cancel adding to cart if needed.
  • OrderCheckedOut: after payment; should contain a default handler for updating the inventory, this is where we'll trigger workflows too.

Services and Providers

  • IProductInventoryService & ProductInventoryService (Product prefix so the implementation doesn't start with the letter I.)
    • Task<int> QueryInventoryAsync(int sku): return the current inventory count. (also suggest a Task<bool> IsAvailableAsync(int sku) as an extension method)
    • Task UpdateInventoryAsync(int sku, int difference, bool reset = false): Normally you want to change the inventory by decrementing with sales amount or incrementing with shipment amount. Reset sets the value to zero first. This should be locking/atomic to prevent problems with simultaneous orders.
  • IProductInventoryProvider & LocalInventoryProvider:
    • Include the actual implementations of the two methods above.
    • Order property.
    • Task<bool> IsApplicableAsync(string sku): check if it product can use this provider. For LocalInventoryProvider this means the presence of the InventoryPart.

Workflows

They are great, but it should be part of a followup issue to tighten the scope.

Project Structure

The events, interfaces and services (excluding LocalInventoryProvider) should go to OrchardCore.Commerce. LocalInventoryProvider and InventoryPart should go it a new module, e.g. OrchardCore.Comerce.Inventory.Local. Future ERP-based implementations would go to OrchardCore.Comerce.Inventory.{ProductName} projects accordingly.

sarahelsaig avatar Jul 31 '22 18:07 sarahelsaig

The simple implementation should simply store the info on the part, but the abstraction it implements should assume remote data. In all but the most simple shops, inventory won't be managed in the Orchard app, but will be an existing system that Orchard Commerce will have to integrate with. The abstraction may also separate cart/order integration from inventory management, which should have lower-level APIs to increment, decrement, change, query, etc. inventory levels for a given SKU.

bleroy avatar Aug 11 '22 19:08 bleroy

Nwazet links for reference:

  • https://github.com/bleroy/Nwazet.Commerce/blob/master/Services/IProductInventoryService.cs
  • https://github.com/bleroy/Nwazet.Commerce/blob/master/Services/ProductInventoryServiceBase.cs

bleroy avatar Aug 11 '22 19:08 bleroy

Good point, an external inventory manager sounds more likely. The internal inventory management service would be a reference/fallback implementation of the same abstraction and creating additional implementations for specific ERP integration would be an exercise for the library consumer.

As for cart management, why is it important to make it detailed like that? I thought just query and change/reserve should be enough, as numeric management of the cart really happens on the client side and we just have to make sure no more is offered than what's available.

sarahelsaig avatar Aug 15 '22 11:08 sarahelsaig

Not sure what you mean by cart management. One thing that may seem unfamiliar in the Nwazet implementation is bundles. Those are composite products that are made of an aggregation of products. When one of the products composing the bundle becomes unavailable, you want the bundle to also become unavailable. Since bundles are not implemented in Orchard.Commerce, this part can be skipped (but might be a thing to keep in mind as an example of interesting extensions that it should be possible to implement).

bleroy avatar Aug 15 '22 17:08 bleroy

@DAud-IcI, a few questions:

  1. Is it only the Product content type that should have an inventory part?
  2. SKUs are normally strings — are they meant to be used as integers instead in the proposed methods, or is that just a typo?
  3. UpdateInventoryAsync() is supposed to be locking; is there a simple way to achieve this in OC? For reference, this is the implementation I have added. It seems to work as expected, but I'm not sure if it's locking: image
  4. IInventoryEvents: these two methods (OrderCheckedOut(), AddingToCart()), where should they be used and what parameters should they take? If you could, please elaborate on this part a bit more in general as I'm a bit confused.
  5. IProductInventoryProvider and LocalInventoryProvider, and their relation to IProductInventoryService's QueryInventoryAsync() and UpdateInventoryAsync() methods: what do you mean by including the implementation of the two above mentioned methods? These two methods are already implemented in ProductInventoryService (on the issue branch, that is). Should both inventory providers be derived from IProductInventoryService just to make it so the methods are available here as well? . I also noticed that IsApplicableAsync() is a method (and Order is a property) of ISortableUpdaterProvider, should these new providers be based on this (much like IPriceProvider or ITaxProvider)? Just like above, a little more elaboration on the goal of these additions would be helpful.

porgabi avatar Nov 16 '22 14:11 porgabi

  1. Should any of the inventory part's fields be displayed when viewing the product on the user-facing site? image
  2. When should inventory be decreased — upon being added to cart, or upon completing checkout?

porgabi avatar Nov 18 '22 16:11 porgabi

  1. I'd say no by default, but it should be possible to create a template that does show it. As usual different businesses use different rules so you need to account for all possibilities. Most businesses will tell you if a product is unavailable or not, some will tell you how many items are in stock, and some -gasp- will lie about it (which is probably illegal in quite a few places) Inventory management will not even be on for all businesses, so even if you go with displaying it, it should be gracefully disappearing when the feature is not on.
  2. That will also depend on the business. doing it on checkout seems like a reasonable default, but ultimately this looks like something that should be orchestrated with a workflow. There's in fact a security reason not to do it on adding to cart: a business' competitor could create a bot to create fake visitors to your site and add products to the cart until your inventory is virtually depleted, then never complete these purchases. You would never be able to sell anything. Doing it on checkout at least creates the hurdle that you need to pay for depleting the inventory.

bleroy avatar Nov 18 '22 18:11 bleroy

  1. No, but you can assume that the content type that has InventoryPart also has ProductPart.
  2. Looks like a typo, fixed.
  3. I don't think you can, locking and async are kind of opposites. I agree that this process should be locked so feel free to replace UpdateInventoryAsync with a synchronous method.
  4. I will write a followup comment in a bit.
  5. Same.
  6. I suggest adding blank template cshtml files for most of them, so they are hidden by default but can be still overridden with admin templates. Please add a nice template for the Inventory field that shows remaining stock in a similar format to the Price above. Also if the business doesn't need inventory info they should be able to simply remove the inventory part without error.
  7. I think we can assume decreasing inventory on the checkout completion (so after payment) for the MVP. We can add a setting later to disable that e.g. if the business wants to do it via workflows when we have more support for those.

sarahelsaig avatar Nov 21 '22 12:11 sarahelsaig

Async can be combined with locking when using SemaphoreSlim

using System.Threading;

namespace MyNamespace
{
    public class AsyncWithLock
    {
        // SemaphoreSlim with initialCount of 1 means only
        // one thread may enter (lock) it at a time
        private readonly SemaphoreSlim _lock = new(initialCount: 1);

        public async void LockingMethodAsync()
        {
            // The first thread to access WaitAsync() will return immediately;
            // all subsequent threads will be blocked until the first thread
            // calls Release()
            await _lock.WaitAsync();
            try
            {
                // single-thread-accessible code here
            }
            finally
            {
                _lock.Release();
            }
        }
    }
}

UR-ScottShew avatar Nov 21 '22 14:11 UR-ScottShew

IInventoryEvents: these two methods (OrderCheckedOut(), AddingToCart()), where should they be used and what parameters should they take? If you could, please elaborate on this part a bit more in general as I'm a bit confused.

I was thinking about adding some extension points, but now I feel like this was a bit over-engineered for the MVP. We can skip the events for this issue.

IProductInventoryProvider and LocalInventoryProvider, and their relation to IProductInventoryService's QueryInventoryAsync() and UpdateInventoryAsync() methods: what do you mean by including the implementation of the two above mentioned methods? These two methods are already implemented in ProductInventoryService (on the issue branch, that is). Should both inventory providers be derived from IProductInventoryService just to make it so the methods are available here as well?

The IProductInventoryProvider implementation should contain the business logic. the IProductInventoryService should just iterate over the IProductInventoryProvider services. The IProductInventoryService is basically a container for all its providers. See the PriceService.AddPricesAsync() for reference.

I also noticed that IsApplicableAsync() is a method (and Order is a property) of ISortableUpdaterProvider, should these new providers be based on this (much like IPriceProvider or ITaxProvider)?

Yes, IProductInventoryProvider should inherit from ISortableUpdaterProvider<T>,

sarahelsaig avatar Dec 16 '22 03:12 sarahelsaig