Attachment 153706 Details for Bug 33608 – Bug 33608: (QA follow-up) Tidy

[patch] Bug 33608: (QA follow-up) Tidy

Bug-33608-QA-follow-up-Tidy.patch (text/plain), 14.89 KB, created by Marcel de Rooy on 2023-07-20 08:51:22 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

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

Size: 14.89 KB

patch

obsolete

>From 45e1f77c3af9dbf6dac183645823571a1dc4965d Mon Sep 17 00:00:00 2001
>From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>Date: Wed, 19 Jul 2023 15:01:27 +0100
>Subject: [PATCH] Bug 33608: (QA follow-up) Tidy
>Content-Type: text/plain; charset=utf-8
>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>---
> 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 409cfc28a6..6dd07207dd 100644
>--- a/Koha/Statistic.pm
>+++ b/Koha/Statistic.pm
>@@ -70,41 +70,53 @@ 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};
> 
>     if ( $params->{amount} ) {
>-        $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
>@@ -116,12 +128,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;
> }
>@@ -151,7 +163,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.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