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

Don't change the state of a 'request' entity using while the '#pagy_url_for' method for calendar pagination #688

Merged
merged 1 commit into from Apr 15, 2024

Conversation

PyrinAndrii
Copy link
Contributor

This PR fixes a bug that I faced trying to implement the calendar pagination.

Simple explanation:

While doing the calendar pagination I'm getting the next error expected :page >= 1; got "__pagy_page__"

The bug:

I think it was added to the pagy_url_for method in this commit, where we assign the request.Get object to the query_params variable and later change this object in the pagy_set_query_params method, so when after it we execute the pagy_url_for again the request.Get object is different and generates us broken URL.

How I'm reproducing it in my Rails app

  • add gem 'pagy', '~> 8.1', '>= 8.1.2' to my Gemfile (Rails project)
  • include Pagy::Frontend in ApplicationHelper
  • include Pagy::Backend in my controller
  • add @calendar, @pagy, @meals = pagy_calendar(collection, year: { }, month: { }, day: { }) in my action (e.g. index)
  • add several pagy_nav... in your view. For example <%== pagy_nav(@calendar[:year]) %> & <%== pagy_nav(@calendar[:month]) %>

Check the bug in the playground

The bug is reproducible via Pagy Playground. Look at the images below as the proof:

  1. Check generated links before the fix
  2. Check generated links after the fix
pagy calendar before fix pagy calendar after fix

@ddnexus ddnexus changed the base branch from master to dev April 15, 2024 23:40
@ddnexus ddnexus merged commit 56cb7f7 into ddnexus:dev Apr 15, 2024
6 checks passed
@ddnexus
Copy link
Owner

ddnexus commented Apr 15, 2024

🙏🏻 Thank you for the bug report and fix! Brilliant help from Lviv! 🇺🇦

Sorry for the bug. It originated from 2 things:

  1. The query_params started from an assignation like request.GET.merge..., so it was set as a new object, and the commit changed it, forgetting to clone it first
  2. We are missing tests for that, because the tests are currently passing 🤦 (and I don't run all the playground apps manually each time)

I am going to release the fix today, and try to add the tests ASAP.
Thank you!

@PyrinAndrii
Copy link
Contributor Author

No problem, happy to help 👍.
Thank you for such a handy gem.

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.

None yet

2 participants