Summary: | marcrecord2csv searches authorised values inefficiently | ||
---|---|---|---|
Product: | Koha | Reporter: | David Cook <dcook> |
Component: | Tools | Assignee: | David Cook <dcook> |
Status: | CLOSED FIXED | QA Contact: | Julian Maurice <julian.maurice> |
Severity: | normal | ||
Priority: | P5 - low | CC: | david, fridolin.somers, julian.maurice, lucas |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34051 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35588 |
||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
This significantly improves the speed of downloading large lists in CSV format. (It adds a get_descriptions_by_marc_field" method which caches AuthorisedValue descriptions when searched by MARC field, which is used when exporting MARC to CSV.)
|
Version(s) released in: |
24.05.00,23.11.02,23.05.08
|
Circulation function: | |||
Bug Depends on: | |||
Bug Blocks: | 35588 | ||
Attachments: |
Bug 35579: Cache authorised value lookup by MARC field
Bug 35579: Add unit test Bug 35579: Cache authorised value lookup by MARC field Bug 35579: Add unit test Bug 35579: Cache authorised value lookup by MARC field Bug 35579: Add unit test |
Description
David Cook
2023-12-15 05:12:06 UTC
In koha-testing-docker, after making a list, you can use SQL like the following to populate the list with all the bib records in Koha: insert into virtualshelfcontents (shelfnumber,biblionumber,borrowernumber) select 1,biblionumber,51 from biblio; And when making a CSV profile, you'll want "Profile MARC fields" like so: 000|001|003|005|006|007|008|010|015|016|020|022|040|050|082|100|110|111|130|240|243|245|246|247|250|260|300|336|337|338|440|490|500|501|505|520|530|600|610|611|630|648|650|651|690|700|710|711|856|887|942|995 Created attachment 159858 [details] [review] Bug 35579: Cache authorised value lookup by MARC field This patch adds a "get_descriptions_by_marc_field" method which caches AuthorisedValue descriptions when searched by MARC field, which is used when exporting MARC to CSV. I've run out of time for tests, but I figured I'd post the patch anyway. I can come back for tests next week... On koha-testing-docker, this changes a MARC to CSV output from 1.3 minutes to about 7 seconds when exporting the full MARC database. So that's nice :) Created attachment 159936 [details] [review] Bug 35579: Add unit test Test plan: 0. Do not apply the patch yet! 1. Go to http://localhost:8081/cgi-bin/koha/tools/csv-profiles.pl 2. Add a CSV profile with the default values and using the following for the "Profile MARC fields": 000|001|003|005|006|007|008|010|015|016|020|022|040|050|082|100|110|111|130|240|243|245|246|247|250|260|300|336|337|338|440|490|500|501|505|520|530|600|610|611|630|648|650|651|690|700|710|711|856|887|942|995 3. Create a new List 4. Add all the database's bibs to that list using SQL like the following (where the shelfnumber equals the number for your list): insert into virtualshelfcontents (shelfnumber,biblionumber,borrowernumber) select 1,biblionumber,51 from biblio; 5. Go to that list in the staff interface 6. Download that list using your CSV profile 7. Apply the patch 8. koha-plack --reload kohadev 9. Download that list using your CSV profile 10. Note that the output is the same, but that the output completes much more quickly after applying the patch Created attachment 159975 [details] [review] Bug 35579: Cache authorised value lookup by MARC field This patch adds a "get_descriptions_by_marc_field" method which caches AuthorisedValue descriptions when searched by MARC field, which is used when exporting MARC to CSV. Signed-off-by: David Nind <david@davidnind.com> Created attachment 159976 [details] [review] Bug 35579: Add unit test Signed-off-by: David Nind <david@davidnind.com> Testing notes (using KTD): 1. Download speeds (using Web Developer Tools > Network feature in Firefox): . Before: 1 request 270.81 kB / 336 B transferred Finish: 46.66 s . Refreshed browser, flush_memcached, restart_all (so everything fresh) . After : 1 request 270.81 kB / 336 B transferred Finish: 4.03 s 2. Compared downloaded files using a diff tool - no differences identified. (In reply to David Nind from comment #10) > Testing notes (using KTD): > > 1. Download speeds (using Web Developer Tools > Network feature in Firefox): > . Before: 1 request 270.81 kB / 336 B transferred Finish: 46.66 s > . Refreshed browser, flush_memcached, restart_all (so everything fresh) > . After : 1 request 270.81 kB / 336 B transferred Finish: 4.03 s > > 2. Compared downloaded files using a diff tool - no differences identified. You're a champion, David! Created attachment 160234 [details] [review] Bug 35579: Cache authorised value lookup by MARC field This patch adds a "get_descriptions_by_marc_field" method which caches AuthorisedValue descriptions when searched by MARC field, which is used when exporting MARC to CSV. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Created attachment 160235 [details] [review] Bug 35579: Add unit test Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Works as expected, no complaints from QA script and tests pass. Passed QA It looks like tagsubfield is optional in get_descriptions_by_marc_field, but there is no unit test for this case? The example given in the test plan doesn't seem to include any fields that are linked to an AV category. 000|001|003|005|006|007|008|010|015|016|020|022|040|050|082|100|110|111|130|240|243|245|246|247|250|260|300|336|337|338|440|490|500|501|505|520|530|600|610|611|630|648|650|651|690|700|710|711|856|887|942|995 Or maybe only UNIMARC (995). I amended the test to also include 952 and tried single subfields like 952$c, 952$8 as well. ... and please also include test plans in commit messages :) Pushed for 24.05! Well done everyone, thank you! Pushed to 23.11.x for 23.11.02 Backported to 23.05.x for upcoming 23.05.08 |