Description
Nick Clemens (kidclamp)
2016-02-12 16:22:30 UTC
Hi Nick, It's because this patron list contains all patrons matching the search. It'd be quite easy to make it use DT server-side processing, but it's more a new enhancement than a bug. Marking this as a bug due to its potential to crash servers. Created attachment 125224 [details] [review] Bug 15812: Use patron search in place of circ search The 'Checkout' search hijacks some of the DataTables searching code used for 'Search patrons' Rather than try to implement the search again on another page, we can simply send the user to the patron search if the cardnumber is not found Additionally, this patch adds a 'Check out' button to the patron search results to allow going to checkotus directly To test: 1 - Apply patch 2 - Perform a 'Checkout' search from the header 3 - Note that: For a cardnumber, you are redirected directly to checkouts page for the borrower For a search with one result, you are redirected directly to the checkout page for the borrower For a search with many results, you are redirected to the patron search results and there is a 'Checkout' button under the cardnumber 4 - Confirm circulation page works as expected (i.e. checkout to a patron) Note: while changes to circulation.tt appear large, it is mostly a whitespace change to fix indentation after removal of conditional on 'borrowers' Created attachment 125234 [details] [review] Bug 15812: Use patron search in place of circ search The 'Checkout' search hijacks some of the DataTables searching code used for 'Search patrons' Rather than try to implement the search again on another page, we can simply send the user to the patron search if the cardnumber is not found Additionally, this patch adds a 'Check out' button to the patron search results to allow going to checkotus directly To test: 1 - Apply patch 2 - Perform a 'Checkout' search from the header 3 - Note that: For a cardnumber, you are redirected directly to checkouts page for the borrower For a search with one result, you are redirected directly to the checkout page for the borrower For a search with many results, you are redirected to the patron search results and there is a 'Checkout' button under the cardnumber 4 - Confirm circulation page works as expected (i.e. checkout to a patron) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Nice change, I test it.
> For a search with many results, you are redirected to the patron search results and there is a 'Checkout' button under the cardnumber
I see this also when searching via 'Seach patrons' form.
And looks like permission check is needed there right ?
In my opinion this button should be with 'Edit' in actions column.
When searching for patron one can assume cardnumber is unknown so button being next to it is not really useful.
Note than cardnumber column may be hidden.
Patron may be identified by name so when clicking on it we may go to circulation page.
Best regards,
Created attachment 125413 [details] [review] Bug 15812: Use patron search in place of circ search The 'Checkout' search hijacks some of the DataTables searching code used for 'Search patrons' Rather than try to implement the search again on another page, we can simply send the user to the patron search if the cardnumber is not found Additionally, this patch adds a 'Check out' button to the patron search results to allow going to checkotus directly To test: 1 - Apply patch 2 - Perform a 'Checkout' search from the header 3 - Note that: For a cardnumber, you are redirected directly to checkouts page for the borrower For a search with one result, you are redirected directly to the checkout page for the borrower For a search with many results, you are redirected to the patron search results and there is a 'Checkout' button under the cardnumber 4 - Confirm circulation page works as expected (i.e. checkout to a patron) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: George Williams <george@nekls.org> Tested on the biblibre sandboxes and the test plan goes as expected. Nice fix. Sorry guys, patch doesn't apply right now.. could we get a rebase please? Created attachment 125489 [details] [review] Bug 15812: Use patron search in place of circ search The 'Checkout' search hijacks some of the DataTables searching code used for 'Search patrons' Rather than try to implement the search again on another page, we can simply send the user to the patron search if the cardnumber is not found Additionally, this patch adds a 'Check out' button to the patron search results to allow going to checkotus directly To test: 1 - Apply patch 2 - Perform a 'Checkout' search from the header 3 - Note that: For a cardnumber, you are redirected directly to checkouts page for the borrower For a search with one result, you are redirected directly to the checkout page for the borrower For a search with many results, you are redirected to the patron search results and there is a 'Checkout' button under the cardnumber 4 - Confirm circulation page works as expected (i.e. checkout to a patron) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: George Williams <george@nekls.org> Created attachment 125490 [details] [review] Bug 15812: (follow-up) Add permission check for 'Check out' button Super, permission is OK now. (In reply to Fridolin Somers from comment #6) > Note than cardnumber column may be hidden. > Isn't that a problem ? Also for consistency, everywhere in tables buttons are in actions column. But it is minor. It is a great change. This is a lovely improvement... it simplifies and reduces code and adds direct access to checkouts from patron results page. I'd love to see a follow-on bug to do the same for holds searching and then we can ditch some more code :). However, I do think Fridolin raises a good point about the 'cardnumber' field being one you can hide which could cause confusion. The button styling isn't complete and I agree it's a little jarring to see it outside of the 'actions' field. I'd be tempted move the button to the actions column and perhaps update the cardnumber text to be a link directly to checkout (with a tooltip preferably). If you think that would fulfil all your customers requirements, I'd be happy to make that change in a follow-up Nick? Created attachment 125580 [details] [review] Bug 15812: Use patron search in place of circ search The 'Checkout' search hijacks some of the DataTables searching code used for 'Search patrons' Rather than try to implement the search again on another page, we can simply send the user to the patron search if the cardnumber is not found Additionally, this patch adds a 'Check out' button to the patron search results to allow going to checkotus directly To test: 1 - Apply patch 2 - Perform a 'Checkout' search from the header 3 - Note that: For a cardnumber, you are redirected directly to checkouts page for the borrower For a search with one result, you are redirected directly to the checkout page for the borrower For a search with many results, you are redirected to the patron search results and there is a 'Checkout' button under the cardnumber 4 - Confirm circulation page works as expected (i.e. checkout to a patron) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: George Williams <george@nekls.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 125582 [details] [review] Bug 15812: (follow-up) Add permission check for 'Check out' button Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 125583 [details] [review] Bug 15812: (follow-up) Tweaks to patron results display Move the button into the actions column and make 'cardnumber' a link to checkout, with a tooltip. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Code speaks.. let me know what you think of that final followup. Otherwise, I'm happy to PQA :) Whaou for me it is awesome. Thanks a lot Martin ;) Created attachment 125597 [details] [review] Bug 15812: (follow-up) Tweaks to patron results display Move the button into the actions column and make 'cardnumber' a link to checkout, with a tooltip. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Last little tweak made the action button prettier ;) I like the change to the buttons but I'm sorry to see Bug 28081 undone. I'd like to argue that the link is redundant since the button has been added. Good catch Owen.. I wasn't aware of the label stuff.. I thought it had been added here. Created attachment 125604 [details] [review] Bug 15812: (follow-up) Enable click row to select This patch adds handling to allow clicking anywhere in the table row to select/deselect the patron Created attachment 125621 [details] [review] Bug 15812: (follow-up) Enable click cell to select This patch adds handling to allow clicking anywhere in the table cell to select/deselect the patron Created attachment 125622 [details] [review] Bug 15812: (follow-up) Enable click cell to select This patch adds handling to allow clicking anywhere in the table cell to select/deselect the patron OK, I think we're all done now. I restored the label, but made it a visually hidden one inside the check column for accessibility and I made the whole table cell clickable for the checkbox to restore the larger click handling area. Created attachment 125623 [details] [review] Bug 15812: (follow-up) Enable click cell to select This patch adds handling to allow clicking anywhere in the table cell to select/deselect the patron Created attachment 125624 [details] [review] Bug 15812: (follow-up) Enable click cell to select This patch adds handling to allow clicking anywhere in the table cell to select/deselect the patron Signed-off-by: Owen Leonard <oleonard@myacpl.org> (In reply to Martin Renvoize from comment #26) > OK, I think we're all done now. > > I restored the label, but made it a visually hidden one inside the check > column for accessibility and I made the whole table cell clickable for the > checkbox to restore the larger click handling area. Hmm, I'm not able to do this still even with this patch (firefox esr on debian 11). Could you double-check it works and if it does could you explain me how to test more specifically. Created attachment 125957 [details] [review] Bug 15812: Use patron search in place of circ search The 'Checkout' search hijacks some of the DataTables searching code used for 'Search patrons' Rather than try to implement the search again on another page, we can simply send the user to the patron search if the cardnumber is not found Additionally, this patch adds a 'Check out' button to the patron search results to allow going to checkotus directly To test: 1 - Apply patch 2 - Perform a 'Checkout' search from the header 3 - Note that: For a cardnumber, you are redirected directly to checkouts page for the borrower For a search with one result, you are redirected directly to the checkout page for the borrower For a search with many results, you are redirected to the patron search results and there is a 'Checkout' button under the cardnumber 4 - Confirm circulation page works as expected (i.e. checkout to a patron) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: George Williams <george@nekls.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 125958 [details] [review] Bug 15812: (follow-up) Add permission check for 'Check out' button Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 125959 [details] [review] Bug 15812: (follow-up) Tweaks to patron results display Move the button into the actions column and make 'cardnumber' a link to checkout, with a tooltip. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 125960 [details] [review] Bug 15812: (follow-up) Enable click cell to select This patch adds handling to allow clicking anywhere in the table cell to select/deselect the patron Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> (In reply to Joonas Kylmälä from comment #29) > (In reply to Martin Renvoize from comment #26) > > OK, I think we're all done now. > > > > I restored the label, but made it a visually hidden one inside the check > > column for accessibility and I made the whole table cell clickable for the > > checkbox to restore the larger click handling area. > > Hmm, I'm not able to do this still even with this patch (firefox esr on > debian 11). Could you double-check it works and if it does could you explain > me how to test more specifically. Works for me in Firefox - I misread on my first test and thought it was supposed to be the whole row, but it is just clicking inside the first cell, but outside the checkbox will still check the box Tested and signed off Martin's two attachments, so moving to PQA Is there a good reason (apart from laziness) to redirect to member.pl from circulation.pl? 229 print $query->redirect( "/cgi-bin/koha/members/member.pl?quicksearch=1&circsearch=1&searchmember=" . $findborrower ); (In reply to Jonathan Druart from comment #35) > Is there a good reason (apart from laziness) to redirect to member.pl from > circulation.pl? > > 229 print $query->redirect( > "/cgi-bin/koha/members/member.pl?quicksearch=1&circsearch=1&searchmember=" . > $findborrower ); The idea was to avoid a redirect when a single patron was searched for using a cardnumber. If we don't need a search we can proceed with circulation directly, if we need to search, then we redirect to search I won't block it more, but I don't think it's the best implementation. I do like the "Check out" on the patron list. However I don't understand why you didn't go the usual way, which is using common/patron_search.tt and svc/members/search. It was a bit more code but we would have avoided the double redirect when a single patron matches the search (and it's not a cardnumber). Which is certainly the most common use case (after using the cardnumber). Pushed to master for 21.11, thanks to everybody involved! In members/tables/members_results.tt: title='View' This is bad, it breaks translated template. Strings must be surrounded by double quote. However here we will have to escape it with \ and the string won't be picked by the translate script. See bug 29261. (In reply to Jonathan Druart from comment #39) > In members/tables/members_results.tt: > > title='View' > > This is bad, it breaks translated template. Strings must be surrounded by > double quote. > However here we will have to escape it with \ and the string won't be picked > by the translate script. > > See bug 29261. Damn, that's my mistake in a follow-up.. apologies. Hmm, not sure what the resolution is. (In reply to Jonathan Druart from comment #39) > However here we will have to escape it with \ and the string won't be picked > by the translate script. This is not correct, it actually works. (In reply to Jonathan Druart from comment #37) > I won't block it more, but I don't think it's the best implementation. > I do like the "Check out" on the patron list. However I don't understand why > you didn't go the usual way, which is using common/patron_search.tt and > svc/members/search. > > It was a bit more code but we would have avoided the double redirect when a > single patron matches the search (and it's not a cardnumber). Which is > certainly the most common use case (after using the cardnumber). Nick? (In reply to Jonathan Druart from comment #42) > (In reply to Jonathan Druart from comment #37) > > I won't block it more, but I don't think it's the best implementation. > > I do like the "Check out" on the patron list. However I don't understand why > > you didn't go the usual way, which is using common/patron_search.tt and > > svc/members/search. > > > > It was a bit more code but we would have avoided the double redirect when a > > single patron matches the search (and it's not a cardnumber). Which is > > certainly the most common use case (after using the cardnumber). > > Nick? I am not against this idea - this seemed a straightforward path and eliminated a problem so I went this way. circ-patron-search-results.inc is now only used in one last place and I think it could be deprecated in favour of the approach here.. I'll post a bug for that. (In reply to Jonathan Druart from comment #39) > In members/tables/members_results.tt: > > title='View' > > This is bad, it breaks translated template. Strings must be surrounded by > double quote. > However here we will have to escape it with \ and the string won't be picked > by the translate script. > > See bug 29261. Same for title='Checkout'! will fix on bug 29261 as well Comment on attachment 125960 [details] [review] Bug 15812: (follow-up) Enable click cell to select Review of attachment 125960 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/tables/members_results.tt @@ +13,4 @@ > { > [% IF CAN_user_borrowers_edit_borrowers || CAN_user_tools_manage_patron_lists %] > "dt_borrowernumber": > + "<label for='check[% data.borrowernumber | html %]' class='content_hidden'>Select patron</label><input type='checkbox' id='check[% data.borrowernumber | html %]' class='selection' name='borrowernumber' value='[% data.borrowernumber | html %]' />", This is a bit weird, we are adding a new string "Select patron" that will never be displayed (but is translatable however). It's for screen readers I believe... FYI this has caused a regression as noted in bug 30230 but it doesn't seem easily resolvable especially since this change has been around too long to easily revert... |