Bug 33497 - Reduce DB calls on staff detail page
Summary: Reduce DB calls on staff detail page
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 33167 33493 33496
Blocks: 33568 34360 34639
  Show dependency treegraph
 
Reported: 2023-04-11 18:51 UTC by Nick Clemens
Modified: 2024-01-10 09:24 UTC (History)
6 users (show)

See Also:
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


Attachments
Bug 33497: Use 'host_items' param to fetch all items at once (2.66 KB, patch)
2023-04-11 18:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33497: Add a filter relationship for branchtransfers (2.01 KB, patch)
2023-04-11 18:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33497: Use 'host_items' param to fetch all items at once (2.71 KB, patch)
2023-04-12 18:00 UTC, David Nind
Details | Diff | Splinter Review
Bug 33497: Add a filter relationship for branchtransfers (2.06 KB, patch)
2023-04-12 18:01 UTC, David Nind
Details | Diff | Splinter Review
Bug 33497: Use 'host_items' param to fetch all items at once (2.73 KB, patch)
2023-05-31 11:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33497: Add a filter relationship for branchtransfers (2.08 KB, patch)
2023-05-31 11:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33497: (follow-up) Remove unecessary scalar (900 bytes, patch)
2023-05-31 11:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33497: (follow-up) Adjust relationship name from bug 33493 (1.07 KB, patch)
2023-05-31 12:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33497: Use 'host_items' param to fetch all items at once (2.81 KB, patch)
2023-07-05 10:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33497: Add a filter relationship for branchtransfers (2.16 KB, patch)
2023-07-05 10:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33497: (follow-up) Remove unecessary scalar (975 bytes, patch)
2023-07-05 10:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33497: (follow-up) Adjust relationship name from bug 33493 (1.14 KB, patch)
2023-07-05 10:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33497: Don't get host items if the MARC record is invalid (2.01 KB, patch)
2023-07-07 13:52 UTC, Jonathan Druart
Details | Diff | Splinter Review

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