From 845b57ee45617bbce6349b51b3a5db42be4a3d21 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 19 Jul 2023 15:01:27 +0100 Subject: [PATCH] Bug 33608: (QA follow-up) Tidy Signed-off-by: Martin Renvoize --- Koha/Item.pm | 45 ++++++++++----- Koha/Statistic.pm | 64 ++++++++++++--------- t/db_dependent/Koha/Statistics.t | 96 +++++++++++++++++--------------- 3 files changed, 119 insertions(+), 86 deletions(-) diff --git a/Koha/Item.pm b/Koha/Item.pm index 97763b6b0d..84ab3c6c38 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -130,8 +130,9 @@ sub store { $self->cn_sort($cn_sort); } - if( $self->itemlost ) { - $self->_add_statistic('item_lost'); # should be quite rare when adding item + # should be quite rare when adding item + if ( $self->itemlost ) { + $self->_add_statistic('item_lost'); } } else { # ModItem @@ -189,10 +190,20 @@ sub store { $self->permanent_location( $self->location ); } - if( exists $updated_columns{itemlost} && !$updated_columns{itemlost} && $pre_mod_item->itemlost ) { # item found again - $self->_set_found_trigger($pre_mod_item); # reverse any list item charges if necessary + if ( exists $updated_columns{itemlost} + && !$updated_columns{itemlost} + && $pre_mod_item->itemlost ) + { + # item found + # reverse any list item charges if necessary + $self->_set_found_trigger($pre_mod_item); $self->_add_statistic('item_found'); - } elsif( exists $updated_columns{itemlost} && $updated_columns{itemlost} && !$pre_mod_item->itemlost ) { # item lost + } + elsif (exists $updated_columns{itemlost} + && $updated_columns{itemlost} + && !$pre_mod_item->itemlost ) + { + # item lost $self->_add_statistic('item_lost'); } } @@ -219,16 +230,20 @@ sub store { sub _add_statistic { my ( $self, $type ) = @_; - C4::Stats::UpdateStats({ - type => $type, - branch => C4::Context->userenv ? C4::Context->userenv->{branch} : undef, - borrowernumber => undef, - categorycode => undef, - itemnumber => $self->itemnumber, - ccode => $self->ccode, - itemtype => $self->effective_itemtype, - location => $self->location, - }); + C4::Stats::UpdateStats( + { + type => $type, + branch => C4::Context->userenv + ? C4::Context->userenv->{branch} + : undef, + borrowernumber => undef, + categorycode => undef, + itemnumber => $self->itemnumber, + ccode => $self->ccode, + itemtype => $self->effective_itemtype, + location => $self->location, + } + ); } =head3 delete diff --git a/Koha/Statistic.pm b/Koha/Statistic.pm index 3da51b4cc5..b268b828ef 100644 --- a/Koha/Statistic.pm +++ b/Koha/Statistic.pm @@ -70,39 +70,51 @@ Koha::Statistic - Koha Statistic Object class sub new { my ( $class, $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}; + 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 + $params->{value} //= delete $params->{amount}; # legacy amount parameter supported my $category; - if( grep { $_ eq $params->{type} } @allowed_circulation_types ) { + if ( grep { $_ eq $params->{type} } @allowed_circulation_types ) { $category = 'circulation'; - } elsif( grep { $_ eq $params->{type} } @allowed_accounts_types ) { + } elsif ( grep { $_ eq $params->{type} } @allowed_accounts_types ) { $category = 'accounts'; } else { - Koha::Exceptions::WrongParameter->throw( name => 'type', value => $params->{type} ); + Koha::Exceptions::WrongParameter->throw( + name => 'type', + value => $params->{type} + ); } - my @mandatory_keys = $category eq 'circulation' ? @mandatory_circulation_keys : @mandatory_accounts_keys; + 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} // C4::Context->interface, - }); + 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} // C4::Context->interface, + } + ); } =head3 store @@ -114,12 +126,12 @@ sub new { =cut sub store { - my ( $self ) = @_; + my ($self) = @_; $self->SUPER::store; Koha::PseudonymizedTransaction->new_from_statistic($self)->store - if C4::Context->preference('Pseudonymization') - && $self->borrowernumber # Not a real transaction if the patron does not exist - # For instance can be a transfer, or hold trigger + if C4::Context->preference('Pseudonymization') + && $self->borrowernumber # Not a real transaction if the patron does not exist + # For instance can be a transfer, or hold trigger && grep { $_ eq $self->type } qw(renew issue return onsite_checkout); return $self; } @@ -149,7 +161,7 @@ sub insert { =cut sub item { - my ( $self ) = @_; + my ($self) = @_; return Koha::Items->find( $self->itemnumber ); } diff --git a/t/db_dependent/Koha/Statistics.t b/t/db_dependent/Koha/Statistics.t index 999d1b8567..73735d9353 100755 --- a/t/db_dependent/Koha/Statistics.t +++ b/t/db_dependent/Koha/Statistics.t @@ -28,20 +28,20 @@ use Koha::Statistics; use t::lib::TestBuilder; -our $schema = Koha::Database->new->schema; +our $schema = Koha::Database->new->schema; our $builder = t::lib::TestBuilder->new; -our $test_params = { # No FK checks here - branch => "BRA", - itemnumber => 31, +our $test_params = { # No FK checks here + branch => "BRA", + itemnumber => 31, borrowernumber => 5, - categorycode => 'S', - amount => 5.1, - other => "bla", - itemtype => "BK", - location => "LOC", - ccode => "CODE", - interface => 'INTERFACE', + categorycode => 'S', + amount => 5.1, + other => "bla", + itemtype => "BK", + location => "LOC", + ccode => "CODE", + interface => 'INTERFACE', }; subtest 'Basic Koha object tests' => sub { @@ -52,22 +52,24 @@ subtest 'Basic Koha object tests' => sub { my $item = $builder->build_sample_item; my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); - Koha::Statistic->insert({ - type => 'issue', - branch => $library->branchcode, - itemnumber => $item->itemnumber, - borrowernumber => $patron->borrowernumber, - itemtype => $item->effective_itemtype, - location => $item->location, - ccode => $item->ccode, - interface => C4::Context->interface, - }); + Koha::Statistic->insert( + { + type => 'issue', + branch => $library->branchcode, + itemnumber => $item->itemnumber, + borrowernumber => $patron->borrowernumber, + itemtype => $item->effective_itemtype, + location => $item->location, + ccode => $item->ccode, + interface => C4::Context->interface, + } + ); my $stat = Koha::Statistics->search( { itemnumber => $item->itemnumber } )->next; - is( $stat->borrowernumber, $patron->borrowernumber, 'Patron is there' ); - is( $stat->branch, $library->branchcode, 'Library is there' ); - is( ref( $stat->item ), 'Koha::Item', '->item returns a Koha::Item object' ); - is( $stat->item->itemnumber, $item->itemnumber, '->item works great' ); + is( $stat->borrowernumber, $patron->borrowernumber, 'Patron is there' ); + is( $stat->branch, $library->branchcode, 'Library is there' ); + is( ref( $stat->item ), 'Koha::Item', '->item returns a Koha::Item object' ); + is( $stat->item->itemnumber, $item->itemnumber, '->item works great' ); $schema->storage->txn_rollback; }; @@ -77,25 +79,28 @@ subtest 'Test exceptions in ->new' => sub { $schema->storage->txn_begin; throws_ok { Koha::Statistic->new } 'Koha::Exceptions::BadParameter', '->new called without params'; - #FIXME Should we remove this for sake of consistency? + + #FIXME Should we remove this for sake of consistency? # Type is missing - my $params = { %$test_params }; + my $params = {%$test_params}; throws_ok { Koha::Statistic->new($params) } 'Koha::Exceptions::WrongParameter', '->new called without type'; # Type is not allowed - $params = { %$test_params }; - $params ->{type} = "bla"; + $params = {%$test_params}; + $params->{type} = "bla"; throws_ok { Koha::Statistic->new($params) } 'Koha::Exceptions::WrongParameter', '->new called with wrong type'; # Test mandatory accounts/circulation keys - $params = { %$test_params }; + $params = {%$test_params}; $params->{type} = 'payment'; delete $params->{amount}; - throws_ok { Koha::Statistic->new($params) } 'Koha::Exceptions::MissingParameter', '->new called for accounts without amount'; + throws_ok { Koha::Statistic->new($params) } 'Koha::Exceptions::MissingParameter', + '->new called for accounts without amount'; $params->{type} = 'issue'; delete $params->{itemnumber}; - throws_ok { Koha::Statistic->new($params) } 'Koha::Exceptions::MissingParameter', '->new called for circulation without itemnumber'; + throws_ok { Koha::Statistic->new($params) } 'Koha::Exceptions::MissingParameter', + '->new called for circulation without itemnumber'; $schema->storage->txn_rollback; }; @@ -105,16 +110,16 @@ subtest 'Test ->insert (fka UpdateStats)' => sub { $schema->storage->txn_begin; # save the params in the right database fields - my $statistic = insert_and_fetch({ %$test_params, type => 'return' }); - is( $statistic->branch, $test_params->{branch}, "Check branch" ); - is( $statistic->type, 'return', "Check type" ); + my $statistic = insert_and_fetch( { %$test_params, type => 'return' } ); + is( $statistic->branch, $test_params->{branch}, "Check branch" ); + is( $statistic->type, 'return', "Check type" ); is( $statistic->borrowernumber, $test_params->{borrowernumber}, "Check borrowernumber" ); - is( $statistic->value, $test_params->{amount}, "Check value" ); - is( $statistic->other, $test_params->{other}, "Check other" ); - is( $statistic->itemtype, $test_params->{itemtype}, "Check itemtype" ); - is( $statistic->location, $test_params->{location}, "Check location" ); - is( $statistic->ccode, $test_params->{ccode}, "Check ccode" ); - is( $statistic->interface, $test_params->{interface}, "Check interface" ); + is( $statistic->value, $test_params->{amount}, "Check value" ); + is( $statistic->other, $test_params->{other}, "Check other" ); + is( $statistic->itemtype, $test_params->{itemtype}, "Check itemtype" ); + is( $statistic->location, $test_params->{location}, "Check location" ); + is( $statistic->ccode, $test_params->{ccode}, "Check ccode" ); + is( $statistic->interface, $test_params->{interface}, "Check interface" ); # Test location with undef and empty string my $params = { %$test_params, type => 'return' }; @@ -135,7 +140,7 @@ subtest 'Test ->insert (fka UpdateStats)' => sub { is( $statistic->other, undef, "Other is NULL if passed undef" ); # Test amount versus value; value is the db column, amount is the legacy name (to be deprecated?) - $params = { %$test_params, type => 'return', value => 0 }; + $params = { %$test_params, type => 'return', value => 0 }; $statistic = insert_and_fetch($params); is( $statistic->value, 0, "Value is zero, overriding non-zero amount" ); delete $params->{value}; @@ -146,8 +151,9 @@ subtest 'Test ->insert (fka UpdateStats)' => sub { }; sub insert_and_fetch { - my $params = shift; + my $params = shift; my $statistic = Koha::Statistic->insert($params); - return Koha::Statistics->search({ borrowernumber => $test_params->{borrowernumber} })->last; - # FIXME discard_changes would be nicer, but we dont have a PK (yet) + return Koha::Statistics->search( { borrowernumber => $test_params->{borrowernumber} } )->last; + + # FIXME discard_changes would be nicer, but we dont have a PK (yet) } -- 2.41.0