Attachment 135899 Details for Bug 11889 – Bug 11889: (follow-up) Get rid of FIXME in Koha::Patron

[patch] Bug 11889: (follow-up) Get rid of FIXME in Koha::Patron

Bug-11889-follow-up-Get-rid-of-FIXME-in-KohaPatron.patch (text/plain), 12.37 KB, created by Martin Renvoize (ashimema) on 2022-06-09 14:58:30 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2022-06-09 14:58:30 UTC

Size: 12.37 KB

patch

obsolete

>From 7ce42d330bf64fd52193dedeaef2d3ac913f6563 Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Thu, 9 Jun 2022 07:24:31 +0000
>Subject: [PATCH] Bug 11889: (follow-up) Get rid of FIXME in Koha::Patron
>
>Instead of get_lists, I added ->virtualshelves.
>
>I removed the addition of the suffix to the shelf name here; if we
>really want that (doubts), we should do it pref based. Removing a
>staff member with public lists would result in weird names with
>shelf numbers on the OPAC. (Should be very easy to restore.)
>
>Used this opportunity to refactor the patron->delete subtest a bit:
>switching to build_object, removing the 'retrieved patrons',
>differentiating between staff patron and patron for sharing,
>adding a few tests.
>
>Test plan:
>Enable transfer ownership. Delete a patron with lists in interface.
>Check what happened.
>Run t/db_dependent/Koha/Patrons.t.
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>---
> Koha/Patron.pm                |  47 +++++++--------
> t/db_dependent/Koha/Patrons.t | 108 +++++++++++++++-------------------
> 2 files changed, 73 insertions(+), 82 deletions(-)
>
>diff --git a/Koha/Patron.pm b/Koha/Patron.pm
>index 36e0a2cedc..edf2054782 100644
>--- a/Koha/Patron.pm
>+++ b/Koha/Patron.pm
>@@ -388,32 +388,22 @@ sub delete {
>                 $hold->cancel;
>             }
> 
>-            # FIXME Could be $patron->get_lists
>-            # If ListOwnershipUponPatronDeletion = transfer, change ownership of all
>-            # public and shared lists to the user who deleted them.
>-            if ( C4::Context->preference('ListOwnershipUponPatronDeletion') eq 'transfer' ) {
>-                my $userenv = C4::Context->userenv();
>-                my $usernumber = (ref($userenv) eq 'HASH') ? $userenv->{'number'} : 0;
>-                my @publiclists = Koha::Virtualshelves->get_public_shelves->as_list;
>-                my @sharedlists = Koha::Virtualshelves->get_shared_shelves({ borrowernumber => $self->borrowernumber })->as_list;
>-                foreach my $plist ( @publiclists ) {
>-                    if ( $plist->owner == $self->borrowernumber ) {
>-                        my $unique_name = $plist->shelfname . '_' . $self->borrowernumber;
>-                        $plist->set({ owner => $usernumber, shelfname => $unique_name })->store;
>-                    }
>-                }
>-                foreach my $slist ( @sharedlists ) {
>-                    my $unique_name = $slist->shelfname . '_' . $self->borrowernumber;
>-                    $slist->set({ owner => $usernumber, shelfname => $unique_name })->store;
>-                    # if staff member had a share, remove it
>-                    $slist->remove_share( $usernumber );
>+            # Handle lists (virtualshelves)
>+            my $userenv = C4::Context->userenv;
>+            my $usernumber = ref($userenv) eq 'HASH' ? $userenv->{'number'} : 0;
>+            my $lists = $self->virtualshelves;
>+            while( my $list = $lists->next ) {
>+                # if staff member had a share, remove it
>+                $list->remove_share( $usernumber ) if $usernumber && $list->is_private;
>+                if( C4::Context->preference('ListOwnershipUponPatronDeletion') eq 'transfer' &&
>+                    $usernumber && ( $list->is_public || $list->is_shared ))
>+                {
>+                    $list->set({ owner => $usernumber })->store; # transfer ownership of public/shared list
>+                } else { # delete
>+                    $list->delete;
>                 }
>             }
> 
>-            # Delete any remaining lists that this user is an owner of (always private lists,
>-            # only public and shared lists if ListOwnershipUponPatronDeletion = delete)
>-            $_->delete for Koha::Virtualshelves->search({ owner => $self->borrowernumber })->as_list;
>-
>             # We cannot have a FK on borrower_modifications.borrowernumber, the table is also used
>             # for patron selfreg
>             $_->delete for Koha::Patron::Modifications->search( { borrowernumber => $self->borrowernumber } )->as_list;
>@@ -2196,6 +2186,17 @@ sub decoded_secret {
>     return $self->secret;
> }
> 
>+=head3 virtualshelves
>+
>+    my $shelves = $patron->virtualshelves;
>+
>+=cut
>+
>+sub virtualshelves {
>+    my $self = shift;
>+    return Koha::Virtualshelves->_new_from_dbic( scalar $self->_result->virtualshelves );
>+}
>+
> =head2 Internal methods
> 
> =head3 _type
>diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t
>index aeb5ce17ab..b7b544c269 100755
>--- a/t/db_dependent/Koha/Patrons.t
>+++ b/t/db_dependent/Koha/Patrons.t
>@@ -460,91 +460,81 @@ subtest "move_to_deleted" => sub {
> };
> 
> subtest "delete" => sub {
>-    plan tests => 11;
>+    plan tests => 13;
>     t::lib::Mocks::mock_preference( 'BorrowersLog', 1 );
>     t::lib::Mocks::mock_preference( 'ListOwnershipUponPatronDeletion', 'transfer' );
>     Koha::Virtualshelves->delete;
> 
>+    my $patron = $builder->build_object({ class => 'Koha::Patrons' });
>+    my $patron_for_sharing = $builder->build_object({ class => 'Koha::Patrons' });
>     my $staff_patron = $builder->build_object({ class => 'Koha::Patrons' });
>     t::lib::Mocks::mock_userenv({ patron => $staff_patron });
> 
>-    my $patron           = $builder->build( { source => 'Borrower' } );
>-    my $patron_for_sharing = $staff_patron->borrowernumber;
>-    my $retrieved_patron = Koha::Patrons->find( $patron->{borrowernumber} );
>-    my $hold             = $builder->build(
>-        {   source => 'Reserve',
>-            value => { borrowernumber => $patron->{borrowernumber} }
>-        }
>-    );
>-    my $modification = $builder->build_object({ class => 'Koha::Patron::Modifications', value => { borrowernumber => $patron->{borrowernumber} } });
>+    my $hold = $builder->build_object({ class => 'Koha::Holds', value => { borrowernumber => $patron->borrowernumber } });
>+    my $modification = $builder->build_object({ class => 'Koha::Patron::Modifications', value => { borrowernumber => $patron->borrowernumber } });
>     my $private_list = Koha::Virtualshelf->new({
>-            shelfname => "private",
>-            owner => $patron->{borrowernumber},
>-            public => 0,
>-        }
>-    )->store;
>+        shelfname => "private",
>+        owner => $patron->borrowernumber,
>+        public => 0,
>+    })->store;
>     my $public_list = Koha::Virtualshelf->new({
>-            shelfname => "public",
>-            owner => $patron->{borrowernumber},
>-            public => 1,
>-        }
>-    )->store;
>+        shelfname => "public",
>+        owner => $patron->borrowernumber,
>+        public => 1,
>+    })->store;
>     my $list_to_share = Koha::Virtualshelf->new({
>-            shelfname => "shared",
>-            owner => $patron->{borrowernumber},
>-            public => 0,
>-        }
>-    )->store;
>+        shelfname => "shared",
>+        owner => $patron->borrowernumber,
>+        public => 0,
>+    })->store;
> 
>-    my $shared_shelf = eval { $list_to_share->share("valid key")->accept("valid key", $patron_for_sharing) };
>-    my $deleted = $retrieved_patron->delete;
>+    $list_to_share->share("valid key")->accept( "valid key", $patron_for_sharing->borrowernumber );
>+    $list_to_share->share("valid key")->accept( "valid key", $staff_patron->borrowernumber ); # this share should be removed at deletion too
>+    my $deleted = $patron->delete;
>     is( ref($deleted), 'Koha::Patron', 'Koha::Patron->delete should return the deleted patron object if the patron has been correctly deleted' );
>+    ok( $patron->borrowernumber, 'Still have the deleted borrowernumber' );
> 
>-    is( Koha::Patrons->find( $patron->{borrowernumber} ), undef, 'Koha::Patron->delete should have deleted the patron' );
>+    is( Koha::Patrons->find( $patron->borrowernumber ), undef, 'Koha::Patron->delete should have deleted the patron' );
> 
>-    is (Koha::Old::Holds->search( { reserve_id => $hold->{ reserve_id } } )->count, 1, q|Koha::Patron->delete should have cancelled patron's holds| );
>+    is (Koha::Old::Holds->search({ reserve_id => $hold->reserve_id })->count, 1, q|Koha::Patron->delete should have cancelled patron's holds| );
> 
>-    is( Koha::Holds->search( { borrowernumber => $patron->{borrowernumber} } )->count, 0, q|Koha::Patron->delete should have cancelled patron's holds 2| );
>+    is( Koha::Holds->search( { borrowernumber => $patron->borrowernumber } )->count, 0, q|Koha::Patron->delete should have cancelled patron's holds 2| );
> 
>-    my $transferred_lists = Koha::Virtualshelves->search({ owner => $patron_for_sharing })->count;
>+    my $transferred_lists = Koha::Virtualshelves->search({ owner => $staff_patron->borrowernumber })->count;
>     is( $transferred_lists, 2, 'Public and shared lists should stay in database under a different owner with a unique name, while private lists delete, with ListOwnershipPatronDeletion set to Transfer');
>-    is( Koha::Virtualshelfshares->search({ borrowernumber => $patron_for_sharing })->count, 0, "New owner of list should have shares removed" );
>-    is( Koha::Virtualshelves->search({ owner => $patron->{borrowernumber} })->count, 0, q|Koha::Patron->delete should have deleted patron's lists/removed their ownership| );
>+    is( Koha::Virtualshelfshares->search({ borrowernumber => $staff_patron->borrowernumber })->count, 0, "New owner of list should have shares removed" );
>+    is( Koha::Virtualshelfshares->search({ borrowernumber => $patron_for_sharing->borrowernumber })->count, 1, "But the other share is still there" );
>+    is( Koha::Virtualshelves->search({ owner => $patron->borrowernumber })->count, 0, q|Koha::Patron->delete should have deleted patron's lists/removed their ownership| );
> 
>-    is( Koha::Patron::Modifications->search( { borrowernumber => $patron->{borrowernumber} } )->count, 0, q|Koha::Patron->delete should have deleted patron's modifications| );
>+    is( Koha::Patron::Modifications->search( { borrowernumber => $patron->borrowernumber } )->count, 0, q|Koha::Patron->delete should have deleted patron's modifications| );
> 
>-    my $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'DELETE', object => $retrieved_patron->borrowernumber } )->count;
>+    my $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'DELETE', object => $patron->borrowernumber } )->count;
>     is( $number_of_logs, 1, 'With BorrowerLogs, Koha::Patron->delete should have logged' );
> 
>     t::lib::Mocks::mock_preference( 'ListOwnershipUponPatronDeletion', 'delete' );
>-    Koha::Virtualshelves->search({})->delete;
>-    my $patron2           = $builder->build( { source => 'Borrower' } );
>-    my $retrieved_patron2 = Koha::Patrons->find( $patron2->{borrowernumber} );
>+    Koha::Virtualshelves->delete;
>+    my $patron2 = $builder->build_object({ class => 'Koha::Patrons' });
>     my $private_list2 = Koha::Virtualshelf->new({
>-            shelfname => "private",
>-            owner => $patron2->{borrowernumber},
>-            public => 0,
>-        }
>-    )->store;
>+         shelfname => "private",
>+         owner => $patron2->borrowernumber,
>+         public => 0,
>+    })->store;
>     my $public_list2 = Koha::Virtualshelf->new({
>-            shelfname => "public",
>-            owner => $patron2->{borrowernumber},
>-            public => 1,
>-        }
>-    )->store;
>+        shelfname => "public",
>+        owner => $patron2->borrowernumber,
>+        public => 1,
>+    })->store;
>     my $list_to_share2 = Koha::Virtualshelf->new({
>-            shelfname => "shared",
>-            owner => $patron2->{borrowernumber},
>-            public => 0,
>-        }
>-    )->store;
>-
>-    my $shared_shelf2 = eval { $list_to_share2->share("valid key") };
>-    my $deleted2 = $retrieved_patron2->delete;
>+        shelfname => "shared",
>+        owner => $patron2->borrowernumber,
>+        public => 0,
>+    })->store;
>+    $list_to_share2->share("valid key")->accept( "valid key", $patron_for_sharing->borrowernumber );
> 
>-    is( Koha::Virtualshelves->search( { owner => $patron2->{borrowernumber} } )->count, 0, q|Koha::Patron->delete should have deleted patron's lists| );
>-    $transferred_lists = Koha::Virtualshelves->search({})->count;
>-    is( $transferred_lists, 0, 'All lists should be deleted with ListOwnershipUponPatronDeletion set to Delete');
>+    # Delete patron2, check if shelves and shares are now empty
>+    $patron2->delete;
>+    is( Koha::Virtualshelves->count, 0, 'All lists should be gone now' );
>+    is( Koha::Virtualshelfshares->count, 0, 'All shares should be gone too' );
> };
> 
> subtest 'Koha::Patrons->delete' => sub {
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 11889: 25972 | 60224 | 60306 | 60307 | 60308 | 60481 | 61319 | 61320 | 61321 | 61322 | 61451 | 61452 | 61453 | 61454 | 108244 | 108245 | 108246 | 108247 | 135808 | 135809 | 135810 | 135811 | 135812 | 135813 | 135814 | 135815 | 135816 | 135817 | 135818 | 135819 | 135820 | 135821 | 135857 | 135858 | 135859 | 135860 | 135861 | 135862 | 135863 | 135864 | 135865 | 135866 | 135867 | 135868 | 135869 | 135870 | 135871 | 135872 | 135873 | 135874 | 135875 | 135876 | 135891 | 135892 | 135893 | 135894 | 135895 | 135896 | 135897 | 135898 | 135899 | 135900 | 135901