Bug 31313

Summary: Remove GetItemsInfo from opac-detail
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: joonas.kylmala, lucas, m.de.rooy, nick, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33037
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00
Bug Depends on: 31306    
Bug Blocks: 35006, 27272, 29144, 31396, 31463, 32712    
Attachments: Bug 31313: Remove GetItemsInfo from opac-detail
Bug 31313: Fix availability - OPAC opac-detail
Bug 31313: Remove GetItemsInfo from opac-detail
Bug 31313: Fix availability - OPAC opac-detail
Bug 31313: Remove GetItemsInfo from opac-detail
Bug 31313: Fix availability - OPAC opac-detail
Bug 31313: (QA follow-up) Search Koha::Items
Bug 31313: (QA follow-up) Prefetch checkouts and branches
Bug 31313: Unit tests
Bug 31313: Fix holds_count and priority
Bug 31313: (QA follow-up) Show status "On hold" correctly if there are waiting holds
Bug 31313: (QA follow-up) Show transfer status correctly if item in transit
Bug 31313: (QA follow-up) Remove unused bundle_host field from item_info
Bug 31313: (QA follow-up) Show status "On hold" correctly if there are waiting holds
Bug 31313: (QA follow-up) Show transfer status correctly if item in transit
Bug 31313: (QA follow-up) Remove unused bundle_host field from item_info

Description Jonathan Druart 2022-08-08 14:22:49 UTC
Bug 27272 is going to remove C4::Items::GetItemsInfo in favour of Koha::Items->search_ordered.

Here we are going to deal with opac-detail
Comment 1 Jonathan Druart 2022-08-08 14:49:25 UTC
Created attachment 138821 [details] [review]
Bug 31313: Remove GetItemsInfo from opac-detail

Bug 27272 is going to remove C4::Items::GetItemsInfo in favour of Koha::Items->search_ordered.

Here we are going to deal with opac-detail

Test plan:
List items on the modified view and confirm that all the info is
displayed correctly
Comment 2 Jonathan Druart 2022-08-08 14:50:35 UTC
Created attachment 138822 [details] [review]
Bug 31313: Fix availability - OPAC opac-detail

Has to move some code to a method
Comment 3 Owen Leonard 2022-08-08 18:06:18 UTC
Created attachment 138847 [details] [review]
Bug 31313: Remove GetItemsInfo from opac-detail

Bug 27272 is going to remove C4::Items::GetItemsInfo in favour of Koha::Items->search_ordered.

Here we are going to deal with opac-detail

Test plan:
List items on the modified view and confirm that all the info is
displayed correctly

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 4 Owen Leonard 2022-08-08 18:06:22 UTC
Created attachment 138848 [details] [review]
Bug 31313: Fix availability - OPAC opac-detail

Has to move some code to a method

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 5 Nick Clemens 2022-08-15 11:48:00 UTC
Created attachment 139119 [details] [review]
Bug 31313: Remove GetItemsInfo from opac-detail

Bug 27272 is going to remove C4::Items::GetItemsInfo in favour of Koha::Items->search_ordered.

Here we are going to deal with opac-detail

Test plan:
List items on the modified view and confirm that all the info is
displayed correctly

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 Nick Clemens 2022-08-15 11:48:04 UTC
Created attachment 139120 [details] [review]
Bug 31313: Fix availability - OPAC opac-detail

Has to move some code to a method

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Nick Clemens 2022-08-15 11:48:08 UTC
Created attachment 139121 [details] [review]
Bug 31313: (QA follow-up) Search Koha::Items

This ensures host items from otehr biblios are returned as well

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Nick Clemens 2022-08-15 11:48:12 UTC
Created attachment 139122 [details] [review]
Bug 31313: (QA follow-up) Prefetch checkouts and branches

This prefetches objects we are using in templates to avoid extar DB calls

The branch fetches won't actually be used without the extra patch on 31314 that
adapts home_branch and holding_branch routines to use result objects

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Jonathan Druart 2022-08-16 06:04:01 UTC
Thanks for the follow-ups, Nick!
Comment 10 Tomás Cohen Arazi 2022-08-16 13:11:56 UTC
Created attachment 139193 [details] [review]
Bug 31313: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2022-08-16 13:22:09 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 12 Marcel de Rooy 2022-08-16 14:05:21 UTC
I am rebasing 29144 on top of this one.
And have the idea that this is wrong code:

        $item->{holds_count} = $item_reserves{ $item->itemnumber };
        $item->{priority}    = $priority{ $item->itemnumber };

Should that be in $item_info instead ?
Comment 13 Jonathan Druart 2022-08-17 08:16:56 UTC
Created attachment 139239 [details] [review]
Bug 31313: Fix holds_count and priority
Comment 14 Jonathan Druart 2022-08-17 08:17:12 UTC
(In reply to Marcel de Rooy from comment #12)
> I am rebasing 29144 on top of this one.
> And have the idea that this is wrong code:
> 
>         $item->{holds_count} = $item_reserves{ $item->itemnumber };
>         $item->{priority}    = $priority{ $item->itemnumber };
> 
> Should that be in $item_info instead ?

Yes!
Comment 15 Tomás Cohen Arazi 2022-08-17 12:19:44 UTC
Follow-up pushed. Thanks for promptly addressing it, Jonathan!
Comment 16 Joonas Kylmälä 2022-08-21 13:54:31 UTC
regression found: the item transfer status is not shown anymore.
Comment 17 Joonas Kylmälä 2022-08-21 14:07:56 UTC
(In reply to Joonas Kylmälä from comment #16)
> regression found: the item transfer status is not shown anymore.

Seems like few other fields could be missing too, in addition I found item.waiting to be missing.
Comment 18 Joonas Kylmälä 2022-08-23 19:43:14 UTC
(In reply to Joonas Kylmälä from comment #17)
> (In reply to Joonas Kylmälä from comment #16)
> > regression found: the item transfer status is not shown anymore.
> 
> Seems like few other fields could be missing too, in addition I found
> item.waiting to be missing.

Working on the fixes.
Comment 19 Tomás Cohen Arazi 2022-08-23 19:49:01 UTC
(In reply to Joonas Kylmälä from comment #18)
> (In reply to Joonas Kylmälä from comment #17)
> > (In reply to Joonas Kylmälä from comment #16)
> > > regression found: the item transfer status is not shown anymore.
> > 
> > Seems like few other fields could be missing too, in addition I found
> > item.waiting to be missing.
> 
> Working on the fixes.

Thank you! I've been trapped by other devs.
Comment 20 Joonas Kylmälä 2022-08-23 20:14:34 UTC
Created attachment 139693 [details] [review]
Bug 31313: (QA follow-up) Show status "On hold" correctly if there are waiting holds

The patch "Bug 31313: Fix availability - OPAC
opac-detail" (70cb4e22c07) cause the OPAC detail page only show
"Available" status even when there were waiting holds.

The onhold item field is removed at the same go as it was confusing
and unused since "Bug 2655: Fixes availabilty counts if items are on
hold shelf. Patch (2/2)" (a37545eb1)

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 21 Joonas Kylmälä 2022-08-23 20:14:38 UTC
Created attachment 139694 [details] [review]
Bug 31313: (QA follow-up) Show transfer status correctly if item in transit

The patch "Bug 31313: Fix availability - OPAC
opac-detail" (70cb4e22c07) caused the OPAC detail page only show
"Available" status even when the item was in transit.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 22 Joonas Kylmälä 2022-08-23 20:14:42 UTC
Created attachment 139695 [details] [review]
Bug 31313: (QA follow-up) Remove unused bundle_host field from item_info

The patch "Bug 31313: Fix availability - OPAC
opac-detail" (70cb4e22c07) lead to
koha-tmpl/opac-tmpl/bootstrap/en/includes/item-status.inc
use the bundle_host method directly through the passed
Koha::Item object, thus the bundle_host field added by
this script is unused.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 23 Joonas Kylmälä 2022-08-23 20:21:55 UTC
Tomás, I attached a few important fixes here that fix the availability status shown in the opac-detail.pl page, could you push them directly from here to master after reviewing and making sure they look alright?

However, there is one more bug that needs to be solved: item.on_order. The order info is not shown yet, and it has a quite complex implementation in opac-detail.pl and I'm afraid if it was to be implemented on the template side it will cause unneeded SQL queries to be made and slow down the opac-detail.pl page load time. I don't have time to work on it today, could someone else look into this?

I went through item-status.inc file and the missing ordering info should be the only thing left to fix (after applying my patches attached to this bug report).
Comment 24 Nick Clemens 2022-08-25 10:07:02 UTC
Created attachment 139761 [details] [review]
Bug 31313: (QA follow-up) Show status "On hold" correctly if there are waiting holds

The patch "Bug 31313: Fix availability - OPAC
opac-detail" (70cb4e22c07) cause the OPAC detail page only show
"Available" status even when there were waiting holds.

The onhold item field is removed at the same go as it was confusing
and unused since "Bug 2655: Fixes availabilty counts if items are on
hold shelf. Patch (2/2)" (a37545eb1)

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 25 Nick Clemens 2022-08-25 10:07:06 UTC
Created attachment 139762 [details] [review]
Bug 31313: (QA follow-up) Show transfer status correctly if item in transit

The patch "Bug 31313: Fix availability - OPAC
opac-detail" (70cb4e22c07) caused the OPAC detail page only show
"Available" status even when the item was in transit.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 26 Nick Clemens 2022-08-25 10:07:09 UTC
Created attachment 139763 [details] [review]
Bug 31313: (QA follow-up) Remove unused bundle_host field from item_info

The patch "Bug 31313: Fix availability - OPAC
opac-detail" (70cb4e22c07) lead to
koha-tmpl/opac-tmpl/bootstrap/en/includes/item-status.inc
use the bundle_host method directly through the passed
Koha::Item object, thus the bundle_host field added by
this script is unused.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 27 Tomás Cohen Arazi 2022-08-25 11:21:50 UTC
Follow-ups pushed to master. Thanks!

Any other follow-up please make it its own bug (set) ot avoid confusion.
Comment 28 Lucas Gass 2022-09-30 23:14:30 UTC
Missing dependencies for 22.05.x, no backport