Bug 10407 - Allow MARCXML records to be imported with Tools/Stage MARC records for import
Summary: Allow MARCXML records to be imported with Tools/Stage MARC records for import
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic record staging/import (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 18772 16976 19049
  Show dependency treegraph
 
Reported: 2013-06-04 18:46 UTC by Mason James
Modified: 2018-06-04 20:10 UTC (History)
12 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 10407 - allow MARCXML records to be imported via Koha's GUI (8.98 KB, patch)
2015-04-17 17:13 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 10407 - allow MARCXML records to be imported via Koha's GUI (9.29 KB, patch)
2015-04-19 08:20 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 10407 - allow MARCXML records to be imported via Koha's GUI (8.99 KB, patch)
2015-04-19 12:48 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 10407 - allow MARCXML records to be imported via Koha's GUI (9.36 KB, patch)
2015-04-19 15:37 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 10407 - allow MARCXML records to be imported via Koha's GUI - Catch deadly Record-level errors. (1.89 KB, patch)
2015-09-24 06:01 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 10407: Allow MARCXML records to be imported via GUI (9.25 KB, patch)
2016-07-07 13:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10407: Add marcxml import to stage-marc-import script (4.49 KB, patch)
2016-07-07 13:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10407: Allow MARCXML records to be imported via GUI (groundwork) (9.38 KB, patch)
2016-07-08 11:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10407: Resolve warnings from child process (1.28 KB, patch)
2016-07-08 11:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10407: Add marcxml import (follow-up) (9.29 KB, patch)
2016-07-08 11:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10407: Add two subtests to t/ImportBatch.t (3.77 KB, patch)
2016-07-08 11:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10407: Add two subtests to t/ImportBatch.t (3.74 KB, patch)
2016-07-08 12:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10407: [QA Follow-up] Hashref process_batch params (5.50 KB, patch)
2016-07-12 13:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10407: Allow MARCXML records to be imported via GUI (groundwork) (9.39 KB, patch)
2016-07-28 10:11 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 10407: Resolve warnings from child process (1.29 KB, patch)
2016-07-28 10:11 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 10407: Add marcxml import (follow-up) (9.30 KB, patch)
2016-07-28 10:11 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 10407: Add two subtests to t/ImportBatch.t (3.75 KB, patch)
2016-07-28 10:12 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 10407: [QA Follow-up] Hashref process_batch params (5.51 KB, patch)
2016-07-28 10:12 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 10407: Allow MARCXML records to be imported via GUI (groundwork) (9.46 KB, patch)
2016-08-03 12:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10407: Resolve warnings from child process (1.36 KB, patch)
2016-08-03 12:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10407: Add marcxml import (follow-up) (9.37 KB, patch)
2016-08-03 12:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10407: Add two subtests to t/ImportBatch.t (3.83 KB, patch)
2016-08-03 12:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10407: [QA Follow-up] Hashref process_batch params (5.58 KB, patch)
2016-08-03 12:24 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mason James 2013-06-04 18:46:44 UTC
currently, you can only import MARCXML records into Koha using the ./bulkmarcimport.pl tool, from the command-line


this bug will attempt to add that functionality to Koha's GUI
Comment 1 Mathieu Saby 2013-06-04 23:06:19 UTC
I agree it is a needed feature.
For example, if librarians want to use a tool like Zotero to gather bibliographic references on the web, and import the result into Koha for creating orders, presently, they need to use an extra-koha tool for converting marcxml records into iso2709 records.

M. Saby
Comment 2 Pierre Angot 2013-06-05 04:50:56 UTC
+1 !
It's really a good idea !
Comment 3 Katrin Fischer 2014-11-02 10:55:34 UTC
Still an issue.
Comment 4 Olli-Antti Kivilahti 2015-04-17 17:13:29 UTC Comment hidden (obsolete)
Comment 5 Olli-Antti Kivilahti 2015-04-19 08:20:50 UTC Comment hidden (obsolete)
Comment 6 Olli-Antti Kivilahti 2015-04-19 12:48:32 UTC Comment hidden (obsolete)
Comment 7 Olli-Antti Kivilahti 2015-04-19 15:37:27 UTC Comment hidden (obsolete)
Comment 8 Mathieu Saby 2015-04-20 09:02:21 UTC
Hello
Please add a description of your patch in the commit message and a test plan, and change the status to "Need Sign off"


Mathieu
Comment 9 Mathieu Saby 2015-04-20 09:08:32 UTC
If it can help: http://wiki.koha-community.org/wiki/Commit_messages ; http://wiki.koha-community.org/wiki/SubmitingAPatch

As nobody tested the 1st version of your patch, I suppose you can delete "Added check to warn if ISO2709-Records' encoding is different than the expected one." in your commit message.

Also, if you did not did that, you can send your patch to koha-patch list (koha-patches@lists.koha-community.org.) so that other dev be aware of your proposal.

BTW, I am no more involved in Koha, so I won't test it...

Mathieu
Comment 10 Julian Maurice 2015-07-10 12:03:56 UTC
Is this bug still relevant with bug 12412 ?
Comment 11 Tomás Cohen Arazi 2015-07-10 12:15:13 UTC
(In reply to Julian Maurice from comment #10)
> Is this bug still relevant with bug 12412 ?

It is still relevant.
Comment 12 Olli-Antti Kivilahti 2015-09-24 06:01:47 UTC Comment hidden (obsolete)
Comment 13 Mirko Tietgen 2015-11-27 10:35:03 UTC
Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 10407 - allow MARCXML records to be imported via Koha's GUI
fatal: sha1 information is lacking or useless (C4/ImportBatch.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 10407 - allow MARCXML records to be imported via Koha's GUI
Comment 14 Marcel de Rooy 2016-07-07 13:23:57 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2016-07-07 13:24:02 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2016-07-07 13:24:23 UTC
Still working on it.
Comment 17 Marcel de Rooy 2016-07-08 11:40:11 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2016-07-08 11:40:16 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2016-07-08 11:40:20 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2016-07-08 11:40:26 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2016-07-08 11:44:13 UTC
Welcomes a signoff now :)
Will open a new report to see if these changes are interesting for bulkmarcimport too.
Comment 22 Marcel de Rooy 2016-07-08 12:09:09 UTC Comment hidden (obsolete)
Comment 23 Mark Tompsett 2016-07-12 01:01:17 UTC
Comment on attachment 53218 [details] [review]
Bug 10407: Allow MARCXML records to be imported via GUI (groundwork)

Review of attachment 53218 [details] [review]:
-----------------------------------------------------------------

::: C4/ImportBatch.pm
@@ -413,4 @@
>      my $num_items = 0;
>      # FIXME - for now, we're dealing only with bibs
>      my $rec_num = 0;
> -    foreach my $marc_blob (split(/\x1D/, $marc_records)) {

How confusing coding the break in HEX here, but OCT elsewhere.

::: misc/stage_file.pl
@@ +85,4 @@
>  
>  my $dbh = C4::Context->dbh;
>  $dbh->{AutoCommit} = 0;
> +process_batch($format, $input_file, $record_type, $match, $add_items, $batch_comment);

Could we hashref this, please?
Comment 24 Marcel de Rooy 2016-07-12 12:24:49 UTC
(In reply to M. Tompsett from comment #23)
> > -    foreach my $marc_blob (split(/\x1D/, $marc_records)) {
> 
> How confusing coding the break in HEX here, but OCT elsewhere.

Yes, it is :) Note that this patch now removes the \x1D from current code and moves the 035 from even three places into one place. So confusion--.
Personally, I prefer hex above octal btw. Do you want me to change that?
Comment 25 Marcel de Rooy 2016-07-12 13:29:55 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2016-07-12 13:31:56 UTC
(In reply to M. Tompsett from comment #23)
> ::: misc/stage_file.pl
> @@ +85,4 @@
> >  
> >  my $dbh = C4::Context->dbh;
> >  $dbh->{AutoCommit} = 0;
> > +process_batch($format, $input_file, $record_type, $match, $add_items, $batch_comment);
> 
> Could we hashref this, please?

We should not per se hashref to hashref imo. This is just a local sub of one script. But seeing the mix of parameters and file level vars, I decided to do it after all. Hopefully a slight improvement of the code.
Comment 27 Mark Tompsett 2016-07-18 02:59:35 UTC
(In reply to Marcel de Rooy from comment #26)
> (In reply to M. Tompsett from comment #23)
> > ::: misc/stage_file.pl
> > @@ +85,4 @@
> > >  
> > >  my $dbh = C4::Context->dbh;
> > >  $dbh->{AutoCommit} = 0;
> > > +process_batch($format, $input_file, $record_type, $match, $add_items, $batch_comment);
> > 
> > Could we hashref this, please?
> 
> We should not per se hashref to hashref imo. This is just a local sub of one
> script. But seeing the mix of parameters and file level vars, I decided to
> do it after all. Hopefully a slight improvement of the code.

Why did you change everything to the hashref references?
my ($parameters) = @_;
my $format = $parameters->{format};
...
block at the top and all the other code wouldn't need to be changed.
Comment 28 Marcel de Rooy 2016-07-18 06:26:35 UTC
(In reply to M. Tompsett from comment #27)
> Why did you change everything to the hashref references?
> my ($parameters) = @_;
> my $format = $parameters->{format};
> ...
> block at the top and all the other code wouldn't need to be changed.

Only a few vars are referenced more than once or twice. Even without comparison etc.
Comment 29 Tomás Cohen Arazi 2016-07-26 18:56:12 UTC
(In reply to Marcel de Rooy from comment #28)
> (In reply to M. Tompsett from comment #27)
> > Why did you change everything to the hashref references?
> > my ($parameters) = @_;
> > my $format = $parameters->{format};
> > ...
> > block at the top and all the other code wouldn't need to be changed.
> 
> Only a few vars are referenced more than once or twice. Even without
> comparison etc.

What is the status of this one?
Comment 30 Marcel de Rooy 2016-07-27 05:59:52 UTC
(In reply to Tomás Cohen Arazi from comment #29)
> (In reply to Marcel de Rooy from comment #28)
> > (In reply to M. Tompsett from comment #27)
> > > Why did you change everything to the hashref references?
> > > my ($parameters) = @_;
> > > my $format = $parameters->{format};
> > > ...
> > > block at the top and all the other code wouldn't need to be changed.
> > 
> > Only a few vars are referenced more than once or twice. Even without
> > comparison etc.
> 
> What is the status of this one?

Needs Signoff
Comment 31 Josef Moravec 2016-07-28 10:11:31 UTC Comment hidden (obsolete)
Comment 32 Josef Moravec 2016-07-28 10:11:43 UTC Comment hidden (obsolete)
Comment 33 Josef Moravec 2016-07-28 10:11:53 UTC Comment hidden (obsolete)
Comment 34 Josef Moravec 2016-07-28 10:12:02 UTC Comment hidden (obsolete)
Comment 35 Josef Moravec 2016-07-28 10:12:08 UTC Comment hidden (obsolete)
Comment 36 Marcel de Rooy 2016-07-28 10:19:33 UTC
(In reply to Josef Moravec from comment #35)
> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Thx Josef !
Comment 37 Jonathan Druart 2016-08-01 15:06:02 UTC
The import tool does not work for me under plack (master), could you confirm?
Comment 38 Marcel de Rooy 2016-08-02 10:03:31 UTC
(In reply to Jonathan Druart from comment #37)
> The import tool does not work for me under plack (master), could you confirm?

Did you test with plackup.sh ?
There is an essential difference: When you test with koha-plack, the apache include file apache-shared-intranet-plack.conf comes into the picture. It includes a bunch of ProxyPass directives for some problematic scripts, and yes it includes stage-marc-import.pl.

This means that the import tool most probably does not work with plackup.sh too without this patch. (Especially when you repeat the import?)
So your problem here is not the scope of this report.

I retested this patch by importing a marcxml twice with koha-plack enabled, and I confirm that it still works as expected.

Btw I would like to work on resolving these issues making the ProxyPass unneeded, but yes on another report :)

Thx for testing.
Comment 39 Jonathan Druart 2016-08-03 12:24:03 UTC
Created attachment 53937 [details] [review]
Bug 10407: Allow MARCXML records to be imported via GUI (groundwork)

Patch from Olli, manual rebase by Marcel (July 7, 2016).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Needs follow-up. Test plan in the third patch.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 40 Jonathan Druart 2016-08-03 12:24:09 UTC
Created attachment 53938 [details] [review]
Bug 10407: Resolve warnings from child process

After the staging job finishes, the child tries to print results to
STDOUT (which is closed). This creates warnings like:

stage-marc-import.pl: binmode() on closed filehandle STDOUT at C4/Templates.pm line 120.
stage-marc-import.pl: Filehandle STDOUT reopened as FH only for input at /usr/lib/perl5/Template/Provider.pm line 964.

Resolving it is simple: the child should exit after marking the job
as finished.

Test plan:
Import a marc file.
Observe that you do no longer have such warnings.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 41 Jonathan Druart 2016-08-03 12:24:23 UTC
Created attachment 53939 [details] [review]
Bug 10407: Add marcxml import (follow-up)

This patch makes the following changes:

[1] Based on the groundwork of the former patch, add call to
    RecordsFromMARCXMLFile in stage-marc-import. Use format param.
[2] Add format to the template. Use file extension to determine.
    If you use .xml or .marcxml as extension, MARCXML is selected.
[3] In stage-marc-import.tt mark UTF-8 encoding as UTF-8 not as utf8.
[4] BatchStageMarcRecords: do not call plugin if you have no records.
[5] RecordsFromISO2709File: also return errors in an array.
[6] In misc/stage_file.pl also use UTF-8. Handling of errors from [5].

Test plan:
[1] Import an empty file as MARC or MARCXML (with Tools/Stage..import).
[2] Import an non-empty file with invalid contents as MARC or MARCXML.
[3] Export a few records with Tools/Export as MARC and MARCXML.
[4] Import these two files. Check selected format versus file extension.
[5] Import a MARCXML file with misc/stage_file.pl.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 42 Jonathan Druart 2016-08-03 12:24:29 UTC
Created attachment 53940 [details] [review]
Bug 10407: Add two subtests to t/ImportBatch.t

Creates a few simple tests for the two new subroutines in ImportBatch.pm.

Test plan:
Run t/ImportBatch.t.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 43 Jonathan Druart 2016-08-03 12:24:38 UTC
Created attachment 53941 [details] [review]
Bug 10407: [QA Follow-up] Hashref process_batch params

As requested by Mark Tompsett. Hope this guarantees a signoff now..

Note: For consistency four additional parameters were needed to no longer
use file level vars in this subroutine.

Test plan:
Import a file with stage_file.pl.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Imported a marc file and a marcxml file with stage_file.pl.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 44 Kyle M Hall 2016-09-02 16:59:49 UTC
Pushed to master for 16.11, thanks Marcel, Olli!
Comment 45 Pete Edwards 2017-08-03 10:34:12 UTC
This change has broken my 'to_marc' plugin. Prior to this patch stage-marc-import.pl would read the input file with this routine:
while (<$fh>) {
  s/^\s+//;
  s/\s+$//;
  $marcrecord.=$_;
}
And the marcrecord string would end up getting passed to the to_marc function in my plugin. Perhaps I was not using this as expected but this allowed me to import an Excel (xls) file which my plugin parsed using Spreadsheet::ParseExcel and then read to generate Marc records to be staged.

The changes now cause the file to be read in ImportBatch::RecordsFromISO2709File and Marc records added to an array which is passed to the to_marc function. The reading routine is:
while (<IN>) {
  s/^\s+//;
  s/\s+$//;
  next unless $_; # skip if record has only whitespace, as might occur
                  # if file includes newlines between each MARC record
  my ($marc_record, $charset_guessed, $char_errors) = MarcToUTF8Record($_, $marc_type, $encoding);
  push @marc_records, $marc_record;
  ...
}

So in my case I get an array of bad marc records.
Two questions:
1. Was I doing something I shouldn't have been able to and this fix has not broken anything as sch - just done what it always intended?
2. Whatever the answer to the (1) it does seem to have changed the data being passed to to_marc from scalar to array - surely that could break other people's plugins? 

I have not been able to find much by way of documentation about to_marc plugins - am I missing something?
Comment 46 Pete Edwards 2017-08-03 11:08:04 UTC
I also realised that there has been a breaking change in the required return from the to_marc function. It used to return a scalar but must now be an array.

There are plugins out there that are incompatible with this e.g.:
https://github.com/bywatersolutions/koha-plugin-ris-importer/blob/master/Koha/Plugin/Com/ByWaterSolutions/ImportRIS.pm
Comment 47 Jonathan Druart 2017-08-03 14:06:53 UTC
Hi Pete,
Plugins are not maintained by the Koha community, you should contact the author of the plugins directly.
Comment 48 Tomás Cohen Arazi 2017-08-03 14:08:10 UTC
(In reply to Jonathan Druart from comment #47)
> Hi Pete,
> Plugins are not maintained by the Koha community, you should contact the
> author of the plugins directly.

Jonathan is right, you should file an issue there:
https://github.com/bywatersolutions/koha-plugin-ris-importer/issues
Comment 49 Pete Edwards 2017-08-03 14:35:41 UTC
Thanks for the responses but I don't think you quite understood my point. I only gave the specific instance as an example of someone else who would be affected by this patch but I don't use that one.

I have written a plugin myself that is now broken because of this change. How does a plugin author find out when you may a change to koha that breaks plugins? I am saying that both the internal assumptions made in this patch and the types of input and output parameters have changed so all plugins using the to_marc method will have broken. I didn't see anything in the release notes notifying people that there was an API change.
Comment 50 Tomás Cohen Arazi 2017-08-03 14:41:21 UTC
(In reply to Pete Edwards from comment #49)
> Thanks for the responses but I don't think you quite understood my point. I
> only gave the specific instance as an example of someone else who would be
> affected by this patch but I don't use that one.
> 
> I have written a plugin myself that is now broken because of this change.
> How does a plugin author find out when you may a change to koha that breaks
> plugins? I am saying that both the internal assumptions made in this patch
> and the types of input and output parameters have changed so all plugins
> using the to_marc method will have broken. I didn't see anything in the
> release notes notifying people that there was an API change.

So this is a development/comunication workflow question. Please raise this on the koha-devel mailing list instead. If you have suggestions, then it can be formally proposed to be discussed on a IRC dev meeting.
Comment 51 Pete Edwards 2017-08-03 14:57:05 UTC
OK - I see the point about most of my question and will raise it on devel. But, isn't this the correct place to point out when a patch causes a regression problem. It is breaking an existing API.
Comment 52 Tomás Cohen Arazi 2017-08-03 15:01:41 UTC
(In reply to Pete Edwards from comment #51)
> OK - I see the point about most of my question and will raise it on devel.
> But, isn't this the correct place to point out when a patch causes a
> regression problem. It is breaking an existing API.

There are a couple options for you on this issue:
- File a bug explaining the regression and requesting it to be fixed. You need to set the 'depends on' field to this bug number. Beware that others migt consider the change is ok and close the bug as WORKSFORME. API changes are not usually considered a bug, but an enhancement. But this is just a matter of discussing (dis)advantages and reaching a middle ground. If there's a regression it is always a good practice to provide a regression test patch.
- Comment on koha-devel the 'general' situation, in which there's an API change that should be better explained on the release notes. Which I would agree with BTW.

Looking forward to this discussion!
Comment 53 Jonathan Druart 2017-08-03 15:07:17 UTC
(In reply to Pete Edwards from comment #51)
> OK - I see the point about most of my question and will raise it on devel.
> But, isn't this the correct place to point out when a patch causes a
> regression problem. It is breaking an existing API.

The codebase is moving very fast, the legacy code is removed and replaced with oop. Since the community is not maintaining the plugins and some are not publicly released, it is hard to know what changes will break something.
I guess the answer would be to suggest plugin's authors to write tests in order to catch these regressions and then to provide a new version of the plugin.
Another answer would be to contribute to the codebase instead of writing plugins ;)
Comment 54 Marcel de Rooy 2017-08-07 07:59:19 UTC
(In reply to Pete Edwards from comment #45)
Hi Pete,

> 1. Was I doing something I shouldn't have been able to and this fix has not
> broken anything as sch - just done what it always intended?

Until we added the marcxml input format, the codebase only allowed you to import marc records. If you imported excel records and it worked with your plugin, you were just lucky. This was not intended to work at all. But it could certainly be an interesting third input format for the codebase !

> 2. Whatever the answer to the (1) it does seem to have changed the data
> being passed to to_marc from scalar to array - surely that could break other
> people's plugins? 

Since a plugin is not part of the codebase, the author should tell its users for which Koha version it is intended. It is not guaranteed to work with higher versions of Koha.

Yes, behavior changed here. Before a string was sent consisting of multiple marc records separated by linefeed (no excel expected here). Now an array is passed with MARC::Record objects. I agree that this was not [clearly] communicated. If possible, such interface changes should be avoided.
Comment 55 Katrin Fischer 2017-08-07 09:01:25 UTC
Hm, I don't agree with saying that plugins are not part of the codebase and people are 'lucky' if they work. We have introduced the plugin system as an official part of Koha and the hooks that we have added, should be maintained. If something needs to be changed, we should note that in the release notes or even should try to keep backwards compatibility when it's possible.

We highlighted creating plugins to convert arbitrary files as a new feature in the release notes of 3.20:

https://koha-community.org/koha-3-20-0-released/
 Add ability for plugins to convert arbitrary files to MARC from record staging tool (bug 12412)
Comment 56 Marcel de Rooy 2017-08-07 10:53:47 UTC
(In reply to Katrin Fischer from comment #55)
> Hm, I don't agree with saying that plugins are not part of the codebase and
> people are 'lucky' if they work. 
Hmm, this is definitely not what I said!
Specific plugins are simply not part of the codebase. Just a fact.
I was not aware of the statement in the 3.20 release notes btw. Without that statement you were lucky if you got excel files into Koha. Do not take these words out of their context please.

> We highlighted creating plugins to convert arbitrary files as a new feature
> in the release notes of 3.20:
> 
> https://koha-community.org/koha-3-20-0-released/
>  Add ability for plugins to convert arbitrary files to MARC from record
> staging tool (bug 12412)

The conversion from arbitrary files should have been incorporated much more clearly in the interface and the code imo. The format option of the interface does not show Arbitrary only MARC or MARCXML. It is not mentioned in the online Help too. The code passes variable $marcrecords to BatchStageMarcRecords. Inside BatchStageMarcRecords the to_marc plugin is called. Again the same variable name $marcrecords is used. No mention of any arbitrary file type! No comment whatsoever, etc.
So I can imagine that when Olli wrote the initial patches, he was not aware of this. Neither was I when I rescued these patches and got them further.

At this point, we can only open a new report to add e.g. Arbitrary to the formats when we have a plugin, or some other description from the plugin. And call to_marc on these lines before passing them to BatchStageMarcRecords.
See bug 19049.
Comment 57 Pete Edwards 2017-08-07 16:37:27 UTC
I agree with you Marcel, that plugins themselves are not part of the codebase but I think we also agree that the plugin system is. I had forgotten that it was that 3.20 note regarding issue 12412 that had led me to develop the plugin the way I had. I even commented on the issue when I was working it. When I reviewed the code changes this week, I concluded I had been lucky - there is nothing obvious in the code to allow arbitrary file types - the reading routine just allowed a non-MARC record file to drop through to the plugin in a less that obvious way so I can see why you didn't maintain that in this latest update. It highlights the need to have clear in-code documentation that would have explained the intent of the original author. I agree with your suggestion about adding an "Arbitrary" format but that will probably take some time to get through development so for now, I'll move my code to a different plugin that will simply convert to a Marc file and allow my users to download that and then stage it without using any plugin.
Comment 58 Katrin Fischer 2017-08-07 21:45:01 UTC
Hi Marcel, I am sorry, I didn't mean to upset you. I think you are right - this should have been documented in the code base as I think I was the only one to remember the related bug report when seeing this, which explains the comments on the bug report so far. Thx for filing the new bug and trying to undo the regression! 

I think maybe we could add a page in the wiki about existing plugin hooks, what do you think? There is https://wiki.koha-community.org/wiki/Koha_plugins, but it doesn't mention neither import nor EDIFACT plugins.
Comment 59 Tomás Cohen Arazi 2017-08-08 14:06:45 UTC
I think bug 12412 should've added new options to the 'format' dropdown so we don't rely on it magically falling back to some plugin  processing the file, and explicitly choosing it instead.
Comment 60 Katrin Fischer 2017-08-08 14:24:51 UTC
It's probably true, but was missed at the time. Reenabling the feature, it could be done.
Comment 61 Marcel de Rooy 2017-08-08 15:44:50 UTC
(In reply to Tomás Cohen Arazi from comment #59)
> I think bug 12412 should've added new options to the 'format' dropdown so we
> don't rely on it magically falling back to some plugin  processing the file,
> and explicitly choosing it instead.

(In reply to Katrin Fischer from comment #60)
> It's probably true, but was missed at the time. Reenabling the feature, it
> could be done.

Will submit on bug 19049. Putting the plugins into the format combo.
Comment 62 Mason James 2017-09-19 23:35:34 UTC
(In reply to Kyle M Hall from comment #44)
> Pushed to master for 16.11, thanks Marcel, Olli!

Enhancement, skipping for 16.05.x branch