|
Description
Nick Clemens (kidclamp)
2021-11-15 12:34:58 UTC
Created attachment 127656 [details] [review] 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 There is more that can be done here, but I think this first step is enough for this bug Created attachment 127819 [details] [review] 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 Created attachment 127820 [details] [review] 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 Created attachment 127883 [details] [review] 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 Created attachment 127884 [details] [review] 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 Created attachment 127885 [details] [review] 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 Created attachment 129450 [details] [review] Bug 29843: Unit tests This patch adds unit tests for the introduced methods. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 129451 [details] [review] 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> Created attachment 129452 [details] [review] 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> Created attachment 129453 [details] [review] 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> Created attachment 129454 [details] [review] 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> Created attachment 129499 [details] [review] 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> Created attachment 129500 [details] [review] 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> Created attachment 129501 [details] [review] 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> Accidentally put a signoff for another bug here. I've now *actually* signed off here. 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 (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. 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 Created attachment 132094 [details] [review] 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> Created attachment 132095 [details] [review] 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> Created attachment 132096 [details] [review] 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> Did you benchmark? Do you have numbers? Created attachment 132113 [details] [review] 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> (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) Created attachment 133389 [details] [review] 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> Created attachment 133390 [details] [review] 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> Created attachment 133391 [details] [review] 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> 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 Pushed to master for 22.05, thanks to everybody involved [U+1F984] Created attachment 134723 [details] [review] 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> Pushed to 21.11.x for 21.11.06 Backported: Pushed to 21.05.x branch for 21.05.16 ↑↑↑↑↑ errata, wrong message Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed. |