Bug 21336: Do not increase login_attempts after locking
[koha-equinox.git] / t / db_dependent / Koha / Patrons.t
index 7a9b9a5..51f62db 100644 (file)
 
 use Modern::Perl;
 
-use Test::More tests => 22;
+use Test::More tests => 34;
 use Test::Warn;
+use Test::Exception;
+use Test::MockModule;
 use Time::Fake;
 use DateTime;
+use JSON;
 
-use C4::Biblio;
-use C4::Circulation;
-use C4::Members;
 use C4::Circulation;
+use C4::Biblio;
+use C4::Auth qw(checkpw_hash);
 
 use Koha::Holds;
-use Koha::Patron;
 use Koha::Patrons;
 use Koha::Patron::Categories;
 use Koha::Database;
@@ -54,6 +55,7 @@ my $new_patron_1  = Koha::Patron->new(
         surname => 'surname for patron1',
         firstname => 'firstname for patron1',
         userid => 'a_nonexistent_userid_1',
+        flags => 1, # Is superlibrarian
     }
 )->store;
 my $new_patron_2  = Koha::Patron->new(
@@ -66,8 +68,7 @@ my $new_patron_2  = Koha::Patron->new(
     }
 )->store;
 
-C4::Context->_new_userenv('xxx');
-C4::Context->set_userenv(0,0,0,'firstname','surname', $library->{branchcode}, 'Midway Public Library', '', '', '');
+t::lib::Mocks::mock_userenv({ patron => $new_patron_1 });
 
 is( Koha::Patrons->search->count, $nb_of_patrons + 2, 'The 2 patrons should have been added' );
 
@@ -81,7 +82,7 @@ subtest 'library' => sub {
 };
 
 subtest 'guarantees' => sub {
-    plan tests => 8;
+    plan tests => 13;
     my $guarantees = $new_patron_1->guarantees;
     is( ref($guarantees), 'Koha::Patrons', 'Koha::Patron->guarantees should return a Koha::Patrons result set in a scalar context' );
     is( $guarantees->count, 0, 'new_patron_1 should have 0 guarantee' );
@@ -99,6 +100,53 @@ subtest 'guarantees' => sub {
     is( ref(\@guarantees), 'ARRAY', 'Koha::Patron->guarantees should return an array in a list context' );
     is( scalar(@guarantees), 2, 'new_patron_1 should have 2 guarantees' );
     $_->delete for @guarantees;
+
+    #Test return order of guarantees BZ 18635
+    my $categorycode = $builder->build({ source => 'Category' })->{categorycode};
+    my $branchcode = $builder->build({ source => 'Branch' })->{branchcode};
+
+    my $guarantor = $builder->build_object( { class => 'Koha::Patrons' } );
+
+    my $order_guarantee1 = $builder->build_object( { class => 'Koha::Patrons' ,  value => {
+            surname => 'Zebra',
+            guarantorid => $guarantor->borrowernumber
+        }
+    })->borrowernumber;
+
+    my $order_guarantee2 = $builder->build_object( { class => 'Koha::Patrons' ,  value => {
+            surname => 'Yak',
+            guarantorid => $guarantor->borrowernumber
+        }
+    })->borrowernumber;
+
+    my $order_guarantee3 = $builder->build_object( { class => 'Koha::Patrons' ,  value => {
+            surname => 'Xerus',
+            firstname => 'Walrus',
+            guarantorid => $guarantor->borrowernumber
+        }
+    })->borrowernumber;
+
+    my $order_guarantee4 = $builder->build_object( { class => 'Koha::Patrons' ,  value => {
+            surname => 'Xerus',
+            firstname => 'Vulture',
+            guarantorid => $guarantor->borrowernumber
+        }
+    })->borrowernumber;
+
+    my $order_guarantee5 = $builder->build_object( { class => 'Koha::Patrons' ,  value => {
+            surname => 'Xerus',
+            firstname => 'Unicorn',
+            guarantorid => $guarantor->borrowernumber
+        }
+    })->borrowernumber;
+
+    $guarantees = $guarantor->guarantees();
+
+    is( $guarantees->next()->borrowernumber, $order_guarantee5, "Return first guarantor alphabetically" );
+    is( $guarantees->next()->borrowernumber, $order_guarantee4, "Return second guarantor alphabetically" );
+    is( $guarantees->next()->borrowernumber, $order_guarantee3, "Return third guarantor alphabetically" );
+    is( $guarantees->next()->borrowernumber, $order_guarantee2, "Return fourth guarantor alphabetically" );
+    is( $guarantees->next()->borrowernumber, $order_guarantee1, "Return fifth guarantor alphabetically" );
 };
 
 subtest 'category' => sub {
@@ -159,39 +207,12 @@ subtest 'has_overdues' => sub {
     $issue->delete();
 };
 
-subtest 'update_password' => sub {
-    plan tests => 7;
-
-    t::lib::Mocks::mock_preference( 'BorrowersLog', 1 );
-    my $original_userid   = $new_patron_1->userid;
-    my $original_password = $new_patron_1->password;
-    warning_like { $retrieved_patron_1->update_password( $new_patron_2->userid, 'another_password' ) }
-    qr{Duplicate entry},
-      'Koha::Patron->update_password should warn if the userid is already used by another patron';
-    is( Koha::Patrons->find( $new_patron_1->borrowernumber )->userid,   $original_userid,   'Koha::Patron->update_password should not have updated the userid' );
-    is( Koha::Patrons->find( $new_patron_1->borrowernumber )->password, $original_password, 'Koha::Patron->update_password should not have updated the userid' );
-
-    $retrieved_patron_1->update_password( 'another_nonexistent_userid_1', 'another_password' );
-    is( Koha::Patrons->find( $new_patron_1->borrowernumber )->userid,   'another_nonexistent_userid_1', 'Koha::Patron->update_password should have updated the userid' );
-    is( Koha::Patrons->find( $new_patron_1->borrowernumber )->password, 'another_password',             'Koha::Patron->update_password should have updated the password' );
-
-    my $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'CHANGE PASS', object => $new_patron_1->borrowernumber } )->count;
-    is( $number_of_logs, 1, 'With BorrowerLogs, Koha::Patron->update_password should have logged' );
-
-    t::lib::Mocks::mock_preference( 'BorrowersLog', 0 );
-    $retrieved_patron_1->update_password( 'yet_another_nonexistent_userid_1', 'another_password' );
-    $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'CHANGE PASS', object => $new_patron_1->borrowernumber } )->count;
-    is( $number_of_logs, 1, 'With BorrowerLogs, Koha::Patron->update_password should not have logged' );
-};
-
 subtest 'is_expired' => sub {
-    plan tests => 5;
+    plan tests => 4;
     my $patron = $builder->build({ source => 'Borrower' });
     $patron = Koha::Patrons->find( $patron->{borrowernumber} );
     $patron->dateexpiry( undef )->store->discard_changes;
     is( $patron->is_expired, 0, 'Patron should not be considered expired if dateexpiry is not set');
-    $patron->dateexpiry( '0000-00-00' )->store->discard_changes;
-    is( $patron->is_expired, 0, 'Patron should not be considered expired if dateexpiry is not 0000-00-00');
     $patron->dateexpiry( dt_from_string )->store->discard_changes;
     is( $patron->is_expired, 0, 'Patron should not be considered expired if dateexpiry is today');
     $patron->dateexpiry( dt_from_string->add( days => 1 ) )->store->discard_changes;
@@ -203,13 +224,11 @@ subtest 'is_expired' => sub {
 };
 
 subtest 'is_going_to_expire' => sub {
-    plan tests => 9;
+    plan tests => 8;
     my $patron = $builder->build({ source => 'Borrower' });
     $patron = Koha::Patrons->find( $patron->{borrowernumber} );
     $patron->dateexpiry( undef )->store->discard_changes;
     is( $patron->is_going_to_expire, 0, 'Patron should not be considered going to expire if dateexpiry is not set');
-    $patron->dateexpiry( '0000-00-00' )->store->discard_changes;
-    is( $patron->is_going_to_expire, 0, 'Patron should not be considered going to expire if dateexpiry is not 0000-00-00');
 
     t::lib::Mocks::mock_preference('NotifyBorrowerDeparture', 0);
     $patron->dateexpiry( dt_from_string )->store->discard_changes;
@@ -244,16 +263,16 @@ subtest 'is_going_to_expire' => sub {
 
 
 subtest 'renew_account' => sub {
-    plan tests => 36;
+    plan tests => 48;
 
-    for my $date ( '2016-03-31', '2016-11-30', dt_from_string() ) {
+    for my $date ( '2016-03-31', '2016-11-30', '2019-01-31', dt_from_string() ) {
         my $dt = dt_from_string( $date, 'iso' );
         Time::Fake->offset( $dt->epoch );
         my $a_month_ago                = $dt->clone->subtract( months => 1, end_of_month => 'limit' )->truncate( to => 'day' );
         my $a_year_later               = $dt->clone->add( months => 12, end_of_month => 'limit' )->truncate( to => 'day' );
-        my $a_year_later_minus_a_month = $dt->clone->add( months => 11, end_of_month => 'limit' )->truncate( to => 'day' );
+        my $a_year_later_minus_a_month = $a_month_ago->clone->add( months => 12, end_of_month => 'limit' )->truncate( to => 'day' );
         my $a_month_later              = $dt->clone->add( months => 1 , end_of_month => 'limit' )->truncate( to => 'day' );
-        my $a_year_later_plus_a_month  = $dt->clone->add( months => 13, end_of_month => 'limit' )->truncate( to => 'day' );
+        my $a_year_later_plus_a_month  = $a_month_later->clone->add( months => 12, end_of_month => 'limit' )->truncate( to => 'day' );
         my $patron_category = $builder->build(
             {   source => 'Category',
                 value  => {
@@ -267,7 +286,7 @@ subtest 'renew_account' => sub {
                 value  => {
                     dateexpiry   => $a_month_ago,
                     categorycode => $patron_category->{categorycode},
-                    date_renewed => undef, # Force builder to not popular the column for new patron
+                    date_renewed => undef, # Force builder to not populate the column for new patron
                 }
             }
         );
@@ -307,7 +326,7 @@ subtest 'renew_account' => sub {
         $expiry_date = $retrieved_patron->renew_account;
         is( $expiry_date, $a_year_later, "today + 12 months must be $a_year_later" );
         $retrieved_patron = Koha::Patrons->find( $patron->{borrowernumber} );
-        ok( $retrieved_patron->date_renewed, "Date renewed is set when calling renew_account" );
+        is( $retrieved_patron->date_renewed, output_pref({ dt => $dt, dateformat => 'iso', dateonly => 1 }), "Date renewed is set when calling renew_account" );
         $retrieved_expiry_date = $retrieved_patron->dateexpiry;
         is( dt_from_string($retrieved_expiry_date), $a_year_later, "today + 12 months must be $a_year_later" );
         $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'RENEW', object => $retrieved_patron->borrowernumber } )->count;
@@ -378,6 +397,30 @@ subtest "delete" => sub {
     is( $number_of_logs, 1, 'With BorrowerLogs, Koha::Patron->delete should have logged' );
 };
 
+subtest 'Koha::Patrons->delete' => sub {
+    plan tests => 4;
+
+    my $mod_patron = Test::MockModule->new( 'Koha::Patron' );
+    my $moved_to_deleted = 0;
+    $mod_patron->mock( 'move_to_deleted', sub { $moved_to_deleted++; } );
+
+    my $patron1 = $builder->build_object({ class => 'Koha::Patrons' });
+    my $patron2 = $builder->build_object({ class => 'Koha::Patrons' });
+    my $id1 = $patron1->borrowernumber;
+    my $set = Koha::Patrons->search({ borrowernumber => { '>=' => $id1 }});
+    is( $set->count, 2, 'Two patrons found as expected' );
+    is( $set->delete({ move => 1 }), 2, 'Two patrons deleted' );
+    is( $moved_to_deleted, 2, 'Patrons moved to deletedborrowers' );
+
+    # Add again, test if we can raise an exception
+    $mod_patron->mock( 'delete', sub { return -1; } );
+    $patron1 = $builder->build_object({ class => 'Koha::Patrons' });
+    $id1 = $patron1->borrowernumber;
+    $set = Koha::Patrons->search({ borrowernumber => { '>=' => $id1 }});
+    throws_ok { $set->delete } 'Koha::Exceptions::Patron::FailedDelete',
+        'Exception raised for deleting patron';
+};
+
 subtest 'add_enrolment_fee_if_needed' => sub {
     plan tests => 4;
 
@@ -396,42 +439,41 @@ subtest 'add_enrolment_fee_if_needed' => sub {
         branchcode   => $library->{branchcode},
     );
 
-    my $borrowernumber = C4::Members::AddMember(%borrower_data);
+    my $borrowernumber = Koha::Patron->new(\%borrower_data)->store->borrowernumber;
     $borrower_data{borrowernumber} = $borrowernumber;
 
-    my ($total) = C4::Members::GetMemberAccountRecords($borrowernumber);
-    is( $total, $enrolmentfee_K, "New kid pay $enrolmentfee_K" );
+    my $patron = Koha::Patrons->find( $borrowernumber );
+    my $total = $patron->account->balance;
+    is( int($total), int($enrolmentfee_K), "New kid pay $enrolmentfee_K" );
 
     t::lib::Mocks::mock_preference( 'FeeOnChangePatronCategory', 0 );
     $borrower_data{categorycode} = 'J';
-    C4::Members::ModMember(%borrower_data);
-    ($total) = C4::Members::GetMemberAccountRecords($borrowernumber);
-    is( $total, $enrolmentfee_K, "Kid growing and become a juvenile, but shouldn't pay for the upgrade " );
+    $patron->set(\%borrower_data)->store;
+    $total = $patron->account->balance;
+    is( int($total), int($enrolmentfee_K), "Kid growing and become a juvenile, but shouldn't pay for the upgrade " );
 
     $borrower_data{categorycode} = 'K';
-    C4::Members::ModMember(%borrower_data);
+    $patron->set(\%borrower_data)->store;
     t::lib::Mocks::mock_preference( 'FeeOnChangePatronCategory', 1 );
 
     $borrower_data{categorycode} = 'J';
-    C4::Members::ModMember(%borrower_data);
-    ($total) = C4::Members::GetMemberAccountRecords($borrowernumber);
-    is( $total, $enrolmentfee_K + $enrolmentfee_J, "Kid growing and become a juvenile, they should pay " . ( $enrolmentfee_K + $enrolmentfee_J ) );
+    $patron->set(\%borrower_data)->store;
+    $total = $patron->account->balance;
+    is( int($total), int($enrolmentfee_K + $enrolmentfee_J), "Kid growing and become a juvenile, they should pay " . ( $enrolmentfee_K + $enrolmentfee_J ) );
 
     # Check with calling directly Koha::Patron->get_enrolment_fee_if_needed
-    my $patron = Koha::Patrons->find($borrowernumber);
     $patron->categorycode('YA')->store;
-    my $fee = $patron->add_enrolment_fee_if_needed;
-    ($total) = C4::Members::GetMemberAccountRecords($borrowernumber);
-    is( $total,
-        $enrolmentfee_K + $enrolmentfee_J + $enrolmentfee_YA,
+    $total = $patron->account->balance;
+    is( int($total),
+        int($enrolmentfee_K + $enrolmentfee_J + $enrolmentfee_YA),
         "Juvenile growing and become an young adult, they should pay " . ( $enrolmentfee_K + $enrolmentfee_J + $enrolmentfee_YA )
     );
 
     $patron->delete;
 };
 
-subtest 'checkouts + get_overdues' => sub {
-    plan tests => 8;
+subtest 'checkouts + pending_checkouts + get_overdues + old_checkouts' => sub {
+    plan tests => 17;
 
     my $library = $builder->build( { source => 'Branch' } );
     my ($biblionumber_1) = AddBiblio( MARC::Record->new, '' );
@@ -441,7 +483,9 @@ subtest 'checkouts + get_overdues' => sub {
             value  => {
                 homebranch    => $library->{branchcode},
                 holdingbranch => $library->{branchcode},
-                biblionumber  => $biblionumber_1
+                biblionumber  => $biblionumber_1,
+                itemlost      => 0,
+                withdrawn     => 0,
             }
         }
     );
@@ -451,7 +495,9 @@ subtest 'checkouts + get_overdues' => sub {
             value  => {
                 homebranch    => $library->{branchcode},
                 holdingbranch => $library->{branchcode},
-                biblionumber  => $biblionumber_1
+                biblionumber  => $biblionumber_1,
+                itemlost      => 0,
+                withdrawn     => 0,
             }
         }
     );
@@ -462,7 +508,9 @@ subtest 'checkouts + get_overdues' => sub {
             value  => {
                 homebranch    => $library->{branchcode},
                 holdingbranch => $library->{branchcode},
-                biblionumber  => $biblionumber_2
+                biblionumber  => $biblionumber_2,
+                itemlost      => 0,
+                withdrawn     => 0,
             }
         }
     );
@@ -477,12 +525,17 @@ subtest 'checkouts + get_overdues' => sub {
     my $checkouts = $patron->checkouts;
     is( $checkouts->count, 0, 'checkouts should not return any issues for that patron' );
     is( ref($checkouts), 'Koha::Checkouts', 'checkouts should return a Koha::Checkouts object' );
+    my $pending_checkouts = $patron->pending_checkouts;
+    is( $pending_checkouts->count, 0, 'pending_checkouts should not return any issues for that patron' );
+    is( ref($pending_checkouts), 'Koha::Checkouts', 'pending_checkouts should return a Koha::Checkouts object' );
+    my $old_checkouts = $patron->old_checkouts;
+    is( $old_checkouts->count, 0, 'old_checkouts should not return any issues for that patron' );
+    is( ref($old_checkouts), 'Koha::Old::Checkouts', 'old_checkouts should return a Koha::Old::Checkouts object' );
 
     # Not sure how this is useful, but AddIssue pass this variable to different other subroutines
     $patron = Koha::Patrons->find( $patron->borrowernumber )->unblessed;
 
-    my $module = new Test::MockModule('C4::Context');
-    $module->mock( 'userenv', sub { { branch => $library->{branchcode} } } );
+    t::lib::Mocks::mock_userenv({ branchcode => $library->{branchcode} });
 
     AddIssue( $patron, $item_1->{barcode}, DateTime->now->subtract( days => 1 ) );
     AddIssue( $patron, $item_2->{barcode}, DateTime->now->subtract( days => 5 ) );
@@ -492,6 +545,12 @@ subtest 'checkouts + get_overdues' => sub {
     $checkouts = $patron->checkouts;
     is( $checkouts->count, 3, 'checkouts should return 3 issues for that patron' );
     is( ref($checkouts), 'Koha::Checkouts', 'checkouts should return a Koha::Checkouts object' );
+    $pending_checkouts = $patron->pending_checkouts;
+    is( $pending_checkouts->count, 3, 'pending_checkouts should return 3 issues for that patron' );
+    is( ref($pending_checkouts), 'Koha::Checkouts', 'pending_checkouts should return a Koha::Checkouts object' );
+
+    my $first_checkout = $pending_checkouts->next;
+    is( $first_checkout->unblessed_all_relateds->{biblionumber}, $item_3->{biblionumber}, 'pending_checkouts should prefetch values from other tables (here biblio)' );
 
     my $overdues = $patron->get_overdues;
     is( $overdues->count, 2, 'Patron should have 2 overdues');
@@ -499,10 +558,59 @@ subtest 'checkouts + get_overdues' => sub {
     is( $overdues->next->itemnumber, $item_1->{itemnumber}, 'The issue should be returned in the same order as they have been done, first is correct' );
     is( $overdues->next->itemnumber, $item_2->{itemnumber}, 'The issue should be returned in the same order as they have been done, second is correct' );
 
+
+    C4::Circulation::AddReturn( $item_1->{barcode} );
+    C4::Circulation::AddReturn( $item_2->{barcode} );
+    $old_checkouts = $patron->old_checkouts;
+    is( $old_checkouts->count, 2, 'old_checkouts should return 2 old checkouts that patron' );
+    is( ref($old_checkouts), 'Koha::Old::Checkouts', 'old_checkouts should return a Koha::Old::Checkouts object' );
+
     # Clean stuffs
     Koha::Checkouts->search( { borrowernumber => $patron->borrowernumber } )->delete;
     $patron->delete;
-    $module->unmock('userenv');
+};
+
+subtest 'get_routing_lists' => sub {
+    plan tests => 5;
+
+    my $biblio = Koha::Biblio->new()->store();
+    my $subscription = Koha::Subscription->new({
+        biblionumber => $biblio->biblionumber,
+        }
+    )->store;
+
+    my $patron = $builder->build( { source => 'Borrower' } );
+    $patron = Koha::Patrons->find( $patron->{borrowernumber} );
+
+    is( $patron->get_routing_lists->count, 0, 'Retrieves correct number of routing lists: 0' );
+
+    my $routinglist_count = Koha::Subscription::Routinglists->count;
+    my $routinglist = Koha::Subscription::Routinglist->new({
+        borrowernumber   => $patron->borrowernumber,
+        ranking          => 5,
+        subscriptionid   => $subscription->subscriptionid
+    })->store;
+
+    is ($patron->get_routing_lists->count, 1, "Retrieves correct number of routing lists: 1");
+
+    my $routinglists = $patron->get_routing_lists;
+    is ($routinglists->next->ranking, 5, "Retrieves ranking: 5");
+    is( ref($routinglists),   'Koha::Subscription::Routinglists', 'get_routing_lists returns Koha::Subscription::Routinglists' );
+
+    my $subscription2 = Koha::Subscription->new({
+        biblionumber => $biblio->biblionumber,
+        }
+    )->store;
+    my $routinglist2 = Koha::Subscription::Routinglist->new({
+        borrowernumber   => $patron->borrowernumber,
+        ranking          => 1,
+        subscriptionid   => $subscription2->subscriptionid
+    })->store;
+
+    is ($patron->get_routing_lists->count, 2, "Retrieves correct number of routing lists: 2");
+
+    $patron->delete; # Clean up for later tests
+
 };
 
 subtest 'get_age' => sub {
@@ -515,17 +623,17 @@ subtest 'get_age' => sub {
 
     $patron->dateofbirth( undef );
     is( $patron->get_age, undef, 'get_age should return undef if no dateofbirth is defined' );
-    $patron->dateofbirth( $today->clone->add( years => -12, months => -6, days => -1 ) );
+    $patron->dateofbirth( $today->clone->add( years => -12, months => -6, days => -1, end_of_month => 'limit'  ) );
     is( $patron->get_age, 12, 'Patron should be 12' );
-    $patron->dateofbirth( $today->clone->add( years => -18, months => 0, days => 1 ) );
+    $patron->dateofbirth( $today->clone->add( years => -18, months => 0, days => 1, end_of_month => 'limit'  ) );
     is( $patron->get_age, 17, 'Patron should be 17, happy birthday tomorrow!' );
-    $patron->dateofbirth( $today->clone->add( years => -18, months => 0, days => 0 ) );
+    $patron->dateofbirth( $today->clone->add( years => -18, months => 0, days => 0, end_of_month => 'limit'  ) );
     is( $patron->get_age, 18, 'Patron should be 18' );
-    $patron->dateofbirth( $today->clone->add( years => -18, months => -12, days => -31 ) );
+    $patron->dateofbirth( $today->clone->add( years => -18, months => -12, days => -31, end_of_month => 'limit'  ) );
     is( $patron->get_age, 19, 'Patron should be 19' );
-    $patron->dateofbirth( $today->clone->add( years => -18, months => -12, days => -30 ) );
+    $patron->dateofbirth( $today->clone->add( years => -18, months => -12, days => -30, end_of_month => 'limit'  ) );
     is( $patron->get_age, 19, 'Patron should be 19 again' );
-    $patron->dateofbirth( $today->clone->add( years => 0,   months => -1, days => -1 ) );
+    $patron->dateofbirth( $today->clone->add( years => 0,   months => -1, days => -1, end_of_month => 'limit'  ) );
     is( $patron->get_age, 0, 'Patron is a newborn child' );
 
     $patron->delete;
@@ -615,8 +723,8 @@ subtest 'search_upcoming_membership_expires' => sub {
     Koha::Patrons->search({ borrowernumber => { in => [ $patron_1->{borrowernumber}, $patron_2->{borrowernumber}, $patron_3->{borrowernumber} ] } })->delete;
 };
 
-subtest 'holds' => sub {
-    plan tests => 3;
+subtest 'holds and old_holds' => sub {
+    plan tests => 6;
 
     my $library = $builder->build( { source => 'Branch' } );
     my ($biblionumber_1) = AddBiblio( MARC::Record->new, '' );
@@ -673,28 +781,47 @@ subtest 'holds' => sub {
     $holds = $patron->holds;
     is( $holds->count, 2, 'There should be 2 holds placed by this patron' );
 
+    my $old_holds = $patron->old_holds;
+    is( ref($old_holds), 'Koha::Old::Holds',
+        'Koha::Patron->old_holds should return a Koha::Old::Holds objects' );
+    is( $old_holds->count, 0, 'There should not be any old holds yet');
+
+    my $hold = $holds->next;
+    $hold->cancel;
+
+    $old_holds = $patron->old_holds;
+    is( $old_holds->count, 1, 'There should  be 1 old (cancelled) hold');
+
+    $old_holds->delete;
     $holds->delete;
     $patron->delete;
 };
 
+subtest 'notice_email_address' => sub {
+    plan tests => 2;
+
+    my $patron = $builder->build_object({ class => 'Koha::Patrons' });
+
+    t::lib::Mocks::mock_preference( 'AutoEmailPrimaryAddress', 'OFF' );
+    is ($patron->notice_email_address, $patron->email, "Koha::Patron->notice_email_address returns correct value when AutoEmailPrimaryAddress is off");
+
+    t::lib::Mocks::mock_preference( 'AutoEmailPrimaryAddress', 'emailpro' );
+    is ($patron->notice_email_address, $patron->emailpro, "Koha::Patron->notice_email_address returns correct value when AutoEmailPrimaryAddress is emailpro");
+
+    $patron->delete;
+};
+
 subtest 'search_patrons_to_anonymise & anonymise_issue_history' => sub {
     plan tests => 4;
 
     # TODO create a subroutine in t::lib::Mocks
     my $branch = $builder->build({ source => 'Branch' });
-    my $userenv_patron = $builder->build({
-        source => 'Borrower',
-        value  => { branchcode => $branch->{branchcode} },
+    my $userenv_patron = $builder->build_object({
+        class  => 'Koha::Patrons',
+        value  => { branchcode => $branch->{branchcode}, flags => 0 },
     });
-    C4::Context->_new_userenv('DUMMY SESSION');
-    C4::Context->set_userenv(
-        $userenv_patron->{borrowernumber},
-        $userenv_patron->{userid},
-        'usercnum', 'First name', 'Surname',
-        $branch->{branchcode},
-        $branch->{branchname},
-        0,
-    );
+    t::lib::Mocks::mock_userenv({ patron => $userenv_patron });
+
     my $anonymous = $builder->build( { source => 'Borrower', }, );
 
     t::lib::Mocks::mock_preference( 'AnonymousPatron', $anonymous->{borrowernumber} );
@@ -741,8 +868,8 @@ subtest 'search_patrons_to_anonymise & anonymise_issue_history' => sub {
             }
         );
 
-        my ( $returned_1, undef, undef ) = C4::Circulation::AddReturn( $item_1->{barcode}, undef, undef, undef, '2010-10-10' );
-        my ( $returned_2, undef, undef ) = C4::Circulation::AddReturn( $item_2->{barcode}, undef, undef, undef, '2011-11-11' );
+        my ( $returned_1, undef, undef ) = C4::Circulation::AddReturn( $item_1->{barcode}, undef, undef, dt_from_string('2010-10-10') );
+        my ( $returned_2, undef, undef ) = C4::Circulation::AddReturn( $item_2->{barcode}, undef, undef, dt_from_string('2011-11-11') );
         is( $returned_1 && $returned_2, 1, 'The items should have been returned' );
 
         my $patrons_to_anonymise = Koha::Patrons->search_patrons_to_anonymise( { before => '2010-10-11' } )->search( { 'me.borrowernumber' => $patron->{borrowernumber} } );
@@ -805,7 +932,7 @@ subtest 'search_patrons_to_anonymise & anonymise_issue_history' => sub {
             }
         );
 
-        my ( $returned, undef, undef ) = C4::Circulation::AddReturn( $item->{barcode}, undef, undef, undef, '2010-10-10' );
+        my ( $returned, undef, undef ) = C4::Circulation::AddReturn( $item->{barcode}, undef, undef, dt_from_string('2010-10-10') );
         is( $returned, 1, 'The item should have been returned' );
         my $rows_affected = Koha::Patrons->search_patrons_to_anonymise( { before => '2010-10-11' } )->anonymise_issue_history( { before => '2010-10-11' } );
         ok( $rows_affected > 0, 'AnonymiseIssueHistory should not return any error if success' );
@@ -846,7 +973,7 @@ subtest 'search_patrons_to_anonymise & anonymise_issue_history' => sub {
             }
         );
 
-        my ( $returned, undef, undef ) = C4::Circulation::AddReturn( $item->{barcode}, undef, undef, undef, '2010-10-10' );
+        my ( $returned, undef, undef ) = C4::Circulation::AddReturn( $item->{barcode}, undef, undef, dt_from_string('2010-10-10') );
         is( $returned, 1, 'The item should have been returned' );
         my $rows_affected = Koha::Patrons->search_patrons_to_anonymise( { before => '2010-10-11' } )->anonymise_issue_history( { before => '2010-10-11' } );
         ok( $rows_affected > 0, 'AnonymiseIssueHistory should affect at least 1 row' );
@@ -884,17 +1011,117 @@ subtest 'search_patrons_to_anonymise & anonymise_issue_history' => sub {
             }
         );
 
-        my ( $returned, undef, undef ) = C4::Circulation::AddReturn( $item->{barcode}, undef, undef, undef, '2010-10-10' );
+        my ( $returned, undef, undef ) = C4::Circulation::AddReturn( $item->{barcode}, undef, undef, dt_from_string('2010-10-10') );
         is( Koha::Patrons->search_patrons_to_anonymise( { before => '2010-10-11' } )->count, 0 );
         Koha::Patrons->find( $patron->{borrowernumber})->delete;
     };
 
     Koha::Patrons->find( $anonymous->{borrowernumber})->delete;
-    Koha::Patrons->find( $userenv_patron->{borrowernumber})->delete;
+    $userenv_patron->delete;
+
+    # Reset IndependentBranches for further tests
+    t::lib::Mocks::mock_preference('IndependentBranches', 0);
+};
+
+subtest 'libraries_where_can_see_patrons + can_see_patron_infos + search_limited' => sub {
+    plan tests => 3;
+
+    # group1
+    #   + library_11
+    #   + library_12
+    # group2
+    #   + library21
+    $nb_of_patrons = Koha::Patrons->search->count;
+    my $group_1 = Koha::Library::Group->new( { title => 'TEST Group 1', ft_hide_patron_info => 1 } )->store;
+    my $group_2 = Koha::Library::Group->new( { title => 'TEST Group 2', ft_hide_patron_info => 1 } )->store;
+    my $library_11 = $builder->build( { source => 'Branch' } );
+    my $library_12 = $builder->build( { source => 'Branch' } );
+    my $library_21 = $builder->build( { source => 'Branch' } );
+    $library_11 = Koha::Libraries->find( $library_11->{branchcode} );
+    $library_12 = Koha::Libraries->find( $library_12->{branchcode} );
+    $library_21 = Koha::Libraries->find( $library_21->{branchcode} );
+    Koha::Library::Group->new(
+        { branchcode => $library_11->branchcode, parent_id => $group_1->id } )->store;
+    Koha::Library::Group->new(
+        { branchcode => $library_12->branchcode, parent_id => $group_1->id } )->store;
+    Koha::Library::Group->new(
+        { branchcode => $library_21->branchcode, parent_id => $group_2->id } )->store;
+
+    my $sth = C4::Context->dbh->prepare(q|INSERT INTO user_permissions( borrowernumber, module_bit, code ) VALUES (?, 4, ?)|); # 4 for borrowers
+    # 2 patrons from library_11 (group1)
+    # patron_11_1 see patron's infos from outside its group
+    # Setting flags => undef to not be considered as superlibrarian
+    my $patron_11_1 = $builder->build({ source => 'Borrower', value => { branchcode => $library_11->branchcode, flags => undef, }});
+    $patron_11_1 = Koha::Patrons->find( $patron_11_1->{borrowernumber} );
+    $sth->execute( $patron_11_1->borrowernumber, 'edit_borrowers' );
+    $sth->execute( $patron_11_1->borrowernumber, 'view_borrower_infos_from_any_libraries' );
+    # patron_11_2 can only see patron's info from its group
+    my $patron_11_2 = $builder->build({ source => 'Borrower', value => { branchcode => $library_11->branchcode, flags => undef, }});
+    $patron_11_2 = Koha::Patrons->find( $patron_11_2->{borrowernumber} );
+    $sth->execute( $patron_11_2->borrowernumber, 'edit_borrowers' );
+    # 1 patron from library_12 (group1)
+    my $patron_12 = $builder->build({ source => 'Borrower', value => { branchcode => $library_12->branchcode, flags => undef, }});
+    $patron_12 = Koha::Patrons->find( $patron_12->{borrowernumber} );
+    # 1 patron from library_21 (group2) can only see patron's info from its group
+    my $patron_21 = $builder->build({ source => 'Borrower', value => { branchcode => $library_21->branchcode, flags => undef, }});
+    $patron_21 = Koha::Patrons->find( $patron_21->{borrowernumber} );
+    $sth->execute( $patron_21->borrowernumber, 'edit_borrowers' );
+
+    # Pfiou, we can start now!
+    subtest 'libraries_where_can_see_patrons' => sub {
+        plan tests => 3;
+
+        my @branchcodes;
+
+        t::lib::Mocks::mock_userenv({ patron => $patron_11_1 });
+        @branchcodes = $patron_11_1->libraries_where_can_see_patrons;
+        is_deeply( \@branchcodes, [], q|patron_11_1 has view_borrower_infos_from_any_libraries => No restriction| );
+
+        t::lib::Mocks::mock_userenv({ patron => $patron_11_2 });
+        @branchcodes = $patron_11_2->libraries_where_can_see_patrons;
+        is_deeply( \@branchcodes, [ sort ( $library_11->branchcode, $library_12->branchcode ) ], q|patron_11_2 has not view_borrower_infos_from_any_libraries => Can only see patron's from its group| );
+
+        t::lib::Mocks::mock_userenv({ patron => $patron_21 });
+        @branchcodes = $patron_21->libraries_where_can_see_patrons;
+        is_deeply( \@branchcodes, [$library_21->branchcode], q|patron_21 has not view_borrower_infos_from_any_libraries => Can only see patron's from its group| );
+    };
+    subtest 'can_see_patron_infos' => sub {
+        plan tests => 6;
+
+        t::lib::Mocks::mock_userenv({ patron => $patron_11_1 });
+        is( $patron_11_1->can_see_patron_infos( $patron_11_2 ), 1, q|patron_11_1 can see patron_11_2, from its library| );
+        is( $patron_11_1->can_see_patron_infos( $patron_12 ),   1, q|patron_11_1 can see patron_12, from its group| );
+        is( $patron_11_1->can_see_patron_infos( $patron_21 ),   1, q|patron_11_1 can see patron_11_2, from another group| );
+
+        t::lib::Mocks::mock_userenv({ patron => $patron_11_2 });
+        is( $patron_11_2->can_see_patron_infos( $patron_11_1 ), 1, q|patron_11_2 can see patron_11_1, from its library| );
+        is( $patron_11_2->can_see_patron_infos( $patron_12 ),   1, q|patron_11_2 can see patron_12, from its group| );
+        is( $patron_11_2->can_see_patron_infos( $patron_21 ),   0, q|patron_11_2 can NOT see patron_21, from another group| );
+    };
+    subtest 'search_limited' => sub {
+        plan tests => 6;
+
+        t::lib::Mocks::mock_userenv({ patron => $patron_11_1 });
+        my $total_number_of_patrons = $nb_of_patrons + 4; #we added four in these tests
+        is( Koha::Patrons->search->count, $total_number_of_patrons, 'Non-limited search should return all patrons' );
+        is( Koha::Patrons->search_limited->count, $total_number_of_patrons, 'patron_11_1 is allowed to see all patrons' );
+
+        t::lib::Mocks::mock_userenv({ patron => $patron_11_2 });
+        is( Koha::Patrons->search->count, $total_number_of_patrons, 'Non-limited search should return all patrons');
+        is( Koha::Patrons->search_limited->count, 3, 'patron_12_1 is not allowed to see patrons from other groups, only patron_11_1, patron_11_2 and patron_12' );
+
+        t::lib::Mocks::mock_userenv({ patron => $patron_21 });
+        is( Koha::Patrons->search->count, $total_number_of_patrons, 'Non-limited search should return all patrons');
+        is( Koha::Patrons->search_limited->count, 1, 'patron_21 is not allowed to see patrons from other groups, only himself' );
+    };
+    $patron_11_1->delete;
+    $patron_11_2->delete;
+    $patron_12->delete;
+    $patron_21->delete;
 };
 
 subtest 'account_locked' => sub {
-    plan tests => 8;
+    plan tests => 9;
     my $patron = $builder->build({ source => 'Borrower', value => { login_attempts => 0 } });
     $patron = Koha::Patrons->find( $patron->{borrowernumber} );
     for my $value ( undef, '', 0 ) {
@@ -909,12 +1136,492 @@ subtest 'account_locked' => sub {
     is( $patron->account_locked, 0, 'Patron has 2 failed attempts, account should not be considered locked yet' );
     $patron->login_attempts(3)->store;
     is( $patron->account_locked, 1, 'Patron has 3 failed attempts, account should be considered locked yet' );
+    $patron->login_attempts(4)->store;
+    is( $patron->account_locked, 1, 'Patron could not have 4 failed attempts, but account should still be considered locked' );
 
     $patron->delete;
 };
 
+subtest 'is_child | is_adult' => sub {
+    plan tests => 8;
+    my $category = $builder->build_object(
+        {
+            class => 'Koha::Patron::Categories',
+            value => { category_type => 'A' }
+        }
+    );
+    my $patron_adult = $builder->build_object(
+        {
+            class => 'Koha::Patrons',
+            value => { categorycode => $category->categorycode }
+        }
+    );
+    $category = $builder->build_object(
+        {
+            class => 'Koha::Patron::Categories',
+            value => { category_type => 'I' }
+        }
+    );
+    my $patron_adult_i = $builder->build_object(
+        {
+            class => 'Koha::Patrons',
+            value => { categorycode => $category->categorycode }
+        }
+    );
+    $category = $builder->build_object(
+        {
+            class => 'Koha::Patron::Categories',
+            value => { category_type => 'C' }
+        }
+    );
+    my $patron_child = $builder->build_object(
+        {
+            class => 'Koha::Patrons',
+            value => { categorycode => $category->categorycode }
+        }
+    );
+    $category = $builder->build_object(
+        {
+            class => 'Koha::Patron::Categories',
+            value => { category_type => 'O' }
+        }
+    );
+    my $patron_other = $builder->build_object(
+        {
+            class => 'Koha::Patrons',
+            value => { categorycode => $category->categorycode }
+        }
+    );
+    is( $patron_adult->is_adult, 1, 'Patron from category A should be considered adult' );
+    is( $patron_adult_i->is_adult, 1, 'Patron from category I should be considered adult' );
+    is( $patron_child->is_adult, 0, 'Patron from category C should not be considered adult' );
+    is( $patron_other->is_adult, 0, 'Patron from category O should not be considered adult' );
+
+    is( $patron_adult->is_child, 0, 'Patron from category A should be considered child' );
+    is( $patron_adult_i->is_child, 0, 'Patron from category I should be considered child' );
+    is( $patron_child->is_child, 1, 'Patron from category C should not be considered child' );
+    is( $patron_other->is_child, 0, 'Patron from category O should not be considered child' );
+
+    # Clean up
+    $patron_adult->delete;
+    $patron_adult_i->delete;
+    $patron_child->delete;
+    $patron_other->delete;
+};
+
+subtest 'get_overdues' => sub {
+    plan tests => 7;
+
+    my $library = $builder->build( { source => 'Branch' } );
+    my ($biblionumber_1) = AddBiblio( MARC::Record->new, '' );
+    my $item_1 = $builder->build(
+        {
+            source => 'Item',
+            value  => {
+                homebranch    => $library->{branchcode},
+                holdingbranch => $library->{branchcode},
+                biblionumber  => $biblionumber_1
+            }
+        }
+    );
+    my $item_2 = $builder->build(
+        {
+            source => 'Item',
+            value  => {
+                homebranch    => $library->{branchcode},
+                holdingbranch => $library->{branchcode},
+                biblionumber  => $biblionumber_1
+            }
+        }
+    );
+    my ($biblionumber_2) = AddBiblio( MARC::Record->new, '' );
+    my $item_3 = $builder->build(
+        {
+            source => 'Item',
+            value  => {
+                homebranch    => $library->{branchcode},
+                holdingbranch => $library->{branchcode},
+                biblionumber  => $biblionumber_2
+            }
+        }
+    );
+    my $patron = $builder->build(
+        {
+            source => 'Borrower',
+            value  => { branchcode => $library->{branchcode} }
+        }
+    );
+
+    t::lib::Mocks::mock_preference({ branchcode => $library->{branchcode} });
+
+    AddIssue( $patron, $item_1->{barcode}, DateTime->now->subtract( days => 1 ) );
+    AddIssue( $patron, $item_2->{barcode}, DateTime->now->subtract( days => 5 ) );
+    AddIssue( $patron, $item_3->{barcode} );
+
+    $patron = Koha::Patrons->find( $patron->{borrowernumber} );
+    my $overdues = $patron->get_overdues;
+    is( $overdues->count, 2, 'Patron should have 2 overdues');
+    is( $overdues->next->itemnumber, $item_1->{itemnumber}, 'The issue should be returned in the same order as they have been done, first is correct' );
+    is( $overdues->next->itemnumber, $item_2->{itemnumber}, 'The issue should be returned in the same order as they have been done, second is correct' );
+
+    my $o = $overdues->reset->next;
+    my $unblessed_overdue = $o->unblessed_all_relateds;
+    is( exists( $unblessed_overdue->{issuedate} ), 1, 'Fields from the issues table should be filled' );
+    is( exists( $unblessed_overdue->{itemcallnumber} ), 1, 'Fields from the items table should be filled' );
+    is( exists( $unblessed_overdue->{title} ), 1, 'Fields from the biblio table should be filled' );
+    is( exists( $unblessed_overdue->{itemtype} ), 1, 'Fields from the biblioitems table should be filled' );
+
+    # Clean stuffs
+    $patron->checkouts->delete;
+    $patron->delete;
+};
+
+subtest 'userid_is_valid' => sub {
+    plan tests => 9;
+
+    my $library = $builder->build_object( { class => 'Koha::Libraries' } );
+    my $patron_category = $builder->build_object(
+        {
+            class => 'Koha::Patron::Categories',
+            value => { category_type => 'P', enrolmentfee => 0 }
+        }
+    );
+    my %data = (
+        cardnumber   => "123456789",
+        firstname    => "Tomasito",
+        surname      => "None",
+        categorycode => $patron_category->categorycode,
+        branchcode   => $library->branchcode,
+    );
+
+    my $expected_userid_patron_1 = 'tomasito.none';
+    my $borrowernumber = Koha::Patron->new(\%data)->store->borrowernumber;
+    my $patron_1       = Koha::Patrons->find($borrowernumber);
+    is( $patron_1->has_valid_userid, 1, "Should be valid when compared against them self" );
+    is ( $patron_1->userid, $expected_userid_patron_1, 'The userid generated should be the one we expect' );
+
+    $patron_1->userid( 'tomasito.non' );
+    is( $patron_1->has_valid_userid, # FIXME Joubu: What is the difference with the next test?
+        1, 'recently created userid -> unique (borrowernumber passed)' );
+
+    $patron_1->userid( 'tomasitoxxx' );
+    is( $patron_1->has_valid_userid,
+        1, 'non-existent userid -> unique (borrowernumber passed)' );
+    $patron_1->discard_changes; # We compare with the original userid later
+
+    my $patron_not_in_storage = Koha::Patron->new( { userid => '' } );
+    is( $patron_not_in_storage->has_valid_userid,
+        0, 'userid exists for another patron, patron is not in storage yet' );
+
+    $patron_not_in_storage = Koha::Patron->new( { userid => 'tomasitoxxx' } );
+    is( $patron_not_in_storage->has_valid_userid,
+        1, 'non-existent userid, patron is not in storage yet' );
+
+    # Regression tests for BZ12226
+    my $db_patron = Koha::Patron->new( { userid => C4::Context->config('user') } );
+    is( $db_patron->has_valid_userid,
+        0, 'Koha::Patron->has_valid_userid should return 0 for the DB user (Bug 12226)' );
+
+    # Add a new borrower with the same userid but different cardnumber
+    $data{cardnumber} = "987654321";
+    my $new_borrowernumber = Koha::Patron->new(\%data)->store->borrowernumber;
+    my $patron_2 = Koha::Patrons->find($new_borrowernumber);
+    $patron_2->userid($patron_1->userid);
+    is( $patron_2->has_valid_userid,
+        0, 'The userid is already in used, it cannot be used for another patron' );
+
+    my $new_userid = 'a_user_id';
+    $data{cardnumber} = "234567890";
+    $data{userid}     = 'a_user_id';
+    $borrowernumber   = Koha::Patron->new(\%data)->store->borrowernumber;
+    my $patron_3 = Koha::Patrons->find($borrowernumber);
+    is( $patron_3->userid, $new_userid,
+        'Koha::Patron->store should insert the given userid' );
+
+    # Cleanup
+    $patron_1->delete;
+    $patron_2->delete;
+    $patron_3->delete;
+};
+
+subtest 'generate_userid' => sub {
+    plan tests => 7;
+
+    my $library = $builder->build_object( { class => 'Koha::Libraries' } );
+    my $patron_category = $builder->build_object(
+        {
+            class => 'Koha::Patron::Categories',
+            value => { category_type => 'P', enrolmentfee => 0 }
+        }
+    );
+    my %data = (
+        cardnumber   => "123456789",
+        firstname    => "Tomasito",
+        surname      => "None",
+        categorycode => $patron_category->categorycode,
+        branchcode   => $library->branchcode,
+    );
+
+    my $expected_userid_patron_1 = 'tomasito.none';
+    my $new_patron = Koha::Patron->new({ firstname => $data{firstname}, surname => $data{surname} } );
+    $new_patron->generate_userid;
+    my $userid = $new_patron->userid;
+    is( $userid, $expected_userid_patron_1, 'generate_userid should generate the userid we expect' );
+    my $borrowernumber = Koha::Patron->new(\%data)->store->borrowernumber;
+    my $patron_1 = Koha::Patrons->find($borrowernumber);
+    is ( $patron_1->userid, $expected_userid_patron_1, 'The userid generated should be the one we expect' );
+
+    $new_patron->generate_userid;
+    $userid = $new_patron->userid;
+    is( $userid, $expected_userid_patron_1 . '1', 'generate_userid should generate the userid we expect' );
+    $data{cardnumber} = '987654321';
+    my $new_borrowernumber = Koha::Patron->new(\%data)->store->borrowernumber;
+    my $patron_2 = Koha::Patrons->find($new_borrowernumber);
+    isnt( $patron_2->userid, 'tomasito',
+        "Patron with duplicate userid has new userid generated" );
+    is( $patron_2->userid, $expected_userid_patron_1 . '1', # TODO we could make that configurable
+        "Patron with duplicate userid has new userid generated (1 is appened" );
+
+    $new_patron->generate_userid;
+    $userid = $new_patron->userid;
+    is( $userid, $expected_userid_patron_1 . '2', 'generate_userid should generate the userid we expect' );
+
+    $patron_1 = Koha::Patrons->find($borrowernumber);
+    $patron_1->userid(undef);
+    $patron_1->generate_userid;
+    $userid = $patron_1->userid;
+    is( $userid, $expected_userid_patron_1, 'generate_userid should generate the userid we expect' );
+
+    # Cleanup
+    $patron_1->delete;
+    $patron_2->delete;
+};
+
+subtest 'attributes' => sub {
+    plan tests => 2;
+
+    my $library1 = Koha::Library->new({
+        branchcode => 'LIBPATRON',
+        branchname => 'Library of testing patron',
+    })->store;
+
+    my $library2 = Koha::Library->new({
+        branchcode => 'LIBATTR',
+        branchname => 'Library for testing attribute',
+    })->store;
+
+    my $category = Koha::Patron::Category->new({
+        categorycode => 'CAT1',
+        description => 'Category 1',
+    })->store;
+
+    my $patron = Koha::Patron->new({
+        firstname => 'Patron',
+        surname => 'with attributes',
+        branchcode => 'LIBPATRON',
+        categorycode => 'CAT1',
+    })->store;
+
+    my $attribute_type1 = Koha::Patron::Attribute::Type->new({
+        code => 'CODE_A',
+        description => 'Code A desciption',
+    })->store;
+
+    my $attribute_type2 = Koha::Patron::Attribute::Type->new({
+        code => 'CODE_B',
+        description => 'Code A desciption',
+    })->store;
+
+    $attribute_type2->library_limits ( [ $library2->branchcode ] );
+
+    Koha::Patron::Attribute->new({ borrowernumber => $patron->borrowernumber, code => $attribute_type1->code, attribute => 'value 1' } )->store();
+    Koha::Patron::Attribute->new({ borrowernumber => $patron->borrowernumber, code => $attribute_type2->code, attribute => 'value 2' } )->store();
+
+    is( $patron->attributes->count, 1, 'There should be one attribute');
+
+    $attribute_type2->library_limits ( [ $library1->branchcode ] );
+
+    is( $patron->attributes->count, 2, 'There should be 2 attributes');
+
+    $patron->delete;
+};
+
+$nb_of_patrons = Koha::Patrons->search->count;
 $retrieved_patron_1->delete;
-is( Koha::Patrons->search->count, $nb_of_patrons + 1, 'Delete should have deleted the patron' );
+is( Koha::Patrons->search->count, $nb_of_patrons - 1, 'Delete should have deleted the patron' );
+
+subtest 'BorrowersLog tests' => sub {
+    plan tests => 4;
+
+    t::lib::Mocks::mock_preference( 'BorrowersLog', 1 );
+    my $patron = $builder->build_object( { class => 'Koha::Patrons' } );
+
+    my $cardnumber = $patron->cardnumber;
+    $patron->set( { cardnumber => 'TESTCARDNUMBER' });
+    $patron->store;
+
+    my @logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'MODIFY', object => $patron->borrowernumber } );
+    my $log_info = from_json( $logs[0]->info );
+    is( $log_info->{cardnumber}->{after}, 'TESTCARDNUMBER', 'Got correct new cardnumber' );
+    is( $log_info->{cardnumber}->{before}, $cardnumber, 'Got correct old cardnumber' );
+    is( scalar @logs, 1, 'With BorrowerLogs, one detailed MODIFY action should be logged for the modification.' );
+
+    t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', 1 );
+    $patron->track_login();
+    @logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'MODIFY', object => $patron->borrowernumber } );
+    is( scalar @logs, 1, 'With BorrowerLogs and TrackLastPatronActivity we should not spam the logs');
+};
 
 $schema->storage->txn_rollback;
 
+subtest 'Test Koha::Patrons::merge' => sub {
+    plan tests => 110;
+
+    my $schema = Koha::Database->new()->schema();
+
+    my $resultsets = $Koha::Patron::RESULTSET_PATRON_ID_MAPPING;
+
+    $schema->storage->txn_begin;
+
+    my $keeper  = $builder->build_object({ class => 'Koha::Patrons' });
+    my $loser_1 = $builder->build({ source => 'Borrower' })->{borrowernumber};
+    my $loser_2 = $builder->build({ source => 'Borrower' })->{borrowernumber};
+
+    while (my ($r, $field) = each(%$resultsets)) {
+        $builder->build({ source => $r, value => { $field => $keeper->id } });
+        $builder->build({ source => $r, value => { $field => $loser_1 } });
+        $builder->build({ source => $r, value => { $field => $loser_2 } });
+
+        my $keeper_rs =
+          $schema->resultset($r)->search( { $field => $keeper->id } );
+        is( $keeper_rs->count(), 1, "Found 1 $r rows for keeper" );
+
+        my $loser_1_rs =
+          $schema->resultset($r)->search( { $field => $loser_1 } );
+        is( $loser_1_rs->count(), 1, "Found 1 $r rows for loser_1" );
+
+        my $loser_2_rs =
+          $schema->resultset($r)->search( { $field => $loser_2 } );
+        is( $loser_2_rs->count(), 1, "Found 1 $r rows for loser_2" );
+    }
+
+    my $results = $keeper->merge_with([ $loser_1, $loser_2 ]);
+
+    while (my ($r, $field) = each(%$resultsets)) {
+        my $keeper_rs =
+          $schema->resultset($r)->search( {$field => $keeper->id } );
+        is( $keeper_rs->count(), 3, "Found 2 $r rows for keeper" );
+    }
+
+    is( Koha::Patrons->find($loser_1), undef, 'Loser 1 has been deleted' );
+    is( Koha::Patrons->find($loser_2), undef, 'Loser 2 has been deleted' );
+
+    $schema->storage->txn_rollback;
+};
+
+subtest '->store' => sub {
+    plan tests => 3;
+    my $schema = Koha::Database->new->schema;
+    $schema->storage->txn_begin;
+
+    my $print_error = $schema->storage->dbh->{PrintError};
+    $schema->storage->dbh->{PrintError} = 0; ; # FIXME This does not longer work - because of the transaction in Koha::Patron->store?
+
+    my $patron_1 = $builder->build_object({class=> 'Koha::Patrons'});
+    my $patron_2 = $builder->build_object({class=> 'Koha::Patrons'});
+
+    throws_ok
+        { $patron_2->userid($patron_1->userid)->store; }
+        'Koha::Exceptions::Object::DuplicateID',
+        'Koha::Patron->store raises an exception on duplicate ID';
+
+    # Test password
+    t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 );
+    my $password = 'password';
+    $patron_1->set_password({ password => $password });
+    like( $patron_1->password, qr|^\$2|, 'Password should be hashed using bcrypt (start with $2)' );
+    my $digest = $patron_1->password;
+    $patron_1->surname('xxx')->store;
+    is( $patron_1->password, $digest, 'Password should not have changed on ->store');
+
+    $schema->storage->dbh->{PrintError} = $print_error;
+    $schema->storage->txn_rollback;
+};
+
+subtest '->set_password' => sub {
+
+    plan tests => 14;
+
+    $schema->storage->txn_begin;
+
+    my $patron = $builder->build_object( { class => 'Koha::Patrons', value => { login_attempts => 3 } } );
+
+    # Disable logging password changes for this tests
+    t::lib::Mocks::mock_preference( 'BorrowersLog', 0 );
+
+    # Password-length tests
+    t::lib::Mocks::mock_preference( 'minPasswordLength', undef );
+    throws_ok { $patron->set_password({ password => 'ab' }); }
+        'Koha::Exceptions::Password::TooShort',
+        'minPasswordLength is undef, fall back to 3, fail test';
+    is( "$@",
+        'Password length (2) is shorter than required (3)',
+        'Exception parameters passed correctly'
+    );
+
+    t::lib::Mocks::mock_preference( 'minPasswordLength', 2 );
+    throws_ok { $patron->set_password({ password => 'ab' }); }
+        'Koha::Exceptions::Password::TooShort',
+        'minPasswordLength is 2, fall back to 3, fail test';
+
+    t::lib::Mocks::mock_preference( 'minPasswordLength', 5 );
+    throws_ok { $patron->set_password({ password => 'abcb' }); }
+        'Koha::Exceptions::Password::TooShort',
+        'minPasswordLength is 5, fail test';
+
+    # Trailing spaces tests
+    throws_ok { $patron->set_password({ password => 'abcD12d   ' }); }
+        'Koha::Exceptions::Password::WhitespaceCharacters',
+        'Password contains trailing spaces, exception is thrown';
+
+    # Require strong password tests
+    t::lib::Mocks::mock_preference( 'RequireStrongPassword', 1 );
+    throws_ok { $patron->set_password({ password => 'abcd   a' }); }
+        'Koha::Exceptions::Password::TooWeak',
+        'Password is too weak, exception is thrown';
+
+    # Refresh patron from DB, just to make sure
+    $patron->discard_changes;
+    is( $patron->login_attempts, 3, 'Previous tests kept login attemps count' );
+
+    $patron->set_password({ password => 'abcD12 34' });
+    $patron->discard_changes;
+
+    is( $patron->login_attempts, 0, 'Changing the password resets the login attempts count' );
+
+    lives_ok { $patron->set_password({ password => 'abcd   a', skip_validation => 1 }) }
+        'Password is weak, but skip_validation was passed, so no exception thrown';
+
+    # Completeness
+    t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 );
+    $patron->login_attempts(3)->store;
+    my $old_digest = $patron->password;
+    $patron->set_password({ password => 'abcd   a' });
+    $patron->discard_changes;
+
+    isnt( $patron->password, $old_digest, 'Password has been updated' );
+    ok( checkpw_hash('abcd   a', $patron->password), 'Password hash is correct' );
+    is( $patron->login_attempts, 0, 'Login attemps have been reset' );
+
+    my $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'CHANGE PASS', object => $patron->borrowernumber } )->count;
+    is( $number_of_logs, 0, 'Without BorrowerLogs, Koha::Patron->set_password doesn\'t log password changes' );
+
+    # Enable logging password changes
+    t::lib::Mocks::mock_preference( 'BorrowersLog', 1 );
+    $patron->set_password({ password => 'abcd   b' });
+
+    $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'CHANGE PASS', object => $patron->borrowernumber } )->count;
+    is( $number_of_logs, 1, 'With BorrowerLogs, Koha::Patron->set_password does log password changes' );
+
+    $schema->storage->txn_rollback;
+};