Bug 31313 - Remove GetItemsInfo from opac-detail
Summary: Remove GetItemsInfo from opac-detail
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 31306
Blocks: 27272 29144 31396 31463 32712 35006
  Show dependency treegraph
 
Reported: 2022-08-08 14:22 UTC by Jonathan Druart
Modified: 2024-07-04 20:37 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 31313: Remove GetItemsInfo from opac-detail (20.60 KB, patch)
2022-08-08 14:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31313: Fix availability - OPAC opac-detail (7.29 KB, patch)
2022-08-08 14:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31313: Remove GetItemsInfo from opac-detail (20.64 KB, patch)
2022-08-08 18:06 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 31313: Fix availability - OPAC opac-detail (7.33 KB, patch)
2022-08-08 18:06 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 31313: Remove GetItemsInfo from opac-detail (20.70 KB, patch)
2022-08-15 11:48 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 31313: Fix availability - OPAC opac-detail (7.39 KB, patch)
2022-08-15 11:48 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 31313: (QA follow-up) Search Koha::Items (1.36 KB, patch)
2022-08-15 11:48 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 31313: (QA follow-up) Prefetch checkouts and branches (1.07 KB, patch)
2022-08-15 11:48 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 31313: Unit tests (1.80 KB, patch)
2022-08-16 13:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31313: Fix holds_count and priority (1.04 KB, patch)
2022-08-17 08:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31313: (QA follow-up) Show status "On hold" correctly if there are waiting holds (2.90 KB, patch)
2022-08-23 20:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 31313: (QA follow-up) Show transfer status correctly if item in transit (3.57 KB, patch)
2022-08-23 20:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 31313: (QA follow-up) Remove unused bundle_host field from item_info (1.21 KB, patch)
2022-08-23 20:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 31313: (QA follow-up) Show status "On hold" correctly if there are waiting holds (2.96 KB, patch)
2022-08-25 10:07 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 31313: (QA follow-up) Show transfer status correctly if item in transit (3.62 KB, patch)
2022-08-25 10:07 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 31313: (QA follow-up) Remove unused bundle_host field from item_info (1.27 KB, patch)
2022-08-25 10:07 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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