[Bug 43269] New: Add cached description_for() class methods to Koha::Libraries, Koha::ItemTypes, Koha::Patron::Categories
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 Bug ID: 43269 Summary: Add cached description_for() class methods to Koha::Libraries, Koha::ItemTypes, Koha::Patron::Categories Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org Target Milestone: --- Several places in Koha need to resolve codes to human-readable descriptions (branchcode to library name, categorycode to category description, itype to item type description). Currently this is done either per-row via Koha::Libraries->find($code)->branchname (uncached, N+1 queries) or via C4::Biblio::GetAuthorisedValueDesc (cached but MARC-centric API requiring tag/subfield/framework). We should add cached class-level lookup methods on the domain objects: Koha::Libraries->name_for($branchcode); Koha::Patron::Categories->description_for($categorycode); Koha::ItemTypes->description_for($itype); These would: - Use Koha::Caches to pre-fetch all values for the table on first access - Return the description string (or the raw code as fallback) - Be usable from any context (CSV exports, templates, scripts, REST API) This decouples the caching from the MARC layer and makes it available to the Koha::CSV subclass pattern (bug 41621) and anywhere else that needs bulk code-to-description resolution without N+1 queries. -- 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=43269 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | CC| |martin.renvoize@openfifth.c | |o.uk, tomascohen@gmail.com Status|NEW |ASSIGNED -- 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=43269 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #1 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203459 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203459&action=edit Bug 43269: Add Koha::Libraries->name_for my $name = Koha::Libraries->name_for({ branchcode => $code }); Adds a cached class method that resolves a branchcode to its library name. Pre-fetches all library names on first access using Koha::Caches. Cache key: libraries:name (compatible with C4::Biblio::GetAuthorisedValueDesc) Returns the branchcode as fallback if not found. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Libraries.t => SUCCESS: Tests pass! 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #2 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203460 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203460&action=edit Bug 43269: Add Koha::ItemTypes->description_for my $desc = Koha::ItemTypes->description_for({ itemtype => $code }); Adds a cached class method that resolves an item type code to its translated description. Pre-fetches all item types on first access using Koha::Cache::Memory::Lite (per-request cache, since translated_description depends on language context). Cache key: ItemTypes:description_for (Memory::Lite) Returns the item type code as fallback if not found. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ItemTypes.t => SUCCESS: Tests pass! 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #3 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203461 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203461&action=edit Bug 43269: Add Koha::ClassSources->description_for my $desc = Koha::ClassSources->description_for({ cn_source => $code }); Adds a cached class method that resolves a classification source code to its description. Pre-fetches all classification sources on first access using Koha::Caches. Cache key: cn_sources:description (compatible with C4::Biblio::GetAuthorisedValueDesc) Returns the code as fallback if not found. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ClassSources.t => SUCCESS: Tests pass! 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #4 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203462 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203462&action=edit Bug 43269: Add Koha::AuthorisedValues->description_for my $desc = Koha::AuthorisedValues->description_for({ category => $category, value => $value, opac => $opac, }); Adds a cached class method that resolves an authorised value to its description. Pre-fetches all values for the category on first access using Koha::Caches. When opac is true and lib_opac is available, returns lib_opac. Otherwise returns lib. Cache key: AV_descriptions:$category (compatible with C4::Biblio::GetAuthorisedValueDesc) Returns empty string if value is falsy or not found. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/AuthorisedValues.t => SUCCESS: Tests pass! 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #5 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203463 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203463&action=edit Bug 43269: Rewrite GetAuthorisedValueDesc to delegate to domain objects This patch rewrites C4::Biblio::GetAuthorisedValueDesc to delegate all cached lookups to the new domain class methods: - branches -> Koha::Libraries->name_for - itemtypes -> Koha::ItemTypes->description_for - cn_source -> Koha::ClassSources->description_for - other AVs -> Koha::AuthorisedValues->description_for The function retains its role as a MARC-aware dispatcher (resolving tag/subfield to category via tagslib) but no longer contains any caching logic itself. No functional changes - behavior preserved. Test plan: 1. Apply patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Biblio.t => SUCCESS: Tests pass! 3. Verify MARC editor displays authorised value descriptions 4. Verify XSLT displays resolve values correctly 5. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #6 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203464 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203464&action=edit Bug 43269: Define cache keys as constants in singular classes Moves cache key definitions from string literals scattered across read/write codepaths into constants on the singular class: - Koha::Library::CACHE_KEY_NAMES (libraries:name) - Koha::ClassSource::CACHE_KEY_DESCRIPTIONS (cn_sources:description) - Koha::AuthorisedValue::CACHE_KEY_PREFIX (AV_descriptions:) Both the invalidation code (store/delete in singular class) and the lookup code (name_for/description_for in plural class) now reference the same constant. This prevents cache key drift and makes the relationship between reader and writer explicit and grep-friendly. Test plan: 1. Apply patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Libraries.t t/db_dependent/Koha/ClassSources.t t/db_dependent/Koha/AuthorisedValues.t t/db_dependent/Biblio.t => SUCCESS: Tests pass! 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #7 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203465 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203465&action=edit Bug 43269: Use cache key constants in tests Updates test files to reference cache key constants from the singular classes instead of hardcoded strings: - Koha::Library::CACHE_KEY_NAMES - Koha::ClassSource::CACHE_KEY_DESCRIPTIONS - Koha::AuthorisedValue::CACHE_KEY_PREFIX Test plan: 1. Apply patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Item.t t/db_dependent/Koha/Filter/ExpandCodedFields.t => SUCCESS: Tests pass! 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |41620 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41620 [Bug 41620] Make itemsearch.pl use Koha::CSV for CSV generation -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 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=43269 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203459|0 |1 is obsolete| | Attachment #203460|0 |1 is obsolete| | Attachment #203461|0 |1 is obsolete| | Attachment #203462|0 |1 is obsolete| | Attachment #203463|0 |1 is obsolete| | Attachment #203464|0 |1 is obsolete| | Attachment #203465|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #8 from David Nind <david@davidnind.com> --- Created attachment 203495 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203495&action=edit Bug 43269: Add Koha::Libraries->name_for my $name = Koha::Libraries->name_for({ branchcode => $code }); Adds a cached class method that resolves a branchcode to its library name. Pre-fetches all library names on first access using Koha::Caches. Cache key: libraries:name (compatible with C4::Biblio::GetAuthorisedValueDesc) Returns the branchcode as fallback if not found. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Libraries.t => SUCCESS: Tests pass! 3. Sign off :-D 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=43269 --- Comment #9 from David Nind <david@davidnind.com> --- Created attachment 203496 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203496&action=edit Bug 43269: Add Koha::ItemTypes->description_for my $desc = Koha::ItemTypes->description_for({ itemtype => $code }); Adds a cached class method that resolves an item type code to its translated description. Pre-fetches all item types on first access using Koha::Cache::Memory::Lite (per-request cache, since translated_description depends on language context). Cache key: ItemTypes:description_for (Memory::Lite) Returns the item type code as fallback if not found. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ItemTypes.t => SUCCESS: Tests pass! 3. Sign off :-D 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=43269 --- Comment #10 from David Nind <david@davidnind.com> --- Created attachment 203497 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203497&action=edit Bug 43269: Add Koha::ClassSources->description_for my $desc = Koha::ClassSources->description_for({ cn_source => $code }); Adds a cached class method that resolves a classification source code to its description. Pre-fetches all classification sources on first access using Koha::Caches. Cache key: cn_sources:description (compatible with C4::Biblio::GetAuthorisedValueDesc) Returns the code as fallback if not found. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ClassSources.t => SUCCESS: Tests pass! 3. Sign off :-D 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=43269 --- Comment #11 from David Nind <david@davidnind.com> --- Created attachment 203498 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203498&action=edit Bug 43269: Add Koha::AuthorisedValues->description_for my $desc = Koha::AuthorisedValues->description_for({ category => $category, value => $value, opac => $opac, }); Adds a cached class method that resolves an authorised value to its description. Pre-fetches all values for the category on first access using Koha::Caches. When opac is true and lib_opac is available, returns lib_opac. Otherwise returns lib. Cache key: AV_descriptions:$category (compatible with C4::Biblio::GetAuthorisedValueDesc) Returns empty string if value is falsy or not found. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/AuthorisedValues.t => SUCCESS: Tests pass! 3. Sign off :-D 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=43269 --- Comment #12 from David Nind <david@davidnind.com> --- Created attachment 203499 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203499&action=edit Bug 43269: Rewrite GetAuthorisedValueDesc to delegate to domain objects This patch rewrites C4::Biblio::GetAuthorisedValueDesc to delegate all cached lookups to the new domain class methods: - branches -> Koha::Libraries->name_for - itemtypes -> Koha::ItemTypes->description_for - cn_source -> Koha::ClassSources->description_for - other AVs -> Koha::AuthorisedValues->description_for The function retains its role as a MARC-aware dispatcher (resolving tag/subfield to category via tagslib) but no longer contains any caching logic itself. No functional changes - behavior preserved. Test plan: 1. Apply patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Biblio.t => SUCCESS: Tests pass! 3. Verify MARC editor displays authorised value descriptions 4. Verify XSLT displays resolve values correctly 5. Sign off :-D 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=43269 --- Comment #13 from David Nind <david@davidnind.com> --- Created attachment 203500 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203500&action=edit Bug 43269: Define cache keys as constants in singular classes Moves cache key definitions from string literals scattered across read/write codepaths into constants on the singular class: - Koha::Library::CACHE_KEY_NAMES (libraries:name) - Koha::ClassSource::CACHE_KEY_DESCRIPTIONS (cn_sources:description) - Koha::AuthorisedValue::CACHE_KEY_PREFIX (AV_descriptions:) Both the invalidation code (store/delete in singular class) and the lookup code (name_for/description_for in plural class) now reference the same constant. This prevents cache key drift and makes the relationship between reader and writer explicit and grep-friendly. Test plan: 1. Apply patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Libraries.t t/db_dependent/Koha/ClassSources.t t/db_dependent/Koha/AuthorisedValues.t t/db_dependent/Biblio.t => SUCCESS: Tests pass! 3. Sign off :-D 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=43269 --- Comment #14 from David Nind <david@davidnind.com> --- Created attachment 203501 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203501&action=edit Bug 43269: Use cache key constants in tests Updates test files to reference cache key constants from the singular classes instead of hardcoded strings: - Koha::Library::CACHE_KEY_NAMES - Koha::ClassSource::CACHE_KEY_DESCRIPTIONS - Koha::AuthorisedValue::CACHE_KEY_PREFIX Test plan: 1. Apply patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Item.t t/db_dependent/Koha/Filter/ExpandCodedFields.t => SUCCESS: Tests pass! 3. Sign off :-D 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=43269 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #15 from David Nind <david@davidnind.com> --- Note: I ran the tests before checking the MARC editor. Some randomly generated values (I presume from the tests) are left in place for libraries, item types, and patron categories. Testing notes (using KTD): 1. All the tests pass: prove t/db_dependent/Koha/Libraries.t prove t/db_dependent/Koha/ItemTypes.t prove t/db_dependent/Koha/ClassSources.t prove t/db_dependent/Koha/AuthorisedValues.t prove t/db_dependent/Biblio.t prove t/db_dependent/Koha/Libraries.t t/db_dependent/Koha/ClassSources.t t/db_dependent/Koha/AuthorisedValues.t t/db_dependent/Biblio.t prove t/db_dependent/Koha/Item.t t/db_dependent/Koha/Filter/ or prove t/db_dependent/Koha/Libraries.t t/db_dependent/Koha/ItemTypes.t t/db_dependent/Koha/ClassSources.t t/db_dependent/Koha/AuthorisedValues.t t/db_dependent/Biblio.t t/db_dependent/Koha/Item.t t/db_dependent/Koha/Filter/ 2. MARC editor displays authorized value descriptions: - Opened the standard editor for a record: Programming Perl (262) - Check 942$2, 942$c, and 942$n - Add or edit an item and make sure the dropdown lists look okay, for example: home and current library, shelving location 3. XSLT displays resolve values correctly - Staff interface and OPAC search results: information in the location column looks correct - Details page for staff interface and OPAC: all look correct (not sure what values come from authorized value lists) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #16 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to David Nind from comment #15)
Note: I ran the tests before checking the MARC editor. Some randomly generated values
The area relies heavily on caching of descriptions. It is likely that flushing memcached would solve it. I followed prior art in tests, but can take a look and look for solutions. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203495|0 |1 is obsolete| | Attachment #203496|0 |1 is obsolete| | Attachment #203497|0 |1 is obsolete| | Attachment #203498|0 |1 is obsolete| | Attachment #203499|0 |1 is obsolete| | Attachment #203500|0 |1 is obsolete| | Attachment #203501|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #17 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203580 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203580&action=edit Bug 43269: Add Koha::Libraries->name_for my $name = Koha::Libraries->name_for({ branchcode => $code }); Adds a cached class method that resolves a branchcode to its library name. Pre-fetches all library names on first access using Koha::Caches. Cache key: libraries:name (compatible with C4::Biblio::GetAuthorisedValueDesc) Returns the branchcode as fallback if not found. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Libraries.t => SUCCESS: Tests pass! 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #18 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203581 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203581&action=edit Bug 43269: Add Koha::ItemTypes->description_for my $desc = Koha::ItemTypes->description_for({ itemtype => $code }); Adds a cached class method that resolves an item type code to its translated description. Pre-fetches all item types on first access using Koha::Cache::Memory::Lite (per-request cache, since translated_description depends on language context). Cache key: ItemTypes:description_for (Memory::Lite) Returns the item type code as fallback if not found. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ItemTypes.t => SUCCESS: Tests pass! 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #19 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203582 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203582&action=edit Bug 43269: Add Koha::ClassSources->description_for my $desc = Koha::ClassSources->description_for({ cn_source => $code }); Adds a cached class method that resolves a classification source code to its description. Pre-fetches all classification sources on first access using Koha::Caches. Cache key: cn_sources:description (compatible with C4::Biblio::GetAuthorisedValueDesc) Returns the code as fallback if not found. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ClassSources.t => SUCCESS: Tests pass! 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #20 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203583 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203583&action=edit Bug 43269: Add Koha::AuthorisedValues->description_for my $desc = Koha::AuthorisedValues->description_for({ category => $category, value => $value, opac => $opac, }); Adds a cached class method that resolves an authorised value to its description. Pre-fetches all values for the category on first access using Koha::Caches. When opac is true and lib_opac is available, returns lib_opac. Otherwise returns lib. Cache key: AV_descriptions:$category (compatible with C4::Biblio::GetAuthorisedValueDesc) Returns empty string if value is falsy or not found. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/AuthorisedValues.t => SUCCESS: Tests pass! 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #21 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203584 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203584&action=edit Bug 43269: Rewrite GetAuthorisedValueDesc to delegate to domain objects This patch rewrites C4::Biblio::GetAuthorisedValueDesc to delegate all cached lookups to the new domain class methods: - branches -> Koha::Libraries->name_for - itemtypes -> Koha::ItemTypes->description_for - cn_source -> Koha::ClassSources->description_for - other AVs -> Koha::AuthorisedValues->description_for The function retains its role as a MARC-aware dispatcher (resolving tag/subfield to category via tagslib) but no longer contains any caching logic itself. No functional changes - behavior preserved. Test plan: 1. Apply patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Biblio.t => SUCCESS: Tests pass! 3. Verify MARC editor displays authorised value descriptions 4. Verify XSLT displays resolve values correctly 5. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #22 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203585 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203585&action=edit Bug 43269: Define cache keys as constants in singular classes Moves cache key definitions from string literals scattered across read/write codepaths into constants on the singular class: - Koha::Library::CACHE_KEY_NAMES (libraries:name) - Koha::ClassSource::CACHE_KEY_DESCRIPTIONS (cn_sources:description) - Koha::AuthorisedValue::CACHE_KEY_PREFIX (AV_descriptions:) Both the invalidation code (store/delete in singular class) and the lookup code (name_for/description_for in plural class) now reference the same constant. This prevents cache key drift and makes the relationship between reader and writer explicit and grep-friendly. Test plan: 1. Apply patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Libraries.t t/db_dependent/Koha/ClassSources.t t/db_dependent/Koha/AuthorisedValues.t t/db_dependent/Biblio.t => SUCCESS: Tests pass! 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #23 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 203586 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203586&action=edit Bug 43269: Use cache key constants in tests Updates test files to reference cache key constants from the singular classes instead of hardcoded strings: - Koha::Library::CACHE_KEY_NAMES - Koha::ClassSource::CACHE_KEY_DESCRIPTIONS - Koha::AuthorisedValue::CACHE_KEY_PREFIX Test plan: 1. Apply patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Item.t t/db_dependent/Koha/Filter/ExpandCodedFields.t => SUCCESS: Tests pass! 3. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #24 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to David Nind from comment #15)
Note: I ran the tests before checking the MARC editor. Some randomly generated values (I presume from the tests) are left in place for libraries, item types, and patron categories.
I just adjusted the tests so they clear their used cache after they have run. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=36350 --- Comment #25 from Jonathan Druart <jonathan.druart@gmail.com> --- Did you see bug 36350? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #26 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #25)
Did you see bug 36350?
I hadn't. This is much more specific. It just felt too weird adding `use C4::Biblio` in the classes I was introducing. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |In Discussion --- Comment #27 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #26)
(In reply to Jonathan Druart from comment #25)
Did you see bug 36350?
I hadn't. This is much more specific.
There is also bug 32476.
It just felt too weird adding `use C4::Biblio` in the classes I was introducing.
I don't understand what you mean here. My point is that there are already 2 attempts at caching in L1 the result of extensive or widely used/called methods. The description_for solution you are suggesting here is indeed very (too) specific. We should aim for a more global and reusable solution. What the 2 other bugs are suggesting. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff --- Comment #28 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #27)
(In reply to Tomás Cohen Arazi (tcohen) from comment #26)
(In reply to Jonathan Druart from comment #25)
Did you see bug 36350?
I hadn't. This is much more specific.
There is also bug 32476.
It just felt too weird adding `use C4::Biblio` in the classes I was introducing.
I don't understand what you mean here.
`C4::Biblio::GetAuthorisedValueDesc` takes a framework and tag/subtags as parameters. Refactoring it the way I did is a great improvement on its own.
My point is that there are already 2 attempts at caching in L1 the result of extensive or widely used/called methods. The description_for solution you are suggesting here is indeed very (too) specific. We should aim for a more global and reusable solution. What the 2 other bugs are suggesting.
Caching full resultset objects will be complex, full of side effects and edge cases and might probably never get completed. It would be fair to let me move the existing code to a sane place like this bug does, and eventually rewrite it in terms of the new caching solution when that moves. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.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=43269 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |17921 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17921 [Bug 17921] Shelf Browser does not take OpacSuppression or OpacHiddenItems into consideration -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43269 --- Comment #29 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #27)
(In reply to Tomás Cohen Arazi (tcohen) from comment #26)
(In reply to Jonathan Druart from comment #25)
Did you see bug 36350?
I hadn't. This is much more specific.
There is also bug 32476.
It just felt too weird adding `use C4::Biblio` in the classes I was introducing.
I don't understand what you mean here.
My point is that there are already 2 attempts at caching in L1 the result of extensive or widely used/called methods. The description_for solution you are suggesting here is indeed very (too) specific. We should aim for a more global and reusable solution. What the 2 other bugs are suggesting.
I've read both bugs and their code. Besides my opinion on the approaches, I believe my implementation is a sound and useful refactoring of an already widely used method `C4::Biblio::GetAuthorisedValueDesc`, and makes it more useful! I just found the need for this on bug 42503, which will loop through holds queue items and query for the item type descriptions and -in the worst case, all items in a resultset different item type- it would mean N DB hits for N results. And `GetAuthorisedValueDesc` is much more efficient, and so this refactoring! I'm all for the other devs where it makes sense, but that's too much scope for this tiny refactoring which will still be needed IMHO. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org