Improved Stock Item configuration by aleron75 · Pull Request #2297 · magento/inventory

aleron75 · 2019-06-04T13:52:06Z

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:

Created (uncomplete) declaration of inventory_configuration table
Removed all the command methods which alter Stock Configuration data out of
StockItemConfigurationInterface and made them pure immutable, representing
the indexed and aggregated data (see https://github.com/magento-engcom/msi/wiki/Stock-and-Source-Configuration-design#programming-interface-query-api)
Added SourceItemConfigurationInterface
Started adding configuration options facades
Added integration test for backorders
Added web API for setting/getting backorder status

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

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.

aleron75 · 2019-06-04T13:58:35Z

In order to reduce the number of routes in webapi.xml, we've chosen to implement an additional method (which I called execute()) in the service layer and bind this method to the routes.

Depending on passed parameters, the execute() method proxies the call to the proper one.
In the execute() method, the value is a string because it should be possible to have an optional parameter; declaring it as int didn't work.

There are some TODO comments which indicate things that need to be discussed together.

ishakhsuvarov · 2019-06-06T04:52:52Z

+     * @param string|null $sourceCode
+     * @return int|null
+     */
+    public function execute(string $sku = null, string $sourceCode = null): ?int;


execute method definitely creates confusion, given that other methods have clear names and execute is usually used to trigger Functor

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?

Should we look for ways to extract this part related to Web API?

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?

ishakhsuvarov · 2019-06-06T04:53:12Z

+ * All APIs to specify Backorder Config Value on the level of SourceItem/Source/Globally
+ * @api
+ */
+interface SetBackorderStatusConfigurationValueInterface


Same comments as for Get interface apply

ishakhsuvarov · 2019-06-06T04:53:58Z

+    <route url="/V1/inventory/configuration/backorders" method="GET">
+        <service class="Magento\InventoryConfigurationApi\Api\GetBackorderStatusConfigurationValueInterface" method="execute"/>
+        <resources>
+            <resource ref="Magento_Backend::admin"/>


Should we look for more specific access level?

Probably yes, something like Magento_InventoryApi::configuration?

Yes, I think it should be sufficient at this point.

ishakhsuvarov · 2019-06-06T04:55:22Z

+     */
+    public function forGlobal(int $backorderStatus): void
+    {
+        // TODO should validate allowed values?


Imo validation would save a lot of time in the future

ishakhsuvarov · 2019-06-06T04:57:39Z

+            return;
+        }
+
+        if (!is_numeric($backorderStatus)) {


is_numeric might allow values we don't want here (floats, some hex or binary values, larger numbers with powers,
etc)

got it; maybe after introducing validation I can be more specific here

ishakhsuvarov · 2019-06-06T05:01:54Z

     */
    public function isQtyDecimal(): bool;

-    /**


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?

I removed those methods to follow the directive here that asks for immutability. Let me know which way you prefer to follow.

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

sidolov · 2020-01-27T21:58:18Z

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 1.1-develop-legacy. We will keep this branch for a while to make a smooth transition, in the meanwhile, please, apply changes from the current PR to the new code base structure in 1.1-develop branch.

Installation Guide may be helpful for local project development.

Let me know if you need help or have questions!
Thank you for your collaboration!

sidolov · 2020-02-10T16:54:00Z

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.
Thank you for collaboration!

aleron75 added 10 commits May 11, 2019 20:59

Add (uncomplete) declaration of inventory_configuration table

3f62ec7

Add SourceItemConfigurationInterface

6aa6d80

Update nullable field of inventory_configuration table

4e86729

This update sets the `nullable` attribute of `value` column to `true` so that it can be possible to nullify values once they are saved.

Merge branch '2.3-develop' into MSI-1486-rework

ba1219a

Add integration test for backorders configuration

1510f07

Finish integration tests for backorders configuration value

db2ea80

Replace rollback files with @magentoDbIsolation

947707a

Add web api to get and set backorder status

b4c106c

aleron75 added the WIP label Jun 4, 2019

aleron75 requested review from ishakhsuvarov, maghamed and phoenix128 June 4, 2019 13:52

ishakhsuvarov reviewed Jun 6, 2019

View reviewed changes

m2-community-project Bot assigned aleron75 Jul 4, 2019

ishakhsuvarov mentioned this pull request Jul 8, 2019

MSI-1486: Create new infrastructure for StockItem Configuration #1553

Closed

m2-community-project Bot added Progress: review and removed Progress: review labels Jul 10, 2019

ishakhsuvarov removed WIP labels Jul 10, 2019

ishakhsuvarov changed the title ~~MSI 1486 rework~~ Improved Stock Item configuration Jul 10, 2019

m2-community-project Bot added the Progress: review label Jul 10, 2019

Merge branch '2.3-develop' into MSI-1486-rework

bdd2c7d

ishakhsuvarov added the WIP label Aug 8, 2019

m2-community-project Bot removed the Progress: review label Sep 4, 2019

ishakhsuvarov changed the base branch from 2.3-develop to 1.1-develop September 16, 2019 18:13

ishakhsuvarov changed the base branch from 1.1-develop to 2.3-develop September 16, 2019 21:15

engcom-Bravo mentioned this pull request Oct 4, 2019

Maximum Qty Allowed in Shopping Cart can't be set per store magento/magento2#24788

Closed

sdzhepa mentioned this pull request Oct 23, 2019

Issue 24788 magento/magento2#25012

Closed

4 tasks

sidolov changed the base branch from 2.3-develop to 1.1-develop December 5, 2019 22:59

m2-community-project Bot added the Progress: review label Dec 5, 2019

sidolov changed the base branch from 1.1-develop to 1.1-develop-legacy January 27, 2020 21:47

sidolov closed this Feb 10, 2020

sidolov deleted the MSI-1486-rework branch February 20, 2020 17:40

Conversation

aleron75 commented Jun 4, 2019

Description

Contribution checklist (*)

Uh oh!

aleron75 commented Jun 4, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sidolov commented Jan 27, 2020

Uh oh!

sidolov commented Feb 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants