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

Add captures API endpoints #371

Merged
merged 12 commits into from
Sep 30, 2024
Merged

Conversation

janpaepke
Copy link
Collaborator

@janpaepke janpaepke commented Sep 11, 2024

This adds missing support for the captures API.
Specifically adding paymentCaptures.create and missing types.

@janpaepke janpaepke added the API extension Previously missing endpoints/binders. label Sep 11, 2024
@janpaepke janpaepke added this to the 4.1.0 milestone Sep 11, 2024
@janpaepke janpaepke changed the title Add missing captures api Add missing captures API Sep 11, 2024
@janpaepke
Copy link
Collaborator Author

I added a unit test for capture.create().

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.
Unfortunately the issue now is that the payment can only be captured, once it was authorized by the user.
In test-mode this happens through interaction in the checkout screen, available at the checkout URL.

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.
Maybe one way of implementing better testability is to (on the mollie side) add magic amounts, which set the authorization status of the payment.

@fjbender What is your take on this?

Other than that this PR should be good to merge.
How we treat the skipped integration test depends on the feedback.
We can:

  1. Leave it in and update it at a later point, once mollie offers a way to programmatically authorize a payment.
  2. Fix it now (before merging), because there might already be a way I simply am unaware of.
  3. Remove it because there will never be a way or we don't want to test it (this way).

@fjbender
Copy link
Contributor

Hmm, this pattern is also present for regular payments if you want to test refunds:

if (payment.status != PaymentStatus.paid) {
console.log('If you want to test the full flow, set the payment to paid:', payment.getCheckoutUrl());
return;
}
if (!payment.isRefundable()) {
console.log('This payment is not refundable, you cannot test the full flow.');
return;
}

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.

@janpaepke
Copy link
Collaborator Author

Hmm, this pattern is also present for regular payments if you want to test refunds:

Good find. I'll update my test to use the same way, so we're consistent.

@janpaepke
Copy link
Collaborator Author

janpaepke commented Sep 26, 2024

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 test expected this to be the payment from the previous run, which logs the checkout URL.
Unfortunately the should refresh test right below also creates a new payment. This means that even when manually setting the payment status to paid, the test would create a new payment every time, since payment at index 0 would now be the one from the update test.

The old test only worked as expected if it was the only one that ran.

I fixed this by using the metadata field to uniquely identify the correct payment.
Note that this might still fail in the future, if there are ever more payments created in one run, than are included on the default page. But this is definitely better than before.

Unfortunately there's also an issue with the test pipeline:
Testing payment captures requires payment methods that support it, like credit cards. When testing refunds I also stumbled across the issue that some payment methods don't allow refunds, until the amount has arrived in the bank.
So I limited both payment creations to creditcard.

Unfortunately credit cards don't seem to be enabled for the Test Account used in the CI-Pipeline.
@Pimm do you know which account this is? Who has access and can change those settings?

Other than that, the PR should now be ready for review.

@janpaepke janpaepke marked this pull request as ready for review September 26, 2024 14:26
@janpaepke janpaepke requested a review from Pimm September 26, 2024 19:29
@janpaepke janpaepke changed the title Add missing captures API Add captures API endpoints Sep 27, 2024
src/data/payments/captures/CaptureHelper.ts Outdated Show resolved Hide resolved
src/data/payments/captures/data.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@edorivai edorivai left a 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!

@janpaepke
Copy link
Collaborator Author

All right, this means we need to fix the issue with the test-account and we're good to go.
@fjbender Do you by any chance know which account the pipeline uses and who has access?

@fjbender
Copy link
Contributor

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.

@janpaepke
Copy link
Collaborator Author

Ouch. I was so confident this would be passed in from a repo secret, I didn't even check 😅.
If you didn't already do that, we should probably delete the old key and create a new one to store in secrets.TEST_API_KEY.

@janpaepke
Copy link
Collaborator Author

This now fails because it's missing the API key.
However your PR containing the change to GH secrets did not fail, so what gives?
Does it not have access, because this PR was created before the secret?!

@fjbender
Copy link
Contributor

If you didn't already do that, we should probably delete the old key and create a new one to store in secrets.TEST_API_KEY.

It's a different key :)

Secrets are not passed on:

They are not passed to workflows that are triggered by a pull request from a fork.

So maybe we should be creating branches from within the repo in the future.

I will go ahead and merge this one.

@fjbender fjbender merged commit a73f311 into mollie:master Sep 30, 2024
2 of 5 checks passed
@janpaepke
Copy link
Collaborator Author

They are not passed to workflows that are triggered by a pull request from a fork.

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?
I'll create an issue for this, so we can discuss.

@janpaepke janpaepke deleted the feature/captures branch October 2, 2024 12:07
@Pimm
Copy link
Collaborator

Pimm commented Oct 3, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API extension Previously missing endpoints/binders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants