Bug 18579 - Problem with :Filter::MARC::EmbedItemsAvailability
Summary: Problem with :Filter::MARC::EmbedItemsAvailability
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 18208
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-11 12:49 UTC by Koha Team Lyon 3
Modified: 2017-05-15 05:38 UTC (History)
8 users (show)

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


Attachments
Bug 18579: Regression tests (1.99 KB, patch)
2017-05-11 16:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18579: Make EmbedItemsAvailability handle control fields correctly (1.68 KB, patch)
2017-05-11 16:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 18579: Regression tests (2.06 KB, patch)
2017-05-12 05:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 18579: Make EmbedItemsAvailability handle control fields correctly (1.75 KB, patch)
2017-05-12 05:31 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 18579: Regression tests (2.14 KB, patch)
2017-05-12 05:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18579: Make EmbedItemsAvailability handle control fields correctly (1.83 KB, patch)
2017-05-12 05:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18579: Fix call to append_fields, fix tests (2.04 KB, patch)
2017-05-12 07:34 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18579: Fix call to append_fields, fix tests (2.13 KB, patch)
2017-05-12 08:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Koha Team 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!