-
Notifications
You must be signed in to change notification settings - Fork 66
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
Add captures API endpoints #371
Conversation
I added a unit test for Additionally I attempted to add an integration test, testing the creation of a payment meant to be captured, and then creating a capture for it. From what I can tell, there currently is no way to authorize the payment programmatically, which is why I disabled the test, leaving a comment. In the Test Docs, I found 'magic amounts' for testing card payment failures. @fjbender What is your take on this? Other than that this PR should be good to merge.
|
6960c56
to
25886e3
Compare
Hmm, this pattern is also present for regular payments if you want to test refunds: mollie-api-node/tests/integration/payments.test.ts Lines 38 to 46 in 412104f
Being able to programatically set a certain payment state for test payments would help tremendously with testing captures and refunds. For now we would need to ignore it and manually verify it works. I will bring this back to the team for consideration. |
Good find. I'll update my test to use the same way, so we're consistent. |
92d0891
to
8ba1531
Compare
I added the tests, and found an issue with the existing test for payment refunds. It would always use the first payment of the page, which is always the last one created. The old test only worked as expected if it was the only one that ran. I fixed this by using the Unfortunately there's also an issue with the test pipeline: Unfortunately credit cards don't seem to be enabled for the Test Account used in the CI-Pipeline. Other than that, the PR should now be ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, went through some iterations on the integration tests flow.
Ran all tests locally, works well!
All right, this means we need to fix the issue with the test-account and we're good to go. |
I have a suspicion, but uncovered we are hard-coding the API key in the Github actions definition, which is a Bad Idea™ I have added a different Test API Key into a secret variable and proposed #386 which also fixed the error you were seeing. |
Ouch. I was so confident this would be passed in from a repo secret, I didn't even check 😅. |
This now fails because it's missing the API key. |
It's a different key :) Secrets are not passed on:
So maybe we should be creating branches from within the repo in the future. I will go ahead and merge this one. |
I was unaware of this. This may actually have been the reason why the test key was hardcoded. Internally we can definitely start making the PRs from this repo, but it's quite unfortunate that this now means all external PRs from forks fail by default. We might want to set up a separate workflow for these cases? |
Rest assured, friends: the API key which existed in the GitHub action was one that I created once upon a time, specifically for this purpose. It wasn't one of our "real" API keys. The issue ‒ if we consider it that ‒ of forks not being able to run the tests is indeed why we weren't using a proper secret before. I'll discuss this further in #387. |
This adds missing support for the captures API.
Specifically adding
paymentCaptures.create
and missing types.