From b70ae1d50f07d1449630bf738bd0e9d53d542c82 Mon Sep 17 00:00:00 2001
From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Date: Wed, 22 Jun 2022 12:14:37 +0000
Subject: [PATCH] Bug 30933: (follow-up) Use cannot_be_transferred in shelves
 scripts

Test plan:
Verify if transfer shared list on OPAC still works as expected.
Same for intranet counterpart for public lists.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
---
 opac/opac-shelves.pl      | 21 +++++++++------------
 virtualshelves/shelves.pl | 25 +++++++++++++------------
 2 files changed, 22 insertions(+), 24 deletions(-)

diff --git a/opac/opac-shelves.pl b/opac/opac-shelves.pl
index bc7b70d70a..15ee4da412 100755
--- a/opac/opac-shelves.pl
+++ b/opac/opac-shelves.pl
@@ -262,13 +262,11 @@ if ( $op eq 'add_form' ) {
     $shelfnumber = $query->param('shelfnumber');
     $shelf = Koha::Virtualshelves->find($shelfnumber) if $shelfnumber;
     my $new_owner = $query->param('new_owner'); # borrowernumber or undef
+    my $error_code = $shelf
+        ? $shelf->cannot_be_transferred({ by => $loggedinuser, to => $new_owner, interface => 'opac' })
+        : 'does_not_exist';
 
-    $op = 'list';
-    if( !$shelf ) {
-        push @messages, { type => 'error', code => 'does_not_exist' };
-    } elsif( $shelf->public or !$shelf->is_shared or !$shelf->can_be_managed($loggedinuser) ) {
-        push @messages, { type => 'error', code => 'unauthorized_transfer' };
-    } elsif( !$new_owner ) {
+    if( !$new_owner && $error_code eq 'missing_to_parameter' ) { # show transfer form
         my $patrons = [];
         my $shares = $shelf->get_shares->search({ borrowernumber => { '!=' => undef } });
         while( my $share = $shares->next ) {
@@ -281,17 +279,16 @@ if ( $op eq 'add_form' ) {
         } else {
             push @messages, { type => 'error', code => 'no_email_found' };
         }
-    } elsif( !Koha::Patrons->find($new_owner) ) {
-        push @messages, { type => 'error', code => 'new_owner_not_found' };
-    } elsif( !$shelf->get_shares->search({ borrowernumber => $new_owner })->count ) {
-        push @messages, { type => 'error', code => 'new_owner_has_no_share' };
-    } else {
-        # Remove from virtualshelfshares new_owner, add loggedinuser
+    } elsif( $error_code ) {
+        push @messages, { type => 'error', code => $error_code };
+        $op = 'list';
+    } else { # transfer; remove new_owner from virtualshelfshares, add loggedinuser
         $shelf->_result->result_source->schema->txn_do( sub {
             $shelf->get_shares->search({ borrowernumber => $new_owner })->delete;
             Koha::Virtualshelfshare->new({ shelfnumber => $shelfnumber, borrowernumber => $loggedinuser, sharedate => dt_from_string })->store;
             $shelf->owner($new_owner)->store;
         });
+        $op = 'list';
     }
 }
 
diff --git a/virtualshelves/shelves.pl b/virtualshelves/shelves.pl
index 4e9404c87b..b512fe310b 100755
--- a/virtualshelves/shelves.pl
+++ b/virtualshelves/shelves.pl
@@ -60,6 +60,7 @@ my $referer  = $query->param('referer') || $op;
 my $public   = $query->param('public') ? 1 : 0;
 my ( $shelf, $shelfnumber, @messages, $allow_transfer );
 
+# PART1: Perform a few actions
 if ( $op eq 'add_form' ) {
     # Only pass default
     $shelf = { allow_change_from_owner => 1 };
@@ -239,23 +240,22 @@ if ( $op eq 'add_form' ) {
     $shelfnumber = $query->param('shelfnumber');
     $shelf = Koha::Virtualshelves->find($shelfnumber) if $shelfnumber;
     my $new_owner = $query->param('new_owner'); # is a borrowernumber
+    my $error_code = $shelf
+        ? $shelf->cannot_be_transferred({ by => $loggedinuser, to => $new_owner, interface => 'intranet' })
+        : 'does_not_exist';
 
-    if( $new_owner ) {
+    if( !$new_owner && $error_code eq 'missing_to_parameter' ) {
+        # show form
+    } elsif( $error_code ) {
+        push @messages, { type => 'error', code => $error_code };
+        $op = 'list';
+    } else {
+        $shelf->owner($new_owner)->store;
         $op = 'list';
-        # First check: shelf found, permission, patron found?
-        if( !$shelf ) {
-            push @messages, { type => 'alert', code => 'does_not_exist' };
-        } elsif( !haspermission(C4::Context->userenv->{id}, { lists => 'edit_public_lists' }) ) {
-            push @messages, { type => 'alert', code => 'unauthorized_transfer' };
-        } elsif( !Koha::Patrons->find($new_owner) ) {
-            push @messages, { type => 'alert', code => 'new_owner_not_found' };
-            $op = 'transfer'; # find again..
-        } else { # success
-            $shelf->owner($new_owner)->store;
-        }
     }
 }
 
+# PART2: After a possible action, further prepare form
 if ( $op eq 'view' ) {
     $shelfnumber ||= $query->param('shelfnumber');
     $shelf = Koha::Virtualshelves->find($shelfnumber);
@@ -375,6 +375,7 @@ if ( $op eq 'view' ) {
     }
 } elsif( $op eq 'list' ) {
     $allow_transfer = haspermission( C4::Context->userenv->{id}, { lists => 'edit_public_lists' } ) ? 1 : 0;
+        # this check only serves for button display
 }
 
 $template->param(
-- 
2.39.5