Msi 2182 source entity expansion with pickup location attributes #2267

novikor · 2019-05-18T19:40:41Z

Description (*)

Added the next fields:

Frontend Name
Frontend Description (wysiwyg field, label is not shown in BO)
Implemented admin UI for them;
Refactored some code;

Fixed Issues (if relevant)

Source Entity expansion with Pickup Location attributes #2182:Source Entity expansion with Pickup Location attributes

Manual testing scenarios (*)

Go to source edit page
Make sure pickup location fieldset is not visible until the source is used in the store pickup
Check that new fields are present
Make sure you are able to save them

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)

Add rows and extension attributes to Source.

…ce-Entity-expansion-with-Pickup-Location-attributes

open_hours -> frontend_description

Frontend name and frontend description admin UI. Some refactoring.

Improved tests coverage

Show/Hide the pickup location fieldset depending from `is_pickup_location_active` value

Adjusted Pickup Location entity

swnsma · 2019-05-19T15:26:11Z

app/code/Magento/InventoryInStorePickup/Test/_files/pickup_location.php

@@ -0,0 +1,54 @@
+<?php


There is already fixture to make source -> pickup location: app/code/Magento/InventoryInStorePickup/Test/_files/source_pickup_location_attributes.php
Do we really need extra one?

swnsma · 2019-05-19T15:27:12Z

app/code/Magento/InventoryInStorePickup/Test/Integration/PickupLocation/MapperTest.php

@@ -85,86 +88,25 @@ public function testWrongMappingForPickupLocation()
    }

    /**
-     * @magentoDataFixture ../../../../app/code/Magento/InventoryApi/Test/_files/source.php
+     * @magentoDataFixture ../../../../app/code/Magento/InventoryInStorePickup/Test/_files/pickup_location.php
     */
    public function testMapPickupLocation()


What about case:
Source.ExtensionAttributes.Field1 -> PickupLocation.ExtensionAttributes.Field2 ?

Why did you not cover it before?)

Okay, I`ll check

@swnsma , I afraid it is out of scope. Feel free to do it by yourself.

Fixed static tests

Set frontend name default value.

novikor · 2019-05-20T21:09:14Z

TODO: parse FrontendDescription WYSIWYG content while creating pickup location.
See Magento\Widget\Model\Template\Filter

novikor · 2019-05-21T14:30:48Z

@ishakhsuvarov , - the PR is not ready yet.
@swnsma is going to implement a chain of responsibility for the pickup location mapper,
and I will reuse this logic then

…ce-Entity-expansion-with-Pickup-Location-attributes

ishakhsuvarov

Whole mechanism of working with specific fields of SourceInteface via DataObject implementation does not seem correct.

Should we research additional ways? Something cleaner like a Resource maybe? Or maybe you have more approaches on your mind?

ishakhsuvarov · 2019-05-21T21:35:11Z

app/code/Magento/InventoryInStorePickup/Model/Source/SetExtensionAttributes.php

+        $this->extensionAttributesFactory = $extensionAttributesFactory;
+    }
+    /**
+     * @param SourceInterface|DataObject $source


Let's keep doc block and contract consistent

ishakhsuvarov · 2019-05-21T21:35:24Z

app/code/Magento/InventoryInStorePickup/Model/Source/SetExtensionAttributes.php

+/**
+ * Set store-pickup related source extension attributes
+ */
+class SetExtensionAttributes


Class name seems to abstract

InitPickupLocationExtensionAttributes ?

app/code/Magento/InventoryInStorePickup/Model/Source/SetExtensionAttributes.php

ishakhsuvarov · 2019-05-21T22:02:33Z

...InventoryInStorePickup/Plugin/InventoryApi/SourceRepository/LoadInStorePickupOnGetPlugin.php

    }

    /**
     * Enrich the given Source Objects with the In-Store pickup attribute
     *
     * @param SourceRepositoryInterface $subject
-     * @param SourceInterface $source
+     * @param SourceInterface|DataObject $source


That's cheating. We can't typehint for SourceInterface and rely on Dataobject in reality

ishakhsuvarov · 2019-05-21T22:02:54Z

...ento/InventoryInStorePickup/Plugin/InventoryApi/SourceRepository/SaveInStorePickupPlugin.php

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

 class SaveInStorePickupPlugin


Let's add description while we are here

Refactor Pickup Locations mapper. Add possibility to pre-process Source data for projection to Pickup Location.

LocalizedException -> InvalidArgumentException

If frontend name is empty, pickup location uses source regular name now.

Fixed jscs tests

Some refactoring: added explicit DataObject check

…ce-Entity-expansion-with-Pickup-Location-attributes

novikor · 2019-05-28T11:04:12Z

Reminder for myself: display Frontend Description label

ishakhsuvarov · 2019-05-30T15:04:16Z

app/code/Magento/InventoryInStorePickup/Model/PickupLocation/Mapper/CreateFromSource.php

@@ -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


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

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

ishakhsuvarov · 2019-05-30T18:58:02Z

app/code/Magento/InventoryInStorePickupApi/Model/Mapper/PreProcessorInterface.php

+     *
+     * @return mixed
+     */
+    public function process(SourceInterface $source, $value);


Situation where mixed type have to be used may be a hint to a non-optimal design.

In this case, this contract seems incomplete, as Field Name seems to be hardcoded somewhere in the implementation and value has to be mixed.
I also love single responsibility classes, but still, it may be an overkill to introduce a class per field.

ishakhsuvarov · 2019-05-30T19:05:41Z

...ode/Magento/InventoryInStorePickup/Model/PickupLocation/Mapper/PreProcessor/FrontendName.php

+
+class FrontendName implements PreProcessorInterface
+{
+


Just an empty line

That`s really horrible

ishakhsuvarov · 2019-05-30T19:16:26Z

app/code/Magento/InventoryInStorePickup/Model/Source/InitPickupLocationExtensionAttributes.php

+        if (!$source instanceof DataObject) {
+            return;
+        }
+        $pickupAvailable = $source->getData(PickupLocationInterface::IS_PICKUP_LOCATION_ACTIVE);


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.

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 :)

ishakhsuvarov · 2019-05-30T19:17:21Z

...ntoryInStorePickup/Plugin/InventoryApi/SourceRepository/LoadInStorePickupOnGetListPlugin.php

+    public function __construct(
+        InitPickupLocationExtensionAttributes $setExtensionAttributes
+    ) {
+        $this->setExtensionAttributes = $setExtensionAttributes;


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.

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

…h-Pickup-Location-attributes

ghost · 2019-06-05T16:50:55Z

Hi @novikor, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

…xpansion-with-Pickup-Location-attributes Msi 2182 source entity expansion with pickup location attributes

al.kravchuk and others added 7 commits April 16, 2019 18:00

MSI-2182 Source Entity expansion with Pickup Location attributes.

4da7b8b

Add rows and extension attributes to Source.

Merge remote-tracking branch 'origin/store-pickup' into MSI-2182-Sour…

10401ca

…ce-Entity-expansion-with-Pickup-Location-attributes

Source Entity expansion with Pickup Location attributes #2182.

f6d9bb5

open_hours -> frontend_description

Source Entity expansion with Pickup Location attributes #2182.

2314c9f

Frontend name and frontend description admin UI. Some refactoring.

Source Entity expansion with Pickup Location attributes #2182.

748fd78

Improved tests coverage

Source Entity expansion with Pickup Location attributes #2182.

7426756

Show/Hide the pickup location fieldset depending from `is_pickup_location_active` value

Source Entity expansion with Pickup Location attributes #2182.

7559630

Adjusted Pickup Location entity

novikor added the Store Pickup label May 18, 2019

novikor requested a review from ishakhsuvarov May 18, 2019 19:40

novikor assigned ishakhsuvarov May 18, 2019

magento-engcom-team added Partner: ISM eCompany partners-contribution labels May 18, 2019

swnsma reviewed May 19, 2019

View reviewed changes

novikor added 2 commits May 20, 2019 22:05

Msi 2182 source entity expansion with pickup location attributes.

551568b

Fixed static tests

Msi 2182 source entity expansion with pickup location attributes.

143b67f

Set frontend name default value.

Merge remote-tracking branch 'origin/store-pickup' into MSI-2182-Sour…

f46fe29

…ce-Entity-expansion-with-Pickup-Location-attributes

ishakhsuvarov suggested changes May 21, 2019

View reviewed changes

al.kravchuk and others added 2 commits May 22, 2019 14:35

Msi 2182 source entity expansion with pickup location attributes.

28c8dda

Refactor Pickup Locations mapper. Add possibility to pre-process Source data for projection to Pickup Location.

Msi 2182 source entity expansion with pickup location attributes.

f57691c

LocalizedException -> InvalidArgumentException

novikor force-pushed the MSI-2182-Source-Entity-expansion-with-Pickup-Location-attributes branch from 2aa38ff to f57691c Compare May 23, 2019 10:05

Maksym Novik added 2 commits May 23, 2019 13:43

Msi 2182 source entity expansion with pickup location attributes.

3b64018

If frontend name is empty, pickup location uses source regular name now.

Msi 2182 source entity expansion with pickup location attributes.

e276996

Fixed jscs tests

novikor force-pushed the MSI-2182-Source-Entity-expansion-with-Pickup-Location-attributes branch from 17ca2eb to e276996 Compare May 23, 2019 11:44

Maksym Novik and others added 2 commits May 23, 2019 15:17

Msi 2182 source entity expansion with pickup location attributes.

86f5f3a

Some refactoring: added explicit DataObject check

Merge remote-tracking branch 'origin/store-pickup' into MSI-2182-Sour…

0a20315

…ce-Entity-expansion-with-Pickup-Location-attributes

ishakhsuvarov suggested changes May 30, 2019

View reviewed changes

ishakhsuvarov added the needs update label May 30, 2019

ishakhsuvarov added this to the Store Pickup Support milestone May 30, 2019

Merge branch 'store-pickup' into MSI-2182-Source-Entity-expansion-wit…

ca323b2

…h-Pickup-Location-attributes

ishakhsuvarov approved these changes Jun 5, 2019

View reviewed changes

ishakhsuvarov merged commit cd0b5db into store-pickup Jun 5, 2019

magento-engcom-team added Award: category of expertise Award: complex Award: test coverage labels Jun 5, 2019

ishakhsuvarov deleted the MSI-2182-Source-Entity-expansion-with-Pickup-Location-attributes branch June 19, 2019 16:16

ishakhsuvarov removed this from the Store Pickup Support milestone Sep 10, 2019

ishakhsuvarov added a commit that referenced this pull request Nov 22, 2019

Merge pull request #2267 from magento-engcom/MSI-2182-Source-Entity-e…

608ba9e

…xpansion-with-Pickup-Location-attributes Msi 2182 source entity expansion with pickup location attributes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Msi 2182 source entity expansion with pickup location attributes #2267

Msi 2182 source entity expansion with pickup location attributes #2267

novikor commented May 18, 2019 •

edited

Loading

swnsma May 19, 2019

novikor May 23, 2019

swnsma May 19, 2019

novikor May 19, 2019

novikor May 23, 2019

novikor commented May 20, 2019

novikor commented May 21, 2019

ishakhsuvarov left a comment

ishakhsuvarov May 21, 2019

novikor May 23, 2019

ishakhsuvarov May 21, 2019

novikor May 23, 2019

ishakhsuvarov May 21, 2019

novikor May 23, 2019

ishakhsuvarov May 21, 2019

novikor May 23, 2019

novikor commented May 28, 2019

ishakhsuvarov May 30, 2019

novikor May 30, 2019

ishakhsuvarov May 30, 2019

ishakhsuvarov May 30, 2019

novikor May 30, 2019

ishakhsuvarov May 30, 2019

novikor May 30, 2019 •

edited

Loading

ishakhsuvarov May 30, 2019

novikor May 30, 2019

ghost commented Jun 5, 2019

Msi 2182 source entity expansion with pickup location attributes #2267

Msi 2182 source entity expansion with pickup location attributes #2267

Conversation

novikor commented May 18, 2019 • edited Loading

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novikor commented May 20, 2019

novikor commented May 21, 2019

ishakhsuvarov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novikor commented May 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novikor May 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 5, 2019

novikor commented May 18, 2019 •

edited

Loading

novikor May 30, 2019 •

edited

Loading