PWA-2110: [GraphQL] check_inventory parameter and redundant collectQuoteTotals() call (2.4.4)

[cspruiell]

Problem

PWA-2110: [GraphQL] check_inventory parameter and redundant collectQuoteTotals() call (2.4.4) Need to port changes from PWA-2098 to 2.4.4 and add automated tests and proper doc comments for the new optional parameter.

Solution

Add optional parameter "check_inventory" to GraphQL resolvers to override "Check inventory on cart load" admin setting

Notes

There are changes in this pull request that may not seem directly related to the check_inventory parameter. These changes are to provide more complete type definitions that had not been included in the architecture repo. In several cases, entirely new schema files were also added for this reason.

[kandy]

General question: why should it be controlled by the client side?

[cspruiell]

General question: why should it be controlled by the client side?

This allows the client to control whether they see any inventory-related error messaging when the cart is returned. For example, if an item in my cart has become unavailable and I query the cart with check_inventory: true it will include the error message Some of the products are out of stock with the cart data. If I set check_inventory: false the query will return only the cart data.

[kandy]

@cspruiell Is there any scenarios, where for some cart we need to check inventory on load and for other is not?

[cspruiell]

No, this just provides an opportunity to improve performance slightly when the client isn't going to display those error messages.

[kandy]

@cspruiell so based on your answer I don't think we need these changes as the merchant can control these settings on the server-side and the only reason to have it is if we need to control it for each cart

[cspruiell]

@kandy The purpose of this change is to allow the client to override the server-side setting and gain a slight performance boost when the inventory check is not needed. This has already been implemented and delivered to 2.4.2 and 2.4.3 under https://jira.corp.magento.com/browse/PWA-2098 and https://jira.corp.magento.com/browse/PWA-2109. This ticket is to formally update the architecture schema with the changes made there and add a few more that were missed.

[pdohogne-magento]

@kandy While it can be controlled holistically from the server side, the merchant may want to do it on a per-operation basis. For example, "View Cart" probably wants to display these warning messages, but "Apply gift card to cart" may not. Even with the cart query, they might want the warning message on the dedicated cart page but not in the minicart, but those go through the same query. The global admin setting applies everywhere. It's not intended for a per-cart basis, it's for a per-request type basis where we expect the parameter to be adjusted by the merchant's individual preferences in their PWA implementation.

Of note: the ability to set this on a per-operation basis was an explicit request from the merchant who the tickets @cspruiell linked were for, so it does have a known use case.

[akaplya]

this feature looks like a leak of admin functionality, controlling this setting from the client it is quite possible to turn the system into an inconsistent state, which will cause direct fund losses for merchants due to unintentional oversell.

[cspruiell]

There are changes in this pull request that may not seem directly related to the check_inventory parameter. These changes are to provide more complete type definitions that had not been included in the architecture repo. In several cases, entirely new schema files were also added for this reason.

[kandy]

There is a big concern regarding modularity. Why Inventory module check is added to multiple contexts? The proper solution will be adding a separate query just for this validation in the scope of inventory context. We can composite queries, it will resolve the modularity issue

[pdohogne-magento]

@kandy If we were designing the software from the ground-up, sure. However, the inventory check is already baked into the backend. It's not part of any GraphQL framework or module code; it happens as part of the cart repository whenever the cart object is retrieved from the database. To move it to a separate query we'd need to force it to be bypassed for all existing GQL operations that use it, which is not backwards compatible because it's changing functionality. We could tell the merchant to toggle the global admin setting off if they're going to use the separate query to run the check, but that's adding complexity to the upgrade process and anyone who doesn't toggle the setting but does use a new version of PWA that has the separate query will get a performance hit from running said query instead of the benefit of removing the check in unnecessary places.

Also, unfortunately most of this proposal is a formality; this parameter was already released in an expedited patch to a merchant and is being included in the performance pack, so the parameter can't be removed from most of these places for BiC. It can be introduced and immediately marked as deprecated if we find a clean way to go a different direction, but that's also not ideal. It definitely should have gone through an official proposal review, but time was very tight on the patch and it got pushed out.

[pdohogne-magento]

this feature looks like a leak of admin functionality, controlling this setting from the client it is quite possible to turn the system into an inconsistent state, which will cause direct fund losses for merchants due to unintentional oversell.

@akaplya How will this get the system into an inconsistent state? This does not change the admin setting, this is an override purely for the request the parameter gets sent on; subsequent calls (or even GQL operations in the same HTTP request) will go back to the admin setting if they don't also override the parameter. The implementation behind it even goes back to the admin setting if the parameter isn't passed; it's not a required input.

If it wasn't clear from Cari's earlier comment, it's also important to note that the only effect of setting this parameter false to disable the inventory check is not displaying a warning message that has no impact on functionality. Invalid carts still won't be able to check out, invalid item amounts still won't be able to be added to the cart, etc.

[cpartica]

General question: why should it be controlled by the client side?

I am going to let Tommy Wiebell respond to this. I don't know his user, but I sent him the link to this. After also talking to @akaplya it seems that we maybe rethink this and not give so much flexibility to the frontend/client side and have this as a backend setting. It would be the same thing without the "leak of abstraction" (google it)

There is still a frontend usage for this and we need to sort it out. I am thinking of a decoupled future where the cart is not dependent on inventory and things that you add to the cart can get out of inventory if you leave them in the cart for a while. By having this API like this we're directly coupling Cart with Inventory or intentionally decoupling and letting the frontend decide if they need consistency and validation or not. Is that acceptable? It seems that NOT and that seems fair.

To abstract this idea: The state that we're trying to prevent can happen in another way that doesn't require any action from the same actor.

As we are trying to perfect our rules for what's a "good" API vs a "bad" one, and some of us might have different ideas or visions based on what we know, I say each time we find such things we perfect our rulesets and agree on them.

[akaplya]

this feature looks like a leak of admin functionality, controlling this setting from the client it is quite possible to turn the system into an inconsistent state, which will cause direct fund losses for merchants due to unintentional oversell.

@akaplya How will this get the system into an inconsistent state? This does not change the admin setting, this is an override purely for the request the parameter gets sent on; subsequent calls (or even GQL operations in the same HTTP request) will go back to the admin setting if they don't also override the parameter. The implementation behind it even goes back to the admin setting if the parameter isn't passed; it's not a required input.

@pdohogne-magento You can achieve inconsistency by passing a non purchasable item to the cart. As a result of this action, the cart may apply inappropriate shopping cart price rules and show inappropriate prices, which will lead to false advertising, which is forbidden by law in some states.

Also it is an example of leaking implementation details to the API, which is not good from the clear API standpoint.

Along with this, Magento already has a feature that allows you to define backorders, which means bypassing stock validity, but the decision of this bypass should be made by merchant within admin, not by unprotected client call, that can be easily manipulated.

If you want to disable stock validation on each request, you should implement this feature as backend settings and not give the client a choice to manipulate it. Also, the issue with rules should be addressed, to avoid false advertisment.

[pdohogne-magento]

@akaplya As I said before:

If it wasn't clear from Cari's earlier comment, it's also important to note that the only effect of setting this parameter false to disable the inventory check is not displaying a warning message that has no impact on functionality. Invalid carts still won't be able to check out, invalid item amounts still won't be able to be added to the cart, etc.

This is only a warning message. Whether or not it is displayed (with or without this change) does not affect what prices get displayed, what's allowed to be added to the cart, what's allowed to be ordered, reordered, or backordered, or anything else. There are exactly zero calculations or functional validations or operations that consider the output of this check. It is 100% purely just for a warning that says "you have an invalid item/quantity in your cart." If the warning is there (which it is right now), prices are exactly the same as if the check is disabled and the warning doesn't appear. If disabled, no functional operations change. It is a UI message that is not displayed in most cases where it gets output that takes up processing time, but it is desired in some cases over others, even with different uses of the same query. See: full cart display vs. minicart.

If we could redesign from the ground up it would absolutely be different. But we can't. We don't get to ignore backwards compatibility. We also have a big push for GQL performance improvements, so we want to cut down requests to eliminate as many superfluous operations as possible. This is a time-consuming check whose output is not used in some situations, so it should be able to be bypassed in those situations.

As I mentioned, we have an admin setting already that disables it globally, added by the performance team. But in GQL we want to be able to display the warning in some cases but not others. Do you have a suggestion as to how to do that on a per-request basis in a backwards compatible way without changing default behavior for customers who don't change their PWA implementation, but still not requiring an extra upgrade step for users to keep their performance from degrading when they update their PWA and pick up an additional query that is redundant if the admin setting isn't also disabled?

If we could version the parts of the schema where this applies then we could re-do them in a way that allows for this. But this is not deprecating a small subset of fields and replacing them, this would involve deprecating the entire cart query and everything that can return a cart, which we won't do because of extensibility.

[akaplya]

This is only a warning message. Whether or not it is displayed (with or without this change) does not affect what prices get displayed, what's allowed to be added to the cart, what's allowed to be ordered, reordered, or backordered, or anything else

@pdohogne-magento Not sure I understand the purpose of this change then, Am I understanding you correctly that this argument affects if the warning will be returned along with the response? it is kind of unexplicit.

Btw, as a backward-compatible option, can we request the item availability within the new query? assuming the add to cart does not perform the logic of item validation.

[pdohogne-magento]

Am I understanding you correctly that this argument affects if the warning will be returned along with the response? it is kind of unexplicit.

@akaplya Correct. Currently, the warning message is always returned if the check fails (it appears in the "errors" field of the top-level object next to the "data" field that contains the actual operation output). This parameter adds the ability to disable that warning message if you're not going to use it and not run the check at all.

assuming the add to cart does not perform the logic of item validation.

Add to cart still performs item validation of whatever you're adding to the cart, nothing has changed with functionality. Using this flag disables the warning message that would be output if anything else in the cart has an invalid quantity.

To clarify a bit more, the only situation where the inventory check in question triggers the warning message is if something was added to the cart when it was valid, but the item has since gone out of stock or had its available stock decreased below what is already in the cart. It happens when a cart is looked up from the database through the CartRepository class before any operations are performed on the cart or its contents, but it does not interfere with the functionality of those operations (with or without this change). Even if the check is run, the warning message only gets returned if cart items are requested on the output (this is existing behavior). In this case the implementation of this ticket will check if items are requested or not and auto-bypass the check regardless of this parameter since the message it generates won't be available to the caller either way.

[pdohogne-magento]

Decided not to commit these changes at this time. If this feature is necessary in the future, it will be re-proposed with a different strategy.