Source item management #38

larsroettig · 2017-06-30T22:12:59Z

Description

Fixed Issues (if relevant)

magento/magento2#<issue_number>: Issue title
...

Manual testing scenarios

...
...

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)

- UI part of product assignment from source page

source item management

- Code Review Changes - Implement List and Delete Function - Implement Test for List and Delete Function

maghamed · 2017-07-03T12:34:28Z

app/code/Magento/Inventory/Model/ResourceModel/SourceItem/Collection.php

+            $connection,
+            $resource
+        );
+    }


do we really need to have this constructor if all that it does is just proxying input parameters to parent constructor

maghamed · 2017-07-03T12:36:40Z

app/code/Magento/Inventory/Model/SourceItem.php

+    public function setStatus($status)
+    {
+        $this->setData(SourceItemInterface::STATUS, $status);
+    }


we also need to have next methods in this interface and implementation

getExtensionAttributes()/
setExtensionAttributes(\Magento\InventoryApi\Api\Data\SourceItemExtensionInterface $extensionAttributes)

maghamed · 2017-07-03T12:37:41Z

app/code/Magento/InventoryApi/Api/Data/SourceItemInterface.php

+     * @param bool $status
+     * @return void
+     */
+    public function setStatus($status);


getExtensionAttributes()/
setExtensionAttributes(\Magento\InventoryApi\Api\Data\SourceItemExtensionInterface $extensionAttributes)

maghamed · 2017-07-03T12:40:42Z

app/code/Magento/InventoryApi/Api/Data/SourceItemInterface.php

+    /**
+     * Get source item status.
+     *
+     * @return bool


let's make status as INT
it will help us to reserve possible different statuses except "In STOCK" and "Out of Stock" if we will get some other statuses in future

maghamed · 2017-07-03T12:41:41Z

app/code/Magento/InventoryApi/Api/Data/SourceItemInterface.php

+     * Constants for status value.
+     */
+    const IS_IN_STOCK = TRUE;
+    const IS_OUT_OF_STOCK = FALSE;


let's make values of these constants as 0 and 1

maghamed · 2017-07-03T12:45:37Z

app/code/Magento/InventoryApi/Api/SourceItemRepositoryInterface.php

+     * on getList method, because entity loading way is different for these methods
+     *
+     * @param \Magento\InventoryApi\Api\Data\SourceItemInterface $sourceItem
+     * @return int


return type should be void, not int

maghamed · 2017-07-03T12:47:38Z

app/code/Magento/InventoryApi/Api/Data/SourceItemInterface.php

+
+use Magento\Framework\Api\ExtensibleDataInterface;
+
+interface SourceItemInterface extends ExtensibleDataInterface


Let's add a description what's SourceItem interface represents. i.e. amount of particular product on some particular physical storage.

Also, let's add the description to the SourceInterface

maghamed · 2017-07-03T12:53:59Z

app/code/Magento/InventoryApi/Api/SourceItemRepositoryInterface.php

+     * @return int
+     * @throws \Magento\Framework\Exception\CouldNotSaveException
+     */
+    public function save(\Magento\InventoryApi\Api\Data\SourceItemInterface $sourceItem);


let's make this method to accept an array of SourceItems in other case Repository::save would be performance bottleneck, because this method usually used for syncronization with ERP system and should be high performant.

So, bulk operation should be supported

but it's not clear how to handle multiple statuses, we don't have dedicated interfaces which return statuses for Bulk API.
Like source item IDs 1,2,3,5 saved sucessfully
4 and 6 are failed to save.

Will raise this question on Architectural meeting

@maghamed but than we need some bulk save function the following code have also the same e performance bottleneck. We can discuss this in an architectural call.

/** * @inheritdoc */ public function save(array $sourceItemList) { try { $savedSourceItems = []; foreach ($sourceItemList as $sourceItem) { $this->resourceSource->save($sourceItem); $savedItems[] = $sourceItem->getSourceItemId(); } return $savedSourceItems; } catch (\Exception $e) { $this->logger->error($e->getMessage()); throw new CouldNotSaveException(__('Could not save source item'), $e); } }

I draf some posbile methode

/** * @param SourceItemInterface[] $sourceItemList * @return [] list of saved */ public function multiSave(array $sourceItemList) { $sourceItemData = []; foreach ($sourceItemList as $sourceItem) { $sourceItemData[] = [ SourceItemInterface::SKU => $sourceItem->getSku(), SourceItemInterface::QUANTITY => $sourceItem->getQuantity(), /** @todo only a draft */ ]; } $connection = $this->connection->getConnection(); $connection->insertMultiple( $connection->getTableName(InstallSchema::TABLE_NAME_SOURCE_ITEM), $sourceItemData ); }

maghamed · 2017-07-03T13:05:49Z

app/code/Magento/InventoryApi/Api/SourceItemRepositoryInterface.php

+     * @param \Magento\Framework\Api\SearchCriteriaInterface $searchCriteria
+     * @return \Magento\InventoryApi\Api\Data\SourceItemSearchResultsInterface
+     */
+    public function getList(\Magento\Framework\Api\SearchCriteriaInterface $searchCriteria = null);


this search criteria should not be nullable.

why not for SourceRepository it is also nullable?

- Code Review Changes

- Code review changes - Add new save methode for better performence - Add new test case for save methode - Remove nullable search SearchCriteria and the test case

- Fixes after CR of Source item management #38

…rce-item-management # Conflicts: # app/code/Magento/Inventory/Model/ResourceModel/SourceItem/Collection.php # app/code/Magento/InventoryApi/Api/Data/SourceItemInterface.php

maghamed · 2017-07-04T14:26:02Z

app/code/Magento/Inventory/Model/SourceItemRepository.php

+
+            $connection->insertMultiple(
+                $connection->getTableName(InstallSchema::TABLE_NAME_SOURCE_ITEM),
+                $sourceItemData


insertMultiple - should be done by Resource model, not Repository.

The repository should be agnostic to the precise adapter which is used under the hood.
So, switching MySQL -> Postgres SQL -> Mongo DB should not affect Repository code. Just persistent layer should be affected (in our current conditions it's Resource model).

In your implementation Repository works directly with MySQL connection

maghamed · 2017-07-04T14:28:29Z

app/code/Magento/InventoryApi/Api/Data/SourceItemInterface.php

+    /**
+     * Set source item status.
+     *
+     * @param bool $status


status is not bool anymore

maghamed · 2017-07-04T14:29:14Z

app/code/Magento/InventoryApi/Api/Data/SourceItemInterface.php

+     * Constants for status value.
+     */
+    const IS_IN_STOCK = 0;
+    const IS_OUT_OF_STOCK = 1;


as it's not bool, we could eliminate IS_ prefix

- Source item management #38

- product form customization

- backend part

maghamed · 2017-07-07T05:59:45Z

app/code/Magento/Inventory/Model/OptionSource/SourceItemStatus.php

+/**
+ * Class SourceItemStatus
+ *
+ * @api


Just interfaces should be marked as api. Not implementations

It is source model

- backend part

- fix unit tests

- fix static tests

- fix builds

naydav · 2017-07-10T12:23:08Z

About failed tests:

https://travis-ci.org/magento-engcom/magento2/jobs/251955771
Will be fixed in https://github.com/magento-engcom/magento2/issues/44
app/code/Magento/Inventory/Observer/ProcessSourceItemsObserver.php:16 The class Magento\Inventory\Observer\ProcessSourceItemsObserver declared as final.
Will be discussed on internal Magento architecture meeting

maghamed

We agreed that UNIQUE key would be added to SourceItem table (Source_id, SKU)
and that would be added in a separate module.
InventoryCatalog

maghamed · 2017-07-10T13:48:02Z

app/code/Magento/Inventory/Model/OptionSource/SourceItemStatus.php

+/**
+ * Class SourceItemStatus
+ *
+ * @api


maghamed · 2017-07-10T13:48:50Z

app/code/Magento/Inventory/Model/OptionSource/SourceItemStatus.php

+     * Source items status values
+     */
+    const SOURCE_ITEM_STATUS_OUT_OF_STOCK = 0;
+    const SOURCE_ITEM_STATUS_IN_STOCK = 1;


why should we introduce new constants if we already have these constants declared in SourceItemInterface ?

We agreed that UNIQUE key would be added to SourceItem table (Source_id, SKU)
and that would be added in a separate module.

We have separate task for this
https://github.com/magento-engcom/magento2/issues/42

why should we introduce new constants if we already have these constants declared in SourceItemInterface ?

There are NOT new constants. Constants have been moved so Source model (like as in another source models)

maghamed · 2017-07-10T13:49:35Z

app/code/Magento/Inventory/Model/ResourceModel/Source/Collection.php

@@ -16,6 +16,9 @@
 use Magento\InventoryApi\Api\Data\SourceInterface;
 use Psr\Log\LoggerInterface;

+/**
+ * @api


if you add api PHP Doc Block , you must provide a description how this API supposed to be used

maghamed · 2017-07-10T13:49:57Z

app/code/Magento/Inventory/Model/ResourceModel/SourceItem.php

+use Magento\InventoryApi\Api\Data\SourceItemInterface;
+
+/**
+ * Class SourceItem


don't give useless comments

maghamed · 2017-07-10T13:52:01Z

app/code/Magento/Inventory/Model/ResourceModel/SourceItem.php

+    /**
+     * @param Context $context
+     * @param MultipleSave $multipleSave
+     * @param null $connectionName


why do you have null in PHP Doc block ?

maghamed · 2017-07-10T14:10:35Z

app/code/Magento/Inventory/Observer/SourceItemsProcessor.php

+        $sourceItems = $this->sourceItemRepository->getList($searchCriteria)->getItems();
+
+        $sourceItemMap = [];
+        if ($sourceItems) {


try to avoid unneeded offsets in code. It's harder to read code with offsets

maghamed · 2017-07-10T14:11:28Z

app/code/Magento/Inventory/Observer/SourceItemsProcessor.php

+    private function validateSourceItemData(array $sourceItemData)
+    {
+        if (!isset($sourceItemData[SourceItemInterface::SOURCE_ID])) {
+            throw new InputException(__('Wrong Product to Source relation parameters.'));


parameters -> given

maghamed · 2017-07-10T14:12:22Z

app/code/Magento/Inventory/Observer/SourceItemsProcessor.php

+     * @param array $sourceItems
+     * @return void
+     */
+    private function saveSourceItems(array $sourceItems)


why do we need to extract separate method for such simple and easy to read operation ?

maghamed · 2017-07-10T14:13:59Z

app/code/Magento/Inventory/Ui/DataProvider/Product/Form/Modifier/Sources.php

+use Magento\InventoryApi\Api\Data\SourceItemInterface;
+
+/**
+ * Data provider that provide data about source items


please fix this description

maghamed · 2017-07-10T14:16:51Z

app/code/Magento/InventoryApi/Api/SourceItemSaveInterface.php

+namespace Magento\InventoryApi\Api;
+
+/**
+ * Command for multiple saving of source items


fix the description

- fixes after CR

Add "magento/module-inventory-wishlist" to _metapackage/composer.json

kieronthomas and others added 4 commits June 26, 2017 17:17

source item management

4947eb3

MSI

7280a26

- UI part of product assignment from source page

Merge pull request #36 from jonfres/source-item-management

b27c53b

source item management

MSI:

2ec7748

- Code Review Changes - Implement List and Delete Function - Implement Test for List and Delete Function

maghamed suggested changes Jul 3, 2017

View reviewed changes

larsroettig and others added 5 commits July 3, 2017 18:44

MSI:

1e020f2

- Code Review Changes

MSI: - Code Review Changes

78cec9f

MSI:

aad1755

- Code review changes - Add new save methode for better performence - Add new test case for save methode - Remove nullable search SearchCriteria and the test case

MSI

d0a7ede

- Fixes after CR of Source item management #38

Merge remote-tracking branch 'origin/source-item-management' into sou…

ac93d41

…rce-item-management # Conflicts: # app/code/Magento/Inventory/Model/ResourceModel/SourceItem/Collection.php # app/code/Magento/InventoryApi/Api/Data/SourceItemInterface.php

maghamed reviewed Jul 4, 2017

View reviewed changes

Valeriy Nayda added 6 commits July 4, 2017 17:39

MSI

7630364

- Source item management #38

MSI - Pr38

7b5a1a1

- product form customization

MSI - Pr38

06ac206

- backend part

MSI - Pr38

fc83584

- backend part

MSI - Pr38

984ce8b

- backend part

MSI - Pr38

5296b0d

- backend part

maghamed suggested changes Jul 7, 2017

View reviewed changes

Valeriy Nayda added 4 commits July 10, 2017 12:56

MSI - Pr38

c995aee

- backend part

MSI - Pr38

d18bf44

- fix unit tests

MSI - Pr38

7603f58

- fix static tests

MSI - Pr38

10cd6bf

- fix builds

maghamed suggested changes Jul 10, 2017

View reviewed changes

Valeriy Nayda added 3 commits July 10, 2017 18:14

MSI - Pr38

f3de3dc

- fixes after CR

MSI - Pr38

7272f26

- fixes after CR

MSI - Pr38

1e39c8f

- fixes after CR

naydav merged commit 9100fa9 into develop Jul 13, 2017

vrann added msi and removed msi labels Sep 29, 2017

maghamed added Award: complex Award: special achievement labels Oct 2, 2017

This was referenced Dec 11, 2017

Exception during creating New Stock #280

Closed

Exception during editing Default Stock #281

Closed

Exception during creating New Source #282

Closed

smoskaluk mentioned this pull request Mar 23, 2018

Exception in Low stock report for product with another name on additional storeview #737

Closed

smoskaluk mentioned this pull request May 18, 2018

Installation B2b with MSI fails #1198

Closed

maghamed deleted the source-item-management branch December 11, 2018 18:12

mmansoor-magento pushed a commit that referenced this pull request Dec 16, 2020

Merge pull request #38 from magento-l3/MC-33626

bda26c4

Add "magento/module-inventory-wishlist" to _metapackage/composer.json


		use Magento\Framework\Api\ExtensibleDataInterface;

		interface SourceItemInterface extends ExtensibleDataInterface

Source item management #38

Source item management #38

Conversation

larsroettig commented Jun 30, 2017

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsroettig Jul 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naydav commented Jul 10, 2017

maghamed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naydav Jul 10, 2017 • edited

Choose a reason for hiding this comment

naydav Jul 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsroettig Jul 3, 2017 •

edited

naydav Jul 10, 2017 •

edited

naydav Jul 10, 2017 •

edited