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

Keep duplicated filterable list params when paginating #6061

Merged
merged 2 commits into from
Oct 2, 2020

Conversation

wpears
Copy link
Member

@wpears wpears commented Oct 1, 2020

Closes #2199 from over 4 years ago!

The problem here stemmed from the use of request.GET.items(), which returns only the last value when multiple params have the same key, by design.

See https://docs.djangoproject.com/en/3.1/ref/request-response/#django.http.QueryDict.items

Changing to .lists() and updating the logic a bit to handle the resulting lists stops params from being dropped.
Also, filtering out the 'page' param prevents the other bug here which was repeated page params in the querystring.

Closes #2199 from over 4 years ago!
The problem here stemmed from the use of request.GET.items(),
which returns only the last value when multiple params have the
same key, by design. See
https://docs.djangoproject.com/en/3.1/ref/request-response/#django.http.QueryDict.items
Changing to .lists() and updating the logic a bit to handle the
resulting lists stops params from being dropped.
Also, filtering out the 'page' param prevents the other bug here
which was repeated page params in the querystring.
@wpears wpears requested review from a team and anselmbradford October 1, 2020 23:30
Copy link
Member

@willbarton willbarton left a comment

Choose a reason for hiding this comment

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

This is fine... I'm kind of surprised that we're diving into request.GET in a template, though. That should really be done in a Django form that hands off the appropriate values. But so should a lot of other places where we're using request params.

@anselmbradford
Copy link
Member

@wpears

Do me a favor…

  • pull this branch and run it
  • go to http://localhost:8000/blog
  • expand the filter and check "At the CFPB" and "Director's notebook"
  • click "Apply filters'
  • scroll to the bottom of the results and enter "3" in the pagination component.
  • click "Go"

Do you get this error?

Screen Shot 2020-10-02 at 11 56 31 AM

@wpears
Copy link
Member Author

wpears commented Oct 2, 2020

@anselmbradford I did encounter this (once) but I don't think it's related to this PR. I'm looking into the source of the error.

@wpears
Copy link
Member Author

wpears commented Oct 2, 2020

Almost certainly related to #5984

@wpears
Copy link
Member Author

wpears commented Oct 2, 2020

Yeah confirmed, will merge this and PR a different fix (the error is local only)

@wpears wpears merged commit 15a445e into main Oct 2, 2020
@wpears wpears deleted the keep-pagination-params branch October 2, 2020 17:42
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.

Page number form in filterable list pagination molecule drops parameters
3 participants