Bug 9368

Summary: specific behavior of yr and acqdate indexes
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: SearchingAssignee: David Cook <dcook>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: dcook, jonathan.druart, katrin.fischer, mathsabypro, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12742
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Proposed patch
Bug 9368: specific behavior of yr and acqdate indexes
Bug 9368: specific behavior of yr and acqdate indexes
Bug 9368 - do not change if QueryParser
Bug 9368 [ALTERNATE] - specific behavior of yr and acqdate indexes
Bug 9368 [ALTERNATE] - specific behavior of yr and acqdate indexes
[PASSED QA] Bug 9368 [ALTERNATE] - specific behavior of yr and acqdate indexes

Description Fridolin Somers 2013-01-08 16:59:41 UTC
When searching on date of publication or date of acquisition index from advanced search, there are several bugs :
- The search is performed on no index, using only search therm.
- The next or previous pages lead to an empty result page.

This comes from specific behavior in C4/Search.pm.
Comment 1 Fridolin Somers 2013-01-08 17:08:17 UTC Comment hidden (obsolete)
Comment 2 Fridolin Somers 2013-01-08 17:08:39 UTC
See test plan in commit message
Comment 3 Galen Charlton 2013-02-06 16:56:13 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2013-03-18 15:37:31 UTC
QA comment:

Before this patch, if the "publication date" index is selected on the adv search form, zebra requests on 
  @attrset Bib-1 2000
The index is not defined!

After applying it:
  @attrset Bib-1 @attr 1=pubdate @attr 4=109 @attr 2=3 2000

Which is what the user want.

Marked as Passed QA.
Comment 5 Jonathan Druart 2013-03-18 15:37:53 UTC Comment hidden (obsolete)
Comment 6 Jared Camins-Esakov 2013-03-22 02:58:32 UTC
This breaks the yr and acqdate indexes when QP is enabled. I will provide a counter-patch or follow-up once we are in feature freeze (assigning to myself so that I don't forget).
Comment 7 Fridolin Somers 2013-10-23 12:38:40 UTC
(In reply to Jared Camins-Esakov from comment #6)
> This breaks the yr and acqdate indexes when QP is enabled. I will provide a
> counter-patch or follow-up once we are in feature freeze (assigning to
> myself so that I don't forget).
If QP is enabled, those special behavior for yr and acqdate are usless non ?
May we add a "unless useQueryParser" condition ?
Comment 8 Fridolin Somers 2013-10-23 12:39:08 UTC
(In reply to Jared Camins-Esakov from comment #6)
> This breaks the yr and acqdate indexes when QP is enabled. I will provide a
> counter-patch or follow-up once we are in feature freeze (assigning to
> myself so that I don't forget).
If QP is enabled, those special behavior for yr and acqdate are usless non ?
May we add a "unless useQueryParser" condition ?
Comment 9 Katrin Fischer 2013-10-27 14:42:25 UTC
Hi Jared, can you submit a follow-up for the QueryParser?
Comment 10 Mathieu Saby 2013-12-01 12:57:27 UTC
Hello
I discovered the problem recently and signaled it to Biblibre, who pointed to this bug...
Not a big issue, but it must be fixed, for QP and non-QP instances...

Maybe an alternative could be to suppress the ability to select a date with the dropdown list (I'm not sure that's very intuitive), and just use the special input field made for filtering by dates.

Mathieu
Comment 11 Fridolin Somers 2014-01-23 16:15:23 UTC
(In reply to mathieu saby from comment #10)
> Maybe an alternative could be to suppress the ability to select a date with
> the dropdown list (I'm not sure that's very intuitive), and just use the
> special input field made for filtering by dates.
+1
Comment 12 Fridolin Somers 2014-03-27 16:40:00 UTC Comment hidden (obsolete)
Comment 13 Fridolin Somers 2014-03-27 16:42:26 UTC
@Jared
Here is the follow-up to change code only if not QP on.

"Bug 9368 - do not change if QueryParser" needs signoff
Comment 14 David Cook 2014-06-18 02:43:21 UTC
It looks to me that "Publication Date (yyyy)" searching is already broken in master (before applying these patches) when using QueryParser.

I get a query like this:

@attr 2=102 @or @or @attr 1=1016 @attr 5=1 @attr 4=6 "yr:2012" @attr 9=20 @attr 2=102 @attr 5=1 @attr 4=6 "yr:2012" @attr 9=34 @attr 2=102 @attr 5=1 @attr 4=6 "yr:2012"

It's trying to do a keyword search with the keyword string as "yr:2012".

It's worth noting that the "Bug 9368 - do not change if QueryParser" patch doesn't fix this.

--

I have a counter-patch that I will be posting in a moment, which will fix the non-QueryParser search, and which will cause QueryParser to fallback to non-QueryParser mode instead of breaking.

(It seems to me that QueryParser needs a lot more work before its able to replace the existing buildQuery, unfortunately.)
Comment 15 David Cook 2014-06-18 03:54:37 UTC Comment hidden (obsolete)
Comment 16 David Cook 2014-06-18 03:56:46 UTC
Btw, it also fixes the paging issue.
Comment 17 David Cook 2014-06-18 03:59:05 UTC
I'm going to assign this to myself, since I think my alternate patch solves the key problems of this issue.

I have a few other bugs out there which mention QueryParser problems, and I'm hoping to unite those some point down the road
Comment 18 Fridolin Somers 2014-07-30 09:18:57 UTC Comment hidden (obsolete)
Comment 19 Fridolin Somers 2014-07-30 09:21:37 UTC
Works great, thanks David.

I've tested search on year range (yr:1999-2001) and using next/previous page in results. Everything is ok.
Comment 20 Kyle M Hall 2014-09-12 13:29:53 UTC
Created attachment 31552 [details] [review]
[PASSED QA] Bug 9368 [ALTERNATE] - specific behavior of yr and acqdate indexes

This patch fixes the "Publication date" and "Acquisition date"
searches when using non-QueryParser and QueryParser searches.

It adds structure attributes to the template, which is consistent
with how phrase searching is currently handled.

It removes unnecessary code from Search.pm, adds some necessary
code which is consistent with existing code, and adds a lot of
explanatory comments.

_TEST PLAN_

Before applying:
0) Turn off QueryParser
1) Try a "Publication date" or "Acquisition date" search from the
staff client advanced search.
2) Note that even though the description on the result page makes
it seem like you're doing an index-specific search, you're actually
doing a keyword search. You can verify this by checking the 008
from positions 7 to 10 for "Publication date" or "Date accessioned"
on items for "Acquisition date".

3) Turn on QueryParser
4) Try doing the searches from Step 1.
5) A "Publication date" search should probably produce zero results

After applying patch:
6) Keep QueryParser on
7) Try doing the searches from Step 1.
8) Notice that you're actually getting results consistent with
your search (ie the 008/7-10 shows the date you searched for,
and there is a "Date accessioned" in items which matches your
search)

9) Turn off QueryParser
10) Note that your results are exactly the same as step 8
(N.B. this is because QueryParser is falling back to non-QP mode
instead of producing a bad query.)

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Tomás Cohen Arazi 2014-10-22 18:24:54 UTC
Patch pushed to master.

Thanks David!