Bug 19049 - Fix regression on stage-marc-import with to_marc plugin
Summary: Fix regression on stage-marc-import with to_marc plugin
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 10407
Blocks: 19163
  Show dependency treegraph
 
Reported: 2017-08-07 10:53 UTC by Marcel de Rooy
Modified: 2019-06-27 09:24 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19049: Restore import with to_marc plugin (2.97 KB, patch)
2017-08-07 19:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19049: Restore import with to_marc plugin (5.63 KB, patch)
2017-08-08 15:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19049: Tests (4.70 KB, patch)
2017-08-08 15:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19049: Fix regression on stage-marc-import with to_marc plugin (8.65 KB, patch)
2017-08-09 12:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19049: Testing RecordsFromMarcPlugin with a to_marc plugin (5.72 KB, patch)
2017-08-09 12:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19049: [Follow-up] Make sure that GetPlugins finds the plugin (1.69 KB, patch)
2017-08-10 11:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED OFF] Bug 19049: Fix regression on stage-marc-import with to_marc plugin (8.68 KB, patch)
2017-08-10 11:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 19049: Testing RecordsFromMarcPlugin with a to_marc plugin (5.75 KB, patch)
2017-08-10 11:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 19049: [Follow-up] Make sure that GetPlugins finds the plugin (1.72 KB, patch)
2017-08-10 11:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19049: Fix regression on stage-marc-import with to_marc plugin (8.72 KB, patch)
2017-08-10 12:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19049: Testing RecordsFromMarcPlugin with a to_marc plugin (5.79 KB, patch)
2017-08-10 12:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19049: [Follow-up] Make sure that GetPlugins finds the plugin (1.83 KB, patch)
2017-08-10 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19049 [QA Followup] - Make plugin name first item in description (1.17 KB, patch)
2017-08-10 13:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19049: [QA Follow-up] Mock config, default format (4.04 KB, patch)
2017-08-11 06:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19049: [QA Follow-up] Mock config, default format (4.20 KB, patch)
2017-08-11 06:57 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 Marcel de Rooy 2017-08-07 10:53:06 UTC
Needed since 10407 broke this undocumented feature. Interface and code were not clear enough.
Comment 1 Marcel de Rooy 2017-08-07 11:06:56 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2017-08-07 19:32:02 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2017-08-08 15:50:40 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2017-08-08 15:50:44 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2017-08-09 12:38:50 UTC
Created attachment 65707 [details] [review]
Bug 19049: Fix regression on stage-marc-import with to_marc plugin

Bug 12412 added the use of to_marc plugins allowing arbitrary file formats
in stage-marc-import (as long as the plugins can handle them). The feature
was not very visible in the code, and when bug 10407 added the marcxml
format, it made some changes that broke the use of to_marc.

This patch restores the functionality by:
[1] Adding a sub RecordsFromMarcPlugin to ImportBatch.pm, specifically
    addressing the conversion from arbitrary formats to MARC::Record.
    The original to_marc interface is used: pass it the file contents,
    and it returns a string consisting of a number of MARC blobs separated
    by \x1D.
    Consequently, the call of to_marc is removed from routine
    BatchStageMarcRecords where it did not belong. The to_marc_plugin
    parameter is removed and two calls are adjusted accordingly.
[2] Instead of a separate combo with plugins, the format combo contains
    MARC, MARCXML and optionally some plugin formats.
[3] The code in stage-marc-import.pl now clearly shows the three main
    format types: MARC, MARCXML or plugin based.

Note: This patch restores more or less the situation after bug 12412, but
I would actually recommend to have the to_marc plugins return MARC::Record
objects instead of large text strings. In the second example I added a
to_marc plugin that actually converts MARC record objects to string format,
while RecordsFromMarcPlugin reconverts them to MARC::Records.

Test plan:
See second patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2017-08-09 12:38:54 UTC
Created attachment 65708 [details] [review]
Bug 19049: Testing RecordsFromMarcPlugin with a to_marc plugin

This patch adds a simple to_marc plugin in t/Koha/Plugin that is used
in the added subtest in ImportBatch.t.

Test plan:
[1] Run t/db_dependent/ImportBatch.t
[2] Copy the to_marc test plugin from t to your plugin directory.
    Under Debian packages, you should do something like:
    mkdir -p /var/lib/koha/master/plugins/Koha/Plugin/
    cp [yourclone]/t/Koha/Plugin/MarcFieldValues.pm /var/lib/koha/master/plugins/Koha/Plugin/
[3] Check if you see this plugin on plugins/plugins-home.pl
[4] Create a text file with some fields like:
    (Note: The plugin needs an empty line between both "records".)
    100,a = Test Author 1
    245,a = Title One

    100,a = Author 2
    245,a = Title Two
[5] Go to stage-marc-import.pl. Upload the created file. Select the plugin
    in the format combo and proceed. Did you create two records ?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2017-08-09 12:47:37 UTC
Your assistance in testing this is appreciated now.
Would be nice too if we could backport this to 17.05 and 16.11 sooner.

Looking at the code changes, that should be possible. The first patch actually applies under 16.11. The GetPlugins call with metadata should also work under 16.11 in the test script.
Comment 8 Marcel de Rooy 2017-08-10 09:48:10 UTC
Increasing Severity
Comment 9 Katrin Fischer 2017-08-10 10:30:02 UTC
Sorry Marcel, but tests are failing:

Files=1, Tests=14,  2 wallclock secs ( 0.02 usr  0.00 sys +  1.22 cusr  0.26 csys =  1.50 CPU)
Result: FAIL
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ perl t/db_dependent/ImportBatch.t
1..14
ok 1 - use C4::ImportBatch;
ok 2 - AddImportBatch for sample_import_batch1 return an id
ok 3 - AddImportBatch for sample_import_batch2 return an id
ok 4 - GetImportBatch returns the right informations about HASH(0x99e12b8)
ok 5 - GetImportBatch returns the right informations about HASH(0x99e13f0)
ok 6 - GetRecordFromImportBiblio should return the record with items if specified
ok 7 - GetRecordFromImportBiblio should return the record without items by default
ok 8 - No item numbers expected since we did not commit
ok 9 - Expecting two items now
ok 10 - Check the first returned itemnumber
ok 11 - No item numbers expected since we deleted the item
ok 12 - Batch 3 has been cleaned
ok 13 - Batch 3 has been deleted
# Subtest: RecordsFromMarcPlugin
    1..5
    # No tests run!
not ok 14 - No tests run for subtest "RecordsFromMarcPlugin"
#   Failed test 'No tests run for subtest "RecordsFromMarcPlugin"'
#   at t/db_dependent/ImportBatch.t line 195.
Can't call method "GetPlugins" on an undefined value at t/db_dependent/ImportBatch.t line 186.
# Looks like your test exited with 255 just after 14.

Also QA tool complains about POD coverage, but I think that is less important.
Comment 10 Marcel de Rooy 2017-08-10 11:04:26 UTC
Created attachment 65797 [details] [review]
Bug 19049: [Follow-up] Make sure that GetPlugins finds the plugin

We have to mock_config the pluginsdir before Plugins is loaded, and
we should pass an absolute path (not a relative one).
If you did not install the test to_marc plugin, this test would fail.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2017-08-10 11:19:43 UTC
Note for QA

QA Tools gives this warning:
 FAIL   t/Koha/Plugin/MarcFieldValues.pm
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   FAIL   pod coverage
                POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
   SKIP   spelling
   OK     valid

This is a false negative. Since the t folder is not in the PERL5LIB path, the module cannot be loaded.

After adding [myclone]/t to PERL5LIB:
perl -MPod::Coverage=Koha::Plugin::MarcFieldValues -e1
RETURNS:
Koha::Plugin::MarcFieldValues has a Pod::Coverage rating of 1
Comment 12 Katrin Fischer 2017-08-10 11:51:12 UTC
Created attachment 65802 [details] [review]
[SIGNED OFF] Bug 19049: Fix regression on stage-marc-import with to_marc plugin

Bug 12412 added the use of to_marc plugins allowing arbitrary file formats
in stage-marc-import (as long as the plugins can handle them). The feature
was not very visible in the code, and when bug 10407 added the marcxml
format, it made some changes that broke the use of to_marc.

This patch restores the functionality by:
[1] Adding a sub RecordsFromMarcPlugin to ImportBatch.pm, specifically
    addressing the conversion from arbitrary formats to MARC::Record.
    The original to_marc interface is used: pass it the file contents,
    and it returns a string consisting of a number of MARC blobs separated
    by \x1D.
    Consequently, the call of to_marc is removed from routine
    BatchStageMarcRecords where it did not belong. The to_marc_plugin
    parameter is removed and two calls are adjusted accordingly.
[2] Instead of a separate combo with plugins, the format combo contains
    MARC, MARCXML and optionally some plugin formats.
[3] The code in stage-marc-import.pl now clearly shows the three main
    format types: MARC, MARCXML or plugin based.

Note: This patch restores more or less the situation after bug 12412, but
I would actually recommend to have the to_marc plugins return MARC::Record
objects instead of large text strings. In the second example I added a
to_marc plugin that actually converts MARC record objects to string format,
while RecordsFromMarcPlugin reconverts them to MARC::Records.

Test plan:
See second patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2017-08-10 11:51:16 UTC
Created attachment 65803 [details] [review]
[SIGNED OFF] Bug 19049: Testing RecordsFromMarcPlugin with a to_marc plugin

This patch adds a simple to_marc plugin in t/Koha/Plugin that is used
in the added subtest in ImportBatch.t.

Test plan:
[1] Run t/db_dependent/ImportBatch.t
[2] Copy the to_marc test plugin from t to your plugin directory.
    Under Debian packages, you should do something like:
    mkdir -p /var/lib/koha/master/plugins/Koha/Plugin/
    cp [yourclone]/t/Koha/Plugin/MarcFieldValues.pm /var/lib/koha/master/plugins/Koha/Plugin/
[3] Check if you see this plugin on plugins/plugins-home.pl
[4] Create a text file with some fields like:
    (Note: The plugin needs an empty line between both "records".)
    100,a = Test Author 1
    245,a = Title One

    100,a = Author 2
    245,a = Title Two
[5] Go to stage-marc-import.pl. Upload the created file. Select the plugin
    in the format combo and proceed. Did you create two records ?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2017-08-10 11:51:19 UTC
Created attachment 65804 [details] [review]
[SIGNED OFF] Bug 19049: [Follow-up] Make sure that GetPlugins finds the plugin

We have to mock_config the pluginsdir before Plugins is loaded, and
we should pass an absolute path (not a relative one).
If you did not install the test to_marc plugin, this test would fail.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Kyle M Hall 2017-08-10 12:57:52 UTC
Created attachment 65807 [details] [review]
Bug 19049: Fix regression on stage-marc-import with to_marc plugin

Bug 12412 added the use of to_marc plugins allowing arbitrary file formats
in stage-marc-import (as long as the plugins can handle them). The feature
was not very visible in the code, and when bug 10407 added the marcxml
format, it made some changes that broke the use of to_marc.

This patch restores the functionality by:
[1] Adding a sub RecordsFromMarcPlugin to ImportBatch.pm, specifically
    addressing the conversion from arbitrary formats to MARC::Record.
    The original to_marc interface is used: pass it the file contents,
    and it returns a string consisting of a number of MARC blobs separated
    by \x1D.
    Consequently, the call of to_marc is removed from routine
    BatchStageMarcRecords where it did not belong. The to_marc_plugin
    parameter is removed and two calls are adjusted accordingly.
[2] Instead of a separate combo with plugins, the format combo contains
    MARC, MARCXML and optionally some plugin formats.
[3] The code in stage-marc-import.pl now clearly shows the three main
    format types: MARC, MARCXML or plugin based.

Note: This patch restores more or less the situation after bug 12412, but
I would actually recommend to have the to_marc plugins return MARC::Record
objects instead of large text strings. In the second example I added a
to_marc plugin that actually converts MARC record objects to string format,
while RecordsFromMarcPlugin reconverts them to MARC::Records.

Test plan:
See second patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2017-08-10 12:57:59 UTC
Created attachment 65808 [details] [review]
Bug 19049: Testing RecordsFromMarcPlugin with a to_marc plugin

This patch adds a simple to_marc plugin in t/Koha/Plugin that is used
in the added subtest in ImportBatch.t.

Test plan:
[1] Run t/db_dependent/ImportBatch.t
[2] Copy the to_marc test plugin from t to your plugin directory.
    Under Debian packages, you should do something like:
    mkdir -p /var/lib/koha/master/plugins/Koha/Plugin/
    cp [yourclone]/t/Koha/Plugin/MarcFieldValues.pm /var/lib/koha/master/plugins/Koha/Plugin/
[3] Check if you see this plugin on plugins/plugins-home.pl
[4] Create a text file with some fields like:
    (Note: The plugin needs an empty line between both "records".)
    100,a = Test Author 1
    245,a = Title One

    100,a = Author 2
    245,a = Title Two
[5] Go to stage-marc-import.pl. Upload the created file. Select the plugin
    in the format combo and proceed. Did you create two records ?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2017-08-10 12:58:02 UTC
Created attachment 65809 [details] [review]
Bug 19049: [Follow-up] Make sure that GetPlugins finds the plugin

We have to mock_config the pluginsdir before Plugins is loaded, and
we should pass an absolute path (not a relative one).
If you did not install the test to_marc plugin, this test would fail.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Verified patch is compatible with original KitchenSink to_marc plugin
Comment 18 Kyle M Hall 2017-08-10 13:03:22 UTC
Created attachment 65810 [details] [review]
Bug 19049 [QA Followup] - Make plugin name first item in description
Comment 19 Jonathan Druart 2017-08-10 15:55:28 UTC
Marcel,

-my $format                     = $input->param('format') || 'ISO2709';
+my $format                     = $input->param('format');

Then later:
+    } else {
+        die "No format specified";
+    }

I would prefer to avoid the die and default to "MARC" if format is not passed, especially if we backport this change.
Comment 20 Jonathan Druart 2017-08-10 15:59:19 UTC
t/db_dependent/ImportBatch.t .. 1/14     # No tests run!

#   Failed test 'No tests run for subtest "RecordsFromMarcPlugin"'
#   at t/db_dependent/ImportBatch.t line 199.
Can't call method "GetPlugins" on an undefined value at t/db_dependent/ImportBatch.t line 190.


Must skip tests if enable_plugins is not set in koha-conf.xml or pass it in arg.
Comment 21 Tomás Cohen Arazi 2017-08-10 18:44:49 UTC
(In reply to Jonathan Druart from comment #20)
> t/db_dependent/ImportBatch.t .. 1/14     # No tests run!
> 
> #   Failed test 'No tests run for subtest "RecordsFromMarcPlugin"'
> #   at t/db_dependent/ImportBatch.t line 199.
> Can't call method "GetPlugins" on an undefined value at
> t/db_dependent/ImportBatch.t line 190.
> 
> 
> Must skip tests if enable_plugins is not set in koha-conf.xml or pass it in
> arg.

It has to be mocked IMHO.
Comment 22 Marcel de Rooy 2017-08-11 06:42:45 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2017-08-11 06:44:44 UTC
(In reply to Jonathan Druart from comment #19)
> I would prefer to avoid the die and default to "MARC" if format is not
> passed, especially if we backport this change.

Done.

(In reply to Jonathan Druart from comment #20)
> Must skip tests if enable_plugins is not set in koha-conf.xml or pass it in
> arg.

Fixed. Thanks!

(In reply to Tomás Cohen Arazi from comment #21)
> It has to be mocked IMHO.

YHO is always appreciated.
Comment 24 Marcel de Rooy 2017-08-11 06:57:15 UTC
Created attachment 65877 [details] [review]
Bug 19049: [QA Follow-up] Mock config, default format

As requested by QA:
[1] Mock_config enable_plugins in the test.
[2] Fallback to MARC when format is empty. Remove die statement.
Added:
[3] Remove $marc. This variable got obsolete during development.
[4] Add test on $input_file and $plugin_class. Test $text before calling
    Handler or processing $text. No need to split undef if somehow Handler
    returned undef, etc. If the routine returns an empty arrayref,
    stage-marc-import will do fine.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Katrin Fischer 2017-08-12 10:16:40 UTC
Marcel commented on this error from the qa script here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19049#c11
Comment 26 Katrin Fischer 2017-08-12 14:48:01 UTC
Wrong bug!
Comment 27 Jonathan Druart 2017-08-15 15:28:03 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 28 Marcel de Rooy 2017-08-22 14:23:38 UTC
This causes a critical bug. See fix on bug 19163
Comment 29 Fridolin Somers 2017-09-06 13:31:08 UTC
Pushed to 17.05.x, will be in 17.05.04
Comment 30 Katrin Fischer 2017-09-10 09:01:58 UTC
These patches have been pushed to 16.11.x and will be in 16.11.12.

Fridolin, please take a look at the dependent bug 19163 - it fixes a critical problem with this patch set!
Comment 31 Mason James 2017-09-20 00:06:13 UTC
Blocked by Enhancement (BZ 10407), skipping for 16.05.x