-
Notifications
You must be signed in to change notification settings - Fork 89
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(open-payments): add list incoming payments #851
Conversation
describe('backward pagination', (): void => { | ||
test.each` | ||
last | cursor | ||
${undefined} | ${uuid()} | ||
${5} | ${uuid()} |
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.
This test is setup correctly, but in general makes me confused about the backward pagination parameters.
If we only provide a cursor
, like (backwards-pagination allows) then by default we will end up forwards paginating, based on this code in the backend
:
rafiki/packages/backend/src/shared/pagination.ts
Lines 9 to 20 in b19919a
export function parsePaginationQueryParameters({ | |
first, | |
last, | |
cursor | |
}: PageQueryParams): Pagination { | |
return { | |
first, | |
last, | |
before: last ? cursor : undefined, | |
after: cursor && !last ? cursor : undefined | |
} | |
} |
would this mean that we should make last
required as well in Open Payments API? Only cursor
is required:
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.
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.
I'm in favour of not supporting both parameters (for now) to make the Open Payments pagination type simple & similar to what you describe in comment:
after + count
|| before + count
|| count
(default forward pagination)
|
||
for (const incomingPayment of incomingPayments.result) { | ||
try { | ||
validateIncomingPayment(incomingPayment) |
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.
For list responses, we could validate that pagination.hasPreviousPage
is false
if cursor
was not specified (which should be forward pagination from the start).
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.
Mostly agree, but I'm not sure it's worth failing the whole request over it, however.
@@ -74,6 +77,21 @@ describe('incoming-payment', (): void => { | |||
method: HttpMethod.POST | |||
}) | |||
}) | |||
|
|||
test('creates listIncomingPaymentsOpenApiValidator', async (): Promise<void> => { |
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.
@raducristianpopa can we try testing that the list
route calls the proper validator, similar to this test? https://github.com/interledger/rafiki/pull/848/files#diff-4f318c2a374cc145f8825fecf95d77b29a728927b5a64ce393897b01becb2b93R37-R38
If it takes too long, I'm going to approve the PR anyway so we can merge it later and working on these route tests at a separate time
|
||
for (const incomingPayment of incomingPayments.result) { | ||
try { | ||
validateIncomingPayment(incomingPayment) |
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.
Mostly agree, but I'm not sure it's worth failing the whole request over it, however.
return { | ||
// https://jestjs.io/docs/jest-object#jestmockmodulename-factory-options | ||
__esModule: true, | ||
...jest.requireActual('./requests') |
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.
I think we can mock out the get
here, so we don't need to .mockResolvedValueOnce(incomingPaymentPaginationResult)
on the getSpy
below
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.
I also think we can remove __esModule: true,
because we aren't using a default export
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.
I think we can mock out the
get
here, so we don't need to.mockResolvedValueOnce(incomingPaymentPaginationResult)
on thegetSpy
below
If we mock the get
, all the tests that are using it will fail:
Test Suites: 1 failed, 1 total
Tests: 8 failed, 26 passed, 34 total
I also think we can remove __esModule: true, because we aren't using a default export
When __esModule: true
is not specified, we get the following error:
incoming-payment › routes › list › calls get method with correct validator
Error when making Open Payments GET request: Nock: No match for request {
"method": "GET",
"url": "http://localhost:1000/.well-known/pay/incoming-payments",
"headers": {
"accept": "application/json, text/plain, */*",
"content-type": "application/json",
"authorization": "GNAP accessToken",
"signature": "sig1=:VYmcFqf6cSzLN10+dG/TnXQI23FxWY/n2/wVbNW4oX+U4rCBHpbT75AaMZOe3qzoUDiQUxZg0fpXjSBxHkNpDw==:",
"signature-input": "sig1=(\"@method\" \"@target-uri\" \"authorization\");created=1671199508;keyid=\"default-key-id\";alg=\"ed25519\"",
"user-agent": "axios/1.1.2",
"accept-encoding": "gzip, deflate, br"
}
}
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.
Looks good, can merge in once the conflict is resolved
8d6ccd0
Changes proposed in this pull request
Context
Checklist
fixes #number