Bug 18152 - UNIMARC bib records imported with invalid 'a' char in label pos.9
Summary: UNIMARC bib records imported with invalid 'a' char in label pos.9
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic record staging/import (show other bugs)
Version: master
Hardware: All All
: P1 - high major (vote)
Assignee: stephane.delaune
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 18533 18910
  Show dependency treegraph
 
Reported: 2017-02-22 11:07 UTC by stephane.delaune
Modified: 2017-08-16 21:08 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18152 : fix unimarc label in SetMarcUnicodeFlag (1.63 KB, patch)
2017-02-22 11:30 UTC, stephane.delaune
Details | Diff | Splinter Review
to test import (787 bytes, text/plain)
2017-02-22 11:46 UTC, stephane.delaune
Details
marc21-with-a-char-in-label-pos9.mrc (787 bytes, text/plain)
2017-02-22 11:47 UTC, stephane.delaune
Details
marc21-without-a-char-in-label-pos9.mrc (1.96 KB, text/plain)
2017-02-22 11:47 UTC, stephane.delaune
Details
unimarc-without-a-char-in-label-pos9.mrc (1.21 KB, text/plain)
2017-02-22 11:48 UTC, stephane.delaune
Details
Bug 18152 : fix unimarc label in SetMarcUnicodeFlag (1.69 KB, patch)
2017-04-18 01:17 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18152 : fix unimarc label in SetMarcUnicodeFlag (1.70 KB, patch)
2017-05-03 09:06 UTC, stephane.delaune
Details | Diff | Splinter Review
Bug 18152: Add tests (1.89 KB, patch)
2017-05-03 18:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18152: Leader 9 for UNIMARC records must be blank (1.22 KB, patch)
2017-05-03 18:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18152 : fix unimarc label in SetMarcUnicodeFlag (1.77 KB, patch)
2017-05-04 13:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18152: Add tests (2.10 KB, patch)
2017-05-04 13:38 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 stephane.delaune 2017-02-22 11:07:19 UTC
The standard UNIMARC requires than the 9th character (starting from 0) in labels must be blank (while it may be 'a' in marc21)

the problem is that C4::Charset::SetMarcUnicodeFlag (called in particular when we import a record) always add 'a' char in the 9th label'pos whereas it should do it just for MARC21 and NORMARC (not for UNIMARC) :

C4::Charset::SetMarcUnicodeFlag add 'a' char in the 9th label character for MARC21 and NORMARC (it's normal), but just before doing this it call "$marc_record->encoding('UTF-8')" which is a MARC::Record function which, when called with 'UTF-8' parameter, do only one thing : add 'a' char in the 9th label character
Comment 1 stephane.delaune 2017-02-22 11:30:05 UTC
Created attachment 60547 [details] [review]
Bug 18152 : fix unimarc label in SetMarcUnicodeFlag

The standard UNIMARC requires than the 9th character (starting from 0) in
labels must be blank (while it may be 'a' in marc21)

the problem is that C4::Charset::SetMarcUnicodeFlag (called in particular when
we import a record) always add 'a' char in the 9th label'pos whereas it should
do it just for MARC21 and NORMARC (not for UNIMARC) :

C4::Charset::SetMarcUnicodeFlag add 'a' char in the 9th label character for
MARC21 and NORMARC (it's normal), but just before doing this it call
"$marc_record->encoding('UTF-8')" which is a MARC::Record function which, when
called with 'UTF-8' parameter, do only one thing : add 'a' char in the 9th
label character

This patch only removes this incorrect function call, so, when we import a bib
record in UNIMARC : it no longer adds erroneous character (this does not change
anything for MARC21 and NORMARC because SetMarcUnicodeFlag explicitly adds 'a'
char in the 9th label for them)
Comment 2 stephane.delaune 2017-02-22 11:46:18 UTC
Created attachment 60548 [details]
to test import
Comment 3 stephane.delaune 2017-02-22 11:47:28 UTC
Created attachment 60549 [details]
marc21-with-a-char-in-label-pos9.mrc
Comment 4 stephane.delaune 2017-02-22 11:47:50 UTC
Created attachment 60550 [details]
marc21-without-a-char-in-label-pos9.mrc
Comment 5 stephane.delaune 2017-02-22 11:48:11 UTC
Created attachment 60551 [details]
unimarc-without-a-char-in-label-pos9.mrc
Comment 6 stephane.delaune 2017-02-22 11:48:41 UTC
to test : 

before applying the patch : 
- change marcflavour syspref in UNIMARC
- import unimarc-without-a-char-in-label-pos9.mrc in koha (see below how to): a 'a' char will be add in label pos.9 (it's the bug)
- change marcflavour syspref in MARC21
- import marc21-with-a-char-in-label-pos9.mrc (record already have a 'a' char in label pos.9)in koha : nothing is change, label keep 'a' char in label pos.9 (it's correct for MARC21 records)
- import marc21-without-a-char-in-label-pos9.mrc in koha : a 'a' char will be add in label pos.9 (it's correct for MARC21 records)
- apply patch
- change marcflavour syspref in UNIMARC
- import unimarc-without-a-char-in-label-pos9.mrc in koha (see below how to): label pos.9 is blank (bug fixed \o/)
- change marcflavour syspref in MARC21
- import marc21-with-a-char-in-label-pos9.mrc (record already have a 'a' char in label pos.9)in koha : nothing is change, label keep 'a' char in label pos.9 (it's correct for MARC21 records, It's like before the patch)
- import marc21-without-a-char-in-label-pos9.mrc in koha : a 'a' char will be add in label pos.9 (like before the patch, it's correct for MARC21 records)

Note : how to import a record in Koha :
Home › Tools › Stage MARC records for import, find file, click on "upload",verify character encoding is UTF-8, click on "stage for import" ; Home › Tools › Manage staged MARC records : choose your file and clic on "import" button ; then you can click below on file name to see it (and note (or not) the presence of char 'a' in label pos.9)
Comment 7 Jonathan Druart 2017-02-27 15:35:40 UTC
Hello Stéphane,
Maybe a dumb question: what side-effects this bug can cause?
Comment 8 Paul Poulain 2017-02-27 16:36:12 UTC
Hi Jonathan,

In fact... I don't know. That's invalid and being invalid is evil. 
more info : We're using the exporter to send the iso2709 file to another (proprietary) ILS, and the file is rejected, as it is invalid.

"La loi est dure mais c'est la loi" ;-)
Comment 9 Jonathan Druart 2017-02-28 08:21:31 UTC
Ok! So I let you find a signoffer at the hackfest :)
Comment 10 Alex Buckley 2017-04-18 01:17:12 UTC
Created attachment 62246 [details] [review]
Bug 18152 : fix unimarc label in SetMarcUnicodeFlag

The standard UNIMARC requires than the 9th character (starting from 0) in
labels must be blank (while it may be 'a' in marc21)

the problem is that C4::Charset::SetMarcUnicodeFlag (called in particular when
we import a record) always add 'a' char in the 9th label'pos whereas it should
do it just for MARC21 and NORMARC (not for UNIMARC) :

C4::Charset::SetMarcUnicodeFlag add 'a' char in the 9th label character for
MARC21 and NORMARC (it's normal), but just before doing this it call
"$marc_record->encoding('UTF-8')" which is a MARC::Record function which, when
called with 'UTF-8' parameter, do only one thing : add 'a' char in the 9th
label character

This patch only removes this incorrect function call, so, when we import a bib
record in UNIMARC : it no longer adds erroneous character (this does not change
anything for MARC21 and NORMARC because SetMarcUnicodeFlag explicitly adds 'a'
char in the 9th label for them)

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 11 Jonathan Druart 2017-05-02 16:06:50 UTC
Additional questions:

1. MARC::Record does not handle UNIMARC records correctly, so we should never call ->encoding('utf-8') from Koha, but SetMarcUnicodeFlag instead, right?

2. Sounds like the code in SetMarcUnicodeFlag should then be:
  If MARC21 or NORMARC:
    $marc_record->encoding('utf-8')
  else # unimarc
    do our stuffs

3. Please fix author's name of the patch
Comment 12 stephane.delaune 2017-05-03 07:24:42 UTC
Answers to the above questions :

- Question 1 : 
I never said "MARC::Record does not handle UNIMARC records correctly" ! 
MARC::Record->encoding('utf-8') add 'a' char in the 9th label'pos, it's ok in MARC21 and NORMARC but not in UNIMARC (requires than the 9th character in labels must be blank)
C4::Charset::SetMarcUnicodeFlag always call MARC::Record->encoding('utf-8') but it should only do for MARC21 and NORMARC

- Question 2 : 
In SetMarcUnicodeFlag, if MARC21 or NORMARC already do «substr($leader, 9, 1) = 'a'» in addition to MARC::Record->encoding('utf-8') (Which does exactly the same thing for UNIMARC MARC21 NORMARC)

We had the choice between :
- move the «MARC::Record->encoding('utf-8')»'s call instead of «substr($leader, 9, 1) = 'a'» (it's your proposition)
- delete the «MARC::Record->encoding('utf-8')»'s call and keep «substr($leader, 9, 1) = 'a'» (chosen solution)
Both are equivalent because «MARC::Record->encoding('utf-8')» only do one thing : «substr($leader, 9, 1) = 'a'»

- Question 3 : 
I do not understand the problem and I do not know how to do this
Comment 13 Paul Poulain 2017-05-03 08:48:37 UTC
Stephane, if you look at your patch:

From 9f6f0e05c9c44164d4c2d8f00f4bbd2afd87b847 Mon Sep 17 00:00:00 2001
From: Koha User <support@biblibre.com> <<<<<<< THIS IS WRONG
Date: Wed, 22 Feb 2017 12:25:33 +0100

Set your name => https://wiki.koha-community.org/wiki/Version_Control_Using_Git#Set_your_name_.26_mail_address
Comment 14 stephane.delaune 2017-05-03 09:06:44 UTC
Created attachment 63008 [details] [review]
Bug 18152 : fix unimarc label in SetMarcUnicodeFlag

only fix author's name of the patch
Comment 15 Jonathan Druart 2017-05-03 16:10:05 UTC
(In reply to stephane.delaune from comment #12)

"MARC::Record does not handle UNIMARC records correctly"

and

"MARC::Record->encoding('utf-8') add 'a' char in the 9th label'pos, it's ok in MARC21 and NORMARC but not in UNIMARC"

mean the same thing to me
Comment 16 Jonathan Druart 2017-05-03 18:31:12 UTC
Created attachment 63040 [details] [review]
Bug 18152: Add tests
Comment 17 Jonathan Druart 2017-05-03 18:31:17 UTC
Created attachment 63041 [details] [review]
Bug 18152: Leader 9 for UNIMARC records must be blank
Comment 18 Jonathan Druart 2017-05-03 18:32:48 UTC
Paul, Stéphane,

Regarding the description of this bug we are expecting the Leader 9 for UNIMARC records to be blank, does it make sense to set it in SetMarcUnicodeFlag?

Let me know if you agree with the last patch, otherwise I will adapt the tests to make them pass without it.
Comment 19 Jonathan Druart 2017-05-03 18:35:05 UTC
(In reply to Jonathan Druart from comment #18)
> Regarding the description of this bug we are expecting the Leader 9 for
> UNIMARC records to be blank, does it make sense to set it in
> SetMarcUnicodeFlag?

I do not think it makes sense actually, must be done elsewhere.
Comment 20 stephane.delaune 2017-05-04 08:00:41 UTC
(In reply to Jonathan Druart from comment #19)
> (In reply to Jonathan Druart from comment #18)
> > Regarding the description of this bug we are expecting the Leader 9 for
> > UNIMARC records to be blank, does it make sense to set it in
> > SetMarcUnicodeFlag?
> 
> I do not think it makes sense actually, must be done elsewhere.

What does not make sense is that SetMarcUnicodeFlag adds an invalid character where it should not (which can be considered a bug). To do so elsewhere is to leave a bug in place to do corrective action elsewhere: a nonsense
Comment 21 Jonathan Druart 2017-05-04 13:27:47 UTC
I do not understand your last comment.
Comment 22 Jonathan Druart 2017-05-04 13:38:16 UTC
Created attachment 63102 [details] [review]
Bug 18152 : fix unimarc label in SetMarcUnicodeFlag

The standard UNIMARC requires than the 9th character (starting from 0) in
labels must be blank (while it may be 'a' in marc21)

the problem is that C4::Charset::SetMarcUnicodeFlag (called in particular when
we import a record) always add 'a' char in the 9th label'pos whereas it should
do it just for MARC21 and NORMARC (not for UNIMARC) :

C4::Charset::SetMarcUnicodeFlag add 'a' char in the 9th label character for
MARC21 and NORMARC (it's normal), but just before doing this it call
"$marc_record->encoding('UTF-8')" which is a MARC::Record function which, when
called with 'UTF-8' parameter, do only one thing : add 'a' char in the 9th
label character

This patch only removes this incorrect function call, so, when we import a bib
record in UNIMARC : it no longer adds erroneous character (this does not change
anything for MARC21 and NORMARC because SetMarcUnicodeFlag explicitly adds 'a'
char in the 9th label for them)

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Jonathan Druart 2017-05-04 13:38:20 UTC
Created attachment 63103 [details] [review]
Bug 18152: Add tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2017-05-04 13:41:27 UTC
Seen with Stéphane: It does not make really sense to modify position 9 of leader for UNIMARC records in SetMarcUnicodeFlag, this flag is not related to encoding.

I have opened bug 18533 to remove the MARC::Record->encoding calls everywhere and replace them with SetMarcUnicodeFlag.
Comment 25 Kyle M Hall 2017-05-08 13:05:26 UTC
Pushed to master for 17.05, thanks Stephane, Jonathan!
Comment 26 Katrin Fischer 2017-05-13 12:40:13 UTC
These patches have been pushed to 16.11.x and will be in 16.11.08.
Comment 27 Julian Maurice 2017-05-22 09:30:40 UTC
Pushed to 3.22.x for 3.22.21
Comment 28 Mason James 2017-05-24 02:10:49 UTC
Pushed to 16.05.x, for 16.05.13 release
Comment 29 Jonathan Druart 2017-07-11 19:15:14 UTC
This patch introduced a regression, please provide a patch ASAP on bug 18910.
Comment 30 Jonathan Druart 2017-07-17 19:05:33 UTC
(In reply to Jonathan Druart from comment #29)
> This patch introduced a regression, please provide a patch ASAP on bug 18910.

I suggest to revert these 2 patches, Rmaints please take a look at bug 18910.
Comment 31 Katrin Fischer 2017-08-16 21:08:44 UTC
Was reverted from 16.11.x for the 16.11.10 release.