-
Notifications
You must be signed in to change notification settings - Fork 247
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
Changes from all commits
4da7b8b
10401ca
f6d9bb5
2314c9f
748fd78
7426756
7559630
551568b
143b67f
f46fe29
28c8dda
f57691c
3b64018
e276996
86f5f3a
0a20315
ca323b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
{ | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just an empty line There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
|
||
/** | ||
|
@@ -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; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add description while we are here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
{ | ||
/** | ||
|
@@ -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]; | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
(likeRouterList
), mark it as@api
and inject into constructor then?