Description
Elaine Bradtke
2014-08-19 22:12:59 UTC
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. |