Description
Claire Gravely
2018-10-29 15:29:07 UTC
OMG, we're using a TT template to convert Perl to JSON.. no wonder this doesn't work properly :'( 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. Created attachment 82776 [details] [review] Bug 21716: Item Search hangs when \ exists in MARC fields 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. This patch fixes this issue. To Test: 1. Create or edit a biblio and add in \ to the publisher field. eg: Narr\Francke\Attempto 2. Make sure this biblio has items attached that you can search for 3. Do an item search which would include the edited biblio in the results - a general search for a home library or shelving location will work 4. Try to load All results (or the page with the edited biblio) The page will hang :-( 5. Apply patch 6. Perform the item search again 7. Try to load All results The page will load :-) Mentored by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 82779 [details] [review] Bug 21716: Item Search hangs when \ exists in MARC fields 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. This patch fixes this issue. To Test: 1. Create or edit a biblio and add in \ to the publisher field. eg: Narr\Francke\Attempto 2. Make sure this biblio has items attached that you can search for 3. Do an item search which would include the edited biblio in the results - a general search for a home library or shelving location will work 4. Try to load All results (or the page with the edited biblio) The page will hang :-( 5. Apply patch 6. Perform the item search again 7. Try to load All results The page will load :-) Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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). (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. 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> Oh nice :) Fancy testing Claire and I'll grab it for QA? 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 %] Created attachment 83147 [details] [review] Bug 21716: Add missing $raw filters 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 Maybe the fix from bug 20891 would be helpful here. 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 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' 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> 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> Nice work! Pushed to master for 19.11.00 backported to 19.05.x for 19.05.03 backported to 18.11.x for 18.11.10 |