Bug 17755: (QA followup) Return $self when appropriate

As failure situations raise exceptions that should be handled outside
the object class, methods should return $self so successive calls can be
chained nicely.

This patch makes methods return $self and adjusts the tests to reflect
this change.

Make sure tests pass:
- Run:
  $ prove t/db_dependent/Koha/Patron/Attribute/Types.t
=> SUCCESS: Tests return green
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/Koha/Object/Limit/Library.pm b/Koha/Object/Limit/Library.pm
index 678782b..7d86b94 100644 (file)
--- a/Koha/Object/Limit/Library.pm
+++ b/Koha/Object/Limit/Library.pm
@@ -19,6 +19,7 @@ use Modern::Perl;
 
 use Koha::Database;
 use Koha::Exceptions;
+use Koha::Libraries;
 
 use Try::Tiny;
 
@@ -50,6 +51,10 @@ my $limits = $object->library_limits();
 
 $object->library_limits( \@branchcodes );
 
+Accessor method for library limits. When updating library limits, it accepts
+a list of branchcodes. If requested to return the current library limits
+it returns a Koha::Libraries object with the corresponding libraries.
+
 =cut
 
 sub library_limits {
@@ -67,6 +72,9 @@ sub library_limits {
 
 my $limits = $object->get_library_limits();
 
+Returns the current library limits in the form of a Koha::Libraries iterator object.
+It returns undef if no library limits defined.
+
 =cut
 
 sub get_library_limits {
@@ -77,7 +85,12 @@ sub get_library_limits {
         { $self->_library_limits->{id} => $self->id } )
         ->get_column( $self->_library_limits->{library} )->all();
 
-    return \@branchcodes;
+    return unless @branchcodes;
+
+    my $filter = [ map { { branchcode => $_ } } @branchcodes ];
+    my $libraries = Koha::Libraries->search( $filter );
+
+    return $libraries;
 }
 
 =head3 add_library_limit
@@ -93,9 +106,8 @@ sub add_library_limit {
         "Required parameter 'branchcode' missing")
         unless $branchcode;
 
-    my $limitation;
     try {
-        $limitation = $self->_library_limit_rs->update_or_create(
+        $self->_library_limit_rs->update_or_create(
             {   $self->_library_limits->{id}      => $self->id,
                 $self->_library_limits->{library} => $branchcode
             }
@@ -105,7 +117,7 @@ sub add_library_limit {
         Koha::Exceptions::CannotAddLibraryLimit->throw( $_->{msg} );
     };
 
-    return $limitation ? 1 : undef;
+    return $self;
 }
 
 =head3 del_library_limit
@@ -145,14 +157,19 @@ $object->replace_library_limits( \@branchcodes );
 sub replace_library_limits {
     my ( $self, $branchcodes ) = @_;
 
-    $self->_library_limit_rs->search(
-        { $self->_library_limits->{id} => $self->id } )->delete;
+    $self->_result->result_source->schema->txn_do(
+        sub {
+            $self->_library_limit_rs->search(
+                { $self->_library_limits->{id} => $self->id } )->delete;
 
-    my @return_values = map { $self->add_library_limit($_) } @$branchcodes;
+            map { $self->add_library_limit($_) } @$branchcodes;
+        }
+    );
 
-    return \@return_values;
+    return $self;
 }
 
+
 =head3 Koha::Objects->_library_limit_rs
 
 Returns the internal resultset for the branch limitation table or creates it if undefined

diff --git a/t/db_dependent/Koha/Patron/Attribute/Types.t b/t/db_dependent/Koha/Patron/Attribute/Types.t
index 8fa73cc..876e908 100644 (file)
--- a/t/db_dependent/Koha/Patron/Attribute/Types.t
+++ b/t/db_dependent/Koha/Patron/Attribute/Types.t
@@ -72,7 +72,7 @@ subtest 'new() tests' => sub {
 
 subtest 'library_limits() tests' => sub {
 
-    plan tests => 5;
+    plan tests => 13;
 
     $schema->storage->txn_begin;
 
@@ -89,43 +89,68 @@ subtest 'library_limits() tests' => sub {
     my $library = $builder->build( { source => 'Branch' } )->{branchcode};
 
     my $library_limits = $attribute_type->library_limits();
-    is_deeply( $library_limits, [],
+    is( $library_limits, undef,
         'No branch limitations defined for attribute type' );
 
     my $print_error = $dbh->{PrintError};
     $dbh->{PrintError} = 0;
 
     throws_ok {
-        $library_limits = $attribute_type->library_limits( ['fake'] );
+        $attribute_type->library_limits( ['fake'] );
     }
     'Koha::Exceptions::CannotAddLibraryLimit',
         'Exception thrown on single invalid branchcode';
+    $library_limits = $attribute_type->library_limits();
+    is( $library_limits, undef,
+        'No branch limitations defined for attribute type' );
 
     throws_ok {
-        $library_limits
-            = $attribute_type->library_limits( [ 'fake', $library ] );
+        $attribute_type->library_limits( [ 'fake', $library ] );
     }
     'Koha::Exceptions::CannotAddLibraryLimit',
         'Exception thrown on invalid branchcode present';
 
+    $library_limits = $attribute_type->library_limits();
+    is( $library_limits, undef,
+        'No branch limitations defined for attribute type' );
+
     $dbh->{PrintError} = $print_error;
 
-    $library_limits = $attribute_type->library_limits( [$library] );
-    is_deeply( $library_limits, [1], 'Library limits correctly set' );
+    $attribute_type->library_limits( [$library] );
+    $library_limits = $attribute_type->library_limits;
+    is( $library_limits->count, 1, 'Library limits correctly set (count)' );
+    my $limit_library = $library_limits->next;
+    ok( $limit_library->isa('Koha::Library'),
+        'Library limits correctly set (type)'
+    );
+    is( $limit_library->branchcode,
+        $library, 'Library limits correctly set (value)' );
 
     my $another_library
         = $builder->build( { source => 'Branch' } )->{branchcode};
-
-    $library_limits
-        = $attribute_type->library_limits( [ $library, $another_library ] );
-    is_deeply( $library_limits, [ 1, 1 ], 'Library limits correctly set' );
+    my @branchcodes_list = ( $library, $another_library );
+
+    $attribute_type->library_limits( \@branchcodes_list );
+    $library_limits = $attribute_type->library_limits;
+    is( $library_limits->count, 2, 'Library limits correctly set (count)' );
+
+    while ( $limit_library = $library_limits->next ) {
+        ok( $limit_library->isa('Koha::Library'),
+            'Library limits correctly set (type)'
+        );
+        ok( eval {
+                grep { $limit_library->branchcode eq $_ } @branchcodes_list;
+            },
+            'Library limits correctly set (values)'
+        );
+    }
 
     $schema->storage->txn_rollback;
 };
 
 subtest 'add_library_limit() tests' => sub {
 
-    plan tests => 3;
+    plan tests => 4;
 
     $schema->storage->txn_begin;
 
@@ -140,12 +165,15 @@ subtest 'add_library_limit() tests' => sub {
     )->store();
 
     throws_ok { $attribute_type->add_library_limit() }
-    'Koha::Exceptions::MissingParameter',
-        'branchcode parameter is mandatory';
+    'Koha::Exceptions::MissingParameter', 'branchcode parameter is mandatory';
 
     my $library = $builder->build( { source => 'Branch' } )->{branchcode};
-    is( $attribute_type->add_library_limit($library),
-        1, 'Library limit successfully added' );
+    $attribute_type->add_library_limit($library);
+    my $rs = Koha::Database->schema->resultset('BorrowerAttributeTypesBranch')
+        ->search( { bat_code => 'code' } );
+    is( $rs->count, 1, 'Library limit successfully added (count)' );
+    is( $rs->next->b_branchcode->branchcode,
+        $library, 'Library limit successfully added (value)' );
 
     my $print_error = $dbh->{PrintError};
     $dbh->{PrintError} = 0;
@@ -209,7 +237,7 @@ subtest 'del_library_limit() tests' => sub {
 
 subtest 'replace_library_limits() tests' => sub {
 
-    plan tests => 6;
+    plan tests => 10;
 
     $schema->storage->txn_begin;
 
@@ -223,26 +251,36 @@ subtest 'replace_library_limits() tests' => sub {
         }
     )->store();
 
-    is_deeply( $attribute_type->replace_library_limits( [] ),
-        [], 'Replacing with empty array returns an empty array as expected' );
-
-    is_deeply( $attribute_type->library_limits(),
-        [], 'Replacing with empty array yields no library limits' );
+    $attribute_type->replace_library_limits( [] );
+    my $library_limits = $attribute_type->library_limits;
+    is( $library_limits, undef, 'Replacing with empty array yields no library limits' );
 
     my $library_1 = $builder->build({ source => 'Branch'})->{branchcode};
     my $library_2 = $builder->build({ source => 'Branch'})->{branchcode};
-
-    is_deeply( $attribute_type->replace_library_limits( [$library_1] ),
-        [ 1 ], 'Successfully adds a single library limit' );
-
-    is_deeply( $attribute_type->library_limits(),
-        [ $library_1 ], 'Library limit correctly set' );
-
-    is_deeply( $attribute_type->replace_library_limits( [$library_1, $library_2] ),
-        [ 1, 1 ], 'Successfully adds two library limit' );
-
-    is_deeply( $attribute_type->library_limits(),
-        [ $library_1, $library_2 ], 'Library limit correctly set' );
+    my $library_3 = $builder->build({ source => 'Branch'})->{branchcode};
+
+    $attribute_type->replace_library_limits( [$library_1] );
+    $library_limits = $attribute_type->library_limits;
+    is( $library_limits->count, 1, 'Successfully adds a single library limit' );
+    my $library_limit = $library_limits->next;
+    is( $library_limit->branchcode, $library_1, 'Library limit correctly set' );
+
+
+    my @branchcodes_list = ($library_1, $library_2, $library_3);
+    $attribute_type->replace_library_limits( [$library_1, $library_2, $library_3] );
+    $library_limits = $attribute_type->library_limits;
+    is( $library_limits->count, 3, 'Successfully adds two library limit' );
+
+    while ( my $limit_library = $library_limits->next ) {
+        ok( $limit_library->isa('Koha::Library'),
+            'Library limits correctly set (type)'
+        );
+        ok( eval {
+                grep { $limit_library->branchcode eq $_ } @branchcodes_list;
+            },
+            'Library limits correctly set (values)'
+        );
+    }
 
     $schema->storage->txn_rollback;
 };