Bug 5981

Summary: OPAC: Add limits to search history
Product: Koha Reporter: Chris Cormack <chris>
Component: OPACAssignee: Alex Arnaud <alex.arnaud>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: blocker    
Priority: P1 - high CC: claire.hernandez, jonathan.druart, katrin.fischer, koha.sekjal, m.de.rooy, nengard, oleonard, paul.poulain
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 8381    
Bug Blocks: 5584    
Attachments: Proposed Patch
Followup - change Location to Shelving Location
proposed patch for T::TT + fix for missing bracket
proposed patch (updated for Nicole comment about updatedatabase XXX)
proposed patch, rebased, with QA problems fixed
Proposed patch
Signed-off patches, squashed
Bug 5981 - OPAC: Add limits to search history
[SIGNED-OFF] Bug 5981 - OPAC: Add limits to search history
[SIGNED-OFF] Bug 5981 - OPAC: Add limits to search history

Description Chris Cormack 2011-03-28 23:47:59 UTC

    
Comment 1 Chris Cormack 2011-03-28 23:55:35 UTC Comment hidden (obsolete)
Comment 2 Magnus Enger 2011-04-05 10:00:16 UTC Comment hidden (obsolete)
Comment 3 Magnus Enger 2011-04-05 10:03:15 UTC
Comment on attachment 3657 [details] [review]
Followup - change Location to Shelving Location

Attached the patch to the wrong bug, sorry!
Comment 4 claire.hernandez@biblibre.com 2011-04-05 13:46:18 UTC
Patch applies but I have:
Missing right curly or square bracket at /home/koha/versions/koha_community/opac/opac-search.pl line 759, at end of line
syntax error at /home/koha/versions/koha_community/opac/opac-search.pl line 759, at EOF
Execution of /home/koha/versions/koha_community/opac/opac-search.pl aborted due to compilation errors.

~/cgi-bin/koha/opac-search.pl
Comment 5 Paul Poulain 2011-08-01 16:17:43 UTC Comment hidden (obsolete)
Comment 6 Nicole C. Engard 2011-08-02 23:55:13 UTC
Can someone provide an explanation of what this patch does and a test plan? What am I looking for when I apply this patch?

Nicole
Comment 7 Paul Poulain 2011-08-03 07:53:36 UTC
Nicole : before this patch, if you do an advanced search, with limits like branch/itemtype, it was not saved correctly in the search history, so if you re-do a search from there you won't get the same results.

After this patch = it works correctly

Updating severity, it's a bug not an ENH !
Comment 8 Nicole C. Engard 2011-08-03 16:11:09 UTC
This patch should not have a number in the updatedatabse:

+$DBversion = "3.05.00.005";

It should be 

+$DBversion = "3.05.00.XXX";

The RM is the one who chooses the next number.
Comment 9 Paul Poulain 2011-08-05 10:05:24 UTC Comment hidden (obsolete)
Comment 10 Ian Walls 2011-08-12 17:18:49 UTC
The current iteration of the patch is not signed off...
Comment 11 Nicole C. Engard 2011-08-12 19:23:54 UTC
Yes it was, I signed off on it as soon as Paul sent in the updated patch.

http://lists.koha-community.org/pipermail/koha-patches/2011-August/015586.html

Nicole
Comment 12 Ian Walls 2011-08-12 19:37:43 UTC
Changes in C4/Auth.pm include references to system preferences that do not exist (TemplateEncoding, IntranetXSLTResultsDisplay and IntranetXSLTDetailsDisplay) and are not used anywhere else in the patch.

Also defines a subroutine, enabled_opac_search_views, that is not used in the patch.

There is also something in here about Tags, which appears to be outside the scope of this patch.

Because of all this unnecessary inclusion, I'm marking this as Failed QA.  Please strip this patch down to only deal with the issue presented in this bug report, and file separate bug reports and patches for separate issues.
Comment 13 Paul Poulain 2011-09-01 16:40:13 UTC
I've checked the patches on biblibre/master (2), they are much smaller than this one. That should be a merge/split mistake, will submit a fixed version asap
Comment 14 Paul Poulain 2011-09-02 08:55:37 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2011-09-02 09:12:20 UTC
GBSD#4 Looking at this one.
Comment 16 Marcel de Rooy 2011-09-02 09:47:10 UTC
If I search (coming from Advanced Search) with a limit on year:

cgi-bin/koha/opac-search.pl?idx=kw&q=vermeer&idx=kw&idx=kw&do=Zoek&limit-yr=-2005&sort_by=pubdate_dsc

the limit is saved in the database (looking via sql), but is not applied to the search when I run it again from the search history form, giving different results:
cgi-bin/koha/opac-search.pl?idx=kw&q=vermeer
I would expect something with: kw,wrdl: vermeer yr,st-numeric=-2005

Changing status
Comment 17 Paul Poulain 2011-09-05 15:36:30 UTC
Alex has worked on this and should send an updated patch soon
Comment 18 Alex Arnaud 2011-09-14 08:38:09 UTC Comment hidden (obsolete)
Comment 19 Paul Poulain 2011-09-28 14:21:19 UTC
*** Bug 4029 has been marked as a duplicate of this bug. ***
Comment 20 Owen Leonard 2011-09-30 12:46:26 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2011-11-14 11:33:08 UTC
This patch is already SIGNED, but unfortunately does not apply. Please rebase.

Applying: Bug 5981 : (MT 4231) Adds limits to search history
/usr/share/koha/testclone/.git/rebase-apply/patch:101: trailing whitespace.
        ADD `limit_cgi` VARCHAR( 255 ) NULL DEFAULT NULL AFTER `limit_desc`
warning: C4/Auth.pm has type 100755, expected 100644
error: patch failed: C4/Auth.pm:130
error: C4/Auth.pm: patch does not apply
error: patch failed: installer/data/mysql/updatedatabase.pl:4446
error: installer/data/mysql/updatedatabase.pl: patch does not apply
Using index info to reconstruct a base tree...
<stdin>:101: trailing whitespace.
        ADD `limit_cgi` VARCHAR( 255 ) NULL DEFAULT NULL AFTER `limit_desc`
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merged C4/Auth.pm
CONFLICT (content): Merge conflict in C4/Auth.pm
Auto-merged C4/Search.pm
Auto-merged installer/data/mysql/kohastructure.sql
Auto-merged installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merged koha-tmpl/opac-tmpl/prog/en/modules/opac-search-history.tt
Auto-merged opac/opac-search.pl
Failed to merge in the changes.
Comment 22 Alex Arnaud 2012-06-10 11:15:20 UTC Comment hidden (obsolete)
Comment 23 Alex Arnaud 2012-06-10 11:17:31 UTC
This patch does the same thing than the previous one but it should apply.
Comment 24 Katrin Fischer 2012-06-16 10:20:11 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2012-07-04 12:50:50 UTC
Created attachment 10632 [details] [review]
[SIGNED-OFF] Bug 5981 - OPAC: Add limits to search history

Rebased patch 

+ reindentation (replaces \t with 4 spaces)

Works as expected.
Marking as Passed QA
Comment 26 Chris Cormack 2012-07-04 19:31:27 UTC
Bugfix, pushed to 3.8.x will be in 3.8.3
Comment 27 Chris Cormack 2012-07-07 18:18:55 UTC
Reverted out of 3.8.x this breaks the opac search navigation, see bug 8381.
Comment 28 Katrin Fischer 2012-07-08 10:11:52 UTC
There are some strange side effects of this patch on master too. It all resumes to working normally as soon as I turn opacsearchhistory off or revert this patch out. 

Symptoms:
- Every search seems to freeze the browser, but after a minute or so the search result page appears
- Paging works for me, but at a 1 page/minute speed

I tried lots of different things before I figured out it was this patch making the difference:
- rebuilt my indexes several times
- ran Makefile to make sure I have all current zebra files
- changed from grs to dom indexing
- changed to icu 

None of that worked.

I have no idea what's going on here, but I think reverting out of master would be good until we figure it out.
Comment 29 Paul Poulain 2012-07-09 11:42:05 UTC
(In reply to comment #28)
> There are some strange side effects of this patch on master too. It all
> resumes to working normally as soon as I turn opacsearchhistory off or
> revert this patch out. 
> 
> Symptoms:
> - Every search seems to freeze the browser, but after a minute or so the
> search result page appears
> - Paging works for me, but at a 1 page/minute speed
Katrin,

I can't reproduce your problem, it works well for me, I just tested again. Is it still relevant for you and do you have something in your setup (plack or no ? zebra DOM or GRS1 ? how many records indexed ?adv search is working or also very slow ? anything in your logs ?

(I'm also Investigating 8381)
Comment 30 Katrin Fischer 2012-07-09 12:04:09 UTC
Hi Paul, 

nothing in the logs - I know, there should probably be something.

I started with grs, switched to DOM and ICU. The problem remained.

I did a keyword search for "der" (German article).

Staff search worked without any problems (no search history), the problem only occured in OPAC and vanished after deactivating the search history or reverting the patch. 

I did sign off on this patch and I am sure it was not there when I initially tested it - but it's a big problem now. I suspect something changed in between that is causing the problem.
Comment 31 Paul Poulain 2012-07-09 12:07:45 UTC
(In reply to comment #30)

> I did sign off on this patch and I am sure it was not there when I initially
> tested it - but it's a big problem now. I suspect something changed in
> between that is causing the problem.

really weird... let's wait until Alex find a fix for bug 8381, maybe they are related ? (even if I don't see how they could be)
Comment 32 Katrin Fischer 2012-07-09 12:13:50 UTC
No Plack, but memcache - restarted it several times. Maybe that is it?
Comment 33 Paul Poulain 2012-07-09 12:18:14 UTC
(In reply to comment #32)
> No Plack, but memcache - restarted it several times. Maybe that is it?

Should not be memcache, but I have no memcache by default. Could you try after shutting down memcache server ?
Comment 34 Katrin Fischer 2012-07-09 12:21:01 UTC
I can try that tonight.
Comment 35 Katrin Fischer 2012-07-09 18:16:31 UTC
Hi Paul and Jonathan,

I checked the search worked for me on current master with search history deactivated.

Next I stopped the memcache server and reactivated the search history using the system preference.

I still see the same systems - the browser seems to freeze, it takes over a minute, then I get to see a result list. 

I noticed something else, that is weird: on the result list the first line with the title of the first result on the result list is missing. It's also missing on other the other pages of the same search.

XSLT results list is activated and used.
Comment 36 Paul Poulain 2012-07-17 13:18:48 UTC
Pushed to master, follow-up in bug 8381 also pushed, resolving this one, chris_c, you can push both on 3.8.x I think
Comment 37 Jared Camins-Esakov 2012-07-17 16:17:08 UTC
Either this patch or the patch to bug 8381 has broken the OPAC search in Chrome. With EnableOPACSearchHistory enabled, Chrome crashes on a regular keyword search (q=smith), and hangs on searches using idx=kw&q=smith. Disabling EnableOPACSearchHistory fixes the problem.
Comment 38 Paul Poulain 2012-07-23 08:37:40 UTC
(In reply to comment #37)
> Either this patch or the patch to bug 8381 has broken the OPAC search in
> Chrome. With EnableOPACSearchHistory enabled, Chrome crashes on a regular
> keyword search (q=smith), and hangs on searches using idx=kw&q=smith.
> Disabling EnableOPACSearchHistory fixes the problem.

mmm... strange, I can't reproduce this problem.

I use chromium shipped with Ubuntu: 18.0.1025.168 (Build de développement 134367 Linux) Ubuntu 12.04

I don't think i've any plugin installed (I don't use chrome daily)

I played with some searches, history page, running a search from history,...

Could you describe exactly what you were doing ?
Comment 39 Chris Cormack 2012-08-24 02:24:27 UTC
Jared could you add some more information or, close this if the problem has gone away in master please.
Comment 40 Jared Camins-Esakov 2012-08-24 02:35:19 UTC
The hanging is no longer an issue on Master.