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

fix(mollie): Add missing status type #2499

Conversation

casperiv0
Copy link
Contributor

Description

Adds a missing status field to the molliePaymentMethods query. This type was previously not typed by Mollie. It is documented however.

PR on Mollie's API plugin: mollie/mollie-api-node#335

Breaking changes

None.

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

Copy link

netlify bot commented Oct 31, 2023

❌ Deploy Preview for effervescent-donut-4977b2 failed.

Name Link
πŸ”¨ Latest commit 2c00333
πŸ” Latest deploy log https://app.netlify.com/sites/effervescent-donut-4977b2/deploys/65411714d6c96500085e67f7

@casperiv0
Copy link
Contributor Author

Not sure if the generated types have to be included, my guess is that they're formatting changes. yarn format seems to be configured incorrectly, unfortunately?

@michaelbromley
Copy link
Member

Hi!
Thanks for the PR πŸ‘
@martijnvdbrug would you mind just quickly reviewing the addition of the field here?

Regarding the generated type files - yes they should be updated just to add this new field, but that should be a single line change. What happens if you run codegen without then formatting afterwards?

@casperiv0
Copy link
Contributor Author

Regarding the generated type files - yes they should be updated just to add this new field, but that should be a single line change. What happens if you run codegen without then formatting afterwards?

Should be resolved now :)!

@martijnvdbrug
Copy link
Collaborator

Well, the Mollie API only returns active payment methods, so this field will always return 'activated'. What is the reason for adding this field?

@casperiv0
Copy link
Contributor Author

Well, the Mollie API only returns active payment methods, so this field will always return 'activated'. What is the reason for adding this field?

It doesn't actually! It will also return ones with status null. We want to be able to manually remove those in our codebase

@michaelbromley
Copy link
Member

It doesn't actually! It will also return ones with status null.

Is that a bug in the Mollie API or SDK? We're using the mollie client methods.list method:

const methods = await client.methods.list({ resource: 'orders' });

and the jsdoc for that method says:

Retrieve all enabled payment methods. The results are not paginated.

whereas in the mollie docs it says that null signifies that the payment has not been requested
image

so what doesn't make sense to me is how a "not requested" (null) method can at the same time be "enabled". Something doesn't add up.

@casperiv0
Copy link
Contributor Author

Yeah, does seem to be a bug in the Mollie API. Additional context: We're loading the enabled Mollie payment methods, however, "Vouchers" show up with status null. They cannot be disabled directly via the Mollie dashboard.
The quickest way was to filter out the invalid statuses while waiting to hear back from Mollie.

@michaelbromley michaelbromley merged commit 071aa9d into vendure-ecommerce:master Nov 3, 2023
15 checks passed
@michaelbromley
Copy link
Member

OK, thanks for the clarifications. There's no harm adding this field even in future if Mollie fixes their probably bug and then it will just always be "activated"

@martijnvdbrug
Copy link
Collaborator

Oh wow, good find. Then yes, in this case we should just proxy the status field. Thanks!

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.

None yet

3 participants