From 5d153ad2e36610929b1df70e5cd6ff298949165c 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 --- C4/Budgets.pm | 31 +++++++++++---------- acqui/addorder.pl | 13 ++++----- acqui/finishreceive.pl | 16 ++++++----- acqui/invoice.pl | 58 +++++++++------------------------------- admin/aqbudgetperiods.pl | 20 +++++++------- 5 files changed, 57 insertions(+), 81 deletions(-) diff --git a/C4/Budgets.pm b/C4/Budgets.pm index 5086d71715..6e9442880b 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; @@ -649,15 +650,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; @@ -672,19 +674,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 7039d5c17b..bf4b8c4b1d 100755 --- a/acqui/addorder.pl +++ b/acqui/addorder.pl @@ -119,6 +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; use C4::Auth; # get_template_and_user use C4::Acquisition; # ModOrder use C4::Suggestions; # ModStatus @@ -314,15 +315,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) ); } } @@ -330,15 +331,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 5c89985c09..b89f172f07 100755 --- a/acqui/finishreceive.pl +++ b/acqui/finishreceive.pl @@ -22,6 +22,7 @@ use Modern::Perl; use CGI qw ( -utf8 ); +use JSON; use C4::Auth; use C4::Output; use C4::Context; @@ -193,18 +194,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 8185bbd0fe..14ac5e2630 100755 --- a/acqui/invoice.pl +++ b/acqui/invoice.pl @@ -29,6 +29,7 @@ Invoice details use Modern::Perl; use CGI qw ( -utf8 ); +use JSON; use C4::Auth; use C4::Output; use C4::Acquisition; @@ -144,19 +145,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(); @@ -191,12 +191,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) ); } } @@ -205,7 +204,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], @@ -214,12 +213,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); @@ -346,35 +345,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 be03f56f5b..0d82636204 100755 --- a/admin/aqbudgetperiods.pl +++ b/admin/aqbudgetperiods.pl @@ -48,6 +48,7 @@ use Modern::Perl; use CGI qw ( -utf8 ); use List::Util qw/min/; +use JSON; use Koha::DateUtils; use Koha::Database; use C4::Koha; @@ -124,19 +125,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.20.1