[Bug 29483] New: AllowRenewalIfOtherItemsAvailable has poor performance for records with many items
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Bug ID: 29483 Summary: AllowRenewalIfOtherItemsAvailable has poor performance for records with many items Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org When a record has many items and holds and the check to see if other items available can take a long time to return -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |29474 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 [Bug 29474] Automatic renewals cronjob is slow on systems with large numbers of reserves -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 127656 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127656&action=edit Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron ItemsAnyAvailableAndNotRestricted can take a long time and create nested loops. We can check it once per patron, however, this requires us to flip the loops. Since an item can only be used once, we now add a check to see if this item has already been assigned to a borrower. To test: 1 - Find or create a biblio with 100 items 2 - Place ten 'Next available' holds on a biblio 3 - Set preference 'AllowRenewalIfOtherItemsAvailable' to 'Allow' Set circ rules 'On shelf holds allowed' to 'If any unavailable' 4 - Checkout one of the items to a patron, backdated to be overdue 5 - Note a long loading time for the patron's checkouts 6 - Apply patch, restart_all 7 - Patron loads much faster -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #2 from Nick Clemens <nick@bywatersolutions.com> --- There is more that can be done here, but I think this first step is enough for this bug -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127656|0 |1 is obsolete| | --- Comment #3 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 127819 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127819&action=edit Bug 29483: Unit tests This patch updates the AllowRenewalIfOtherItemsAvailable tests to remove deletion of all data, and create specific circ rules for this test. It adjust several other tests that were relying on the rules from this test, so thy too create their opwn specific rules. Additionally, we add tests to cover the case of mutliple items on the record, and some items cannot fill some reserves. What is uncovered here is that the same patron is checked twice, so two holds can be filled, but they only satisfy a single patron -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #4 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 127820 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127820&action=edit Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron ItemsAnyAvailableAndNotRestricted can take a long time and create nested loops. We can check it once per patron, however, this requires us to flip the loops. Since an item can only be used once, we now add a check to see if this item has already been assigned to a borrower. To test: 1 - Find or create a biblio with 100 items 2 - Place ten 'Next available' holds on a biblio 3 - Set preference 'AllowRenewalIfOtherItemsAvailable' to 'Allow' Set circ rules 'On shelf holds allowed' to 'If any unavailable' 4 - Checkout one of the items to a patron, backdated to be overdue 5 - Note a long loading time for the patron's checkouts 6 - Apply patch, restart_all 7 - Patron loads much faster -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127819|0 |1 is obsolete| | Attachment #127820|0 |1 is obsolete| | --- Comment #5 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 127883 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127883&action=edit Bug 29483: Unit tests This patch updates the AllowRenewalIfOtherItemsAvailable tests to remove deletion of all data, and create specific circ rules for this test. It adjust several other tests that were relying on the rules from this test, so thy too create their opwn specific rules. Additionally, we add tests to cover the case of mutliple items on the record, and some items cannot fill some reserves. What is uncovered here is that the same patron is checked twice, so two holds can be filled, but they only satisfy a single patron -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #6 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 127884 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127884&action=edit Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron ItemsAnyAvailableAndNotRestricted can take a long time and create nested loops. We can check it once per patron, however, this requires us to flip the loops. Since an item can only be used once, we now add a check to see if this item has already been assigned to a borrower. To test: 1 - Find or create a biblio with 100 items 2 - Place ten 'Next available' holds on a biblio 3 - Set preference 'AllowRenewalIfOtherItemsAvailable' to 'Allow' Set circ rules 'On shelf holds allowed' to 'If any unavailable' 4 - Checkout one of the items to a patron, backdated to be overdue 5 - Note a long loading time for the patron's checkouts 6 - Apply patch, restart_all 7 - Patron loads much faster -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #7 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 127885 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127885&action=edit Bug 29483: Further improve performance of script This patch adds a few tests to cover more cases, and to highlight current functionality. The script only allows renewal if all outstanding holds can be filled by available items. This means we can return as soon as we have determined that not all holds can be filled. I add FIXME and some explanatory comments - I will file a follow-up bug for those, but I feel we can accept these improvements to the performance and deal with the issues of how it 'should' work versus how it does work on another report. To test: 1 - prove -v t/db_dependent/Circulation.t -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29531 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #8 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 129450 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129450&action=edit Bug 29843: Unit tests This patch adds unit tests for the introduced methods. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #9 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 129451 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129451&action=edit Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts This patch implements high-level methods for: * Filtering a Koha::Old::Checkouts resultset so only anonymizable rows are left * Anonymizing the rows in a resultset To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/Koha/Old/Checkouts.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #10 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 129452 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129452&action=edit Bug 29843: Use in batch_anonymise.pl This patch makes the batch_anonymise.pl cronjob script use the newly introduced methods instead of the old ones. To test: 1. Try the tool => SUCCESS: No behavior change 2. Sign off :-D Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #11 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 129453 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129453&action=edit Bug 29843: Use in opac/opac-privacy.pl This patch makes the opac/opac-privacy.pl OPAC page use the new anonymize method. To test: 1. Have some checked-in materiales 2. Have OPACPrivacy enabled 3. Notice your checkouts history contains what you expect 4. Go to 'your privacy' 5. Click on 'Delete checkout history' => SUCCESS: It works, no crash. 6. Sign off :-D Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #12 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 129454 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129454&action=edit Bug 29843: Use in tools/cleanborrowers.pl This patch makes tools/cleanborrowers.pl use the new methods. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127883|0 |1 is obsolete| | --- Comment #13 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 129499 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129499&action=edit Bug 29483: Unit tests This patch updates the AllowRenewalIfOtherItemsAvailable tests to remove deletion of all data, and create specific circ rules for this test. It adjust several other tests that were relying on the rules from this test, so thy too create their opwn specific rules. Additionally, we add tests to cover the case of mutliple items on the record, and some items cannot fill some reserves. What is uncovered here is that the same patron is checked twice, so two holds can be filled, but they only satisfy a single patron Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127884|0 |1 is obsolete| | --- Comment #14 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 129500 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129500&action=edit Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron ItemsAnyAvailableAndNotRestricted can take a long time and create nested loops. We can check it once per patron, however, this requires us to flip the loops. Since an item can only be used once, we now add a check to see if this item has already been assigned to a borrower. To test: 1 - Find or create a biblio with 100 items 2 - Place ten 'Next available' holds on a biblio 3 - Set preference 'AllowRenewalIfOtherItemsAvailable' to 'Allow' Set circ rules 'On shelf holds allowed' to 'If any unavailable' 4 - Checkout one of the items to a patron, backdated to be overdue 5 - Note a long loading time for the patron's checkouts 6 - Apply patch, restart_all 7 - Patron loads much faster Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127885|0 |1 is obsolete| | --- Comment #15 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 129501 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129501&action=edit Bug 29483: Further improve performance of script This patch adds a few tests to cover more cases, and to highlight current functionality. The script only allows renewal if all outstanding holds can be filled by available items. This means we can return as soon as we have determined that not all holds can be filled. I add FIXME and some explanatory comments - I will file a follow-up bug for those, but I feel we can accept these improvements to the performance and deal with the issues of how it 'should' work versus how it does work on another report. To test: 1 - prove -v t/db_dependent/Circulation.t Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com Attachment #129453|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129454|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129499|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129500|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129501|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #16 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Accidentally put a signoff for another bug here. I've now *actually* signed off here. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Bug 29483 depends on bug 29474, which changed state. Bug 29474 Summary: Automatic renewals cronjob is slow on systems with large numbers of reserves https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m, | |jonathan.druart+koha@gmail. | |com, tomascohen@gmail.com --- Comment #17 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I don't understand what happened here, this is in master already: Bug 29843: Use in opac/opac-privacy.pl Bug 29843: Use in tools/cleanborrowers.pl Bug 29843: Use in batch_anonymise.pl Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts Bug 29843: Unit tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129499|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129500|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129501|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129450|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129451|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129452|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #18 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Jonathan Druart from comment #17)
I don't understand what happened here, this is in master already:
Bug 29843: Use in opac/opac-privacy.pl Bug 29843: Use in tools/cleanborrowers.pl Bug 29843: Use in batch_anonymise.pl Bug 29843: Add ->anonymize and ->filter_by_anonymizable to Koha::Old::Checkouts Bug 29843: Unit tests
ok got it, all sorted out now. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply --- Comment #19 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- And so finally it does not apply cleanly. CONFLICT (content): Merge conflict in C4/Circulation.pm Recorded preimage for 'C4/Circulation.pm' error: Failed to merge in the changes. Patch failed at 0001 Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129499|0 |1 is obsolete| | --- Comment #20 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 132094 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=132094&action=edit Bug 29483: Unit tests This patch updates the AllowRenewalIfOtherItemsAvailable tests to remove deletion of all data, and create specific circ rules for this test. It adjust several other tests that were relying on the rules from this test, so thy too create their opwn specific rules. Additionally, we add tests to cover the case of mutliple items on the record, and some items cannot fill some reserves. What is uncovered here is that the same patron is checked twice, so two holds can be filled, but they only satisfy a single patron Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129500|0 |1 is obsolete| | --- Comment #21 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 132095 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=132095&action=edit Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron ItemsAnyAvailableAndNotRestricted can take a long time and create nested loops. We can check it once per patron, however, this requires us to flip the loops. Since an item can only be used once, we now add a check to see if this item has already been assigned to a borrower. To test: 1 - Find or create a biblio with 100 items 2 - Place ten 'Next available' holds on a biblio 3 - Set preference 'AllowRenewalIfOtherItemsAvailable' to 'Allow' Set circ rules 'On shelf holds allowed' to 'If any unavailable' 4 - Checkout one of the items to a patron, backdated to be overdue 5 - Note a long loading time for the patron's checkouts 6 - Apply patch, restart_all 7 - Patron loads much faster Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129501|0 |1 is obsolete| | --- Comment #22 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 132096 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=132096&action=edit Bug 29483: Further improve performance of script This patch adds a few tests to cover more cases, and to highlight current functionality. The script only allows renewal if all outstanding holds can be filled by available items. This means we can return as soon as we have determined that not all holds can be filled. I add FIXME and some explanatory comments - I will file a follow-up bug for those, but I feel we can accept these improvements to the performance and deal with the issues of how it 'should' work versus how it does work on another report. To test: 1 - prove -v t/db_dependent/Circulation.t Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #23 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Did you benchmark? Do you have numbers? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #132096|0 |1 is obsolete| | --- Comment #24 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 132113 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=132113&action=edit Bug 29483: Further improve performance of script This patch adds a few tests to cover more cases, and to highlight current functionality. The script only allows renewal if all outstanding holds can be filled by available items. This means we can return as soon as we have determined that not all holds can be filled. I add FIXME and some explanatory comments - I will file a follow-up bug for those, but I feel we can accept these improvements to the performance and deal with the issues of how it 'should' work versus how it does work on another report. To test: 1 - prove -v t/db_dependent/Circulation.t Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #25 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #23)
Did you benchmark? Do you have numbers?
Following the test plan I setup a patron with an item checked out from a bib with 100 books Loading the checkouts directly: http://localhost:8081/cgi-bin/koha/svc/checkouts?borrowernumber=5 Before patch: > 4 seconds (~4.4s) After patch: < 1 second (~750ms) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #132094|0 |1 is obsolete| | --- Comment #26 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 133389 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=133389&action=edit Bug 29483: Unit tests This patch updates the AllowRenewalIfOtherItemsAvailable tests to remove deletion of all data, and create specific circ rules for this test. It adjust several other tests that were relying on the rules from this test, so thy too create their opwn specific rules. Additionally, we add tests to cover the case of mutliple items on the record, and some items cannot fill some reserves. What is uncovered here is that the same patron is checked twice, so two holds can be filled, but they only satisfy a single patron Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #132095|0 |1 is obsolete| | --- Comment #27 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 133390 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=133390&action=edit Bug 29483: Check ItemsAnyAvailableAndNotRestricted once per patron ItemsAnyAvailableAndNotRestricted can take a long time and create nested loops. We can check it once per patron, however, this requires us to flip the loops. Since an item can only be used once, we now add a check to see if this item has already been assigned to a borrower. To test: 1 - Find or create a biblio with 100 items 2 - Place ten 'Next available' holds on a biblio 3 - Set preference 'AllowRenewalIfOtherItemsAvailable' to 'Allow' Set circ rules 'On shelf holds allowed' to 'If any unavailable' 4 - Checkout one of the items to a patron, backdated to be overdue 5 - Note a long loading time for the patron's checkouts 6 - Apply patch, restart_all 7 - Patron loads much faster Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #132113|0 |1 is obsolete| | --- Comment #28 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 133391 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=133391&action=edit Bug 29483: Further improve performance of script This patch adds a few tests to cover more cases, and to highlight current functionality. The script only allows renewal if all outstanding holds can be filled by available items. This means we can return as soon as we have determined that not all holds can be filled. I add FIXME and some explanatory comments - I will file a follow-up bug for those, but I feel we can accept these improvements to the performance and deal with the issues of how it 'should' work versus how it does work on another report. To test: 1 - prove -v t/db_dependent/Circulation.t Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Status|Signed Off |Passed QA --- Comment #29 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- It took me a little while to wrap my head around the logic change, but all made sense in the end. I concur, it does improve performance and I found no regressions. QA scripts are happy and unit tests are all passing. Passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@ptfs-europe |y.org |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |22.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #30 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to master for 22.05, thanks to everybody involved 🦄 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com Resolution|--- |FIXED Status|Pushed to master |RESOLVED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #31 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 134723 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=134723&action=edit Bug 29483: [21.11.x] Check ItemsAnyAvailableAndNotRestricted once per patron ItemsAnyAvailableAndNotRestricted can take a long time and create nested loops. We can check it once per patron, however, this requires us to flip the loops. Since an item can only be used once, we now add a check to see if this item has already been assigned to a borrower. To test: 1 - Find or create a biblio with 100 items 2 - Place ten 'Next available' holds on a biblio 3 - Set preference 'AllowRenewalIfOtherItemsAvailable' to 'Allow' Set circ rules 'On shelf holds allowed' to 'If any unavailable' 4 - Checkout one of the items to a patron, backdated to be overdue 5 - Note a long loading time for the patron's checkouts 6 - Apply patch, restart_all 7 - Patron loads much faster Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 29483: Unit tests This patch updates the AllowRenewalIfOtherItemsAvailable tests to remove deletion of all data, and create specific circ rules for this test. It adjust several other tests that were relying on the rules from this test, so thy too create their opwn specific rules. Additionally, we add tests to cover the case of mutliple items on the record, and some items cannot fill some reserves. What is uncovered here is that the same patron is checked twice, so two holds can be filled, but they only satisfy a single patron Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 29483: Further improve performance of script This patch adds a few tests to cover more cases, and to highlight current functionality. The script only allows renewal if all outstanding holds can be filled by available items. This means we can return as soon as we have determined that not all holds can be filled. I add FIXME and some explanatory comments - I will file a follow-up bug for those, but I feel we can accept these improvements to the performance and deal with the issues of how it 'should' work versus how it does work on another report. To test: 1 - prove -v t/db_dependent/Circulation.t Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|FIXED |--- Status|RESOLVED |REOPENED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|REOPENED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|22.05.00 |22.05.00,21.11.06 released in| | Status|Pushed to master |Pushed to stable --- Comment #32 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to 21.11.x for 21.11.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |RESOLVED Resolution|--- |FIXED CC| |victor@tuxayo.net --- Comment #33 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Backported: Pushed to 21.05.x branch for 21.05.16 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29483 --- Comment #34 from Victor Grousset/tuxayo <victor@tuxayo.net> --- ↑↑↑↑↑ errata, wrong message Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org