Bug 17377 - ES - control fields are not taken into account
Summary: ES - control fields are not taken into account
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 12478
Blocks:
  Show dependency treegraph
 
Reported: 2016-09-29 15:20 UTC by Jonathan Druart
Modified: 2017-06-14 22:18 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17377: Add tests for json2marc (1.54 KB, patch)
2016-09-29 15:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17377: ES - Take control fields into account (2.45 KB, patch)
2016-09-29 15:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17377: Add tests for json2marc (1.60 KB, patch)
2016-09-30 10:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17377: ES - Take control fields into account (2.50 KB, patch)
2016-09-30 10:28 UTC, Nick Clemens
Details | Diff | Splinter Review
[PASSED QA] Bug 17377: Add tests for json2marc (1.67 KB, patch)
2016-11-18 09:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 17377: ES - Take control fields into account (2.58 KB, patch)
2016-11-18 09:07 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-09-29 15:20:43 UTC
There is a bug in Koha::SearchEngine::Elasticsearc::Search->json2marc, it assumes that the MARC::Field constructor always takes >= 5 parameters.
This assumption is wrong for control fields, to create a control field you need to call the constructor with:
  MARC::Field->new($tag, $value);

Note that I got "Too much data for control field" in the _warning value of my MARC::Field because too many parameters were passed, and the value was undef.

That broke the result search links (on the staff interface) for DB with biblio.biblionumber mapped with 001.

Other bugs will certainly be fixed by this patch.
For instance I got:
  GetMarcBiblio called with undefined biblionumber at /home/koha/src/opac/opac-search.pl line 664.
  GetCOinSBiblio called with undefined record at /home/koha/src/opac/opac-search.pl line 665.

in my logs.
Comment 1 Jonathan Druart 2016-09-29 15:43:20 UTC
Created attachment 55932 [details] [review]
Bug 17377: Add tests for json2marc
Comment 2 Jonathan Druart 2016-09-29 15:43:23 UTC
Created attachment 55933 [details] [review]
Bug 17377: ES - Take control fields into account

There is a bug in Koha::SearchEngine::Elasticsearc::Search->json2marc, it
assumes that the MARC::Field constructor always takes >= 5 parameters.
This assumption is wrong for control fields, to create a control field you
need to call the constructor with:
  MARC::Field->new($tag, $value);

Note that I got "Too much data for control field" in the _warning value of my
MARC::Field because too many parameters were passed, and the value was undef.

That broke the result search links (on the staff interface) for DB with
biblio.biblionumber mapped with 001.

Other bugs will certainly be fixed by this patch.
For instance I got:
  GetMarcBiblio called with undefined biblionumber at /home/koha/src/opac/opac-search.pl line 664.
  GetCOinSBiblio called with undefined record at /home/koha/src/opac/opac-search.pl line 665.
in my logs, with this patch I don't get them anymore.

Test plan:
You can try to recreate the different issues and confirm than this patch
fixes them.
Or just run the tests
Comment 3 Nick Clemens 2016-09-30 10:28:06 UTC
Created attachment 55954 [details] [review]
Bug 17377: Add tests for json2marc

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Nick Clemens 2016-09-30 10:28:11 UTC
Created attachment 55955 [details] [review]
Bug 17377: ES - Take control fields into account

There is a bug in Koha::SearchEngine::Elasticsearc::Search->json2marc, it
assumes that the MARC::Field constructor always takes >= 5 parameters.
This assumption is wrong for control fields, to create a control field you
need to call the constructor with:
  MARC::Field->new($tag, $value);

Note that I got "Too much data for control field" in the _warning value of my
MARC::Field because too many parameters were passed, and the value was undef.

That broke the result search links (on the staff interface) for DB with
biblio.biblionumber mapped with 001.

Other bugs will certainly be fixed by this patch.
For instance I got:
  GetMarcBiblio called with undefined biblionumber at /home/koha/src/opac/opac-search.pl line 664.
  GetCOinSBiblio called with undefined record at /home/koha/src/opac/opac-search.pl line 665.
in my logs, with this patch I don't get them anymore.

Test plan:
You can try to recreate the different issues and confirm than this patch
fixes them.
Or just run the tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Nick Clemens 2016-09-30 10:29:13 UTC
I can't recreate the errors, but code looks correct

Tests pass with caveat that the very first run failed, I can no longer recreate that problem though and don't see an issue in the tests
Comment 6 Martin Renvoize 2016-11-18 09:07:02 UTC
Created attachment 57620 [details] [review]
[PASSED QA] Bug 17377: Add tests for json2marc

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2016-11-18 09:07:13 UTC
Created attachment 57621 [details] [review]
[PASSED QA] Bug 17377: ES - Take control fields into account

There is a bug in Koha::SearchEngine::Elasticsearc::Search->json2marc, it
assumes that the MARC::Field constructor always takes >= 5 parameters.
This assumption is wrong for control fields, to create a control field you
need to call the constructor with:
  MARC::Field->new($tag, $value);

Note that I got "Too much data for control field" in the _warning value of my
MARC::Field because too many parameters were passed, and the value was undef.

That broke the result search links (on the staff interface) for DB with
biblio.biblionumber mapped with 001.

Other bugs will certainly be fixed by this patch.
For instance I got:
  GetMarcBiblio called with undefined biblionumber at /home/koha/src/opac/opac-search.pl line 664.
  GetCOinSBiblio called with undefined record at /home/koha/src/opac/opac-search.pl line 665.
in my logs, with this patch I don't get them anymore.

Test plan:
You can try to recreate the different issues and confirm than this patch
fixes them.
Or just run the tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2016-11-18 09:07:51 UTC
Thanks Jonathan, all works as expected, passing QA
Comment 9 Kyle M Hall 2016-11-18 13:53:46 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 10 Mason James 2016-12-08 03:15:49 UTC
Pushed to 16.05.x, will be in 16.05.06