From df5decdf5b69192aaf71316e4ee201319f9db4c5 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 Content-Type: text/plain; charset=utf-8 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 --- Koha/Statistic.pm | 111 +++++++++++++++++++---------------------- t/db_dependent/Stats.t | 16 ++---- 2 files changed, 56 insertions(+), 71 deletions(-) diff --git a/Koha/Statistic.pm b/Koha/Statistic.pm index ba19c53826..027c51d4a2 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 @@ -66,66 +69,40 @@ 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); - - my @mandatory_keys = (); - if (! exists $params->{type} or ! defined $params->{type}) { - croak ("UpdateStats did not received type param"); - } - 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; + + Koha::Exceptions::BadParameter->throw( parameter => $params ) if !$params || ref($params) ne 'HASH'; + Koha::Exceptions::WrongParameter->throw( name => 'type', value => $params->{type} ) if !$params->{type}; + + $params->{value} //= delete $params->{amount}; # legacy amount parameter supported + + 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; - 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, - } - ); + + my @mandatory_keys = $category eq 'circulation' ? @mandatory_circulation_keys : @mandatory_accounts_keys; + my $first; + Koha::Exceptions::MissingParameter->throw( parameter => $first ) + if grep { exists $params->{$_} ? 0 : ( $first ||= $_ ) } @mandatory_keys; + + return $class->SUPER::new({ + datetime => Koha::Database->new->schema->storage->datetime_parser->format_datetime( dt_from_string ), + branch => $params->{branch}, + type => $params->{type}, + value => _key_or_default( $params, 'value', 0 ), + other => _key_or_default( $params, 'other', q{} ), + itemnumber => $params->{itemnumber}, + itemtype => _key_or_default( $params, 'itemtype', q{} ), + location => $params->{location}, + borrowernumber => $params->{borrowernumber}, # no longer sending empty string (changed 2023) + categorycode => $params->{categorycode}, + ccode => _key_or_default( $params, 'ccode', q{} ), + interface => $params->{interface}, + }); } =head3 store @@ -178,6 +155,20 @@ sub item { =head2 INTERNAL METHODS +=head3 _key_or_default + + $value = _key_or_default( $hash, $key, $default ); + + Returns $hash->{$key} if it exists, otherwise $default. + Note: this is not the same as $hash->{key} // $default ! + +=cut + +sub _key_or_default { + my ( $hash, $key, $default ) = @_; + return exists $hash->{$key} ? $hash->{$key} : $default; +} + =head3 _type =cut diff --git a/t/db_dependent/Stats.t b/t/db_dependent/Stats.t index fba30415ba..bd78e20b49 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,8 +30,9 @@ $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", @@ -95,14 +97,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 +118,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.30.2