feat(payments-plugin): mollie plugin make `redirecturl` dynamic #2094

seminarian · 2023-03-22T18:57:58Z

closes #2093

It's my first contribution, best to check everything well.
Also didn't know if I had to add an @since tag somewhere and is the Breaking change in the commit message correct?

Relates to vendure-ecommerce#2093. This commit removes redirectUrl from the payment method and lets it get passed via the Shop API createPaymentIntent mutation. BREAKING CHANGE: `redirectUrl` got removed from the mollie payment method. Consumers of the ecommerce-engine now have to pass `redirectUrl` via the createPaymentIntent mutation.

michaelbromley · 2023-03-23T08:23:36Z

Hi, congrats on your first PR to Vendure! 🥳

So I'd prefer not to make this a breaking change if we can avoid that. How about we keep the redirectUrl config arg, and make the new GraphQL input field optional. So the logic would be:

If the redirectUrl is passed in the createMolliePaymentIntent mutation, we use it.
If not, we use the configured redirectUrl from the PaymentMethod config args.

We discussed this via Slack and the conclusion is to:

Keep the configArgs so it is not a breaking change
Add a new config arg to toggle whether the redirectUrl should come from the config arg or from the mutation input
Add validation to enforce this

martijnvdbrug · 2023-03-23T16:40:24Z

Hi, congrats on your first PR to Vendure! partying_face

So I'd prefer not to make this a breaking change if we can avoid that. How about we keep the redirectUrl config arg, and make the new GraphQL input field optional. So the logic would be:

If the redirectUrl is passed in the createMolliePaymentIntent mutation, we use it.

If not, we use the configured redirectUrl from the PaymentMethod config args.

We discussed this via Slack and the conclusion is to:

Keep the configArgs so it is not a breaking change

Add a new config arg to toggle whether the redirectUrl should come from the config arg or from the mutation input

Add validation to enforce this

Oops, this might have been my suggestion. My argument is: The same and more can be accomplished with the redirectUrl via graphql, so why maintain both ways?

…RedirectUrl option for mollie

seminarian · 2023-03-23T20:17:43Z

Alright well I did the changes as Michael requested them.
Once the final decision has been made I'll do a rebase to clean it up and select the version we want to keep.

About this version:
Only did the tests for useDynamicRedirectUrl set to false. Added some tests to test the validation.

Doing the tests for useDynamicRedirectUrl would probably have changed a too huge of a diff and make it impossible to merge with Martijns changes in #2092

Let me know what you think.

michaelbromley · 2023-03-24T08:00:05Z

packages/payments-plugin/src/mollie/mollie.plugin.ts

+     * For backwards compatibility, by default set to false.
+     * When enabled, the `redirectUrl` can be passed via the `createPaymentIntent` mutation 
+     * instead of being configured in the Payment Method.
+     */


You can add

@default false @since 2.0.0

It's being merged to minor right? But still only @SInCE 2.0.0 ? Would expect that to be the next minor release actually?

There won't be another minor release!

michaelbromley · 2023-03-24T08:00:47Z

packages/payments-plugin/src/mollie/mollie.service.ts

@@ -101,17 +103,32 @@ export class MollieService {
        if (!paymentMethod) {
            return new PaymentIntentError(`No paymentMethod found with code ${paymentMethodCode}`);
        }
+        if (this.options.useDynamicRedirectUrl == true) {
+            if (!input.redirectUrl) {
+                return new InvalidInputError(`Cannot create payment intent without redirectUrlspecified`);


Missing a space "redirectUrlspecified"

michaelbromley · 2023-03-24T08:04:10Z

Oops, this might have been my suggestion. My argument is: The same and more can be accomplished with the redirectUrl via graphql, so why maintain both ways?

@martijnvdbrug yeah that's a valid point. My counter-argument was that with v2 there are already lots of breaking changes for people to deal with, so I wanted to avoid another one here. We can certainly deprecate the old way and remove it in a future version though. In fact, @seminarian it is probably worth starting that process now and mentioning that the configArg will be deprecated in the documentation.

@seminarian it looks good to me other than the couple of small comments.

martijnvdbrug · 2023-03-24T08:13:20Z

Very understandable! Deprecation is perfect. As long as we can clean it up at some point, I'm a very happy man 😄

martijnvdbrug · 2023-03-24T10:39:45Z

packages/payments-plugin/src/mollie/mollie.handler.ts

-            type: 'string',
-            label: [{ languageCode: LanguageCode.en, value: 'Redirect URL' }],
-            description: [
-                { languageCode: LanguageCode.en, value: 'Redirect the client to this URL after payment' },


Can/should we also add a deprecation message here?

michaelbromley · 2023-03-24T11:17:23Z

@seminarian Martijn's other Mollie PR is now merged in and the minor branch includes it.

StampixSMO · 2023-03-24T12:20:53Z

@michaelbromley @seminarian @martijnvdbrug Since this PR's goal is to make the redirect URL more dynamic, I would suggest to not append the order code to the end of the incoming redirectUrl.

It forces the storefront unnecessarily to be able to accept that order code as a last path parameter. Wouldn't it be better to have the storefront decide on what it can or cannot handle? I'm thinking: native mobile apps, less flexible frontend frameworks where query parameters are preferred, ...

So basically, the frontend would send the full redirectUrl, e.g.:

https://storefront.tld/order/[code]/payment
storefront://check-order-payment?order=[code] (native app)
...

michaelbromley · 2023-03-24T12:50:15Z

@StampixSMO Great point, I agree.

…o true

StampixSMO · 2023-03-28T06:31:07Z

So while working on this, I bumped on another issue that is actually on Mollie's side (and has been there for a while).

Issue

If you don't specify a payment method, selecting a failed payment status on the Mollie hosted page results in being redirected to the payment selection screen and the redirection is never triggered.

I have attached a year-old screenshot from a support case where I asked about this bug and what they recommended as solution.

Solution

The bug is gone if you select a payment method upfront when making a test payment. For example paypal (creditcard is annoying as you need to enter actual credit card numbers).

You could have the storefront request a certain payment method already, using the exposed input variables. However, that's not ideal since the storefront has no idea whether Vendure+Mollie is running in test mode or not.

I would suggest that we force a payment method when the API key is a test key (can be checked using the test_ prefix).

Let me know what you think.
PS: I bumped that ticket and asked for another update on this bug.

seminarian · 2023-03-28T07:26:14Z

@michaelbromley Could you enlighten me what's actually the role of ArrangingPayment status in Vendure:

I noticed that the mollie plugin skips out on that, here you can see the docs: https://www.vendure.io/docs/typescript-api/payments-plugin/mollie-plugin/
The storefront guide mentions transitionToState mutation calls: https://www.vendure.io/docs/storefront/shop-api-guide/#order-flow

Like, does the not maintaining the "correct" Order state have any implications? Is that something we would want to see streamlined/added to the Mollie plugin?

seminarian · 2023-04-03T09:14:34Z

@martijnvdbrug I've managed to get the same payment multiple times listed in Vendure:

I was playing around with introducing exceptions in random places and ended up in place like this.
A 500 error was returned to Mollie and Mollie kept retrying the webhook.

I'd like to add the @Transaction() decorator to the mollie.controller.ts:

    @Post('mollie/:channelToken/:paymentMethodId')
    // @Transaction()
    async webhook(
        @Ctx() ctx: RequestContext,
        @Param('channelToken') channelToken: string,
        @Param('paymentMethodId') paymentMethodId: string,
        @Body() body: any,
    ): Promise<void> {
        if (!body.id) {
            return Logger.warn(` Ignoring incoming webhook, because it has no body.id.`, loggerCtx);
        }
        try {
            await this.mollieService.handleMollieStatusUpdate(ctx, { channelToken, paymentMethodId, orderId: body.id });
        } catch (error) {
            Logger.error(`Failed to process incoming webhook: ${error?.message}`, loggerCtx, error);
            throw error;
        }
    }

And instead of just creating a new RequestContext I pass it and in mollie.handler.ts in createPayment I add custom to the allowed sources:

        if (ctx.apiType !== 'admin' && ctx.apiType !== 'custom') {
            throw Error(`CreatePayment is not allowed for apiType '${ctx.apiType}'`);
        }

That seems to work to revert things when they go wrong.
WDYT?

martijnvdbrug · 2023-04-04T07:29:23Z

@seminarian That's a good improvement!

Just make sure that we can't end up with a scenario where the actual payment has been done through Mollie successfully, but reverted in Vendure. As long as the error boils up to the controller, and thus returning a non-200 to Mollie, we should be good, because Mollie will retry the webhook

michaelbromley · 2023-04-19T14:48:50Z

Hi @seminarian & @martijnvdbrug
Apologies for the lack of response from my side - I was wrapped up in getting the beta release out and a bunch of other things.

Could you enlighten me what's actually the role of ArrangingPayment status in Vendure:

The original intent is that in this state:

The order cannot be modified, so the price will not change
The order is not yet placed because payment has not completed yet.

It turns out that there are a wide variety of flows with different payment providers, so e.g. in the Mollie integration we don't require the storefront the do the transition as it is handled internally.

I've managed to get the same payment multiple times listed in Vendure:

Is any further action needed - do you intend to add the Transaction decorator as suggested?

martijnvdbrug · 2023-04-19T18:46:04Z

@seminarian let me know if there are any open points, could do a short call if needed

seminarian · 2023-04-20T07:47:03Z

It turns out that there are a wide variety of flows with different payment providers, so e.g. in the Mollie integration we don't require the storefront the do the transition as it is handled internally.

For our usecase, a photo site, it would sure come in handy to hook into onTransitionStart and perform validation if OrderLines are properly configured.
Though we can just let our FE go to ArrangingPayment first I guess :-)

Is any further action needed - do you intend to add the Transaction decorator as suggested?

I've pushed the changes!

@michaelbromley and @martijnvdbrug perhaps one of you guys could audit my changes or test it.. Thanks! :)

seminarian · 2023-04-21T08:54:11Z

I've discovered a small validation issue. The Mollie library throws if firstName, lastName or emailAddress are empty strings. Will add a check for that.

seminarian · 2023-04-24T05:46:46Z

@michaelbromley & @martijnvdbrug done!

martijnvdbrug

LGTM!

michaelbromley · 2023-04-24T07:45:03Z

Excellent work @seminarian! This will go in the next v2 beta release.

seminarian added 3 commits March 22, 2023 19:52

fix(payments-plugin): fix typo

4a5d864

fix: undo unwanted prettify of file

0ad1880

feat(payments-plugin): avoid breaking changes, introducing useDynamic…

4d990a9

…RedirectUrl option for mollie

michaelbromley reviewed Mar 24, 2023

View reviewed changes

martijnvdbrug reviewed Mar 24, 2023

View reviewed changes

seminarian added 3 commits March 24, 2023 16:42

feat: no appendage of redirectUrl when useDynamicRedirectUrl is set t…

d792eb3

…o true

chore: add some tests for useDynamicRedirectUrl set to true

ea773a6

chore: update docs

1f22e15

seminarian requested review from martijnvdbrug and michaelbromley and removed request for martijnvdbrug and michaelbromley March 24, 2023 15:42

fix: add transaction decorator

93394cb

fix: add some validation for fields required by Mollie

371bd6b

martijnvdbrug approved these changes Apr 24, 2023

View reviewed changes

michaelbromley merged commit b452419 into vendure-ecommerce:minor Apr 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(payments-plugin): mollie plugin make `redirecturl` dynamic #2094

feat(payments-plugin): mollie plugin make `redirecturl` dynamic #2094

seminarian commented Mar 22, 2023

michaelbromley commented Mar 23, 2023 •

edited

Loading

martijnvdbrug commented Mar 23, 2023

seminarian commented Mar 23, 2023

michaelbromley Mar 24, 2023

seminarian Mar 24, 2023

michaelbromley Mar 24, 2023

michaelbromley Mar 24, 2023

michaelbromley commented Mar 24, 2023

martijnvdbrug commented Mar 24, 2023 •

edited

Loading

martijnvdbrug Mar 24, 2023

michaelbromley commented Mar 24, 2023

StampixSMO commented Mar 24, 2023 •

edited

Loading

michaelbromley commented Mar 24, 2023

StampixSMO commented Mar 28, 2023 •

edited

Loading

seminarian commented Mar 28, 2023

seminarian commented Apr 3, 2023 •

edited

Loading

martijnvdbrug commented Apr 4, 2023

michaelbromley commented Apr 19, 2023

martijnvdbrug commented Apr 19, 2023

seminarian commented Apr 20, 2023 •

edited

Loading

seminarian commented Apr 21, 2023

seminarian commented Apr 24, 2023

martijnvdbrug left a comment

michaelbromley commented Apr 24, 2023

feat(payments-plugin): mollie plugin make redirecturl dynamic #2094

feat(payments-plugin): mollie plugin make redirecturl dynamic #2094

Conversation

seminarian commented Mar 22, 2023

michaelbromley commented Mar 23, 2023 • edited Loading

martijnvdbrug commented Mar 23, 2023

seminarian commented Mar 23, 2023

michaelbromley Mar 24, 2023

Choose a reason for hiding this comment

seminarian Mar 24, 2023

Choose a reason for hiding this comment

michaelbromley Mar 24, 2023

Choose a reason for hiding this comment

michaelbromley Mar 24, 2023

Choose a reason for hiding this comment

michaelbromley commented Mar 24, 2023

martijnvdbrug commented Mar 24, 2023 • edited Loading

martijnvdbrug Mar 24, 2023

Choose a reason for hiding this comment

michaelbromley commented Mar 24, 2023

StampixSMO commented Mar 24, 2023 • edited Loading

michaelbromley commented Mar 24, 2023

StampixSMO commented Mar 28, 2023 • edited Loading

seminarian commented Mar 28, 2023

seminarian commented Apr 3, 2023 • edited Loading

martijnvdbrug commented Apr 4, 2023

michaelbromley commented Apr 19, 2023

martijnvdbrug commented Apr 19, 2023

seminarian commented Apr 20, 2023 • edited Loading

seminarian commented Apr 21, 2023

seminarian commented Apr 24, 2023

martijnvdbrug left a comment

Choose a reason for hiding this comment

michaelbromley commented Apr 24, 2023

feat(payments-plugin): mollie plugin make `redirecturl` dynamic #2094

feat(payments-plugin): mollie plugin make `redirecturl` dynamic #2094

michaelbromley commented Mar 23, 2023 •

edited

Loading

martijnvdbrug commented Mar 24, 2023 •

edited

Loading

StampixSMO commented Mar 24, 2023 •

edited

Loading

StampixSMO commented Mar 28, 2023 •

edited

Loading

seminarian commented Apr 3, 2023 •

edited

Loading

seminarian commented Apr 20, 2023 •

edited

Loading