Bug 9859 - C4::Charset -> nsb_clean() remove Â
Summary: C4::Charset -> nsb_clean() remove Â
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Stéphane Delaune
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-20 09:10 UTC by Stéphane Delaune
Modified: 2015-06-04 23:33 UTC (History)
6 users (show)

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


Attachments
proposed patch (763 bytes, patch)
2013-03-20 10:00 UTC, Stéphane Delaune
Details | Diff | Splinter Review
to test the patch run this file : ( perl ./test9859.pl ) (108 bytes, text/x-perl)
2013-03-20 10:06 UTC, Stéphane Delaune
Details
screenshot : Missing  in search-authority plugin (110.73 KB, image/jpeg)
2013-04-12 15:50 UTC, Mathieu Saby
Details
[PATCH][SIGNED OFF] Bug 9859:fix nsb_clean side effect (1.17 KB, patch)
2013-04-12 17:09 UTC, Mathieu Saby
Details | Diff | Splinter Review
0001-PASSED-QA-Bug-9859-fix-nsb_clean-side-effect.patch (1.31 KB, patch)
2014-10-16 14:14 UTC, Katrin Fischer
Details | Diff | Splinter Review
0002-Bug-9859-Follow-up-Adding-a-simple-unit-test-for-nsb.patch (990 bytes, patch)
2014-10-16 14:14 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 Stéphane Delaune 2013-03-20 09:10:07 UTC
the nsb_clean sub (from C4::Charset) that aims to remove Non Sorting Block from strings have an undesirable side effect : it remove this char : Â
Comment 1 Stéphane Delaune 2013-03-20 10:00:39 UTC Comment hidden (obsolete)
Comment 2 Stéphane Delaune 2013-03-20 10:06:42 UTC Comment hidden (obsolete)
Comment 3 Mathieu Saby 2013-04-12 14:33:47 UTC
I will test it.
I note that nsb_clean is only called in 3 files in Koha : 
authorities/ysearch.pl
cataloguing/ysearch.pl
C4/Biblio.pm::RemoveAllNsb

authorities/ysearch.pl is called in 1 file : 
koha-tmpl/intranet-tmpl/prog/en/includes/auth-finder-search.inc:

cataloguing/ysearch.pl is called in 1 file : 
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/unimarc_field_210c_bis.tt

RemoveAllNsb is called in 1 file :
tools/export.pl

If it is a usefull function, it would probably be usefull to use it more often.

M. Saby
Comment 4 Mathieu Saby 2013-04-12 14:47:57 UTC
Note : 
In tools/export.pl, RemoveAllNsb is called only if export.pl is used as in command line, with --clean option.
It is not called if export.pl is called from export.tt or an other page.

M. Saby
Comment 5 Mathieu Saby 2013-04-12 15:04:25 UTC
My own test to proove the problem with Â

1 creating a record with 200$aMoyen Âge and 200$f = Pierre Reverdy
2 exporting with tools/export.pl --format=marc --record-type=bibs --filename=koha.mrc
3 exporting with tools/export.pl --format=marc --record-type=bibs --clean --filename=koha2.mrc
4 searching the record in koha1.mrc with yaz-marcdump koha1.mrc | grep Reverdy
200 1  $a Moyen ge $b Texte imprimé $e notes 1930-1936 $f Pierre Reverdy
700  1 $3 027096327 $a Reverdy $b Pierre $f 1889-1960 $4 070 $9 21567
5 searching the record in koha2.mrc with yaz-marcdump koha1.mrc | grep Reverdy
200 1  $a Moyen Âge $b Texte imprimé $e notes 1930-1936 $f Pierre Reverdy
700  1 $3 027096327 $a Reverdy $b Pierre $f 1889-1960 $4 070 $9 21567

In 1st file, the  is removed, because --clean option force the script to call RemoveAllNsb, which call nsb_clean.

M. Saby
Comment 6 Mathieu Saby 2013-04-12 15:49:45 UTC
The 2d little issue is caused by authorities/ysearch.pl : 
koha-tmpl/intranet-tmpl/prog/en/includes/auth-finder-search.inc calls ysearch.pl for building the dynamic suggestions (in ajax).

1. Create an authority Moyen Âge
2. In a record, use the auth-finder-search plugin (6XX) fields
3. Enter "Moyen Âg" and see the  is missing in the suggested authority : "Moyen ge"

See screenshot
Comment 7 Mathieu Saby 2013-04-12 15:50:38 UTC
Created attachment 17399 [details]
screenshot : Missing  in search-authority plugin
Comment 8 Kyle M Hall 2013-04-12 16:51:42 UTC
Tested, works as advertised. When I try to use git bz to attach I get an error:

Traceback (most recent call last):
  File "/usr/local/bin/git-bz", line 2218, in <module>
    do_attach(*args)
  File "/usr/local/bin/git-bz", line 1670, in do_attach
    attach_commits(bug, commits, edit_comments=global_options.edit)
  File "/usr/local/bin/git-bz", line 1608, in attach_commits
    description, body, obsoletes, statuses = edit_attachment_comment(bug, commit.subject, body)
  File "/usr/local/bin/git-bz", line 1571, in edit_attachment_comment
    lines = edit_template(template.getvalue())
  File "/usr/local/bin/git-bz", line 760, in edit_template
    f.write(template)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xc2' in position 61: ordinal not in range(128)

So consider this signed off, unless there is some underlying issue that needs to be resolved.
Comment 9 Mathieu Saby 2013-04-12 17:02:15 UTC
I sign it off too.
After applying the patch,
- NSB/NSE are still removed by nsb_clean
- tools/exports.pl --clean is no more suppressing Â
- authority search plugins is no more suppressing Â

But I am not sure the removal of C2 char before NSB or NSE is usefull, as it is not a control character in UNIMARC standard...

M. Saby
Comment 10 Mathieu Saby 2013-04-12 17:09:26 UTC Comment hidden (obsolete)
Comment 11 Mathieu Saby 2013-04-12 17:12:59 UTC
Sorry, I have just sign off, but I see the code is wrong : 

+    s/($C2){0,1}($NSE|$NSE2)//g ;
should be
+    s/($C2){0,1}($NSB2|$NSE2)//g ;

B is for begin ; E for end...

Stéphane, could you re-write it, and addin the commit the description of what your patch is doing, and what problem it solved ?
(You can use the comments I left in the commit ;-) )

Mathieu
Comment 12 Mathieu Saby 2013-04-13 09:42:31 UTC
Regarding C2 char : 
There is a comment in Charset.pm :
+    my $C2   = '\xC2' ;        # What is this char ? It is sometimes left by the regexp after removing NSB / NSE

I think it was added by Matthias Meusburger, when he created the function nsb_clean in Bug 7400 (commit 1e7437bbae653840136fd1c2faed80a83aa08d2b).
Maybe Matthias could tell us in which precise cases he found this C2 char?

I place him in copy of this bug.

M. Saby
Comment 13 Stéphane Delaune 2014-10-15 13:52:37 UTC
(In reply to mathieu saby from comment #11)
> Sorry, I have just sign off, but I see the code is wrong : 
> 
> +    s/($C2){0,1}($NSE|$NSE2)//g ;
> should be
> +    s/($C2){0,1}($NSB2|$NSE2)//g ;
> 
> B is for begin ; E for end...
> 
> Stéphane, could you re-write it, and addin the commit the description of
> what your patch is doing, and what problem it solved ?
> (You can use the comments I left in the commit ;-) )
> 
> Mathieu

there is two lines in patch :
+    s/($C2){0,1}($NSB|$NSB2)//g ;
to proceed B for Begin and
+    s/($C2){0,1}($NSE|$NSE2)//g ;
to proceed E for End

so it don't need to be rewrite for this
Comment 14 Mathieu Saby 2014-10-16 06:37:12 UTC
Hello
That's pretty old, I don't remember this bug at all, and I won't test it again ;-)
Do you mean in your previous comment that for you the patch is OK?

If so, let's wait for QA.

Mathiue
Comment 15 Stéphane Delaune 2014-10-16 06:46:01 UTC
(In reply to mathieu saby from comment #14)
> Hello
> That's pretty old, I don't remember this bug at all, and I won't test it
> again ;-)
> Do you mean in your previous comment that for you the patch is OK?
> 
> If so, let's wait for QA.
> 
> Mathiue

Yes the patch is ok
Comment 16 Katrin Fischer 2014-10-16 14:14:22 UTC
Created attachment 32415 [details] [review]
0001-PASSED-QA-Bug-9859-fix-nsb_clean-side-effect.patch
Comment 17 Katrin Fischer 2014-10-16 14:14:48 UTC
Created attachment 32416 [details] [review]
0002-Bug-9859-Follow-up-Adding-a-simple-unit-test-for-nsb.patch
Comment 18 Stéphane Delaune 2014-10-16 15:05:12 UTC
Thanks Katrin
Comment 19 Tomás Cohen Arazi 2014-10-22 17:33:42 UTC
Patch pushed to master.

Thanks Stephane!