Bug 34178

Summary: Add caching of ItemsAnyAvailableAndNotRestricted to IsAvailableForItemLevelRequest
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: Hold requestsAssignee: Nick Clemens (kidclamp) <nick>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: emily.lamancusa, fridolin.somers, gmcharlt, johanna.raisa, jonathan.druart, kyle, martin.renvoize, pedro.amorim, samalau
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.02
Bug Depends on:    
Bug Blocks: 33746    
Attachments: Bug 34178: Cache ItemsAnyAvailableAndNotRestricted in memory and don't precalculate
Bug 34178: Adjust tests
Bug 34178: Cache ItemsAnyAvailableAndNotRestricted in memory and don't precalculate
Bug 34178: Adjust tests
Bug 34178: Cache ItemsAnyAvailableAndNotRestricted in memory and don't precalculate
Bug 34178: Adjust tests
Bug 34178: (QA follow-up) Tidy

Description Nick Clemens (kidclamp) 2023-06-30 19:25:42 UTC
There are several places in the code where we precalculate ItemsAnyAvailableAndNotRestricted to avoid looping on this routinee when calling IsAvailableForItemLevelRequest on a list of items form a biblio

The value of ItemsAnyAvailableAndNotRestricted is only used when there is a circulation rule for 'onshelfholds' with a value of '2' (If all unavailable)

Rather than calculate a value that may never be used, let's cache this value per request when we do calculate it - and reuse the cached value
Comment 1 Nick Clemens (kidclamp) 2023-06-30 19:36:44 UTC
Created attachment 152913 [details] [review]
Bug 34178: Cache ItemsAnyAvailableAndNotRestricted in memory and don't precalculate

There are several places in the code where we precalculate ItemsAnyAvailableAndNotRestricted to avoid
looping on this routine when calling IsAvailableForItemLevelRequest on a list of items form a biblio

The value of ItemsAnyAvailableAndNotRestricted is only used when there is a circulation rule for
'onshelfholds' with a value of '2' (If all unavailable)

Rather than calculate a value that may never be used, let's cache this value per request when we do
calculate it - and reuse the cached value

To test:
 1 - Apply patch
 2 - Set circulation rule 'On shelf holds allowed' as 'If all unavailable'
    make sure the rule applies to all of the items/patrons you test with
 3 - Find a record with two items that are available
 4 - Try to place a hold for a patron - not allowed
 5 - Check out one item to another patron
 6 - Attempt hold - still not allowed
 7 - Check out second item to another patron
 8 - Attempt hold - allowed!
 9 - Apply patch
10 - Cancel and replace hold - it is allowed!
11 - Check in one item, and cancel hold
12 - Place hold - not allowed!
13 - Check in second item
14 - Place hold - not allowed!
15 - prove -v t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
Comment 2 Nick Clemens (kidclamp) 2023-06-30 19:36:46 UTC
Created attachment 152914 [details] [review]
Bug 34178: Adjust tests

We need to clear the cache when changing rules/statuses

To test:
1 - prove -v t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
Comment 3 Sam Lau 2023-07-03 14:57:53 UTC
Created attachment 152981 [details] [review]
Bug 34178: Cache ItemsAnyAvailableAndNotRestricted in memory and don't precalculate

There are several places in the code where we precalculate ItemsAnyAvailableAndNotRestricted to avoid
looping on this routine when calling IsAvailableForItemLevelRequest on a list of items form a biblio

The value of ItemsAnyAvailableAndNotRestricted is only used when there is a circulation rule for
'onshelfholds' with a value of '2' (If all unavailable)

Rather than calculate a value that may never be used, let's cache this value per request when we do
calculate it - and reuse the cached value

To test:
 1 - Apply patch
 2 - Set circulation rule 'On shelf holds allowed' as 'If all unavailable'
    make sure the rule applies to all of the items/patrons you test with
 3 - Find a record with two items that are available
 4 - Try to place a hold for a patron - not allowed
 5 - Check out one item to another patron
 6 - Attempt hold - still not allowed
 7 - Check out second item to another patron
 8 - Attempt hold - allowed!
 9 - Apply patch
10 - Cancel and replace hold - it is allowed!
11 - Check in one item, and cancel hold
12 - Place hold - not allowed!
13 - Check in second item
14 - Place hold - not allowed!
15 - prove -v t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 4 Sam Lau 2023-07-03 14:57:56 UTC
Created attachment 152982 [details] [review]
Bug 34178: Adjust tests

We need to clear the cache when changing rules/statuses

To test:
1 - prove -v t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 5 Nick Clemens (kidclamp) 2023-07-12 13:54:15 UTC
Upping to major, we see this cause delays of up to 1-2 minutes when loading a patron's checkouts
Comment 6 Martin Renvoize 2023-07-19 09:28:36 UTC
Created attachment 153628 [details] [review]
Bug 34178: Cache ItemsAnyAvailableAndNotRestricted in memory and don't precalculate

There are several places in the code where we precalculate ItemsAnyAvailableAndNotRestricted to avoid
looping on this routine when calling IsAvailableForItemLevelRequest on a list of items form a biblio

The value of ItemsAnyAvailableAndNotRestricted is only used when there is a circulation rule for
'onshelfholds' with a value of '2' (If all unavailable)

Rather than calculate a value that may never be used, let's cache this value per request when we do
calculate it - and reuse the cached value

To test:
 1 - Apply patch
 2 - Set circulation rule 'On shelf holds allowed' as 'If all unavailable'
    make sure the rule applies to all of the items/patrons you test with
 3 - Find a record with two items that are available
 4 - Try to place a hold for a patron - not allowed
 5 - Check out one item to another patron
 6 - Attempt hold - still not allowed
 7 - Check out second item to another patron
 8 - Attempt hold - allowed!
 9 - Apply patch
10 - Cancel and replace hold - it is allowed!
11 - Check in one item, and cancel hold
12 - Place hold - not allowed!
13 - Check in second item
14 - Place hold - not allowed!
15 - prove -v t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t

Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2023-07-19 09:28:39 UTC
Created attachment 153629 [details] [review]
Bug 34178: Adjust tests

We need to clear the cache when changing rules/statuses

To test:
1 - prove -v t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t

Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2023-07-19 09:28:42 UTC
Created attachment 153630 [details] [review]
Bug 34178: (QA follow-up) Tidy

Tidy the relevant lines to pass the new QA rules

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2023-07-19 09:30:20 UTC
All working for me, no regressions found. QA scripts happy, Passing QA
Comment 10 Tomás Cohen Arazi 2023-07-19 16:00:13 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 11 Fridolin Somers 2023-07-25 20:18:20 UTC
Pushed to 23.05.x for 23.05.02
Comment 12 Pedro Amorim 2023-08-17 15:35:03 UTC
Does not apply for 22.11.x because bug 30860 and bug 31735 have not been pushed, maybe others. Not pushing.
Comment 13 Johanna Räisä 2023-12-07 11:25:04 UTC
*** Bug 35225 has been marked as a duplicate of this bug. ***