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

Synchronization between sourceItem and stockItem deletion for defaultSourceId #269

Merged
merged 25 commits into from
Dec 21, 2017

Conversation

sinisa86
Copy link
Contributor

@sinisa86 sinisa86 commented Dec 8, 2017

Synchronization between sourceItem and stockItem deletion for defaultSourceId

Fixed Issues (if relevant)

  1. Provide possibility to enable/disable synchronization logic of stock deduction in legacy tables at checkout time #216: Provide possibility to enable/disable synchronization logic of stock deduction in legacy tables at checkout time
  2. Add Legacy Stock Synchronization at the time of Deleting StockItem (Legacy) #218: Add Legacy Stock Synchronization at the time of Deleting StockItem (Legacy)
  3. Add Update to the Legacy Inventory (StockItem) when Default Source item deleted  #219: Add Update to the Legacy Inventory (StockItem) when Default Source item deleted

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)

@sinisa86 sinisa86 force-pushed the delete_source_items_stock_items_synchronization branch from e069578 to fa3bf14 Compare December 12, 2017 14:19
valentinych pushed a commit that referenced this pull request Dec 14, 2017
…rce_items_stock_items_synchronization

# Conflicts:
#	app/code/Magento/InventoryApi/Test/Api/StockRepository/DeleteTest.php
#	app/code/Magento/InventoryApi/Test/_files/source_items.php
@naydav naydav added the WIP label Dec 20, 2017
@@ -0,0 +1,103 @@
<?php
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls remove this file

$sourceItemsDelete->execute($sourceItems);
try {
$sourceItemsDelete->execute($sourceItems);
} catch (NoSuchEntityException $e) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need wrap this code in try catch? Looks like sourceItemsDelete does not throw NoSuchEntityException

[
StockItemInterface::STOCK_ID . ' = ?' => $this->defaultStockProvider->getId(),
StockItemInterface::PRODUCT_ID . ' = ?' => $productId,
'website_id = ?' => 0
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to check how do we write website value in this table
Looks like we need to use some service for resolving website id (instead of hardcoded value)

return;
}

$productIds = $this->productIdLocator->retrieveProductIdsBySkus([$sourceItem->getSku()]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls merge develop branch
We have provided global refactoring of product ids by sku resolving

Pls use \Magento\InventoryCatalog\Model\GetProductIdsBySkusInterface

* Plugin help to delete related entries from the legacy catalog inventory tables cataloginventory_stock_status and
* cataloginventory_stock_item if deleted source item is default source item.
*/
class DeleteLegacyCatalogInventoryPlugin
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class name and description are not equal to behavior of this class

use Magento\CatalogInventory\Api\StockItemCriteriaInterfaceFactory;
use Magento\CatalogInventory\Api\Data\StockItemCollectionInterface;

class DeleteLegacyCatalogInventoryPluginTest extends TestCase
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change name
We does not test plugin but we test some testcase

* @magentoDataFixture ../../../../app/code/Magento/InventoryApi/Test/_files/stocks.php
* @magentoDataFixture ../../../../app/code/Magento/InventoryApi/Test/_files/source_items.php
* @magentoDataFixture ../../../../app/code/Magento/InventoryApi/Test/_files/stock_source_link.php
* @magentoDbIsolation enabled
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magentoDbIsolation is enabled by default

We have problem with this option only if we do not apply any fixtures
But in our case it is extra configuration

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to do this in all cases
It is related to the other notices

$stockItemsBeforeDelete = $this->oldStockItemRepository->getList($criteria)->getItems();
$this->assertCount(1, $stockItemsBeforeDelete);

$searchCriteria = $this->searchCriteriaBuilder
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we have a few times with code
Maybe it will be better to create sugar service (not interface but implementation) and put under hood this logic (I mean creating and configuring of Search Criteria)

$this->sourceItemsSave->execute([$sourceItem]);

/** @var StockItemCollectionInterface $collectionBeforeChange */
$stockItemsAfterUpdate = $this->oldStockItemRepository->getList($criteria)->getItems();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls use legacy instead of old

*/
public function testThatReservationPlacedDefersUpdatingLegacyStockWhenCanSubtractOff()
{
$this->productRepository = Bootstrap::getObjectManager()->get(ProductRepositoryInterface::class);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra line
We have productRepository initialization in setUp

bartekszymanski and others added 8 commits December 20, 2017 22:39
…faultSourceId #269

-- Set to zero legacy catalocinventory data if deleted source item which is related to default source
…faultSourceId #269

-- set data to legacy catalog inventory at source items related to default source save
…faultSourceId #269

-- set data to legacy catalog inventory at source items related to default source save
…faultSourceId #269

-- Apply inventory data changes  (qty, stock status) to legacy CatalogInventory (cataloginventory_stock_status and
   cataloginventory_stock_item tables) at the time when Reservation(-s) have been appended using MSI APIs,
   and these reservation(-s) correspond to Default Stock
…faultSourceId #269

-- Apply inventory data changes  (qty, stock status) to legacy CatalogInventory (cataloginventory_stock_status and
   cataloginventory_stock_item tables) at the time when Reservation(-s) have been appended using MSI APIs,
   and these reservation(-s) correspond to Default Stock
Valeriy Nayda added 3 commits December 21, 2017 17:23
…faultSourceId #269

-- build fixes, new task introducing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants