Bug 15812 - Checkout search with too many results (single character search) causes poor performance or timeout
Summary: Checkout search with too many results (single character search) causes poor ...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P3 normal (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 29411 30230 29261 29430 29463
  Show dependency treegraph
 
Reported: 2016-02-12 16:22 UTC by Nick Clemens
Modified: 2022-06-25 18:39 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch replaces the special case patron results page from circulation searches and instead redirects to the standard patron search results page. To enable quick onward navigation to checkout, we add a link to the cardnumber field and a button to the actions column.
Version(s) released in:
21.11.00


Attachments
Bug 15812: Use patron search in place of circ search (58.69 KB, patch)
2021-09-24 00:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15812: Use patron search in place of circ search (58.76 KB, patch)
2021-09-24 10:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15812: Use patron search in place of circ search (58.80 KB, patch)
2021-09-28 14:40 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 15812: Use patron search in place of circ search (58.20 KB, patch)
2021-09-30 11:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15812: (follow-up) Add permission check for 'Check out' button (2.21 KB, patch)
2021-09-30 11:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15812: Use patron search in place of circ search (58.26 KB, patch)
2021-10-01 09:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15812: (follow-up) Add permission check for 'Check out' button (2.27 KB, patch)
2021-10-01 09:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15812: (follow-up) Tweaks to patron results display (4.11 KB, patch)
2021-10-01 09:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15812: (follow-up) Tweaks to patron results display (4.14 KB, patch)
2021-10-01 09:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15812: (follow-up) Enable click row to select (1.20 KB, patch)
2021-10-01 11:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15812: (follow-up) Enable click cell to select (1.20 KB, patch)
2021-10-01 11:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15812: (follow-up) Enable click cell to select (2.56 KB, patch)
2021-10-01 11:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15812: (follow-up) Enable click cell to select (2.56 KB, patch)
2021-10-01 12:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15812: (follow-up) Enable click cell to select (2.61 KB, patch)
2021-10-01 12:04 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 15812: Use patron search in place of circ search (57.48 KB, patch)
2021-10-08 13:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15812: (follow-up) Add permission check for 'Check out' button (2.27 KB, patch)
2021-10-08 13:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15812: (follow-up) Tweaks to patron results display (4.20 KB, patch)
2021-10-08 13:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15812: (follow-up) Enable click cell to select (2.67 KB, patch)
2021-10-08 13:16 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2016-02-12 16:22:30 UTC
On a system with many patrons a checkout search for a single character (2,X,J) can cause a slow search or timeout from the large number of results returned.

Patron search has no problem with the same searches
Comment 1 Jonathan Druart 2016-03-15 14:49:35 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.
Comment 2 Jesse Weaver 2016-03-23 17:41:00 UTC
Marking this as a bug due to its potential to crash servers.
Comment 3 Nick Clemens 2021-09-24 00:11:09 UTC Comment hidden (obsolete)
Comment 4 Nick Clemens 2021-09-24 00:13:01 UTC
Note: while changes to circulation.tt appear large, it is mostly a whitespace change to fix indentation after removal of conditional on 'borrowers'
Comment 5 Kyle M Hall 2021-09-24 10:58:01 UTC Comment hidden (obsolete)
Comment 6 Fridolin Somers 2021-09-24 20:50:10 UTC
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,
Comment 7 sandboxes@biblibre.com 2021-09-28 14:40:45 UTC Comment hidden (obsolete)
Comment 8 George Williams (NEKLS) 2021-09-28 14:43:13 UTC
Tested on the biblibre sandboxes and the test plan goes as expected.  Nice fix.
Comment 9 Martin Renvoize 2021-09-30 10:04:24 UTC
Sorry guys, patch doesn't apply right now.. could we get a rebase please?
Comment 10 Nick Clemens 2021-09-30 11:32:40 UTC Comment hidden (obsolete)
Comment 11 Nick Clemens 2021-09-30 11:32:44 UTC Comment hidden (obsolete)
Comment 12 Fridolin Somers 2021-09-30 21:06:52 UTC
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.
Comment 13 Martin Renvoize 2021-10-01 08:40:28 UTC
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?
Comment 14 Martin Renvoize 2021-10-01 09:00:08 UTC
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>
Comment 15 Martin Renvoize 2021-10-01 09:00:12 UTC
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>
Comment 16 Martin Renvoize 2021-10-01 09:00:16 UTC
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>
Comment 17 Martin Renvoize 2021-10-01 09:00:56 UTC
Code speaks.. let me know what you think of that final followup.  Otherwise, I'm happy to PQA :)
Comment 18 Fridolin Somers 2021-10-01 09:10:17 UTC
Whaou for me it is awesome.
Thanks a lot Martin ;)
Comment 19 Martin Renvoize 2021-10-01 09:24:48 UTC
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>
Comment 20 Martin Renvoize 2021-10-01 09:27:16 UTC
Last little tweak made the action button prettier ;)
Comment 21 Owen Leonard 2021-10-01 10:42:29 UTC
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.
Comment 22 Martin Renvoize 2021-10-01 10:46:28 UTC
Good catch Owen.. I wasn't aware of the label stuff.. I thought it had been added here.
Comment 23 Martin Renvoize 2021-10-01 11:26:58 UTC Comment hidden (obsolete)
Comment 24 Martin Renvoize 2021-10-01 11:39:47 UTC Comment hidden (obsolete)
Comment 25 Martin Renvoize 2021-10-01 11:47:04 UTC Comment hidden (obsolete)
Comment 26 Martin Renvoize 2021-10-01 11:49:42 UTC
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.
Comment 27 Martin Renvoize 2021-10-01 12:00:11 UTC Comment hidden (obsolete)
Comment 28 Owen Leonard 2021-10-01 12:04:03 UTC
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>
Comment 29 Joonas Kylmälä 2021-10-03 10:06:02 UTC
(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.
Comment 30 Nick Clemens 2021-10-08 13:15:56 UTC
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>
Comment 31 Nick Clemens 2021-10-08 13:16:00 UTC
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>
Comment 32 Nick Clemens 2021-10-08 13:16:04 UTC
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>
Comment 33 Nick Clemens 2021-10-08 13:16:08 UTC
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>
Comment 34 Nick Clemens 2021-10-08 13:17:34 UTC
(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
Comment 35 Jonathan Druart 2021-10-11 14:45:17 UTC
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 );
Comment 36 Nick Clemens 2021-10-12 10:11:49 UTC
(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
Comment 37 Jonathan Druart 2021-10-14 10:02:16 UTC
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).
Comment 38 Jonathan Druart 2021-10-15 09:29:07 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 39 Jonathan Druart 2021-10-18 09:00:36 UTC
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.
Comment 40 Martin Renvoize 2021-10-18 09:03:53 UTC
(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.
Comment 41 Jonathan Druart 2021-10-18 09:40:22 UTC
(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.
Comment 42 Jonathan Druart 2021-10-22 12:31:49 UTC
(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?
Comment 43 Nick Clemens 2021-10-22 15:05:56 UTC
(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.
Comment 44 Martin Renvoize 2021-10-26 10:44:26 UTC
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.
Comment 45 Jonathan Druart 2021-10-28 15:19:25 UTC
(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 46 Jonathan Druart 2021-11-02 11:11:21 UTC
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).
Comment 47 Martin Renvoize 2021-11-02 11:31:44 UTC
It's for screen readers I believe...