From 50109e06f903ad25ac1e84270cb371c243a3581d Mon Sep 17 00:00:00 2001 From: Andrew Isherwood Date: Mon, 28 Sep 2020 10:41:16 +0100 Subject: [PATCH] Bug 24190: (follow-up) Respond to QA feedback This commit makes changes in response to Jonathan's feedback in comment - Moved from using zero padded strings to store log data to a JSON object - Stopped storing formatted dates in logged data Signed-off-by: Nick Clemens Signed-off-by: Jonathan Druart --- C4/Budgets.pm | 31 +++++++++++---------- acqui/addorder.pl | 14 +++++----- acqui/finishreceive.pl | 16 ++++++----- acqui/invoice.pl | 58 +++++++++------------------------------- admin/aqbudgetperiods.pl | 20 +++++++------- 5 files changed, 57 insertions(+), 82 deletions(-) diff --git a/C4/Budgets.pm b/C4/Budgets.pm index 439fcaf4b7..8b6d63c7e5 100644 --- a/C4/Budgets.pm +++ b/C4/Budgets.pm @@ -18,6 +18,7 @@ package C4::Budgets; # along with Koha; if not, see . use Modern::Perl; +use JSON; use C4::Context; use Koha::Database; use Koha::Patrons; @@ -645,15 +646,16 @@ sub AddBudget { # Log the addition if (C4::Context->preference("AcqLog")) { - my $infos = - sprintf("%010d", $budget->{budget_amount}) . - sprintf("%010d", $budget->{budget_encumb}) . - sprintf("%010d", $budget->{budget_expend}); + my $infos = { + budget_amount => $budget->{budget_amount}, + budget_encumb => $budget->{budget_encumb}, + budget_expend => $budget->{budget_expend} + }; logaction( 'ACQUISITIONS', 'CREATE_FUND', $id, - $infos + encode_json($infos) ); } return $id; @@ -668,19 +670,20 @@ sub ModBudget { # Log this modification if (C4::Context->preference("AcqLog")) { - my $infos = - sprintf("%010d", $budget->{budget_amount}) . - sprintf("%010d", $budget->{budget_encumb}) . - sprintf("%010d", $budget->{budget_expend}) . - sprintf("%010d", $result->budget_amount) . - sprintf("%010d", $result->budget_encumb) . - sprintf("%010d", $result->budget_expend) . - sprintf("%010d", 0 - ($result->budget_amount - $budget->{budget_amount})); + my $infos = { + budget_amount_new => $budget->{budget_amount}, + budget_encumb_new => $budget->{budget_encumb}, + budget_expend_new => $budget->{budget_expend}, + budget_amount_old => $result->budget_amount, + budget_encumb_old => $result->budget_encumb, + budget_expend_old => $result->budget_expend, + budget_amount_change => 0 - ($result->budget_amount - $budget->{budget_amount}) + }; logaction( 'ACQUISITIONS', 'MODIFY_FUND', $budget->{budget_id}, - $infos + encode_json($infos) ); } diff --git a/acqui/addorder.pl b/acqui/addorder.pl index c7aa868968..dbfad999a1 100755 --- a/acqui/addorder.pl +++ b/acqui/addorder.pl @@ -119,7 +119,7 @@ if it is an order from an existing suggestion : the id of this suggestion. use Modern::Perl; use CGI qw ( -utf8 ); -use JSON qw( to_json ); +use JSON qw ( to_json encode_json ); use C4::Auth qw( get_template_and_user ); use C4::Acquisition qw( FillWithDefaultValues populate_order_with_prices ModOrder ModOrderUsers ); use C4::Suggestions qw( ModSuggestion ); @@ -320,15 +320,15 @@ if ( $basket->{is_standing} || $orderinfo->{quantity} ne '0' ) { ModOrder($orderinfo); # Log the order modification if (C4::Context->preference("AcqLog")) { - my $infos = ''; + my $infos = {}; foreach my $field(@log_order_fields) { - $infos .= sprintf("%010d", $orderinfo->{$field}); + $infos->{$field} = $orderinfo->{$field}; } logaction( 'ACQUISITIONS', 'MODIFY_ORDER', $orderinfo->{ordernumber}, - $infos + encode_json($infos) ); } } @@ -336,15 +336,15 @@ if ( $basket->{is_standing} || $orderinfo->{quantity} ne '0' ) { $order->store; # Log the order creation if (C4::Context->preference("AcqLog")) { - my $infos = ''; + my $infos = {}; foreach my $field(@log_order_fields) { - $infos .= sprintf("%010d", $orderinfo->{$field}); + $infos->{$field} = $orderinfo->{$field}; } logaction( 'ACQUISITIONS', 'CREATE_ORDER', $order->ordernumber, - $infos + encode_json($infos) ); } } diff --git a/acqui/finishreceive.pl b/acqui/finishreceive.pl index e3305be479..9934af6aa0 100755 --- a/acqui/finishreceive.pl +++ b/acqui/finishreceive.pl @@ -23,6 +23,7 @@ use Modern::Perl; use CGI qw ( -utf8 ); use C4::Auth qw( checkauth ); +use JSON qw( encode_json ); use C4::Output; use C4::Context; use C4::Acquisition qw( GetInvoice GetOrder populate_order_with_prices ModReceiveOrder ); @@ -192,18 +193,19 @@ if ($suggestion_id) { # Log the receipt if (C4::Context->preference("AcqLog")) { - my $infos = - sprintf("%010d", $quantityrec) . - sprintf("%010d", $bookfund) . - sprintf("%010.2f", $input->param("tax_rate")) . - sprintf("%010.2f", $replacementprice) . - sprintf("%010.2f", $unitprice); + my $infos = { + quantityrec => $quantityrec, + bookfund => $bookfund, + tax_rate => $input->param("tax_rate"), + replacementprice => $replacementprice, + unitprice => $unitprice + }; logaction( 'ACQUISITIONS', 'RECEIVE_ORDER', $ordernumber, - $infos + encode_json($infos) ); } diff --git a/acqui/invoice.pl b/acqui/invoice.pl index 2e05c0dbe0..8e73e6dd63 100755 --- a/acqui/invoice.pl +++ b/acqui/invoice.pl @@ -33,6 +33,7 @@ use C4::Auth qw( get_template_and_user ); use C4::Output qw( output_and_exit output_html_with_http_headers ); use C4::Acquisition qw( CloseInvoice ReopenInvoice ModInvoice MergeInvoices DelInvoice GetInvoice GetInvoiceDetails get_rounded_price ); use C4::Budgets qw( GetBudgetHierarchy GetBudget CanUserUseBudget ); +use JSON qw( encode_json ); use C4::Log qw(logaction); use Koha::Acquisition::Booksellers; @@ -148,19 +149,18 @@ elsif ( $op && $op eq 'del_adj' ) { my $del_adj = Koha::Acquisition::Invoice::Adjustments->find( $adjustment_id ); if ($del_adj) { if (C4::Context->preference("AcqLog")) { - my $reason_length = length $del_adj->reason; - my $reason_padded = ( ' ' x (80 - $reason_length) ) . $del_adj->reason; - my $infos = - sprintf("%010d", $del_adj->invoiceid) . - sprintf("%010d", $del_adj->budget_id) . - sprintf("%010d", $del_adj->encumber_open) . - sprintf("%010.2f", $del_adj->adjustment) . - $reason_padded; + my $infos = { + invoiceid => $del_adj->invoiceid, + budget_id => $del_adj->budget_id, + encumber_open => $del_adj->encumber_open, + adjustment => $del_adj->adjustment, + reason => $del_adj->reason + }; logaction( 'ACQUISITIONS', 'DELETE_INVOICE_ADJUSTMENT', $adjustment_id, - $infos + encode_json($infos) ); } $del_adj->delete(); @@ -195,12 +195,11 @@ elsif ( $op && $op eq 'mod_adj' ) { $new_adj->store(); # Log this addition if (C4::Context->preference("AcqLog")) { - my $infos = get_log_string($adj); logaction( 'ACQUISITIONS', 'CREATE_INVOICE_ADJUSTMENT', $new_adj->adjustment_id, - $infos + encode_json($adj) ); } } @@ -209,7 +208,7 @@ elsif ( $op && $op eq 'mod_adj' ) { unless ( $old_adj->adjustment == $adjustment[$i] && $old_adj->reason eq $reason[$i] && $old_adj->budget_id == $budget_id[$i] && $old_adj->encumber_open == $e_open{$adjustment_id[$i]} && $old_adj->note eq $note[$i] ){ # Log this modification if (C4::Context->preference("AcqLog")) { - my $infos = get_log_string({ + my $infos = { adjustment => $adjustment[$i], reason => $reason[$i], budget_id => $budget_id[$i], @@ -218,12 +217,12 @@ elsif ( $op && $op eq 'mod_adj' ) { reason_old => $old_adj->reason, budget_id_old => $old_adj->budget_id, encumber_open_old => $old_adj->encumber_open - }); + }; logaction( 'ACQUISITIONS', 'UPDATE_INVOICE_ADJUSTMENT', $adjustment_id[$i], - $infos + encode_json($infos) ); } $old_adj->timestamp(undef); @@ -350,35 +349,4 @@ sub get_infos { return \%line; } -sub get_log_string { - my ($data) = @_; - - # We specify the keys we're dealing for logging within an array in order - # to preserve order, if we just iterate the hash keys, we could get - # the values in any order - my @keys = ( - 'budget_id', - 'encumber_open', - 'budget_id_old', - 'encumber_open_old' - ); - # Adjustment amount - my $return = sprintf("%010.2f", $data->{adjustment}); - # Left pad "reason" to the maximum length of aqinvoice_adjustments.reason - # (80 characters) - my $reason_len = length $data->{reason}; - $return .= ( ' ' x (80 - $reason_len) ) . $data->{reason}; - # Append the remaining fields - foreach my $key(@keys) { - $return .= sprintf("%010d", $data->{$key}); - } - # Old adjustment amount - $return .= sprintf("%010.2f", $data->{adjustment_old}); - # Left pad "reason_old" to the maximum length of aqinvoice_adjustments.reason - # (80 characters) - my $reason_old_len = length $data->{reason_old}; - $return .= ( ' ' x (80 - $reason_old_len) ) . $data->{reason_old}; - return $return; -} - output_html_with_http_headers $input, $cookie, $template->output; diff --git a/admin/aqbudgetperiods.pl b/admin/aqbudgetperiods.pl index d6192c2d7c..091943acb3 100755 --- a/admin/aqbudgetperiods.pl +++ b/admin/aqbudgetperiods.pl @@ -48,6 +48,7 @@ use Modern::Perl; use CGI qw ( -utf8 ); use Koha::DateUtils qw( dt_from_string ); +use JSON qw( encode_json ); use Koha::Database; use C4::Koha; use C4::Context; @@ -121,19 +122,20 @@ elsif ( $op eq 'add_validate' ) { # Log the budget modification if (C4::Context->preference("AcqLog")) { my $diff = 0 - ($budgetperiod_old->{budget_period_total} - $budget_period_hashref->{budget_period_total}); - my $infos = - eval { output_pref({ dt => dt_from_string( $input->param('budget_period_startdate') ), dateformat => 'iso', dateonly => 1 } ); } . - eval { output_pref({ dt => dt_from_string( $input->param('budget_period_enddate') ), dateformat => 'iso', dateonly => 1 } ); } . - sprintf("%010d", $budget_period_hashref->{budget_period_total}) . - eval { output_pref({ dt => dt_from_string( $budgetperiod_old->{budget_period_startdate} ), dateformat => 'iso', dateonly => 1 } ); } . - eval { output_pref({ dt => dt_from_string( $budgetperiod_old->{budget_period_enddate} ), dateformat => 'iso', dateonly => 1 } ); } . - sprintf("%010d", $budgetperiod_old->{budget_period_total}) . - sprintf("%010d", $diff); + my $infos = { + budget_period_startdate => $input->param('budget_period_startdate'), + budget_period_enddate => $input->param('budget_period_enddate'), + budget_period_total => $budget_period_hashref->{budget_period_total}, + budget_period_startdate_old => $budgetperiod_old->{budget_period_startdate}, + budget_period_enddate_old => $budgetperiod_old->{budget_period_enddate}, + budget_period_total_old => $budgetperiod_old->{budget_period_total}, + budget_period_total_change => $diff + }; logaction( 'ACQUISITIONS', 'MODIFY_BUDGET', $budget_period_id, - $infos + encode_json($infos) ); } } -- 2.39.5