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.

Sign up for GitHub

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

Jump to bottom

feat: invoice pagination #3512

Merged
merged 3 commits into from
Nov 8, 2023
Merged

feat: invoice pagination #3512

merged 3 commits into from
Nov 8, 2023

Conversation

UncleSamtoshi
Copy link
Contributor

@UncleSamtoshi UncleSamtoshi commented Nov 7, 2023 •
edited
Loading

  • add paginated invoices to account and wallet objects

DISCLAIMER:

  • There is a potential for a bug because we are paginating using the dates of invoices to create an ordering. If two invoices were to have the exact same date and were used as a pagination cursor, one of the invoices would be skipped. It is highly unlikely that two invoices would have the exact same timestamp however it is not impossible. An alternative to this approach would be to sort on timestamp AND id however it would most likely require extra round trips to the database and be a performance hit.

Follow Up Items:

  • Remove pending transactions from transactions pagination
  • Fix transaction backwards pagination
  • Switch transaction pagination types

@github-actions github-actions bot added the core label Nov 7, 2023
bodymindarts
bodymindarts reviewed Nov 8, 2023
View reviewed changes
core/api/src/app/wallets/get-invoices-for-wallets.ts Outdated
const walletIds = wallets.map((wallet) => wallet.id)

const parsedPaginationArgs = parsePaginationArgs({
const parsedPaginationArgs = checkedToPaginatedQueryArgs({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the variable should now be checkedPaginationArgs? Or even better paginationArgs and rename the above one to rawPaginationArgs... the checked type should be the default one (ie simpler name) the unchecked one should have the special name (ie prefix).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will make this change.

core/api/src/domain/ledger/index.types.d.ts Outdated
@@ -2,6 +2,8 @@ type LedgerError = import("./errors").LedgerError
type FeeDifferenceError = import("./errors").FeeDifferenceError
type LedgerServiceError = import("./errors").LedgerServiceError

type PaginationArgs = import("graphql-relay").ConnectionArguments
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Domain should not depend on external libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already present before the PR. I think it might be showing up as a diff because I had moved it and then added it back and now there might be some whitespace change. I will have a follow up pr that fixes Transaction pagination by using the new types and removing pending transactions from it. The follow up pr can not be merged till the mobile app has migrated to using the pendingTransactions endpoint.

bodymindarts
bodymindarts previously approved these changes Nov 8, 2023
View reviewed changes
@UncleSamtoshi
chore: address pr feedback
429b62d 
- remove use of graphql types
@UncleSamtoshi UncleSamtoshi merged commit 6232ee2 into main Nov 8, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bodymindarts bodymindarts bodymindarts approved these changes

Assignees
No one assigned
Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@UncleSamtoshi @bodymindarts