Bug 24720

Maybe we need a unit test for this change

Don't know about unit test Frido talked about, but this patch works pretty well !
Thanks a lot David !!

I agree with Frido. This behaviour should be separated into a separate function and unit tested.

I will say though that my own manual texting of the regular expression looks good. 

I've plugged in English, French, and Chinese into my tests and it's all working as expected. (I tried Arabic as well and I think that worked too, although I imagine Séverine could confirm that better than me.)

If we can just get those automated unit tests to prove that, then I think we'd be good to go.

I agree with the ask for unit tests - they can be added to tests for marc_records_to_documents in t/db_dependent/Koha/SearchEngine/Elasticsearch.t

Additionally, I think this should be optional in search field configs:
1 - Title already has options for 'non-filing' characters - so if you want to ignore a leading '[' you can 
2 - Author names may contain real punctation, consider the band '!!!'
    https://en.wikipedia.org/wiki/!!!

Good point, Nick!

There's nothing more frustrating for a library user than the search not retrieving records that you know it has...

(In reply to David Cook from comment #6)
> I will say though that my own manual texting of the regular expression looks
> good. 
> 
> I've plugged in English, French, and Chinese into my tests and it's all
> working as expected. (I tried Arabic as well and I think that worked too,
> although I imagine Séverine could confirm that better than me.)
> 
> If we can just get those automated unit tests to prove that, then I think
> we'd be good to go.

I added one simple test. I first attempted to test multiple variants with different scripts but it turned out to be more effort than think is warranted for this simple line of code. Firstly, since sort field are concatenated you have to add multiple different fields with mappings for different strings to be testable (since multiple values on same field are concatenated), no big deal but is it really worth it? (Could break into separate method and test that, but then we are missing out making sure it's actually called in Koha::SearchEngine::Elasticsearch::marc_records_to_documents and don't get the same coverage, and we would be creating a method just to be able to test this piece of code in isolation, which if we where to be consistent and treat the whole code-base this way would create a mess). I don't know if an issue with my terminal/editor or something else, but when testing other scripts, like Chinese I also get error messages like "UTF-8 "\xC3" does not map to Unicode at /usr/share/perl5/MARC/File/Encode.pm line 35." before even reaching the regular expression.

What we are testing is effectively be the trivial regular expression "s/^\W+//u", that is to replace all non word unicode characters at the beginning of the string with the empty string (remove them). I think the Perl core implementation regarding separating word and non-word characters in different scripts is not something that we should be writing tests for, I would trust Perl in this case. But sure, to make sure we get to that piece of code, and that the regexp does not do something completely unexpected doesn't hurt.

(In reply to Nick Clemens from comment #7)
> I agree with the ask for unit tests - they can be added to tests for
> marc_records_to_documents in t/db_dependent/Koha/SearchEngine/Elasticsearch.t
> 
> Additionally, I think this should be optional in search field configs:
> 1 - Title already has options for 'non-filing' characters - so if you want
> to ignore a leading '[' you can 
> 2 - Author names may contain real punctation, consider the band '!!!'
>     https://en.wikipedia.org/wiki/!!!

(In reply to David Cook from comment #8)
> Good point, Nick!
> 
> There's nothing more frustrating for a library user than the search not
> retrieving records that you know it has...

I made a mistake describing the bug as "Strip special chars from search fields", so I can understand the confusion. It should be sort, not search fields. I have corrected this in the commit message. This fix doesn't have any impact on search fields so will not effect what is search on (the stripping of special characters for search fields is already handled in Elastic). So the band name '!!!' would still be searchable, but would probably have the same sort weight as other strings containing only non-word characters, but this is more of an edge case that will probably never have any major impact in real life situations. To for example start allowing some special characters and not others I think could be a potientially larger source of confusion, it's also hard to know how to decide which characters should be exempted.

(In reply to Nick Clemens from comment #7)
> ...
> Additionally, I think this should be optional in search field configs:
> 1 - Title already has options for 'non-filing' characters - so if you want
> to ignore a leading '[' you can
> ...

I forgot to address this. You are right that ideally marc-posts with initial characters that should not be considered when sorting should have a proper value for non-filing characters. In practice though when either have a large collection of biblios, or do not have complete control over imported records this becomes very cumbersome to handle since this value has to be correctly set on each record. Also some fields which you might want to sort on don't have this indicator available.

Created attachment 125209 [details] [review]
Bug 24720: Strip special chars from sort fields

Strip all non alphanumerical characters from beginning
of sort field values so that sorting is performed on
the first alphanumeric character and not special characters
like "-" or "[".

To test:

1. Prepend "[" to the title of a biblio and save
2. Perform a search that matches this biblio and at least one
   more biblio.
3. Sort alphabetically a-z.
4. The biblio with "[" should appear first
5. Apply the patch
6. Perform a full reindexing
7. Perform steps 2-3 again
8. The result should now be alphabetically sorted,
   ignoring the initial bracket in the title.
9. Run tests in t/db_dependent/Koha/SearchEngine/Elasticsearch.t

Created attachment 125210 [details] [review]
Bug 24720: Add test

Rebased against master.

I was not able to repeat the bug. 

The message expected in step 8 is already present in step 4. The libraries are already sorted no matter if there's a "[" or not before.

Very simple rebase need here in Elasticsearch.t.

Created attachment 162674 [details] [review]
Bug 24720: Add test

Now rebased.

@solene.ngamga@inlibro.com It's still reproducible for me, there could be issues if the mappings for title have sort enabled for more fields than 245, in which case the sort order will be unpredictable as some other field might be the one actually sorted on. Had that same issue myself.

Created attachment 171988 [details] [review]
Bug 24720: Strip special chars from sort fields

Strip all non alphanumerical characters from beginning
of sort field values so that sorting is performed on
the first alphanumeric character and not special characters
like "-" or "[".

To test:

1. Prepend "[" to the title of a biblio and save
2. Perform a search that matches this biblio and at least one
   more biblio.
3. Sort alphabetically a-z.
4. The biblio with "[" should appear first
5. Apply the patch
6. Perform a full reindexing
7. Perform steps 2-3 again
8. The result should now be alphabetically sorted,
   ignoring the initial bracket in the title.
9. Run tests in t/db_dependent/Koha/SearchEngine/Elasticsearch.t

Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com>

Created attachment 171989 [details] [review]
Bug 24720: Add test

Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com>