Skip to content

Add ability to fetch next page with persisted query parameters#17

Merged
etagwerker merged 3 commits intofastruby:masterfrom
RoleModel:fetch-next-page-with-options
Nov 17, 2018
Merged

Add ability to fetch next page with persisted query parameters#17
etagwerker merged 3 commits intofastruby:masterfrom
RoleModel:fetch-next-page-with-options

Conversation

@dsul
Copy link
Copy Markdown
Contributor

@dsul dsul commented Oct 23, 2018

When requesting a new page of results (Not page 1), the query parameters are not persisted which leads to incorrect results. This PR intends to persist the initial query parameters that were correctly passed into the API request for the first page, along to the subsequent API requests for the rest of the pages of results.

@dsul dsul changed the title Ability to fetch next page with persisted query parameters Add ability to fetch next page with persisted query parameters Oct 23, 2018
@gap777
Copy link
Copy Markdown
Contributor

gap777 commented Oct 30, 2018

@etagwerker @benphelps @mscottford can we get some response to this? Seems like a few PRs have stalled.

@gap777
Copy link
Copy Markdown
Contributor

gap777 commented Nov 13, 2018

@etagwerker should we update this to master as the next PR to discuss?

@etagwerker
Copy link
Copy Markdown
Member

@gap777 Yes, that's a good idea. Thanks!

@gap777 gap777 force-pushed the fetch-next-page-with-options branch from 783b421 to 0964228 Compare November 15, 2018 14:41
@gap777 gap777 force-pushed the fetch-next-page-with-options branch from 0964228 to 930a55e Compare November 15, 2018 14:51
@gap777
Copy link
Copy Markdown
Contributor

gap777 commented Nov 15, 2018

@etagwerker Rebased onto your master, but getting a test failure in

1) Harvesting::Client#me returns the authenticated user
     Failure/Error: expect(user.first_name).to eq(account_first_name)

       expected: "<replace me with a test account>"
            got: "123"

       (compared using ==)
     # ./spec/harvesting/client_spec.rb:107:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:32:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:32:in `block (2 levels) in <top (required)>'

I'm suspecting that's just an environmental issue, though. Can you try on your machine, and if all is well, merge?

@etagwerker etagwerker merged commit 930a55e into fastruby:master Nov 17, 2018
@etagwerker
Copy link
Copy Markdown
Member

@gap777 Cool, thanks! I fixed that failing spec in this commit: a0a3733

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants