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

auto_paging_each should paginate back when ending_before is used #864

Closed
ruanltbg opened this issue Oct 9, 2019 · 3 comments · Fixed by #865
Closed

auto_paging_each should paginate back when ending_before is used #864

ruanltbg opened this issue Oct 9, 2019 · 3 comments · Fixed by #865

Comments

@ruanltbg
Copy link

ruanltbg commented Oct 9, 2019

  • all the code and examples are based on a limit of 4 elements by page

Following the Charge API documentation all the queries to it are going to return the charges on a DESCending order. That means, for charges created on this order:

Day Charges ID
1 1 , 2, 3, 4, 5, 6
2 7, 8, 9, 10
3 11. 12. 13

The following code will return:

charges = Stripe::Charge.list(
  limit: 4,
)
Page Charges ID
1 13, 12, 11, 10
2 9, 8, 7, 6
3 5, 4, 3, 2
4 1

I could easily use the auto_paging_each method and be happy.

I also can use the starting_after filter to navigate and filter my results and the auto_paging_each to loop through the result:

charges = Stripe::Charge.list(
  limit: 4,
  starting_after: '7'
)
Page Charges ID
1 6, 5, 4, 3
2 2, 1

But auto_paging_each is not compatible with ending_before, as I would get these results:

charges = Stripe::Charge.list(
  limit: 4,
  ending_before: '6'
)
Page Charges ID
1 10, 9, 8, 7
2 6, 5, 4, 3
3 2, 1

We are missing the charge 11 on this call and repeating everything after the charge 7.
The first call to the API is made right, but the code on auto_paging_each is:

page = page.next_page
break if page.empty?

and for this case, it should be

page = page. previous_page
break if page.empty?

If the latter code was used I would get this pagination:

Page Charges ID
1 10, 9, 8, 7
2 11

What is weird but it is ok, it is what the API is supposed to return.

I can not define if it is a bug or a new feature.
Please let me know if you need any help with this.
@brandur-stripe
Copy link
Contributor

Thanks @ruanltbg — great use of Markdown tables here. Supporting automatic pagination for ending_before is probably a good idea.

What is weird but it is ok, it is what the API is supposed to return.

Yeah, the API is a little quirky in this respect in that although you're probably trying to iterate backwards, you still go the normal direction within each page.

Let me take a quick look at this to see how difficult it'd be to implement.

brandur-stripe pushed a commit that referenced this issue Oct 9, 2019
@brandur
Support backwards pagination with list's #auto_paging_each
5c20fe4 
Previously, `#auto_paging_each` would always try to paginate forward,
even if it was clear based on the list's current filters that the user
had been intending to iterate backwards by specifying an `ending_before`
filter exclusively.

Here we implement backwards iteration by detecting this condition,
reversing the current list data, and making new requests for the
previous page (instead of the next one) as needed, which allows the user
to handle elements in reverse logical order.

Reversing the current page's list is intended as a minor user feature,
but may possibly be contentious. For background, when backwards
iterating in the API, results are still returned in "normal" order. So
if I specifying `ending_before=7`, the next page would look like `[4, 5,
6`] instead of `[6, 5, 4]`. In `#auto_paging_each` I reverse it to `[6,
5, 4]` so it feels to the user like they're handling elements in the
order they're iterating, which I think is okay. The reason it might be
contentious though is that it could be a tad confusing to someone who
already understands the normal `ending_before` ordering in the API.

Fixes #864.
@brandur brandur mentioned this issue Oct 9, 2019
Merged
@brandur-stripe
Copy link
Contributor

Opened #865.

brandur-stripe pushed a commit that referenced this issue Oct 10, 2019
@brandur @brandur-stripe
Support backwards pagination with list's #auto_paging_each (#865)
e3cc91d 
* Support backwards pagination with list's `#auto_paging_each`

Previously, `#auto_paging_each` would always try to paginate forward,
even if it was clear based on the list's current filters that the user
had been intending to iterate backwards by specifying an `ending_before`
filter exclusively.

Here we implement backwards iteration by detecting this condition,
reversing the current list data, and making new requests for the
previous page (instead of the next one) as needed, which allows the user
to handle elements in reverse logical order.

Reversing the current page's list is intended as a minor user feature,
but may possibly be contentious. For background, when backwards
iterating in the API, results are still returned in "normal" order. So
if I specifying `ending_before=7`, the next page would look like `[4, 5,
6`] instead of `[6, 5, 4]`. In `#auto_paging_each` I reverse it to `[6,
5, 4]` so it feels to the user like they're handling elements in the
order they're iterating, which I think is okay. The reason it might be
contentious though is that it could be a tad confusing to someone who
already understands the normal `ending_before` ordering in the API.

Fixes #864.

* Allow `ending_before` and `starting_after` to remain in hydrated list object
@brandur-stripe
Copy link
Contributor

brandur-stripe commented Oct 10, 2019 •
edited
Loading

Feature released in 5.7.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support backwards pagination with list's #auto_paging_each stripe/stripe-ruby
2 participants
@ruanltbg @brandur-stripe