Bug 6681 - Provide a way for removing authority links and holdings on Z39.50 record import
Summary: Provide a way for removing authority links and holdings on Z39.50 record import
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P2 enhancement (vote)
Assignee: simith.doliveira
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-08 15:54 UTC by Frédérick Capovilla
Modified: 2015-06-04 23:32 UTC (History)
13 users (show)

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


Attachments
Remove Koha-specific fields from records imported via Z39.50 (2.71 KB, patch)
2011-08-08 15:54 UTC, Frédérick Capovilla
Details | Diff | Splinter Review
Corrected patch, ported to Koha 3.6 (2.91 KB, patch)
2011-12-23 15:34 UTC, Frédérick Capovilla
Details | Diff | Splinter Review
Corrected patch - Now checks system preferences (3.30 KB, patch)
2012-01-12 17:55 UTC, Frédérick Capovilla
Details | Diff | Splinter Review
Bug 6681: Remove Koha-specific fields from records imported via Z39.50 - Corrected (3.36 KB, patch)
2012-01-13 03:46 UTC, Chris Cormack
Details | Diff | Splinter Review
This patch update the code and fix MARC flavour problem by removing all subfields $9 from the imported record. (3.25 KB, patch)
2014-08-14 17:37 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 6681 - When importing a biblio record via Z39.50, authority links and items also get imported. (4.86 KB, patch)
2014-08-27 12:21 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 6681: introduce XSLT scripts to remove items and links (5.11 KB, patch)
2014-11-03 19:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 6681: introduce XSLT scripts to remove items and links (5.40 KB, patch)
2014-11-10 08:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6681: introduce XSLT scripts to remove items and links (5.52 KB, patch)
2014-11-10 08:53 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 6681: (qa followup) document the existence of the sample files (3.91 KB, patch)
2014-11-10 14:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 6681: (qa followup) document the existence of the sample files (4.03 KB, patch)
2014-11-10 15:09 UTC, Frédéric Demians
Details | Diff | Splinter Review
[PASSED QA] Bug 6681: introduce XSLT scripts to remove items and links (5.59 KB, patch)
2014-11-13 21:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 6681: (qa followup) document the existence of the sample files (4.10 KB, patch)
2014-11-13 21:48 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Frédérick Capovilla 2011-08-08 15:54:15 UTC
When importing biblio records from another Koha instance via Z39.50, the authority links (subfields 9) also get imported.
Because of this, the imported biblio record has its authorities linked with the wrong authids.
Comment 1 Frédérick Capovilla 2011-08-08 15:54:56 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2011-09-02 08:45:04 UTC
GBSD#4
This patch is marked for release 3.2, but 3.2 has reached end-of-life.

It fails to apply on current master:
error: patch failed: C4/Biblio.pm:2490
error: C4/Biblio.pm: patch does not apply

Changing status.
Comment 3 Frédérick Capovilla 2011-10-13 19:01:41 UTC
Comment on attachment 4892 [details] [review]
Remove Koha-specific fields from records imported via Z39.50

A bug was found in this patch :
If a field contains only one subfield and this subfield is $9, the $9 gets deleted, but the field isn't deleted.
This can cause the record to become invalid MARC (empty field).

Patch marked as obsolete.

Keeping the bug open. A new, corrected version of this patch can be sent when it is adapted for Koha version 3.6 .
Comment 4 Frédérick Capovilla 2011-12-23 15:34:08 UTC Comment hidden (obsolete)
Comment 5 Frédérick Capovilla 2011-12-23 15:35:19 UTC
Patch sponsored by the CCSR ( http://www.ccsr.qc.ca )
Comment 6 Chris Cormack 2011-12-31 06:58:02 UTC
Hmm it looks to me like this only does MARC21, and it will try to do it for UNIMARC and NORMARC probably making a mess of the record.

I think it needs to check the syspref for MARC Flavour and do the right thing for each flavour.
Comment 7 Frédérick Capovilla 2012-01-12 17:55:59 UTC Comment hidden (obsolete)
Comment 8 Chris Cormack 2012-01-13 03:46:13 UTC Comment hidden (obsolete)
Comment 9 Chris Cormack 2012-01-13 03:48:23 UTC
Signed off, this now has the added benefit of fixing neworderempty.pl which used to blindly delete 952, wether the record was UNIMARC, NORMARC or MARC21. 

A follow up to add removing the authority id's for UNIMARC and NORMARC would be good. But this introduces no regressions, adds a feature and fixes a bug so should not have to wait for a follow up.
Comment 10 Paul Poulain 2012-02-01 17:03:38 UTC
QA comment
This patch depends on MARC flavour, and unimarc should be added. But there's an other option: make it marc independant, and use
select DISTINCT tagfield FROM marc_subfield_structure WHERE authtypecode<>'';
to get the tags that are "connected" to an authority.

could you update your patch ?

(marking failed QA this one)
Comment 11 Frédérick Capovilla 2013-11-06 18:50:40 UTC
Having never worked with unimarc or normarc records and Z39.50 servers, we are not equipped to easily adapt our patch for it to work with these formats.

Could anyone improve this patch so it becomes flexible enough for inclusion on master?
Comment 12 simith.doliveira 2014-08-14 17:37:37 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2014-08-15 13:30:08 UTC
Comment on attachment 30795 [details] [review]
This patch update the code and fix MARC flavour problem by removing all subfields $9 from the imported record.

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

Starting with a code review here:

1) This patch lacks a test plan and a description of the changes - please add those to the commit message.
2) This patch also doesn't do, what the bug description says. It not only removes $9 subfields, but it also removes items. I am not quite happy with both those features combined into one method. I'd like it a bit better if they were separated - but there might be reason to have both combined?
3) Anyway, we need a unit test for the new routine(s).
4) It's not MARC-agnostic, but only adds code for MARC21 - not for UNIMARC or NORMARC. It would be nice to add code for the other dialects as well or at least file bugs for them.

::: C4/Biblio.pm
@@ +2800,5 @@
> +    }
> +
> +    # Remove all authid links ($9) in the record
> +    if ( $marcflavour eq "MARC21" ) {
> +        my @heading = qw(100 110 111 130 440 600 610 611 630 648 650 651 655 700 710 711 730 800 810 811 830);

I am a bit torn about hardcoding the fields.
One idea I had was to look up the authority linked fields in the frameworks - but you might have a library that doesn't use authorities at all or differently and import from another that does use them - in that case it won't work. So I don't have a better idea currently.
Comment 14 simith.doliveira 2014-08-27 12:21:29 UTC
Hi Katrin,

1) Done.

2) When a record is imported from Z39.50, both authorities and items are specific to the library imported, so should be removed.

3) Done.

4) I sent a wrong patch.
Comment 15 simith.doliveira 2014-08-27 12:21:50 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2014-08-27 12:47:32 UTC
Hi Simith,

(In reply to simith.doliveira from comment #14)
> 2) When a record is imported from Z39.50, both authorities and items are
> specific to the library imported, so should be removed.

The main problem here was that the change was hidden and undocumented - noone would know that the patch will have this consequence without reading the patch code. Much better now :)
Comment 17 Tomás Cohen Arazi 2014-08-27 13:34:25 UTC
I'm not sure about the approach. Comments:

1) We should fix Koha's public server configuration so that it doesn't share information we don't want it to. Maybe applying an XSLT transformation.

2) We still might use Z39.50 targets that give us wrong data in their records (not Koha servers for example), so a way of defining what to remove is needed. This is harder to implement, but it can be achieved Z39.50 server-wise, using YAML or the usual | to separate fields$subfields.

3) This should be an opt-in, people who wants stuff removed should be able to choose to do it, not a forced record munging. So, if you continue with the current approach, please introduce a way to control it.

4) I like that you capture the warnings on the unit tests, and the fact that there are unit tests :-D But we need functional tests, in the sense that the behaviour should be tested. I can help if you need advise on the tests writing.
Comment 18 Tomás Cohen Arazi 2014-08-27 14:07:42 UTC
(In reply to Tomás Cohen Arazi from comment #17)
> 
> 2) We still might use Z39.50 targets that give us wrong data in their
> records (not Koha servers for example), so a way of defining what to remove
> is needed. This is harder to implement, but it can be achieved Z39.50
> server-wise, using YAML or the usual | to separate fields$subfields.

Thinking a bit more about this: i forgot to mention that Marcel's work on bug 6536 will be in master soon. While it might be improved, it provides a framework for using XSLT on z39.50 search and import operations. So this specific bug might reduce to just implementing a good default XSLT for removing Koha's authid's and items on top of bug 6536.
Comment 19 Tomás Cohen Arazi 2014-11-03 19:01:31 UTC Comment hidden (obsolete)
Comment 20 Frédéric Demians 2014-11-09 08:32:35 UTC
It works well. This would be a good thing to have those XSL files in Koha. And at the first place, bug 6536 offers new interesting perspectives. But aren't this functionality (XSL transfo of z3950 search result) and those specific XSL files under-documented?
Comment 21 David Cook 2014-11-09 23:23:08 UTC
(In reply to Frédéric Demians from comment #20)
> It works well. This would be a good thing to have those XSL files in Koha.
> And at the first place, bug 6536 offers new interesting perspectives. But
> aren't this functionality (XSL transfo of z3950 search result) and those
> specific XSL files under-documented?

I'm definitely in favour of Tomas's patch using bug 6536. I think this makes a lot of sense.

I also agree about the XSL files being under-documented. I'll add some suggestions in splinter review.
Comment 22 David Cook 2014-11-09 23:30:27 UTC
Comment on attachment 33177 [details] [review]
Bug 6681: introduce XSLT scripts to remove items and links

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

::: koha-tmpl/intranet-tmpl/prog/en/xslt/Del952.xsl
@@ +1,5 @@
> +<xsl:stylesheet
> +  xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
> +  xmlns:marc="http://www.loc.gov/MARC21/slim"
> +  version="1.0">
> +

<!-- This XSLT is intended for use when importing records via Z39.50 -->

@@ +4,5 @@
> +  version="1.0">
> +
> +  <xsl:output method="xml" indent="yes" encoding="UTF-8"/>
> +  <xsl:strip-space elements="*"/>
> +

This template matches datafields with the 952 tag. As it provides no specific instructions for how to output this input, the datafield will be deleted from the output.

@@ +6,5 @@
> +  <xsl:output method="xml" indent="yes" encoding="UTF-8"/>
> +  <xsl:strip-space elements="*"/>
> +
> +  <xsl:template match="marc:datafield[@tag=952]"/>
> +

The following template is an "Identity Transform" which will copy all attributes and nodes from the input to the output. Without this template, the output will just be a text document with no XML tags.

::: koha-tmpl/intranet-tmpl/prog/en/xslt/Del995.xsl
@@ +1,5 @@
> +<xsl:stylesheet
> +  xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
> +  xmlns:marc="http://www.loc.gov/MARC21/slim"
> +  version="1.0">
> +

<!-- This XSLT is intended for use when importing records via Z39.50 -->

@@ +4,5 @@
> +  version="1.0">
> +
> +  <xsl:output method="xml" indent="yes" encoding="UTF-8"/>
> +  <xsl:strip-space elements="*"/>
> +

This template matches datafields with the 995 tag. As it provides no specific instructions for how to output this input, the datafield will be deleted from the output.

@@ +6,5 @@
> +  <xsl:output method="xml" indent="yes" encoding="UTF-8"/>
> +  <xsl:strip-space elements="*"/>
> +
> +  <xsl:template match="marc:datafield[@tag=995]"/>
> +

The following template is an "Identity Transform" which will copy all attributes and nodes from the input to the output. Without this template, the output will just be a text document with no XML tags.

::: koha-tmpl/intranet-tmpl/prog/en/xslt/Del9LinksExcept952.xsl
@@ +1,4 @@
> +<xsl:stylesheet
> +  xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
> +  xmlns:marc="http://www.loc.gov/MARC21/slim"
> +  version="1.0">

<!-- This XSLT is intended for use when importing records via Z39.50 -->

@@ +4,5 @@
> +  version="1.0">
> +
> +  <xsl:output method="xml" indent="yes" encoding="UTF-8"/>
> +  <xsl:strip-space elements="*"/>
> +

The following template is an "Identity Transform" which will copy all attributes and nodes from the input to the output. Without this template, the output will just be a text document with no XML tags.

@@ +10,5 @@
> +    <xsl:copy>
> +      <xsl:apply-templates select="@* | node()"/>
> +    </xsl:copy>
> +  </xsl:template>
> +

The following template will match every datafield which contains a subfield with a code of 9, except those datafields that have a tag of 952. Since it has no output instructions, this datafield will be deleted from the output.

It's important to delete the subfield with a code of 9, since this subfield contains links to authority records, which will most likely not link correctly in Koha when importing from Z39.50.

::: koha-tmpl/intranet-tmpl/prog/en/xslt/Del9LinksExcept995.xsl
@@ +1,5 @@
> +<xsl:stylesheet
> +  xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
> +  xmlns:marc="http://www.loc.gov/MARC21/slim"
> +  version="1.0">
> +

<!-- This XSLT is intended for use when importing records via Z39.50 -->

@@ +4,5 @@
> +  version="1.0">
> +
> +  <xsl:output method="xml" indent="yes" encoding="UTF-8"/>
> +  <xsl:strip-space elements="*"/>
> +

The following template is an "Identity Transform" which will copy all attributes and nodes from the input to the output. Without this template, the output will just be a text document with no XML tags.

@@ +10,5 @@
> +    <xsl:copy>
> +      <xsl:apply-templates select="@* | node()"/>
> +    </xsl:copy>
> +  </xsl:template>
> +

The following template will match every datafield which contains a subfield with a code of 9, except those datafields that have a tag of 995. Since it has no output instructions, this datafield will be deleted from the output.

It's important to delete the subfield with a code of 9, since this subfield contains links to authority records, which will most likely not link correctly in Koha when importing from Z39.50.
Comment 23 Marcel de Rooy 2014-11-10 08:02:26 UTC Comment hidden (obsolete)
Comment 24 Frédéric Demians 2014-11-10 08:53:49 UTC Comment hidden (obsolete)
Comment 25 Tomás Cohen Arazi 2014-11-10 14:20:57 UTC Comment hidden (obsolete)
Comment 26 Frédéric Demians 2014-11-10 15:09:13 UTC Comment hidden (obsolete)
Comment 27 Frédéric Demians 2014-11-10 15:13:34 UTC
For the future, a better way for storing XSL could be designed. In the disorder: - a z3950severs table new field, - a new db table for XSL, - a centralized repository, - a link to a Koha community page (like for SQL report library).
Comment 28 David Cook 2014-11-10 22:32:09 UTC
(In reply to Frédéric Demians from comment #27)
> For the future, a better way for storing XSL could be designed. In the
> disorder: - a z3950severs table new field, - a new db table for XSL, - a
> centralized repository, - a link to a Koha community page (like for SQL
> report library).

I've been thinking about this for a little while as well. I've been working again on an OAI harvester which uses XSLTs to filter out the 952, 999, and $9, and I figure "koha-tmpl\intranet-tmpl\prog\en\xslt" isn't always the best place to put these things.

I'm intrigued by the idea of using a db field or db table. If we allowed uploads, that would make it easier for libraries to add their own XSLTs (for better or worse) without meddling around on the file system. 

Actually, a db table could be interesting. We could have the blob, the filename, an id... and then have a db field in z3950servers (or whatever other server config table) that has a foreign key to the primary key id in the XSLT table. 

I'd kind of like the ability to set defaults as well though... so you could set up XSLT profiles to apply on import (rather than having a string of filenames).  Of course, we'd probably need a linking table then for profiles to files. 

I think that'd be a good way to go about setting up defaults though. You could have a "Default Z3950" profile (and maybe a "Default OAI" table). 

--

Actually, as I work on this OAI client, I start thinking about ways to integrate code and storage of Z3950 and OAI targets. I'm trying to think if there are other protocols we use for importing records as well... and how instead of making more tables, we could make some tables more generic.

I wonder too about using XSLTs when importing records via the batch import, although that would probably slow things down a lot as you'd be going from Iso Marc to Marc Object to MARCXML to XSLT to MARCXML. 

I suppose I like the idea of making storage and code more generalizable...

</ramble>
Comment 29 Katrin Fischer 2014-11-11 09:14:46 UTC
I think as the initial feature is already pushed, having examples for what you can do with it is a good start - but more documentation is of course always better :)
Comment 30 Katrin Fischer 2014-11-13 21:48:28 UTC
Created attachment 33540 [details] [review]
[PASSED QA] Bug 6681: introduce XSLT scripts to remove items and links

This patch adds the following sample files:

- Del952.xsl
- Del995.xsl
- Del9LinksExcept952.xsl
- Del9LinksExcept995.xsl

Thay can be used on the new SRU/Z39.50 XSLT processing feature. At the same
time they can be used to solve this bug.

To test:
- Have an SRU/Z39.50 target configured
- Have a search that returns at least a record with the following
  properties:
  * It contains $9 links
  * It contains items (952 or 995 fields in MARC21/NORMARC and UNIMARC respectively).

MARC21/NORMARC test:
- Do a Z39.50/SRU cataloguing search that retrieves the desired record
- Verify it includes $9 and 952 field(s) by clicking on the MARC link
- Edit your Z39.50 target and add Del952.xsl to the "XSLT File(s) for transforming results:" field
- Re-run the search
=> SUCCESS: the MARC view of the imported record doesn't contain the 952 field
- Edit your Z39.50 target and add Del9LinksExcept952.xsl to the "XSLT File(s) for transforming results:" field
- Re-run the search
=> SUCCESS: the MARC view contains the 952 field (including the $9 subfield), and
            all other $9 subfields have been removed
- Edit your Z39.50 target and add
  Del952.xsl, Del9LinksExcept952.xsl
  to the "XSLT File(s) for transforming results:" field (both, comma separated)
- Re-run the search
=> SUCCESS: the MARC view doesn't contain $9 subfields nor items.

- Repeat for UNIMARC, replacing 952 for 995.
- Sign off :-D

Regards
Tomas

Sponsored-by: Universidad Nacional de Cordoba

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested under MARC21. Works fine. (Additionally verified 995 with xsltproc.)
As noted on Bugzilla, we could still improve documentation but imo no need
to block these example transformations.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>
I can't resist to put my own signature on this patch, confirming it works as
described with z39.50 Unimarc targets.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Katrin Fischer 2014-11-13 21:48:55 UTC
Created attachment 33541 [details] [review]
[PASSED QA] Bug 6681: (qa followup) document the existence of the sample files

As requestedby people testing the patch, I add references to the new
xslt files on the Z39.50/SRU servers help.

Example usages are also provided.

Test:
- Apply the patch
- Go to the help page on the 'Z39.50/SRU Servers' page
=> SUCCESS: Notice there's a section documenting XSLT file(s) usage
            and provides some examples that cover the introduced files.
- Sign off

Thanks
Tomas

Sponsored-by: Universidad Nacional de Cordoba

Signed-off-by: Frederic Demians <f.demians@tamil.fr>
  Help page does shed some light to the XSLT usage. Enough to my taste.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 32 Tomás Cohen Arazi 2014-11-14 12:40:13 UTC
Patches pushed to master.