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

Fix pagination of "Page" models #1725

Merged
merged 3 commits into from
Jul 31, 2020

Conversation

pablobm
Copy link
Collaborator

@pablobm pablobm commented Jul 30, 2020

Fixes #1709

Using the param :page for top-level pagination (ie: index pages as opposed to has_many lists in show pages) conflicts with paginating resources whose type happens to be "Page".

I'd rather have written a unit spec for this, instead of a whole new dashboard for the example app. However the pagination code is all over the place making this quite tricky. Ideally, we should move all the pagination code into a single module that can be tested in isolation. This in turn is not easy, as Kaminari doesn't make it easy. I'm leaving that for another day.

I have created a new feature spec called pagination_spec.rb. Into this I have moved other pre-existing pagination examples, to help organise things, and also to take advantage of the helper method expect_to_appear_in_order in the new example.

pablobm added 3 commits July 30, 2020 13:38
Moved other pagination-related specs to new location, to better organise
specs as well as to share the helper `expect_to_appear_in_order`.
expect_to_appear_in_order("Page 1", "Page 2", "Page 3")

click_on "Next"
expect_to_appear_in_order("Page 4", "Page 5")
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

click_on "Title"
expect_to_appear_in_order("Page 1", "Page 2", "Page 3")

click_on "Next"
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.


visit admin_pages_path(per_page: 3)
click_on "Title"
expect_to_appear_in_order("Page 1", "Page 2", "Page 3")
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

FactoryBot.create(:page, title: "Page 3")

visit admin_pages_path(per_page: 3)
click_on "Title"
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

FactoryBot.create(:page, title: "Page 4")
FactoryBot.create(:page, title: "Page 1")
FactoryBot.create(:page, title: "Page 5")
FactoryBot.create(:page, title: "Page 3")
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

query = "bar@baz.com"

visit admin_customers_path(search: query)
click_on "Name"
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

end

it "preserves search" do
query = "bar@baz.com"
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

expect_to_appear_in_order("unique name one", "unique name two")
end

it "preserves search" do
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

visit admin_customers_path
3.times { click_on "Name" }

expect_to_appear_in_order("unique name one", "unique name two")
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

create(:customer, name: "unique name two")

visit admin_customers_path
3.times { click_on "Name" }
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

Copy link
Member

@nickcharlton nickcharlton left a comment

Choose a reason for hiding this comment

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

Looks good!

@nickcharlton nickcharlton merged commit dd72fb2 into thoughtbot:master Jul 31, 2020
harley added a commit to CoderPush/administrate-base_controller that referenced this pull request Oct 3, 2020
See thoughtbot/administrate#1725

Relevant excerpt:

> Using the param :page for top-level pagination (ie: index pages as opposed to has_many lists in show pages) conflicts with paginating resources whose type happens to be "Page".
@stadia
Copy link

stadia commented Mar 22, 2021

@pablobm
Copy link
Collaborator Author

pablobm commented Mar 25, 2021

@stadia - Care to elaborate on what you mean by providing that link?

SleeplessByte pushed a commit to XPBytes/administrate-base_controller that referenced this pull request May 17, 2021
See thoughtbot/administrate#1725

Relevant excerpt:

> Using the param :page for top-level pagination (ie: index pages as opposed to has_many lists in show pages) conflicts with paginating resources whose type happens to be "Page".
G-Rath added a commit to ackama/nzsl-share that referenced this pull request Jun 8, 2021
@pablobm pablobm deleted the fix-page-pagination branch June 10, 2021 08:59
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.

Is there a workaround for a model named "Page"?
3 participants