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

Is there a workaround for a model named "Page"? #1709

Closed
milkfarm opened this issue Jul 13, 2020 · 10 comments · Fixed by #1725
Closed

Is there a workaround for a model named "Page"? #1709

milkfarm opened this issue Jul 13, 2020 · 10 comments · Fixed by #1725
Labels
bug breakages in functionality that is implemented

Comments

@milkfarm
Copy link

I didn't find any documentation about reserved names for the administrate gem. Is there a workaround for a model named Page? When I create a standard dashboard (rails g administrate:dashboard Page), I encounter a variety of errors. For example, when trying to access the pagination links on the index view.

NoMethodError in Admin::PagesController#index
undefined method `fetch' for "2":String

def order
  @order ||= Administrate::Order.new(
    params.fetch(resource_name, {}).fetch(:order, "created_at"),
    params.fetch(resource_name, {}).fetch(:direction, "desc")
  )
end

The issue relates to the paging/sorting information being stored under the same param key as the resource.

Rails: 6.0.3.2
administrate: 0.13.0

@milkfarm milkfarm added the bug breakages in functionality that is implemented label Jul 13, 2020
@pablobm
Copy link
Collaborator

pablobm commented Jul 14, 2020

Uh, that's indeed a bug. I have put together some code that might help. Mind trying out branch master...pablobm:fix-page and tell us how it went? Thank you.

@milkfarm
Copy link
Author

The paging links for unsorted results now work. Clicking a table column header to sort records, however, generates the following error message:

NoMethodError in Admin::PagesController#index
undefined method `to_i' for #<ActionController::Parameters:0x00007fec239ddbd8> Did you mean? to_s to_h

Extracted source (around line #17):

15  def self.#{Kaminari.config.page_method_name}(num = nil)
16    per_page = max_per_page && (default_per_page > max_per_page) ? max_per_page : default_per_page
17    limit(per_page).offset(per_page * ((num = num.to_i - 1) < 0 ? 0 : num)).extending do
18      include Kaminari::ActiveRecordRelationMethods
19      include Kaminari::PageScopeMethods
20  end

I'm ok renaming my model if you don't think this can be remedied in an elegant manner. I'm also here to help if you'd like to continue investigating. In any regard, thanks!

@pablobm
Copy link
Collaborator

pablobm commented Jul 17, 2020

Thank you again for that diagnosis. I have pushed a new change to that branch. Would you have time to try it out?

The bug should definitely be fixed, and I don't expect it to be a particularly difficult fix.

@milkfarm
Copy link
Author

milkfarm commented Jul 18, 2020

You've squashed another part it -- well done. The only problem I see is the paging when the records are sorted. I don't encounter an exception but the behavior isn't correct. The paging links don't retain the sorting. I didn't look at the code but I imagine the page[:order] and page[:direction] are being overwritten by the simple page param that contains the page number.

Example:

  • I click the ID column header to sort the records. The resultant URL is:
    • http://localhost:3000/admin/pages?page%5Bdirection%5D=desc&page%5Border%5D=id
  • The paging links, however, are:
    • http://localhost:3000/admin/pages?page=2
    • http://localhost:3000/admin/pages?page=3
    • etc

Note: The page%5Bdirection%5D=desc&page%5Border%5D=id string is missing.

@pablobm
Copy link
Collaborator

pablobm commented Jul 23, 2020

Uh, that's strange. I'm not able to reproduce.

There's something odd about the URLs you show though: the query param should be called _page now, but you show it as page. Perhaps Spring is playing us, and the version of the code you are running is not quite correct? Would you be able to check again, running ./bin/spring stop before running the server, just in case?

@milkfarm
Copy link
Author

You're right, it's my setup. Sorry for the confusion. Thanks for all your help. 👏

@pablobm
Copy link
Collaborator

pablobm commented Jul 30, 2020

Actually, I'm reopening because the bug does still exist. I should create a PR from my branch above, which should fix it properly.

@pablobm pablobm reopened this Jul 30, 2020
nickcharlton pushed a commit that referenced this issue Jul 31, 2020
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".

Instead of a unit test, a feature spec is used as 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.

A new feature spec called pagination_spec.rb, bringing over 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.

Fixes #1709
@rohmanitsavirus
Copy link

rohmanitsavirus commented Apr 19, 2021

You're right, it's my setup. Sorry for the confusion. Thanks for all your help.

How to solve this issue?
What's wrong with your setup?
I get this error and get stuck

@rohmanitsavirus
Copy link

You're right, it's my setup. Sorry for the confusion. Thanks for all your help.

How to solve this issue?
What's wrong with your setup?
I get this error and get stuck

I updated the administrate version to 0.15 and fix this error.

@pablobm
Copy link
Collaborator

pablobm commented Apr 20, 2023

If anyone else is having an issue with this, see the following for a potential solution: #2359 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug breakages in functionality that is implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants