Bug 32712

Summary: OPACShowCheckoutName makes OPAC explode
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: OPACAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: critical    
Priority: P5 - low CC: jonathan.druart, lucas, martin.renvoize, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.03
Bug Depends on: 31313    
Bug Blocks:    
Attachments: Bug 32712: Fix OPAC breakage when OPACShowCheckoutName enabled
Bug 32712: Fix OPAC breakage when OPACShowCheckoutName enabled
Bug 32712: Fix OPAC breakage when OPACShowCheckoutName enabled
Bug 32712: Fix OPAC breakage when OPACShowCheckoutName enabled
Bug 32712: Fix OPAC breakage when OPACShowCheckoutName enabled
Bug 32712: (QA follow-up) Use Koha.Preference
Bug 32712: (QA follow-up) Add missing include

Description Katrin Fischer 2023-01-24 15:46:19 UTC
When OpacShoeCheckoutName is turned on, the OPAC shows this error if an item is checked out:

Template process failed: undef error - The method Koha::Item->firstname is not covered by tests!


I had talked to Joubu about it briefly, the fix could be:

diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/includes/item-status.inc b/koha-tmpl/opac-tmpl/bootstrap/en/includes/item-status.inc
index 0573b90e068..be5dbf004f9 100644
--- a/koha-tmpl/opac-tmpl/bootstrap/en/includes/item-status.inc
+++ b/koha-tmpl/opac-tmpl/bootstrap/en/includes/item-status.inc
@@ -47,15 +47,16 @@
 
 [% IF datedue %]
     [% SET itemavailable = 0 %]
+    [% SET checkout_patron = item.checkout.patron %]
     [% IF onsite_checkout %]
         [% IF ( OPACShowCheckoutName ) %]
-            <span class="item-status checkedout">Currently in local use by [% item.firstname | html %] [% item.surname | html %] [% IF ( item.cardnumber ) %]([% item.cardnumber | html %])[% END %]</span>
+            <span class="item-status checkedout">Currently in local use by [% checkout_patron.firstname | html %] [% checkout_patron.surname | html %] [% IF ( checkout_patron.cardnumber ) %]([% checkout_patron.cardnumber | html %])[% END %]</span>
         [% ELSE %]
             <span class="item-status checkedout">Currently in local use</span>
         [% END %]
     [% ELSE %]
         [% IF ( OPACShowCheckoutName ) %]
-            <span class="item-status checkedout">Checked out to [% item.firstname | html %] [% item.surname | html %] [% IF ( item.cardnumber ) %]([% item.cardnumber | html %])[% END %]</span>
+            <span class="item-status checkedout">Checked out to [% checkout_patron.firstname | html %] [% checkout_patron.surname | html %] [% IF ( checkout_patron.cardnumber ) %]([% checkout_patron.cardnumber | html %])[% END %]</span>
         [% ELSE %]
             <span class="item-status checkedout">Checked out</span>
         [% END %]
Comment 1 Tomás Cohen Arazi 2023-01-30 18:47:09 UTC
Caused by bug 31313
Comment 2 Tomás Cohen Arazi 2023-01-30 20:36:02 UTC
Created attachment 145808 [details] [review]
Bug 32712: Fix OPAC breakage when OPACShowCheckoutName enabled

Bug 31313 removed GetItemsInfo and proper Koha::Object-based objects are
passed now. But some automatic search & replace kept patron-related
attributes incorrectly linked to Koha::Item.

This patch makes the item-status.inc file reuse the checkout object and
get the linked patron to gather the required information. Some
simplification is done on hte same patch... should be trivial.

To test:
1. Have OpacShowCheckoutName enabled
2. Pick a biblio with items (all of them not checkecd-out)
3. Open the OPAC detail view for the biblio
=> SUCCESS: You see the biblio and the items
4. Check one of the items out
5. Refresh the OPAC page
=> FAIL: The page explodes!
6. Apply this patch
7. Refresh the OPAC page
=> SUCCESS: You see the biblio and the items, the patron info is
correctly displayed.
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Lucas Gass 2023-01-30 23:05:53 UTC
Created attachment 145831 [details] [review]
Bug 32712: Fix OPAC breakage when OPACShowCheckoutName enabled

Bug 31313 removed GetItemsInfo and proper Koha::Object-based objects are
passed now. But some automatic search & replace kept patron-related
attributes incorrectly linked to Koha::Item.

This patch makes the item-status.inc file reuse the checkout object and
get the linked patron to gather the required information. Some
simplification is done on hte same patch... should be trivial.

To test:
1. Have OpacShowCheckoutName enabled
2. Pick a biblio with items (all of them not checkecd-out)
3. Open the OPAC detail view for the biblio
=> SUCCESS: You see the biblio and the items
4. Check one of the items out
5. Refresh the OPAC page
=> FAIL: The page explodes!
6. Apply this patch
7. Refresh the OPAC page
=> SUCCESS: You see the biblio and the items, the patron info is
correctly displayed.
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 4 Katrin Fischer 2023-01-30 23:12:55 UTC
Created attachment 145832 [details] [review]
Bug 32712: Fix OPAC breakage when OPACShowCheckoutName enabled

Bug 31313 removed GetItemsInfo and proper Koha::Object-based objects are
passed now. But some automatic search & replace kept patron-related
attributes incorrectly linked to Koha::Item.

This patch makes the item-status.inc file reuse the checkout object and
get the linked patron to gather the required information. Some
simplification is done on hte same patch... should be trivial.

To test:
1. Have OpacShowCheckoutName enabled
2. Pick a biblio with items (all of them not checkecd-out)
3. Open the OPAC detail view for the biblio
=> SUCCESS: You see the biblio and the items
4. Check one of the items out
5. Refresh the OPAC page
=> FAIL: The page explodes!
6. Apply this patch
7. Refresh the OPAC page
=> SUCCESS: You see the biblio and the items, the patron info is
correctly displayed.
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 5 Jonathan Druart 2023-01-31 10:16:27 UTC
Created attachment 145840 [details] [review]
Bug 32712: Fix OPAC breakage when OPACShowCheckoutName enabled

Bug 31313 removed GetItemsInfo and proper Koha::Object-based objects are
passed now. But some automatic search & replace kept patron-related
attributes incorrectly linked to Koha::Item.

This patch makes the item-status.inc file reuse the checkout object and
get the linked patron to gather the required information. Some
simplification is done on hte same patch... should be trivial.

To test:
1. Have OpacShowCheckoutName enabled
2. Pick a biblio with items (all of them not checkecd-out)
3. Open the OPAC detail view for the biblio
=> SUCCESS: You see the biblio and the items
4. Check one of the items out
5. Refresh the OPAC page
=> FAIL: The page explodes!
6. Apply this patch
7. Refresh the OPAC page
=> SUCCESS: You see the biblio and the items, the patron info is
correctly displayed.
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 6 Jonathan Druart 2023-01-31 10:17:11 UTC
Created attachment 145841 [details] [review]
Bug 32712: Fix OPAC breakage when OPACShowCheckoutName enabled

Bug 31313 removed GetItemsInfo and proper Koha::Object-based objects are
passed now. But some automatic search & replace kept patron-related
attributes incorrectly linked to Koha::Item.

This patch makes the item-status.inc file reuse the checkout object and
get the linked patron to gather the required information. Some
simplification is done on hte same patch... should be trivial.

To test:
1. Have OpacShowCheckoutName enabled
2. Pick a biblio with items (all of them not checkecd-out)
3. Open the OPAC detail view for the biblio
=> SUCCESS: You see the biblio and the items
4. Check one of the items out
5. Refresh the OPAC page
=> FAIL: The page explodes!
6. Apply this patch
7. Refresh the OPAC page
=> SUCCESS: You see the biblio and the items, the patron info is
correctly displayed.
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 7 Jonathan Druart 2023-01-31 10:17:44 UTC
Thanks, Tomas, better than mine.

I would have been tempted to use Koha.Preference for OPACShowCheckoutName, but that's for another day ;)
Comment 8 Jonathan Druart 2023-01-31 10:20:51 UTC
Worth noting: the only difference would be (theorically) if we have a checkout without due date, which does not make sense. By the way, shouldn't we have it not null in DB structure?
Comment 9 Tomás Cohen Arazi 2023-01-31 11:25:17 UTC
(In reply to Jonathan Druart from comment #7)
> I would have been tempted to use Koha.Preference for OPACShowCheckoutName,
> but that's for another day ;)

Yeah, it just needs tweaking several .pl and felt out of the scope.

(In reply to Jonathan Druart from comment #8)
> Worth noting: the only difference would be (theorically) if we have a
> checkout without due date, which does not make sense.

We should (at some point) allow 'indefinite checkouts'. I felt like we should be prepared for due_date=NULL for the future.

Anyway, as you also noticed, we are not setting due_date=NULL so the behavior is kept.
Comment 10 Tomás Cohen Arazi 2023-01-31 12:35:04 UTC
Created attachment 145847 [details] [review]
Bug 32712: (QA follow-up) Use Koha.Preference

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2023-01-31 12:42:29 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 12 Jacob O'Mara 2023-01-31 17:47:25 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 13 Jonathan Druart 2023-02-02 09:58:56 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> Created attachment 145847 [details] [review] [review]
> Bug 32712: (QA follow-up) Use Koha.Preference
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

koha-tmpl/opac-tmpl/bootstrap/en/includes/item-status.inc is missing [% USE Koha %]
Comment 14 Tomás Cohen Arazi 2023-02-02 11:26:23 UTC
(In reply to Jonathan Druart from comment #13)
> (In reply to Tomás Cohen Arazi from comment #10)
> > Created attachment 145847 [details] [review] [review] [review]
> > Bug 32712: (QA follow-up) Use Koha.Preference
> > 
> > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
> 
> koha-tmpl/opac-tmpl/bootstrap/en/includes/item-status.inc is missing [% USE
> Koha %]

That was on purpose. Do you think it needs to be added even if the callers already include it?
Comment 15 Jonathan Druart 2023-02-02 13:18:37 UTC
It should be included where used. We faced issues because of that in the past (w
hen backported for instance).
Comment 16 Tomás Cohen Arazi 2023-02-02 15:31:40 UTC
Created attachment 146014 [details] [review]
Bug 32712: (QA follow-up) Add missing include

I assumed it wasn't needed because teh .tt files using this already
includ teh Koha plugin. But we shuoldn't skip, just in case someone
doesn't notice when reusing this, and debugging will be a pain.

Sorry for that.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Lucas Gass 2023-02-03 21:44:41 UTC
Not needed in 22.05.x, no backport
Comment 18 Martin Renvoize 2023-02-23 14:39:35 UTC
All in 22.11.x already.