== Reproduce == 1. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result. 2. Click on the first result 3. Check the "Return to results" It should be like staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a This is the bug. Because when clicking the link, your search will be 'kw,wrdl: a' instead of 'kw,wrdl: a "alice"' == Cause == Something about escaping in koha-tmpl/intranet-tmpl/js/browser.js Search for BROWSER_RETURN_TO_SEARCH
Created attachment 74251 [details] [review] Bug 20529: Fix "Return to results" link broken by a double quote (in the search query) cause: decodeURIComponent used when URI encoding was necessary. (At least in main usecase of this feature) It could have been a copy and paste error. From the above browseRecords() function. == Test plan == 1. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 2. Click on the first result 3. Check the "Return to results" link It should be like staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a This is the bug. Because when clicking the link, your search will be 'kw,wrdl: a' instead of 'kw,wrdl: a "alice"' 4. Apply this patch. 5. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 6. Click on the first result 7. Check the "Return to results" link The link should preserve the full query
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74553 [details] [review] Bug 20529: Fix "Return to results" link broken by a double quote (in the search query) cause: decodeURIComponent used when URI encoding was necessary. (At least in main usecase of this feature) It could have been a copy and paste error. From the above browseRecords() function. == Test plan == 1. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 2. Click on the first result 3. Check the "Return to results" link It should be like staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a This is the bug. Because when clicking the link, your search will be 'kw,wrdl: a' instead of 'kw,wrdl: a "alice"' 4. Apply this patch. 5. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 6. Click on the first result 7. Check the "Return to results" link The link should preserve the full query Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74554 [details] [review] Bug 20529: Fix "Return to results" link broken by a double quote (in the search query) cause: decodeURIComponent used when URI encoding was necessary. (At least in main usecase of this feature) It could have been a copy and paste error. From the above browseRecords() function. == Test plan == 1. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 2. Click on the first result 3. Check the "Return to results" link It should be like staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a This is the bug. Because when clicking the link, your search will be 'kw,wrdl: a' instead of 'kw,wrdl: a "alice"' 4. Apply this patch. 5. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 6. Click on the first result 7. Check the "Return to results" link The link should preserve the full query Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Works with both Zebra (Biblibre's sandbox #10) and ElasticSearch (Biblibre's sandbox #12), with the original navigation tool and the new " BrowseResultSelection" option. Thanks Victor !
Thanks Séverine for testing with ES!
Created attachment 74610 [details] [review] Bug 20529: Fix "Return to results" link broken by a double quote (in the search query) cause: decodeURIComponent used when URI encoding was necessary. (At least in main usecase of this feature) It could have been a copy and paste error. From the above browseRecords() function. == Test plan == 1. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 2. Click on the first result 3. Check the "Return to results" link It should be like staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a This is the bug. Because when clicking the link, your search will be 'kw,wrdl: a' instead of 'kw,wrdl: a "alice"' 4. Apply this patch. 5. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 6. Click on the first result 7. Check the "Return to results" link The link should preserve the full query Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Why this occurrence and not the others?
Because I wasn't able to trigger the code of the other occurrences ^^" https://gitlab.com/koha-community/Koha/blob/95031b035b5de87211d0de2040eb685ae005606c/koha-tmpl/intranet-tmpl/js/browser.js#L33
I guess it's: go to first, next, previous, go to last, etc.
> go to first, next, previous, go to last, etc. It doesn't work, placed few console.log() and those next to the two occurrences of the usage of `decodeURIComponent(current_search.query)` weren't triggered.
Reading the code I'd say these 2 switches prevent wrong calls to browseRecords (like you call it with movement != 1 and -1)
Unfortunately I'm out of time to expand much more [1] the scope of this issue. 1. Is the fix simple enough to be applied without having a test plan? 2. Is it okay to keep the scope as it is? 3. Should the patch stay pending waiting until someone to complete it? [1] implementation (not much work) + test plan that is still a mystery for me about how to expand it
I'd say we could go without test plan, if the changes make sense and work in that case it must work for the "special" cases.
Sorry Victor, it failed when I tried.
Why keeping decodeURIComponent() for current_search.limit ?
(In reply to Jonathan Druart from comment #15) > I'd say we could go without test plan, if the changes make sense and work in > that case it must work for the "special" cases. Ok, thanks for the additional analysis. (In reply to Séverine Queune from comment #16) > Sorry Victor, it failed when I tried. Thanks for testing. (In reply to Fridolin SOMERS from comment #17) > Why keeping decodeURIComponent() for current_search.limit ? I mostly lost the context. I guess that was because it didn't relate to the bug. Should decodeURIComponent be also removed for current_search.limit? The good news is that this part is known how to be tested. As opposed to the other occurrences.
What does this bug need ?
The patch needs a major rebase, since we since moved some things around like changing the way we do the translations on this script. But the problem remains. I tested with: a "test" The link back to the results turns it into a search for: a ++<<<<<<< HEAD + var searchURL = '/cgi-bin/koha/catalogue/search.pl?' + decodeURIComponent(current_search.query) + '&limit=' + decodeURIComponent(current_search.limit) + '&sort_by=' + current_search.sort + '&searchid=' + me.searchid + '&offset=' + me.offset; ++======= + var searchURL = '/cgi-bin/koha/catalogue/search.pl?' + current_search.query + '&limit=' + decodeURIComponent(current_search.limit) + '&sort=' + current_search.sort + '&searchid=' + me.searchid + '&offset=' + me.offset; ++>>>>>>> Bug 20529: Fix "Return to results" link broken by a double quote
Created attachment 120816 [details] [review] Bug 20529: Fix "Return to results" link broken by a double quote (in the search query) cause: decodeURIComponent used when URI encoding was necessary. (At least in main usecase of this feature) It could have been a copy and paste error. From the above browseRecords() function. == Test plan == 1. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 2. Click on the first result 3. Check the "Return to results" link It should be like staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a This is the bug. Because when clicking the link, your search will be 'kw,wrdl: a' instead of 'kw,wrdl: a "alice"' 4. Apply this patch. 5. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 6. Click on the first result 7. Check the "Return to results" link The link should preserve the full query Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Rebased and solved conflict. ==== > The patch needs a major rebase, since we since moved some things around like changing the way we do the translations on this script. Does it? Test plan work on my instance. ==== > But the problem remains. I tested with: a "test" With sample data, the test plan succeeds with «a "test"»
Created attachment 122626 [details] [review] Bug 20529: Fix "Return to results" link broken by a double quote (in the search query) cause: decodeURIComponent used when URI encoding was necessary. (At least in main usecase of this feature) It could have been a copy and paste error. From the above browseRecords() function. == Test plan == 1. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 2. Click on the first result 3. Check the "Return to results" link It should be like staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a This is the bug. Because when clicking the link, your search will be 'kw,wrdl: a' instead of 'kw,wrdl: a "alice"' 4. Apply this patch. 5. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 6. Click on the first result 7. Check the "Return to results" link The link should preserve the full query Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
I totally reproduce bug and patch fixes it. We already see : decodeURIComponent(current_search.limit) It is logical to use : decodeURIComponent(current_search.query)
Created attachment 122736 [details] [review] Bug 20529: Fix "Return to results" link broken by a double quote (in the search query) cause: decodeURIComponent used when URI encoding was necessary. (At least in main usecase of this feature) It could have been a copy and paste error. From the above browseRecords() function. == Test plan == 1. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 2. Click on the first result 3. Check the "Return to results" link It should be like staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a This is the bug. Because when clicking the link, your search will be 'kw,wrdl: a' instead of 'kw,wrdl: a "alice"' 4. Apply this patch. 5. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 6. Click on the first result 7. Check the "Return to results" link The link should preserve the full query Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Fridolin Somers from comment #24) > I totally reproduce bug and patch fixes it. > > We already see : > decodeURIComponent(current_search.limit) > It is logical to use : > decodeURIComponent(current_search.query) Hum? What do you mean? If it makes sense to remove this decode call then we should remove other occurrences from this file. I don't see the point of decodeURIComponent to generate an URI, unless we are doing weird stuffs somewhere else (or it's Friday afternoon and I am tired).
Created attachment 122751 [details] [review] Bug 20529: Remove other occurrences of decodeURIComponent from browser.js
Can someone stress the feature with this patch? IIRC weird bugs usually happens "on the edge" of the pagination. Like search with your favourite weird terms and click the last records from the page, then next, previous.
(In reply to Jonathan Druart from comment #28) > Can someone stress the feature with this patch? IIRC weird bugs usually > happens "on the edge" of the pagination. > > Like search with your favourite weird terms and click the last records from > the page, then next, previous. This needs feedback from the author
> Can someone stress the feature with this patch? What do you mean? Is the patch name too vague? The first 3 steps of the test plan show the issue.
(In reply to Victor Grousset/tuxayo from comment #30) > > Can someone stress the feature with this patch? > > What do you mean? Is the patch name too vague? > The first 3 steps of the test plan show the issue. I've explained it: "Like search with your favourite weird terms and click the last records from the page, then next, previous."
*** Bug 28866 has been marked as a duplicate of this bug. ***
Created attachment 124329 [details] [review] Bug 20529: Fix "Return to results" link broken by a double quote (in the search query) cause: decodeURIComponent used when URI encoding was necessary. (At least in main usecase of this feature) It could have been a copy and paste error. From the above browseRecords() function. == Test plan == 1. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 2. Click on the first result 3. Check the "Return to results" link It should be like staff_url://cgi-bin/koha/catalogue/search.pl?idx=kw&q=a This is the bug. Because when clicking the link, your search will be 'kw,wrdl: a' instead of 'kw,wrdl: a "alice"' 4. Apply this patch. 5. Search in the staff catalogue «a "alice"» Replace a and alice with whatever than will return a result for you 6. Click on the first result 7. Check the "Return to results" link The link should preserve the full query Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 124330 [details] [review] Bug 20529: Remove other occurrences of decodeURIComponent from browser.js Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Moving to PQA - I tested with as many odd searches as I could, easier to break search and get no results than to break this. Any search with ccode will break without these patches as we double quote their values in the search code (in case they contain strange values, but we don't care about other auth_values?) so I think we should get this in
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.04
Pushed to 20.11.x for 20.11.10
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.