Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
AO3-4987 Add POST pagination support for admin email bulk search #3175
Conversation
sarken
added
the
Awaiting review
label
Nov 20, 2017
ariana-paris
approved these changes
Nov 20, 2017
I happen to be working on a different issue on this page so I added in your changes and I can confirm that they work - the pagination works correctly and the list of emails isn't removed.
ariana-paris
added
Reviewed: Ready to Merge
Deferred or Superseded
and removed
Awaiting review
Reviewed: Ready to Merge
labels
Nov 20, 2017
ariana-paris
requested changes
Nov 20, 2017
Hmm, actually, I was a bit too quick to cry victory. It has come back to me why I had to make this a POST action in the first place. The reason I added this bulk search was to enable Open Doors to search all the emails addresses for an import; this means at least hundreds, if not thousands of email addresses. A GET request can't handle that many as the URI becomes too large for the server and/or the browser, so this will need a different approach.
I'll consult with the committee but we'll probably have to close this, apndmaybe give up on paginating the results on the same page, unless you can think of a solution that can both accommodate a large number of email addresses and preserve pagination?
|
Let me consider this for a bit.
…
|
| @@ -0,0 +1,11 @@ | ||
| +module PostPaginationHelper |
| @@ -0,0 +1,33 @@ | ||
| +class PostLinkRenderer < WillPaginate::ActionView::LinkRenderer |
| +class PostLinkRenderer < WillPaginate::ActionView::LinkRenderer | ||
| + def previous_or_next_page(page, text, classname) | ||
| + if page | ||
| + submit(text, page,classname,:class => classname) |
| + if page | ||
| + submit(text, page,classname,:class => classname) | ||
| + else | ||
| + tag(:span, text, :class => classname + ' disabled') |
| + | ||
| + def page_number(page) | ||
| + if page == current_page | ||
| + tag(:em, page, :class => 'current') |
| + if page == current_page | ||
| + tag(:em, page, :class => 'current') | ||
| + else | ||
| + submit(page, page,nil,:rel => rel_value(page)) |
| + def submit(text, target, target_name, attributes = {}) | ||
| + string_attributes = attributes.inject('') do |attrs, pair| | ||
| + unless pair.last.nil? | ||
| + attrs << %( #{pair.first}="#{CGI::escapeHTML(pair.last.to_s)}") |
| + %(<input#{string_attributes} type="submit" name="page" value="#{text}">) | ||
| + else | ||
| + %(<input type="hidden" name="#{target_name}_value" value="#{target}">) + | ||
| + %(<input#{string_attributes} type="submit" name="#{target_name}" value="#{text}">) |
| +class PostLinkRenderer < WillPaginate::ActionView::LinkRenderer | ||
| + def previous_or_next_page(page, text, classname) | ||
| + if page | ||
| + submit(text, page, classname,class: classname) |
| + end | ||
| + | ||
| + def submit(text, target, target_name, attributes = {}) | ||
| + string_attributes = attributes.inject(''.dup) do |attrs, pair| |
| +class PostLinkRenderer < WillPaginate::ActionView::LinkRenderer | ||
| + def previous_or_next_page(page, text, classname) | ||
| + if page | ||
| + submit(text, page, classname,class: classname) |
| + end | ||
| + | ||
| + def submit(text, target, target_name, attributes = {}) | ||
| + string_attributes = attributes.inject(''.dup) do |attrs, pair| |
|
I think I managed to create a decently designed post paginator. Mind taking a look? |
sarken
added
the
Coder has actioned review
label
Nov 22, 2017
|
I patched up my local Archive installation and I can confirm that your link renderer works correctly and allows the form to handle hundreds of emails while also correctly paginating the results. \o/ Just one thing, we already have a Also, I think it would make sense for both renderers to live in the same folder distinct from |
| + if page | ||
| + submit(text, page, classname, class: classname) | ||
| + else | ||
| + tag(:span, text, class: classname + ' disabled') |
| + | ||
| + def page_number(page) | ||
| + if page == current_page | ||
| + tag(:em, page, class: 'current') |
ariana-paris
Nov 25, 2017
Contributor
This is one of the differences with our current PaginationListLinkRenderer: the existing one renders spans instead of ems, so it would be nice if this one did too so that the post pagination looks the same as the pagination we have elsewhere on the site (but maybe have a look at our existing renderer and see if there are any other differences too)
ariana-paris
added
Reviewed: Needs Coder Action
and removed
Coder has actioned review
labels
Nov 25, 2017
ariana-paris
changed the title from
AO3-4987 Switch admin email bulk search to get so paginate will work
to
AO3-4987 Add POST pagination support for admin email bulk search
Nov 25, 2017
| + end | ||
| + | ||
| + def page_number(page, remote = nil) | ||
| + unless page == current_page |
hatal175 commentedNov 20, 2017
Issue
https://otwarchive.atlassian.net/browse/AO3-4987
Purpose
I've changed the form method of the bulk email search to get instead of post. The paginate works now. I would like a code review on that though. Not entirely sure why it was post in the first place - they went to the same function anyway.
Testing
Recreation steps in the bug report.
Credit
Tal Hayon
Please use he.