Add possibility To Delete Stocks in Admin Panel #101

bartekszymanski · 2017-09-17T13:35:38Z

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)

larsroettig · 2017-09-17T16:14:07Z

app/code/Magento/Inventory/Controller/Adminhtml/Stock/Delete.php

+    {
+        $resultRedirect = $this->resultRedirectFactory->create();
+        $stockId        = $this->getRequest()->getParam(StockInterface::STOCK_ID);
+        if ($stockId !== null) {


if ($stockId=== null) { $this->messageManager->addErrorMessage(__('Wrong request.')); $resultRedirect->setPath('*/*'); return $resultRedirect;

it is shorter and better maintainable @see https://phpmd.org/rules/cleancode.html#elseexpression

larsroettig · 2017-09-17T16:23:36Z

app/code/Magento/Inventory/Controller/Adminhtml/Stock/MassDelete.php

+     */
+    public function execute()
+    {
+        if ($this->getRequest()->isPost()) {


You can also simplify with

if ($this->getRequest()->isPost() !== true) { $this->messageManager->addErrorMessage(__('Wrong request.')); return $this->resultRedirectFactory->create()->setPath('*/*'); } Then you don't nee the else @see https://phpmd.org/rules/cleancode.html#elseexpression

bartekszymanski · 2017-09-17T21:08:41Z

app/code/Magento/Inventory/Controller/Adminhtml/Stock/Delete.php and
app/code/Magento/Inventory/Controller/Adminhtml/Stock/MassDelete.php refactoring according to @larsroettig comments

maghamed · 2017-09-19T12:44:02Z

app/code/Magento/Inventory/Controller/Adminhtml/Stock/Delete.php

+    private $stockRepository;
+
+    /**
+     * @param Context                  $context


Please, don't use such alignments
just make a space after type before the variable declaration

maghamed · 2017-09-19T12:46:47Z

app/code/Magento/Inventory/Controller/Adminhtml/Stock/Delete.php

+    public function execute()
+    {
+        $resultRedirect = $this->resultRedirectFactory->create();
+        $stockId        = $this->getRequest()->getParam(StockInterface::STOCK_ID);


don't try to make the fancy alignment.
It's hard to read and in any case it would be broken along the code evolution

maghamed · 2017-09-19T12:48:42Z

app/code/Magento/Inventory/Controller/Adminhtml/Stock/Edit.php

@@ -49,7 +58,7 @@ public function execute()
        $stockId = $this->getRequest()->getParam(StockInterface::STOCK_ID);
        try {
            $stock = $this->stockRepository->get($stockId);
-
+            $this->registry->register('stock',$stock);


It's preferable to use explicit Extension Points than Registry.
Because usage of Registry is forbidden for newly created code
as it introduces mutable global state

maghamed · 2017-09-19T12:51:49Z

app/code/Magento/Inventory/Controller/Adminhtml/Stock/MassDelete.php

+    /**
+     * @param Context                  $context
+     * @param StockRepositoryInterface $stockRepository
+     * @param Filter                   $massActionFilter


please fix the formatting

maghamed · 2017-09-19T12:54:57Z

app/code/Magento/Inventory/Ui/Component/Control/Stock/DeleteButton.php

+    /**
+     * @return array
+     */
+    public function getButtonData()


Please provide a description for all public methods

maghamed · 2017-09-19T12:56:38Z

app/code/Magento/Inventory/Ui/Component/MassAction/Filter.php

+use Magento\Ui\Component\MassAction\Filter as BaseFilter;
+
+/**
+ * @author  naydav <[email protected]>


please fix the author PHP Doc Block

maghamed · 2017-09-19T12:57:22Z

app/code/Magento/Inventory/Ui/Component/MassAction/Filter.php

+        $component    = $this->filter->getComponent();
+        $dataProvider = $component->getContext()->getDataProvider();
+        $searchResult = $dataProvider->getSearchResult();
+        $ids          = [];


please fix formatting

maghamed · 2017-09-19T13:04:14Z

app/code/Magento/Inventory/Ui/Component/MassAction/Filter.php

+        $ids          = [];
+        foreach ($searchResult->getItems() as $item) {
+            $ids[] = $item->getId();
+        }


I prefer to use next syntax :

array_map(function(\Magento\Framework\Api\Search\DocumentInterface $item) { return $item->getId(); }, $searchResult->getItems());

Doing that way you can ensure the type validation of the array returned

maghamed · 2017-09-19T13:05:01Z

app/code/Magento/Inventory/view/adminhtml/ui_component/inventory_stock_listing.xml

+                        <item name="url" xsi:type="url" path="inventory/stock/massDelete"/>
+                        <item name="confirm" xsi:type="array">
+                            <item name="title" xsi:type="string" translate="true">Delete items</item>
+                            <item name="message" xsi:type="string" translate="true">Are you sure you wan't to delete selected items?</item>


wan't -> want

maghamed · 2017-09-19T13:05:47Z

app/code/Magento/Inventory/Ui/Component/MassAction/Filter.php

+
+/**
+ * @author  naydav <[email protected]>
+ * @todo remove this class after code from Engine\MagentoFix\Ui\Component\MassAction\Filter was deploy to production


was -> would be

maghamed · 2017-09-20T10:16:20Z

app/code/Magento/Inventory/Ui/Component/Control/Stock/GenericButton.php

+    /**
+     * @param Context $context
+     */
+    public function __construct(Context $context)


dependency on Context is not recommended. As Context is a container for other objects.

Use dependencies on the precise components you are dependent on

maghamed · 2017-09-20T10:17:51Z

app/code/Magento/Inventory/Ui/Component/Control/Stock/GenericButton.php

+     */
+    public function getId()
+    {
+        return $this->context->getRequest()->getParam(StockInterface::STOCK_ID) ?? null;


This code
$this->context->getRequest()->getParam(StockInterface::STOCK_ID)
violates the Law of Demeter https://en.wikipedia.org/wiki/Law_of_Demeter
which is one of the good design guidelince for developing Object Oriented software

maghamed · 2017-09-20T10:27:10Z

app/code/Magento/Inventory/Ui/Component/Control/Stock/DeleteButton.php

+/**
+ * Class DeleteButton
+ */
+class DeleteButton extends GenericButton implements ButtonProviderInterface


it's preferable to use Composition over Inheritance,
so try to not inherit from GenericButton, but just inject it as Constructor Dependency

maghamed · 2017-09-20T10:32:22Z

app/code/Magento/Inventory/Ui/Component/MassAction/Filter.php

+        $this->filter->applySelectionOnTargetProvider();
+        $component = $this->filter->getComponent();
+        $dataProvider = $component->getContext()->getDataProvider();
+        $searchResult = $dataProvider->getSearchResult();


I see a violation of encapsulation and Law of Demeter here.
Because current code is equivalent to
$this->filter->getComponent()->getContext()->getDataProvider()->getSearchResult()

@naydav is it really recommended way of applying Filtering?

GitHub issue number magento/magento2#10988

maghamed · 2017-09-20T10:33:57Z

app/code/Magento/Inventory/Controller/Adminhtml/Stock/MassDelete.php

+    /**
+     * @see _isAllowed()
+     */
+    const ADMIN_RESOURCE = 'Magento_Inventory::stock';


in both Delete and MassDelete we have a duplication of this constant, that means that it should be declared in another place.
Because currently, such code violates DRY

navarr · 2017-09-21T23:36:21Z

app/code/Magento/Inventory/Controller/Adminhtml/Stock/MassDelete.php

+            } catch (CouldNotDeleteException $e) {
+                $errorMessage = __('[ID: %1] ', $id) . $e->getMessage();
+                $this->messageManager->addErrorMessage($errorMessage);
+            }


What about NoSuchEntityException?

I think that it is not necessary because deleteById method throw CouldNotDeleteException and this scenario is served. See Magento\InventoryApi\Api\StockRepositoryInterface.

@bartekszymanski correct

-- slight refactoring

-- code style fixes

[Sidecar] MQE-2528: Create automated test for: [MSI] "Tables data updated when SKU of Simple product has been changed from Admin"

larsroettig reviewed Sep 17, 2017

View reviewed changes

bartekszymanski force-pushed the delete-stock branch from 3971aa7 to 3f7f271 Compare September 17, 2017 21:06

maghamed suggested changes Sep 19, 2017

View reviewed changes

bartekszymanski force-pushed the delete-stock branch from 3f7f271 to f6fb89e Compare September 20, 2017 07:47

maghamed suggested changes Sep 20, 2017

View reviewed changes

Add possibility To Delete Stocks in Admin Panel

9300183

bartekszymanski force-pushed the delete-stock branch from f6fb89e to 9300183 Compare September 20, 2017 21:10

Add link to issue

44db36f

navarr reviewed Sep 21, 2017

View reviewed changes

Valeriy Nayda added 2 commits September 27, 2017 14:20

MSI: Add possibility To Delete Stocks in Admin Panel

16bfd46

-- slight refactoring

MSI: Add possibility To Delete Stocks in Admin Panel

a2697d7

-- slight refactoring

maghamed approved these changes Sep 27, 2017

View reviewed changes

MSI: Add possibility To Delete Stocks in Admin Panel

67c0692

-- code style fixes

naydav merged commit 0cc081f into develop Sep 27, 2017

maghamed added Award: advanced Award: special achievement labels Oct 3, 2017

smoskaluk mentioned this pull request Mar 23, 2018

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

Closed

maghamed deleted the delete-stock branch December 11, 2018 18:13

magento-cicd2 pushed a commit that referenced this pull request Apr 1, 2021

Merge pull request #101 from magento-tsg/MQE-2528

7b5d93c

[Sidecar] MQE-2528: Create automated test for: [MSI] "Tables data updated when SKU of Simple product has been changed from Admin"

Add possibility To Delete Stocks in Admin Panel #101

Add possibility To Delete Stocks in Admin Panel #101

Uh oh!

Conversation

bartekszymanski commented Sep 17, 2017

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

Uh oh!

larsroettig Sep 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartekszymanski commented Sep 17, 2017

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!

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!

Uh oh!

larsroettig Sep 17, 2017 •

edited

Loading