Bug 7345 - Should be possible to export MARC records without private fields
Summary: Should be possible to export MARC records without private fields
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic data support (show other bugs)
Version: 3.8
Hardware: All All
: P3 enhancement (vote)
Assignee: Jared Camins-Esakov
QA Contact: Marcel de Rooy
URL:
Keywords:
: 3377 (view as bug list)
Depends on:
Blocks: 7392
  Show dependency treegraph
 
Reported: 2011-12-11 00:34 UTC by Jared Camins-Esakov
Modified: 2012-10-25 23:10 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7345: Enable exporting records sans private fields (3.68 KB, patch)
2011-12-11 01:25 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7345: Enable exporting records sans private fields (3.85 KB, patch)
2011-12-11 13:18 UTC, Sébastien Marie
Details | Diff | Splinter Review
Bug 7345: Enable exporting records sans private fields (3.95 KB, patch)
2011-12-13 08:28 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7392: Add system preference to define export options in OPAC (20.66 KB, patch)
2012-01-01 13:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7345: Follow up: Adding the new export option to staff interface (2.52 KB, patch)
2012-01-01 19:35 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7345: Follow up: Adding the new export option to staff interface (2.63 KB, patch)
2012-01-02 22:03 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Revised patch (3.70 KB, patch)
2012-02-05 20:01 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Signed patch (3.77 KB, patch)
2012-02-06 07:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7345: Enable exporting records sans private fields (5.65 KB, patch)
2012-02-10 15:27 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Patch (6.01 KB, patch)
2012-02-13 08:06 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 Jared Camins-Esakov 2011-12-11 00:34:45 UTC
At the moment, if you use the opac-export.pl script to obtain a MARC record programmatically from Koha, it includes all private fields and subfields. There should be an option that allows someone to request a records with all private fields (i.e. 9XX, X9X [excepting 490; great going, LC!], and XX9 and subfield $9) removed.
Comment 1 Jared Camins-Esakov 2011-12-11 01:25:46 UTC Comment hidden (obsolete)
Comment 2 Sébastien Marie 2011-12-11 10:59:51 UTC
I think the patch could have a better design if UNIMARC was handle better: currently, if marcflavour is UNIMARC, "exporting records sans private fields" will result "exporting records with private fields" without notice to users...

The design could be to have a hash with:
 - key $from_marcflavour
 - value a hash for function which are flavour dependent (MARC::Record->new_from_usmarc' / 'is_privatefield')

And if not key, set $error to 'unimplemented'.

This will permit easily extend for other marcflavour...
Comment 3 Jared Camins-Esakov 2011-12-11 12:38:43 UTC
Frère Sébastien,

I don't use UNIMARC, but the patch is supposed to strip private fields from UNIMARC too. At least, I thought that fields containing a 9 were private fields in UNIMARC.

The only difference between MARC21/NORMARC and UNIMARC is supposed to be that field 490 is left in for flavors other than UNIMARC. Can you provide more details on exactly which fields are being left in, so that I can troubleshoot it?

Regards,
Jared
Comment 4 Sébastien Marie 2011-12-11 13:04:36 UTC
(In reply to comment #3)
> Frère Sébastien,
> 
> I don't use UNIMARC, but the patch is supposed to strip private fields from
> UNIMARC too. At least, I thought that fields containing a 9 were private fields
> in UNIMARC.
> 
> The only difference between MARC21/NORMARC and UNIMARC is supposed to be that
> field 490 is left in for flavors other than UNIMARC.
>
> Can you provide more details on exactly which fields are being left in,
> so that I can troubleshoot it?

Sorry, I have too quickly read your code... 
But as you say UNIMARC is supported, I will test it.
Comment 5 Sébastien Marie 2011-12-11 13:18:16 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2011-12-13 08:28:44 UTC Comment hidden (obsolete)
Comment 7 Paul Poulain 2011-12-15 17:28:20 UTC
QA comment: I must be dumb, but: the test plan include some jquery. this is OK for testing. But in real life, how will we control it we export 9 or no ?

marking failed QA until I understand, switch back to signed-off once you've answered
Comment 8 Jared Camins-Esakov 2011-12-15 19:48:32 UTC
Paul,

(In reply to comment #7)
> QA comment: I must be dumb, but: the test plan include some jquery. this is OK
> for testing. But in real life, how will we control it we export 9 or no ?

No, you're not dumb. There is no user interface change. I suggested the jQuery so that people could just copy-and-paste something for testing, rather than needing to modify a URL by hand. I don't think we should change the behavior in the OPAC without a compelling reason, if everyone's been happy with it as it is for over a decade. However, as a web service, opac-export.pl and export.pl really need to be able to provide standard MARC to services like RefWorks (well, actually, RefWorks doesn't support MARC at all, but that's another story).

> marking failed QA until I understand, switch back to signed-off once you've
> answered

Will do.

Regards,
Jared
Comment 9 Paul Poulain 2011-12-16 09:24:33 UTC
(In reply to comment #8)
> Paul,
> 
> (In reply to comment #7)
> > QA comment: I must be dumb, but: the test plan include some jquery. this is OK
> > for testing. But in real life, how will we control it we export 9 or no ?
> 
> No, you're not dumb. There is no user interface change. 
OK, but I still don't see what to do with this patch: if it changes nothing practically, why applying it ?

I was expecting a syspref for example, to decide wether to export or no 9 fields. Or some checkbox on opac-export script, to let user decide.
Or something like that.
Comment 10 Jared Camins-Esakov 2011-12-16 12:39:44 UTC
(In reply to comment #9)
> OK, but I still don't see what to do with this patch: if it changes nothing
> practically, why applying it ?

Because if you don't apply it, people can't use /cgi-bin/koha/opac-export.pl to provide standard-compliant MARC to other programs. This is a programmer's feature, but no less valid for that, in my opinion. I use it with a non-Koha utility script that retrieves records from Koha for further processing and display.

> I was expecting a syspref for example, to decide wether to export or no 9
> fields. Or some checkbox on opac-export script, to let user decide.
> Or something like that.

This would be possible, but I am disinclined to try and build consensus about the proper way to handle this in the user interface. Obviously no one is bothered by the fact that Koha by default offers non-standard-compliant MARC for download. Even with no changes to existing functionality, getting consensus on adding this to Koha is taking a while.
Comment 11 Katrin Fischer 2011-12-16 13:06:35 UTC
Hi Jared and Paul,
perhaps this patch could go in as a first step and we could argue about how to add it to the gui later on? I would like a way to export MARC without local fields as we use some of them and an easy way to strip them out would be good.
I think it could be an option for the staff interface and perhaps a system preference for the OPAC?
Comment 12 Jared Camins-Esakov 2011-12-16 14:51:35 UTC
Katrin,

That makes sense to me. Paul?
Comment 13 Paul Poulain 2011-12-30 09:45:52 UTC
Jared, I just spoke of this entry with cait on IRC.
I mark this bug "failed QA" until there is a practical & easy to use way to activate this feature.
My first thoughts would be:
* add a syspref to define the "local field behaviour": let the user choose to upload them or no / remove them automatically
* depending on the syspref, add a checkbox on the export screen or no
* depending on the syspref and/or the user choice, remove local fields before exporting or no.

PS: the items are also stored in a 9xx field (995 in UNIMARC, 952 in MARC21). I strongly think we must let user export those fields (with an option not to have them ?), otherwise, they will never have items !

PS2: cait told me:
<cait> perhaps I can write a follow up adding the option to the gui
then a few minuts later:
<cait> paul_p: I will try to work on a follow up
Comment 14 Katrin Fischer 2012-01-01 13:27:04 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2012-01-01 19:35:52 UTC Comment hidden (obsolete)
Comment 16 Jared Camins-Esakov 2012-01-02 22:03:56 UTC Comment hidden (obsolete)
Comment 17 Jared Camins-Esakov 2012-01-07 17:09:17 UTC
*** Bug 3377 has been marked as a duplicate of this bug. ***
Comment 18 Marcel de Rooy 2012-01-13 15:52:32 UTC
QA Comment:
This looks VERY promising! Some points to note:

CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Auto-merged installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Resolved them but unfortunately had more remarks ;)

In the pref file I find a template variable:
Use the CSS stylesheet [% opacthemelang %]/css/
This does not work here.

OpacExportOptions: It does not really read comfortable in the Preferences screen. All options exceed the length of the field. Could you extend the field, make it better readable (new line)? [Is no blocker but a design thing..]

MARC standard is promised but not delivered yet (while pending on other report). Does not block this, but you could move it to the report it belongs..

Just a thought: If you add an unexisting format, it now comes in the list and you will have an error message on screen. Would it be interesting to exclude these formats (question with some doubts on my part too)..
Comment 19 Jared Camins-Esakov 2012-01-13 15:59:39 UTC
Marcel,

Is that comment about this bug? There are no preference changes here. Maybe you're looking at bug 7392, which depends on this one?

Jared
Comment 20 Katrin Fischer 2012-01-13 16:07:44 UTC
Hi Marcel,

thx for testing this :)

>In the pref file I find a template variable:
>Use the CSS stylesheet [% opacthemelang %]/css/ This does not work here.

I changed that from the old template syntax to TT - so it probably never worked. Not sure how to make it work? I can take the line change out - but it will still be broken.

>OpacExportOptions: It does not really read comfortable in the Preferences
>screen. All options exceed the length of the field. Could you extend the field,
>make it better readable (new line)? [Is no blocker but a design thing..]

It seems the appearance of the field is determined by the type you give in the pref file. I am not sure there is a better type we can use here. The only other option seems to be a box like we use for OpacUserCSS and others?

>MARC standard is promised but not delivered yet (while pending on other
>report). Does not block this, but you could move it to the report it belongs.

I have marked both bugs as dependent on each other. The only thing keeping 7392 from being pushed is the missing presence in the GUI. So I thought I would look into it and it turned into a new feature, going beyond a simple follow-up.

>Just a thought: If you add an unexisting format, it now comes in the list and
>you will have an error message on screen. Would it be interesting to exclude
>these formats (question with some doubts on my part too)..

I can make them not show up - that's probably the easiest way to solve this.
Comment 21 Marcel de Rooy 2012-01-13 17:59:47 UTC
(In reply to comment #19)
> Marcel,
> 
> Is that comment about this bug? There are no preference changes here. Maybe
> you're looking at bug 7392, which depends on this one?
> 
> Jared

You are right, saved in the wrong report.. About quality ;)
Comment 22 Marcel de Rooy 2012-01-13 18:07:21 UTC
Restoring patch status..
Comment 23 Marcel de Rooy 2012-02-03 09:27:33 UTC
QA Comment:
First, looks very good, but still have a few questions:

1) Will a user know that marcstd means marc without private fields (9XX, X9X and XX9)? I did not realize it rightaway too ;) BTW Does not block this patch!

2) While testing it, I saw that the fields 952 and 999 are still included. Note that these are certainly no marc standard fields, so if any should be excluded, they should. Since your regex tests /9/, I do however not understand why they are still there. Do you? Note that a local field 942 and subfields 9 were correctly removed in my testing. Could you test this too? 

3) I doubt if the encoding is 100% correct. When I opened the exported file in notepad, some characters with umlaut did not come up completely correct. But when copying them somewhere else, they were correct after all. When I commented the call to SetUTFFlag, the results in notepad were correct.
Tested with title: Vorläufer, Schüler, Zeitgenossen.
Please note that GetMarcBiblio is already calling _new_from_xml with a utf8 parameter. 
Also note that this remark also pertains to existing format utf8 in export scripts. (So formally, it could be a report on itself.) 
Maybe Katrin can test it (probably she has the most experience with umlauts ;)

4) Finally, elaborating on point 3: Not blocking this patch, but why would the marc2marc function not just demand a marc object (instead of optionally creating it). Note that the only call now already receives an object, created by GetMarcBiblio. And why not leave the call to as_usmarc to the export script, just as done in the case of format marc8, utf8 ?

Changing status just to reflect need for some testing and answers..
Comment 24 Katrin Fischer 2012-02-03 09:32:55 UTC
Hi Marcel, no idea about most of the points you made, but perhaps 1)
>1) Will a user know that marcstd means marc without private fields (9XX, X9X
>and XX9)? I did not realize it rightaway too ;) BTW Does not block this patch!

I don't think the user is seeing this somewhere? I used the codes for my syspref (bug 7392) but tried to explain in the text of the pref. So perhaps we should change the explanation / name shown in OPAC if it's not clear?
Comment 25 Marcel de Rooy 2012-02-03 09:48:42 UTC
(In reply to comment #24)
> I don't think the user is seeing this somewhere? I used the codes for my
> syspref (bug 7392) but tried to explain in the text of the pref. So perhaps we
> should change the explanation / name shown in OPAC if it's not clear?

Yes, maybe Jared could come up with a "better" description?
Comment 26 Marcel de Rooy 2012-02-03 09:49:31 UTC
Additional observation:
See point 2 of comment23. You only delete ONE field. Method delete_fields does not seem to do what it promises. In my example the 942 was deleted, but the 952 and 999 remained.
If you change the call as follows, all three are gone:
   $marc_record_obj->delete_field($_) for @privatefields;

Somewhere above Paul mentioned the need of keeping items (while we now remove
952s). I would argue that if you want these items, pick the other format. If
you really want marc *standard*, do not pass 952. Or -- in some followup --
convert 952 to real marc, 852 or so ..
Comment 27 Katrin Fischer 2012-02-03 09:54:47 UTC
I think as the item format is Koha specific it will not be useful for patrons, so perhaps the OPAC should not offer this information. Also - will the download include fields like internal notes? Because if it does... this is another problem.
Comment 28 Marcel de Rooy 2012-02-03 09:59:01 UTC
(In reply to comment #27)
> I think as the item format is Koha specific it will not be useful for patrons,
> so perhaps the OPAC should not offer this information. Also - will the download
> include fields like internal notes? Because if it does... this is another
> problem.
You could open up a new report for that. Now you can see those internal notes when you export with MARC/UTF8 option.
Comment 29 Jared Camins-Esakov 2012-02-05 20:01:13 UTC
(In reply to comment #23)
> QA Comment:
> First, looks very good, but still have a few questions:
> 
> 1) Will a user know that marcstd means marc without private fields (9XX, X9X
> and XX9)? I did not realize it rightaway too ;) BTW Does not block this patch!

I still argue that this is primarily a developer-centric patch. It's needed so that we can interoperate with the rest of the library world who do not speak Koha. If you have a better idea for the label, please submit a follow-up patch by all means. I do not have any better ideas. I would probably use jQuery to call it "Raw MARC" because I don't see any use for the existing UTF-8 and MARC-8 exports.

> 2) While testing it, I saw that the fields 952 and 999 are still included. Note
> that these are certainly no marc standard fields, so if any should be excluded,
> they should. Since your regex tests /9/, I do however not understand why they
> are still there. Do you? Note that a local field 942 and subfields 9 were
> correctly removed in my testing. Could you test this too? 

I did not have this problem, but your proposed fix worked, so I will attach the revised patch momentarily.

> 3) I doubt if the encoding is 100% correct. When I opened the exported file in
> notepad, some characters with umlaut did not come up completely correct. But
> when copying them somewhere else, they were correct after all. When I commented
> the call to SetUTFFlag, the results in notepad were correct.
> Tested with title: Vorläufer, Schüler, Zeitgenossen.
> Please note that GetMarcBiblio is already calling _new_from_xml with a utf8
> parameter. 
> Also note that this remark also pertains to existing format utf8 in export
> scripts. (So formally, it could be a report on itself.) 
> Maybe Katrin can test it (probably she has the most experience with umlauts ;)

I think it is better not to fix this here, because I am not an expert in this, and it would be better to have someone who understands the problem fix it in both places than have me try to hack in a fix here.

> 4) Finally, elaborating on point 3: Not blocking this patch, but why would the
> marc2marc function not just demand a marc object (instead of optionally
> creating it). Note that the only call now already receives an object, created
> by GetMarcBiblio. And why not leave the call to as_usmarc to the export script,
> just as done in the case of format marc8, utf8 ?

I was using the existing marc2marc API, which specified that it should return the results of as_usmarc.

> Changing status just to reflect need for some testing and answers..
Comment 30 Jared Camins-Esakov 2012-02-05 20:01:53 UTC Comment hidden (obsolete)
Comment 31 Marcel de Rooy 2012-02-06 07:38:47 UTC Comment hidden (obsolete)
Comment 32 Marcel de Rooy 2012-02-06 07:46:14 UTC
QA Comment:
With reference to comment 23.
Point 1. Name may need a followup. Note BTW that this patch needs 7392 to make the option visible. For testing purposes I manually added a line to opac-results.tt to add this option.

Point 2. Most important one: Solved. Note that this may depend too on the version of MARC::Record. The problem of delete_fields may have been resolved in a newer version.

Point 3. Still needs attention, but does not block this patch. This already occurs in current code (option utf8). 

Point 4. May not have been fully understood. Does not matter to me. Only theory.

Marking it as Passed QA.
Comment 33 Paul Poulain 2012-02-10 15:15:46 UTC
My pre-applypatch says the Signed patch is wrong:

Global symbol "$error" requires explicit package name at catalogue/export.pl line 56.
catalogue/export.pl had compilation errors.

(just tried on uptodate master)

Seems relevant as the patch adds
+            elsif ($format =~ /marcstd/) {
+                C4::Charset::SetUTF8Flag($marc,1);
+                ($error,$marc) = marc2marc($marc, 'marcstd', C4::Context->preference('marcflavour'));
+            }
Comment 34 Paul Poulain 2012-02-10 15:16:25 UTC
oups, wrong status sorry
Comment 35 Jared Camins-Esakov 2012-02-10 15:27:40 UTC Comment hidden (obsolete)
Comment 36 Marcel de Rooy 2012-02-13 08:06:02 UTC
Created attachment 7612 [details] [review]
Patch

Amended Jared's patch to resolve double defition of $error in export.pl
Comment 37 Marcel de Rooy 2012-02-13 08:08:07 UTC
s/defition/definition

Paul: please push this patch in conjunction with 7392. Also at Passed QA.