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
Improved Stock Item configuration #2297
Conversation
This change is intended to make the StockItem configuration pure immutable, representing the indexed and aggregated data. Refer to https://github.com/magento-engcom/msi/wiki/Stock-and-Source-Configuration-design#programming-interface-query-api
This update sets the `nullable` attribute of `value` column to `true` so that it can be possible to nullify values once they are saved.
This commit adds the declaration for the `GetBackorderStatusConfigurationValue` and `SetBackorderStatusConfigurationValue` service. The `SetBackorderStatusConfigurationValue` has been implemented (still to refine), next step is adding integration tests.
In order to reduce the number of routes in Depending on passed parameters, the There are some |
...code/Magento/InventoryConfigurationApi/Api/GetBackorderStatusConfigurationValueInterface.php
Show resolved
Hide resolved
* @param string|null $sourceCode | ||
* @return int|null | ||
*/ | ||
public function execute(string $sku = null, string $sourceCode = null): ?int; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
execute
method definitely creates confusion, given that other methods have clear names and execute
is usually used to trigger Functor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need a method to map to a route which is not one of the above Command Facades because to reduce routes we decided to use one method which proxies to Command Facades.
Since the class name already expresses the intention, what would be a better name for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we look for ways to extract this part related to Web API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, an InventoryConfigurationWebapi
module would be the perfect candidate I guess.
But that doesn't solve the execute
naming issue, given we still want to go with one route per option, differentiating based on parameters in the body.
What would be an appropriate naming for the function called by the get and set web API?
* All APIs to specify Backorder Config Value on the level of SourceItem/Source/Globally | ||
* @api | ||
*/ | ||
interface SetBackorderStatusConfigurationValueInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments as for Get interface apply
<route url="/V1/inventory/configuration/backorders" method="GET"> | ||
<service class="Magento\InventoryConfigurationApi\Api\GetBackorderStatusConfigurationValueInterface" method="execute"/> | ||
<resources> | ||
<resource ref="Magento_Backend::admin"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we look for more specific access level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably yes, something like Magento_InventoryApi::configuration
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it should be sufficient at this point.
*/ | ||
public function forGlobal(int $backorderStatus): void | ||
{ | ||
// TODO should validate allowed values? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo validation would save a lot of time in the future
return; | ||
} | ||
|
||
if (!is_numeric($backorderStatus)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_numeric
might allow values we don't want here (floats, some hex or binary values, larger numbers with powers,
etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it; maybe after introducing validation I can be more specific here
@@ -64,11 +66,6 @@ interface StockItemConfigurationInterface extends \Magento\Framework\Api\Extensi | |||
*/ | |||
public function isQtyDecimal(): bool; | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing methods from the interface would break BC and therefore postpone this feature to the later releases. Should we look for ways to preserve methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed those methods to follow the directive here that asks for immutability. Let me know which way you prefer to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General approach for immutability is still in works, so I'm not sure if we should remove methods right away. Still, it's possible that once this PR is ready we would be working on a release which would allow some changes to the interfaces.
In short, I'd keep it at this point. We can always remove it down the road if it looks like a good idea then
Hi @aleron75 , due to changes in the repository structure (Magento core code is extracted from the MSI code base) we have changed the base branch to Installation Guide may be helpful for local project development. Let me know if you need help or have questions! |
Hi @aleron75 , We are about to complete the MSI project and current issue is out of the planned scope. I'm closing the PR since the feature postponed and moved to the internal backlog. |
Description
This PR is intended as a rework of PR #1553 which addresses issue #1486.
This is still a work in progress solution, labeled as WIP and used to discuss implementation.
In the initial PR I've:
inventory_configuration
tableStockItemConfigurationInterface
and made them pure immutable, representingthe indexed and aggregated data (see https://github.com/magento-engcom/msi/wiki/Stock-and-Source-Configuration-design#programming-interface-query-api)
SourceItemConfigurationInterface
Contribution checklist (*)