Implemented order available payment methods and order update payment #422

dlobato · 2019-04-08T16:01:23Z

Hi,

We have implemented two new actions to handle payment state on placed orders:

Get available payment methods: Same as /{channelCode}/checkout/{token}/payment but on order endpoint.
Update payment method on unpaid placed orders to implement the same behavior as in ShopBundle to allow a customer to change the payment method while order is unpaid.

1 is just reusing the same action from checkout, not sure if I should reimplement this as a different action.
2 adds validation as I've seen on other controllers, is this the preferred approach?

Tests are failing, there's something weird I couldn't figure out on loading the fixtures. I bet I'm doing something wrong. Am I missing something obvious there?

I've also added a new field to placedorder view: paymentState.

David.

src/Validator/Constraints/OrderExists.php

tests/Controller/Order/OrderUpdatePaymentMethodApiTest.php

src/Command/Order/UpdatePaymentMethod.php

src/Handler/Order/UpdatePaymentMethodHandler.php

src/Resources/config/routing/order.yml

src/Validator/Constraints/OrderExists.php

tests/Controller/Order/OrderUpdatePaymentMethodApiTest.php

dlobato · 2019-04-16T11:55:43Z

@GSadee @mamazu I'm struggling here with the tests... Can't figure out what's wrong. If I run the tests alone on local they work fine, but when running them one after the other there's an unrelated error. Seems to me some sort of race condition while loading the fixtures for the second time. Is something obvious that I'm not seeing here?

src/Validator/Constraints/OrderExists.php

dlobato · 2019-04-29T12:55:47Z

I fixed the tests by changing the item used on the order. Sadly, I couldn't figure out what was the problem but it is related to mug item having product attributes. Since the items on the order are irrelevant for the test purposes I've just used another one and updated the expected responses accordingly.

@mamazu @GSadee is there anything else to code review?

mamazu · 2019-04-29T13:56:01Z

Ill have a look at it this evening.

spec/Handler/Order/UpdatePaymentMethodHandlerSpec.php

src/Handler/Order/UpdatePaymentMethodHandler.php

mamazu · 2019-04-29T19:51:34Z

There are still some open conversations if you could resolve that, this would be great.

lchrusciel

I know it was a while, but can you apply my changes as well?

Sorry, for late review

lchrusciel · 2019-04-30T11:23:15Z

doc/swagger.yml

@@ -1009,6 +1009,53 @@ paths:
                    description: "Order with given tokenValue not found"
            security:
                -   bearerAuth: []
+    /{channelCode}/orders/{token}/payment:


payment -> payments ?

I'd rather keep it like this to be homogeneous with /{channelCode}/checkout/{token}/payment

spec/Command/Order/UpdatePaymentMethodSpec.php

spec/Handler/Order/UpdatePaymentMethodHandlerSpec.php

tests/DataFixtures/ORM/order.yml

src/Handler/Order/UpdatePaymentMethodHandler.php

spec/Validator/Order/OrderExistsValidatorSpec.php

src/Command/Order/UpdatePaymentMethod.php

lchrusciel · 2019-05-10T14:13:32Z

Hello David,

Sorry for such a long response time. I found some time to dig into ShopAPI and recently fix the way, how we handle placed orders in the test environment (0e445e4)

This made this PR conflicted. Would you like to fix it?

dlobato · 2019-05-10T17:28:54Z

@lchrusciel I've updated the tests. But now I can't set the payment state to something different from awaiting_payment to test it_does_not_allow_to_update_payment_method_on_paid_order, any suggestion?
I'd say that I need to trigger pay, but not sure how...

mamazu · 2019-05-15T18:28:27Z

You can not set the payment method once the payment is completed.

mamazu · 2019-05-15T18:36:10Z

Ah, I see your problem. Currently there is no way for the API to mark an order as payed. What you can do is try to generate a fixture with a payed order. Other than that I don't know see #310

jdeveloper · 2019-09-11T12:03:35Z

What's the status of this issue?

Is there any hope to get merged? It would be great. So if I choose a payment method, paypal for example, complete the order but then I cancel in paypal, I could change the payment method.

mamazu · 2020-02-04T23:36:12Z

I have rebased the issue but I can not push it. @dlobato Please allow edits from maintainers to your branch.

dlobato · 2020-02-05T16:19:09Z

@mamazu I don't know how to enable that. I was trying this: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork But I don't see said checkbox anywhere...

Implemented validator and tests Update to symfony messenger Use tokenValue from fixture Revert small changes on non related order fixtures Check order state on UpdatePaymentMethodHandler Assert payment is in new state

Fix typos.

mamazu · 2020-02-05T21:26:02Z

Hm, very strange. I can push to other merge request. Anyhow I have pushed my version of the branch to my repository mamazu/handle_order_payment. You can run the following commands to get this merge request up to speed:

# Add me as a remote
git remote add mamazu git@github.com:mamazu/ShopApiPlugin.git

# Fetch all the branches
git fetch mamazu

# If you are not on your local branch change to it. Otherwise just run
git reset --hard mamazu/handle_order_payment

# Push the new changes to git (you might need to explicitly add `origin handle_order_payment`)
git push --force-with-lease

If you have any questions, don't hesitate to ask.

dlobato · 2020-02-05T22:37:05Z

Your changes are already on my branch...

mamazu · 2020-02-06T09:11:27Z

This can't be I have fixed the tests (eg. removed the channelcode from the route).

dlobato · 2020-02-06T09:28:28Z

Check it out now

mamazu · 2020-02-06T09:38:42Z

Looks good. Can you run composer fix and then commit the changes. Then the merge request ready for merge.

mamazu

If you apply those fixes this should be all.

src/Request/Order/UpdatePaymentMethodRequest.php

src/Command/Order/UpdatePaymentMethod.php

src/Validator/Order/OrderExistsValidator.php

Co-Authored-By: mamazu <14860264+mamazu@users.noreply.github.com>

mamazu

Last one I promise. :D

src/Validator/Order/OrderExistsValidator.php

Co-Authored-By: mamazu <14860264+mamazu@users.noreply.github.com>

mamazu · 2020-02-06T18:29:20Z

Can you please checkout the code and put the right doc comment in there.

mamazu · 2020-02-07T17:03:53Z

Thank you, David! 🥇

lchrusciel · 2020-02-09T11:03:21Z

src/Handler/Order/UpdatePaymentMethodHandler.php

+        $order = $this->orderRepository->findOneBy(['tokenValue' => $choosePaymentMethod->orderToken()]);
+
+        Assert::notNull($order, 'Order has not been found.');
+        Assert::notSame(OrderInterface::STATE_CART, $order->getState(), 'Only orders can be updated.');


Not 100% true, I suppose. Order's in fulfilled state should be blocked as well.

Suggested change

Assert::notSame(OrderInterface::STATE_CART, $order->getState(), 'Only orders can be updated.');

Assert::same(OrderInterface::STATE_NEW, $order->getState(), 'Only new orders can be updated.');

The validator checks if the payment you are trying to edit was not payed yet. This would be a safer option as an assertion?

dlobato requested a review from a team as a code owner April 8, 2019 16:01

mamazu reviewed Apr 8, 2019

View reviewed changes

src/Validator/Constraints/OrderExists.php Outdated Show resolved Hide resolved

mamazu reviewed Apr 8, 2019

View reviewed changes

tests/Controller/Order/OrderUpdatePaymentMethodApiTest.php Outdated Show resolved Hide resolved

GSadee reviewed Apr 9, 2019

View reviewed changes

dlobato force-pushed the handle_order_payment branch from a55667c to 02a1f72 Compare April 15, 2019 15:54

mamazu reviewed Apr 22, 2019

View reviewed changes

src/Validator/Constraints/OrderExists.php Outdated Show resolved Hide resolved

mamazu reviewed Apr 29, 2019

View reviewed changes

spec/Handler/Order/UpdatePaymentMethodHandlerSpec.php Outdated Show resolved Hide resolved

mamazu reviewed Apr 29, 2019

View reviewed changes

src/Handler/Order/UpdatePaymentMethodHandler.php Show resolved Hide resolved

lchrusciel reviewed Apr 30, 2019

View reviewed changes

dlobato force-pushed the handle_order_payment branch from ec33962 to f76e387 Compare April 30, 2019 15:16

GSadee reviewed May 2, 2019

View reviewed changes

spec/Validator/Order/OrderExistsValidatorSpec.php Outdated Show resolved Hide resolved

src/Command/Order/UpdatePaymentMethod.php Outdated Show resolved Hide resolved

GSadee approved these changes May 6, 2019

View reviewed changes

dlobato force-pushed the handle_order_payment branch from 467db1f to 2f174e9 Compare May 10, 2019 17:22

mamazu added the New Feature label Aug 28, 2019

dlobato added 5 commits February 5, 2020 22:17

Implemented order available payment methods and order update payment

ff2a6ad

Implemented validator and tests Update to symfony messenger Use tokenValue from fixture Revert small changes on non related order fixtures Check order state on UpdatePaymentMethodHandler Assert payment is in new state

Fix tests

41ce42f

Rename UpdatePaymentMethod command properties.

77ea84d

Fix typos.

Update tests

175eb07

Fix test

b0c0dbd

Fixing tests

10050cc

dlobato force-pushed the handle_order_payment branch from 51034f4 to 10050cc Compare February 6, 2020 09:27

mamazu approved these changes Feb 6, 2020

View reviewed changes

Fix code style

6db288e

mamazu reviewed Feb 6, 2020

View reviewed changes

src/Request/Order/UpdatePaymentMethodRequest.php Show resolved Hide resolved

src/Command/Order/UpdatePaymentMethod.php Show resolved Hide resolved

src/Command/Order/UpdatePaymentMethod.php Show resolved Hide resolved

src/Validator/Order/OrderExistsValidator.php Show resolved Hide resolved

dlobato and others added 3 commits February 6, 2020 14:11

Update src/Request/Order/UpdatePaymentMethodRequest.php

54f0cb0

Co-Authored-By: mamazu <14860264+mamazu@users.noreply.github.com>

Update src/Command/Order/UpdatePaymentMethod.php

f8d3d3b

Co-Authored-By: mamazu <14860264+mamazu@users.noreply.github.com>

Apply suggestions from code review

c11044f

Co-Authored-By: mamazu <14860264+mamazu@users.noreply.github.com>

mamazu reviewed Feb 6, 2020

View reviewed changes

src/Validator/Order/OrderExistsValidator.php Outdated Show resolved Hide resolved

src/Validator/Order/OrderExistsValidator.php Show resolved Hide resolved

Apply suggestions from code review

891b179

Co-Authored-By: mamazu <14860264+mamazu@users.noreply.github.com>

Proper type handling on OrderExistsValidator

a2d10f7

mamazu merged commit 6f2133c into Sylius:master Feb 7, 2020

lchrusciel reviewed Feb 9, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented order available payment methods and order update payment #422

Implemented order available payment methods and order update payment #422

dlobato commented Apr 8, 2019 •

edited

Loading

dlobato commented Apr 16, 2019

dlobato commented Apr 29, 2019

mamazu commented Apr 29, 2019

mamazu commented Apr 29, 2019

lchrusciel left a comment

lchrusciel Apr 30, 2019

dlobato Apr 30, 2019

lchrusciel commented May 10, 2019

dlobato commented May 10, 2019

mamazu commented May 15, 2019

mamazu commented May 15, 2019

jdeveloper commented Sep 11, 2019

mamazu commented Feb 4, 2020

dlobato commented Feb 5, 2020

mamazu commented Feb 5, 2020 •

edited

Loading

dlobato commented Feb 5, 2020

mamazu commented Feb 6, 2020

dlobato commented Feb 6, 2020

mamazu commented Feb 6, 2020

mamazu left a comment

mamazu left a comment

mamazu commented Feb 6, 2020

mamazu commented Feb 7, 2020

lchrusciel Feb 9, 2020

mamazu Feb 9, 2020

	Assert::notSame(OrderInterface::STATE_CART, $order->getState(), 'Only orders can be updated.');
	Assert::same(OrderInterface::STATE_NEW, $order->getState(), 'Only new orders can be updated.');

Implemented order available payment methods and order update payment #422

Implemented order available payment methods and order update payment #422

Conversation

dlobato commented Apr 8, 2019 • edited Loading

dlobato commented Apr 16, 2019

dlobato commented Apr 29, 2019

mamazu commented Apr 29, 2019

mamazu commented Apr 29, 2019

lchrusciel left a comment

Choose a reason for hiding this comment

lchrusciel Apr 30, 2019

Choose a reason for hiding this comment

dlobato Apr 30, 2019

Choose a reason for hiding this comment

lchrusciel commented May 10, 2019

dlobato commented May 10, 2019

mamazu commented May 15, 2019

mamazu commented May 15, 2019

jdeveloper commented Sep 11, 2019

mamazu commented Feb 4, 2020

dlobato commented Feb 5, 2020

mamazu commented Feb 5, 2020 • edited Loading

dlobato commented Feb 5, 2020

mamazu commented Feb 6, 2020

dlobato commented Feb 6, 2020

mamazu commented Feb 6, 2020

mamazu left a comment

Choose a reason for hiding this comment

mamazu left a comment

Choose a reason for hiding this comment

mamazu commented Feb 6, 2020

mamazu commented Feb 7, 2020

lchrusciel Feb 9, 2020

Choose a reason for hiding this comment

mamazu Feb 9, 2020

Choose a reason for hiding this comment

dlobato commented Apr 8, 2019 •

edited

Loading

mamazu commented Feb 5, 2020 •

edited

Loading