From beeebdbf821e5ced39ed7871d81cfa79b4aa9917 Mon Sep 17 00:00:00 2001
From: Nicolas Legrand <nicolas.legrand@bulac.fr>
Date: Wed, 8 Jul 2020 09:52:51 +0200
Subject: [PATCH] Bug 24412: (follow-up) QA

Following Josef Moravec QA comments :

- rewrite Koha::Hold->desk according to Object Oriented Koha
Guidelines and use it to fetch desk name in various templates
- remove unused Desks.GetName
- Check for columns existence in db update
---
 Koha/Hold.pm                                       | 11 ++++------
 Koha/Template/Plugin/Desks.pm                      | 14 ------------
 .../bug_24412_Attach_waiting_reserve_to_desk.perl  | 25 +++++++++++-----------
 .../intranet-tmpl/prog/en/includes/holds_table.inc |  1 -
 .../prog/en/includes/waiting_holds.inc             |  3 +--
 .../prog/en/modules/catalogue/detail.tt            |  3 +--
 .../bootstrap/en/includes/holds-table.inc          |  3 +--
 7 files changed, 20 insertions(+), 40 deletions(-)

diff --git a/Koha/Hold.pm b/Koha/Hold.pm
index 1f79e29..617ca42 100644
--- a/Koha/Hold.pm
+++ b/Koha/Hold.pm
@@ -172,8 +172,6 @@ sub set_waiting {
         desk_id => $desk_id,
     };
 
-    if (defined $desk_id) { $values->{'desk_id'} = $desk_id };
-
     my $requested_expiration;
     if ($self->expirationdate) {
         $requested_expiration = dt_from_string($self->expirationdate);
@@ -329,11 +327,10 @@ Returns the related Koha::Library object for this Hold
 =cut
 
 sub desk {
-    my ($self) = @_;
-
-    $self->{_desk} ||= Koha::Desks->find( $self->desk_id() );
-
-    return $self->{_desk};
+    my $self = shift;
+    my $desk_rs = $self->_result->desk;
+    return unless $desk_rs;
+    return Koha::Desk->_new_from_dbic($desk_rs);
 }
 
 =head3 borrower
diff --git a/Koha/Template/Plugin/Desks.pm b/Koha/Template/Plugin/Desks.pm
index 02af1b3..4216497 100644
--- a/Koha/Template/Plugin/Desks.pm
+++ b/Koha/Template/Plugin/Desks.pm
@@ -37,20 +37,6 @@ got or the current one.
 
 =head2 Methods
 
-=head3 GetName
-
-[% Desk.GetName(desk_id) %]
-
-return desk name or empty string
-
-=cut
-
-sub GetName {
-    my ( $self, $desk_id ) = @_;
-    my $d = Koha::Desks->find( $desk_id );
-    return (defined $d) ? $d->unblessed->{'desk_name'} : q{};
-}
-
 =head3 GetLoggedInDeskId
 
 [% Desks.GetLoggedInDeskId %]
diff --git a/installer/data/mysql/atomicupdate/bug_24412_Attach_waiting_reserve_to_desk.perl b/installer/data/mysql/atomicupdate/bug_24412_Attach_waiting_reserve_to_desk.perl
index c743eab..9242ffb 100644
--- a/installer/data/mysql/atomicupdate/bug_24412_Attach_waiting_reserve_to_desk.perl
+++ b/installer/data/mysql/atomicupdate/bug_24412_Attach_waiting_reserve_to_desk.perl
@@ -1,16 +1,17 @@
 $DBversion = 'XXX';  # will be replaced by the RM
 if( CheckVersion( $DBversion ) ) {
-    # you can use $dbh here like:
-    $dbh->do(q{
-ALTER TABLE reserves ADD COLUMN desk_id INT(11) DEFAULT NULL AFTER branchcode,
-  ADD KEY desk_id (`desk_id`),
-  ADD CONSTRAINT `reserves_ibfk_6` FOREIGN KEY (`desk_id`) REFERENCES `desks` (`desk_id`) ON DELETE CASCADE ON UPDATE CASCADE ;
-             });
-    $dbh->do(q{
-ALTER TABLE old_reserves ADD COLUMN desk_id INT(11) DEFAULT NULL AFTER branchcode,
-  ADD KEY `old_desk_id` (`desk_id`);
-             });
+    if ( !column_exists( 'reserves', 'desk_id' ) ) {
+        $dbh->do(q{
+                 ALTER TABLE reserves ADD COLUMN desk_id INT(11) DEFAULT NULL AFTER branchcode,
+                 ADD KEY desk_id (`desk_id`),
+                 ADD CONSTRAINT `reserves_ibfk_6` FOREIGN KEY (`desk_id`) REFERENCES `desks` (`desk_id`) ON DELETE CASCADE ON UPDATE CASCADE ;
+                 });
+        $dbh->do(q{
+                 ALTER TABLE old_reserves ADD COLUMN desk_id INT(11) DEFAULT NULL AFTER branchcode,
+                 ADD KEY `old_desk_id` (`desk_id`);
+                 });
 
-    SetVersion( $DBversion );
-    print "Upgrade to $DBversion done (Bug XXXXX - Attach waiting reserve to desk)\n";
+        SetVersion( $DBversion );
+        print "Upgrade to $DBversion done (Bug XXXXX - Attach waiting reserve to desk)\n";
+    }
 }
diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc
index de74f6c..53974e9 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc
@@ -1,5 +1,4 @@
 [% USE Koha %]
-[% USE Desks %]
 <table>
     <tr>
         [% IF ( CAN_user_reserveforothers_modify_holds_priority ) %]
diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/waiting_holds.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/waiting_holds.inc
index 7f68262..eacacd9 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/waiting_holds.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/waiting_holds.inc
@@ -1,5 +1,4 @@
 [% USE ItemTypes %]
-[% USE Desks %]
 [% USE AuthorisedValues %]
 <table id="[% table_name | html %]">
     <thead>
@@ -39,7 +38,7 @@
                     [% END %]
                 </td>
                 <td>[% Branches.GetName( reserveloo.item.homebranch ) | html %]</td>
-                <td>[% Branches.GetName( reserveloo.item.holdingbranch ) | html %][% IF (reserveloo.desk_id  ) %], [% Desks.GetName (reserveloo.desk_id) | html %][% END %]</td>
+                <td>[% Branches.GetName( reserveloo.item.holdingbranch ) | html %][% IF (reserveloo.desk_id  ) %], [% reserveloo.desk.desk_name | html %][% END %]</td>
                 <td>[% AuthorisedValues.GetByCode('LOC', reserveloo.item.location) | html %]</td>
                 <td>[% reserveloo.item.itemcallnumber | html %]</td>
                 <td>[% reserveloo.item.copynumber | html %]</td>
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
index a998750..b3d4c12 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
@@ -4,7 +4,6 @@
 [% USE KohaDates %]
 [% USE AuthorisedValues %]
 [% USE Branches %]
-[% USE Desks %]
 [% USE Biblio %]
 [% USE TablesSettings %]
 [% PROCESS 'i18n.inc' %]
@@ -385,7 +384,7 @@ Note that permanent location is a code, and location may be an authval.
                         [% SET hold = item.first_hold %]
                         [% IF hold %]
                             [% IF hold.waitingdate %]
-                                <span class="waitingat">Waiting at [% Branches.GetName( hold.branchcode ) | html %][% IF ( hold.desk_id ) %], [% Desks.GetName ( hold.desk_id ) | html %][% END %] since [% hold.waitingdate | $KohaDates %].</span>
+                                <span class="waitingat">Waiting at [% Branches.GetName( hold.branchcode ) | html %][% IF ( hold.desk_id ) %], [% hold.desk.desk_name | html %][% END %] since [% hold.waitingdate | $KohaDates %].</span>
                                 [% IF canreservefromotherbranches AND ( hold.waitingdate OR hold.priority == 1 ) %]
                                     <span class="heldfor">Hold for:</span>
                                     [% INCLUDE 'patron-title.inc' patron=hold.borrower hide_patron_infos_if_needed=1 %]
diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc b/koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc
index e24d888..7b3c3ab 100644
--- a/koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc
+++ b/koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc
@@ -1,5 +1,4 @@
 [% USE Branches %]
-[% USE Desks  %]
 [% USE ItemTypes %]
 [% USE KohaDates %]
 
@@ -107,7 +106,7 @@
                                 [% IF ( HOLD.is_at_destination ) %]
                                     [% IF ( HOLD.found ) %]
 
-                                        Item waiting at <strong> [% HOLD.branch.branchname | html %]</strong>[% IF ( HOLD.desk_id ) %], [% Desks.GetName ( HOLD.desk_id ) | html %],[% END %]
+                                        Item waiting at <strong> [% HOLD.branch.branchname | html %]</strong>[% IF ( HOLD.desk_id ) %], [% HOLD.desk.desk_name | html %],[% END %]
 
                                         [% IF ( HOLD.waitingdate ) %]
                                             since [% HOLD.waitingdate | $KohaDates %]
-- 
2.1.4