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

[BUG] Payments list doesn't work with pagination parameters #1439

Closed
lsampras opened this issue Jun 14, 2023 · 9 comments · Fixed by #1556
Closed

[BUG] Payments list doesn't work with pagination parameters #1439

lsampras opened this issue Jun 14, 2023 · 9 comments · Fixed by #1556
Assignees
Labels
A-core Area: Core flows C-bug Category: Bug good first issue Good for newcomers help wanted Extra attention is needed

Comments

@lsampras
Copy link
Member

Context

  • Currently the API accepts payment_id as pagination parameters for e.g starting_after = pay_Ajabdhakjhah
  • However the underlying code relies on the auto-increment characteristic of databases
  • While we do some sort of conversion here our pagination filters are based on id whereas the sort is modified_at
  • This causes the response list to be not actually be paginated but return inconsistent & wrong results

Possible Solutions

  • Either we can update the pagination filters to use modified_at (which can cause inconsistencies with data lists since modified_at can be changed)
  • We can also try to sort out the list by incremental id
@lsampras lsampras added A-core Area: Core flows C-bug Category: Bug labels Jun 14, 2023
@ShankarSinghC ShankarSinghC self-assigned this Jun 15, 2023
@Abhicodes-crypto
Copy link
Contributor

It was sorted by id before it is changed in this PR - #317
The same thing has been repeated for disputes and refunds as well.

@lsampras lsampras added help wanted Extra attention is needed good first issue Good for newcomers labels Jun 15, 2023
@NavinShrinivas
Copy link
Contributor

Hey @lsampras I'd like to work on this, can I be assigned to it?

@lsampras
Copy link
Member Author

lsampras commented Jun 21, 2023

Ideally we would want it sorted by the modified_atcreated_at DESC column in payment_intent.

Also we use futures::stream::iter when getting the associated payment_attempts,
If possible you can try to make this concurrent as well for extra challenge/brownie points

@NavinShrinivas
Copy link
Contributor

NavinShrinivas commented Jun 21, 2023

Just as a clarification, we want the payments_list to be sorted by created_at in DESC ordered as possibly the front-end pagination depends on it being sorted that way?

Also, we would probably sort wrt created_at in the payments intent either after we fetch the "pa" or before we fetch, I don't see how keeping the sorting concurrent will help...unless there is some way to keep the sorting at the same time as fetching payment_attempts

@lsampras
Copy link
Member Author

Just as a clarification, we want the payments_list to be sorted by created_at in DESC ordered as possibly the front-end pagination depends on it being sorted that way?

Yeah, we prefer returning payments list from most recent to least recent...

I don't see how keeping the sorting concurrent will help...unless there is some way to keep the sorting at the same time as fetching payment_attempts

I meant that the db calls could be moved to a concurrent model as with the current approach we still call database queries in a sequential mode...

you can probably sort again once the pa's are fetched or you can keep a map of payment_intent_id <-> payment_attempt & order the received payment_attempts based on the initial list of payment_intents

@NavinShrinivas
Copy link
Contributor

Update : I have some code waiting for this issue, it implements async db fetches and uses a map, this has some considerable changes and would like to test before I open a PR.

@NavinShrinivas
Copy link
Contributor

image
This is despite havbing created an API key and it beign stored to variables, any idea?

@NavinShrinivas
Copy link
Contributor

@lsampras if the above commit seems fine, I'll open a p. I've tested in local and it returns payment lists from most recent to oldest payments.

@SanchithHegde
Copy link
Member

Closing as completed in #1556.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Core flows C-bug Category: Bug good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants