Bug 9859

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

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

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

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

Created attachment 17399 [details]
screenshot : Missing  in search-authority plugin

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.

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

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

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

(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

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

(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

Created attachment 32415 [details] [review]
0001-PASSED-QA-Bug-9859-fix-nsb_clean-side-effect.patch

Created attachment 32416 [details] [review]
0002-Bug-9859-Follow-up-Adding-a-simple-unit-test-for-nsb.patch

Thanks Katrin

Patch pushed to master.

Thanks Stephane!