Skip to content

Conversation

chrom
Copy link
Contributor

@chrom chrom commented Dec 31, 2017

No description provided.

* @var array
*/
protected $_websites;
private $websites;
Copy link

Choose a reason for hiding this comment

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

This is legacy class, we need to follow backward compatibility
So need to revert all changes in this class
We change the only if ($this->productIsSalable->isSalable condition

*/
declare(strict_types=1);

namespace Magento\InventorySalesAlert\Api;
Copy link

Choose a reason for hiding this comment

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

More related name is InventoryProductAlert (now we have ProductAlert module but not SalesAlert)
NAmespace should be Magento\ProductAlert\Model\...

*
* @api
*/
interface ProductIsSalableInterface
Copy link

Choose a reason for hiding this comment

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

This interface should be placed in current ProductAlert module and should be part of current implementation
ProductAlert module must work without new InventoryProductAlert

* @param \Magento\InventoryApi\Api\IsProductInStockInterface $stockItem
*/
public function __construct(
StockResolver $stockResolver = null,
Copy link

Choose a reason for hiding this comment

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

It is new code, so parameters should be not null (without '= null')

StockResolver $stockResolver = null,
IsProductInStockInterface $stockItem = null
) {
$this->stockItem = $stockItem ?: ObjectManager::getInstance()->get(IsProductInStock::class);
Copy link

Choose a reason for hiding this comment

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

This trick is needed only for backward compatibility, but i this case it is new code

<?xml version="1.0"?>
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="urn:magento:framework:Module/etc/module.xsd">
<module name="Magento_InventorySalesAlert" setup_version="0.1.0">
Copy link

Choose a reason for hiding this comment

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

use 1.0.0 version

/**
* Is product salable
*
* Used fully qualified namespaces in annotations for proper work of WebApi request parser
Copy link

Choose a reason for hiding this comment

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

Remove this comments because this interface is not part of Web-API

{
/**
* @param ProductInterface $product
* @param string $websiteCode
Copy link

Choose a reason for hiding this comment

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

Pls use Magento code style for formatting

*/
public function isSalable(
ProductInterface $product,
string $websiteCode,
Copy link

Choose a reason for hiding this comment

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

The second parameter should be only $website
This is related to current implementation

*/
-->
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:ObjectManager/etc/config.xsd">
<preference for="Magento\InventorySalesAlert\Api\ProductIsSalableInterface" type="Magento\InventorySalesAlert\Model\ProductIsSalable"/>
Copy link

Choose a reason for hiding this comment

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

Default implementation (related to current code with calling $product->isSalable()) of ProductIsSalableInterface is missed

@naydav naydav mentioned this pull request Jan 2, 2018
@naydav
Copy link

naydav commented Jan 11, 2018

We try to resolve this issue in more global way

Implementation will be finished in
#387
#388

@naydav naydav closed this Jan 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants