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

Prevent Default Source/Stock to be deleted #126

Merged
merged 5 commits into from
Oct 12, 2017

Conversation

bartekszymanski
Copy link
Contributor

Description

Fixed Issues (if relevant)

  1. Prevent Default Source/Stock to be deleted #120: Prevent Default Source/Stock to be deleted

Manual testing scenarios

  1. Login to Magento admin
  2. Go to Manage Stock
  3. Try to delete default stock (id:1) from grid or detail page

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)

@larsroettig larsroettig self-requested a review October 11, 2017 11:32
throw new CouldNotDeleteException(__('Default Stock could not be deleted.'));
}

return null;
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 return
We should return nothing (void)

public function beforeDeleteById(StockRepositoryInterface $subject, int $stockId)
{
if ($stockId === $this->defaultStockProvider->getId()) {
throw new CouldNotDeleteException(__('Default Stock could not be deleted.'));
Copy link

Choose a reason for hiding this comment

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

Need to cover this check with web-api test
Example of negative scenarios you could find in MSI web-api tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

* @param StockRepositoryInterface $subject
* @param int $stockId
*
* @return null
Copy link

Choose a reason for hiding this comment

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

return void

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is from http://devdocs.magento.com/guides/v2.2/extension-dev-guide/plugins.html "If the method does not change the argument for the observed method, it should return null" but maybe my thinking was wrong in this case.

@@ -8,4 +8,7 @@
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:ObjectManager/etc/config.xsd">
<preference for="Magento\InventoryCatalog\Api\DefaultStockProviderInterface" type="Magento\InventoryCatalog\Model\DefaultStockProvider"/>
<preference for="Magento\InventoryCatalog\Api\DefaultSourceProviderInterface" type="Magento\InventoryCatalog\Model\DefaultSourceProvider"/>
<type name="Magento\Inventory\Model\StockRepository">
Copy link

Choose a reason for hiding this comment

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

We need to add this plugin not on implementation Magento\Inventory\Model\StockRepository but on interface \Magento\InventoryApi\Api\StockRepositoryInterface

If somebody will replace StockRepository on own implementation our check will be work

@@ -8,4 +8,7 @@
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:ObjectManager/etc/config.xsd">
<preference for="Magento\InventoryCatalog\Api\DefaultStockProviderInterface" type="Magento\InventoryCatalog\Model\DefaultStockProvider"/>
<preference for="Magento\InventoryCatalog\Api\DefaultSourceProviderInterface" type="Magento\InventoryCatalog\Model\DefaultSourceProvider"/>
<type name="Magento\Inventory\Model\StockRepository">
<plugin name="prevent-default-stock-deletion" type="Magento\InventoryCatalog\Plugin\Model\StockRepositoryPlugin" sortOrder="1"/>
Copy link

Choose a reason for hiding this comment

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

As I see in Magento generally used underscore for keys (but not dashe)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, again like in magento2 original code but of course I will fix that

@@ -8,4 +8,7 @@
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:ObjectManager/etc/config.xsd">
<preference for="Magento\InventoryCatalog\Api\DefaultStockProviderInterface" type="Magento\InventoryCatalog\Model\DefaultStockProvider"/>
<preference for="Magento\InventoryCatalog\Api\DefaultSourceProviderInterface" type="Magento\InventoryCatalog\Model\DefaultSourceProvider"/>
<type name="Magento\Inventory\Model\StockRepository">
<plugin name="prevent-default-stock-deletion" type="Magento\InventoryCatalog\Plugin\Model\StockRepositoryPlugin" sortOrder="1"/>
</type>
Copy link

Choose a reason for hiding this comment

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

Also, I think we could expand Delete button object.
We do not need show delete button in admin panel if it is default Stock or Source

* @param StockRepositoryInterface $subject
* @param int $stockId
*
* @return null
Copy link
Member

Choose a reason for hiding this comment

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

why null return void is enough

use Magento\InventoryCatalog\Api\DefaultStockProviderInterface;

/**
* Plugin Stock Repository
Copy link
Member

Choose a reason for hiding this comment

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

Describe better or remove

@@ -8,4 +8,7 @@
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:ObjectManager/etc/config.xsd">
<preference for="Magento\InventoryCatalog\Api\DefaultStockProviderInterface" type="Magento\InventoryCatalog\Model\DefaultStockProvider"/>
<preference for="Magento\InventoryCatalog\Api\DefaultSourceProviderInterface" type="Magento\InventoryCatalog\Model\DefaultSourceProvider"/>
<type name="Magento\Inventory\Model\StockRepository">
<plugin name="prevent-default-stock-deletion" type="Magento\InventoryCatalog\Plugin\Model\StockRepositoryPlugin" sortOrder="1"/>
Copy link
Member

Choose a reason for hiding this comment

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

pls use prevent_default_stock_deletion as name

@naydav naydav added the WIP label Oct 11, 2017
@naydav naydav merged commit 3090cb4 into develop Oct 12, 2017
@maghamed maghamed deleted the prevent-delete-default-stock branch December 11, 2018 18:15
magento-cicd2 pushed a commit that referenced this pull request Jun 8, 2021
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.

5 participants