[Bug 34051] New: Koha::AuthorisedValues->get_description_by_koha_field not caching results for non-existent values
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 Bug ID: 34051 Summary: Koha::AuthorisedValues->get_description_by_koha_field not caching results for non-existent values Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: dcook@prosentient.com.au QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=31216 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 --- Comment #1 from David Cook <dcook@prosentient.com.au> --- Created attachment 152457 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152457&action=edit 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal Assignee|koha-bugs@lists.koha-commun |dcook@prosentient.com.au |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 --- Comment #2 from David Cook <dcook@prosentient.com.au> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=33989 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 --- Comment #3 from David Cook <dcook@prosentient.com.au> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 --- Comment #4 from David Cook <dcook@prosentient.com.au> --- The test plan shows that the AV lookups work as expected and the inventory completes faster than before. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152457|0 |1 is obsolete| | --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 152464 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152464&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #6 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152464|0 |1 is obsolete| | --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 152474 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152474&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA CC| |martin.renvoize@ptfs-europe | |.com --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice find.. Passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |23.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 --- Comment #9 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|23.11.00 |23.11.00,23.05.02 released in| | Status|Pushed to master |Pushed to stable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Thanks for all the hard work! Pushed to 23.05.x for the next release -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|23.11.00,23.05.02 |23.11.00,23.05.02,22.11.08 released in| | Status|Pushed to stable |Pushed to oldstable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 --- Comment #11 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Nice work everyone! Pushed to 22.11.x for next release -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=35579 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|martin.renvoize@ptfs-europe | |.com | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to oldstable |RESOLVED CC| |wainuiwitikapark@catalyst.n | |et.nz -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=40554 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org