Attachment 153703 Details for Bug 33608 – Bug 33608: Polish Koha::Statistic further

[patch] Bug 33608: Polish Koha::Statistic further

Bug-33608-Polish-KohaStatistic-further.patch (text/plain), 9.68 KB, created by Marcel de Rooy on 2023-07-20 08:51:13 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2023-07-20 08:51:13 UTC

Size: 9.68 KB

patch

obsolete

>From df5decdf5b69192aaf71316e4ee201319f9db4c5 Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>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 <m.de.rooy@rijksmuseum.nl>
>Signed-off-by: Owen Leonard <oleonard@myacpl.org>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>---
> 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

Actions: View | Diff | Splinter Review

Attachments on bug 33608: 150840 | 150841 | 150842 | 150843 | 150844 | 150845 | 150846 | 151051 | 151052 | 151053 | 151054 | 151055 | 151056 | 151057 | 151913 | 151915 | 151916 | 151917 | 151918 | 151919 | 151920 | 151921 | 151922 | 153653 | 153654 | 153655 | 153656 | 153657 | 153658 | 153659 | 153660 | 153661 | 153698 | 153699 | 153700 | 153701 | 153702 | 153703 | 153704 | 153705 | 153706 | 153707 | 153708 | 153709 | 153710 | 153711 | 153712 | 153713 | 153765 | 157440 | 157441 | 157442 | 157443 | 157444 | 157445 | 157446 | 157447 | 157448 | 157449