Bug 18579

Summary: Problem with :Filter::MARC::EmbedItemsAvailability
Product: Koha Reporter: Koha Team University Lyon 3 <koha>
Component: CatalogingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: critical    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, julian.maurice, katrin.fischer, koha, kyle, m.de.rooy, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 18208    
Bug Blocks:    
Attachments: Bug 18579: Regression tests
Bug 18579: Make EmbedItemsAvailability handle control fields correctly
[SIGNED OFF] Bug 18579: Regression tests
[SIGNED OFF] Bug 18579: Make EmbedItemsAvailability handle control fields correctly
Bug 18579: Regression tests
Bug 18579: Make EmbedItemsAvailability handle control fields correctly
Bug 18579: Fix call to append_fields, fix tests
Bug 18579: Fix call to append_fields, fix tests

Description Koha Team University Lyon 3 2017-05-11 12:49:37 UTC
This is a real bug, in
Koha::Filter::MARC::EmbedItemsAvailability::_processrecord, $biblionumber_field
should be corrected to allow controlfield like 001.
Comment 1 Julian Maurice 2017-05-11 12:56:27 UTC
Upgrading severity to critical, this prevents reindexing zebra:

$ misc/migration_tools/rebuild_zebra.pl -r -b -v
Zebra configuration information
================================
Zebra biblio directory      = /home/koha/env/master/var/lib/zebradb/biblios
Zebra authorities directory = /home/koha/env/master/var/lib/zebradb/authorities
Koha directory              = /home/koha/env/master/src
Lockfile                    = /home/koha/env/master/var/lock/zebradb/rebuild/rebuild..LCK
BIBLIONUMBER in :     001$@
BIBLIOITEMNUMBER in : 090$a
================================
skipping authorities
====================
exporting biblio
====================
1Control fields (generally, just tags below 010) do not have subfields, use data() at /home/koha/env/master/src/Koha/Filter/MARC/EmbedItemsAvailability.pm line 75.
Comment 2 Marcel de Rooy 2017-05-11 12:59:49 UTC
(In reply to Koha Team Lyon 3 from comment #0)
> This is a real bug, in
> Koha::Filter::MARC::EmbedItemsAvailability::_processrecord,
> $biblionumber_field
> should be corrected to allow controlfield like 001.

Good catch!
Should use logic as in Koha/SearchEngine/Search.pm (extract_biblionumber).

Ping Tomas
Comment 3 Tomás Cohen Arazi 2017-05-11 16:10:42 UTC
Created attachment 63389 [details] [review]
Bug 18579: Regression tests

This patch makes sure the tests have the biblio.biblionumber mapping mocked
so we test the case where the mapping is to a control field instead of just
regular data fields (in the case of UNIMARC).

To test:
- Apply the patch
- Run:
  $ prove t/db_dependent/Koha/Filter/EmbedItemsAvailability.t
=> FAIL: Tests fail due to an attemp to access a subfield on a control field.

Sponsored-by: ByWater Solutions
Comment 4 Tomás Cohen Arazi 2017-05-11 16:10:46 UTC
Created attachment 63390 [details] [review]
Bug 18579: Make EmbedItemsAvailability handle control fields correctly

This patch makes the EmbedItemsAvailability filter handle the biblio.biblionumber > control field mapping gracefully.
Right now, it was assuming a regular data field was used, and such is not the case for UNIMARC (001).

To test:
- Apply the tests patch
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Koha/Filter/EmbedItemsAvailability.t
=> FAIL: Tests fail to run
- Apply this patch
- Run:
 k$ prove t/db_dependent/Koha/Filter/EmbedItemsAvailability.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions
Comment 5 Tomás Cohen Arazi 2017-05-11 16:12:11 UTC
Hi, please test on your UNIMARC test environment! I provided a fix for the tests so they actually cover the control field case, and the patch correctly makes it work as expected (thanks marcelr for the pointer).
Comment 6 Katrin Fischer 2017-05-12 05:30:45 UTC
Created attachment 63394 [details] [review]
[SIGNED OFF] Bug 18579: Regression tests

This patch makes sure the tests have the biblio.biblionumber mapping mocked
so we test the case where the mapping is to a control field instead of just
regular data fields (in the case of UNIMARC).

To test:
- Apply the patch
- Run:
  $ prove t/db_dependent/Koha/Filter/EmbedItemsAvailability.t
=> FAIL: Tests fail due to an attemp to access a subfield on a control field.

Sponsored-by: ByWater Solutions

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Katrin Fischer 2017-05-12 05:31:31 UTC
Created attachment 63395 [details] [review]
[SIGNED OFF] Bug 18579: Make EmbedItemsAvailability handle control fields correctly

This patch makes the EmbedItemsAvailability filter handle the biblio.biblionumber > control field mapping gracefully.
Right now, it was assuming a regular data field was used, and such is not the case for UNIMARC (001).

To test:
- Apply the tests patch
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Koha/Filter/EmbedItemsAvailability.t
=> FAIL: Tests fail to run
- Apply this patch
- Run:
 k$ prove t/db_dependent/Koha/Filter/EmbedItemsAvailability.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Marcel de Rooy 2017-05-12 05:41:11 UTC
Created attachment 63396 [details] [review]
Bug 18579: Regression tests

This patch makes sure the tests have the biblio.biblionumber mapping mocked
so we test the case where the mapping is to a control field instead of just
regular data fields (in the case of UNIMARC).

To test:
- Apply the patch
- Run:
  $ prove t/db_dependent/Koha/Filter/EmbedItemsAvailability.t
=> FAIL: Tests fail due to an attemp to access a subfield on a control field.

Sponsored-by: ByWater Solutions

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2017-05-12 05:41:16 UTC
Created attachment 63397 [details] [review]
Bug 18579: Make EmbedItemsAvailability handle control fields correctly

This patch makes the EmbedItemsAvailability filter handle the biblio.biblionumber > control field mapping gracefully.
Right now, it was assuming a regular data field was used, and such is not the case for UNIMARC (001).

To test:
- Apply the tests patch
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Koha/Filter/EmbedItemsAvailability.t
=> FAIL: Tests fail to run
- Apply this patch
- Run:
 k$ prove t/db_dependent/Koha/Filter/EmbedItemsAvailability.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Julian Maurice 2017-05-12 07:21:56 UTC
There are still problems in a UNIMARC setup:

#   Failed test 'EmbedItemsAvailability tests'
#   at t/db_dependent/Koha/Filter/EmbedItemsAvailability.t line 151.
Can't call method "subfield" on an undefined value at t/db_dependent/Koha/Filter/EmbedItemsAvailability.t line 145.

and rebuild_zebra.pl die with:

Arguments must be MARC::Field objects at /home/koha/env/master/src/Koha/Filter/MARC/EmbedItemsAvailability.pm line 93

I'm working on a patch
Comment 11 Marcel de Rooy 2017-05-12 07:27:02 UTC
Good catch Julian. (Poor Tomas ;)
Comment 12 Julian Maurice 2017-05-12 07:34:19 UTC
Created attachment 63414 [details] [review]
Bug 18579: Fix call to append_fields, fix tests

MARC::Record::append_fields takes a list of MARC::Field (not an arrayref)

Use $record->subfield() instead of $record->field()->subfield() to avoid errors
when field doesn't exist
Comment 13 Julian Maurice 2017-05-12 07:37:50 UTC
Back to Signed off
Comment 14 Marcel de Rooy 2017-05-12 08:00:58 UTC
Created attachment 63415 [details] [review]
Bug 18579: Fix call to append_fields, fix tests

MARC::Record::append_fields takes a list of MARC::Field (not an arrayref)

Use $record->subfield() instead of $record->field()->subfield() to avoid errors
when field doesn't exist

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Tomás Cohen Arazi 2017-05-12 10:46:23 UTC
(In reply to Marcel de Rooy from comment #11)
> Good catch Julian. (Poor Tomas ;)

Hahaha
Comment 16 Kyle M Hall 2017-05-12 12:43:31 UTC
Pushed to master for 17.05, thanks Tomas, Julian!
Comment 17 Katrin Fischer 2017-05-14 09:45:45 UTC
Patches don't apply to 16.11.x - please provide patches for 16.11.x if it's needed there too!
Comment 18 Tomás Cohen Arazi 2017-05-15 02:46:48 UTC
(In reply to Katrin Fischer from comment #17)
> Patches don't apply to 16.11.x - please provide patches for 16.11.x if it's
> needed there too!

Not needed!