[Bug 35052] New: OpacHiddenItemsHidesRecord system preference should be considered on index time instead
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 Bug ID: 35052 Summary: OpacHiddenItemsHidesRecord system preference should be considered on index time instead Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Searching Assignee: koha-bugs@lists.koha-community.org Reporter: pedro.amorim@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org CC: alexander.blanchard@ptfs-europe.com, dcook@prosentient.com.au, jacob.omara@ptfs-europe.com, jonathan.field@ptfs-europe.com, martin.renvoize@ptfs-europe.com, matt.blenkinsop@ptfs-europe.com Currently, results are being skipped after the search has been processed in C4/Search.pm. This may cause several unintended scenarios: 1) The first page has all the results skipped, even thought there are more results in 2nd page and so on, this will will result in a "No search results" page 2) The total search results number still shows 80 (for example), even if it has skipped 70 and is only showing 10. To reproduce we need to use both OpacHiddenItems and OpacHiddenItemsHidesRecord Use OpacHiddenItems and put some rules that hide some items Use OpacHiddenItemsHidesRecord = 'hide' -- 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=35052 Chris Rowlands <chris.rowlands6@nhs.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chris.rowlands6@nhs.net -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #1 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I actually see two ways to achieve this.. a) we add a field to the index to denote hidden by these options and fill it during index runs. or b) we adapt the queries at runtime with each search to take into consideration the preferences -- 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=35052 --- Comment #2 from Nick Clemens <nick@bywatersolutions.com> --- for b) I think we need to begin indexing the items individually, right now we take the values from each, but the connections are broken. i.e. homebranch index gets the list of homebranches on the item holdingbranch the same We cannot search for records with items with matching homebranch and holdingbranch, only records that have some items in a homebranch and some items in a holdingbranch -- 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=35052 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=24403 --- Comment #3 from David Cook <dcook@prosentient.com.au> --- I recall Frido and I talking about this on a separate bug but now I can't remember which one it was... One problem with indexing whether or not to hide based on OpacHiddenItems and OpacHiddenItemsHidesRecord is that you'd need to do a full reindex for any change to OpacHiddenItems or OpacHiddenItemsHidesRecord. (Of course, if we note that caveat in the syspref descriptions, then it's not necessarily a huge problem.) Unless we re-use the index used by OpacSuppression, then we'll need to adapt the search queries to take into account a new OpacHiddenItems index as well. -- As Nick says, I don't think just updating the search query is enough, since I don't think there's any way to know if all the items in a record should be hidden in Zebra. In bug 19704, I'm able to generate a SQL query using OpacHiddenItems, but it's able to deal with individual item records. That said, I don't think we can index items separately in Zebra and there wouldn't be any way to join the bibs and items together anyway. So I think that really just leaves the indexing option? -- 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=35052 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |42835 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42835 [Bug 42835] ElasticSearch should have an items index -- 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=35052 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|42835 |43151 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42835 [Bug 42835] ElasticSearch should have an items index https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43151 [Bug 43151] ES should apply search filters on each item discretely -- 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=35052 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|OpacHiddenItemsHidesRecord |OpacHiddenItemsHidesRecord |system preference should be |system preference should be |considered on index time |considered in search |instead |indexes instead of post | |filtering results -- 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=35052 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43355 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43355 [Bug 43355] Add a /search API endpoint for biblios -- 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=35052 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@openfifth.c |ity.org |o.uk -- 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=35052 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Sponsorship status|--- |Unsponsored Status|NEW |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=35052 --- Comment #4 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 203925 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203925&action=edit Bug 35052: Extract shared items-index composite aggregation helper -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 203926 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203926&action=edit Bug 35052: Add _get_item_counts_by_biblio -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #6 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 203927 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203927&action=edit Bug 35052: Add _resolve_opac_hidden_items_rules -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #7 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 203928 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203928&action=edit Bug 35052: Attach resolved OpacHiddenItems rules to the built query Wire _resolve_opac_hidden_items_rules into build_query_compat by: 1. Calling the method to resolve the rules (returns undef if not applicable) 2. Attaching the result to $query->{_opac_hidden_items_rules} when non-undef This allows Task 5's _apply_opac_hidden_items_filter to read and delete these rules from the query object when filtering search results. Adds 2 new assertions testing that: - OPAC searches with is_opac=1 carry the resolved hidden-items rules - Staff searches with is_opac=0 do not carry the rules -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #8 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 203929 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203929&action=edit Bug 35052: Add _apply_opac_hidden_items_filter -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #9 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 203930 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203930&action=edit Bug 35052: Apply the opac-hidden-items filter in search_compat -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #10 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 203931 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203931&action=edit Bug 35052: Pass the searching patron into build_query_compat for OpacHiddenItemsExceptions -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #11 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 203932 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203932&action=edit Bug 35052: Add end-to-end test for the query-time OpacHiddenItems filter -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #12 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 203933 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203933&action=edit Bug 35052: Fix test warning in OpacHiddenItems end-to-end test Changed build_query_compat call to use empty arrays [] instead of [undef] for operators and indexes parameters, which prevents uninitialized value warnings. This makes the test output pristine while maintaining the same semantics (both represent 'no operators/indexes specified'). Removed the Test::Warn wrapper since the warning is no longer generated, and reverted the subtest assertion count from 4 back to 3. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #13 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 203934 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203934&action=edit Bug 35052: Add defensive null check for patron category Prevent potential null pointer dereference if $patron->category unexpectedly returns undef. While this shouldn't happen in normal Koha operation (category is a required foreign key), add explicit check to close the defensive-coding gap. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #14 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 203935 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203935&action=edit Bug 35052: (QA follow-up) Fix perlcritic and POD coverage findings - QueryBuilder.pm: replace explicit "return undef" with bare "return" in _resolve_opac_hidden_items_rules (perlcritic severity 5, PBP p.199) - Search.pm: add missing POD for _composite_item_aggregation and _get_item_counts_by_biblio; correct the POD block that had drifted onto _composite_item_aggregation when it actually documented _get_matching_biblionumbers' public contract (Task 1's refactor moved the sub the POD was written for without moving the POD itself) Test plan: 1) ktd --name "${KTD_INSTANCE:-kohadev}" --shell --run '/kohadevbox/qa-test-tools/koha-qa.pl -v 2 --more-tests' 2) Only remaining flagged item is a pre-existing critic finding on MockESSearchClient's package-vs-filename mismatch in t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t, inherited unchanged from Bug 43151 - not introduced or touched by this bug 3) ktd --name "${KTD_INSTANCE:-kohadev}" --shell --run 'prove t/Koha/SearchEngine/Elasticsearch/QueryBuilder.t t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t' 4) All tests pass -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|--- |26.11 Initiative type|--- |Feature -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203925|0 |1 is obsolete| | Attachment #203926|0 |1 is obsolete| | Attachment #203927|0 |1 is obsolete| | Attachment #203928|0 |1 is obsolete| | Attachment #203929|0 |1 is obsolete| | Attachment #203930|0 |1 is obsolete| | Attachment #203931|0 |1 is obsolete| | Attachment #203932|0 |1 is obsolete| | Attachment #203933|0 |1 is obsolete| | Attachment #203934|0 |1 is obsolete| | Attachment #203935|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=35052 --- Comment #15 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 205607 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205607&action=edit Bug 35052: Extract shared items-index composite aggregation helper - Extract _composite_item_aggregation() with parameterized cache key prefix - Refactor _get_matching_biblionumbers() as thin wrapper - Preserve exact existing contract for Task 2+ callers - Add test coverage for new helper method Test plan: 1) ktd --name "${KTD_INSTANCE:-kohadev}" --shell --run 'prove t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t' 2) All tests pass, including the new '_composite_item_aggregation() tests' subtest 3) Confirm no regression: existing subtests (_apply_item_level_filters, _get_matching_biblionumbers, _items_index_ready) still pass unchanged -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #16 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 205608 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205608&action=edit Bug 35052: Add _get_item_counts_by_biblio Sibling to _get_matching_biblionumbers(), built on the same shared _composite_item_aggregation() helper, but returning a { biblionumber => count } hashref instead of a flat biblionumber list. This is what the OpacHiddenItems filter (added in a later commit) uses to tell "some items hidden" apart from "all items hidden" for a biblio. Test plan: 1) ktd --name "${KTD_INSTANCE:-kohadev}" --shell --run 'prove t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t' 2) All tests pass, including the new '_get_item_counts_by_biblio() tests' subtest -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #17 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 205609 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205609&action=edit Bug 35052: Add _resolve_opac_hidden_items_rules New method on Koha::SearchEngine::Elasticsearch::QueryBuilder that decides whether the OpacHiddenItems rules should be enforced at query time for a given search, and returns them if so. Returns undef when: the search isn't an OPAC search, OpacHiddenItemsHidesRecord is off, no OpacHiddenItems rules are configured, or the searching patron's category is listed in OpacHiddenItemsExceptions (delegates to the existing Koha::Patron::Category->override_hidden_items rather than reimplementing that check). Reads preferences live on every call - nothing is cached or baked in, so editing the sysprefs takes effect on the very next search. Not yet wired into build_query_compat (next commit) or consumed anywhere - this commit only adds the method and its unit tests. Test plan: 1) ktd --name "${KTD_INSTANCE:-kohadev}" --shell --run 'prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t' 2) All tests pass, including the new '_resolve_opac_hidden_items_rules() tests' subtest, covering: staff (non-OPAC) search, OpacHiddenItemsHidesRecord off, no rules configured, the normal OPAC case, and a patron category listed in OpacHiddenItemsExceptions -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #18 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 205610 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205610&action=edit Bug 35052: Attach resolved OpacHiddenItems rules to the built query Wire _resolve_opac_hidden_items_rules into build_query_compat by: 1. Calling the method to resolve the rules (returns undef if not applicable) 2. Attaching the result to $query->{_opac_hidden_items_rules} when non-undef This allows Task 5's _apply_opac_hidden_items_filter to read and delete these rules from the query object when filtering search results. Adds 2 new assertions testing that: - OPAC searches with is_opac=1 carry the resolved hidden-items rules - Staff searches with is_opac=0 do not carry the rules Test plan: 1) ktd --name "${KTD_INSTANCE:-kohadev}" --shell --run 'prove t/Koha/SearchEngine/Elasticsearch/QueryBuilder.t t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t' 2) All tests pass in both files, including the new 'build_query_compat() attaches _opac_hidden_items_rules' subtest -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #19 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 205611 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205611&action=edit Bug 35052: Add _apply_opac_hidden_items_filter Core mechanism of this bug fix. Given the rules attached by the previous commit, finds every biblio where ALL items match the OpacHiddenItems rules and excludes them from the search via a must_not/ids filter clause, evaluated live at query time rather than baked into the index. Implementation is two scoped items-index queries, not a full-catalog scan: 1. Items matching the hide-rule, grouped by biblionumber with counts (scoped to the usually-small population of actually-hidden items) 2. For just those candidate biblionumbers, their total item count (scoped to that small candidate set, not the whole catalogue) A biblio is "fully hidden" iff its hidden count equals its total count - this is what distinguishes it from a biblio with only some items hidden, which must never be excluded. Falls back to no-op (existing post-processing behaviour) if the items index isn't ready, matching the pattern already established by _apply_item_level_filters (Bug 43151). Not yet wired into search_compat (next commit) - this commit only adds the method and its unit tests. Test plan: 1) ktd --name "${KTD_INSTANCE:-kohadev}" --shell --run 'prove t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t' 2) All tests pass, including the new '_apply_opac_hidden_items_filter() tests' subtest - pay particular attention to the assertion that a biblio with some (but not all) items hidden is NOT excluded, only the fully-hidden one -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #20 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 205612 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205612&action=edit Bug 35052: Apply the opac-hidden-items filter in search_compat Calls _apply_opac_hidden_items_filter alongside the existing _apply_item_level_filters, inside the same items-index-readiness-gated block. Adds the matching unconditional delete of $query->{_opac_hidden_items_rules} outside that block, mirroring the existing safety net for _item_level_constraints, so the key never leaks through to Elasticsearch as an unrecognised query field when the items index isn't ready. Also adds an integration test proving the filter is actually wired into search_compat's real code path (mocking only the ES-network boundary), closing part of the "no end-to-end test through this pipeline" gap flagged on Bug 43151 - fully closed by the next commit's test through build_query_compat too. Test plan: 1) ktd --name "${KTD_INSTANCE:-kohadev}" --shell --run 'prove t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t' 2) All tests pass, including the new 'search_compat() applies the opac-hidden-items filter' subtest -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #21 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 205613 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205613&action=edit Bug 35052: Pass the searching patron into build_query_compat for OpacHiddenItemsExceptions Adds patron => $patron to both existing build_query_compat call sites in opac/opac-search.pl, using the $patron variable already in scope (set at line 132, before either call). This is what QueryBuilder::_resolve_opac_hidden_items_rules (added earlier in this series) needs to check OpacHiddenItemsExceptions against the searching patron's category. For an anonymous OPAC session $patron is undef (Koha::Patrons->find(undef) returns undef without querying - see Koha::Objects::find), which is handled safely: _resolve_opac_hidden_items_rules only dereferences $patron inside an "if ($patron && ...)" check, so anonymous sessions correctly fall through to having the hidden-items filter applied, never exempted. No automated test - opac-search.pl is a CGI script, not a module. Full verification is the end-to-end test (next commit) plus the KTD manual test plan documented there. Test plan: 1) perl -c opac/opac-search.pl 2) Reports "opac/opac-search.pl syntax OK" -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #22 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 205614 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205614&action=edit Bug 35052: Add end-to-end test for the query-time OpacHiddenItems filter Adds a test exercising the real QueryBuilder->build_query_compat -> Search->search_compat pipeline together, mocking only the ES-network boundary (_items_index_ready, _apply_item_level_filters, _get_item_counts_by_biblio, search) - not _apply_opac_hidden_items_filter or build_query_compat themselves, both of which run for real. Closes the gap explicitly flagged on Bug 43151 (2026-08-04): "no end-to-end test through build_query_compat exercising this pipeline ... the actual join between 'what a real search sends' and 'what gets extracted' is only verified manually." Test plan: 1) ktd --name "${KTD_INSTANCE:-kohadev}" --shell --run 'prove t/Koha/SearchEngine/Elasticsearch/QueryBuilder.t t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t' 2) All tests pass, including the new 'end-to-end: build_query_compat -> search_compat honours OpacHiddenItemsHidesRecord' subtest For final human verification with a live Elasticsearch, before this bug is signed off: 3) Boot KTD with Elasticsearch: ktd --search-engine es8 up (or your usual ${KTD_INSTANCE} invocation if already running with ES) 4) Catalogue a record titled ZZZ35052TEST with two items: item 1: withdrawn = 1; item 2: withdrawn = 0 5) Set system preferences: OpacHiddenItems to withdrawn: - 1 OpacHiddenItemsHidesRecord to "Hide", OpacHiddenItemsExceptions empty 6) Rebuild the indices: misc/search_tools/rebuild_elasticsearch.pl -a -r -v 7) Search ZZZ35052TEST in the OPAC - record appears (item 2 is visible, so the record must not be hidden) 8) Edit item 2's withdrawn to 1 as well (now both items hidden). Wait ~15s for the aggregation cache to expire, or flush it directly: ktd --name "${KTD_INSTANCE}" --shell --run 'perl -e "Koha::Caches->get_instance->flush_all"' 9) Search ZZZ35052TEST again - record is now absent from results, AND the reported total-hits count does not include it either. This is the actual bug being fixed: previously the total included the fully-hidden record and a results page could render with fewer results than requested because the record was skipped only after being counted 10) Set OpacHiddenItemsExceptions to a patron category with a test patron account in it, log in to the OPAC as that patron, repeat step 9 - the record reappears for that patron only -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #23 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 205615 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205615&action=edit Bug 35052: Fix test warning in OpacHiddenItems end-to-end test Changed build_query_compat call to use empty arrays [] instead of [undef] for operators and indexes parameters, which prevents uninitialized value warnings. This makes the test output pristine while maintaining the same semantics (both represent 'no operators/indexes specified'). Removed the Test::Warn wrapper since the warning is no longer generated, and reverted the subtest assertion count from 4 back to 3. Test plan: 1) ktd --name "${KTD_INSTANCE:-kohadev}" --shell --run 'prove t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t' 2) All tests pass with pristine output - no warnings at all, not even a suppressed/expected one -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #24 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 205616 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205616&action=edit Bug 35052: Add defensive null check for patron category Prevent potential null pointer dereference if $patron->category unexpectedly returns undef. While this shouldn't happen in normal Koha operation (category is a required foreign key), add explicit check to close the defensive-coding gap. Test plan: 1) ktd --name "${KTD_INSTANCE:-kohadev}" --shell --run 'prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t' 2) All tests pass -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #25 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 205617 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205617&action=edit Bug 35052: (QA follow-up) Fix perlcritic and POD coverage findings - QueryBuilder.pm: replace explicit "return undef" with bare "return" in _resolve_opac_hidden_items_rules (perlcritic severity 5, PBP p.199) - Search.pm: add missing POD for _composite_item_aggregation and _get_item_counts_by_biblio; correct the POD block that had drifted onto _composite_item_aggregation when it actually documented _get_matching_biblionumbers' public contract (Task 1's refactor moved the sub the POD was written for without moving the POD itself) Test plan: 1) ktd --name "${KTD_INSTANCE:-kohadev}" --shell --run '/kohadevbox/qa-test-tools/koha-qa.pl -v 2 --more-tests' 2) Only remaining flagged item is a pre-existing critic finding on MockESSearchClient's package-vs-filename mismatch in t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t, inherited unchanged from Bug 43151 - not introduced or touched by this bug 3) ktd --name "${KTD_INSTANCE:-kohadev}" --shell --run 'prove t/Koha/SearchEngine/Elasticsearch/QueryBuilder.t t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t' 4) All tests pass -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #26 from David Nind <david@davidnind.com> --- I'm not sure if I'm testing this correctly, but here is what I found (testing this bug before testing bug 43355): 1. Search results in the staff interface and OPAC don't show the newly created record when searching for test (or TEST): - This is with items not hidden or with one item withdrawn - Rebuilt the index using: koha-elasticsearch --rebuild -d -b -a kohadev - Not sure where the rebuild in the test plan is different - Sufficient time allowed for index to update (but rebuilt as well) - Also used as per the test plan: misc/search_tools/rebuild_elasticsearch.pl -a -r -v 2. I get a page not found when searching or directly accessing created record: Sorry, the requested page is not available Error 404 - If the record doesn't exist (or is hidden) shouldn't it be no results found, instead of a 404? For example, search for DITA: No results found! No results found for that in catalog. - If you access the record directly with all items hidden, then I would expect a page not found (the number for a new record created in KTD is 439): /cgi-bin/koha/opac-detail.pl?biblionumber=439 - Search results, with one item withdrawn and the other available - search for test or TEST: does not show up the search results (there are three other records) 3. Tests fail for t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t - can fix by changing 12 to 13: .. 1/12 # Looks like you planned 12 tests but ran 13. Testing notes (using KTD and ES9): 1. Apply the patches, including dependent bugs: - Rebuild everything (not sure if this is required or not): yarn build - Restart everything: restart_all - Rebuild the index: koha-elasticsearch --rebuild -d -b -a kohadev 2. Run through the test plan for the patch: Add end-to-end test for the query-time OpacHiddenItems filter - Step 5 (as per wiki) for OpacHiddenItems (no changes required to a standard KTD instance for OpacHiddenItemsHidesRecord and OpacHiddenItemsExceptions): withdrawn: [1] - Step 9: Get a page not found when searching or directly accessing: Sorry, the requested page is not available Error 404 . If the record doesn't exist (or is hidden) shouldn't it be no results found, instead of a 404? For example, search for DITA: No results found! No results found for that in catalog. . If you access the record directly with all items hidden, then I would expect a page not found (the number for a new record created in KTD is 439): /cgi-bin/koha/opac-detail.pl?biblionumber=439 . Search results, with one item withdrawn and the other available - search for test or TEST: does not show up the search results (there are three other records) 3. Run through the test plan for patch: Pass the searching patron into build_query_compat for OpacHiddenItemsExceptions 4. Run through steps 1 and 2 of the test plan for patch: (QA follow-up) Fix perlcritic and POD coverage findings -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35052 --- Comment #27 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Following the benchmarks and discussion on bug 42835 (comments 82 and 83), we are proposing to move to nested items for Elasticsearch rather than a separate items index. If the split proposed there is agreed, this bug would be repurposed to cover enforcing OpacHiddenItems and OpacHiddenItemsHidesRecord at search time on nested items (including an allowlist of the item fields a user-supplied limit may reference, so hiding rules cannot be overridden from the URL). It would depend on the same-item filters and facets work (bug 43151, itself repurposed), instead of the separate items index. Nothing changes here yet: the title and the existing patches are untouched until the split is agreed on bug 42835. This note is just so that anyone following this bug knows what is being discussed. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org