Add possibility To Delete Stocks in Admin Panel

[bartekszymanski]

Description

Fixed Issues (if relevant)

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

Manual testing scenarios

1. ...
2. ...

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]

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]

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]

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]

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

[maghamed]

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]

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]

please fix the formatting

[maghamed]

Please provide a description for all public methods

[maghamed]

please fix the author PHP Doc Block

[maghamed]

please fix formatting

[maghamed]

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]

wan't -> want

[maghamed]

was -> would be

[maghamed]

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]

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]

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

[maghamed]

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?

[bartekszymanski]

GitHub issue number magento/magento2#10988

[maghamed]

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]

What about NoSuchEntityException?

[bartekszymanski]

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

[maghamed]

@bartekszymanski correct