https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 124941 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=124941&action=edit Bug 29043: Don't load items unless a patron has been chosen on request.pl This page wraps the item processing into an 'if( $patron )' conditional to boost performance before a patron has been located. To test: 1 - Browse to a bib in the staff client 2 - Click on the holds tab 3 - Note loading time 4 - Add 500 items to bib 5 - Note loading time - it takes longer 6 - Note items are not displayed 7 - Apply patch 8 - Reload page - is much faster 9 - Find/choose a patron 10 - Confirm items are loaded correctly -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joonas.kylmala@iki.fi --- Comment #3 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- I think this makes the code harder to understand by adding one more indentation layer to the giant loop. Could I propose that in this outer loop "foreach my $biblionumber (@biblionumbers) {" all the non-patron requiring code comes first and calls next unless we have a $patron? This way we we are not adding in all the different places calls to if ($patron) but only in one place. This would help also to make the multi-holds work for clubs – bug 29117. If anyhow other people think it is okay to go with the patch here then "my $items_any_available;" and the comment above it should be inside the loop as well, and probably the outer loop should also actually be in this if condition. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Items are prcoessed but not |Items are processed but not |displayed on request.pl |displayed on request.pl |before a patron is selected |before a patron is selected -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 --- Comment #5 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 127731 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127731&action=edit Bug 29043: Don't load items unless a p[atron has been chosen on request.pl This patch moves the code that we sh9ould run if there is no patron to the head of the biblio loop. If we have no patron we execute this and go to the next loop. A few variables that were being passed to the template in the loop are moved outside - no reason to overwrite each loop To test: 1 - Browse to a bib in the staff client 2 - Click on the holds tab 3 - Note loading time 4 - Add 500 items to bib 5 - Note loading time - it takes longer 6 - Note items are not displayed 7 - Apply patch 8 - Reload page - is much faster 9 - Find/choose a patron 10 - Confirm items are loaded correctly -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #7 from Fridolin Somers <fridolin.somers@biblibre.com> --- Hi, There is a small typo in commit message : Don't load items unless a p[atron Causing previous patch to not be obsolete. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Samu Heiskanen <samu.heiskanen@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |samu.heiskanen@hypernova.fi --- Comment #8 from Samu Heiskanen <samu.heiskanen@hypernova.fi> --- Load time is much faster but the title has changed to: Place a hold on No title -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 --- Comment #10 from Samu Heiskanen <samu.heiskanen@hypernova.fi> --- If you move the line: 323 my $biblio = Koha::Biblios->find( $biblionumber ); just before the foreach loop and add the line: biblio => $biblio, to: 738 # display infos 739 $template->param( 740 reserveloop => 1, 741 itemdata_enumchron => $itemdata_enumchron, 742 itemdata_ccode => $itemdata_ccode, 743 date => $date, 744 biblionumber => $biblionumber, 745 findborrower => $findborrower, 746 holdsview => 1, 747 C4::Search::enabled_staff_search_views, 748 ); like: # display infos $template->param( reserveloop => 1, itemdata_enumchron => $itemdata_enumchron, itemdata_ccode => $itemdata_ccode, date => $date, biblionumber => $biblionumber, findborrower => $findborrower, biblio => $biblio, holdsview => 1, C4::Search::enabled_staff_search_views, ); The patch works. What is your opinion, have I got it done correcly? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lari.taskula@hypernova.fi --- Comment #12 from Lari Taskula <lari.taskula@hypernova.fi> --- Hi Samu and welcome, I can confirm the "No title" bug from Nick's patches. I tested Samu's patch and noticed that your patch is introducing a new bug - batch placement of holds gets broken, follow this plan to replicate: 0. Have at least two bibliographic records in your Koha installation 1. In staff client, perform a search that returns at least two results 2. On search result list there are checkboxes, check them for two or more results 3. Click "Place hold" button 4. Observe the following error: "Cannot place hold: one or more records don't exist." This is because at line 326 (with your patch included) we are checking if any of the biblios in batch request is missing. The loop that starts at line 321 is handling each biblio of the batch request individually and each iteration requires that specific biblio to be loaded with "my $biblio = Koha::Biblios->find( $biblionumber );" so that required steps can be performed on it. Also, patch/commit title should be "Bug 29043: subject". See https://wiki.koha-community.org/wiki/Commit_messages#Subject_line -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Samu Heiskanen <samu.heiskanen@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128309|0 |1 is obsolete| | --- Comment #14 from Samu Heiskanen <samu.heiskanen@hypernova.fi> --- Created attachment 128336 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128336&action=edit Bug 29043: Remaining bug is the incorrect title "Place a hold on No title" This version takes account of the batch-mode. I only just add the line: $template->param( biblio => $biblio ) if scalar @biblionumbers == 1; inside the foreach loop. All my other patch-fixes are obsolete. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Samu Heiskanen <samu.heiskanen@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128254|0 |1 is obsolete| | Attachment #128337|0 |1 is obsolete| | --- Comment #16 from Samu Heiskanen <samu.heiskanen@hypernova.fi> --- Created attachment 128338 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128338&action=edit Bug 29043: Remaining bug is the incorrect title "Place a hold on No title" This version takes account of the batch-mode. I only just add the line: $template->param( biblio => $biblio ) if scalar @biblionumbers == 1; inside the foreach loop. All my other patch-fixes are obsolete. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Samu Heiskanen <samu.heiskanen@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128339|0 |1 is obsolete| | --- Comment #18 from Samu Heiskanen <samu.heiskanen@hypernova.fi> --- Created attachment 128340 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128340&action=edit Bug 29043: Remaining bug is the incorrect title "Place a hold on No title" This version takes account of the batch-mode. I only just add the line: $template->param( biblio => $biblio ) if scalar @biblionumbers == 1; inside the foreach loop. All my other patch-fixes are obsolete. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 --- Comment #19 from Samu Heiskanen <samu.heiskanen@hypernova.fi> --- Sorry, I got a little carried away with this bug. But original assignee, Nick please continue from here. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 --- Comment #21 from Samu Heiskanen <samu.heiskanen@hypernova.fi> --- Ok, I'll do that. Thx Katrin! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Samu Heiskanen <samu.heiskanen@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127732|0 |1 is obsolete| | --- Comment #23 from Samu Heiskanen <samu.heiskanen@hypernova.fi> --- Created attachment 128422 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128422&action=edit Bug 29043: Prune dead code Remove sort_borrowerlist as it was not used Remove "time" for form name I don't see nay reason we needed this Remove messagetransfert from template Never passed in code Remove totalcount variable Assigned and never referenced Remove 'alreadyres'/'alreadyreserved' from template All references in coide were in template - never set or passed Signed-off-by: Samu Heiskanen <samu.heiskanen@hypernova.fi> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 --- Comment #26 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 128475 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128475&action=edit Bug 29043: Prune dead code Remove sort_borrowerlist as it was not used Remove "time" for form name I don't see nay reason we needed this Remove messagetransfert from template Never passed in code Remove totalcount variable Assigned and never referenced Remove 'alreadyres'/'alreadyreserved' from template All references in coide were in template - never set or passed Signed-off-by: Samu Heiskanen <samu.heiskanen@hypernova.fi> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply --- Comment #28 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Sorry Nick, can you please rebase? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 29043: Don't load items unless a p[atron has been chosen on request.pl Using index info to reconstruct a base tree... M reserve/request.pl Falling back to patching base and 3-way merge... Auto-merging reserve/request.pl CONFLICT (content): Merge conflict in reserve/request.pl error: Failed to merge in the changes. Patch failed at 0001 Bug 29043: Don't load items unless a p[atron has been chosen on request.pl The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-29043-Dont-load-items-unless-a-patron-has-been-vtn8py.patch -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128474|0 |1 is obsolete| | Attachment #128475|0 |1 is obsolete| | Attachment #128476|0 |1 is obsolete| | --- Comment #29 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 128971 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128971&action=edit Bug 29043: Don't load items unless a p[atron has been chosen on request.pl This patch moves the code that we sh9ould run if there is no patron to the head of the biblio loop. If we have no patron we execute this and go to the next loop. A few variables that were being passed to the template in the loop are moved outside - no reason to overwrite each loop To test: 1 - Browse to a bib in the staff client 2 - Click on the holds tab 3 - Note loading time 4 - Add 500 items to bib 5 - Note loading time - it takes longer 6 - Note items are not displayed 7 - Apply patch 8 - Reload page - is much faster 9 - Find/choose a patron 10 - Confirm items are loaded correctly Signed-off-by: Samu Heiskanen <samu.heiskanen@hypernova.fi> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 --- Comment #31 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 128973 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128973&action=edit Bug 29043: Remaining bug is the incorrect title "Place a hold on No title" This version takes account of the batch-mode. I only just add the line: $template->param( biblio => $biblio ) if scalar @biblionumbers == 1; inside the foreach loop. All my other patch-fixes are obsolete. Signed-off-by: Samu Heiskanen <samu.heiskanen@hypernova.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com --- Comment #33 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I am late here and I was not aware of these patches when I wrote bug 29660. I have tried to rebase bug 29660 on top of this but it's very tricky. This patch is suggesting to do the minimum required to not fetch the infos if we don't need to (no club or patron selected). Please read this patch with `git show -w HEAD` and you will notice there are only ~10 lines. It will help a lot if we can move forward with this version as I will be able to rebase easily bug 29660 on top of it. I haven't stressed the patch much but the simple use cases work so far. Will be back to it next week. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |29660 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29660 [Bug 29660] reserve/request.pl should not deal with biblioitem -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 --- Comment #37 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- @ reserve/request.pl:199 @ $template->param( multi_hold => $multi_hold, ); +# If we are coming from the search result and only 1 is selected +$biblionumber ||= $biblionumbers[0] unless $multi_hold; # If we have the borrowernumber because we've performed an action, then we # don't want to try to place another reserve. if ($borrowernumber_hold && !$action) { @ reserve/request.pl:297 @ $template->param( messageclub => $messageclub ); +# Load the hold list if +# - we are searching for a patron or club and found one +# - we are not searching for anything +if ( ( $findborrower && $borrowernumber_hold || $findclub && $club_hold ) + || ( !$findborrower && !$findclub ) ) +{ # FIXME launch another time GetMember perhaps until (Joubu: Why?) my $patron = Koha::Patrons->find( $borrowernumber_hold ); @ reserve/request.pl:771 @ foreach my $biblionumber (@biblionumbers) { $template->param( biblioloop => \@biblioloop ); $template->param( no_reserves_allowed => $no_reserves_allowed ); -$template->param( biblionumbers => join('/', @biblionumbers) ); $template->param( exceeded_maxreserves => $exceeded_maxreserves ); $template->param( exceeded_holds_per_record => $exceeded_holds_per_record ); $template->param( subscriptionsnumber => CountSubscriptionFromBiblionumber($biblionumber)); +} elsif ( ! $multi_hold ) { + my $biblio = Koha::Biblios->find( $biblionumber ); + $template->param( biblio => $biblio ); +} +if ( $multi_hold ) { + $template->param( biblionumbers => join('/', @biblionumbers) ); +} else { + $template->param( biblionumber => $biblionumber || $biblionumbers[0] ); +} # pass the userenv branch if no pickup location selected $template->param( pickup => $pickup || C4::Context->userenv->{branch} ); -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 --- Comment #39 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 129530 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129530&action=edit Bug 29043: Don't fetch items if when are not on the "Place a hold on" form -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 --- Comment #41 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- @ reserve/request.pl:440 @ if ( ( $findborrower && $borrowernumber_hold || $findclub && $club_hold ) } }; + if ( $club_hold or $borrowernumber_hold ) { my @bibitemloop; my @available_itemtypes; @ reserve/request.pl:673 @ if ( ( $findborrower && $borrowernumber_hold || $findclub && $club_hold ) } @available_itemtypes = uniq( @available_itemtypes ); - $template->param( available_itemtypes => \@available_itemtypes ); + $template->param( + bibitemloop => \@bibitemloop, + available_itemtypes => \@available_itemtypes + ); + } # existingreserves building my @reserveloop; @ reserve/request.pl:744 @ if ( ( $findborrower && $borrowernumber_hold || $findclub && $club_hold ) # display infos $template->param( - bibitemloop => \@bibitemloop, itemdata_enumchron => $itemdata_enumchron, itemdata_ccode => $itemdata_ccode, date => $date, -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |29058 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29058 [Bug 29058] Add option to not load existing holds table automatically -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 --- Comment #43 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 129821 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129821&action=edit Bug 29043: Don't fetch items if when are not on the 'Place a hold on' form -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129820|0 |1 is obsolete| | Attachment #129821|0 |1 is obsolete| | --- Comment #44 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 129915 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129915&action=edit Bug 29043: Don't fetch biblios info unless a patron is picked If we haven't picked a patron or a club yet we don't need to fetch biblios, items and holdability infos Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|29660 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29660 [Bug 29660] reserve/request.pl should not deal with biblioitem -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129916|0 |1 is obsolete| | --- Comment #47 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 129950 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129950&action=edit Bug 29043: Don't fetch items if when are not on the 'Place a hold on' form Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |22.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com Version(s)|22.05.00 |22.05.00,21.11.03 released in| | Status|Pushed to master |Pushed to stable --- Comment #50 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to 21.11.x for 21.11.03 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29043 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net Status|Pushed to oldstable |RESOLVED Resolution|--- |FIXED --- Comment #52 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed. -- You are receiving this mail because: You are watching all bug changes.