Bug 22070 - OpacBrowseResults has broken link back to search results
Summary: OpacBrowseResults has broken link back to search results
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Bugs List
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 11515 11944
Blocks:
  Show dependency treegraph
 
Reported: 2019-01-04 13:39 UTC by Katrin Fischer
Modified: 2022-08-19 18:12 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22070: Try to fix encoding and escaping issue on browse result (2.66 KB, patch)
2021-01-21 11:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22070: Try to fix encoding and escaping issue on browse result (2.78 KB, patch)
2022-06-25 13:29 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2019-01-04 13:39:03 UTC
Under certain conditions the OPACBrowseResults (navigation between results in OPAC) feature will have a broken link "back to results".

To test:
- Search for a term with diacritics. You can cheat a little here and search for something like "bööks" with the sample data. (ö will also find o). 
It's important that it's at least to result pages.
- Go to the last result of the first result page
- Verify the URL "back to results" works and diacritic shows up nicely
- Go to the last result of the first page again
- Navigate "Next >>" 
- Check "Back to results" again
- Verify the link is broken and the diacritics are mangled


Link looks something like this:
http://localhost:8080/cgi-bin/koha/opac-search.pl?offset=20&q=b%C3%83%C2%83%C3%82%C2%B6%C3%83%C2%83%C3%82%C2%B6ks
Comment 1 Katrin Fischer 2019-04-26 09:11:48 UTC
Still valid for 18.11.
Comment 2 Jonathan Druart 2021-01-21 11:24:27 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2021-04-17 14:53:48 UTC
Could this be somehow related to the discussion on bug 20529?
Comment 4 Katrin Fischer 2022-05-06 14:57:01 UTC
The encoding issue is still present in 20.11 and master.
Comment 5 Katrin Fischer 2022-05-06 15:03:35 UTC
The Next link is not always hidden on the last result of the page, but sometimes, this might be another bug.

If you keep previous/next'ing you will eventually see the next link to jump to first result of the second page and the link back to results breaks.

The patch doesn't fix it.
Comment 6 Jonathan Druart 2022-05-09 07:16:36 UTC
I don't understand, Katrin. What you are describing does not seem related to this patch.
Comment 7 Katrin Fischer 2022-05-09 08:46:50 UTC
Hi Jonathan, 

it is related. In my test plan, it asks you to click on next from the last result of the first page, but you noted:

> Note that if there are several pages, the next link won't appear if you
> are on the last result of the first page.

And it's true... but then it's not. I managed to sometimes have that 'next' button and then when you click it, it causes the encoding issue on the 'back to results' link.
Comment 8 Owen Leonard 2022-06-03 12:34:13 UTC
I don't seem to be able to reproduce this bug in master
Comment 9 Katrin Fischer 2022-06-25 13:25:59 UTC
I'll have another go at making this reproducable in a simpler way:

1) Fresh ktd sample database, reset_all, even used a private window.
2) OPAC: search for: bööks 
3) Open detail page of  last result on the result list.
4) In the browser, click on "Previous"
5) Check the URL for "back to results" - it will show broken characters
6) Click on it - it's broken

... and the patch fixes this for me.
Comment 10 Katrin Fischer 2022-06-25 13:29:18 UTC
Created attachment 136550 [details] [review]
Bug 22070: Try to fix encoding and escaping issue on browse result

Pretty sure this does not work but in my tests it seems to fix the
problem.
Note that if there are several pages, the next link won't appear if you
are on the last result of the first page.

I don't understand why this code is so complicated, couldn't we port
what we have on the staff interface (browser.js)?

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Katrin Fischer 2022-06-25 13:30:07 UTC
I know the patch and commit message need some more work, but maybe the updated test plan in comment#9 and the SO will help to get some more eyes on it.
Comment 12 Fridolin Somers 2022-06-30 06:11:47 UTC
I do reproduce.

Searching for bööks.

Browsing from result n°18 to n°19 then n°20 does not break encoding.
Browsing from result n°20 to n°19 breaks encoding.
Browsing from result n°19 to n°18 does not break encoding.

o_O
Comment 13 Katrin Fischer 2022-07-02 21:45:18 UTC
(In reply to Fridolin Somers from comment #12)
> I do reproduce.
> 
> Searching for bööks.
> 
> Browsing from result n°18 to n°19 then n°20 does not break encoding.
> Browsing from result n°20 to n°19 breaks encoding.
> Browsing from result n°19 to n°18 does not break encoding.
> 
> o_O

Thx Frido, I have a feeling it could have to do with the "last record on a page" somehow?
Comment 14 Jonathan Druart 2022-07-04 12:18:43 UTC
(In reply to Fridolin Somers from comment #12)
> I do reproduce.
> 
> Searching for bööks.
> 
> Browsing from result n°18 to n°19 then n°20 does not break encoding.
> Browsing from result n°20 to n°19 breaks encoding.
> Browsing from result n°19 to n°18 does not break encoding.
> 
> o_O

With or without the patch?
Comment 15 Fridolin Somers 2022-07-06 09:16:04 UTC
(In reply to Jonathan Druart from comment #14)
> (In reply to Fridolin Somers from comment #12)
> > I do reproduce.
> > 
> > Searching for bööks.
> > 
> > Browsing from result n°18 to n°19 then n°20 does not break encoding.
> > Browsing from result n°20 to n°19 breaks encoding.
> > Browsing from result n°19 to n°18 does not break encoding.
> > 
> > o_O
> 
> With or without the patch?

Without patch.

Feels strange to remove URI escape, we should try with URI chars like & ? =
Comment 16 Marcel de Rooy 2022-07-29 06:47:18 UTC
QA: Looking here
Comment 17 Marcel de Rooy 2022-07-29 07:36:19 UTC
This is very weird code. Apart from the encoding, the paging seems to be buggy too.

Can reproduce the issue Frido mentioned:

Back to results (pos 9 with 5 results per page WHEN coming back from pos 10)
/cgi-bin/koha/opac-search.pl?idx=kw&sort_by=relevance_dsc&q=b%C3%83%C2%B6%C3%83%C2%B6k
=> böök

The previous and next links are good, but the Back to results becomes even more bad when paging back and forth..
Comment 18 Marcel de Rooy 2022-07-29 08:30:56 UTC
Have to conclude that this is not ready for passing QA yet.

Just removing the encoding/escaping does not look good.
We meet the issue here too of the encoding of CGI::Session. If we store unicode stuff (in the URL) within param busc, how does it get stored and how should we retrieve?
It feels like we should do conversion as early as we retrieve and as late as we store it again. And such that it survives pure ASCII, Latin and 'real' Unicode.

-    use URI::Escape qw( uri_escape_utf8 uri_unescape );
+    use C4::Search;
This does not look good either. Probably we need to move URI::Escape to the top.

The whole logic for Back and Next in this feature looks buggy. But that is outside the scope of this report.
Comment 19 Jonathan Druart 2022-07-29 08:53:39 UTC
I won't have time to dedicate to this in the near future.