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.
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.
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.
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.
The test plan shows that the AV lookups work as expected and the inventory completes faster than before.
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>
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.
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>
Nice find.. Passing QA
Pushed to master for 23.11. Nice work everyone, thanks!
Thanks for all the hard work! Pushed to 23.05.x for the next release
Nice work everyone! Pushed to 22.11.x for next release