[Api][POC]Convert iri to code in command #12487

Tomanhez · 2021-03-28T21:27:49Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
License	MIT

src/Sylius/Behat/Context/Api/Shop/ProductReviewContext.php

src/Sylius/Bundle/ApiBundle/Converter/CommandFieldItemIriToIdConverter.php

src/Sylius/Bundle/ApiBundle/Converter/ItemIriToIdConverter.php

src/Sylius/Bundle/ApiBundle/Converter/ItemIriToIdConverterInterface.php

src/Sylius/Bundle/ApiBundle/Serializer/CommandFieldItemIriToIdDenormalizer.php

AdamKasp · 2021-03-30T09:51:00Z

@Tomanhez, before potentially merge, you should add specs for it, and all classes should be @experimental

AdamKasp · 2021-03-30T09:52:49Z

And probably you should update the description of this PR :)
And it is a new and custom logic, so I suggest adding a doc with an explanation of how it works and how to use it (maybe ADR?

src/Sylius/Bundle/ApiBundle/Command/AddProductReview.php

src/Sylius/Bundle/ApiBundle/Converter/CommandFieldItemIriToIdentifierStorage.php

src/Sylius/Bundle/ApiBundle/Converter/CommandFieldItemIriToIdentifierStorageInterface.php

src/Sylius/Bundle/ApiBundle/Converter/ItemIriToIdentifierConverter.php

src/Sylius/Bundle/ApiBundle/Resources/config/services.xml

src/Sylius/Bundle/ApiBundle/Serializer/CommandFieldItemIriToIdentifierDenormalizer.php

…hez) This PR was merged into the 1.10-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | master | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | License | MIT  This is extension and improvement for create generic transformation command field iri's to code: #12487 Commits ------- 8cb5087 Convert AddProductReview product iri to productCode 3521679 Add spec for AddPRoductReviewCommandFieldItemIriToIdentifierDenormalize

…hez) This PR was merged into the 1.10-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | master | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | License | MIT  This is extension and improvement for create generic transformation command field iri's to code: Sylius/Sylius#12487 Commits ------- 8cb5087106ad60e137290d7af342502045e78f3c Convert AddProductReview product iri to productCode 35216795264ef19246a7717102d757057bfd96d7 Add spec for AddPRoductReviewCommandFieldItemIriToIdentifierDenormalize

CoderMaggie

The 🇺🇸 language police at your service 🖖🏼

adr/2021_04_15_using_iri_instead_of_code_id.md

src/Sylius/Bundle/ApiBundle/Converter/ItemIriToIdentifierConverterInterface.php

GSadee

Could you list the rest of the commands and fields that should be changed to the new approach?

adr/2021_04_15_using_iri_instead_of_code_id.md

src/Sylius/Bundle/ApiBundle/spec/Map/CommandItemIriArgumentToIdentifierMapSpec.php

adr/2021_04_15_using_iri_instead_of_code_id.md

…ArgumentToIdentifierMap

…aymentMethod

adr/2021_04_15_using_iri_as_api_resource_identifier_in_request_instead_of_code_id.md

lchrusciel · 2021-04-29T12:36:05Z

src/Sylius/Bundle/ApiBundle/Resources/config/services.xml

+            <argument type="collection">
+                <argument key="Sylius\Bundle\ApiBundle\Command\AddProductReview">product</argument>
+                <argument key="Sylius\Bundle\ApiBundle\Command\Checkout\ChoosePaymentMethod">paymentMethod</argument>
+                <argument key="Sylius\Bundle\ApiBundle\Command\Account\ChangePaymentMethod">paymentMethod</argument>
+                <argument key="Sylius\Bundle\ApiBundle\Command\RequestResetPasswordToken">locale</argument>
+                <argument key="Sylius\Bundle\ApiBundle\Command\ResendVerificationEmail">locale</argument>
+                <argument key="Sylius\Bundle\ApiBundle\Command\Cart\PickupCart">locale</argument>
+                <argument key="Sylius\Bundle\ApiBundle\Command\Cart\AddItemToCart">productVariant</argument>
+                <argument key="Sylius\Bundle\ApiBundle\Command\Checkout\ChooseShippingMethod">shippingMethod</argument>
+            </argument>


That's a neat idea, but I'm afraid about this list expandability. One would have to override this service definition, therefore they will not receive changes to this collection that we will do in the later releases. We need some refactoring here. Probably configuration tree would be the best choice. Maybe second class for client-specific mappings?

lchrusciel · 2021-04-29T12:39:18Z

src/Sylius/Bundle/ApiBundle/Serializer/CommandFieldItemIriToIdentifierDenormalizer.php

+        /** @psalm-var class-string $inputClassName */
+        $inputClassName = $this->getInputClassName($context);
+
+        $fieldName = $this->commandItemIriArgumentToIdentifierMap->get($inputClassName);

-        $data['product'] = $product->getCode();
+        if (array_key_exists($fieldName, $data)) {
+            $data[$fieldName] = $this->itemIriToIdentifierConverter->getIdentifier($data[$fieldName]);
+        }


Also, improvement for the second iteration, but what is blocking us from checking if the field consists IRI and execute this logic on all fields like this? This way, we could remove the whole map from the concept

lchrusciel · 2021-04-29T12:40:36Z

src/Sylius/Bundle/ApiBundle/Tests/Converter/ItemIriToIdentifierConverterTest.php

+        $identifierConverter->convert(['id' => 3, 'nextId' => 5], AddProductReview::class)->willReturn(['3', '5']);
+        $converter = new ItemIriToIdentifierConverter($router->reveal(), $identifierConverter->reveal());
+
+        $this->assertSame('3', $converter->getIdentifier('/users/3/nexts/5'));


Suggested change

$this->assertSame('3', $converter->getIdentifier('/users/3/nexts/5'));

$converter->getIdentifier('/users/3/nexts/5');

lchrusciel · 2021-04-29T13:25:41Z

Thanks, Tomasz & Artur! 🥇

…RIs to identifiers (Tomanhez, GSadee) This PR was merged into the 1.11-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Related tickets | #12487 | License | MIT Commits ------- feb3022 Remove redundant CommandItemIriArgumentToIdentifierMap fe15cd4 Add CommandFieldItemIriToIdentifierAwareInterface 4492d10 Modify CommandFieldItemIriToIdentifierDenormalizer ae2e91a Fix CommandFieldItemIriToIdentifierDenormalizer and its spec 1d5dc71 [API] Add missing experimental tag 647dde2 [API] Change name of iri to identifier converter 32d9c18 [API] Change name of denormalizer for converting iris to identifiers 0ee8cae [API] Change name of interface to mark commands with convertible arguments a87ef15 [API] Use an endpoint that is not customized in ApiBundle tests

Tomanhez requested a review from a team as a code owner March 28, 2021 21:27

probot-autolabeler bot added the API APIs related issues and PRs. label Mar 28, 2021

Tomanhez force-pushed the convert-iri-to-code-in-command branch 5 times, most recently from 3d618f7 to 7762ff4 Compare March 29, 2021 07:27

Tomanhez mentioned this pull request Mar 29, 2021

[Api] Visitor and Customer add ProductReview #12472

Merged

Tomanhez force-pushed the convert-iri-to-code-in-command branch from 7762ff4 to 03a4211 Compare March 29, 2021 11:54

probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Mar 29, 2021

Tomanhez force-pushed the convert-iri-to-code-in-command branch 2 times, most recently from cbf5597 to 6d3588b Compare March 29, 2021 12:05

GSadee reviewed Mar 29, 2021

View reviewed changes

Tomanhez force-pushed the convert-iri-to-code-in-command branch 3 times, most recently from 783e179 to e93c345 Compare March 30, 2021 09:41

Tomanhez force-pushed the convert-iri-to-code-in-command branch 5 times, most recently from cf59028 to 53a4d93 Compare March 30, 2021 10:30

pamil reviewed Mar 30, 2021

View reviewed changes

Tomanhez force-pushed the convert-iri-to-code-in-command branch from 53a4d93 to 574bd5a Compare March 31, 2021 05:59

Tomanhez mentioned this pull request Mar 31, 2021

Convert iri to code in command AddProductReview #12498

Merged

Tomanhez force-pushed the convert-iri-to-code-in-command branch 2 times, most recently from 3261f10 to 4e2626d Compare April 13, 2021 11:58

CoderMaggie reviewed Apr 15, 2021

View reviewed changes

arti0090 approved these changes Apr 15, 2021

View reviewed changes

src/Sylius/Bundle/ApiBundle/Converter/ItemIriToIdentifierConverterInterface.php Outdated Show resolved Hide resolved

GSadee reviewed Apr 15, 2021

View reviewed changes

Tomanhez force-pushed the convert-iri-to-code-in-command branch 2 times, most recently from 144fd86 to aa11089 Compare April 15, 2021 14:42

GSadee reviewed Apr 16, 2021

View reviewed changes

Tomanhez force-pushed the convert-iri-to-code-in-command branch from aa11089 to dcb1f7d Compare April 16, 2021 06:39

Tomanhez and others added 8 commits April 29, 2021 08:49

Remove /AddProductReviewCommandFieldItemIriToIdentifierDenormalizer

3e2cf94

Add ItemIriToIdentifierConverter

70dff14

dd CommandFieldItemIriToIdentifierDenormalizerSpec and CommandItemIri…

e037a75

…ArgumentToIdentifierMap

Add fixes for tests and implementation

6bb6936

replace code to iri in ChangePaymentMethod and change id to code in P…

deb2cfa

…aymentMethod

Add ADR and note to Upgrade file

05a355b

Add fixes for tests and implementations

f7b6f2d

Implement another endpoints

8392bcd

arti0090 force-pushed the convert-iri-to-code-in-command branch 3 times, most recently from e780947 to 33a50d8 Compare April 29, 2021 07:34

Rebase and rework product adding endpoint

08cc365

arti0090 force-pushed the convert-iri-to-code-in-command branch 3 times, most recently from 86973cd to 6b438bf Compare April 29, 2021 10:02

GSadee approved these changes Apr 29, 2021

View reviewed changes

adr/2021_04_15_using_iri_as_api_resource_identifier_in_request_instead_of_code_id.md Outdated Show resolved Hide resolved

Fix issues in configuration and failing behat

786a7d6

arti0090 force-pushed the convert-iri-to-code-in-command branch from 6b438bf to 786a7d6 Compare April 29, 2021 11:39

lchrusciel reviewed Apr 29, 2021

View reviewed changes

lchrusciel merged commit 43e3e8e into Sylius:master Apr 29, 2021

GSadee mentioned this pull request Jul 21, 2021

[API] Refactor converting arguments of commands from IRIs to identifiers #12769

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Api][POC]Convert iri to code in command #12487

[Api][POC]Convert iri to code in command #12487

Tomanhez commented Mar 28, 2021 •

edited

Loading

AdamKasp commented Mar 30, 2021

AdamKasp commented Mar 30, 2021

CoderMaggie left a comment

GSadee left a comment

lchrusciel Apr 29, 2021

lchrusciel Apr 29, 2021

lchrusciel Apr 29, 2021

lchrusciel commented Apr 29, 2021 •

edited

Loading

	$this->assertSame('3', $converter->getIdentifier('/users/3/nexts/5'));
	$converter->getIdentifier('/users/3/nexts/5');

[Api][POC]Convert iri to code in command #12487

[Api][POC]Convert iri to code in command #12487

Conversation

Tomanhez commented Mar 28, 2021 • edited Loading

AdamKasp commented Mar 30, 2021

AdamKasp commented Mar 30, 2021

CoderMaggie left a comment

Choose a reason for hiding this comment

GSadee left a comment

Choose a reason for hiding this comment

lchrusciel Apr 29, 2021

Choose a reason for hiding this comment

lchrusciel Apr 29, 2021

Choose a reason for hiding this comment

lchrusciel Apr 29, 2021

Choose a reason for hiding this comment

lchrusciel commented Apr 29, 2021 • edited Loading

Tomanhez commented Mar 28, 2021 •

edited

Loading

lchrusciel commented Apr 29, 2021 •

edited

Loading