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.

Sign up for GitHub

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

Jump to bottom

[Api][Checkout] Modify ShippingMethod and PaymentMethod paths on Checkout #11745

Merged
merged 7 commits into from
Sep 7, 2020
Merged

[Api][Checkout] Modify ShippingMethod and PaymentMethod paths on Checkout #11745

merged 7 commits into from
Sep 7, 2020

Conversation

Tomanhez
Copy link
Contributor

@Tomanhez Tomanhez commented Aug 17, 2020 •
edited
Loading

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

Its continue: #11735

Screenshot 2020-08-20 at 12 31 59

Screenshot 2020-09-02 at 12 24 38

Screenshot 2020-09-02 at 16 45 33

@Tomanhez Tomanhez requested a review from a team as a code owner August 17, 2020 14:15
@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Aug 17, 2020
@Tomanhez Tomanhez force-pushed the modify-selection-shipping-method-checkout branch 5 times, most recently from f58ec77 to 03072b9 Compare August 17, 2020 19:19
@lchrusciel
Copy link
Member

image
Why is this shipmentId duplicated? Can we have it only on a path?

@Tomanhez Tomanhez force-pushed the modify-selection-shipping-method-checkout branch from 03072b9 to 6669f37 Compare August 20, 2020 06:37
@Tomanhez Tomanhez changed the title [Api][Checkout] ShippingMethod on Checkout - minor fixes [WIP][Api][Checkout] ShippingMethod on Checkout - minor fixes Aug 20, 2020
@Tomanhez Tomanhez force-pushed the modify-selection-shipping-method-checkout branch 2 times, most recently from a837755 to c79410f Compare August 20, 2020 10:26
Tomanhez
Tomanhez commented Aug 20, 2020
View reviewed changes
src/Sylius/Bundle/ApiBundle/Command/CommandAwareDataTransformerInterface.php

namespace Sylius\Bundle\ApiBundle\Command;

interface CommandAwareDataTransformerInterface
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added new tag interface, because some DataTransformer can be executed only one in certain object, if we have, for example two DataTransformers where supportsTransformation (...) equals to us conditions, will be executed first in line .

@Tomanhez Tomanhez force-pushed the modify-selection-shipping-method-checkout branch from c79410f to f8baa28 Compare August 20, 2020 10:46
@Tomanhez Tomanhez changed the title [WIP][Api][Checkout] ShippingMethod on Checkout - minor fixes [Api][Checkout] ShippingMethod on Checkout - minor fixes Aug 20, 2020
@Tomanhez Tomanhez force-pushed the modify-selection-shipping-method-checkout branch 5 times, most recently from 949bedd to 45ba3a4 Compare August 20, 2020 12:30
lchrusciel
lchrusciel reviewed Aug 28, 2020
View reviewed changes
src/Sylius/Bundle/ApiBundle/CommandHandler/Checkout/ChooseShippingMethodHandler.php Outdated

$shipment = $cart->getShipments()[$shipmentIdentifier];
$shipment = $this->getShipmentById($cart, $chooseShippingMethod->shipmentId);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of checking id for all shipments, we have a repository method on ShipmentRepository::findOneByOrderId. With it, we can skill foreach loop, as well, as fetching cart.

What is more, as we moved the definition of the route from cart to shipment, we should return shipment instead of cart.

src/Sylius/Bundle/ApiBundle/DataTransformer/CommandAwareInputDataTransformer.php Outdated Show resolved Hide resolved
src/Sylius/Bundle/ApiBundle/DataTransformer/CommandAwareInputDataTransformer.php Outdated Show resolved Hide resolved
src/Sylius/Bundle/ApiBundle/Command/Checkout/ChooseShippingMethod.php

public function getSubresourceIdAttributeKey(): string
{
return 'shipmentId';
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, the command should not be aware of path definition details. An additional interface is ok, but I would prefer to make more detailed (get/setShipmentId), make additional transformer, where shipmentId will be injected into the constructor.

Also, can you check if we can use parameters in route definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added getSubresourceIdAttributeKey instead of details like get/setShipmentId, because i would use it in next cases like payment, out of the box without duplicated boilerplate code.

@Tomanhez Tomanhez force-pushed the modify-selection-shipping-method-checkout branch 6 times, most recently from f6f4f43 to c12ff32 Compare September 2, 2020 10:29
@Tomanhez Tomanhez changed the base branch from master to 1.8 September 2, 2020 14:54
@Tomanhez Tomanhez force-pushed the modify-selection-shipping-method-checkout branch 7 times, most recently from 81983b8 to 0e6a366 Compare September 3, 2020 08:12
@Tomanhez Tomanhez force-pushed the modify-selection-shipping-method-checkout branch 2 times, most recently from b4f4b5b to a4f0dab Compare September 3, 2020 10:46
@Tomanhez Tomanhez force-pushed the modify-selection-shipping-method-checkout branch from e23e38d to 9f60b6f Compare September 4, 2020 09:03
@Tomanhez Tomanhez force-pushed the modify-selection-shipping-method-checkout branch 3 times, most recently from 3002262 to 5905d78 Compare September 7, 2020 09:20
@Tomanhez Tomanhez force-pushed the modify-selection-shipping-method-checkout branch from 5905d78 to fc12d21 Compare September 7, 2020 11:39
@GSadee GSadee merged commit 62e5163 into Sylius:1.8 Sep 7, 2020
@GSadee
Copy link
Member

GSadee commented Sep 7, 2020

Thank you, Tomasz! 🎉

@lchrusciel lchrusciel mentioned this pull request Sep 8, 2020
Merged
lchrusciel added a commit that referenced this pull request Sep 9, 2020
@lchrusciel
refactor #11819 [API] Refactored data transformers (lchrusciel)
685f8b1 
This PR was merged into the 1.8 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | 1.8
| Bug fix?        | yes
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | Following #11745
| License         | MIT

I've adjusted service names to follow our conventions, removed unneeded classes, and typehinted variadic expression

Commits
-------

a19f1ac [API] Adjust services names to common convention
4dbf19f [API] Add variadic typehint
e878b19 [API] Refactored CommandAwareInputDataTransformer
lchrusciel added a commit to Sylius/SyliusApiBundle that referenced this pull request Sep 9, 2020
@lchrusciel
refactor #11819 [API] Refactored data transformers (lchrusciel)
d72d5c4 
This PR was merged into the 1.8 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | 1.8
| Bug fix?        | yes
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | Following Sylius/Sylius#11745
| License         | MIT

I've adjusted service names to follow our conventions, removed unneeded classes, and typehinted variadic expression

Commits
-------

a19f1ac1a7f902e9183979fd414ef2ab3063ee96 [API] Adjust services names to common convention
4dbf19f7224992749c2ed3b14e43aba15866cd5c [API] Add variadic typehint
e878b1935ac351ac0214a50060b940abd43f4a8e [API] Refactored CommandAwareInputDataTransformer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lchrusciel lchrusciel lchrusciel left review comments

@GSadee GSadee GSadee approved these changes

Assignees
No one assigned
Labels
Admin AdminBundle related issues and PRs. API APIs related issues and PRs. Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Tomanhez @lchrusciel @GSadee