[Koha-bugs] [Bug 15541] Prevent normalization during matching/import process

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15541

--- Comment #12 from David Cook <dcook at 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.