Bug 12788

Summary: Refine search displays 4xx fields from authority records
Product: Koha Reporter: Elaine Bradtke <eb>
Component: SearchingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Galen Charlton <gmcharlt>
Severity: normal    
Priority: P5 - low CC: dcook, jcamins, katrin.fischer, mathsabypro, sandboxes, tomascohen, veron
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12347
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bub 12788: (regression test) refactor facet extraction code to allow testing
Bug 12788: facets calculation should skip 100 if ind1=z
(screenshot) without patch
(screenshot) with patch
Bug 12788: facets calculation should skip 100 if ind1=z
Bug 12788: (followup) minor optimization with proper tests
Bub 12788: (regression test) refactor facet extraction code to allow testing
Bug 12788: facets calculation should skip 100 if ind1=z
Bug 12788: (followup) minor optimization with proper tests
Bub 12788: (regression test) refactor facet extraction code to allow testing
Bug 12788: facets calculation should skip 100 if ind1=z
Bug 12788: (followup) minor optimization with proper tests
[PASSED QA] Bub 12788: (regression test) refactor facet extraction code to allow testing
[PASSED QA] Bug 12788: facets calculation should skip 100 if ind1=z
[PASSED QA] Bug 12788: (followup) minor optimization with proper tests

Description Elaine Bradtke 2014-08-19 22:12:59 UTC
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".
Comment 1 Tomás Cohen Arazi 2014-08-20 04:07:29 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2014-08-20 04:07:33 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2014-08-20 04:11:36 UTC
(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).
Comment 4 Tomás Cohen Arazi 2014-08-20 04:14:46 UTC
Created attachment 31004 [details]
(screenshot) without patch
Comment 5 Tomás Cohen Arazi 2014-08-20 04:15:14 UTC
Created attachment 31005 [details]
(screenshot) with patch
Comment 6 Tomás Cohen Arazi 2014-08-20 13:50:21 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2014-08-20 14:46:23 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2014-08-23 22:37:28 UTC
Just leaving an encouraging note: This should be testable with the sandboxes :) Running the unit tests can be done by QA.
Comment 9 Tomás Cohen Arazi 2014-08-26 23:13:55 UTC
This one is important for 3.16 users. Please take some minutes to test it.
Comment 10 Biblibre Sandboxes 2014-08-28 19:35:30 UTC
Patch tested with a sandbox, by Nick Clemens <nick@quecheelibrary.org>
Comment 11 Biblibre Sandboxes 2014-08-28 19:36:09 UTC Comment hidden (obsolete)
Comment 12 Biblibre Sandboxes 2014-08-28 19:36:13 UTC Comment hidden (obsolete)
Comment 13 Biblibre Sandboxes 2014-08-28 19:36:16 UTC Comment hidden (obsolete)
Comment 14 David Cook 2014-09-02 03:26:56 UTC Comment hidden (obsolete)
Comment 15 David Cook 2014-09-02 03:27:04 UTC Comment hidden (obsolete)
Comment 16 David Cook 2014-09-02 03:27:11 UTC Comment hidden (obsolete)
Comment 17 David Cook 2014-09-02 03:28:05 UTC
Patches and unit tests work as described.
Comment 18 Katrin Fischer 2014-09-05 18:34:26 UTC
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.
Comment 19 Katrin Fischer 2014-09-05 18:34:40 UTC
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>
Comment 20 Katrin Fischer 2014-09-05 18:34:57 UTC
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>
Comment 21 Tomás Cohen Arazi 2014-09-05 19:39:34 UTC
Patches pushed to master.
Comment 22 Katrin Fischer 2014-09-05 19:40:33 UTC
Thanks Tomas!
Comment 23 Galen Charlton 2014-09-23 20:54:13 UTC
Pushed to 3.16.x for inclusion in 3.16.4.
Comment 24 Mathieu Saby 2014-10-25 16:17:14 UTC
Do you know if the issue was also occuring for UNIMARC Koha?

M. Saby
Comment 25 Tomás Cohen Arazi 2014-10-27 23:36:21 UTC
(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.