Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verification of Order might fail if it contains OrderLines exceeding the stock, but compensating OrderLines exist #424

Open
Deric-W opened this issue Dec 13, 2022 · 3 comments
Assignees
Labels

Comments

@Deric-W
Copy link

Deric-W commented Dec 13, 2022

While implementing methods for checking if a UniqueInventory has sufficient stock for completing orders I noticed that there are a few edge cases that might be useful to solve here:

  • Order can have multiple OrderLines per Product -> quantities have to be added
  • Order can have OrderLines with negative Quantity -> solved like the one above
  • UniqueInventoryItems can be missing -> missing Products are assumed to have a Quantity of zero

I implemented two new methods on UniqueInventory (hasSufficientQuantity(ProductIdentifier, Quantity) and hasSufficientQuantity(Order)) and can create a PR if you agree with my interpretation.

Deric-W added a commit to Deric-W/salespoint that referenced this issue Dec 13, 2022
… sufficient stock

There are a few edge cases when checking if a `UnqiueInventory` has sufficient
stock for completing orders which are solved by providing tested methods for this.
@odrotbohm
Copy link
Member

odrotbohm commented Dec 13, 2022

Can you elaborate on the premise? Why did you implement the methods in the first place? The entire check whether sufficient stock is available to satisfy an Order is taken care of by the inventory (InventoryManagement in particular). This is the case to encapsulate that logic within the module. Exposing methods on the repository interfaces would incentivize clients to perform these checks themselves, which subverts the goal of the original design.

Fundamentally, the verifications of the Inventory are triggered when you complete the order, and on the surface, I don't quite see anything actually broken (which doesn't necessarily mean I am right 😬). Let me iterate over the issues you brought up:

  • Multiple OrderLines per Product – this can be improved by tweaking our verification algorithm to not simply iterate over the OrderLines but grouping them by Product first to calculate the proper amounts to be reduced. But I actually think that our current algorithm should even catch this, as it would reduce the stock for each of the OrderLine instances and – if insufficient – would eventually end up, rejecting the entire order. I guess an integration test case for that case would help us find out.

  • OrderLines with negative Quantity – I guess we could prevent negative quantities right away, but that would prevent projects to implement returns by e.g. allowing an admin to create a return order that would use these negative values to indicate that some customer has returned a few items. The negative values would then still update the inventory properly, AFAICT. What problem did you see with that scenario in the first place? Also, I'd like to see a failing test case against the current API to indicate we actually have a problem to solve.

  • UII missing – in that case, no MII present would cause the error to be rejected, too (see InventoryManagement.verify(…)) and thus indicate the bug that the developer has failed to create InventoryItems of either kind when adding a Product. I'd have to reiterate on whether we could do something about this, but we so far haven't done anything about that as ultimately, the UniqueInventory and MultiInventory exist within an app at the same time and to automate the inventory update, we currently consider the sole presence of an II in either of the flavors to indicate, which of them to interact with.

To me, it feels like it might make sense to discuss these items individually as they each leave a few things to discuss and explore and trying to elaborate on them in parallel might get a bit confusing. I'd still like to hear about your immediate response before deciding how to proceed in that regard.

@Deric-W
Copy link
Author

Deric-W commented Dec 14, 2022

Thank you for your response.

Use Case

My use case was highlighting orders in the front end which can be completed.
I tought that exposing this logic would prevent users from reinventing the wheel.

negative Quantities

I do have no problems with negative quantities (please dont remove them) but I noticed that completing an Order with mixed negative and positive quantities can fail even if the sum is in stock depending on the order in which the OrderLines are completed, for example if the quantites 11 and -4 are applied to an inventory with 10 in stock.

UII missing

You are right.

@odrotbohm
Copy link
Member

I guess the way to go would be to use the currently existing Inventory interfaces to call ….findByProductIdentifier(…) to avoid the user adding more elements to the Cart in the first place. That's essentially what completing an order does eventually, anyway. But there's no reason that either some presentation logic or the logic adding items to the Cart could not check that beforehand.

You're right regarding the processing of the individual quantities. A single OrderLine exceeding the currently available quantity would cause the order to rollback, even if there was another compensating OrderLine that prevents the threshold from being met. I guess it makes sense to tweak the verification algorithm to take that into account.

@odrotbohm odrotbohm changed the title add methods for checking if UniqueInventory has sufficient stock Verification of Order might fail if it contains OrderLines exceeding the stock, but compensating OrderLines exist Dec 14, 2022
@odrotbohm odrotbohm self-assigned this Dec 14, 2022
@odrotbohm odrotbohm changed the title Verification of Order might fail if it contains OrderLines exceeding the stock, but compensating OrderLines exist Verification of Order might fail if it contains OrderLines exceeding the stock, but compensating OrderLines exist Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants