Bug 10542

Summary: QueryParser + OpacSuppression doesn't allow search in 'all libraries'
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: SearchingAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact:
Severity: critical    
Priority: P1 - high CC: bgkriegel, dcook, f.demians, jcamins, martin.renvoize, mathsabypro, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12738
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 12742    
Attachments: Bug 10542: Fix QueryParser with OpacSupression
Attachment to Bug 10542 - QueryParser + OpacSuppression doesn't allow search in 'all libraries'
Bug 10542: Fix QueryParser with OpacSupression
Bug 10542: Fix QueryParser with OpacSupression
Bug 10542: Fix QueryParser with OpacSupression
Bug 10542: Fix QueryParser with OpacSupression
Bug 10542: Fix QueryParser with OpacSupression
Bug 10542 [Follow-up] - QueryParser + OpacSuppression doesn't allow search in 'all libraries'
[SIGNED-OFF] Bug 10542: Fix QueryParser with OpacSupression
[SIGNED-OFF] Bug 10542: QueryParser + OpacSuppression doesn't allow search in 'all libraries'
[PASSED QA] Bug 10542: Fix QueryParser with OpacSupression
[PASSED QA] Bug 10542: QueryParser + OpacSuppression doesn't allow search in 'all libraries'

Description Katrin Fischer 2013-07-05 08:11:18 UTC
When QueryParser is activated and OpacSuppression is used, you don't find any results for "All libraries" (pull down next to simple search in OPAC).
Comment 1 Galen Charlton 2013-10-24 15:13:46 UTC
Here is the error message that gets logged when I try such a search with both UseQueryParser and OpacSuppression on:

opac-search.pl: WARNING: query problem with (@attr 2=102 @or @or @attr 1=1016 @attr 5=1 @attr 4=6 "history" @attr 9=20 @attr 2=102 @attr 5=1 @attr 4=6 "history" @attr 9=34 @attr 2=102 @attr 5=1 @attr 4=6 "history") not Suppress=1 ZOOM error 20003 "can't set prefix query" (addinfo: "(@attr 2=102 @or @or @attr 1=1016 @attr 5=1 @attr 4=6 "history" @attr 9=20 @attr 2=102 @attr 5=1 @attr 4=6 "history" @attr 9=34 @attr 2=102 @attr 5=1 @attr 4=6 "history") not Suppress=1") from diag-set 'ZOOM' at /home/gmc/koha/C4/Search.pm line 378., referer: http://opac.zadi.librarypolice.com/cgi-bin/koha/opac-search.pl?idx=&q=history&branch_group_limit=

I'm bumping up the severity to critical as this would be a blocker to making QueryParser turned on by default for new installations.
Comment 2 Galen Charlton 2013-10-24 16:33:38 UTC
Poking at this more, the immediate cause of the problem is that tacking on 'not Suppress=1' to the end of a PQF query generated by QP is a syntax error.  That can be addressed by munging the input to buildQuery(), but I've run into a roadblock: the PQF QueryParser driver needs to be taught how to use the negation operator.
Comment 3 Katrin Fischer 2013-11-22 13:17:36 UTC
Hi Galen, 

this doesn't sound good:
>PQF QueryParser driver needs to be taught how to use the negation operator.

How bad is it?
Comment 4 Martin Renvoize 2014-03-12 17:46:53 UTC Comment hidden (obsolete)
Comment 5 Martin Renvoize 2014-03-12 17:47:58 UTC
Many thanks for the hints form Jared on this one.  I'm sure this can be more cleanly implimented, but wanted to post up a patch asap for others to comment upon and test.
Comment 6 Jesse Maseto 2014-03-13 10:39:32 UTC Comment hidden (obsolete)
Comment 7 Martin Renvoize 2014-03-13 14:00:40 UTC Comment hidden (obsolete)
Comment 8 Dobrica Pavlinusic 2014-03-13 15:38:29 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2014-03-14 13:30:59 UTC
In my tests the search was no longer blocked, but the suppressed records showed up in OPAC. I have to fail this :(
Comment 10 Martin Renvoize 2014-03-18 11:29:58 UTC
Just an Update: My approach was way to simplistic, and I'm still looking into this, but it may take a little longer than hoped.
Comment 11 David Cook 2014-07-17 07:24:50 UTC
Here's the start of the solution:

Add the following line of code to the OpacSuppression code block:

$query = '@not '.$query.' @attr 1=9011 1';

if (C4::Context->preference('OpacSuppression')) {
    # OPAC suppression by IP address
    if (C4::Context->preference('OpacSuppressionByIPRange')) {
        my $IPAddress = $ENV{'REMOTE_ADDR'};
        my $IPRange = C4::Context->preference('OpacSuppressionByIPRange');
        if ($IPAddress !~ /^$IPRange/)  {
            $query = "($query) not Suppress=1";
        }
    }
    else {
        #$query = "($query) not Suppress=1";
        $query = '@not '.$query.' @attr 1=9011 1';
    }
}

Now...we can't comment out [$query = "($query) not Suppress=1";]. That's just for demonstration purposes.

Unfortunately, we can't use the QP syspref to detect whether we should use the CCL version or the PQF version, because buildQuery can output both depending on the query that is input into it.

So...the hack here is to use some regex to detect "@attr" in the outbound query probably. You can get fancier than that.

Even better...changing buildQuery, so that it returns what query language it's using! Then no hacking necessary.

I can probably do a patch for this sometime when I'm less pressed for time.
Comment 12 David Cook 2014-07-17 07:50:30 UTC
Hmm, now I'm unsure about my answer due to the order of operations...

Example from IndexData (http://www.indexdata.com/yaz/doc/tools.html#PQF)

@or "dylan" "zimmerman"

@and @or dylan zimmerman when

@and when @or dylan zimmerman

It seems like you always need to have your operators at the beginning...and that it figures out the order on its own. 

In the 3rd case, not all the operators are at the beginning, but they are before the "pair" of operands on which they're operating.

Here's my query that worked:

@not @or @or @or @attr 1=1016 @attr 5=1 @attr 4=6 "e" @attr 9=75 @attr 2=102 @attr 5=1 @attr 4=6 "e" @attr 9=20 @attr 2=102 @attr 5=1 @attr 4=6 "e" @attr 9=34 @attr 2=102 @attr 5=1 @attr 4=6 "e" @attr 1=9011 1

Yet...consider the following:

  prefixQuery("dinosaur");
    CCLQuery("dinosaur");

    prefixQuery("@and complete dinosaur");
    CCLQuery("complete and dinosaur");

    prefixQuery("@and complete @or dinosaur pterosaur");
    CCLQuery("complete and (dinosaur or pterosaur)");

    prefixQuery("@attr 1=7 0253333490");
    CCLQuery("isbn=0253333490");
   

Even these examples don't make sense but it's probably something like...

"e and not e or e or e or suppress=1"

Maybe... "e and not (e or e or e or suppress=1)"

Which...will hide the suppressed records...but it'll also kill those other "e" subqueries...
Comment 13 David Cook 2014-07-17 08:08:04 UTC
I'm looking again at this example:

 prefixQuery("@and complete @or dinosaur pterosaur");
    CCLQuery("complete and (dinosaur or pterosaur)");

And considering the PQF grammar:

PQF Grammar

    complex ::= operator query-struct query-struct.

http://www.indexdata.com/yaz/doc/tools.html#PQF

If it is "reverse polish notation" (ie RPN)...

Then the following:

@not 
@or
@or
@or 
@attr 1=1016 @attr 5=1 @attr 4=6 "e" 
@attr 9=75 @attr 2=102 @attr 5=1 @attr 4=6 "e" 
@attr 9=20 @attr 2=102 @attr 5=1 @attr 4=6 "e" 
@attr 9=34 @attr 2=102 @attr 5=1 @attr 4=6 "e" 
@attr 1=9011 1

Becomes:

@attr 1=1016 @attr 5=1 @attr 4=6 "e" 
@not 
@attr 9=75 @attr 2=102 @attr 5=1 @attr 4=6 "e" 
@or
@attr 9=20 @attr 2=102 @attr 5=1 @attr 4=6 "e" 
@or
@attr 9=34 @attr 2=102 @attr 5=1 @attr 4=6 "e" 
@or 
@attr 1=9011 1

OR 

kw,wrdl,rt=e and not (kw,wrdl,rt=e or (kw,wrdl,rt=e or (kw,wrdl,rt=e or suppress=1)))

Well...there is also relevance ranking in there but I got lazy...

I'm not sure how to make the query structure into:

(kw,wrdl,rt=e or kw,wrdl,rt=e or kw,wrdl,rt=e or kw,wrdl,rt=e) and not (suppress=1)

I know you can do grouping with CCL but how that works with PQF...I have no idea.
Comment 14 David Cook 2014-07-17 08:12:12 UTC
I see this too in the grammar, but I don't understand how to read grammars well enough to understand:

query-struct ::= attr-spec | simple | complex | '@term' term-type query
Comment 15 David Cook 2014-07-17 08:20:42 UTC
I wonder if these are supposed to be equivalent...


@and when @or dylan zimmerman

becomes...

when and (dylan or zimmerman)


So what does that mean for the following?

@and @or dylan zimmerman when

Does it become:

1) "dylan" and "zimmerman" or "when"
2) "dylan" and ("zimmerman" or "when")
3) ("dylan" or "zimmerman") and "when"

I suppose the only way is to read YAZ source code or to do more tests...
Comment 16 David Cook 2014-07-18 00:23:08 UTC
So I have 5 records: "A", "B", "C", "D", and "E".

[As of this writing, all of them return 1 record except D which returns all 5...should've picked a different letter. Oh well.]

Every one of them has the value "test" in them as well.

--
TEST:

@and "a" "b" = No records

"a" and "b"

--
TEST:

@and @or "a" "b" "c" = No records

[no] ("a" and "b") or "c" ["a" and "b" = 0. "c" = 1.]
[?] "a" and ("b" or "c") ["a" and "b" = 0. "a" and "c" = 0.]
[?] ("a" or "b") and "c" ["a" and "c" = 0. "b" and "c" = 0.]
[no] "a" or ("b" and "c") ["a" = 1. "b" and "c" = 0.]

--
TEST:

@and @or "a" "b" "test" = 2 records ("A","B")

[?] ("a") or ("b" and "test") ["a" = 1. "b" and "test" = 1. Grand total of 2.
[?] ("a" or "b") and "test" ["a" and "test" = 1. "b" and "test" = 1. Grand total of 2.
[no] "a" and ("b" or "test") ["a" and "b" = 0. "a" and "test" = 5.]
[no] ("a" and "b") or "test" ["a" and "b" = 0. "test" = 5.]

--
TEST:

@and @or "test" "B" "C" = 1 record ("C")

[no] ("test" and "B") or "C" ["test and "B" = 1. "C" = 1. Grand total 2 records of "B" and "C"]
[no] "test" and ("B" or "C") ["test" and "B" = 1. "test" and "C" = 1. Grand total 2 records of "B" and "C"]
[YES] ("test" or "B") and "C" ["test" and "C" = 1. "B" and "C" = 0.]
[no] "test" or ("B" and "C") ["test" = 5. "B" and "C" = 0]


--
So...it looks like queries are expanded from the inside out...

@and @or "test" "B" "C"

("test" or "b") and "C"

Let's do some more tests to double-check this...

--
TEST:

@not "test" "b" = 4 records ("A","C","D",E")

"test" and not "b"

--
TEST:
@not @or "b" "test" "b" = 4 records ("A","C","D","E")

("b" or "test") and not "b"

--
TEST:
@not @or "b" "c" "b" = 1 record ("C")

("b" or "c") and not "b"

--
TEST:
@not @or @or "b" "c" "a" "b" = 2 records ("A","C")

(("b" or "c") or "a") and not "b"

--
TEST:
@not @or @or "b" "c" "a" "e" = 3 records ("A","B","C")

(("b" or "c") or "a") and not "e"

--
TEST:
@not @or @and "b" "c" "a" "e" = 1 record ("A")

(("b" and "c") or "a") and not "e" ["b" and "c" = 0. "a" = 1". Not "E"..."A" isn't "E" so it goes through.

--
TEST:
@not @or @and "b" "c" "a" "a"

(("b" and "c") or "a") and not "a" ["b" and "c" = 0. "a" = 1". Not "A"..."A" is "A" so it doesn't make it to the result set.]

--
TEST:

@or @not @or "b" "c" "c" "e" = 2 records ("B","E")

(("b" or "c") and not "c") or "e" ["b" = 1. "c" = 1. But not C...so only B. Or "E". Ergo, "B","E" returned)

--
TEST:

@or @not "test" "c" "e" = 4 records ("A","B","D","E")

("test" and not "C") or "e"

--
TEST:

@or @not "test" "c" "c" = 4 records ("A","B","C","D","E")

("test" and not "C") or "C"

--
TEST:

@not "test" @or "c" "e" = 3 records ("A","B","D")

("test") and not ("c" or "e")

--
So...it appears that the Zebra parser takes the right-most operator and puts it after the first operand it encounters to its right...

So...let's look at my query again...

@not 
@or
@or
@or 
@attr 1=1016 @attr 5=1 @attr 4=6 "e" 
@attr 9=75 @attr 2=102 @attr 5=1 @attr 4=6 "e" 
@attr 9=20 @attr 2=102 @attr 5=1 @attr 4=6 "e" 
@attr 9=34 @attr 2=102 @attr 5=1 @attr 4=6 "e" 
@attr 1=9011 1

This would become 

(((@attr 1=1016 @attr 5=1 @attr 4=6 "e" @or @attr 9=75 @attr 2=102 @attr 5=1 @attr 4=6 "e") @or @attr 9=20 @attr 2=102 @attr 5=1 @attr 4=6 "e") @or @attr 9=34 @attr 2=102 @attr 5=1 @attr 4=6 "e") @not @attr 1=9011 1

That should be OK...

That means that it will perform 5 sub-queries...4 of them will have OR between them...which produces the result set we want...

Then the final sub-query returns a result set for records we don't want, and it acts as a filter to make sure that no suppressed records get through.

Voila!
Comment 17 David Cook 2014-07-18 01:49:22 UTC
If I want to check my Koha query again...

--
TEST:

@not @or @or @or "a" "b" c" "e" "b" = 3 records("A","C","E")

((("a" or "b") or "c") or "e") and not "b"

--

Ok...now that we've got this basic concept down...let's try something more advanced.

Let's consider the parsing logic again...

prefixQuery("@and complete @or dinosaur pterosaur");
CCLQuery("complete and (dinosaur or pterosaur)");

(In hindsight, the above example quite clearly illustrates how operators are applied to operands. The tests helped clarify that though...at least in my mind and hopefully for anyone else reading this who didn't understand before.)

So let's look at the grammar again (http://www.indexdata.com/yaz/doc/tools.html#PQF):

"simple ::= result-set | term."

So a simple query may just be a single term like "A".

"complex ::= operator query-struct query-struct."

A complex query can have an operator and two query structures...but a query structure is not the same thing as just a term.

"query-struct ::= attr-spec | simple | complex | '@term' term-type query"

A query structure can itself be a complex query! So we should be able to join multiple complex queries together to form a new complex query.

--
TEST:

@or @or "a" "b" @or "c" "e" = 4 records ("A","B","C","E")

("a" or "b") or ("c" or "e")

--
TEST:

@not "test" @or "c" "e" = 3 records ("A","B","D")

("test") and not ("c" or "e")

--

So let's add some more complex data... I'm going to add "bull" to "B".

--
TEST:

@not @or "a" @or @and "b" "bull" "c" "a" = 2 records ("B","C")

("a" or ("b" and "bull") or "c") and not "a"

--
TEST:

@or "a" @or @and "b" "bull" "c" = 3 records ("A","B","C")

("a" or ("b" and "bull") or "c")

--

So something like the following is probably valid in CCL...

(kw,wrdl,rt=e or kw,wrdl,rt=e or kw,wrdl,rt=e or kw,wrdl,rt=e)

But I bet when it's parsed into RPN it is broken up into pairs like so:

(((kw,wrdl,rt=e or kw,wrdl,rt=e) or kw,wrdl,rt=e) or kw,wrdl,rt=e)

Because there has to be an order of operations...

For instance ("b" and "bull" or "c") would probably be parsed as (("b" and "bull") or "c") rather than ("b" and ("bull" or "c")).

--
TEST:

(After using "querytype ccl2rpn" and "set_cclfile /koha/etc/zebradb/ccl.properties" in yaz-client)

("b" and "bull" or "c") = 2 records ("B","C")

(("b" and "bull") or "c")

--
TEST:

("b" and ("bull" or "c")) = 1 record ("B")

------------

So...I'm sure that was more than anyone ever wanted to know about CCL and PQF syntax. But...it proves that my solution will work.

Ideally, we should be adding OpacSuppression when the query is being built, rather than after the fact. Ah, we do pass $query_type back from buildQuery. While it's usually null, if it is a PQF query, it will say "pqf". That means it won't be too hard to change our current code to work with QP.

At the moment, I'm inclined to agree with Galen about QueryParser not being able to handle negation...although I'll have to look some more. 

That said, we could add OpacSuppression into the QP query building without requiring it to parse it from a query we put in, so it might not matter too much.
Comment 18 David Cook 2014-07-18 02:43:21 UTC
Ah, I believe that Galen was incorrect. QueryParser indeed does appear to handle negation.

QueryParser:

[Negation via query_plan.pm]
(test) && !(suppress:1)
OR
(test) && -(suppress:1)

OR

[Negation via node.pm]
(test) && (suppress:!"1")
OR
(test) && (suppress:-"1")

(N.B. When negating at the "node" level, you have to wrap the 1 in quotes to treat it as a phrase or else the ! or - appear to be parsed as part of the operand.)

CCL:
(test) not (suppress:1)

If you think about it, "not" really is "and not", so CCL's "not" is equivalent to "&& !" in QP.

------

Ahh...so I've finally read through Martin's patch. 

tl;dr

Martin, if you change the following:

$query = "$query -suppress:1";

to:

$query = "($query) && -(suppress:1)";

It should work.

[Note: If we wanted to add other fields to disallow, we could throw them inside that disallowed group when using QP as well...]
Comment 19 Martin Renvoize 2014-07-21 07:40:05 UTC Comment hidden (obsolete)
Comment 20 Martin Renvoize 2014-07-21 07:43:42 UTC
dcook, thanks for all your efforts on this one. 

I've made the suggested changes, I think, and am currently proving the process to myself too, but thought it worth posting up this trivial amendment for others to start testing sooner rather than later.

I'm now trying to work out all the ways in which this should be tested to repvent a regression... I'm thinking.

Testing with and without QP turned on, with and without OpacSupressions turned on, and testing from both the front page search and the advanced search pages.  Combinations of all those should close the door on a regression right?  Is there anywhere I've missed in terms of sending different queries down the pipe?
Comment 21 Martin Renvoize 2014-07-21 07:51:22 UTC
Bear with me, I'm adapting the patch to be a bit cleaner, and hopefully catch a few more cases as per dcooks other suggestions.
Comment 22 Martin Renvoize 2014-07-21 07:55:05 UTC Comment hidden (obsolete)
Comment 23 Martin Renvoize 2014-07-21 07:56:25 UTC
Next attempt ready.. go forth and destroy.. I mean test
Comment 24 Chris Cormack 2014-07-21 21:49:25 UTC Comment hidden (obsolete)
Comment 25 David Cook 2014-07-21 23:22:39 UTC
I realized last night after getting off IRC that I forgot to mention that we would need to use PQF syntax rather than QP syntax, if using the $query_type to know what type of query we're working with. 

The following syntax would have to be used "before" buildQuery/parseQuery, as this is QueryParser syntax which is parsed and then transformed into PQF.

$query = "($query) && -(suppress:1)"

Since the latest patch is adding the OpacSuppression after the query has already been returned from buildQuery as PQF, we need to use the following PQF syntax:

$query = '@not '."$query".' @attr 1=9011 1';

--

Otherwise we generate an invalid query that will return 0 records when using QueryParser:

ZOOM error 20003 "can't set prefix query" (addinfo: "(@or @or @or @attr 1=1016 @attr 5=1 @attr 4=6 "test" @attr 9=75 @attr 2=102 @attr 5=1 @attr 4=6 "test" @attr 9=20 @attr 2=102 @attr 5=1 @attr 4=6 "test" @attr 9=34 @attr 2=102 @attr 5=1 @attr 4=6 "test") && -(suppress:1)") from diag-set 'ZOOM'

--

We also can't use parentheses as those aren't used in PQF either. 

ZOOM error 20003 "can't set prefix query" (addinfo: "@not (@or @or @or @attr 1=1016 @attr 5=1 @attr 4=6 "test" @attr 9=75 @attr 2=102 @attr 5=1 @attr 4=6 "test" @attr 9=20 @attr 2=102 @attr 5=1 @attr 4=6 "test" @attr 9=34 @attr 2=102 @attr 5=1 @attr 4=6 "test") @attr 1=9011 1") from diag-set 'ZOOM'

--

The following query will work as expected though:

@not @or @or @or @attr 1=1016 @attr 5=1 @attr 4=6 "test" @attr 9=75 @attr 2=102 @attr 5=1 @attr 4=6 "test" @attr 9=20 @attr 2=102 @attr 5=1 @attr 4=6 "test" @attr 9=34 @attr 2=102 @attr 5=1 @attr 4=6 "test" @attr 1=9011 1

I had to look over those past comments of mine to make sure but if this were CCL it should look like this:

(((kw,wrdl,rt=test OR kw,wrdl,rt=test) OR kw,wrdl,rt=test) OR kw,wrdl,rt=test) NOT suppress=1
Comment 26 David Cook 2014-07-21 23:38:44 UTC Comment hidden (obsolete)
Comment 27 Bernardo Gonzalez Kriegel 2014-08-02 04:24:38 UTC Comment hidden (obsolete)
Comment 28 Bernardo Gonzalez Kriegel 2014-08-02 04:24:47 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2014-08-03 13:17:47 UTC
I am sorry, but it looks like we are not quite there yet. I am using "der" (similar to "the") as a search term. Without OpacSuppression this gives 34 results.

I hid one record, without QueryParser the search gives 33 results now. OK

With QueryParser I see some weird behaviour - sometimes the results are 21, then 33, then a specific recod is pulled up or no results are found... I am puzzled.

The URL is:
http://localhost/cgi-bin/koha/opac-search.pl?idx=&q=der

Reloaded the page using F5.

I reindexed my database after hiding the record.
Comment 30 David Cook 2014-08-03 23:14:31 UTC
(In reply to Katrin Fischer from comment #29)
> I am sorry, but it looks like we are not quite there yet. I am using "der"
> (similar to "the") as a search term. Without OpacSuppression this gives 34
> results.
> 
> I hid one record, without QueryParser the search gives 33 results now. OK
> 
> With QueryParser I see some weird behaviour - sometimes the results are 21,
> then 33, then a specific recod is pulled up or no results are found... I am
> puzzled.
> 
> The URL is:
> http://localhost/cgi-bin/koha/opac-search.pl?idx=&q=der
> 
> Reloaded the page using F5.
> 
> I reindexed my database after hiding the record.

You always manage to find the strangest behaviour, Katrin :p.

When you say sometimes the results are 21, then 33, then a specific record is pulled up or no results are found...is that all with the same query? Could you explain a bit more about the process you take to get to those results?
Comment 31 Katrin Fischer 2014-08-04 21:16:21 UTC
It's my secret super power... 

It's all with the same query - just hitting F5 or setting the cursor back into the search field and submitting. I have NO idea how this is possible sadly :(
Comment 32 Martin Renvoize 2014-08-07 08:13:07 UTC
Might this have something to do with the QP bug regarding hash order?  Wonder if caits perl version is one of those that forcefully randomizes hash order and thus 'breaks' QP a certain amount?  I'm not fully up on how it affects stuff, but it feels like a random enough symptom that it may match?
Comment 33 Katrin Fischer 2014-08-07 08:21:02 UTC
At least it would be some explanation - because that was really crazy behaviour. I am using the latest Ubuntu - can check the exact version when I am back home.
Comment 34 Martin Renvoize 2014-08-07 08:22:51 UTC
Latest Ubuntu would give you the affected perl version, so it's certailny feeling like my hunch may be correct.
Comment 35 David Cook 2014-08-07 23:39:19 UTC
Intriguing! I hadn't heard about this problem. Is there a bug number for it?

All the more reason to get Katrin to print out the queries that are being generated each time.

Katrin, if you do a warn "Query = $query" just after this line (http://repo.or.cz/w/koha.git/blob/HEAD:/opac/opac-search.pl#l521), we should be able to see the different queries that are being produced.

Of course, if it is a problem with randomizing hash order...that problem would exist before this patch is applied as well, so that's worth checking too.
Comment 36 Martin Renvoize 2014-08-08 13:45:14 UTC
Tomas just opened a bug for exactly what I was talking about!  Must have been reading my mind: bug 12738
Comment 37 Martin Renvoize 2014-08-08 14:15:23 UTC
OK, I've now tested with and without this patch, on Debian and Ubuntu (i.e. the differing perl versions)

I can verify that it works perfectly as expected on Deb (older perl) with the patch and breaks in the expected way before the patch.  On Ubuntu, QP is broken before and after the patch. If we mask the bug this patch addresses (by disabling OpacSupression) then pre patch and post patch breakage matches.

I think this should get passed qa really now.  Other QP issues need addressing separately.
Comment 38 Tomás Cohen Arazi 2014-08-08 21:31:23 UTC
I haven't read the patch, but please test this one on top of bug 12738 's patch if you're dealing with QP's PQF output.
Comment 39 Katrin Fischer 2014-08-10 15:24:05 UTC
Created attachment 30660 [details] [review]
[PASSED QA] Bug 10542: Fix QueryParser with OpacSupression

OpacSupressions manipulates the query string after the buildQuery
call and so breaks with queryParser enabled.  This patch adds
checks for queryParser and manipulates the query before passing it
to buildQuery if it is enabled, but leaves the post buildQuery
manipultation when queryParser is disabled

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Adding a sing after test

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 40 Katrin Fischer 2014-08-10 15:24:55 UTC
(In reply to Tomás Cohen Arazi from comment #38)
> I haven't read the patch, but please test this one on top of bug 12738 's
> patch if you're dealing with QP's PQF output.

I did - worked perfectly :)
Comment 41 Katrin Fischer 2014-08-10 16:09:09 UTC
Created attachment 30661 [details] [review]
[PASSED QA] Bug 10542: QueryParser + OpacSuppression doesn't allow search in 'all libraries'

Since we're using the $query_type variable to detect if this pre-built
query is PQF, we need to use PQF syntax (rather than QueryParser
syntax) when adding to the query. I've made a lot of notes of somewhat
incoherent notes on Bugzilla talking about PQF, CCL, and QP syntaxes,
but I'm hoping to refine these notes on a wiki page for future
reference.

_TEST PLAN_

1) Set 'Suppress in Opac' (ie 942$n) to 1 for one record
2) Re-index Zebra
3) Set 'OpacSuppression' to 'Hide'
4) Set 'UseQueryParser' to 'Do not try'
5) In the staff client, do a search that will return that suppressed
record as well as a few records that are NOT suppressed
6) Note that you can return that suppressed record in the staff client
7) Do the same search in the OPAC
8) Note that the suppressed record doesn't appear
9) Set 'UseQueryParser' to 'Try' && re-run the OPAC search
10) Note that no results appear (the logs will probably mention a
ZOOM error)

11) Apply patch

12) Re-run the OPAC search
13) Note that the suppressed record doesn't appear, and that the
not suppressed records are showing (it's important that you are
getting some results...as ZOOM errors are silent in the UI).
14) Set 'UseQueryParser' to 'Do not try'
15) Re-run the search
16) Note that the suppressed record doesn't appear, and that the
not suppressed records are showing

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Work as described following test plan.
No koha-qa errors.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works as described - hidden records are hidden.
Passes tests and QA script.
Comment 42 Tomás Cohen Arazi 2014-08-11 14:52:48 UTC
Patches pushed to master.

Thanks Martin and David!