Bug 34178 - Add caching of ItemsAnyAvailableAndNotRestricted to IsAvailableForItemLevelRequest
Summary: Add caching of ItemsAnyAvailableAndNotRestricted to IsAvailableForItemLevelRe...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Testopia
URL:
Keywords:
: 35225 (view as bug list)
Depends on:
Blocks: 33746
  Show dependency treegraph
 
Reported: 2023-06-30 19:25 UTC by Nick Clemens (kidclamp)
Modified: 2023-12-07 11:25 UTC (History)
9 users (show)

See Also:
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


Attachments
Bug 34178: Cache ItemsAnyAvailableAndNotRestricted in memory and don't precalculate (9.89 KB, patch)
2023-06-30 19:36 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 34178: Adjust tests (5.40 KB, patch)
2023-06-30 19:36 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 34178: Cache ItemsAnyAvailableAndNotRestricted in memory and don't precalculate (9.93 KB, patch)
2023-07-03 14:57 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 34178: Adjust tests (5.45 KB, patch)
2023-07-03 14:57 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 34178: Cache ItemsAnyAvailableAndNotRestricted in memory and don't precalculate (10.00 KB, patch)
2023-07-19 09:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34178: Adjust tests (5.51 KB, patch)
2023-07-19 09:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34178: (QA follow-up) Tidy (28.41 KB, patch)
2023-07-19 09:28 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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. ***