|
Description
Liz Rea
2019-04-16 14:27:16 UTC
Created attachment 88074 [details] [review] Bug 22715: Searching for patrons with "" in the circulation note hangs patron search To replicate: Edit a patron to have a circ note, include quoted text in it, such as "this is quoted string" Go to the patron search, search for a set of results that would include the patron with the quotes in the note Enjoy looking at "Processing" for forever. Test Plan: 1) Apply this patch 2) Attempt to replicate using the steps above Created attachment 88076 [details] [review] Bug 22715: Searching for patrons with "" in the circulation note hangs patron search To replicate: Edit a patron to have a circ note, include quoted text in it, such as "this is quoted string" Go to the patron search, search for a set of results that would include the patron with the quotes in the note Enjoy looking at "Processing" for forever. Test Plan: 1) Apply this patch 2) Attempt to replicate using the steps above Created attachment 88077 [details] [review] Bug 22715: Searching for patrons with "" in the circulation note hangs patron search To replicate: Edit a patron to have a circ note, include quoted text in it, such as "this is quoted string" Go to the patron search, search for a set of results that would include the patron with the quotes in the note Enjoy looking at "Processing" for forever. Test Plan: 1) Apply this patch 2) Attempt to replicate using the steps above Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88116 [details] [review] Bug 22715: Searching for patrons with "" in the circulation note hangs patron search To replicate: Edit a patron to have a circ note, include quoted text in it, such as "this is quoted string" Go to the patron search, search for a set of results that would include the patron with the quotes in the note Enjoy looking at "Processing" for forever. Test Plan: 1) Apply this patch 2) Attempt to replicate using the steps above Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> This works as described, my only concern would be that we should handle these kinds of characters in a more global approach - this only fixes one field. I think we need to look a little more here. A note like: "Don't checkout to me!" will still break the results page (In reply to Katrin Fischer from comment #5) > This works as described, my only concern would be that we should handle > these kinds of characters in a more global approach - this only fixes one > field. This entire script needs to be re-engineered to output json using the JSON module at the end of the script instead of using a template to generate json. This will solve all these encoding problems. (In reply to Kyle M Hall from comment #7) > (In reply to Katrin Fischer from comment #5) > > This works as described, my only concern would be that we should handle > > these kinds of characters in a more global approach - this only fixes one > > field. > > This entire script needs to be re-engineered to output json using the JSON > module at the end of the script instead of using a template to generate > json. This will solve all these encoding problems. Makes sense. My thinking here was that as we have another patch right now making HTML available for this field it might be worth catching some of the possible problems that would break search immediately. We also have similar problems in item search - we have a publisher with a backslash that breaks the item search regularly. We might want to take a closer look at the places where we work with JSON. I think we need to get this in, because this case will happen quite often and a partial fix now is better than months later complete fix. I've been seeing this crop up ever since the patron search results were converted to JSON, which means this bug has technically been active for at least 5 releases now. See bug 20891 for a similar issue, I think it is the correct fix. Created attachment 88749 [details] [review] Bug 22715: Searching for patrons with "" in the circulation note hangs patron search To replicate: Edit a patron to have a circ note, include quoted text in it, such as "this is quoted string" Go to the patron search, search for a set of results that would include the patron with the quotes in the note Enjoy looking at "Processing" for forever. Test Plan: 1) Apply this patch 2) Attempt to replicate using the steps above Created attachment 88786 [details] [review] Bug 22715: Searching for patrons with "" in the circulation note hangs patron search To replicate: Edit a patron to have a circ note, include quoted text in it, such as "this is quoted string" Go to the patron search, search for a set of results that would include the patron with the quotes in the note Enjoy looking at "Processing" for forever. Test Plan: 1) Apply this patch 2) Attempt to replicate using the steps above Signed-off-by: Liz Rea <wizzyrea@gmail.com> I took this a step further and used <script>"something in quotes"</script> for my test note and this worked just fine. YAY. I am in doubt about the filters used here: "[% data.borrowernotes | html_line_break | collapse | html | $To %]" html_line_break = turns line breaks into <br> OK collapse = removes additional whitespace - OK html = escapes... so it shoudl escape the <br> tags $To = ? We had other cases where we needed to switch to html html_line_break to make things work. Can someone help me out here an explain? That said.... it DOES work. Ok... actually it's me: I was testing without the patch applied. It did work. But now with the patch, I have: <b>TEst</b><br /> das <br /> Hier\ Instead of the correct display. Without the patch (just master), I get: TEst (in bold) das Hier\ Help? Created attachment 88804 [details] [review] Bug 22715: Remove extraneous html filter (In reply to Katrin Fischer from comment #13) > I am in doubt about the filters used here: > > "[% data.borrowernotes | html_line_break | collapse | html | $To %]" I think we just need one html filter - the link break should take care of filtering: [% data.borrowernotes | html_line_break | collapse | $To %] Alternately: If we don't want to process any of the html here but add line breaks: [% data.borrowernotes | html | html_line_break | collapse | $To %] I suspect we do expect 'bold' etc to be processed though, I imagine some libraries could have links here (In reply to Nick Clemens from comment #16) > (In reply to Katrin Fischer from comment #13) > > I am in doubt about the filters used here: > > > > "[% data.borrowernotes | html_line_break | collapse | html | $To %]" > > I think we just need one html filter - the link break should take care of > filtering: > [% data.borrowernotes | html_line_break | collapse | $To %] > > Alternately: > If we don't want to process any of the html here but add line breaks: > [% data.borrowernotes | html | html_line_break | collapse | $To %] > > I suspect we do expect 'bold' etc to be processed though, I imagine some > libraries could have links here I don't think that html_line_break does any escapign beyond changing the new lines. I think actually there should be a $raw as we just allowed to add HTML to that field. (In reply to Katrin Fischer from comment #17) > I don't think that html_line_break does any escapign beyond changing the new > lines. I think actually there should be a $raw as we just allowed to add > HTML to that field. I don't think we need a raw since we are filtering in other ways, I think we do want the link breaks, so this works as expected, no? I think the $raw would have no other effect than showing we want HTML there. We are not really filtering.. we are adding and removing whitespace. Not sure what $To all does. Need another opinion, maybe Jonathan? Created attachment 88805 [details] [review] Bug 22715: Searching for patrons with "" in the circulation note hangs patron search To replicate: Edit a patron to have a circ note, include quoted text in it, such as "this is quoted string" Go to the patron search, search for a set of results that would include the patron with the quotes in the note Enjoy looking at "Processing" for forever. Test Plan: 1) Apply this patch 2) Attempt to replicate using the steps above Signed-off-by: Liz Rea <wizzyrea@gmail.com> I took this a step further and used <script>"something in quotes"</script> for my test note and this worked just fine. YAY. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 88806 [details] [review] Bug 22715: Remove extraneous html filter Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> (In reply to Katrin Fischer from comment #19) > I think the $raw would have no other effect than showing we want HTML there. > We are not really filtering.. we are adding and removing whitespace. Not > sure what $To all does. Need another opinion, maybe Jonathan? Hi Katrin, see bug 20891 comment 5: """ If this patch works we will need to rethink the 'To' TT plugin. It was originally designed to have several escape methods, but with these changes it will not make sense to name it 'To' if used only to escape JSON """ (In reply to Jonathan Druart from comment #22) > (In reply to Katrin Fischer from comment #19) > > I think the $raw would have no other effect than showing we want HTML there. > > We are not really filtering.. we are adding and removing whitespace. Not > > sure what $To all does. Need another opinion, maybe Jonathan? > > Hi Katrin, see bug 20891 comment 5: > """ > If this patch works we will need to rethink the 'To' TT plugin. > It was originally designed to have several escape methods, but with > these changes it will not make sense to name it 'To' if used only to > escape JSON > """ Sorry Jonathan, but that doesn't help me :) What filters should be used here? We need "To" (which must be renamed or adjusted) to correctly json escaped. We do not want to html filter the circulation notes, so it makes sense to remove this filter. The patches look ok to me. Additionally we should json filter all the other TT variables in this template. Thank you! Awesome work all! Pushed to master for 19.05 Pushed to 18.11.x for 18.11.05 backported to 18.05.x for 18.05.13 |