We recently had time-out issues and high server loads for patrons with high checkout counts (some patrons have several hundreds, up to about 400). One of the major bottlenecks turned out to be C4::Circulation::CanBookBeRenewed. The primary reason it performed so poorly was constant re-fetching of different database entities which could just as well be passed down directly instead of passing ids which requires re-fetching further down the call stack. After refactoring the most performance critical subroutines to take objects instead of object ids the calls to expensive DBIx-methods has been greatly reduced. In addition some other optimizations and bug fixes related to non working caching resulted in further speedups. The performance gain for a patron with around 400 checkouts is currently about 500%. I made an effort to fix all tests affected by the changes, but I can not guarantee there are absolutely no regressions left in the test-suite. Since many tests seem to fail due to reasons not related to this patch, mainly due to the state of my local instance triggering tests-bugs (for example caused by preferences not being mocked correctly, or relying on database test-entries that are not guaranteed to exist), it was impractical and very time consuming to make sure all tests still run successfully. So subset of tests where the changed subroutines are called directly was selected and these should now pass. The following tests should be effected by the changes and should be run as a part of the test-plan: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t The following still fails for me but should either work on a fresh Koha installation, or fail for unrelated reasons (that is they also fail in Koha master): t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Items.t t/db_dependent/SIP/ILS.t To be able to run t/db_dependent/Circulation.t in my Koha instance there where some existing bugs that needed fixing, so this patch have a dependency on Bug 31676.
Created attachment 141603 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library
Created attachment 141604 [details] [review] Bug 31735: Avoid unesssary unblessing of objects
Created attachment 141605 [details] [review] Bug 31735: Add caching for relatively expensive patron attributes
Created attachment 141606 [details] [review] Bug 31735: Remove Koha::Config::SysPref->find since bypasses cache get_yaml_pref_hash also allows invalid YAML and only parses a limited subset so remove this method to avoid future issues.
Created attachment 141607 [details] [review] Bug 31735: Match rules on effective item type instead of itype
Created attachment 141608 [details] [review] Bug 31735: Optimize OPAC checkouts view
Created attachment 141609 [details] Benchmark before
Created attachment 141610 [details] Benchmark after
Created attachment 141795 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library
Created attachment 141796 [details] [review] Bug 31735: Avoid unesssary unblessing of objects
Created attachment 141797 [details] [review] Bug 31735: Add caching for relatively expensive patron attributes
Created attachment 141798 [details] [review] Bug 31735: Remove Koha::Config::SysPref->find since bypasses cache get_yaml_pref_hash also allows invalid YAML and only parses a limited subset so remove this method to avoid future issues.
Created attachment 141799 [details] [review] Bug 31735: Match rules on effective item type instead of itype
Created attachment 141800 [details] [review] Bug 31735: Optimize OPAC checkouts view
Created attachment 141819 [details] [review] Bug 31735: Remove Koha::Config::SysPref->find since bypasses cache get_yaml_pref_hash also allows invalid YAML and only parses a limited subset so remove this method to avoid future issues.
Created attachment 141820 [details] [review] Bug 31735: Match rules on effective item type instead of itype
Created attachment 141821 [details] [review] Bug 31735: Optimize OPAC checkouts view
Created attachment 142865 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library
Created attachment 142866 [details] [review] Bug 31735: Avoid unesssary unblessing of objects
Created attachment 142867 [details] [review] Bug 31735: Add caching for relatively expensive patron attributes
Created attachment 142868 [details] [review] Bug 31735: Remove Koha::Config::SysPref->find since bypasses cache get_yaml_pref_hash also allows invalid YAML and only parses a limited subset so remove this method to avoid future issues.
Created attachment 142869 [details] [review] Bug 31735: Match rules on effective item type instead of itype
Created attachment 142870 [details] [review] Bug 31735: Optimize OPAC checkouts view
Optimizations are always good, but reading a bug title as general like this, always makes me suspicious. I feel like this should be an omnibus with separate dependent bugs for the identified issues. If they need to be done in a certain sequence (which seems likely for conflicts) they can also be linked to each other using depends. I'd strongly recommend 'small steps' over big massive patch sets, as they tend to get stuck in queues because it's too hard to find the time to test them in one session and they are also most of the time scary.
Sorry about the late reply. Yes I should probably have split this one up into several issues and fully understand this is would be very cumbersome to to review all at once. I anticipated this could be the case so each commit should already be pretty isolated and even though there are some dependencies breaking them into separate issues is doable. Have begun in doing so and changing status of this one until ready to review again.
Created attachment 144628 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library
Created attachment 144629 [details] [review] Bug 31735: Avoid unesssary unblessing of objects
Created attachment 144630 [details] [review] Bug 31735: Remove Koha::Config::SysPref->find since bypasses cache get_yaml_pref_hash also allows invalid YAML and only parses a limited subset so remove this method to avoid future issues.
Created attachment 144631 [details] [review] Bug 31735: Match rules on effective item type instead of itype
Created attachment 144632 [details] [review] Bug 31735: Optimize OPAC checkouts view
Created attachment 144635 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library
Have moved two of the commits to bug 32476 and bug 32478 which has no dependencies. What is left to do is create an issue depending on this one with the changes regarding uncalled-for unblessings of Koha objects. This one should now be ready for review.
Created attachment 144721 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library
Created attachment 144724 [details] [review] Bug 31735: Optimize OPAC checkouts view
I had a go at testing. Failed QA because tests failed and there were error traces when renewing items from the OPAC. Tested using KTD (koha-testing-docker) with the sample data. Test failures ============= These tests failed - note that I didn't check whether these failed before the patch was applied: prove t/db_dependent/api/v1/biblios.t prove t/db_dependent/api/v1/checkouts.t prove t/db_dependent/api/v1/return_claims.t prove t/db_dependent/Circulation/Returns.t prove t/db_dependent/Circulation.t prove t/db_dependent/Circulation/transferbook.t prove t/db_dependent/Illrequests.t prove t/db_dependent/Koha/Account/Line.t prove t/db_dependent/Koha/Items.t prove t/db_dependent/Koha/Object.t prove t/db_dependent/Koha/Patrons.t prove t/db_dependent/Koha/Recalls.t prove t/db_dependent/Koha/Recall.t prove t/db_dependent/Letters/TemplateToolkit.t prove t/db_dependent/Reserves.t prove t/db_dependent/selenium/regressions.t prove t/db_dependent/SIP/ILS.t Renewing from the OPAC - errors =============================== I checked out three different items to one patron, making one of them already overdue. The default circulation rule in KTD allows 5 renewals. Error 1: Renew a single overdue item from the OPAC (Summary > Overdue tab) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ SQL::Abstract::puke(): [SQL::Abstract::__ANON__] Fatal: Argument passed to the 'IN' operator can not be undefined at /kohadevbox/koha/Koha/Objects.pm line 312 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 in DBIx::Class::Exception::throw at /usr/share/perl5/DBIx/Class/Exception.pm line 77 ...... Note that it renewed fine from Summary > Checkout tab. Error 2: Renew multiple items (two items individually selected) from the Summary > Checked out tab (where one is overdue) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ SQL::Abstract::puke(): [SQL::Abstract::__ANON__] Fatal: unexpected operator '715' with undef operand at /kohadevbox/koha/Koha/Objects.pm line 312 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 in DBIx::Class::Exception::throw at /usr/share/perl5/DBIx/Class/Exception.pm line 77 .... Error 3: Renew multiple items (renew all button selected) from the Summary > Checked out tab where one is overdue: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ SQL::Abstract::puke(): [SQL::Abstract::__ANON__] Fatal: Argument passed to the 'IN' operator can not be undefined at /kohadevbox/koha/Koha/Objects.pm line 312 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 in DBIx::Class::Exception::throw at /usr/share/perl5/DBIx/Class/Exception.pm line 77 ....
Created attachment 144784 [details] [review] Bug 31735: Fix array that should be passed as reference
Created attachment 144790 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library
Created attachment 144791 [details] [review] Bug 31735: Optimize OPAC checkouts view
Created attachment 144792 [details] [review] Bug 31735: Fix array that should be passed as reference
Thanks for the review. I obviously failed to manually test the opac-view, this is now fixed (the array for the in-condition should have been passed by reference). Regarding the failing tests, it's surprising so many are failing. I pretty sure I originally ran through them successfully, so it could be that those test regressions have been introduced after the rebase was performed, even though it seems odd regarding how many are now failing. I ran through those that failed for you on the current master, and got the following results: Passing: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/transferbook.t Failing: t/db_dependent/api/v1/checkouts.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation.t t/db_dependent/Illrequests.t t/db_dependent/Koha/Account/Line.t (foreign key constraint, could work on clean Koha with only test data) t/db_dependent/Koha/Items.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Letters/TemplateToolkit.t (Duplicate entry 'a_t_barcode', could work with test data) t/db_dependent/Reserves.t t/db_dependent/SIP/ILS.t (foreign key constraint, might work with test data) The ones passing also pass after applying the patches (though there were some changes in bug 32496 that should have been included here which I have now moved for transferbook.t to pass). I haven't used koha-testing-docker before, but could try to run the tests there and see if I get the same result (for koha-master). Feel free to do the same if you have the time I don't know how easy it will be to get it up and running. It could be that the current test suite is pretty broken, even though it seems odd this has gone under the radar regarding the number of failing tests.
Hi David. Thanks for the updates! I'll have another go at testing in the next day and let you know how I get on - I'll run the tests before applying the patch to see if any have "problems". David
Ok! Thanks. Got the test-suite working and most tests that failed in my local docker setup now works, so must be some issue with missing test-data or some other difference in environment. You don't have to run the tests, I will have a look at the tests that passes on master in KTD but fails after applying the patch.
Created attachment 144798 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library
Created attachment 144799 [details] [review] Bug 31735: Optimize OPAC checkouts view
Created attachment 144800 [details] [review] Bug 31735: Fix array that should be passed as reference
Created attachment 144801 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t t/db_dependent/Holds.t t/db_dependent/Holds/LocalHoldsPriority.t t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Holds/HoldItemtypeLimit.t t/db_dependent/Circulation/transferbook.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library
Created attachment 144802 [details] [review] Bug 31735: Optimize OPAC checkouts view
Created attachment 144803 [details] [review] Bug 31735: Fix array that should be passed as reference
The tests should now be passing, must have previously failed to run all relevant tests (added a few), and found some issues that now should be fixed.
Running the entire test-suite and will check the results tomorrow, in case there are any other tests affected that also failed to check.
Koha testing docker is really not working all that well for me, it keeps crashing erratically, some times starts up successfully, very often crashes for a host of different reasons. Thus it's very hard for me to currently judge if there are more tests then those listed affected by the changes. Feel free to run the test suite with and without the patch set, for me it seems to fail at various points in both cases, but it's possible the reason is my ktd-environment is screwed up someway.
Created attachment 145028 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t t/db_dependent/Holds.t t/db_dependent/Holds/LocalHoldsPriority.t t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Holds/HoldItemtypeLimit.t t/db_dependent/Circulation/transferbook.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library
Created attachment 145029 [details] [review] Bug 31735: Optimize OPAC checkouts view
Created attachment 145030 [details] [review] Bug 31735: Fix array that should be passed as reference
Created attachment 145032 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t t/db_dependent/Holds.t t/db_dependent/Holds/LocalHoldsPriority.t t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Holds/HoldItemtypeLimit.t t/db_dependent/Circulation/transferbook.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library
Created attachment 145033 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t t/db_dependent/Holds.t t/db_dependent/Holds/LocalHoldsPriority.t t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Holds/HoldItemtypeLimit.t t/db_dependent/Circulation/transferbook.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library
Created attachment 145034 [details] [review] Bug 31735: Optimize OPAC checkouts view
Created attachment 145035 [details] [review] Bug 31735: Fix array that should be passed as reference
Created attachment 146735 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t t/db_dependent/Holds.t t/db_dependent/Holds/LocalHoldsPriority.t t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Holds/HoldItemtypeLimit.t t/db_dependent/Circulation/transferbook.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library
Created attachment 146736 [details] [review] Bug 31735: Optimize OPAC checkouts view Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 146737 [details] [review] Bug 31735: Fix array that should be passed as reference Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Getting one unit test failure: root@kohadevbox:koha(bug31735-qa)$ prove t/db_dependent/Koha/CirculationRules.t t/db_dependent/Koha/CirculationRules.t .. 1/7 # Failed test 'No rule for branch, no default rule, fallback default (refund)' # at t/db_dependent/Koha/CirculationRules.t line 856. # Structures begin differing at: # $got->{lostreturn} = '0' # $expected->{lostreturn} = 'refund' # Looks like you failed 1 test of 7. # Failed test 'get_lostreturn_policy() tests' # at t/db_dependent/Koha/CirculationRules.t line 865. # Looks like you failed 1 test of 7. t/db_dependent/Koha/CirculationRules.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/7 subtests Test Summary Report ------------------- t/db_dependent/Koha/CirculationRules.t (Wstat: 256 Tests: 7 Failed: 1) Failed test: 7 Non-zero exit status: 1 Files=1, Tests=7, 3 wallclock secs ( 0.01 usr 0.00 sys + 1.90 cusr 0.13 csys = 2.04 CPU) Result: FAIL
Created attachment 147124 [details] [review] Bug 31735: Optimize OPAC checkouts view
Created attachment 147125 [details] [review] Bug 31735: Fix array that should be passed as reference
Created attachment 147126 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t t/db_dependent/Holds.t t/db_dependent/Holds/LocalHoldsPriority.t t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Holds/HoldItemtypeLimit.t t/db_dependent/Circulation/transferbook.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library
Created attachment 147127 [details] [review] Bug 31735: Optimize OPAC checkouts view
Created attachment 147128 [details] [review] Bug 31735: Fix array that should be passed as reference
(In reply to Kyle M Hall from comment #62) > Getting one unit test failure: > > root@kohadevbox:koha(bug31735-qa)$ prove > t/db_dependent/Koha/CirculationRules.t > t/db_dependent/Koha/CirculationRules.t .. 1/7 > # Failed test 'No rule for branch, no default rule, fallback default > (refund)' > # at t/db_dependent/Koha/CirculationRules.t line 856. > # Structures begin differing at: > # $got->{lostreturn} = '0' > # $expected->{lostreturn} = 'refund' > # Looks like you failed 1 test of 7. > > # Failed test 'get_lostreturn_policy() tests' > # at t/db_dependent/Koha/CirculationRules.t line 865. > # Looks like you failed 1 test of 7. > t/db_dependent/Koha/CirculationRules.t .. Dubious, test returned 1 (wstat > 256, 0x100) > Failed 1/7 subtests > > Test Summary Report > ------------------- > t/db_dependent/Koha/CirculationRules.t (Wstat: 256 Tests: 7 Failed: 1) > Failed test: 7 > Non-zero exit status: 1 > Files=1, Tests=7, 3 wallclock secs ( 0.01 usr 0.00 sys + 1.90 cusr 0.13 > csys = 2.04 CPU) > Result: FAIL Ok! This should now be fixed. Was actually a bug in the test (not clearing cache after changing rule ) exposed by fixing caching of falsy values. That fix however should not be in this bug but in Bug 32092, so removed the change and the test should now pass.
Created attachment 147164 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t t/db_dependent/Holds.t t/db_dependent/Holds/LocalHoldsPriority.t t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Holds/HoldItemtypeLimit.t t/db_dependent/Circulation/transferbook.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147165 [details] [review] Bug 31735: Optimize OPAC checkouts view Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147166 [details] [review] Bug 31735: Fix array that should be passed as reference Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
I am getting a conflict here that I managed to resolve, but then a "sha1 lacking or useless" on bug 32496. Could you rebase and eventually push both to a remote branch?
Created attachment 150611 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t t/db_dependent/Holds.t t/db_dependent/Holds/LocalHoldsPriority.t t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Holds/HoldItemtypeLimit.t t/db_dependent/Circulation/transferbook.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 150612 [details] [review] Bug 31735: Optimize OPAC checkouts view Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 150613 [details] [review] Bug 31735: Fix array that should be passed as reference Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Rebased against master.
(In reply to Jonathan Druart from comment #72) > I am getting a conflict here that I managed to resolve, but then a "sha1 > lacking or useless" on bug 32496. Could you rebase and eventually push both > to a remote branch? I pushed the patches to https://gitlab.com/joubu/Koha/-/commits/bug_32496 in case we need it later.
Created attachment 150676 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t t/db_dependent/Holds.t t/db_dependent/Holds/LocalHoldsPriority.t t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Holds/HoldItemtypeLimit.t t/db_dependent/Circulation/transferbook.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 150677 [details] [review] Bug 31735: Optimize OPAC checkouts view Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 150678 [details] [review] Bug 31735: Fix array that should be passed as reference Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 150699 [details] [review] Bug 31735: Avoid re-fetcing objects from database by passing them directly instead of ids to various subroutines To test: 1) Run the following test and make sure all pass: t/db_dependent/api/v1/biblios.t t/db_dependent/api/v1/checkouts.t t/db_dependent/api/v1/return_claims.t t/db_dependent/Circulation/CalcDateDue.t t/db_dependent/Circulation/CheckIfIssuedToPatron.t t/db_dependent/Circulation/dateexpiry.t t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t t/db_dependent/Circulation/GetTopIssues.t t/db_dependent/Circulation_holdsqueue.t t/db_dependent/Circulation/IsItemIssued.t t/db_dependent/Circulation/issue.t t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Circulation/maxsuspensiondays.t t/db_dependent/Circulation/ReturnClaims.t t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/SwitchOnSiteCheckouts.t t/db_dependent/Circulation.t t/db_dependent/Circulation/TooMany.t t/db_dependent/Circulation/transferbook.t t/db_dependent/DecreaseLoanHighHolds.t t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t t/db_dependent/HoldsQueue.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/Illrequests.t t/db_dependent/ILSDI_Services.t t/db_dependent/Items.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Acquisition/Order.t t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Holds.t t/db_dependent/Koha/Items.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Object.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Plugins/Circulation_hooks.t t/db_dependent/Koha/Pseudonymization.t t/db_dependent/Koha/Recalls.t t/db_dependent/Koha/Recall.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Members/GetAllIssues.t t/db_dependent/Members/IssueSlip.t t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_PrevCheckout.t t/db_dependent/Reserves/GetReserveFee.t t/db_dependent/Reserves.t t/db_dependent/rollingloans.t t/db_dependent/selenium/regressions.t t/db_dependent/SIP/ILS.t t/db_dependent/Holds.t t/db_dependent/Holds/LocalHoldsPriority.t t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Holds/HoldItemtypeLimit.t t/db_dependent/Circulation/transferbook.t 2) Performe one or more checkouts for a patron, making sure that the circulation rules allows for renewals (for example by setting an earlier due-date). 3) Log in as this patron in OPAC and make sure the list of checkouts is displayed correctly, and that renewing an issue still works. Sponsored-by: Gothenburg University Library Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 150700 [details] [review] Bug 31735: Optimize OPAC checkouts view Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 150701 [details] [review] Bug 31735: Fix array that should be passed as reference Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com>
I've tested and added my sign-off (status was set to needs signoff - maybe Kyle or Nick didn't change the status?). Testing notes (using KTD): - All tests pass before after the patch is applied (ran prove -v t/db_dependent/) - Everything works as per the test plan - I also checked out an item with a due date before today and checked the automatic renewal, then ran the automatic renewal script misc/cronjobs/automatic_renwal.pl -v -c
Pushed to master for 23.05. Nice work everyone, thanks!
Enhancement - not backporting to 22.11.x