Needed since 10407 broke this undocumented feature. Interface and code were not clear enough.
Will be submitting something soon..
Created attachment 65595 [details] [review] Bug 19049: Restore import with to_marc plugin Still needs some polishing and testing.
Created attachment 65655 [details] [review] Bug 19049: Restore import with to_marc plugin Still needs some polishing and testing.
Created attachment 65656 [details] [review] Bug 19049: Tests
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>
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>
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.
Increasing Severity
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.
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>
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
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>
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>
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>
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>
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>
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
Created attachment 65810 [details] [review] Bug 19049 [QA Followup] - Make plugin name first item in description
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.
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.
(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.
Created attachment 65876 [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 if to the Handler call and add if around processing $text. No need to split undef if somehow plugin was not found, 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>
(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.
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>
Marcel commented on this error from the qa script here: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19049#c11
Wrong bug!
Pushed to master for 17.11, thanks to everybody involved!
This causes a critical bug. See fix on bug 19163
Pushed to 17.05.x, will be in 17.05.04
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!
Blocked by Enhancement (BZ 10407), skipping for 16.05.x