Koha-community/Koha
13
7
Fork 0
Code Issues Releases Wiki Activity

Bug 33608: Polish Koha::Statistic further

Browse source 
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 <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

[EDIT] Fixed the mandatory check in Stats.t. Removed key_or_default.
       Additional tidy.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
This commit is contained in:
Marcel de Rooy 2023-05-08 09:17:32 +00:00 committed by Tomas Cohen Arazi
parent 7ceb3959c3
commit 917e328341
Signed by: tomascohen
GPG key ID: 0A272EA1B2F3C15F
2 changed files with 45 additions and 71 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

88
Koha/Statistic.pm
View file

@ -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;
Koha::Exceptions::BadParameter->throw( parameter => $params ) if !$params || ref($params) ne 'HASH';
Koha::Exceptions::WrongParameter->throw( name => 'type', value => $params->{type} ) if !$params->{type};
# 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 ( 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 @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 $dtf = Koha::Database->new->schema->storage->datetime_parser;
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,
}
);
}

28
t/db_dependent/Stats.t
View file

@ -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");