From 62576b5f591736c5133aaa3ee6abccb4ad15c9a2 Mon Sep 17 00:00:00 2001 From: Fridolin Somers Date: Fri, 4 Mar 2022 16:48:16 -1000 Subject: [PATCH] Bug 30232: Logged in user performance for CanUserUseBudget() and CanUserModifyBudget() Some acquisition methods can take borrowernumber or a Koha::Patron->unblessed. When called with logged in user, sometimes in a loop, performance will be better if fetching Koha::Patron of logged in user only once. This patch optimises call for CanUserUseBudget() and CanUserModifyBudget(); You may benchmark on a user with a lot of budgets acess. Test plan : Play with impacted pages and check budgets list is the same than wihtout patch --- acqui/acqui-home.pl | 5 ++++- acqui/addorderiso2709.pl | 8 ++++---- acqui/basket.pl | 7 ++++--- acqui/booksellers.pl | 2 +- acqui/duplicate_orders.pl | 6 ++++-- acqui/invoice.pl | 6 ++++-- acqui/invoices.pl | 6 +++++- acqui/neworderempty.pl | 7 ++++--- acqui/orderreceive.pl | 6 ++++-- acqui/parcels.pl | 6 +++++- admin/aqbudgets.pl | 11 +++++++---- admin/aqplan.pl | 5 ++++- suggestion/suggestion.pl | 7 +++++-- 13 files changed, 55 insertions(+), 27 deletions(-) diff --git a/acqui/acqui-home.pl b/acqui/acqui-home.pl index 05eb16bf06..ea48eece42 100755 --- a/acqui/acqui-home.pl +++ b/acqui/acqui-home.pl @@ -46,6 +46,9 @@ my ( $template, $loggedinuser, $cookie, $userflags ) = get_template_and_user( } ); +my $logged_in_patron = Koha::Patrons->find($loggedinuser); +my $logged_in_patron_unblessed = $logged_in_patron->unblessed; + my $status = $query->param('status') || "ASKED"; # Get current branch count and total viewable count, if they don't match then pass # both to template @@ -77,7 +80,7 @@ my $totavail_active = 0; my @budget_loop; foreach my $budget ( @{$budget_arr} ) { - next unless (CanUserUseBudget($loggedinuser, $budget, $userflags)); + next unless (CanUserUseBudget($logged_in_patron_unblessed, $budget, $userflags)); my $patron = Koha::Patrons->find( $budget->{budget_owner_id} ); if ( $patron ) { diff --git a/acqui/addorderiso2709.pl b/acqui/addorderiso2709.pl index 658adb03da..746498e128 100755 --- a/acqui/addorderiso2709.pl +++ b/acqui/addorderiso2709.pl @@ -68,6 +68,8 @@ my $op = $cgiparams->{'op'} || ''; my $booksellerid = $input->param('booksellerid'); my $allmatch = $input->param('allmatch'); my $bookseller = Koha::Acquisition::Booksellers->find( $booksellerid ); +my $logged_in_patron = Koha::Patrons->find($loggedinuser); +my $logged_in_patron_unblessed = $logged_in_patron->unblessed; $template->param(scriptname => "/cgi-bin/koha/acqui/addorderiso2709.pl", booksellerid => $booksellerid, @@ -312,14 +314,13 @@ if ($op eq ""){ } } else { # 3rd add order - my $patron = Koha::Patrons->find( $loggedinuser ); # get quantity in the MARC record (1 if none) my $quantity = GetMarcQuantity($marcrecord, C4::Context->preference('marcflavour')) || 1; my %orderinfo = ( biblionumber => $biblionumber, basketno => $cgiparams->{'basketno'}, quantity => $c_quantity, - branchcode => $patron->branchcode, + branchcode => $logged_in_patron->branchcode, budget_id => $c_budget_id, uncertainprice => 1, sort1 => $c_sort1, @@ -406,14 +407,13 @@ if ($op eq ""){ my $budgets = GetBudgets(); my $budget_id = @$budgets[0]->{'budget_id'}; # build bookfund list -my $patron = Koha::Patrons->find( $loggedinuser )->unblessed; my $budget = GetBudget($budget_id); # build budget list my $budget_loop = []; my $budgets_hierarchy = GetBudgetHierarchy; foreach my $r ( @{$budgets_hierarchy} ) { - next unless (CanUserUseBudget($patron, $r, $userflags)); + next unless (CanUserUseBudget($logged_in_patron_unblessed, $r, $userflags)); push @{$budget_loop}, { b_id => $r->{budget_id}, b_txt => $r->{budget_name}, diff --git a/acqui/basket.pl b/acqui/basket.pl index 0710861946..f61127f83d 100755 --- a/acqui/basket.pl +++ b/acqui/basket.pl @@ -85,7 +85,8 @@ our ( $template, $loggedinuser, $cookie, $userflags ) = get_template_and_user( } ); -my $logged_in_patron = Koha::Patrons->find( $loggedinuser ); +my $logged_in_patron = Koha::Patrons->find($loggedinuser); +my $logged_in_patron_unblessed = $logged_in_patron->unblessed; our $basket = GetBasket($basketno); $booksellerid = $basket->{booksellerid} unless $booksellerid; @@ -95,7 +96,7 @@ my $rs = $schema->resultset('VendorEdiAccount')->search( { vendor_id => $booksellerid, } ); $template->param( ediaccount => ($rs->count > 0)); -unless (CanUserManageBasket($logged_in_patron->unblessed, $basket, $userflags)) { +unless (CanUserManageBasket($logged_in_patron_unblessed, $basket, $userflags)) { $template->param( cannot_manage_basket => 1, basketno => $basketno, @@ -370,7 +371,7 @@ if ( $op eq 'list' ) { my $budgets = GetBudgetHierarchy; my $has_budgets = 0; foreach my $r (@{$budgets}) { - next unless (CanUserUseBudget($loggedinuser, $r, $userflags)); + next unless (CanUserUseBudget($logged_in_patron_unblessed, $r, $userflags)); $has_budgets = 1; last; diff --git a/acqui/booksellers.pl b/acqui/booksellers.pl index 959f829d48..ce285c45f0 100755 --- a/acqui/booksellers.pl +++ b/acqui/booksellers.pl @@ -108,7 +108,7 @@ my $userbranch = $userenv->{branch}; my $budgets = GetBudgetHierarchy; my $has_budgets = 0; foreach my $r (@{$budgets}) { - next unless (CanUserUseBudget($loggedinuser, $r, $userflags)); + next unless (CanUserUseBudget($logged_in_patron_unblessed, $r, $userflags)); $has_budgets = 1; last; diff --git a/acqui/duplicate_orders.pl b/acqui/duplicate_orders.pl index 21e382e72f..38010b1b16 100755 --- a/acqui/duplicate_orders.pl +++ b/acqui/duplicate_orders.pl @@ -29,6 +29,7 @@ use Koha::Acquisition::Baskets; use Koha::Acquisition::Currencies; use Koha::Acquisition::Orders; use Koha::DateUtils qw( dt_from_string output_pref ); +use Koha::Patrons; my $input = CGI->new; my $basketno = $input->param('basketno'); @@ -49,7 +50,8 @@ output_and_exit( $input, $cookie, $template, 'unknown_basket' ) unless $basket; my $vendor = $basket->bookseller; -my $patron = Koha::Patrons->find($loggedinuser)->unblessed; +my $logged_in_patron = Koha::Patrons->find($loggedinuser); +my $logged_in_patron_unblessed = $logged_in_patron->unblessed; my $filters = { basket => scalar $input->param('basket'), @@ -102,7 +104,7 @@ elsif ( $op eq 'batch_edit' ) { my $budgets_hierarchy = GetBudgetHierarchy; foreach my $r ( @{$budgets_hierarchy} ) { next - unless ( C4::Budgets::CanUserUseBudget( $patron, $r, $userflags ) ); + unless ( C4::Budgets::CanUserUseBudget( $logged_in_patron_unblessed, $r, $userflags ) ); push @{$budget_loop}, { diff --git a/acqui/invoice.pl b/acqui/invoice.pl index 7240d9319f..5da329e3ef 100755 --- a/acqui/invoice.pl +++ b/acqui/invoice.pl @@ -52,7 +52,9 @@ my ( $template, $loggedinuser, $cookie, $flags ) = get_template_and_user( } ); -my $logged_in_patron = Koha::Patrons->find( $loggedinuser ); +my $logged_in_patron = Koha::Patrons->find($loggedinuser); +my $logged_in_patron_unblessed = $logged_in_patron->unblessed; + my $invoiceid = $input->param('invoiceid'); my $op = $input->param('op'); @@ -279,7 +281,7 @@ my $shipmentcost_budgetid = $details->{shipmentcost_budgetid}; my $budget_loop = []; my $budgets = GetBudgetHierarchy(); foreach my $r ( @{$budgets} ) { - next unless ( CanUserUseBudget( $loggedinuser, $r, $flags ) ); + next unless ( CanUserUseBudget( $logged_in_patron_unblessed, $r, $flags ) ); my $selected = $shipmentcost_budgetid ? $r->{budget_id} eq $shipmentcost_budgetid : 0; diff --git a/acqui/invoices.pl b/acqui/invoices.pl index 9720ead33e..173eac24a4 100755 --- a/acqui/invoices.pl +++ b/acqui/invoices.pl @@ -36,6 +36,7 @@ use C4::Acquisition qw( GetInvoices GetInvoice ); use C4::Budgets qw( GetBudget GetBudgets CanUserUseBudget ); use Koha::DateUtils qw( dt_from_string output_pref ); use Koha::Acquisition::Booksellers; +use Koha::Patrons; my $input = CGI->new; my ( $template, $loggedinuser, $cookie, $flags ) = get_template_and_user( @@ -47,6 +48,9 @@ my ( $template, $loggedinuser, $cookie, $flags ) = get_template_and_user( } ); +my $logged_in_patron = Koha::Patrons->find($loggedinuser); +my $logged_in_patron_unblessed = $logged_in_patron->unblessed; + my $invoicenumber = $input->param('invoicenumber'); my $supplierid = $input->param('supplierid'); my $shipmentdatefrom = $input->param('shipmentdatefrom'); @@ -107,7 +111,7 @@ foreach (@suppliers) { my $budgets = GetBudgets(); my @budgets_loop; foreach my $budget (@$budgets) { - push @budgets_loop, $budget if CanUserUseBudget( $loggedinuser, $budget, $flags ); + push @budgets_loop, $budget if CanUserUseBudget( $logged_in_patron_unblessed, $budget, $flags ); } my (@openedinvoices, @closedinvoices); diff --git a/acqui/neworderempty.pl b/acqui/neworderempty.pl index 7c443147b1..078f366bf4 100755 --- a/acqui/neworderempty.pl +++ b/acqui/neworderempty.pl @@ -125,6 +125,9 @@ our ( $template, $loggedinuser, $cookie, $userflags ) = get_template_and_user( } ); +my $logged_in_patron = Koha::Patrons->find($loggedinuser); +my $logged_in_patron_unblessed = $logged_in_patron->unblessed; + our $marcflavour = C4::Context->preference('marcflavour'); if(!$basketno) { @@ -328,14 +331,12 @@ $suggestion = GetSuggestionInfo($suggestionid) if $suggestionid; my $active_currency = Koha::Acquisition::Currencies->get_active; # build bookfund list -my $patron = Koha::Patrons->find( $loggedinuser )->unblessed; - my $budget = GetBudget($budget_id); # build budget list my $budget_loop = []; my $budgets = GetBudgetHierarchy; foreach my $r (@{$budgets}) { - next unless (CanUserUseBudget($patron, $r, $userflags)); + next unless (CanUserUseBudget($logged_in_patron_unblessed, $r, $userflags)); push @{$budget_loop}, { b_id => $r->{budget_id}, b_txt => $r->{budget_name}, diff --git a/acqui/orderreceive.pl b/acqui/orderreceive.pl index e1b76d3273..8671766edb 100755 --- a/acqui/orderreceive.pl +++ b/acqui/orderreceive.pl @@ -103,6 +103,9 @@ unless ( $order ) { exit; } +my $logged_in_patron = Koha::Patrons->find($loggedinuser); +my $logged_in_patron_unblessed = $logged_in_patron->unblessed; + # prepare the form for receiving my $basket = $order->basket; my $currencies = Koha::Acquisition::Currencies->search; @@ -169,7 +172,6 @@ if ( $suggestion ) { $template->param( suggestion => $suggestion ); } -my $patron = Koha::Patrons->find( $loggedinuser )->unblessed; my @budget_loop; my $periods = GetBudgetPeriods( ); foreach my $period (@$periods) { @@ -180,7 +182,7 @@ foreach my $period (@$periods) { my $budget_hierarchy = GetBudgetHierarchy( $period->{'budget_period_id'} ); my @funds; foreach my $r ( @{$budget_hierarchy} ) { - next unless ( CanUserUseBudget( $patron, $r, $userflags ) ); + next unless ( CanUserUseBudget( $logged_in_patron_unblessed, $r, $userflags ) ); if ( !defined $r->{budget_amount} || $r->{budget_amount} == 0 ) { next; } diff --git a/acqui/parcels.pl b/acqui/parcels.pl index cd6e2562b1..c6f1f8168c 100755 --- a/acqui/parcels.pl +++ b/acqui/parcels.pl @@ -76,6 +76,7 @@ use C4::Budgets qw( GetBudgetHierarchy GetBudget CanUserUseBudget ); use Koha::Acquisition::Booksellers; use Koha::DateUtils qw( output_pref dt_from_string ); +use Koha::Patrons; my $input = CGI->new; my $booksellerid = $input->param('booksellerid'); @@ -96,6 +97,9 @@ our ( $template, $loggedinuser, $cookie, $flags ) = get_template_and_user( } ); +my $logged_in_patron = Koha::Patrons->find($loggedinuser); +my $logged_in_patron_unblessed = $logged_in_patron->unblessed; + my $invoicenumber = $input->param('invoice'); my $shipmentcost = $input->param('shipmentcost'); my $shipmentcost_budgetid = $input->param('shipmentcost_budgetid'); @@ -179,7 +183,7 @@ if ($count_parcels) { my $budget_loop = []; my $budgets = GetBudgetHierarchy; foreach my $r (@{$budgets}) { - next unless (CanUserUseBudget($loggedinuser, $r, $flags)); + next unless (CanUserUseBudget($logged_in_patron_unblessed, $r, $flags)); push @{$budget_loop}, { b_id => $r->{budget_id}, b_txt => $r->{budget_name}, diff --git a/admin/aqbudgets.pl b/admin/aqbudgets.pl index 4ed48d47d1..3b2d5fdb79 100755 --- a/admin/aqbudgets.pl +++ b/admin/aqbudgets.pl @@ -56,6 +56,9 @@ my ($template, $borrowernumber, $cookie, $staffflags ) = get_template_and_user( } ); +my $logged_in_patron = Koha::Patrons->find($borrowernumber); +my $logged_in_patron_unblessed = $logged_in_patron->unblessed; + my $active_currency = Koha::Acquisition::Currencies->get_active; if ( $active_currency ) { $template->param( symbol => $active_currency->symbol, @@ -115,7 +118,7 @@ if ($op eq 'add_form') { my $dropbox_disabled; if (defined $budget_id ) { ### MOD $budget = GetBudget($budget_id); - if (!CanUserModifyBudget($borrowernumber, $budget, $staffflags)) { + if (!CanUserModifyBudget($logged_in_patron_unblessed, $budget, $staffflags)) { $template->param(error_not_authorised_to_modify => 1); output_html_with_http_headers $input, $cookie, $template->output; exit; @@ -211,7 +214,7 @@ if ($op eq 'add_form') { my $budget_modified = 0; if (defined $budget_id) { - if (CanUserModifyBudget($borrowernumber, $budget_hash->{budget_id}, + if (CanUserModifyBudget($logged_in_patron_unblessed, $budget_hash->{budget_id}, $staffflags) ) { ModBudget( $budget_hash ); @@ -252,8 +255,8 @@ if ( $op eq 'list' ) { my @budgets_to_display; foreach my $budget (@budgets) { # PERMISSIONS - next unless CanUserUseBudget($borrowernumber, $budget, $staffflags); - unless(CanUserModifyBudget($borrowernumber, $budget, $staffflags)) { + next unless CanUserUseBudget($logged_in_patron_unblessed, $budget, $staffflags); + unless(CanUserModifyBudget($logged_in_patron_unblessed, $budget, $staffflags)) { $budget->{'budget_lock'} = 1; } diff --git a/admin/aqplan.pl b/admin/aqplan.pl index 64b6cd05ab..d5839f403d 100755 --- a/admin/aqplan.pl +++ b/admin/aqplan.pl @@ -41,6 +41,7 @@ use C4::Context; use C4::Output qw( output_html_with_http_headers ); use C4::Auth qw( get_template_and_user ); use Koha::Acquisition::Currencies; +use Koha::Patrons; our $input = CGI->new; #### $input @@ -54,6 +55,8 @@ my ( $template, $borrowernumber, $cookie, $staff_flags ) = get_template_and_user flagsrequired => { acquisition => 'planning_manage' }, } ); +my $logged_in_patron = Koha::Patrons->find($borrowernumber); +my $logged_in_patron_unblessed = $logged_in_patron->unblessed; my $budget_period_id = $input->param('budget_period_id'); # ' ------- get periods stuff ------------------' @@ -315,7 +318,7 @@ my ( @budget_lines, %cell_hash ); foreach my $budget (@budgets) { my $budget_lock; - unless (CanUserUseBudget($borrowernumber, $budget, $staff_flags)) { + unless (CanUserUseBudget($logged_in_patron_unblessed, $budget, $staff_flags)) { $budget_lock = 1 } diff --git a/suggestion/suggestion.pl b/suggestion/suggestion.pl index 5372e00d5b..07de3cf51b 100755 --- a/suggestion/suggestion.pl +++ b/suggestion/suggestion.pl @@ -122,6 +122,9 @@ $borrowernumber = $input->param('borrowernumber') if ( $input->param('borrowernu $template->param('borrowernumber' => $borrowernumber); my $branchfilter = $input->param('branchcode') || C4::Context->userenv->{'branch'}; +my $logged_in_patron = Koha::Patrons->find($borrowernumber); +my $logged_in_patron_unblessed = $logged_in_patron->unblessed; + ######################################### ## Operations ## @@ -421,7 +424,7 @@ $template->param(patron_reason_loop=>$patron_reason_loop); my $budgets = GetBudgets; my @budgets_loop; foreach my $budget ( @{$budgets} ) { - next unless (CanUserUseBudget($borrowernumber, $budget, $userflags)); + next unless (CanUserUseBudget($logged_in_patron_unblessed, $budget, $userflags)); ## Please see file perltidy.ERR $budget->{'selected'} = 1 @@ -436,7 +439,7 @@ $template->param( budgetsloop => \@budgets_loop); my $sugg_budget_loop = []; my $sugg_budgets = GetBudgetHierarchy(); foreach my $r ( @{$sugg_budgets} ) { - next unless ( CanUserUseBudget( $borrowernumber, $r, $userflags ) ); + next unless ( CanUserUseBudget( $logged_in_patron_unblessed, $r, $userflags ) ); my $selected = ( $$suggestion_ref{budgetid} && $r->{budget_id} eq $$suggestion_ref{budgetid} ) ? 1 : 0; push @{$sugg_budget_loop}, { -- 2.35.1