Bug 18655

Summary: Unimarc field 210c fails on importing fields with a simple quote
Product: Koha Reporter: Baptiste <baptiste.wojtkowski>
Component: CatalogingAssignee: Fridolin Somers <fridolin.somers>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: charles.farmer, fridolin.somers, katrin.fischer, m.de.rooy, martin.renvoize, nick, sandboxes, simon.pouchol, sonia.bouis
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 18655: correct the escaping of ' and move code toward jquery part
Bug 18655: Followup
Bug 18655: Correct the "chose" link
Bug 18655: Correct the choose link in unimarc_field_210c value builder
Bug 18655: Correct the choose link in unimarc_field_210c value builder
Bug 18655: Correct the choose link in unimarc_field_210c value builder
Bug 18655: Correct the choose link in unimarc_field_210c value builder

Description Baptiste 2017-05-23 08:52:33 UTC
When you search for an editor with a simple quote in the name, the escaping fails and nothing happens.
Comment 1 Baptiste 2017-05-23 08:54:35 UTC Comment hidden (obsolete)
Comment 2 Fridolin Somers 2017-05-23 13:35:16 UTC Comment hidden (obsolete)
Comment 3 Baptiste 2017-05-23 13:42:33 UTC
Created attachment 63653 [details] [review]
Bug 18655: Correct the "chose" link

- Link 'chose' is now a button.
- It doesn't call directly javascript anymore (use of listener).
- field to load stored in html on the page (less escaping needed).
Comment 4 Katrin Fischer 2017-10-15 13:01:32 UTC
Maybe someone familiar with the plugin could test? 

It sounds like a search within the plugin which contains ' should fail without the patch.
Comment 5 Simon Pouchol 2017-10-26 13:11:54 UTC
Hi Baptiste,
I was looking to test this patch, could you provide a Test Plan ?
I can't seem to able to reproduce the bug within the unimarc plugin 210c, with or without the patch I can search for an authority with a ' in the name.
The choose button doesn't seem appear for me at all (can't find it hidden in dev tools either).
Comment 6 Fridolin Somers 2017-11-20 11:21:45 UTC
(In reply to Simon Pouchol from comment #5)
> Hi Baptiste,
> I was looking to test this patch, could you provide a Test Plan ?
> I can't seem to able to reproduce the bug within the unimarc plugin 210c,
> with or without the patch I can search for an authority with a ' in the name.
> The choose button doesn't seem appear for me at all (can't find it hidden in
> dev tools either).

I continue Baptiste's job.

You need an existing authority with a ' in its name, like "L'histoire".

When trying to choose this authority it fails.

Btw I think that writing JS inside HTML tags is now prohibited.
Comment 7 Charles Farmer 2018-03-30 21:33:10 UTC
Patch doesn't apply anymore, because of what looks like an added MACRO jsinclude BLOCK:

> CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/unimarc_field_210c.tt
Comment 8 Katrin Fischer 2018-03-31 05:58:52 UTC
Javascript has been moved to the bottom of all the tt in intranet - that's probably causing the conflict.
Comment 9 Fridolin Somers 2018-06-19 07:50:39 UTC
Created attachment 76168 [details] [review]
Bug 18655: Correct the choose link in unimarc_field_210c value builder

- Link 'choose' is now a button.
- It doesn't call directly javascript anymore (use of listener).
- field to load stored in html on the page (less escaping needed).

Test plan :
1) Use UNIMARC catalog
2) Define unimarc_field_210c value builder on 210$c
3) Define an autority type EDITORS with heading on 200$b
4) Create an autority of this type with a single quote in heading, ie : l'avenir
5) Index this new autority
6) Edit a biblio record and launch 210$c value builder
7) Search for new autority
8) Click on 'Choose'
=> Heading should be pasted in 210$c
Comment 10 Fridolin Somers 2018-09-14 19:10:10 UTC
Created attachment 78710 [details] [review]
Bug 18655: Correct the choose link in unimarc_field_210c value builder

- Link 'choose' is now a button.
- It doesn't call directly javascript anymore (use of listener).
- field to load stored in html on the page (less escaping needed).

Test plan :
1) Use UNIMARC catalog
2) Define unimarc_field_210c value builder on 210$c
3) Define an autority type EDITORS with heading on 200$b
4) Create an autority of this type with a single quote in heading, ie : l'avenir
5) Index this new autority
6) Edit a biblio record and launch 210$c value builder
7) Search for new autority
8) Click on 'Choose'
=> Heading should be pasted in 210$c
Comment 11 Biblibre Sandboxes 2018-09-15 17:29:14 UTC
Patch tested with a sandbox, by David Nind <david@davidnind.com>
Comment 12 Biblibre Sandboxes 2018-09-15 17:29:45 UTC
Created attachment 78865 [details] [review]
Bug 18655: Correct the choose link in unimarc_field_210c value builder

- Link 'choose' is now a button.
- It doesn't call directly javascript anymore (use of listener).
- field to load stored in html on the page (less escaping needed).

Test plan :
1) Use UNIMARC catalog
2) Define unimarc_field_210c value builder on 210$c
3) Define an autority type EDITORS with heading on 200$b
4) Create an autority of this type with a single quote in heading, ie : l'avenir
5) Index this new autority
6) Edit a biblio record and launch 210$c value builder
7) Search for new autority
8) Click on 'Choose'
=> Heading should be pasted in 210$c

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 Chris Cormack 2018-10-10 00:08:58 UTC
Created attachment 80315 [details] [review]
Bug 18655: Correct the choose link in unimarc_field_210c value builder

- Link 'choose' is now a button.
- It doesn't call directly javascript anymore (use of listener).
- field to load stored in html on the page (less escaping needed).

Test plan :
1) Use UNIMARC catalog
2) Define unimarc_field_210c value builder on 210$c
3) Define an autority type EDITORS with heading on 200$b
4) Create an autority of this type with a single quote in heading, ie : l'avenir
5) Index this new autority
6) Edit a biblio record and launch 210$c value builder
7) Search for new autority
8) Click on 'Choose'
=> Heading should be pasted in 210$c

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 14 Nick Clemens 2018-10-15 16:57:40 UTC
Awesome work all!

Pushed to master for 18.11
Comment 15 Martin Renvoize 2018-10-17 10:44:59 UTC
Pushed to 18.05.x for 18.05.05
Comment 16 Fridolin Somers 2018-10-17 12:33:20 UTC
Pushed to 17.11.x for 17.11.11