Bug 34051 - Koha::AuthorisedValues->get_description_by_koha_field not caching results for non-existent values
Summary: Koha::AuthorisedValues->get_description_by_koha_field not caching results for...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-06-19 00:58 UTC by David Cook
Modified: 2023-12-17 23:28 UTC (History)
2 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,22.11.08


Attachments
Bug 34051: Cache empty hashref for non-existent authorised values (1.40 KB, patch)
2023-06-19 01:28 UTC, David Cook
Details | Diff | Splinter Review
Bug 34051: Cache empty hashref for non-existent authorised values (1.44 KB, patch)
2023-06-19 07:16 UTC, David Nind
Details | Diff | Splinter Review
Bug 34051: Cache empty hashref for non-existent authorised values (1.51 KB, patch)
2023-06-19 14:19 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 David Cook 2023-06-19 00:58:51 UTC
As I note at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31216#c5 Koha::AuthorisedValues->get_description_by_koha_field isn't caching database results for non-existent values.

Many authorised values lists don't have a value for 0, but many records contain values of 0. 

Koha::AuthorisedValues->get_description_by_koha_field() will do a database call every time it encounters a value that doesn't exist in the AV table, because it isn't caching the empty/null that is calculated based on the incoming value not existing in the AV table.

In terms of code, it is a simple change, but Koha::AuthorisedValues->get_description_by_koha_field is used in a lot of places, so obviously we'll want to be careful making any changes.
Comment 1 David Cook 2023-06-19 01:28:58 UTC
Created attachment 152457 [details] [review]
Bug 34051: Cache empty hashref for non-existent authorised values

When fetching authorised value descriptions, we need to handle
scenarios where a value doesn't exist in the cache and doesn't
exist in the database. In this situation, we return an empty
hashref, and this patch makes us cache this empty hashref.

This is important because otherwise the function
Koha::AuthorisedValues->get_description_by_koha_field will
do a database call every time it encounters the same value that
doesn't exist in the authorised values table.
Comment 2 David Cook 2023-06-19 01:31:06 UTC
I'm not sure the best way to test this one...

"prove t/db_dependent/AuthorisedValues.t" succeeds.

Koha::AuthorisedValues->get_description_by_koha_field is used across Koha, but the scenario of having non-existent AV values doesn't seem to pop up a lot. I first noticed it in inventory.pl in bug 31216.
Comment 3 David Cook 2023-06-19 01:43:05 UTC
Test plan:

0. Apply patch and koha-plack --restart kohadev
1. Go to create a SQL report from SQL
/cgi-bin/koha/reports/guided_reports.pl?phase=Create%20report%20from%20SQL
2. Save a report with the following SQL:
SELECT barcode FROM items where barcode <> '';
3. Run the report
4. Download as CSV
5. Edit the CSV and remove the "barcode" heading

6. Go to edit item for barcode 3999900000001
/cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=1&itemnumber=1
7. Add "Damaged status" of "Damaged"
8. Click "Save changes"

9. koha-mysql kohadev
10. update items set notforloan = 9 where barcode = 3999900000001;

11. Go to inventory tool
http://localhost:8081/cgi-bin/koha/tools/inventory.pl
12. Upload barcode file via "Choose file"
13. Uncheck "Compare barcodes list to results"
14. Open F12 tools
15. Click "Submit"
16. Click "OK" on confirm box

17. Note that the inventory job takes around 30 seconds to run
instead of 50 seconds
17b. The exact figures may vary. If you compare with before patch
inventory runs, you'll be saving roughly 20 seconds.
18. Note that "Damaged" appears in the "Damaged" column, which
demonstrates that the authorized value lookup was completed for
the item in the result list
19. Note that "9" appears in the "Not for loan" column, and
"0" appears in the "Lost" and "Withdrawn" columns, which demonstrates
that the authorized value lookup wasn't able to find descriptions for
these codes.
Comment 4 David Cook 2023-06-19 01:43:47 UTC
The test plan shows that the AV lookups work as expected and the inventory completes faster than before.
Comment 5 David Nind 2023-06-19 07:16:38 UTC
Created attachment 152464 [details] [review]
Bug 34051: Cache empty hashref for non-existent authorised values

When fetching authorised value descriptions, we need to handle
scenarios where a value doesn't exist in the cache and doesn't
exist in the database. In this situation, we return an empty
hashref, and this patch makes us cache this empty hashref.

This is important because otherwise the function
Koha::AuthorisedValues->get_description_by_koha_field will
do a database call every time it encounters the same value that
doesn't exist in the authorised values table.

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2023-06-19 07:19:44 UTC
Testing notes (using KTD):

1. Running through the test plan (before and after patch applied) - from Network tab for web developer tools: from about 20s (before) to about 10s.
Comment 7 Martin Renvoize 2023-06-19 14:19:05 UTC
Created attachment 152474 [details] [review]
Bug 34051: Cache empty hashref for non-existent authorised values

When fetching authorised value descriptions, we need to handle
scenarios where a value doesn't exist in the cache and doesn't
exist in the database. In this situation, we return an empty
hashref, and this patch makes us cache this empty hashref.

This is important because otherwise the function
Koha::AuthorisedValues->get_description_by_koha_field will
do a database call every time it encounters the same value that
doesn't exist in the authorised values table.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2023-06-19 14:19:50 UTC
Nice find.. Passing QA
Comment 9 Tomás Cohen Arazi 2023-06-21 16:18:55 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 10 Martin Renvoize 2023-07-17 12:22:10 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 11 Pedro Amorim 2023-07-18 10:50:42 UTC
Nice work everyone!

Pushed to 22.11.x for next release