https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15541 --- Comment #12 from David Cook <dcook@prosentient.com.au> --- Thanks for your comments, Marcel! I appreciate you taking the time to look at the patch. (In reply to Marcel de Rooy from comment #11)

QA Comment: Thanks for your patch, David. BTW This is imo not a trivial patch or something to signoff for an academy user, new to Koha.

Fair enough. I think perhaps the patch grew with time and I forgot to remove the Academy keyword.

[1] Although your changes in SimpleSearch are very small, it would probably help if you would add a test case in t/db_dependent/Search.t. (It seems to fail some tests currently, separate from your patch.)

Hmm, also fair enough. I'll look at doing that.

[2] The source_normalizer or norms just lead me to two FIXMEs: # FIXME normalize, substr # FIXME - default normalizer You introduce none and raw in the regex while this field actually does nothing (and should not have been there in this stage). Your patch starts using this field, while the only thing you want is not running _normalize for (mainly?) URLs. If we do not really solve the problem of this unused field, I think we should not touch it. BTW What would be the difference between raw and none? The regex suggests more than what we offer.

I'm not sure what you mean here... so I'll look at the code again. I think Kyle was having problems with normalization, so it's possible that he removed _normalize() in a different patch?

[3] I have some doubts about this new condition: if ($QParser && $matchpoint->{'index'} !~ m/\w,\w/) { Somehow you managed to bump in another unfinished area here too :) I would rather leave the condition as it was. In the QParser branch you could choose to ignore/remove the second specifier word with a similar regex.

Hmm, I don't think that's a good idea. You don't want to ignore/remove the second qualifier; it's important that the qualifier is in the query. You want to use it, but I don't think the QParser can do it. I think I lifted this condition from a different part of the Koha code to make the behaviour a bit more consistent. Perhaps it would be smarter to add a more universal "can_query_parse()" or something to test if the query is parseable by QParser...

Changing status for now.

-- You are receiving this mail because: You are watching all bug changes.