Bug 6483 - Paging in opac-detail when coming from a search
Summary: Paging in opac-detail when coming from a search
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: PATCH-Sent (DO NOT USE) enhancement (vote)
Target Milestone: ---
Assignee: Salva Zaragoza Rubio
QA Contact: Bugs List
URL:
Whiteboard:
Keywords:
: 3075 6120 (view as bug list)
Depends on:
Blocks: 7394 7935 11515
  Show dependency treegraph
 
Reported: 2011-06-08 11:21 UTC by Salva Zaragoza Rubio
Modified: 2014-01-10 10:51 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Proposed Patch for Paging in opac-detail when coming from a search (28.37 KB, patch)
2011-06-08 11:26 UTC, Salva Zaragoza Rubio
Details | Diff | Splinter Review
Proposed patch for Add paging in opac-detail when coming from a search with session storage (29.29 KB, patch)
2011-06-13 16:16 UTC, Salva Zaragoza Rubio
Details | Diff | Splinter Review
Pagination, standard view (67.64 KB, image/png)
2011-06-15 10:10 UTC, Magnus Enger
Details
Pagination, expanded view (91.96 KB, image/png)
2011-06-15 10:12 UTC, Magnus Enger
Details
Suggested change (79.04 KB, image/png)
2011-06-15 15:57 UTC, Owen Leonard
Details
New look proposal for the pagination in opac-detail (660.45 KB, application/pdf)
2011-06-16 12:11 UTC, Salva Zaragoza Rubio
Details
Extracted first image from PDF: Pagination links (59.96 KB, image/png)
2011-06-16 12:19 UTC, Owen Leonard
Details
Extracted second image from PDF: Expanded results menu (68.67 KB, image/png)
2011-06-16 12:20 UTC, Owen Leonard
Details
Proposed patch with the new look (32.88 KB, patch)
2011-06-20 09:01 UTC, Salva Zaragoza Rubio
Details | Diff | Splinter Review
Signed-off patch (32.90 KB, patch)
2011-06-21 07:08 UTC, Magnus Enger
Details | Diff | Splinter Review
Rebased patch to fix conflict on opac.css (32.86 KB, patch)
2011-08-12 07:13 UTC, Salva Zaragoza Rubio
Details | Diff | Splinter Review
Signed-off patch (32.97 KB, patch)
2011-08-24 13:24 UTC, Frédéric Demians
Details | Diff | Splinter Review
Passed QA patch (rebased on current master) (33.08 KB, patch)
2011-10-19 17:55 UTC, Ian Walls
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Salva Zaragoza Rubio 2011-06-08 11:21:30 UTC
Add paging in opac-detail when coming from a search to be able to continue viewing the results list in the opac-detail without returning to the results list page.
Comment 1 Salva Zaragoza Rubio 2011-06-08 11:26:31 UTC Comment hidden (obsolete)
Comment 2 Magnus Enger 2011-06-08 13:03:35 UTC
This looks like a really, really cool new feature! Assessing the implementation of it is a little bit over my head, but I do have a couple of questions: 

1. I'm not crazy about that busc-parameter in the URL of the detail view - I'm rather fond of the clean and simple URL structure of Koha... Would it be possible to store that information in a cookie/session? You say something about "avoid storing it in the session" - I'm not sure it should be avoided... ;-) 

2. This is such a cool feature, I think it deserves a more prominent place than at the bottom of the screen, possibly below lots and lots of items. Could it be placed in the right hand column, as a new "box" between the existing two? (Below "more searches" and above "save record".)
Comment 3 Salva Zaragoza Rubio 2011-06-08 13:45:15 UTC
(In reply to comment #2)
> This looks like a really, really cool new feature! Assessing the implementation
> of it is a little bit over my head, but I do have a couple of questions: 
> 
> 1. I'm not crazy about that busc-parameter in the URL of the detail view - I'm
> rather fond of the clean and simple URL structure of Koha... Would it be
> possible to store that information in a cookie/session? You say something about
> "avoid storing it in the session" - I'm not sure it should be avoided... ;-) 

The reason for "avoid storing it in the session" is to be able to take the url and paste it in another place and have the same result, if you have it in a session it's impossible, and with the session you have the obligued task of cleaning all the information you've stored, and it'll have to be made from all the pages you go from the opac-detail, a lot of more coding. But I'm open to all options. I thought it was the more simple solution.

> 
> 2. This is such a cool feature, I think it deserves a more prominent place than
> at the bottom of the screen, possibly below lots and lots of items. Could it be
> placed in the right hand column, as a new "box" between the existing two?
> (Below "more searches" and above "save record".)
Of course, I'll change it and present the new rendering.

Thanks for the positive response.
Comment 4 Magnus Enger 2011-06-08 13:52:41 UTC
(In reply to comment #3)
> The reason for "avoid storing it in the session" is to be able to take the url
> and paste it in another place and have the same result

Hm, that doesn't seem to work for me. I copied the URL for a detail page from Firefox and pasted it into Chromium, but the next and previous links do not show up there...

Here's the URL: http://localhost/cgi-bin/koha/opac-detail.pl?biblionumber=661&busc=eNo9zMEKwjAQBNBf8eRpD4ltNxskl%2F6IVJpCMDU1iVT%2F3iEFDw%2BGnWHD%2FHGP%2FTyt2%2FXlUompxbQs%0AxVene3U0b5%2B%2FmNGe50i55mebnlpZwrpFfzs2%2Fw8xlDqGewypOGZFzBp6GEDAEhvczAU6YkEWZEEv%0ADOgtsjVklIYOehja%2F5rqFJ0V%2FQNRVznG%0A
Comment 5 Owen Leonard 2011-06-08 14:12:54 UTC
(In reply to comment #3)
> The reason for "avoid storing it in the session" is to be able to take the url
> and paste it in another place and have the same result

That is a positive aspect of it, but I wonder if it isn't outweighed by the negative of the unfriendly URL. I think yes, but it might be a question to raise on the mailing list. I think it's much more likely that someone is going to want to share the URL of a page because of the item in question. I think the use case of wanting to share the item along with search context is less likely.

(In reply to comment #4)
> Hm, that doesn't seem to work for me. I copied the URL for a detail page from
> Firefox and pasted it into Chromium, but the next and previous links do not
> show up there...

It works for me.
Comment 6 Salva Zaragoza Rubio 2011-06-08 14:16:25 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > The reason for "avoid storing it in the session" is to be able to take the url
> > and paste it in another place and have the same result
> 
> Hm, that doesn't seem to work for me. I copied the URL for a detail page from
> Firefox and pasted it into Chromium, but the next and previous links do not
> show up there...
> 
> Here's the URL:
> http://localhost/cgi-bin/koha/opac-detail.pl?biblionumber=661&busc=eNo9zMEKwjAQBNBf8eRpD4ltNxskl%2F6IVJpCMDU1iVT%2F3iEFDw%2BGnWHD%2FHGP%2FTyt2%2FXlUompxbQs%0AxVene3U0b5%2B%2FmNGe50i55mebnlpZwrpFfzs2%2Fw8xlDqGewypOGZFzBp6GEDAEhvczAU6YkEWZEEv%0ADOgtsjVklIYOehja%2F5rqFJ0V%2FQNRVznG%0A

From what I see it's a fine url, but obviously I can't access you localhost ;)

I've tested it on my server and works the copying and pasting action for
Chromium, Opera, IE, Firefox, Safari. But maybe you've found a bug that I
didn't expect or foresee. Can you see your logs to narrow the problem?

Thanks
Comment 7 Salva Zaragoza Rubio 2011-06-08 14:23:10 UTC
(In reply to comment #5)
> (In reply to comment #3)
> > The reason for "avoid storing it in the session" is to be able to take the url
> > and paste it in another place and have the same result
> 
> That is a positive aspect of it, but I wonder if it isn't outweighed by the
> negative of the unfriendly URL. I think yes, but it might be a question to
> raise on the mailing list. I think it's much more likely that someone is going
> to want to share the URL of a page because of the item in question. I think the
> use case of wanting to share the item along with search context is less likely.
> 

Perfect, I'll change it as the community decides, I know it's an ugly url, so as I said before I'm open to all options.


> (In reply to comment #4)
> > Hm, that doesn't seem to work for me. I copied the URL for a detail page from
> > Firefox and pasted it into Chromium, but the next and previous links do not
> > show up there...
> 
> It works for me.
Comment 8 Magnus Enger 2011-06-08 14:23:33 UTC
> (In reply to comment #4)
> > Hm, that doesn't seem to work for me. I copied the URL for a detail page from
> > Firefox and pasted it into Chromium, but the next and previous links do not
> > show up there...
> 
> It works for me.

Sorry about that, it works for me too. (I forgot to switch from Norwegian to
English when testing - again...) 

Anyway, I do agree with Owen's comment that the benefit of this functionality
probably does not outweigh the "unfriendliness" of the URL.
Comment 9 Salva Zaragoza Rubio 2011-06-13 16:16:31 UTC Comment hidden (obsolete)
Comment 10 Magnus Enger 2011-06-15 10:09:10 UTC
I have tested the revised patch (attachment 4446 [details] [review]) and it works beautifully. I do have some comments, though: 

- the size of the text in the box on the right (Pagination: « previous next »
) should be a little larger, to match the text above and below it

- I would prefer if all the added functionality was in the box on the right, and nothing was below the items - but that might just be me? 

This is kind of a big change to how the OPAC detail view looks, so it would be good to have some more opinions! I'll attach a couple screen shots so others can see without applying the patch.
Comment 11 Magnus Enger 2011-06-15 10:10:18 UTC Comment hidden (obsolete)
Comment 12 Magnus Enger 2011-06-15 10:12:10 UTC Comment hidden (obsolete)
Comment 13 Salva Zaragoza Rubio 2011-06-15 10:28:06 UTC
(In reply to comment #10)
> I have tested the revised patch (attachment 4446 [details] [review]) and it works beautifully. I
> do have some comments, though: 
> 
> - the size of the text in the box on the right (Pagination: « previous next »
> ) should be a little larger, to match the text above and below it

You're right the font size doesn't match, I'll change the css to fix it.

> 
> - I would prefer if all the added functionality was in the box on the right,
> and nothing was below the items - but that might just be me? 

I've added only the simple functionality in the right box, and the same with
the paging list below the items to not overload the page at the top area.
But it can be modified to render it as the community decides.

> 
> This is kind of a big change to how the OPAC detail view looks, so it would be
> good to have some more opinions! I'll attach a couple screen shots so others
> can see without applying the patch.

Ok, good idea.
Comment 14 Nicole C. Engard 2011-06-15 15:11:53 UTC
I think that the back to results button or link should be in the top left, since that's where it is on most browsers and most popular websites, that's where I always look for a back option.  See WorldCat for an example (search for anything and click on any title, the link is in the top left to get back to the results.

Nicole
Comment 15 Owen Leonard 2011-06-15 15:57:27 UTC Comment hidden (obsolete)
Comment 16 Nicole C. Engard 2011-06-15 16:12:38 UTC
I approve of that image (since I previously said it should be elsewhere I figured I should mention that).
Comment 17 Salva Zaragoza Rubio 2011-06-16 12:11:19 UTC Comment hidden (obsolete)
Comment 18 Owen Leonard 2011-06-16 12:19:55 UTC
Created attachment 4496 [details]
Extracted first image from PDF: Pagination links
Comment 19 Owen Leonard 2011-06-16 12:20:40 UTC
Created attachment 4497 [details]
Extracted second image from PDF: Expanded results menu
Comment 20 Owen Leonard 2011-06-16 12:21:10 UTC
I think the latest proposal looks great.
Comment 21 Magnus Enger 2011-06-16 12:36:53 UTC
+1
Comment 22 Nicole C. Engard 2011-06-16 17:08:37 UTC
I LOVE IT!! +1
Comment 23 Salva Zaragoza Rubio 2011-06-20 09:01:58 UTC Comment hidden (obsolete)
Comment 24 Magnus Enger 2011-06-21 07:08:49 UTC Comment hidden (obsolete)
Comment 25 Ian Walls 2011-07-15 11:49:19 UTC
Agreed, these new screenshots do look really good.  Will test the functionality more in depth soon, but I wanted to provide this bit of feedback:

Should this be a system-preference controlled option?  Can we safely assume that all libraries will want this feature, since it's really neat, or should we err on the side of caution and make in an "opt-in" feature.

If a system preference was added, should it go in OPAC preferences, or Searching preferences?
Comment 26 Salva Zaragoza Rubio 2011-07-15 12:48:02 UTC
The idea was to enhance the usability and the navigation of the opac-detail page. We didn't create a new sytem preference because the patch doesn't show new information or add new content features, only links to a result already achieved.

But as always the patch can be modified to give the option of activate it through a system preference if the community thinks so.

I suggest the system preference to go in the Opac Preferences (Appearance) if it's decided that has to be modified.
Comment 27 Magnus Enger 2011-07-15 13:54:18 UTC
I'd say get the feature in there and let's worry about the syspref later if anyone complains...
Comment 28 Katrin Fischer 2011-07-15 13:58:40 UTC
It's quite a big change and because it's OPAC and not staff I would like to see a system preference. Perhaps a compromise could be to turn it ON by default instead of OFF? :)
Comment 29 Owen Leonard 2011-07-15 16:30:19 UTC
To hide this using OPACUserCSS you would add this:

.nav_results { display: none; }
Comment 30 Katrin Fischer 2011-07-15 16:55:07 UTC
Good point :)
Comment 31 Ian Walls 2011-07-15 20:18:26 UTC
As per discussion in IRC, the need for a system preference to control this can be put off onto another ticket, since for anyone who wants to hide this, they can use the CSS provided.
Comment 32 Paul Poulain 2011-08-11 16:31:04 UTC
(In reply to comment #31)
> As per discussion in IRC, the need for a system preference to control this can
> be put off onto another ticket, since for anyone who wants to hide this, they
> can use the CSS provided.

+1 from me for this option or not creating a syspref
join me at the LASTCEBABTAASM (the "league against sysprefs that can easily be avoided, because there are already so many")
Comment 33 Paul Poulain 2011-08-11 16:38:48 UTC
QA comment

This patch does not apply anymore, there is a conflict on opac.css

I think it's easy to solve, but I don't want to do something wrong, so I mark "does not apply". Salva, please rebase & fix the problem, i'll have a look ASAP, I agree it's a great feature.
Comment 34 Paul Poulain 2011-08-11 16:41:19 UTC
Other comment : it seems this ENH has not been announced on the wiki, through a RFC. Am I right ?
Comment 35 Salva Zaragoza Rubio 2011-08-12 07:13:49 UTC Comment hidden (obsolete)
Comment 36 Salva Zaragoza Rubio 2011-08-12 07:32:00 UTC
(In reply to comment #34)
> Other comment : it seems this ENH has not been announced on the wiki, through a
> RFC. Am I right ?

You're right, I didn't think it was worth adding as is a minor enhancement, without database modification or large number of pages affected. But I'll add a RFC if you consider it necessary.

BTW, looking at bugs I think it might cause a confict with bug 6120 (http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6120) as both add the back to search functionality in the same place. I see that bug 6120 doesn't add paging, only the back to results button. We'll have to evaluate whether both bugs can coexist, a merge is done or one is rejected.
Comment 37 Paul Poulain 2011-08-12 07:45:22 UTC
(In reply to comment #36)
> (In reply to comment #34)
> > Other comment : it seems this ENH has not been announced on the wiki, through a
> > RFC. Am I right ?
> 
> You're right, I didn't think it was worth adding as is a minor enhancement,
> without database modification or large number of pages affected. But I'll add 
> a RFC if you consider it necessary.

well, IMO :
* it's not a minor ENH ! And I just discovered the feature when QAing it. The bad news is that BibLibre just developed something very similar to this one. Lack of coordination/communication :(( (see my RM for 3.8/4.0 application on the wiki )
* in RFC, the C means 'Comment'. Once it's done, there are no real comments we can do. I would vote to continue without RFCs on the wiki.

> BTW, looking at bugs I think it might cause a confict with bug 6120
> (http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6120) as both add the
> back to search functionality in the same place. I see that bug 6120 doesn't add
> paging, only the back to results button. We'll have to evaluate whether both
> bugs can coexist, a merge is done or one is rejected.

Well spotted ! i've removed the "passed QA" to bug 6120, we must discuss what we do with those conflicting developments. My personal preference would be to deal with your patch, as he does more things.
Comment 38 Frédéric Demians 2011-08-24 13:24:55 UTC Comment hidden (obsolete)
Comment 39 Frédéric Demians 2011-08-24 13:27:12 UTC
There is a bug with search containing accented characters. For example, if you search 'café', the 'busc' variable used to create 'back to results' link contain: café
Comment 40 Owen Leonard 2011-08-24 13:29:01 UTC
Shouldn't that bug be fixed before this gets a signoff?
Comment 41 Frédéric Demians 2011-08-24 13:37:08 UTC
> Shouldn't that bug be fixed before this gets a signoff?
'
I'd expect a following patch from the (great) enhancement submitter. He is in situation to fix it quickly. If he can't, he could leave a note here, and I may try...
Comment 42 Salva Zaragoza Rubio 2011-08-24 20:51:05 UTC
I can't seem to recreate the bug reported by Frédéric with the accented characters. I've tested the patch with Koha running on servers OpenBSD 4.9 and FreeBSD 8.2 without success.

OS FreeBSD:
-----------

Table sessions in mysql:

mysql> show create table sessions;

| sessions | CREATE TABLE `sessions` (
  `id` varchar(32) NOT NULL,
  `a_session` text NOT NULL,
  UNIQUE KEY `id` (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 |


mysql> SELECT CCSA.character_set_name FROM information_schema.`TABLES` T,
    ->        information_schema.`COLLATION_CHARACTER_SET_APPLICABILITY` CCSA
    -> WHERE CCSA.collation_name = T.table_collation AND T.table_name = 'sessions';
+--------------------+
| character_set_name |
+--------------------+
| utf8               |
+--------------------+

Query: Café

busc variable on session: busc: "q=Caf\xC3\xA9&query=kw,wrdl,rtrn=Caf\xC3\xA9 &count=10&simple_query=Caf\xC3\xA9&listBiblios=29,30&total=2"

Link back to results:

Opera on PCBSD: http://192.168.1.12:8079/cgi-bin/koha/opac-search.pl?q=Caf%C3%A9
SeaMonkey on PCBSD: http://192.168.1.12:8079/cgi-bin/koha/opac-search.pl?q=Caf%C3%A9
Chromium on PCBSD: http://192.168.1.12:8079/cgi-bin/koha/opac-search.pl?q=Caf%C3%A9


When returns always shows Café with the accent fine.

OS OpenBSD:
-----------

mysql> show create table sessions;

| sessions | CREATE TABLE `sessions` (
  `id` varchar(32) NOT NULL,
  `a_session` text NOT NULL,
  UNIQUE KEY `id` (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 |


mysql> SELECT CCSA.character_set_name FROM information_schema.`TABLES` T,
    ->        information_schema.`COLLATION_CHARACTER_SET_APPLICABILITY` CCSA
    -> WHERE CCSA.collation_name = T.table_collation AND T.table_name = 'sessions';
+--------------------+
| character_set_name |
+--------------------+
| utf8               |
+--------------------+

Query: Sé

busc variable on session: busc: "q=S\xC3\xA9&query=kw,wrdl,rtrn=S\xC3\xA9 &count=20&simple_query=S\xC3\xA9&listBiblios=25,24,23,21,18,17,16,12,7,6,4,2&total=12"

Link back to results:

Opera on PCBSD: http://192.168.1.14:8079/cgi-bin/koha/opac-search.pl?q=S%C3%A9
SeaMonkey on PCBSD: http://192.168.1.14:8079/cgi-bin/koha/opac-search.pl?q=S%C3%A9
Chromium on PCBSD: http://192.168.1.14:8079/cgi-bin/koha/opac-search.pl?q=S%C3%A9


When returns always shows Sé with the accent fine.


I've seen that café is the utf8 codification of Café, maybe there's a codification in some place, but in the patch there is not one.

[salzaru@pcbsd /usr/home/salzaru]$ echo "Café" | iconv -t UTF-8 -f ISO-8859-1 
Café

[salzaru@pcbsd /usr/home/salzaru]$ perl -e 'use Encode; print (encode("utf8", "Café") . chr(10));'
Café

[salzaru@pcbsd /usr/home/salzaru]$ perl -e 'use Encode; print (decode("utf8", "café") . chr(10));'
café

Any ideas?
Comment 43 Frédéric Demians 2011-08-25 16:08:33 UTC
Thanks for your quick and detailed reply.

> Query: Café

> busc variable on session: busc:

> "q=Caf\xC3\xA9&query=kw,wrdl,rtrn=Caf\xC3\xA9
> &count=10&simple_query=Caf\xC3\xA9&listBiblios=29,30&total=2"

I haven't Caf\xC3\xA9 but Caf\xC3\x83\xC2\xA9. 

And I have the same sessions table charset: utf8. So something get wrong
when writing the busc variable. I can reproduce the bug on whatever web
browser.
Comment 44 Owen Leonard 2011-08-25 16:10:53 UTC
I can't reproduce that bug either:

<li class="back_results"><a title="Back to the results search list" href="opac-search.pl?q=café ">Back to results</a></li>
Comment 45 Ian Walls 2011-10-19 17:55:56 UTC
Created attachment 5988 [details] [review]
Passed QA patch (rebased on current master)

Patch had minor conflict in opac.css (resolved now).

Marking as Passed QA
Comment 46 Chris Cormack 2011-10-20 15:59:41 UTC
Pushed, please test
Comment 47 Owen Leonard 2011-10-24 12:26:48 UTC
*** Bug 6120 has been marked as a duplicate of this bug. ***
Comment 48 Owen Leonard 2011-10-27 16:46:54 UTC
*** Bug 3075 has been marked as a duplicate of this bug. ***
Comment 49 Owen Leonard 2011-10-27 17:07:14 UTC
This is working great in production.
Comment 50 Chris Cormack 2012-01-03 07:10:55 UTC
(In reply to comment #49)
> This is working great in production.

Except for bug 7394