Bug 33497

Summary: Reduce DB calls on staff detail page
Product: Koha Reporter: Nick Clemens <nick>
Component: Staff interfaceAssignee: Nick Clemens <nick>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: david, dcook, gmcharlt, jonathan.druart, matt.blenkinsop, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.02
Bug Depends on: 33167, 33493, 33496    
Bug Blocks: 33568, 34360, 34639    
Attachments: Bug 33497: Use 'host_items' param to fetch all items at once
Bug 33497: Add a filter relationship for branchtransfers
Bug 33497: Use 'host_items' param to fetch all items at once
Bug 33497: Add a filter relationship for branchtransfers
Bug 33497: Use 'host_items' param to fetch all items at once
Bug 33497: Add a filter relationship for branchtransfers
Bug 33497: (follow-up) Remove unecessary scalar
Bug 33497: (follow-up) Adjust relationship name from bug 33493
Bug 33497: Use 'host_items' param to fetch all items at once
Bug 33497: Add a filter relationship for branchtransfers
Bug 33497: (follow-up) Remove unecessary scalar
Bug 33497: (follow-up) Adjust relationship name from bug 33493
Bug 33497: Don't get host items if the MARC record is invalid

Description Nick Clemens 2023-04-11 18:51:45 UTC

    
Comment 1 Nick Clemens 2023-04-11 18:57:03 UTC
Created attachment 149467 [details] [review]
Bug 33497: Use 'host_items' param to fetch all items at once

This patch adjusts the detail page to fetch items and host items
together, and prefetches transfers and checkouts

To test:
1 - Enable easy analytics
2 - Attach some items to a bib
3 - Checkout an item on the bib
4 - View the details page
5 - Apply patch
6 - View details page, confirm nothing has changed
Comment 2 Nick Clemens 2023-04-11 18:57:05 UTC
Created attachment 149468 [details] [review]
Bug 33497: Add a filter relationship for branchtransfers

This uses the new relationship from bug 33493 to fetch the transfers for items

To test:
1 - Transfer some items on a bib
2 - View the biblio details page in the staff interface
3 - Apply patch
4 - Confirm  the view is the same
Comment 3 David Nind 2023-04-11 19:12:36 UTC
Added assignee.
Comment 4 David Nind 2023-04-12 18:00:59 UTC
Created attachment 149573 [details] [review]
Bug 33497: Use 'host_items' param to fetch all items at once

This patch adjusts the detail page to fetch items and host items
together, and prefetches transfers and checkouts

To test:
1 - Enable easy analytics
2 - Attach some items to a bib
3 - Checkout an item on the bib
4 - View the details page
5 - Apply patch
6 - View details page, confirm nothing has changed

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2023-04-12 18:01:01 UTC
Created attachment 149574 [details] [review]
Bug 33497: Add a filter relationship for branchtransfers

This uses the new relationship from bug 33493 to fetch the transfers for items

To test:
1 - Transfer some items on a bib
2 - View the biblio details page in the staff interface
3 - Apply patch
4 - Confirm  the view is the same

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 Jonathan Druart 2023-04-14 13:28:47 UTC
Nick, I have a plan to rewrite the items table on the staff detail page to make it use the REST API. I will base my patches on top of this bug, but keep it in mind in case you would like to provide more things. Maybe we are going to collide :)
Comment 7 Nick Clemens 2023-04-14 13:48:39 UTC
(In reply to Jonathan Druart from comment #6)
> Nick, I have a plan to rewrite the items table on the staff detail page to
> make it use the REST API. I will base my patches on top of this bug, but
> keep it in mind in case you would like to provide more things. Maybe we are
> going to collide :)

Excellent! I had started trying to clean the page, then realized it would just be better to move to the API, so switched to setting the stage for that. Let me know if any of this set needs adjustment or how I can help. I won't file anymore touching this one for now :-)
Comment 8 Jonathan Druart 2023-04-18 13:56:30 UTC
+my $items = $biblio->items({ host_items => 1 })->search_ordered( $params, { prefetch => ['issue','branchtransfers'] } );
[...]
+$dat->{'count'} = $biblio->items({ host_items => 1 })->count;
+$dat->{'showncount'} = $items->count;

Are not we going to always get count == showncount?
Comment 9 Jonathan Druart 2023-04-18 15:20:23 UTC
Nick answered me, itemlost is passed in $params
Comment 10 Jonathan Druart 2023-04-19 12:26:48 UTC
$dat->{'count'} = $biblio->items->count + $hostitems->count;

Should we apply the same count (ie. include host items) in other catalogue scripts as well? See catalog-strings.inc
  var count = [% count | html %];
Comment 11 Nick Clemens 2023-04-19 17:15:53 UTC
(In reply to Jonathan Druart from comment #10)
> $dat->{'count'} = $biblio->items->count + $hostitems->count;
> 
> Should we apply the same count (ie. include host items) in other catalogue
> scripts as well? See catalog-strings.inc
>   var count = [% count | html %];

moredetail needs it, the other areas using catalog-strings don't actually seemd to display count or items
Comment 12 Jonathan Druart 2023-05-25 12:01:02 UTC
+if (C4::Context->preference("AlternateHoldingsField") && scalar $items->count == 0) {

scalar should be removed here.
Comment 13 Nick Clemens 2023-05-31 11:33:15 UTC
Created attachment 151861 [details] [review]
Bug 33497: Use 'host_items' param to fetch all items at once

This patch adjusts the detail page to fetch items and host items
together, and prefetches transfers and checkouts

To test:
1 - Enable easy analytics
2 - Attach some items to a bib
3 - Checkout an item on the bib
4 - View the details page
5 - Apply patch
6 - View details page, confirm nothing has changed

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 Nick Clemens 2023-05-31 11:33:17 UTC
Created attachment 151862 [details] [review]
Bug 33497: Add a filter relationship for branchtransfers

This uses the new relationship from bug 33493 to fetch the transfers for items

To test:
1 - Transfer some items on a bib
2 - View the biblio details page in the staff interface
3 - Apply patch
4 - Confirm  the view is the same

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 Nick Clemens 2023-05-31 11:33:19 UTC
Created attachment 151863 [details] [review]
Bug 33497: (follow-up) Remove unecessary scalar
Comment 16 Nick Clemens 2023-05-31 12:06:56 UTC
Created attachment 151866 [details] [review]
Bug 33497: (follow-up) Adjust relationship name from bug 33493
Comment 17 Jonathan Druart 2023-07-05 10:31:56 UTC
Created attachment 153046 [details] [review]
Bug 33497: Use 'host_items' param to fetch all items at once

This patch adjusts the detail page to fetch items and host items
together, and prefetches transfers and checkouts

To test:
1 - Enable easy analytics
2 - Attach some items to a bib
3 - Checkout an item on the bib
4 - View the details page
5 - Apply patch
6 - View details page, confirm nothing has changed

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2023-07-05 10:31:59 UTC
Created attachment 153047 [details] [review]
Bug 33497: Add a filter relationship for branchtransfers

This uses the new relationship from bug 33493 to fetch the transfers for items

To test:
1 - Transfer some items on a bib
2 - View the biblio details page in the staff interface
3 - Apply patch
4 - Confirm  the view is the same

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2023-07-05 10:32:02 UTC
Created attachment 153048 [details] [review]
Bug 33497: (follow-up) Remove unecessary scalar

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2023-07-05 10:32:06 UTC
Created attachment 153049 [details] [review]
Bug 33497: (follow-up) Adjust relationship name from bug 33493

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Tomás Cohen Arazi 2023-07-06 13:10:36 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 22 Jonathan Druart 2023-07-07 13:52:41 UTC
Created attachment 153183 [details] [review]
Bug 33497: Don't get host items if the MARC record is invalid

When don't want to explode with "invalid data" later so we need to
condition the host_item search

call trace was
        Koha::Biblio::Metadata::record('Koha::Biblio::Metadata=HASH(0x55ab92e60120)') called at /kohadevbox/koha/Koha/Biblio.pm line 538
        Koha::Biblio::_host_itemnumbers('Koha::Biblio=HASH(0x55ab92e4d5c0)') called at /kohadevbox/koha/Koha/Biblio.pm line 498
        Koha::Biblio::items('Koha::Biblio=HASH(0x55ab92e4d5c0)', 'HASH(0x55ab934615d8)') called at /kohadevbox/koha/catalogue/detail.pl line 200

This is fixing t/db_dependent/selenium/regressions.t
Comment 23 Tomás Cohen Arazi 2023-07-07 15:12:27 UTC
(In reply to Jonathan Druart from comment #22)
> Created attachment 153183 [details] [review] [review]
> Bug 33497: Don't get host items if the MARC record is invalid
> 
> When don't want to explode with "invalid data" later so we need to
> condition the host_item search
> 
> call trace was
>        
> Koha::Biblio::Metadata::record('Koha::Biblio::
> Metadata=HASH(0x55ab92e60120)') called at /kohadevbox/koha/Koha/Biblio.pm
> line 538
>         Koha::Biblio::_host_itemnumbers('Koha::Biblio=HASH(0x55ab92e4d5c0)')
> called at /kohadevbox/koha/Koha/Biblio.pm line 498
>         Koha::Biblio::items('Koha::Biblio=HASH(0x55ab92e4d5c0)',
> 'HASH(0x55ab934615d8)') called at /kohadevbox/koha/catalogue/detail.pl line
> 200
> 
> This is fixing t/db_dependent/selenium/regressions.t

Follow-up pushed. Thanks, Jonathan!
Comment 24 Martin Renvoize 2023-07-17 15:44:49 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 25 Matt Blenkinsop 2023-07-18 13:28:20 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x
Comment 26 Matt Blenkinsop 2023-07-19 09:56:18 UTC
Backport to 22.11.x reverted - a missing dependency in the tree (33167) has been identified