Bug 21716 - Item Search hangs when \ exists in MARC fields
Summary: Item Search hangs when \ exists in MARC fields
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Claire Gravely
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-29 15:29 UTC by Claire Gravely
Modified: 2019-09-03 21:02 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00


Attachments
Bug 21716: Item Search hangs when \ exists in MARC fields (2.00 KB, patch)
2018-11-30 10:42 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 21716: Item Search hangs when \ exists in MARC fields (2.05 KB, patch)
2018-11-30 12:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21716: Do not use TT to produce JSON for item search results (16.74 KB, patch)
2018-12-07 13:44 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 21716: Add missing $raw filters (3.91 KB, patch)
2018-12-13 12:25 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 21716: Escape publisher in item search (1.95 KB, patch)
2019-04-25 15:39 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 21716: Escape publisher in item search (2.01 KB, patch)
2019-07-25 22:30 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 21716: Escape publisher in item search (2.07 KB, patch)
2019-07-26 12:45 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Claire Gravely 2018-10-29 15:29:07 UTC
When there are \ (backslashes) in MARC records the item search hangs when trying to load the page the record should appear on. It gets stuck on processing. 

My specific example has a Publisher written as Narr\Francke\Attempto in 260 $b

The error is:

SyntaxError: JSON.parse: bad escaped character at line 8 column 9 of the JSON data


Tested on 16.11 17.11 and master.
Comment 1 Martin Renvoize 2018-11-29 12:09:33 UTC
OMG, we're using a TT template to convert Perl to JSON.. no wonder this doesn't work properly :'(
Comment 2 Martin Renvoize 2018-11-29 12:44:25 UTC
JSON.Escape and the `| json` filter is probably your friend here... or go for the bigger job and convert it to using a json parser properly.
Comment 3 Claire Gravely 2018-11-30 10:42:28 UTC Comment hidden (obsolete)
Comment 4 Martin Renvoize 2018-11-30 12:41:59 UTC Comment hidden (obsolete)
Comment 5 Martin Renvoize 2018-11-30 12:45:21 UTC
This works as expected and is a helpful addition... I've tested it and am signing off.

QA Note.. this is sort of horrible, that's we're generating JSON by passing it through a template.. but that's how it currently is and it would be a fairly major refactor to undertake for such a minor fix.

We could apply the 'json' filter further too, but I believe that should be as a followup somewhere myself, rather than confusing the issue here.

Might be nice to add regression tests too.. wonder if Jonathan or someone could mentor writing those (I must admit, to an extent the Selenium tests are still a mystery to me).
Comment 6 Julian Maurice 2018-12-07 13:24:51 UTC
(In reply to Martin Renvoize from comment #5)
> This works as expected and is a helpful addition... I've tested it and am
> signing off.
It doesn't work for me. The page loads, yes. But publisher and publication year have disappeared.

> QA Note.. this is sort of horrible, that's we're generating JSON by passing
> it through a template.. but that's how it currently is and it would be a
> fairly major refactor to undertake for such a minor fix.
I agree this is horrible, and I'm the one responsible for it :)
When I wrote the patch, I believed it was the only way to have translatable strings in a server-side Datatable setup.
Fortunately I was wrong, and we can make use of the datatable's 'mRender' option to separate the data (what should be returned in the JSON response) from the rendered html (which should contain translated strings)
It does not require a big refactor. I will submit a patch soon.
Comment 7 Julian Maurice 2018-12-07 13:44:32 UTC
Created attachment 82980 [details] [review]
Bug 21716: Do not use TT to produce JSON for item search results

It was initially done this way to have translatable strings in the
results table, but this method has flaws and it can be done in a better
way.

This patch moves the rendering code to client-side javascript, so the
JSON returned by the server now contains only unprocessed data.

Also, this patch makes use of the new i18n macros for translation and
fixes a whitespace bug in i18n.inc

Reported-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 8 Martin Renvoize 2018-12-10 13:09:17 UTC
Oh nice :)

Fancy testing Claire and I'll grab it for QA?
Comment 9 Owen Leonard 2018-12-10 17:52:54 UTC
The QA tool says there are missing filters, for example:
  missing_filter at line 383 ( output += ' ' + [% t('by').json %]; )

it should be:

[% t('by').json | $raw %]
Comment 10 Julian Maurice 2018-12-13 12:25:18 UTC
Created attachment 83147 [details] [review]
Bug 21716: Add missing $raw filters
Comment 11 Liz Rea 2019-03-26 00:14:40 UTC
Hi,

I've just checked this patch, and it seems that the hang doesn't occur on current master, but it is broken again without the patch (infinite processing).

Is this still needed? It appears to me that it isn't.

Cheers,
Liz
Comment 12 Katrin Fischer 2019-04-25 05:46:22 UTC
Maybe the fix from bug 20891 would be helpful here.
Comment 13 Liz Rea 2019-04-25 15:39:25 UTC
Created attachment 88790 [details] [review]
Bug 21716: Escape publisher in item search

To prevent endless processing errors.

To test:

* create a biblio with a publisher with a backslash - i.e. 260$b with
graham\lineham
* do an item search that will include that title in the results
* wait forever, because it won't work.
* apply this patch
* repeat steps, notice that you
    a. get results
    b. your result is listed with it's correct publisher
* rejoice
Comment 14 Claire Gravely 2019-04-26 06:12:50 UTC
Hi, I tried testing but with the patch applied I get no results at all (even the pages without my dodgy biblio). The item search results page hangs on processing and in the console I get a response 'internal server error'
Comment 15 Hayley Mapley 2019-07-25 22:30:58 UTC
Created attachment 91780 [details] [review]
Bug 21716: Escape publisher in item search

To prevent endless processing errors.

To test:

* create a biblio with a publisher with a backslash - i.e. 260$b with
graham\lineham
* do an item search that will include that title in the results
* wait forever, because it won't work.
* apply this patch
* repeat steps, notice that you
    a. get results
    b. your result is listed with it's correct publisher
* rejoice

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 16 Nick Clemens 2019-07-26 12:45:52 UTC
Created attachment 91807 [details] [review]
Bug 21716: Escape publisher in item search

To prevent endless processing errors.

To test:

* create a biblio with a publisher with a backslash - i.e. 260$b with
graham\lineham
* do an item search that will include that title in the results
* wait forever, because it won't work.
* apply this patch
* repeat steps, notice that you
    a. get results
    b. your result is listed with it's correct publisher
* rejoice

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Martin Renvoize 2019-07-26 15:19:08 UTC
Nice work!

Pushed to master for 19.11.00
Comment 18 Lucas Gass 2019-09-03 21:02:01 UTC
backported to 19.05.x for 19.05.03


backported to 18.11.x for 18.11.10