To recreate: 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
Created attachment 124941 [details] [review] 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
Created attachment 125060 [details] [review] 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 Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
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.
+ if ( $patron ) { + $items_any_available = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblioitem->{biblionumber}, patron => $patron }); + + foreach my $itemnumber ( @{ $itemnumbers_of_biblioitem{$biblioitemnumber} } ) { + my $item = $iteminfos_of->{$itemnumber}; + my $do_check; + if ( $patron ) { Double condition
Created attachment 127731 [details] [review] 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
Created attachment 127732 [details] [review] 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
Hi, There is a small typo in commit message : Don't load items unless a p[atron Causing previous patch to not be obsolete.
Load time is much faster but the title has changed to: Place a hold on No title
Hi Samu, thx for testing! If you find an issue it's ok to set status to failed QA to alert the deveveloper.
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?
Created attachment 128254 [details] [review] Here's the fixed patch for testing.
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
Created attachment 128309 [details] [review] Bug 29043: Remaining bug is the incorrect title "Place a hold on No title" I tried to fix this bug with my previous attempt, but introduced a new bug. I return the line: my $biblio = Koha::Biblios->find( $biblionumber ); to it's previous place and just copy it to the end of the foreach loop. The display infos for template will remain as they were in my first attempt. Hope this fix works properly.
Created attachment 128336 [details] [review] 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.
Created attachment 128337 [details] [review] 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.
Created attachment 128338 [details] [review] 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.
Created attachment 128339 [details] [review] 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.
Created attachment 128340 [details] [review] 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.
Sorry, I got a little carried away with this bug. But original assignee, Nick please continue from here.
Hi Samu, do you want to sign-off on the first 2 patches in combination with yours? Then we could ask Nick to sign-off on yours and move to Signed off.
Ok, I'll do that. Thx Katrin!
Created attachment 128421 [details] [review] 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>
Created attachment 128422 [details] [review] 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>
Created attachment 128423 [details] [review] 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>
Created attachment 128474 [details] [review] 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>
Created attachment 128475 [details] [review] 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>
Created attachment 128476 [details] [review] 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>
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
Created attachment 128971 [details] [review] 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>
Created attachment 128972 [details] [review] 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>
Created attachment 128973 [details] [review] 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>
Created attachment 129200 [details] [review] Bug 29043: [ALTERNATIVE PATCH] 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
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.
@ reserve/request.pl:201 @ $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:299 @ $template->param( messageclub => $messageclub ); +if ( $club_hold or $borrowernumber_hold ) { # FIXME launch another time GetMember perhaps until (Joubu: Why?) my $patron = Koha::Patrons->find( $borrowernumber_hold ); @ reserve/request.pl:692 @ foreach my $biblionumber (@biblionumbers) { push @biblioloop, \%biblioloopiter; } $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] ); +}
Nick, will this alternative patch answers your needs?
Created attachment 129498 [details] [review] 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
@ 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} );
Rebased and fixed a bug.
Created attachment 129530 [details] [review] Bug 29043: Don't fetch items if when are not on the "Place a hold on" form
Created attachment 129531 [details] [review] Bug 29043: Don't fetch items if when are not on the 'Place a hold on' form
@ 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,
Created attachment 129820 [details] [review] 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
Created attachment 129821 [details] [review] Bug 29043: Don't fetch items if when are not on the 'Place a hold on' form
Created attachment 129915 [details] [review] 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>
Created attachment 129916 [details] [review] 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>
Created attachment 129949 [details] [review] 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> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 129950 [details] [review] 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>
Nice bit of cleaning up here, thanks.. some pretty involved code, but everything appears to work as expected and there's a noticeable performance gain. QA scripts happy Passing QA
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Pushed to 21.11.x for 21.11.03
Pushed to 21.05.x for 21.05.11
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.