[API] Add Payment request by Prometee · Pull Request #15502 · Sylius/Sylius

Prometee · 2023-11-06T19:19:56Z

Q	A
Branch?	1.13
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets
License	MIT

First PR to handle payments via requests.

github-actions · 2023-11-06T19:30:46Z

Bunnyshell Preview Environment deleted

Available commands:

/bns:deploy to redeploy the environment

src/Sylius/Bundle/ApiBundle/Applicator/PaymentStateMachineTransitionApplicator.php

src/Sylius/Bundle/ApiBundle/CommandHandler/Payment/AddPaymentRequestHandler.php

src/Sylius/Component/Payment/Factory/PaymentRequestFactory.php

composer.json

src/Sylius/Bundle/ApiBundle/CommandHandler/Payment/UpdatePaymentRequestHandler.php

src/Sylius/Bundle/CoreBundle/PaymentRequest/Payum/CommandHandler/ModelPaymentRequestHandler.php

NoResponseMate

The graph is not necessarily true since you can technically immediately finalize the payment via the create request endpoint.
Now the question is whether we should allow that or force using the post endpoint for initializing/validating/handshake only. It'd then require using the payments API at least twice but might be easier to manage, at least the responsibilities of each would be clearer. Food for thought.

src/Sylius/Component/Payment/Model/PaymentRequest.php

src/Sylius/Bundle/PaymentBundle/Resources/config/doctrine/model/PaymentRequest.orm.xml

src/Sylius/Bundle/CoreBundle/Resources/config/services/integrations/payum/stripe.xml

NoResponseMate · 2024-02-19T14:07:48Z

src/Sylius/Bundle/CoreBundle/PaymentRequest/Processor/AfterOfflineCaptureProcessor.php

+        $stateMachine = $this->stateMachineFactory->get($payment, PaymentTransitions::GRAPH);
+        if ($paymentRequest->getResponseData()['paid']) {
+            $stateMachine->apply(PaymentTransitions::TRANSITION_COMPLETE);
+        } else {
+            $stateMachine->apply(PaymentTransitions::TRANSITION_PROCESS);
+        }


The fact that we need to manually trigger a transition cascade to payment is annoying, but I don't really have any good idea ATM how to abstract it out, or if it's even possible given the multiple request types.

Yes, but keep in mind that this is required only for non Payum gateways, Payum is doing this transition too.

What you've wrote, makes me think about a way to setup a state machine on the payment request which can cascade status to the payment.

I can imagine it clearly for this Offline gateway: transition from new|processing to completed (if the type === "capture|authorize"), cascade state to completed on the related payment.

WDTY?

The main problem I see with that is you'd need a matrix of paymentRequest state machines for every factory and paymentRequest type within, which might get messy fast.
Let's say we have 3 factories, each having 4 steps per payment (4 different types of requests), I'd assume you'd need 12 state machines to process the payment's state correctly since each factory can change the state of payment differently per type.
Having to set it manually is maybe annoying, but at least the person implementing it can be sure what state will be the result and there is no internal magic that can be potentially buggy.

...ius/Bundle/CoreBundle/PaymentRequest/CommandHandler/Offline/CapturePaymentRequestHandler.php

src/Sylius/Bundle/CoreBundle/Migrations/Version20240203222417.php

src/Sylius/Bundle/ApiBundle/Resources/config/validation/AddPaymentRequest.xml

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

diimpp

Request changes is due PaymentRequest/Payum/Checker/PayumRequirementsChecker, looks like this system needs to be refactored. (#15502 (comment))

src/Sylius/Bundle/PayumBundle/Resources/config/integrations/payum/paypal.xml

src/Sylius/Bundle/PayumBundle/Resources/config/integrations/payum/stripe.xml

src/Sylius/Component/Payment/Factory/PaymentRequestFactory.php

src/Sylius/Component/Payment/Model/PaymentRequestInterface.php

src/Sylius/Bundle/CoreBundle/PaymentRequest/Payum/Factory/PayumTokenFactory.php

src/Sylius/Bundle/CoreBundle/PaymentRequest/Payum/Checker/PayumRequirementsChecker.php

...e/CoreBundle/PaymentRequest/CommandProvider/ServiceProviderAwareCommandProviderInterface.php

src/Sylius/Bundle/CoreBundle/PaymentRequest/Payum/Checker/PayumRequirementsChecker.php

Co-authored-by: Dmitri Perunov <diimpp@gmail.com>

Prometee · 2024-02-28T19:52:46Z

Request changes is due PaymentRequest/Payum/Checker/PayumRequirementsChecker, looks like this system needs to be refactored. (#15502 (comment))

I refactored it, thank you very much for your review @diimpp !

lchrusciel · 2024-02-29T10:55:45Z

phpstan-baseline.neon

 			count: 1
 			path: src/Sylius/Bundle/ApiBundle/CommandHandler/Checkout/UpdateCartHandler.php

+		-


In general it would be good to apply rule that we can only remove from the baseline, not add to it. We can either fix it (preferable option) or write ignore for some type of problem

I totally agree, I only add what other existing rules were doing like the one for the repositories for example, the rest are needed exclusion for mixed data.

lchrusciel · 2024-02-29T10:58:21Z

src/Sylius/Behat/Context/Api/Shop/Checkout/CheckoutOrderDetailsContext.php

+        $response = $this->client->show(Resources::ORDERS, $this->sharedStorage->get('cart_token'));
+        Assert::same($this->client->getLastResponse()->getStatusCode(), 200);
+
+        // If the payment is canceled we won't be able to retrieve it because only new one are retrievable


shouldn't we eventaully change it and just return cancelled payments as well. Or allow to fetch them with filtration

If the payment state of the Order is canceled then it's a final state (sylius_order_payment graph):

We don't need to retrieve payment when the order itself is canceled.

Having a way to get access to all payments related to an order can be something that developers will want, but the current behaviour is fine IMHO.

lchrusciel · 2024-02-29T11:11:25Z

src/Sylius/Bundle/CoreBundle/PaymentRequest/Checker/PaymentRequestIntegrityChecker.php

+    ) {
+    }
+
+    public function check(PaymentRequestHashAwareInterface $command): PaymentRequestInterface


I'm not convinced about naming of this service. It should check something, but it mostly asserts and provides request from command

I agree, since it mainly retrieve a PaymentRequest it can be named "Provider".
@lchrusciel What about PaymentRequestProvider and divide the actual method to 2 methods : provideFromCommand and provideFromHash ?

lchrusciel · 2024-02-29T11:21:10Z

...ylius/Bundle/CoreBundle/PaymentRequest/CommandDispatcher/PaymentRequestCommandDispatcher.php

+
+    public function add(PaymentRequestInterface $paymentRequest): void
+    {
+        $this->paymentRequestRepository->add($paymentRequest);


this logic will persist new object and commit transaction. Do we really want to do it at this stage? Shouldn't we rather just persist it and depend on flushing around command bus?

All in all, maybe this logic should be based more on commands?

To some extend logic in this class could be simplify to one function, where we will always persist() on object manager

Reading the code of promotion catalog, I see how to solve this.
Lets just make the same as here : https://github.com/Sylius/Sylius/blob/1.13/src/Sylius/Bundle/ApiBundle/CommandHandler/Catalog/AddProductReviewHandler.php#L68
And then I will add an event subscriber like this one dispatching the commands :
https://github.com/Sylius/Sylius/blob/1.13/src/Sylius/Bundle/ApiBundle/EventSubscriber/CatalogPromotionEventSubscriber.php#L47

lchrusciel · 2024-02-29T11:22:41Z

...ylius/Bundle/CoreBundle/PaymentRequest/CommandDispatcher/PaymentRequestCommandDispatcher.php

+
+        $command = $this->paymentRequestCommandProvider->provide($paymentRequest);
+
+        $this->commandBus->dispatch($command);


We should dispatch command after flushing the transaction as we are processing it on async queue

Resolved by comment above

lchrusciel · 2024-02-29T11:24:25Z

...ylius/Bundle/CoreBundle/PaymentRequest/CommandDispatcher/PaymentRequestCommandDispatcher.php

+use Sylius\Component\Payment\Repository\PaymentRequestRepositoryInterface;
+use Symfony\Component\Messenger\MessageBusInterface;
+
+final class PaymentRequestCommandDispatcher implements PaymentRequestCommandDispatcherInterface


I would like to rethink this class name. Maybe some saver or something like that?

Catalog promotion called this kind of class "Announcer" WDYT ?

...ius/Bundle/CoreBundle/PaymentRequest/CommandHandler/Offline/CapturePaymentRequestHandler.php

src/Sylius/Bundle/CoreBundle/PaymentRequest/Payum/Checker/DoctrineProxyObjectResolver.php

lchrusciel · 2024-02-29T11:35:58Z

src/Sylius/Bundle/CoreBundle/PaymentRequest/Processor/Offline/CaptureProcessor.php

+        }
+
+        $paymentRequest->setResponseData($responseData);
+        $paymentRequest->setState(PaymentRequestInterface::STATE_COMPLETED);


maybe therefore we should change function to somethin like $paymentRequest->complete()?

lchrusciel · 2024-02-29T11:37:00Z

src/Sylius/Bundle/CoreBundle/PaymentRequest/Processor/Offline/AfterCaptureProcessor.php

+use Sylius\Component\Payment\Model\PaymentRequestInterface;
+use Sylius\Component\Payment\PaymentTransitions;
+
+final class AfterCaptureProcessor implements AfterCaptureProcessorInterface


What is the main idea behind splitting capture into normal and after? If I see correctly, we always trigger both of them

You are right, I wanted to split those two but it's not required.

NoResponseMate · 2024-03-01T15:55:24Z

Before someone asks why was it closed.
I have no idea.
Github has outages at the moment and it might be related to that, I'll try to reopen it when it stabilizes.
On the off chance it won't be possible, we'll go with a new PR and link this one in the description.

This PR was merged into the payment-request branch. Discussion ---------- | Q | A | |-----------------|--------------------------------------------------------------| | Branch? | payment-request | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | no | | Related tickets | #15502 | | License | MIT | Commits ------- Add Payment request model Add API resource Add first step of a PAYUM Capture request Finalise Payum POC and 1st try with no Payum using Offline payment method Rename data to payload Rename input/output data to requestPayload/responseData Move Payum stuff to PayumBundle Move services from ApiBundle to PaymentBundle Move services from ApiBundle to PayumBundle Update field in migration Add validation on adding payment request Behat api scenario for PayPal Attempt to get a real object instead of proxy one Pay with PayPal via authorize working Refactor some repeated processes and fix ECS 3/5 behat tests ok Behat ecs fix @paying_for_order&&@api fully working Fix PHPStan real issue and regenrate baseline for the rest Fix service definition and use state machine absctraction Move Payum related integrations to a dedicated folder Fix composer require checker missing new usages Regenerate baseline for PHP8.2 and Sf 6.4 Fix baseline Add missing deps Fix old ignore Use Sylius abstract doctrine migration to allow PostgreSQL Create migration for PostgreSQL Add missing messenger buses Refactor to use CoreBundle instead of SyliusPay(um|ment)Bundle

Prometee requested a review from a team as a code owner November 6, 2023 19:19

Prometee force-pushed the payment-request branch from 83ebb5d to da5d3c1 Compare November 6, 2023 19:29

Prometee force-pushed the payment-request branch 2 times, most recently from ec4f275 to 487ba19 Compare November 7, 2023 10:59

Prometee force-pushed the payment-request branch from 487ba19 to 2d43682 Compare November 17, 2023 12:03

probot-autolabeler bot added the API APIs related issues and PRs. label Nov 17, 2023

Prometee force-pushed the payment-request branch 2 times, most recently from 5d54f31 to cd679fc Compare November 23, 2023 19:08

Prometee force-pushed the payment-request branch from cd679fc to 74c252a Compare December 20, 2023 16:28

Prometee force-pushed the payment-request branch 3 times, most recently from 162906d to f2c46d1 Compare February 1, 2024 19:11

Prometee requested a review from a team as a code owner February 2, 2024 16:26

Prometee force-pushed the payment-request branch 2 times, most recently from 4a52858 to 2de795b Compare February 2, 2024 19:03

probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Feb 3, 2024

Prometee force-pushed the payment-request branch 3 times, most recently from f054ae6 to 77e2207 Compare February 3, 2024 17:19

Prometee mentioned this pull request Feb 6, 2024

[WIP] [API] Use Payum 🎉 #14572

Closed

5 tasks

Prometee force-pushed the payment-request branch 2 times, most recently from 3e9e391 to 5d8c23b Compare February 7, 2024 14:43

diimpp reviewed Feb 20, 2024

View reviewed changes

NoResponseMate reviewed Feb 20, 2024

View reviewed changes

Prometee force-pushed the payment-request branch 3 times, most recently from be9dc0a to a3fa5a7 Compare February 22, 2024 22:31

Prometee self-assigned this Feb 23, 2024

Prometee force-pushed the payment-request branch from ad38580 to dccffa8 Compare February 23, 2024 18:15

Prometee force-pushed the payment-request branch from 2187db3 to 9e03e1d Compare February 28, 2024 11:57

Prometee added 4 commits February 28, 2024 13:55

Fix service definition

f660cf5

Use new transport for payment_request

c55023b

Revert, even if it works it's not the purpose of this PR

7b6924f

Fix Postgresql error when select UUID binary value

fa6f6e6

Prometee changed the title ~~[WIP] [API] Add Payment request~~ [API] Add Payment request Feb 28, 2024

diimpp requested changes Feb 28, 2024

View reviewed changes

diimpp reviewed Feb 28, 2024

View reviewed changes

src/Sylius/Bundle/CoreBundle/PaymentRequest/Payum/Checker/PayumRequirementsChecker.php Show resolved Hide resolved

Prometee and others added 8 commits February 28, 2024 20:06

Fix typo

215a74b

Co-authored-by: Dmitri Perunov <diimpp@gmail.com>

Add Cancel action

cb5bea2

Fix typo

a7fad5a

Co-authored-by: Dmitri Perunov <diimpp@gmail.com>

Drop "!"

51d92a1

Co-authored-by: Dmitri Perunov <diimpp@gmail.com>

Reorder methods

ab3556e

Fix sentences

3acc5e3

Inline type

8f44b5f

Refactor to a Resolver

98ba814

Prometee requested review from NoResponseMate and diimpp February 28, 2024 19:55

lchrusciel reviewed Feb 29, 2024

View reviewed changes

Prometee added 2 commits February 29, 2024 22:50

Rename Checker to Resolver

eb0b9bd

Introducing PaymentRequestAnnoncer same as CatalogPromotionAnnouncer

0be4e3f

NoResponseMate changed the base branch from 1.13 to payment-request March 1, 2024 14:46

NoResponseMate deleted the branch Sylius:payment-request March 1, 2024 15:25

NoResponseMate closed this Mar 1, 2024

Prometee mentioned this pull request Mar 4, 2024

[API] Payment request #15943

Merged

Prometee mentioned this pull request Mar 31, 2024

[Roadmap] RFC: Future of Payum (payments) in Sylius #12521

Closed

Prometee mentioned this pull request May 8, 2024

Payment request a new way to handle payments #16237

Open

6 tasks


		$command = $this->paymentRequestCommandProvider->provide($paymentRequest);

		$this->commandBus->dispatch($command);

Uh oh!

Conversation

Prometee commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bunnyshell Preview Environment deleted

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NoResponseMate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoResponseMate Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

diimpp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Prometee commented Feb 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Prometee Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Prometee Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Prometee commented Nov 6, 2023 •

edited

Loading

github-actions bot commented Nov 6, 2023 •

edited

Loading

NoResponseMate Feb 23, 2024 •

edited

Loading

diimpp left a comment •

edited

Loading

Prometee Feb 29, 2024 •

edited

Loading

Prometee Feb 29, 2024 •

edited

Loading