From 917e3283411d8a25d86ed57a470ce6d224365a24 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Mon, 8 May 2023 09:17:32 +0000 Subject: [PATCH] Bug 33608: Polish Koha::Statistic further Adding exceptions, removing croaks. No exception in new for unknown hash keys, store will catch that. Prepare switching amount parameter to value (db column name). Test plan: Run t/db_dependent/Stats.t Run t/db_dependent/Koha/Items.t Signed-off-by: Marcel de Rooy Signed-off-by: Owen Leonard Signed-off-by: Martin Renvoize [EDIT] Fixed the mandatory check in Stats.t. Removed key_or_default. Additional tidy. Signed-off-by: Tomas Cohen Arazi --- Koha/Statistic.pm | 90 ++++++++++++++++-------------------------- t/db_dependent/Stats.t | 28 ++++++------- 2 files changed, 46 insertions(+), 72 deletions(-) diff --git a/Koha/Statistic.pm b/Koha/Statistic.pm index c33abf5493..fb9c28a7c6 100644 --- a/Koha/Statistic.pm +++ b/Koha/Statistic.pm @@ -1,5 +1,7 @@ package Koha::Statistic; +# Copyright 2019, 2023 Koha development team +# # This file is part of Koha. # # Koha is free software; you can redistribute it and/or modify it @@ -27,7 +29,7 @@ use base qw(Koha::Object); our @allowed_accounts_types = qw( writeoff payment ); our @allowed_circulation_types = qw( renew issue localuse return onsite_checkout recall item_found item_lost ); -our @mandatory_accounts_keys = qw( type branch borrowernumber amount ); +our @mandatory_accounts_keys = qw( type branch borrowernumber value ); # note that amount is mapped to value our @mandatory_circulation_keys = qw( type branch borrowernumber itemnumber ccode itemtype ); =head1 NAME @@ -48,7 +50,8 @@ Koha::Statistic - Koha Statistic Object class itemnumber : itemnumber borrowernumber : borrowernumber categorycode : patron category - amount : transaction amount + amount : transaction amount (legacy parameter name) + value : transaction amount other : sipmode itemtype : itemtype ccode : collection code @@ -67,67 +70,42 @@ Koha::Statistic - Koha Statistic Object class sub new { my ( $class, $params ) = @_; - # make some controls - return () if !defined $params; - - # change these arrays if new types of transaction or new parameters are allowed - my @allowed_keys = - qw (type branch amount other itemnumber itemtype borrowernumber ccode location categorycode interface); + Koha::Exceptions::BadParameter->throw( parameter => $params ) if !$params || ref($params) ne 'HASH'; + Koha::Exceptions::WrongParameter->throw( name => 'type', value => $params->{type} ) if !$params->{type}; - my @mandatory_keys = (); - if ( !exists $params->{type} or !defined $params->{type} ) { - croak("UpdateStats did not received type param"); + if ( exists $params->{amount} ) { # legacy amount parameter still supported + $params->{value} //= delete $params->{amount}; } - if ( grep ( $_ eq $params->{type}, @allowed_circulation_types ) ) { - @mandatory_keys = @mandatory_circulation_keys; - } elsif ( grep ( $_ eq $params->{type}, @allowed_accounts_types ) ) { - @mandatory_keys = @mandatory_accounts_keys; + + my $category; + if ( grep { $_ eq $params->{type} } @allowed_circulation_types ) { + $category = 'circulation'; + } elsif ( grep { $_ eq $params->{type} } @allowed_accounts_types ) { + $category = 'accounts'; } else { - croak( "UpdateStats received forbidden type param: " . $params->{type} ); - } - my @missing_params = (); - for my $mykey (@mandatory_keys) { - push @missing_params, $mykey if !grep ( /^$mykey/, keys %$params ); - } - if ( scalar @missing_params > 0 ) { - croak( "UpdateStats did not received mandatory param(s): " . join( ", ", @missing_params ) ); - } - my @invalid_params = (); - for my $myparam ( keys %$params ) { - push @invalid_params, $myparam unless grep { $_ eq $myparam } @allowed_keys; - } - if ( scalar @invalid_params > 0 ) { - croak( "UpdateStats received invalid param(s): " . join( ", ", @invalid_params ) ); + Koha::Exceptions::WrongParameter->throw( name => 'type', value => $params->{type} ); } - # get the parameters - my $branch = $params->{branch}; - my $type = $params->{type}; - my $borrowernumber = exists $params->{borrowernumber} ? $params->{borrowernumber} : ''; - my $itemnumber = exists $params->{itemnumber} ? $params->{itemnumber} : undef; - my $amount = exists $params->{amount} ? $params->{amount} : 0; - my $other = exists $params->{other} ? $params->{other} : ''; - my $itemtype = exists $params->{itemtype} ? $params->{itemtype} : ''; - my $location = exists $params->{location} ? $params->{location} : undef; - my $ccode = exists $params->{ccode} ? $params->{ccode} : ''; - my $categorycode = exists $params->{categorycode} ? $params->{categorycode} : undef; - my $interface = exists $params->{interface} ? $params->{interface} : undef; - - my $dtf = Koha::Database->new->schema->storage->datetime_parser; + my @mandatory_keys = $category eq 'circulation' ? @mandatory_circulation_keys : @mandatory_accounts_keys; + my @missing = map { exists $params->{$_} ? () : $_ } @mandatory_keys; + Koha::Exceptions::MissingParameter->throw( parameter => join( ',', @missing ) ) if @missing; + + my $datetime = Koha::Database->new->schema->storage->datetime_parser->format_datetime( dt_from_string() ); return $class->SUPER::new( { - datetime => $dtf->format_datetime( dt_from_string() ), - branch => $branch, - type => $type, - value => $amount, - other => $other, - itemnumber => $itemnumber, - itemtype => $itemtype, - location => $location, - borrowernumber => $borrowernumber, - categorycode => $categorycode, - ccode => $ccode, - interface => $interface, + borrowernumber => $params->{borrowernumber}, # no longer sending empty string (changed 2023) + branch => $params->{branch}, + categorycode => $params->{categorycode}, + ccode => exists $params->{ccode} ? $params->{ccode} : q{}, + datetime => $datetime, + interface => $params->{interface}, + itemnumber => $params->{itemnumber}, + itemtype => exists $params->{itemtype} ? $params->{itemtype} : q{}, + location => $params->{location}, + other => exists $params->{other} ? $params->{other} : q{}, + type => $params->{type}, + value => exists $params->{value} ? $params->{value} : 0, + } ); } diff --git a/t/db_dependent/Stats.t b/t/db_dependent/Stats.t index fba30415ba..fa04b826ce 100755 --- a/t/db_dependent/Stats.t +++ b/t/db_dependent/Stats.t @@ -19,6 +19,7 @@ use Modern::Perl; use Test::More tests => 1; +use Test::Exception; use C4::Context; use C4::Stats qw( UpdateStats ); @@ -29,14 +30,15 @@ $schema->storage->txn_begin; my $dbh = C4::Context->dbh; subtest 'UpdateStats' => sub { - plan tests => 17; - is (UpdateStats () ,undef, "UpdateStats returns undef if no params"); + plan tests => 16; + + throws_ok { UpdateStats() } 'Koha::Exceptions::BadParameter', 'UpdateStats called without params'; my $params = { branch => "BRA", itemnumber => 31, borrowernumber => 5, - amount =>5.1, + amount => 5.1, other => "bla", itemtype => "BK", location => "LOC", @@ -64,13 +66,14 @@ subtest 'UpdateStats' => sub { isnt ($return_error,'',"UpdateStats returns undef and croaks if type is undef"); # returns undef and croaks if mandatory params are missing - my @allowed_circulation_types = qw (renew issue localuse return onsite_checkout recall); - my @allowed_accounts_types = qw (writeoff payment); - my @circulation_mandatory_keys = qw (branch borrowernumber itemnumber ccode itemtype); #don't check type here - my @accounts_mandatory_keys = qw (branch borrowernumber amount); #don't check type here + my @allowed_circulation_types = @Koha::Statistic::allowed_circulation_types; + my @allowed_accounts_types = @Koha::Statistic::allowed_accounts_types; + my @circulation_mandatory_keys = @Koha::Statistic::mandatory_circulation_keys; + my @accounts_mandatory_keys = @Koha::Statistic::mandatory_accounts_keys; my @missing_errors = (); foreach my $key (@circulation_mandatory_keys) { + next if $key eq 'type'; my $value = $params->{$key}; delete $params->{$key}; foreach my $type (@allowed_circulation_types) { @@ -82,6 +85,7 @@ subtest 'UpdateStats' => sub { $params->{$key} = $value; } foreach my $key (@accounts_mandatory_keys) { + next if $key eq 'type'; my $value = $params->{$key}; delete $params->{$key}; foreach my $type (@allowed_accounts_types) { @@ -95,14 +99,6 @@ subtest 'UpdateStats' => sub { } is (join (", ", @missing_errors),'',"UpdateStats returns undef and croaks if mandatory params are missing"); - # returns undef and croaks if forbidden params are given - $params -> {type} = "return"; - $params -> {newparam} = "true"; - eval {UpdateStats($params)}; - $return_error = $@; - isnt ($return_error,'',"UpdateStats returns undef and croaks if a forbidden param is given"); - delete $params->{newparam}; - # save the params in the right database fields $dbh->do(q|DELETE FROM statistics|); $params = { @@ -124,7 +120,7 @@ subtest 'UpdateStats' => sub { is ($params->{branch}, $line->{branch}, "UpdateStats save branch param in branch field of statistics table"); is ($params->{type}, $line->{type}, "UpdateStats save type param in type field of statistics table"); is ($params->{borrowernumber}, $line->{borrowernumber}, "UpdateStats save borrowernumber param in borrowernumber field of statistics table"); - cmp_ok($params->{amount},'==', $line->{value}, "UpdateStats save amount param in value field of statistics table"); + is ($params->{value}, $line->{value}, "UpdateStats save amount param in value field of statistics table"); is ($params->{other}, $line->{other}, "UpdateStats save other param in other field of statistics table"); is ($params->{itemtype}, $line->{itemtype}, "UpdateStats save itemtype param in itemtype field of statistics table"); is ($params->{location}, $line->{location}, "UpdateStats save location param in location field of statistics table"); -- 2.39.5