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.

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

feat(mollie-plugin): Support extra parameters for listing methods #2516

Conversation

casperiv0
Copy link
Contributor

@casperiv0 casperiv0 commented Nov 7, 2023

Description

This PR includes:

Breaking changes

Nope

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains a single feature
  • I have checked my own PR

⚡ Most of the time:

  • I have added or updated test cases
  • I have updated the README if needed

closes #2510

Copy link

netlify bot commented Nov 7, 2023

Deploy Preview for effervescent-donut-4977b2 canceled.

Name Link
🔨 Latest commit 24bab6e
🔍 Latest deploy log https://app.netlify.com/sites/effervescent-donut-4977b2/deploys/654f70864d85f70008e25ff0

@casperiv0 casperiv0 marked this pull request as ready for review November 7, 2023 15:44
@casperiv0 casperiv0 marked this pull request as draft November 7, 2023 15:44
@casperiv0 casperiv0 marked this pull request as ready for review November 7, 2023 15:53
@StampixSMO
Copy link
Contributor

I'm being undecisive if it makes more sense to have Vendure just calculate the amount/currency, instead of having a frontend pass it in via GQL inputs.

On one hand, it allows a for a broader usage of the query endpoint, on the other hand it allows for unintentional wrong currencies or values that are not equal to the active order one's.

Curious to hear input from @martijnvdbrug & @michaelbromley

@martijnvdbrug
Copy link
Collaborator

martijnvdbrug commented Nov 9, 2023

I agree with @StampixSMO, I would say amount, currency and billingCountry should be passed to Mollie by Vendure. These are all properties of an order and should not be determined by the frontend.

I do see locale as a front end responsibility, though, so that is a nice addition 👍

@StampixSMO
Copy link
Contributor

Thanks for the input @martijnvdbrug

Shouldn't the locale also just be inferred from the RequestContext instead? In the end, that's also controlled by the frontend (using the proper query parameter) so it'd boil down to the same thing but aligns better with the rest of the core.

@casperiv0
Copy link
Contributor Author

Shouldn't the locale also just be inferred from the RequestContext instead? In the end, that's also controlled by the frontend (using the proper query parameter) so it'd boil down to the same thing but aligns better with the rest of the core.

The locale used by Mollie will most likely be different then the one used by a storefront. Mollie only accepts certain locales :)

@martijnvdbrug
Copy link
Collaborator

@StampixSMO Good point, I agree on that. But as @casperiv0 said, Mollie unfortunately only supports specific locales. For payment intent creation we also do a best guess of the locale (see this line), so it would make sense to be able to overidde this from a storefront perspective.

This goes both for eligibleMollieMethods as well as for createPaymentIntent (@casperiv0 if you want the payment intent to be in the same locale, you'd have to add locale to that mutation as well`

@casperiv0
Copy link
Contributor Author

@martijnvdbrug, I've added the locale as an input type for the mutation. This PR is now ready for review :)

@casperiv0
Copy link
Contributor Author

Failing build doesn't look to be related to this PR

@michaelbromley
Copy link
Member

Thanks for this PR and for the feedback which helps me understand the issues involved.

I have a couple of questions:

  1. Is there any way this could act as a breaking change? Could it happen that someone upgrades and then the methods being returned from Mollie changes in their checkout?
  2. Is it possible a user might want to specifically omit one or more of these properties when listing methods? Would we need to consider some way to configure this?
  3. Is it possible a user might want to include other properties such as sequenceType or includeWallets?

The way we do it in the Stripe Plugin is that we allow the user to supply a function in the plugin init() method which defines what metadata we send to Stripe when interacting with their APIs. Could a similar pattern make sense here?

@casperiv0
Copy link
Contributor Author

casperiv0 commented Nov 11, 2023

Thanks for this PR and for the feedback which helps me understand the issues involved.

I have a couple of questions:

  1. Is there any way this could act as a breaking change? Could it happen that someone upgrades and then the methods being returned from Mollie changes in their checkout?
  2. Is it possible a user might want to specifically omit one or more of these properties when listing methods? Would we need to consider some way to configure this?
  3. Is it possible a user might want to include other properties such as sequenceType or includeWallets?

The way we do it in the Stripe Plugin is that we allow the user to supply a function in the plugin init() method which defines what metadata we send to Stripe when interacting with their APIs. Could a similar pattern make sense here?

That is possible that users would want to provide more information. Yeah, I also think adding that metadata option would be beneficial for all. I'll start implementing that soon.

@casperiv0
Copy link
Contributor Author

Implemented for the enabled payment methods query. Let me know if it should also be done for the payment intent :)!

@michaelbromley
Copy link
Member

@casperiv0 excellent work!

So last thing: this should be made again the minor branch because it is a new feature.

And yes let's add support for the payment intent too but we'll get this one merged first and then do it as a separate PR.

@casperiv0 casperiv0 changed the base branch from master to minor November 14, 2023 16:11
@casperiv0 casperiv0 force-pushed the feat/support-extra-params-mollie-plugin branch from 24bab6e to a182d76 Compare November 14, 2023 16:16
@casperiv0
Copy link
Contributor Author

Thanks! @michaelbromley base branch has been updated!

@michaelbromley michaelbromley merged commit cb9846b into vendure-ecommerce:minor Nov 15, 2023
9 checks passed
alexisvigoureux pushed a commit to alexisvigoureux/vendure that referenced this pull request Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mollie plugin returns unavailable payment methods for given currency
4 participants