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

Msi 2182 source entity expansion with pickup location attributes #2267

Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
4da7b8b
MSI-2182 Source Entity expansion with Pickup Location attributes.
Apr 16, 2019
10401ca
Merge remote-tracking branch 'origin/store-pickup' into MSI-2182-Sour…
novikor May 18, 2019
f6d9bb5
Source Entity expansion with Pickup Location attributes #2182.
novikor May 18, 2019
2314c9f
Source Entity expansion with Pickup Location attributes #2182.
novikor May 18, 2019
748fd78
Source Entity expansion with Pickup Location attributes #2182.
novikor May 18, 2019
7426756
Source Entity expansion with Pickup Location attributes #2182.
novikor May 18, 2019
7559630
Source Entity expansion with Pickup Location attributes #2182.
novikor May 18, 2019
551568b
Msi 2182 source entity expansion with pickup location attributes.
novikor May 20, 2019
143b67f
Msi 2182 source entity expansion with pickup location attributes.
novikor May 20, 2019
f46fe29
Merge remote-tracking branch 'origin/store-pickup' into MSI-2182-Sour…
May 21, 2019
28c8dda
Msi 2182 source entity expansion with pickup location attributes.
May 22, 2019
f57691c
Msi 2182 source entity expansion with pickup location attributes.
May 23, 2019
3b64018
Msi 2182 source entity expansion with pickup location attributes.
May 23, 2019
e276996
Msi 2182 source entity expansion with pickup location attributes.
May 23, 2019
86f5f3a
Msi 2182 source entity expansion with pickup location attributes.
May 23, 2019
0a20315
Merge remote-tracking branch 'origin/store-pickup' into MSI-2182-Sour…
novikor May 24, 2019
ca323b2
Merge branch 'store-pickup' into MSI-2182-Source-Entity-expansion-wit…
ishakhsuvarov May 30, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 0 additions & 16 deletions app/code/Magento/InventoryInStorePickup/Model/PickupLocation.php
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,6 @@ class PickupLocation implements PickupLocationInterface
*/
private $phone;

/**
* @var string[]|null
*/
private $openHours;

/**
* @var string|null
*/
Expand All @@ -117,7 +112,6 @@ class PickupLocation implements PickupLocationInterface
* @param string|null $street
* @param string|null $postcode
* @param string|null $phone
* @param string[]|null $openHours
* @param PickupLocationExtensionInterface|null $extensionAttributes
*/
public function __construct(
Expand All @@ -136,7 +130,6 @@ public function __construct(
?string $street = null,
?string $postcode = null,
?string $phone = null,
?array $openHours = null,
?PickupLocationExtensionInterface $extensionAttributes = null
) {
$this->sourceCode = $sourceCode;
Expand All @@ -154,7 +147,6 @@ public function __construct(
$this->street = $street;
$this->postcode = $postcode;
$this->phone = $phone;
$this->openHours = $openHours;
$this->extensionAttributes = $extensionAttributes;
}

Expand Down Expand Up @@ -278,14 +270,6 @@ public function getPhone(): ?string
return $this->phone;
}

/**
* @inheritdoc
*/
public function getOpenHours(): ?array
{
return $this->openHours;
}

/**
* @inheritdoc
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use Magento\InventoryInStorePickupApi\Api\Data\PickupLocationInterface;
use Magento\InventoryInStorePickupApi\Api\Data\PickupLocationInterfaceFactory;
use Magento\InventoryInStorePickupApi\Model\Mapper\CreateFromSourceInterface;
use Magento\InventoryInStorePickupApi\Model\Mapper\PreProcessorInterface;

/**
* @inheritdoc
Expand Down Expand Up @@ -47,28 +48,49 @@ public function __construct(
* @inheritdoc
* @throws \InvalidArgumentException
*/
public function execute(SourceInterface $source, array $map): PickupLocationInterface
public function execute(SourceInterface $source, array $map, array $preProcessors): PickupLocationInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

Why preProcessors aren't constructor injection? Adequate type checking and hinting is missing as well.

Edit: Would be resolved in a different PR after additional discussion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well.
What about creating something kind of PreProcessorsList (like RouterList), mark it as @api and inject into constructor then?

{
$mappedData = $this->extractDataFromSource($source, $map);
$data = $this->preparePickupLocationFields($mappedData);
$data = $this->extractDataFromSource($source, $map);
$data = $this->preProcessData($source, $data, $preProcessors);
$data = $this->preparePickupLocationFields($data, $map);

return $this->pickupLocationFactory->create($data);
}

/**
* @param array $mappedData
* @param SourceInterface $source
* @param array $data
* @param PreProcessorInterface[] $preProcessors
*
* @return array
*/
private function preparePickupLocationFields(array $mappedData): array
private function preProcessData(SourceInterface $source, array $data, array $preProcessors)
{
foreach ($preProcessors as $field => $processor) {
if (array_key_exists($field, $data)) {
$data[$field] = $processor->process($source, $data[$field]);
}
}

return $data;
}

/**
* @param array $sourceData
* @param array $map
*
* @return array
*/
private function preparePickupLocationFields(array $sourceData, array $map): array
{
$pickupLocationExtension = $this->extensionAttributesFactory->create(PickupLocationInterface::class);
$pickupLocationMethods = get_class_methods(PickupLocationInterface::class);
$data = [
'extensionAttributes' => $pickupLocationExtension
];

foreach ($mappedData as $pickupLocationField => $value) {
foreach ($sourceData as $sourceField => $value) {
$pickupLocationField = $map[$sourceField];
if ($this->isExtensionAttributeField($pickupLocationField)) {
$methodName = $this->getSetterMethodName($this->getExtensionAttributeFieldName($pickupLocationField));

Expand All @@ -91,15 +113,15 @@ private function preparePickupLocationFields(array $mappedData): array
/**
* Extract values from Source according to the provided map.
*
* @param \Magento\InventoryApi\Api\Data\SourceInterface $source
* @param SourceInterface $source
* @param string[] $map
*
* @return array
*/
private function extractDataFromSource(SourceInterface $source, array $map): array
{
$mappedData = [];
foreach ($map as $sourceField => $pickupLocationField) {
$sourceData = [];
foreach (array_keys($map) as $sourceField) {
if ($this->isExtensionAttributeField($sourceField)) {
$methodName = $this->getGetterMethodName($this->getExtensionAttributeFieldName($sourceField));
$entity = $source->getExtensionAttributes();
Expand All @@ -112,10 +134,10 @@ private function extractDataFromSource(SourceInterface $source, array $map): arr
$this->throwException(SourceInterface::class, $sourceField);
}

$mappedData[$pickupLocationField] = $entity->{$methodName}();
$sourceData[$sourceField] = $entity->{$methodName}();
}

return $mappedData;
return $sourceData;
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

declare(strict_types=1);

namespace Magento\InventoryInStorePickup\Model\PickupLocation\Mapper\PreProcessor;

use Magento\InventoryApi\Api\Data\SourceInterface;
use Magento\InventoryInStorePickupApi\Model\Mapper\PreProcessorInterface;

class FrontendName implements PreProcessorInterface
{

Copy link
Contributor

Choose a reason for hiding this comment

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

Just an empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That`s really horrible

/**
* Process Source Field before pass it to Pickup Location
*
* @param SourceInterface $source
* @param string $value Frontend Name Extension Attribute value
*
* @return string
*/
public function process(SourceInterface $source, $value): string
{
if (empty($value)) {
$value = $source->getName();
}

return $value;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

declare(strict_types=1);

namespace Magento\InventoryInStorePickup\Model\Source;

use Magento\Framework\Api\ExtensionAttributesFactory;
use Magento\Framework\DataObject;
use Magento\InventoryApi\Api\Data\SourceInterface;
use Magento\InventoryInStorePickupApi\Api\Data\PickupLocationInterface;

/**
* Set store-pickup related source extension attributes
*/
class InitPickupLocationExtensionAttributes
{
/**
* @var ExtensionAttributesFactory
*/
private $extensionAttributesFactory;

/**
* @param ExtensionAttributesFactory $extensionAttributesFactory
*/
public function __construct(ExtensionAttributesFactory $extensionAttributesFactory)
{
$this->extensionAttributesFactory = $extensionAttributesFactory;
}
/**
* @param SourceInterface $source
*/
public function execute(SourceInterface $source): void
{
if (!$source instanceof DataObject) {
return;
}
$pickupAvailable = $source->getData(PickupLocationInterface::IS_PICKUP_LOCATION_ACTIVE);
Copy link
Contributor

Choose a reason for hiding this comment

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

In the last review I wasn't clear enough.

Just wanted to make sure that this is the best idea we have to load newly introduced fields.

Copy link
Contributor Author

@novikor novikor May 30, 2019

Choose a reason for hiding this comment

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

Well, we could do some magic with converting snake_case field name to camelCase, check if the appropriate getter method exists and then call it...

But I don`t like this idea :)

$frontendName = $source->getData(PickupLocationInterface::FRONTEND_NAME);
$frontendDescription = $source->getData(PickupLocationInterface::FRONTEND_DESCRIPTION);

$extensionAttributes = $source->getExtensionAttributes();

if ($extensionAttributes === null) {
$extensionAttributes = $this->extensionAttributesFactory->create(SourceInterface::class);
/** @noinspection PhpParamsInspection */
$source->setExtensionAttributes($extensionAttributes);
}

$extensionAttributes
->setIsPickupLocationActive((bool)$pickupAvailable)
->setFrontendName($frontendName)
->setFrontendDescription($frontendDescription);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,25 +7,24 @@

namespace Magento\InventoryInStorePickup\Plugin\InventoryApi\SourceRepository;

use Magento\Framework\Api\ExtensionAttributesFactory;
use Magento\InventoryApi\Api\Data\SourceInterface;
use Magento\InventoryApi\Api\Data\SourceSearchResultsInterface;
use Magento\InventoryApi\Api\SourceRepositoryInterface;
use Magento\InventoryInStorePickupApi\Api\Data\PickupLocationInterface;
use Magento\InventoryInStorePickup\Model\Source\InitPickupLocationExtensionAttributes;

class LoadInStorePickupOnGetListPlugin
{
/**
* @var ExtensionAttributesFactory
* @var InitPickupLocationExtensionAttributes
*/
private $extensionAttributesFactory;
private $setExtensionAttributes;

/**
* @param ExtensionAttributesFactory $extensionAttributesFactory
* @param InitPickupLocationExtensionAttributes $setExtensionAttributes
*/
public function __construct(ExtensionAttributesFactory $extensionAttributesFactory)
{
$this->extensionAttributesFactory = $extensionAttributesFactory;
public function __construct(
InitPickupLocationExtensionAttributes $setExtensionAttributes
) {
$this->setExtensionAttributes = $setExtensionAttributes;
Copy link
Contributor

Choose a reason for hiding this comment

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

While this seems to be an excessive extraction, it seems to be the bet solution if we don't figure out to load data in a better way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, as a variant, it probably would be nice to extract this logic into a chain of responsibility.

}

/**
Expand All @@ -40,18 +39,12 @@ public function __construct(ExtensionAttributesFactory $extensionAttributesFacto
public function afterGetList(
SourceRepositoryInterface $subject,
SourceSearchResultsInterface $sourceSearchResults
):SourceSearchResultsInterface {
foreach ($sourceSearchResults->getItems() as $source) {
$extensionAttributes = $source->getExtensionAttributes();

if ($extensionAttributes === null) {
$extensionAttributes = $this->extensionAttributesFactory->create(SourceInterface::class);
$source->setExtensionAttributes($extensionAttributes);
}

$pickupAvailable = $source->getData(PickupLocationInterface::IS_PICKUP_LOCATION_ACTIVE);
$extensionAttributes->setIsPickupLocationActive((bool)$pickupAvailable);
}
): SourceSearchResultsInterface {
$items = $sourceSearchResults->getItems();
array_walk(
$items,
[$this->setExtensionAttributes, 'execute']
);

return $sourceSearchResults;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,24 +7,25 @@

namespace Magento\InventoryInStorePickup\Plugin\InventoryApi\SourceRepository;

use Magento\Framework\Api\ExtensionAttributesFactory;
use Magento\Framework\DataObject;
use Magento\InventoryApi\Api\Data\SourceInterface;
use Magento\InventoryApi\Api\SourceRepositoryInterface;
use Magento\InventoryInStorePickupApi\Api\Data\PickupLocationInterface;
use Magento\InventoryInStorePickup\Model\Source\InitPickupLocationExtensionAttributes;

class LoadInStorePickupOnGetPlugin
{
/**
* @var \Magento\Framework\Api\ExtensionAttributesFactory
* @var InitPickupLocationExtensionAttributes
*/
private $extensionAttributesFactory;
private $setExtensionAttributes;

/**
* @param \Magento\Framework\Api\ExtensionAttributesFactory $extensionAttributesFactory
* @param InitPickupLocationExtensionAttributes $setExtensionAttributes
*/
public function __construct(ExtensionAttributesFactory $extensionAttributesFactory)
{
$this->extensionAttributesFactory = $extensionAttributesFactory;
public function __construct(
InitPickupLocationExtensionAttributes $setExtensionAttributes
) {
$this->setExtensionAttributes = $setExtensionAttributes;
}

/**
Expand All @@ -39,17 +40,8 @@ public function __construct(ExtensionAttributesFactory $extensionAttributesFacto
public function afterGet(
SourceRepositoryInterface $subject,
SourceInterface $source
):SourceInterface {
$pickupAvailable = $source->getData(PickupLocationInterface::IS_PICKUP_LOCATION_ACTIVE);

$extensionAttributes = $source->getExtensionAttributes();

if ($extensionAttributes === null) {
$extensionAttributes = $this->extensionAttributesFactory->create(SourceInterface::class);
$source->setExtensionAttributes($extensionAttributes);
}

$extensionAttributes->setIsPickupLocationActive((bool)$pickupAvailable);
): SourceInterface {
$this->setExtensionAttributes->execute($source);

return $source;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,15 @@

namespace Magento\InventoryInStorePickup\Plugin\InventoryApi\SourceRepository;

use Magento\Framework\DataObject;
use Magento\InventoryApi\Api\Data\SourceInterface;
use Magento\InventoryApi\Api\SourceRepositoryInterface;
use Magento\InventoryInStorePickupApi\Api\Data\PickupLocationInterface as Location;

/**
* Set data to Source itself from its extension attributes to save
* these values to `inventory_source` DB table
*/
class SaveInStorePickupPlugin
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add description while we are here

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

{
/**
Expand All @@ -24,11 +30,13 @@ class SaveInStorePickupPlugin
public function beforeSave(
SourceRepositoryInterface $subject,
SourceInterface $source
):array {
): array {
$extensionAttributes = $source->getExtensionAttributes();

if ($extensionAttributes !== null && $extensionAttributes->getIsPickupLocationActive() !== null) {
$source->setIsPickupLocationActive($extensionAttributes->getIsPickupLocationActive());
if ($extensionAttributes !== null && $source instanceof DataObject) {
$source->setData(Location::IS_PICKUP_LOCATION_ACTIVE, $extensionAttributes->getIsPickupLocationActive());
$source->setData(Location::FRONTEND_NAME, $extensionAttributes->getFrontendName());
$source->setData(Location::FRONTEND_DESCRIPTION, $extensionAttributes->getFrontendDescription());
}

return [$source];
Expand Down
Loading