Bug 30269 - Use objects directly to display holds on request.pl
Summary: Use objects directly to display holds on request.pl
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 30266
Blocks: 30270
  Show dependency treegraph
 
Reported: 2022-03-10 14:10 UTC by Nick Clemens
Modified: 2022-07-29 19:07 UTC (History)
4 users (show)

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


Attachments
Bug 30269: Use objects directly on request.pl (20.16 KB, patch)
2022-03-10 14:13 UTC, Nick Clemens
Details | Diff | Splinter Review
Perf check (999 bytes, application/x-perl)
2022-03-10 14:31 UTC, Nick Clemens
Details
Bug 30269: Use objects directly on request.pl (20.16 KB, patch)
2022-03-25 13:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30269: Use objects directly on request.pl (20.20 KB, patch)
2022-05-03 15:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30269: (follow-up) Remove object references as using directly now (3.40 KB, patch)
2022-05-03 15:45 UTC, Nick Clemens
Details | Diff | Splinter Review
hold page incorrect display after applying the patch (145.38 KB, image/png)
2022-06-30 16:18 UTC, Marie-Luce Laflamme
Details
Bug 30269: Use objects directly on request.pl (21.41 KB, patch)
2022-07-14 14:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30269: (follow-up) Remove object references as using directly now (3.40 KB, patch)
2022-07-14 14:34 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2022-03-10 14:10:21 UTC
request.pl fetches holds objects, then loops over them to build a hash for display - we don't need this intermediate step.
Comment 1 Nick Clemens 2022-03-10 14:13:17 UTC
Created attachment 131558 [details] [review]
Bug 30269: Use objects directly on request.pl

This patch removes our loop over the hold objects, and simply passes them directly to the template

Template is adjusted to use ojbects and accessor method
Related objects are prefetched to avoid more DB calls when display
Use of 'iswaiting' and related methods are removed - it is faster to use the accessor and test

To test:
1 - Place many holds on a record
2 - Set soem waiting, some in transit
3 - View the holds page and note loading time
4 - Apply patch
5 - Reload
6 - Page appears the same
7 - Load time is faster (more with more holds)
Comment 2 Nick Clemens 2022-03-10 14:31:33 UTC
Created attachment 131559 [details]
Perf check

This compared the time for using accessor versus subroutines for checking transit, waiting, etc.

accessor were faster in both cases, but more drastically so in case of a found hold

Unfound hold:
                 Rate objectMethodAtD objectMethodIsW objectMethodIsF objectAccessorW objectAccessorF
objectMethodAtD 546/s              --             -1%             -5%             -6%             -8%
objectMethodIsW 553/s              1%              --             -4%             -5%             -7%
objectMethodIsF 576/s              6%              4%              --             -1%             -3%
objectAccessorW 579/s              6%              5%              1%              --             -3%
objectAccessorF 596/s              9%              8%              3%              3%              --

Found hold:
                 Rate objectMethodIsF objectMethodAtD objectMethodIsW objectAccessorW objectAccessorF
objectMethodIsF 183/s              --            -63%            -65%            -67%            -68%
objectMethodAtD 500/s            173%              --             -3%             -9%            -12%
objectMethodIsW 516/s            182%              3%              --             -7%            -10%
objectAccessorW 552/s            201%             10%              7%              --             -3%
objectAccessorF 572/s            212%             14%             11%              4%              --
Comment 3 Tomás Cohen Arazi 2022-03-23 19:57:52 UTC
I love this patch, but can you name the relationship 'library' instead of 'branch'? Thanks!
Comment 4 Nick Clemens 2022-03-25 13:01:16 UTC
Created attachment 132245 [details] [review]
Bug 30269: Use objects directly on request.pl

This patch removes our loop over the hold objects, and simply passes them directly to the template

Template is adjusted to use ojbects and accessor method
Related objects are prefetched to avoid more DB calls when display
Use of 'iswaiting' and related methods are removed - it is faster to use the accessor and test

To test:
1 - Place many holds on a record
2 - Set soem waiting, some in transit
3 - View the holds page and note loading time
4 - Apply patch
5 - Reload
6 - Page appears the same
7 - Load time is faster (more with more holds)
Comment 5 Nick Clemens 2022-05-03 15:32:54 UTC
Created attachment 134548 [details] [review]
Bug 30269: Use objects directly on request.pl

This patch removes our loop over the hold objects, and simply passes them directly to the template

Template is adjusted to use ojbects and accessor method
Related objects are prefetched to avoid more DB calls when display
Use of 'iswaiting' and related methods are removed - it is faster to use the accessor and test

To test:
1 - Place many holds on a record
2 - Set soem waiting, some in transit
3 - View the holds page and note loading time
4 - Apply patch
5 - Reload
6 - Page appears the same
7 - Load time is faster (more with more holds)
Comment 6 Nick Clemens 2022-05-03 15:45:43 UTC
Created attachment 134552 [details] [review]
Bug 30269: (follow-up) Remove object references as using directly now
Comment 7 Owen Leonard 2022-05-04 18:20:47 UTC
I'm getting this error when I try to place the hold, after selecting the patron:

Broken FK constraint at /usr/share/perl5/Exception/Class/Base.pm line 88
Comment 8 Nick Clemens 2022-05-05 13:46:38 UTC
I cannot recreate, can you test again?
Comment 9 Marie-Luce Laflamme 2022-06-30 16:16:35 UTC
Patch doesn't apply. I've followed the test plan

1 - Place many holds on a record
2 - Set some waiting, some in transit
3 - View the holds page and note loading time
4 - Apply patch
5 - Reload
6 - Page appears the same
7 - Load time is faster (more with more holds)

Somehow, the reserve/request.pl?biblionumber= display changed after applying the patch. It shows up a new button with the bugnumber.


see my test in this sandboxe https://staff-test.sandbox.bywatersolutions.com/cgi-bin/koha/reserve/request.pl?biblionumber=144
Comment 10 Marie-Luce Laflamme 2022-06-30 16:18:23 UTC
Created attachment 136794 [details]
hold page incorrect display after applying the patch
Comment 11 Nick Clemens 2022-07-14 14:34:20 UTC
Created attachment 137725 [details] [review]
Bug 30269: Use objects directly on request.pl

This patch removes our loop over the hold objects, and simply passes them directly to the template

Template is adjusted to use ojbects and accessor method
Related objects are prefetched to avoid more DB calls when display
Use of 'iswaiting' and related methods are removed - it is faster to use the accessor and test

To test:
1 - Place many holds on a record
2 - Set soem waiting, some in transit
3 - View the holds page and note loading time
4 - Apply patch
5 - Reload
6 - Page appears the same
7 - Load time is faster (more with more holds)
Comment 12 Nick Clemens 2022-07-14 14:34:23 UTC
Created attachment 137726 [details] [review]
Bug 30269: (follow-up) Remove object references as using directly now
Comment 13 Andrew Fuerste-Henry 2022-07-29 19:07:41 UTC
With this patch applied on a sandbox, I am not able to open request.pl for a bib with holds on it. I get the following error:
"Template process failed: undef error - The method Koha::Hold->date is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Object.pm line 875"

After a bunch of error output, it goes on to say:
"in C4::Templates::output at /kohadevbox/koha/C4/Templates.pm line 127"