When looking at the results from a search, the Refine Search column displays the headings from the 1xx and 4xx fields in the authorities. This means that all the variations on a term that are not used in the catalog appear as search options. (For example the half dozen different ways to spell and punctuate Ralph Vaughan Williams). This should not be happening. It is confusing to the patrons and needlessly extends the list of search terms (as all the 4xx terms point back to the correct 1xx) Heather Hernandez commented: "if you click one of the facets from a 400 field, it does return correct results based on the 100 field--e.g., if you do a search on Sea Poetry: http://keys.bywatersolutions.com/cgi-bin/koha/opac-search.pl?q=sea+poetry And you click on the facet "Sinbad," (a 400 in the aut record) it will return the two results by "Dingle, Aylward Edward," (which is the 100 field in that aut record), but the user will have no idea why--there's no evidence that your search has been redirected due to your selecting a "see from" reference." Chris Cormack wrote: "We think we have tracked it down to something to do with fix for large MARC records But are continuing to work on it. It is something to do with the IncludeSeeFromInSearches systempreference If you have had this on, or have it on, it makes the links so that when you search for the 400, you can still find results. To do this its linked, however in 3.12.x and 3.14.x it appears these links were filtered out when building the facets and for some reason this is not occuring. I think its to do with an indicator of z that gets added to 100 (of the biblio) on indexing (you wont see it on the record itself, its added when indexing) The old code got rid of this, so no, this is not required behaviour".
Created attachment 31002 [details] [review] Bub 12788: (regression test) refactor facet extraction code to allow testing This patch refactors the facet extraction loop into a proper function. The loop is changed so the MARC::Record objects are created only once instead of the old/current behaviour: once for each defined facet (in C4::Koha::getFacets). To test: - Apply the patch => SUCCESS: verify facets functionality remains unchanged. - Run: $ prove -v t/db_dependent/Search.t => SUCCESS: tests for _get_facets_data_from_record fail, because 100$a is considered for fields with indicator 1=z (field added by IncludeSeeFromInSearches syspref). - Sign off :-D Sponsored-by: Universidad Nacional de Cordoba
Created attachment 31003 [details] [review] Bug 12788: facets calculation should skip 100 if ind1=z This patch adds a test for field 100, to skip it on facet calculation if ind1=z. To test: - Have IncludeSeeFromInSearches set. - Create a biblio record, when adding an author, create a new authority record that contains a 400$a field (see from). - Rebuild zebra db. - Search for the record making sure the search returns more than one record. => FAIL: the facets contain the 'see from' field. - Run $ prove -v t/db_dependent/Search.t => FAIL: it fails - Apply the patch - Run $ prove -v t/db_dependent/Search.t => SUCCESS: it passes - Re-run the search, notice the 'see from' doesn't show anymore on the facets. - Sign off :-D Regards To+ Sponsored-by: Universidad Nacional de Cordoba
(In reply to Tomás Cohen Arazi from comment #2) > Created attachment 31003 [details] [review] [review] > Bug 12788: facets calculation should skip 100 if ind1=z > > This patch adds a test for field 100, to skip it on facet calculation > if ind1=z. I notice I forgot to amend the patch description: it is not field 100-specific, but all fields with ind1=z, which is Koha::Filter::EmbedSeeFromHeadings behaviour (i.e.what happens at rebuild_zebra.pl to the records).
Created attachment 31004 [details] (screenshot) without patch
Created attachment 31005 [details] (screenshot) with patch
Created attachment 31026 [details] [review] Bug 12788: facets calculation should skip 100 if ind1=z This patch adds a test for field 100, to skip it on facet calculation if ind1=z. To test: - Have IncludeSeeFromInSearches set. - Create a biblio record, when adding an author, create a new authority record that contains a 400$a field (see from). - Rebuild zebra db. - Search for the record making sure the search returns more than one record. => FAIL: the facets contain the 'see from' field. - Run $ prove -v t/db_dependent/Search.t => FAIL: it fails - Apply the patch - Run $ prove -v t/db_dependent/Search.t => SUCCESS: it passes - Re-run the search, notice the 'see from' doesn't show anymore on the facets. - Sign off :-D Edit: minor stylistic change Regards To+ Sponsored-by: Universidad Nacional de Cordoba
Created attachment 31027 [details] [review] Bug 12788: (followup) minor optimization with proper tests This patch removes the $facets_info calculation from the _get_facets_data_from_record sub so it is not done for each record. It introduces a new sub, _get_facets_info that is called from the getRecords loop, that does the job only once. To test: - Apply on top of the previous patches - Run $ prove -v t/db_dependent/Search.t => SUCCESS: _get_facets_info gets tested and it passes for both MARC21 and UNIMARC. Facets rendering should remain unchaged on the UI. - Sign off :-D Sponsored-by: Universidad Nacional de Cordoba
Just leaving an encouraging note: This should be testable with the sandboxes :) Running the unit tests can be done by QA.
This one is important for 3.16 users. Please take some minutes to test it.
Patch tested with a sandbox, by Nick Clemens <nick@quecheelibrary.org>
Created attachment 31267 [details] [review] Bub 12788: (regression test) refactor facet extraction code to allow testing This patch refactors the facet extraction loop into a proper function. The loop is changed so the MARC::Record objects are created only once instead of the old/current behaviour: once for each defined facet (in C4::Koha::getFacets). To test: - Apply the patch => SUCCESS: verify facets functionality remains unchanged. - Run: $ prove -v t/db_dependent/Search.t => SUCCESS: tests for _get_facets_data_from_record fail, because 100$a is considered for fields with indicator 1=z (field added by IncludeSeeFromInSearches syspref). - Sign off :-D Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Created attachment 31268 [details] [review] Bug 12788: facets calculation should skip 100 if ind1=z This patch adds a test for field 100, to skip it on facet calculation if ind1=z. To test: - Have IncludeSeeFromInSearches set. - Create a biblio record, when adding an author, create a new authority record that contains a 400$a field (see from). - Rebuild zebra db. - Search for the record making sure the search returns more than one record. => FAIL: the facets contain the 'see from' field. - Run $ prove -v t/db_dependent/Search.t => FAIL: it fails - Apply the patch - Run $ prove -v t/db_dependent/Search.t => SUCCESS: it passes - Re-run the search, notice the 'see from' doesn't show anymore on the facets. - Sign off :-D Edit: minor stylistic change Regards To+ Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Created attachment 31269 [details] [review] Bug 12788: (followup) minor optimization with proper tests This patch removes the $facets_info calculation from the _get_facets_data_from_record sub so it is not done for each record. It introduces a new sub, _get_facets_info that is called from the getRecords loop, that does the job only once. To test: - Apply on top of the previous patches - Run $ prove -v t/db_dependent/Search.t => SUCCESS: _get_facets_info gets tested and it passes for both MARC21 and UNIMARC. Facets rendering should remain unchaged on the UI. - Sign off :-D Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Created attachment 31315 [details] [review] Bub 12788: (regression test) refactor facet extraction code to allow testing This patch refactors the facet extraction loop into a proper function. The loop is changed so the MARC::Record objects are created only once instead of the old/current behaviour: once for each defined facet (in C4::Koha::getFacets). To test: - Apply the patch => SUCCESS: verify facets functionality remains unchanged. - Run: $ prove -v t/db_dependent/Search.t => SUCCESS: tests for _get_facets_data_from_record fail, because 100$a is considered for fields with indicator 1=z (field added by IncludeSeeFromInSearches syspref). - Sign off :-D Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Nick Clemens <nick@quecheelibrary.org> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 31316 [details] [review] Bug 12788: facets calculation should skip 100 if ind1=z This patch adds a test for field 100, to skip it on facet calculation if ind1=z. To test: - Have IncludeSeeFromInSearches set. - Create a biblio record, when adding an author, create a new authority record that contains a 400$a field (see from). - Rebuild zebra db. - Search for the record making sure the search returns more than one record. => FAIL: the facets contain the 'see from' field. - Run $ prove -v t/db_dependent/Search.t => FAIL: it fails - Apply the patch - Run $ prove -v t/db_dependent/Search.t => SUCCESS: it passes - Re-run the search, notice the 'see from' doesn't show anymore on the facets. - Sign off :-D Edit: minor stylistic change Regards To+ Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Nick Clemens <nick@quecheelibrary.org> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 31317 [details] [review] Bug 12788: (followup) minor optimization with proper tests This patch removes the $facets_info calculation from the _get_facets_data_from_record sub so it is not done for each record. It introduces a new sub, _get_facets_info that is called from the getRecords loop, that does the job only once. To test: - Apply on top of the previous patches - Run $ prove -v t/db_dependent/Search.t => SUCCESS: _get_facets_info gets tested and it passes for both MARC21 and UNIMARC. Facets rendering should remain unchaged on the UI. - Sign off :-D Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Nick Clemens <nick@quecheelibrary.org> Signed-off-by: David Cook <dcook@prosentient.com.au>
Patches and unit tests work as described.
Created attachment 31422 [details] [review] [PASSED QA] Bub 12788: (regression test) refactor facet extraction code to allow testing This patch refactors the facet extraction loop into a proper function. The loop is changed so the MARC::Record objects are created only once instead of the old/current behaviour: once for each defined facet (in C4::Koha::getFacets). To test: - Apply the patch => SUCCESS: verify facets functionality remains unchanged. - Run: $ prove -v t/db_dependent/Search.t => SUCCESS: tests for _get_facets_data_from_record fail, because 100$a is considered for fields with indicator 1=z (field added by IncludeSeeFromInSearches syspref). - Sign off :-D Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Nick Clemens <nick@quecheelibrary.org> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Works as described, passes tests and QA script.
Created attachment 31423 [details] [review] [PASSED QA] Bug 12788: facets calculation should skip 100 if ind1=z This patch adds a test for field 100, to skip it on facet calculation if ind1=z. To test: - Have IncludeSeeFromInSearches set. - Create a biblio record, when adding an author, create a new authority record that contains a 400$a field (see from). - Rebuild zebra db. - Search for the record making sure the search returns more than one record. => FAIL: the facets contain the 'see from' field. - Run $ prove -v t/db_dependent/Search.t => FAIL: it fails - Apply the patch - Run $ prove -v t/db_dependent/Search.t => SUCCESS: it passes - Re-run the search, notice the 'see from' doesn't show anymore on the facets. - Sign off :-D Edit: minor stylistic change Regards To+ Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Nick Clemens <nick@quecheelibrary.org> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 31424 [details] [review] [PASSED QA] Bug 12788: (followup) minor optimization with proper tests This patch removes the $facets_info calculation from the _get_facets_data_from_record sub so it is not done for each record. It introduces a new sub, _get_facets_info that is called from the getRecords loop, that does the job only once. To test: - Apply on top of the previous patches - Run $ prove -v t/db_dependent/Search.t => SUCCESS: _get_facets_info gets tested and it passes for both MARC21 and UNIMARC. Facets rendering should remain unchaged on the UI. - Sign off :-D Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Nick Clemens <nick@quecheelibrary.org> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Patches pushed to master.
Thanks Tomas!
Pushed to 3.16.x for inclusion in 3.16.4.
Do you know if the issue was also occuring for UNIMARC Koha? M. Saby
(In reply to mathieu saby from comment #24) > Do you know if the issue was also occuring for UNIMARC Koha? > > M. Saby It was also occuring to UNIMARC. Because the original code foreach my $field (@fields) { # If $field->indicator(1) eq 'z', it means it is a 'see from' # field introduced because of IncludeSeeFromInSearches, so skip it next if $field->indicator(1) eq 'z'; Applied to all fields no matter the flavour. The bug was introduced by me and happened in 3.16.0, with the patches for bug 11096.