Bug 18316 - Add weighting/relevancy options to ElasticSearch
Summary: Add weighting/relevancy options to ElasticSearch
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Arnaud
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 20388 20608 20589 20602 20607 29010
  Show dependency treegraph
 
Reported: 2017-03-22 15:36 UTC by Nick Clemens
Modified: 2022-05-24 21:42 UTC (History)
14 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18316 - Add weighting/relevancy options to ElasticSearch (4.04 KB, patch)
2017-03-22 15:39 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316 - Add weighting/relevancy options to ElasticSearch (4.02 KB, patch)
2018-03-21 13:38 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316 - Add weighting/relevancy options to ElasticSearch (4.08 KB, patch)
2018-03-21 13:40 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316 - Ability to weight search fields (11.81 KB, patch)
2018-03-30 15:20 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316 - Add weighting/relevancy options to ES query on simple search (6.78 KB, patch)
2018-03-30 15:20 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316 - Ability to weight search fields (11.95 KB, patch)
2018-04-04 10:01 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 18316 - Add weighting/relevancy options to ES query on simple search (6.93 KB, patch)
2018-04-04 10:01 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 18316 - String changes (1.51 KB, patch)
2018-04-12 16:13 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316: Ability to weight search fields (12.01 KB, patch)
2018-04-12 16:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (6.98 KB, patch)
2018-04-12 16:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: String changes (1.56 KB, patch)
2018-04-12 16:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set (9.67 KB, patch)
2018-04-23 12:22 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set (9.63 KB, patch)
2018-04-23 12:50 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316: Ability to weight search fields (12.07 KB, patch)
2018-04-23 16:43 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (7.04 KB, patch)
2018-04-23 16:43 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 18316: String changes (1.71 KB, patch)
2018-04-23 16:43 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set (9.77 KB, patch)
2018-04-23 16:44 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 18316: Ability to weight search fields (12.13 KB, patch)
2018-05-04 12:50 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (7.09 KB, patch)
2018-05-04 12:50 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316: String changes (1.76 KB, patch)
2018-05-04 12:50 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set (9.82 KB, patch)
2018-05-04 12:50 UTC, Alex Arnaud
Details | Diff | Splinter Review
Change search field weight field to decimal and add column declaration to DBIx results source (5.63 KB, patch)
2018-05-21 12:26 UTC, David Gustafsson
Details | Diff | Splinter Review
Update Koha/Schema/Result/SearchField.pm (1.86 KB, patch)
2018-05-23 13:59 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1.87 KB, patch)
2018-05-24 09:00 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1.87 KB, patch)
2018-05-24 09:04 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (7.12 KB, patch)
2018-05-24 09:07 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: String changes (1.76 KB, patch)
2018-05-24 09:08 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set (9.85 KB, patch)
2018-05-24 09:08 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Change search field weight field to decimal (5.05 KB, patch)
2018-05-24 09:08 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1.87 KB, patch)
2018-05-24 09:08 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Ability to weight search fields (12.17 KB, patch)
2018-05-24 09:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (7.12 KB, patch)
2018-05-24 09:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: String changes (1.76 KB, patch)
2018-05-24 09:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set (9.85 KB, patch)
2018-05-24 09:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Change search field weight field to decimal (5.05 KB, patch)
2018-05-24 09:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1.87 KB, patch)
2018-05-24 09:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Ability to weight search fields (12.15 KB, patch)
2018-05-24 13:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (7.12 KB, patch)
2018-05-24 13:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: String changes (1.76 KB, patch)
2018-05-24 13:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set (9.85 KB, patch)
2018-05-24 13:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Change search field weight field to decimal (5.04 KB, patch)
2018-05-24 13:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1.45 KB, patch)
2018-05-24 13:23 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1.07 KB, patch)
2018-05-31 12:11 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Fix field weight validation/deletion bug (2.91 KB, patch)
2018-06-01 09:43 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 18316: Ability to weight search fields (12.13 KB, patch)
2018-08-10 13:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (7.09 KB, patch)
2018-08-10 13:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: String changes (1.76 KB, patch)
2018-08-10 13:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set (9.81 KB, patch)
2018-08-10 13:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: Change search field weight field to decimal (5.03 KB, patch)
2018-08-10 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1018 bytes, patch)
2018-08-10 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: Fix field weight validation/deletion bug (2.89 KB, patch)
2018-08-10 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: Ability to weight search fields (12.13 KB, patch)
2018-09-04 13:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (7.55 KB, patch)
2018-09-04 13:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: String changes (1.76 KB, patch)
2018-09-04 13:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: (follow-up) Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set (9.81 KB, patch)
2018-09-04 13:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: Change search field weight field to decimal (5.03 KB, patch)
2018-09-04 13:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1018 bytes, patch)
2018-09-04 13:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: Fix field weight validation/deletion bug (2.93 KB, patch)
2018-09-04 13:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: (follow-up) Fix tests and update statement (3.23 KB, patch)
2018-09-04 13:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18316: remove use_dis_max (848 bytes, patch)
2018-10-09 14:30 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316: (follow-up) remove use_dis_max (860 bytes, patch)
2018-10-10 12:03 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316: Ability to weight search fields (12.18 KB, patch)
2018-10-23 12:06 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (7.61 KB, patch)
2018-10-23 12:06 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 18316: String changes (1.81 KB, patch)
2018-10-23 12:06 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 18316: (follow-up) Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set (9.86 KB, patch)
2018-10-23 12:06 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 18316: Change search field weight field to decimal (5.08 KB, patch)
2018-10-23 12:06 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1.60 KB, patch)
2018-10-23 12:06 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 18316: Fix field weight validation/deletion bug (2.98 KB, patch)
2018-10-23 12:06 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 18316: (follow-up) Fix tests and update statement (3.29 KB, patch)
2018-10-23 12:06 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 18316: (follow-up) remove use_dis_max (914 bytes, patch)
2018-10-23 12:06 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 18316: Ability to weight search fields (12.25 KB, patch)
2018-10-29 14:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18316: Add weighting/relevancy options to ES query on simple search (7.67 KB, patch)
2018-10-29 14:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18316: String changes (1.87 KB, patch)
2018-10-29 14:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18316: (follow-up) Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set (9.92 KB, patch)
2018-10-29 14:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18316: Change search field weight field to decimal (5.14 KB, patch)
2018-10-29 14:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18316: Update Koha/Schema/Result/SearchField.pm (1.67 KB, patch)
2018-10-29 14:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18316: Fix field weight validation/deletion bug (3.04 KB, patch)
2018-10-29 14:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18316: (follow-up) Fix tests and update statement (3.34 KB, patch)
2018-10-29 14:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18316: (follow-up) remove use_dis_max (986 bytes, patch)
2018-10-29 14:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18316: (QA follow-up) Add final missing filter (1.41 KB, patch)
2018-10-29 14:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18316: (follow-up) Don't let 'fields' be undef and adjust tests (1.58 KB, patch)
2018-11-08 01:51 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2017-03-22 15:36:16 UTC

    
Comment 1 Nick Clemens 2017-03-22 15:39:16 UTC Comment hidden (obsolete)
Comment 2 Alex Arnaud 2018-03-21 13:38:31 UTC Comment hidden (obsolete)
Comment 3 Alex Arnaud 2018-03-21 13:40:41 UTC Comment hidden (obsolete)
Comment 4 Gaetan Boisson 2018-03-28 12:41:14 UTC
Alex is working on this.

We are assuming relevancy working is mostly important for simple search, or what zebra calls the "keyword" index.

What we plan to do:

Add columns to the "search fields" tab of the search engine configuration page, where the user will be able to indicate a weight for each field.

This will allow to configure the way the simple search is weighted: it will search in all fields for which a weight has been indicated. If no weight was indicated it will search in all fields. (In addition, this allows excluding some search fields from the simple search!)

This will not affect advanced search, in which the user can pick indices himself. (But it could, and we would then run the query with the weight defined for each field.)

The possibility to add a weight to a search field will only be offered if this field is used in the biblio mapping. So it won't be possible to give weight to a field only used in the authorities mapping, or defined but unused.

In the future it should be possible to have "koha indices" that search across multiple elasticsearch fields, each with a different weight. But this seems a bit ambitious for now, especially with 18.05 so close. So this seems like a reasonable goal for the next release, and certainly a very useful one.
Comment 5 Nick Clemens 2018-03-28 13:07:17 UTC
This sounds like a reasonable approach, looking forward to seeing it :-)
Comment 6 Alex Arnaud 2018-03-30 15:20:40 UTC Comment hidden (obsolete)
Comment 7 Alex Arnaud 2018-03-30 15:20:49 UTC Comment hidden (obsolete)
Comment 8 Séverine Queune 2018-04-04 10:01:25 UTC Comment hidden (obsolete)
Comment 9 Séverine Queune 2018-04-04 10:01:28 UTC Comment hidden (obsolete)
Comment 10 Séverine Queune 2018-04-04 14:33:28 UTC
Patchs work as described.

I misunderstood the way to use numbers, thinking "1" was the most important field(s) I wanted to search in.
Maybe a warning or more detailed description would be useful for reindex can take time for big databases, it would be sad if others make the same mistake as I.

This weighting add a lot of relevance to the simple search.

In reaction to Gaetan's comment, it also appears that having weight on sub-fields used on the same index would be great.
For example, when searching with index "title" in our Unimarc system, we would like to display books that contains the searched word in 200$b before the ones in 410$t.
Comment 11 Alex Arnaud 2018-04-04 15:11:45 UTC
(In reply to Séverine Queune from comment #10)
> Patchs work as described.
> 
> I misunderstood the way to use numbers, thinking "1" was the most important
> field(s) I wanted to search in.
> Maybe a warning or more detailed description would be useful for reindex can
> take time for big databases, it would be sad if others make the same mistake
> as I.
You mean add a warning to tell users that more the weight is hight more the field is important?
> 
> This weighting add a lot of relevance to the simple search.
> 
> In reaction to Gaetan's comment, it also appears that having weight on
> sub-fields used on the same index would be great.
> For example, when searching with index "title" in our Unimarc system, we
> would like to display books that contains the searched word in 200$b before
> the ones in 410$t.
I'm not yet confortable enough with all ES capabilities, but it could be done by adding a "other-title" search field with a different weight. Does it make sens for you?
Comment 12 Nicolas Legrand 2018-04-11 11:21:57 UTC
(In reply to Alex Arnaud from comment #11)
> (In reply to Séverine Queune from comment #10)
> > Patchs work as described.
> > 
> > I misunderstood the way to use numbers, thinking "1" was the most important
> > field(s) I wanted to search in.
> > Maybe a warning or more detailed description would be useful for reindex can
> > take time for big databases, it would be sad if others make the same mistake
> > as I.
> You mean add a warning to tell users that more the weight is hight more the
> field is important?

I think that's what she meant.

> > This weighting add a lot of relevance to the simple search.
> > 
> > In reaction to Gaetan's comment, it also appears that having weight on
> > sub-fields used on the same index would be great.
> > For example, when searching with index "title" in our Unimarc system, we
> > would like to display books that contains the searched word in 200$b before
> > the ones in 410$t.
> I'm not yet confortable enough with all ES capabilities, but it could be
> done by adding a "other-title" search field with a different weight. Does it
> make sens for you?

Yes, but not to our librarians colleagues who want to describe titles in a myriad of MARC' subfield while uniting them all under one index entry. I'm not sure to be able to convince them this may not be a good idea :). The 4XX$t as a title index  for instance, is a relevancy killer. I'll try to convince them to bind it to a linked-with-title index.

Anyway, having a different relevancy for same index/different subfield may be another bug.
Comment 13 Nick Clemens 2018-04-12 11:28:30 UTC
I agree - different weighting for specific fields in an index would be a separate bug

Searching and weighting works well and tests pass

I have questions about the comments:

>Weight: define weight between 1 and 99.
add: Higher numbers indicate increased relevenacy

>search will be done on weighted fields only
this doesn't seem to be the case - weight author - search for an isbn, you get the record

>if no field is weighted, search will be done on all the record
true, but we search all fields even with some wieghts

I think the behaviour is correct, searching _all and adding weighted fields, just need to adjust comments

Should we indicate that weights are not used during adv search? (If we want them I think that can be done on a future patch)
Comment 14 Alex Arnaud 2018-04-12 15:57:17 UTC
(In reply to Nick Clemens from comment #13)
> I agree - different weighting for specific fields in an index would be a
> separate bug
+1
> 
> Searching and weighting works well and tests pass
> 
> I have questions about the comments:
> 
> >Weight: define weight between 1 and 99.
> add: Higher numbers indicate increased relevenacy
> 
> >search will be done on weighted fields only
> this doesn't seem to be the case - weight author - search for an isbn, you
> get the record
Right. Search is done on all fields but boost the weighted ones.
> 
> >if no field is weighted, search will be done on all the record
> true, but we search all fields even with some wieghts
> 
> I think the behaviour is correct, searching _all and adding weighted fields,
> just need to adjust comments
> 
> Should we indicate that weights are not used during adv search? (If we want
> them I think that can be done on a future patch)
I can do it. The future patch for adv search will have to remove the comment.
Comment 15 Alex Arnaud 2018-04-12 16:13:59 UTC
Created attachment 74112 [details] [review]
Bug 18316 - String changes
Comment 16 Nick Clemens 2018-04-12 16:52:14 UTC Comment hidden (obsolete)
Comment 17 Nick Clemens 2018-04-12 16:52:18 UTC Comment hidden (obsolete)
Comment 18 Nick Clemens 2018-04-12 16:52:21 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2018-04-18 20:33:43 UTC
1. 
From Koha::SearchFields->weighted_fields

+    while ( my $field = $fields->next ) {
+        push @$w_fields, $field->name;
+        push @$weight, $field->weight;
+    }
+
+    return ($w_fields, $weight);

From search.pl:
($w_fields, $weight) = Koha::SearchFields->weighted_fields();

then we call build_query_compat with { w_fields => @$w_fields, weight => @$weight  }

which will finally do:
+    if ( defined $weights[0] ) {
+        for (my $i = 0 ; $i < (scalar @weights) ; $i++ ){
+            push @fields, "$w_fields[$i]^$weights[$i]";
+        }
+    }

so basically you want to fetch the weighted fields from build_query_compat, with something like:
  push @fields, sprintf("%s^%s", $_->name, $_->weight) for Koha::SearchFields->weighted_fields;

That seems much more easier, what did I miss?


2. Sounds like you could replaced the loop in is_mapped_biblios, with $self->search_marc_maps->search({ index_name => 'biblios' })->count ? 1 : 0;

3. search_marc_maps reads wrong too.
Comment 20 Alex Arnaud 2018-04-19 12:04:18 UTC
(In reply to Jonathan Druart from comment #19)
> 1. 
> From Koha::SearchFields->weighted_fields
> 
> +    while ( my $field = $fields->next ) {
> +        push @$w_fields, $field->name;
> +        push @$weight, $field->weight;
> +    }
> +
> +    return ($w_fields, $weight);
> 
> From search.pl:
> ($w_fields, $weight) = Koha::SearchFields->weighted_fields();
> 
> then we call build_query_compat with { w_fields => @$w_fields, weight =>
> @$weight  }
> 
> which will finally do:
> +    if ( defined $weights[0] ) {
> +        for (my $i = 0 ; $i < (scalar @weights) ; $i++ ){
> +            push @fields, "$w_fields[$i]^$weights[$i]";
> +        }
> +    }
> 
> so basically you want to fetch the weighted fields from build_query_compat,
> with something like:
>   push @fields, sprintf("%s^%s", $_->name, $_->weight) for
> Koha::SearchFields->weighted_fields;
> 
> That seems much more easier, what did I miss?
We don't want to use weighted fields on advanced search and build_query_compat is called for both simple and adv search.
But i can propose something similar in search.pl
> 
> 
> 2. Sounds like you could replaced the loop in is_mapped_biblios, with
> $self->search_marc_maps->search({ index_name => 'biblios' })->count ? 1 : 0;
Right.
> 
> 3. search_marc_maps reads wrong too.
What is wrong?
Comment 21 Alex Arnaud 2018-04-23 12:22:23 UTC Comment hidden (obsolete)
Comment 22 Alex Arnaud 2018-04-23 12:50:45 UTC Comment hidden (obsolete)
Comment 23 Séverine Queune 2018-04-23 16:43:51 UTC Comment hidden (obsolete)
Comment 24 Séverine Queune 2018-04-23 16:43:55 UTC Comment hidden (obsolete)
Comment 25 Séverine Queune 2018-04-23 16:43:59 UTC Comment hidden (obsolete)
Comment 26 Séverine Queune 2018-04-23 16:44:02 UTC Comment hidden (obsolete)
Comment 27 Séverine Queune 2018-04-23 16:46:03 UTC
Tested with chinese, persian and latin (english/french) languages.
Relevance is the same as before (with the first patch) so good for me !
Comment 28 David Gustafsson 2018-04-24 19:23:54 UTC
Nice work. I was unable to apply patch using git bz apply, getting:

error: sha1 information is lacking or useless (Koha/SearchEngine/Elasticsearch/QueryBuilder.pm).
error: could not build fake ancestor

Am I doing something wrong or is there an issue with the current patches?

I also have some questions/suggestions:

1) I find it slightly confusing that "weight" is in the patch whilst "boost" is used in elastic. Not a big deal at all, but would be nicer to use the same terminology as Elastic imho.

2) Weight/boost should be decimal, for example DECIMAL(5,2) should be more than enough precision.

3) Why not use field boosting in advanced search? As a user I would find it confusing to get different results performing the same search in both interfaces.

4) It seems like field boosts/weights specified in mappings.yaml will not be picked up, this could be added in reset_elasticsearch_mappings in Elasticsearch.pm.

I can contribute patches for some or all of the above s depending on which ones are deemed good suggestions once I am able to apply the current patch.
Comment 29 Katrin Fischer 2018-04-27 06:32:29 UTC
Hi David, please change to 'Patch doesn't apply' in this case. Some devs use saved searches to see where they need to take action, this makes it easier.
Comment 30 Alex Arnaud 2018-04-27 13:08:07 UTC
(In reply to David Gustafsson from comment #28)
> Nice work. I was unable to apply patch using git bz apply, getting:
> 
> error: sha1 information is lacking or useless
> (Koha/SearchEngine/Elasticsearch/QueryBuilder.pm).
> error: could not build fake ancestor
> 
> Am I doing something wrong or is there an issue with the current patches?
Fixed!
> 
> I also have some questions/suggestions:
> 
> 1) I find it slightly confusing that "weight" is in the patch whilst "boost"
> is used in elastic. Not a big deal at all, but would be nicer to use the
> same terminology as Elastic imho.
I'm not against renaming it. In my mind, boost is most seen as a yes/not choice (boosted/not boosted) and not as range of values.
> 
> 2) Weight/boost should be decimal, for example DECIMAL(5,2) should be more
> than enough precision.
Currently we can set it between 1 and 99. Do you think we need more precision?
> 
> 3) Why not use field boosting in advanced search? As a user I would find it
> confusing to get different results performing the same search in both
> interfaces.
We plan to do that later. We believed that adv search allows to search on specific(s) field(s). So boosting seems less relevant.
> 
> 4) It seems like field boosts/weights specified in mappings.yaml will not be
> picked up, this could be added in reset_elasticsearch_mappings in
> Elasticsearch.pm.
See bug 20607 which depend on this one
> 
> I can contribute patches for some or all of the above s depending on which
> ones are deemed good suggestions once I am able to apply the current patch.
Comment 31 Alex Arnaud 2018-05-04 12:50:19 UTC
Created attachment 75073 [details] [review]
Bug 18316: Ability to weight search fields

Weight can be defined between 1 and 99.
only search fields mapped with biblios can be weighted

Test plan:
  - apply this patch,
  - update schema file (perl misc/devel/update_dbix_class_files.pl),
  - go to Administration > Search engine configuration,
  - in search fields tab, define weights for some fields and save,
  - check weights has been saved,
  - reset some weights (empty) and save,
  - check weights has been reset

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 32 Alex Arnaud 2018-05-04 12:50:32 UTC
Created attachment 75074 [details] [review]
Bug 18316: Add weighting/relevancy options to ES query on simple search

The idea is the following: if some search field(s) are weighted in
search
engine config page, Koha will query ES on these fields only and with
the coresponding weights. Else, search is done on the entire record.

Test plan (having Koha working with Elasticsearch):
  - apply this patch,
  - try searches with and without weight defined on search fields

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 33 Alex Arnaud 2018-05-04 12:50:45 UTC
Created attachment 75075 [details] [review]
Bug 18316: String changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 34 Alex Arnaud 2018-05-04 12:50:57 UTC
Created attachment 75076 [details] [review]
Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 35 David Gustafsson 2018-05-09 09:22:54 UTC
Sorry about the late reply.

> Fixed!
Great :)

> I'm not against renaming it. In my mind, boost is most seen as a yes/not
> choice (boosted/not boosted) and not as range of values.

As previously noted, not a big deal, my main issue with it is that boost is used in lucene/elastic.

> Currently we can set it between 1 and 99. Do you think we need more precision?

In lucene/elastic weight is a decimal number. So I really think it should be a decimal for that reason. The admin interface should then probably use text inputs instead of selects with predefined values. There are also situations where you just want to adjust the relevancy a very small amount to make sure queries are perfectly balanced and that one does not always dominate the relevance scoring. With integers this is much more difficult, and you will be forced to assign boosts to many more fields to get the increased precision.

> We plan to do that later. We believed that adv search allows to search on
> specific(s) field(s). So boosting seems less relevant.

Ok, I adjusted some of the queries for authorities search to utilize fields better in bug 20589. So perhaps I can include the field boots if I rebase that patch on this one when it has been merged in Koha.

> See bug 20607 which depend on this one

Aha, ok.
Comment 36 David Gustafsson 2018-05-09 09:41:16 UTC
Oops, regarding my response to:

> We plan to do that later. We believed that adv search allows to search on
> specific(s) field(s). So boosting seems less relevant.

I mixed it up with authorities search. So to clarify, I think boosts should be used also for authorities search (although not that important), but agree it is  not obvious that boosts should be applied in advanced search.
Comment 37 David Gustafsson 2018-05-21 12:26:29 UTC
Created attachment 75458 [details] [review]
Change search field weight field to decimal and add column declaration to DBIx results source
Comment 38 Alex Arnaud 2018-05-23 09:16:35 UTC
(In reply to David Gustafsson from comment #37)
> Created attachment 75458 [details] [review] [review]
> Change search field weight field to decimal and add column declaration to
> DBIx results source

Hello David,

You provide changes on Koha/Schema/Result/SearchField.pm. This should be done in a dedicated patch. RM will ignore it.

Also, you replaced the input type number with a input text. This cause errors when typing non-numeric characters in. You can use a input number with step=0.1 to increment decimal numbers.

Need to fail QA
Comment 39 Katrin Fischer 2018-05-23 10:33:18 UTC
(In reply to Alex Arnaud from comment #38)
> (In reply to David Gustafsson from comment #37)
> > Created attachment 75458 [details] [review] [review] [review]
> > Change search field weight field to decimal and add column declaration to
> > DBIx results source
> 
> Hello David,
> 
> You provide changes on Koha/Schema/Result/SearchField.pm. This should be
> done in a dedicated patch. RM will ignore it.
> 
> Also, you replaced the input type number with a input text. This cause
> errors when typing non-numeric characters in. You can use a input number
> with step=0.1 to increment decimal numbers.
> 
> Need to fail QA

Input number for decimals can be problematic for translations in my experience. Please test for example with de-DE if the value is correctly stored (it will force a decimal comma instead of . on input, which is inconsistent with our other input fields right now)
Comment 40 David Gustafsson 2018-05-23 12:42:36 UTC
(In reply to Alex Arnaud from comment #38)
> (In reply to David Gustafsson from comment #37)
> > Created attachment 75458 [details] [review] [review] [review]
> > Change search field weight field to decimal and add column declaration to
> > DBIx results source
> 
> Hello David,
> 
> You provide changes on Koha/Schema/Result/SearchField.pm. This should be
> done in a dedicated patch. RM will ignore it.
> 
> Also, you replaced the input type number with a input text. This cause
> errors when typing non-numeric characters in. You can use a input number
> with step=0.1 to increment decimal numbers.
> 
> Need to fail QA

Ok, I can fix this, though precision should be 0.01 in that case. There is server side validation, so everything that is not a number will be ignored regardless of input type.

I was not aware that Koha/Schema/Result/* files should not be changed. How does that work for signing off, since without the change Koha will throw an error when trying to save weights in admin UI?
Comment 41 Nick Clemens 2018-05-23 12:45:23 UTC
(In reply to David Gustafsson from comment #40)
> (In reply to Alex Arnaud from comment #38)
> > (In reply to David Gustafsson from comment #37)
> > > Created attachment 75458 [details] [review] [review] [review] [review]
> > > Change search field weight field to decimal and add column declaration to
> > > DBIx results source
> > 
> > Hello David,
> > 
> > You provide changes on Koha/Schema/Result/SearchField.pm. This should be
> > done in a dedicated patch. RM will ignore it.
> > 
> > Also, you replaced the input type number with a input text. This cause
> > errors when typing non-numeric characters in. You can use a input number
> > with step=0.1 to increment decimal numbers.
> > 
> > Need to fail QA
> 
> Ok, I can fix this, though precision should be 0.01 in that case. There is
> server side validation, so everything that is not a number will be ignored
> regardless of input type.
> 
> I was not aware that Koha/Schema/Result/* files should not be changed. How
> does that work for signing off, since without the change Koha will throw an
> error when trying to save weights in admin UI?

Generally you make your code changes, commit the patches, then run:
misc/devel/update_dbix_class_files.pl
And commit those as their own patch - that way the changes are included for testers but can be regenerated when pushing to master
Comment 42 Alex Arnaud 2018-05-23 13:52:57 UTC
(In reply to Katrin Fischer from comment #39)

> Input number for decimals can be problematic for translations in my
> experience. Please test for example with de-DE if the value is correctly
> stored (it will force a decimal comma instead of . on input, which is
> inconsistent with our other input fields right now)

I just tried on a Koha translated into German. Sounds that there no problem with the values stored. Any test plan to reproduice issue you talked about?
Comment 43 David Gustafsson 2018-05-23 13:59:25 UTC
Created attachment 75490 [details] [review]
Update Koha/Schema/Result/SearchField.pm
Comment 44 David Gustafsson 2018-05-23 14:00:33 UTC
(In reply to Katrin Fischer from comment #39)
> (In reply to Alex Arnaud from comment #38)
> > (In reply to David Gustafsson from comment #37)
> > > Created attachment 75458 [details] [review] [review] [review] [review]
> > > Change search field weight field to decimal and add column declaration to
> > > DBIx results source
> > 
> > Hello David,
> > 
> > You provide changes on Koha/Schema/Result/SearchField.pm. This should be
> > done in a dedicated patch. RM will ignore it.
> > 
> > Also, you replaced the input type number with a input text. This cause
> > errors when typing non-numeric characters in. You can use a input number
> > with step=0.1 to increment decimal numbers.
> > 
> > Need to fail QA
> 
> Input number for decimals can be problematic for translations in my
> experience. Please test for example with de-DE if the value is correctly
> stored (it will force a decimal comma instead of . on input, which is
> inconsistent with our other input fields right now)

Now updated the patch with requested changes. Also tried with German translation and worked for me.
Comment 45 David Gustafsson 2018-05-23 14:04:00 UTC
(In reply to Nick Clemens from comment #41)
> Generally you make your code changes, commit the patches, then run:
> misc/devel/update_dbix_class_files.pl
> And commit those as their own patch - that way the changes are included for
> testers but can be regenerated when pushing to master

Thanks for clarifying this, now I understand how to go about it.
Comment 46 Katrin Fischer 2018-05-23 14:33:29 UTC
(In reply to Alex Arnaud from comment #42)
> (In reply to Katrin Fischer from comment #39)
> 
> > Input number for decimals can be problematic for translations in my
> > experience. Please test for example with de-DE if the value is correctly
> > stored (it will force a decimal comma instead of . on input, which is
> > inconsistent with our other input fields right now)
> 
> I just tried on a Koha translated into German. Sounds that there no problem
> with the values stored. Any test plan to reproduice issue you talked about?

The problem we had was that it forced you to enter decimal values with comma but Koha didn't undertand them and then truncated the value. 1,1 = 1. This happened in fines in an older version, some more info here:

Bug 17098 - Validation problems with form field type "number" (decimal separator)

We are still using the patch described there. It might depend on the browser and version you use and will only appear if the template language is one that uses decimal commas.

But what remains, I think, is a problem of consistency. Having some inputs ask for comma and others for . is confusing to libraries.
Comment 47 Alex Arnaud 2018-05-24 07:46:07 UTC
(In reply to David Gustafsson from comment #43)
> Created attachment 75490 [details] [review] [review]
> Update Koha/Schema/Result/SearchField.pm

Hello David,

Your dedicated patch is ok. But, please, remove the changes on the schema file from the previous one: attachment 75458 [details] [review]. And add the bug number at the start of your commit messages.
Comment 48 David Gustafsson 2018-05-24 09:00:14 UTC
Created attachment 75519 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm
Comment 49 David Gustafsson 2018-05-24 09:04:16 UTC
Created attachment 75520 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm
Comment 50 David Gustafsson 2018-05-24 09:07:56 UTC
Created attachment 75521 [details] [review]
Bug 18316: Add weighting/relevancy options to ES query on simple search

The idea is the following: if some search field(s) are weighted in
search
engine config page, Koha will query ES on these fields only and with
the coresponding weights. Else, search is done on the entire record.

Test plan (having Koha working with Elasticsearch):
  - apply this patch,
  - try searches with and without weight defined on search fields

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 51 David Gustafsson 2018-05-24 09:08:11 UTC
Created attachment 75522 [details] [review]
Bug 18316: String changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 52 David Gustafsson 2018-05-24 09:08:38 UTC
Created attachment 75523 [details] [review]
Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 53 David Gustafsson 2018-05-24 09:08:44 UTC
Created attachment 75524 [details] [review]
Bug 18316: Change search field weight field to decimal
Comment 54 David Gustafsson 2018-05-24 09:08:50 UTC
Created attachment 75525 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm
Comment 55 David Gustafsson 2018-05-24 09:14:24 UTC
Created attachment 75526 [details] [review]
Bug 18316: Ability to weight search fields

Weight can be defined between 1 and 99.
only search fields mapped with biblios can be weighted

Test plan:
  - apply this patch,
  - update schema file (perl misc/devel/update_dbix_class_files.pl),
  - go to Administration > Search engine configuration,
  - in search fields tab, define weights for some fields and save,
  - check weights has been saved,
  - reset some weights (empty) and save,
  - check weights has been reset

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 56 David Gustafsson 2018-05-24 09:14:28 UTC
Created attachment 75527 [details] [review]
Bug 18316: Add weighting/relevancy options to ES query on simple search

The idea is the following: if some search field(s) are weighted in
search
engine config page, Koha will query ES on these fields only and with
the coresponding weights. Else, search is done on the entire record.

Test plan (having Koha working with Elasticsearch):
  - apply this patch,
  - try searches with and without weight defined on search fields

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 57 David Gustafsson 2018-05-24 09:14:32 UTC
Created attachment 75528 [details] [review]
Bug 18316: String changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 58 David Gustafsson 2018-05-24 09:14:35 UTC
Created attachment 75529 [details] [review]
Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 59 David Gustafsson 2018-05-24 09:14:39 UTC
Created attachment 75530 [details] [review]
Bug 18316: Change search field weight field to decimal
Comment 60 David Gustafsson 2018-05-24 09:14:43 UTC
Created attachment 75531 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm
Comment 61 David Gustafsson 2018-05-24 09:18:20 UTC
Not used to working with multiple patches in git bz, so had removed the schema changes locally, but used git bz incorrectly so it did not get included. Later managed to screw things up even more, but now managed to restore the patches (which I accidentally obsoleted) and fix the two last ones.
Comment 62 Séverine Queune 2018-05-24 12:45:24 UTC
The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 18316:
<h1>Something went wrong !</h1>Applying: Bug 18316: Ability to weight search fields
Using index info to reconstruct a base tree...
M       installer/data/mysql/kohastructure.sql
M       koha-tmpl/intranet-tmpl/prog/en/modules/admin/searchengine/elasticsearch/mappings.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/searchengine/elasticsearch/mappings.tt
Auto-merging installer/data/mysql/kohastructure.sql
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
error: Failed to merge in the changes.
Patch failed at 0001 Bug 18316: Ability to weight search fields
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 18316 - Add weighting/relevancy options to ElasticSearch

75526 - Bug 18316: Ability to weight search fields
75527 - Bug 18316: Add weighting/relevancy options to ES query on simple search
75528 - Bug 18316: String changes
75529 - Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set
75530 - Bug 18316: Change search field weight field to decimal
75531 - Bug 18316: Update Koha/Schema/Result/SearchField.pm

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-18316-Ability-to-weight-search-fields-MFIcJl.patch .


Same result on my test instance :/
Comment 63 David Gustafsson 2018-05-24 13:14:23 UTC
I think root cause of this is that the "Bug 18316: Ability to weight search fields" commit was created from a commit with the line:
"type` ENUM('', 'string', 'date', 'number', 'boolean', 'sum', 'isbn', 'stdno') ..." in /data/mysql/kohastructure.sql
when in current Koha master the line is:
`type` ENUM('', 'string', 'date', 'number', 'boolean', 'sum')

This is the reason why: https://github.com/Koha-Community/Koha/commit/cff62796ea9fc227c24fec8f8f3f153a715796ef

I can rebase the patch.
Comment 64 David Gustafsson 2018-05-24 13:22:42 UTC
Created attachment 75536 [details] [review]
Bug 18316: Ability to weight search fields

Weight can be defined between 1 and 99.
only search fields mapped with biblios can be weighted

Test plan:
  - apply this patch,
  - update schema file (perl misc/devel/update_dbix_class_files.pl),
  - go to Administration > Search engine configuration,
  - in search fields tab, define weights for some fields and save,
  - check weights has been saved,
  - reset some weights (empty) and save,
  - check weights has been reset

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 65 David Gustafsson 2018-05-24 13:22:46 UTC
Created attachment 75537 [details] [review]
Bug 18316: Add weighting/relevancy options to ES query on simple search

The idea is the following: if some search field(s) are weighted in
search
engine config page, Koha will query ES on these fields only and with
the coresponding weights. Else, search is done on the entire record.

Test plan (having Koha working with Elasticsearch):
  - apply this patch,
  - try searches with and without weight defined on search fields

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 66 David Gustafsson 2018-05-24 13:22:50 UTC
Created attachment 75538 [details] [review]
Bug 18316: String changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 67 David Gustafsson 2018-05-24 13:22:54 UTC
Created attachment 75539 [details] [review]
Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 68 David Gustafsson 2018-05-24 13:22:58 UTC
Created attachment 75540 [details] [review]
Bug 18316: Change search field weight field to decimal
Comment 69 David Gustafsson 2018-05-24 13:23:02 UTC
Created attachment 75541 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm
Comment 70 David Gustafsson 2018-05-24 13:24:08 UTC
Now patches should apply to latest master.
Comment 71 David Gustafsson 2018-05-25 17:15:12 UTC
Forgot to change status.
Comment 72 Séverine Queune 2018-05-30 08:54:01 UTC
Sorry David, I took a few days off and didn't try this patch earlier, it doesn't apply on last master...

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 18316: Ability to weight search fields
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
error: The following untracked working tree files would be overwritten by merge:
	installer/data/mysql/atomicupdate/bug_18316_add-weight-column.perl
	t/db_dependent/Koha/SearchField.t
Please move or remove them before you can merge.
Aborting
Failed to merge in the changes.
Patch failed at 0001 Bug 18316: Ability to weight search fields
Comment 73 Jonathan Druart 2018-05-30 14:40:00 UTC
(In reply to Séverine Queune from comment #72)
> error: The following untracked working tree files would be overwritten by
> merge:
> 	installer/data/mysql/atomicupdate/bug_18316_add-weight-column.perl
> 	t/db_dependent/Koha/SearchField.t

This error means you have these files in your repo and the patch is trying to create it.
You should remove them, then apply the patch.

However there is another conflict later:
CONFLICT (content): Merge conflict in Koha/Schema/Result/SearchField.pm
Comment 74 David Gustafsson 2018-05-30 14:51:36 UTC
Hmm, thought I had fixed the merge conflicts, but perhaps I was using a non up to date or something has changed since last time. Can have a look at it tomorrow if no one else does before then.
Comment 75 David Gustafsson 2018-05-31 12:11:53 UTC
Created attachment 75699 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm
Comment 76 David Gustafsson 2018-05-31 12:14:03 UTC
Yes, work for the master I did the rebase on, but the signature comment in SearchField.pm had changed since then. Now should work again.
Comment 77 Alex Arnaud 2018-06-01 08:38:09 UTC
The patch "Bug 18316: Change search field weight field to decimal" seems to break the ability to erase weights.

I.e
  - set weihgt to 1.25 for title,
  - save,
  - erase (empty) the weight input for title,
  - save,
  - weight is still here with the same value.
Comment 78 David Gustafsson 2018-06-01 09:43:34 UTC
Created attachment 75730 [details] [review]
Bug 18316: Fix field weight validation/deletion bug
Comment 79 David Gustafsson 2018-06-01 09:44:52 UTC
Ok! This should now be fixed.
Comment 80 Katrin Fischer 2018-08-10 12:17:53 UTC
I am sorry, I fixed the conflict in kohastructure.sql, but then the 4th patch won't apply:

sloth:/usr/local/vagrant/kohaclone_master (32-18316-qa-elastic_weight|AM 1/1) $ git bz apply --continue
Applying: Bug 18316: Ability to weight search fields
Applying: Bug 18316: Add weighting/relevancy options to ES query on simple search
Applying: Bug 18316: String changes
Applying: Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set
error: sha1 information is lacking or useless (Koha/SearchEngine/Elasticsearch/QueryBuilder.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-18316---QA-improvement---KohaSearchFieldsearch-dKxuaY.patch
Comment 81 Nick Clemens 2018-08-10 13:21:45 UTC
Created attachment 77661 [details] [review]
Bug 18316: Ability to weight search fields

Weight can be defined between 1 and 99.
only search fields mapped with biblios can be weighted

Test plan:
  - apply this patch,
  - update schema file (perl misc/devel/update_dbix_class_files.pl),
  - go to Administration > Search engine configuration,
  - in search fields tab, define weights for some fields and save,
  - check weights has been saved,
  - reset some weights (empty) and save,
  - check weights has been reset

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 82 Nick Clemens 2018-08-10 13:21:49 UTC
Created attachment 77662 [details] [review]
Bug 18316: Add weighting/relevancy options to ES query on simple search

The idea is the following: if some search field(s) are weighted in
search
engine config page, Koha will query ES on these fields only and with
the coresponding weights. Else, search is done on the entire record.

Test plan (having Koha working with Elasticsearch):
  - apply this patch,
  - try searches with and without weight defined on search fields

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 83 Nick Clemens 2018-08-10 13:21:54 UTC
Created attachment 77663 [details] [review]
Bug 18316: String changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 84 Nick Clemens 2018-08-10 13:21:59 UTC
Created attachment 77664 [details] [review]
Bug 18316 - QA improvement - Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 85 Nick Clemens 2018-08-10 13:22:04 UTC
Created attachment 77665 [details] [review]
Bug 18316: Change search field weight field to decimal
Comment 86 Nick Clemens 2018-08-10 13:22:09 UTC
Created attachment 77666 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm
Comment 87 Nick Clemens 2018-08-10 13:22:14 UTC
Created attachment 77667 [details] [review]
Bug 18316: Fix field weight validation/deletion bug
Comment 88 Katrin Fischer 2018-08-10 15:19:03 UTC
Giving this a go, any help/more eyes much appreaciated!

1) Questions

1.1) >This will allow to configure the way the simple search is weighted: 
>it will search in all fields for which a weight has been indicated. 
>If no weight was indicated it will search in all fields. (In addition, 
>this allows excluding some search fields from the simple search!)

From comments#13+ I tihnk this is no longer the case and it will be
always searched on all fields + boosting the weighted fields.
Can you confirm?

1.2) Should the patches on this bug tested in combination with
other patches from dependent bugs? A more detailed test plan would help
me a lot.

1.3) Could we deliver this with a basic default weighting?

2) QA tools and unit tests

2.1) FAIL	t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   FAIL	  valid
		You tried to plan twice 
        
2.2) When I remove the doubled up line, the test still fails:

t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 2/2 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t (Wstat: 65280 Tests: 0 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 2 tests but ran 0.
Files=1, Tests=0,  0 wallclock secs ( 0.01 usr  0.01 sys +  0.23 cusr  0.05 csys =  0.30 CPU)
Result: FAIL
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ vi t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t .. 1/3     # No tests run!

#   Failed test 'No tests run for subtest "build_query with weighted fields tests"'
#   at t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t line 167.
Can't locate object method "new" via package "t::lib::TestBuilder" (perhaps you forgot to load "t::lib::TestBuilder"?) at t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t line 127.
# Looks like your test exited with 255 just after 3.
t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/3 subtests 

2.3) * Commit title does not start with 'Bug XXXXX: ' - dfe2ab9

2.4) Other tests fail too:

t/db_dependent/Koha/SearchField.t .. 1/15 
#   Failed test 'Title weight is 25.'
#   at t/db_dependent/Koha/SearchField.t line 196.
#          got: '25.00'
#     expected: '25'

#   Failed test 'Subject weight is 15.'
#   at t/db_dependent/Koha/SearchField.t line 198.
#          got: '15.00'
#     expected: '15'

#   Failed test 'Author weight is 5.'
#   at t/db_dependent/Koha/SearchField.t line 200.
#          got: '5.00'
#     expected: '5'
# Looks like you failed 3 tests of 15.
t/db_dependent/Koha/SearchField.t .. Dubious, test returned 3 (wstat 768, 0x300)
Failed 3/15 subtests 

3) Code Review

3.1) Please use AFTER/BEFORE to make sure the column ends always in the correct spot.
+        $dbh->do( "ALTER TABLE `search_field` ADD COLUMN `weight` decimal(5,2) DEFAULT NULL" );

3.2) Can you explain this line?
<input type="hidden" name="advsearch" value="1"/>

4) Further tests

Adding, deleting and editing weights works. Waiting for follow-up and more info before I continue!
Comment 89 Joonas Kylmälä 2018-08-16 10:39:52 UTC
(In reply to Katrin Fischer from comment #88)
> 3.2) Can you explain this line?
> <input type="hidden" name="advsearch" value="1"/>

It is used in search.pl to enable weighted field search to any other search expect the one done in the Advanced search page (/cgi-bin/koha/catalogue/search.pl):

unless ( $cgi->param('advsearch') ) {
    $build_params->{weighted_fields} = 1;
}

So if the advsearch parameter equals '1' the weighted search is disabled.
Comment 90 Joonas Kylmälä 2018-08-16 11:15:26 UTC
Instead of adding a new 'advsearch' parameter maybe checking whether the 'idx' parameter is present would work just fine?
Comment 91 Katrin Fischer 2018-08-16 20:08:34 UTC
Thx, Joonas. Waiting for a follow-up here so I can continue testing - hopefully soon!
Comment 92 Katrin Fischer 2018-08-28 21:28:41 UTC
We had agreed that this bug should be next, still waiting, please fix!
Comment 93 Nick Clemens 2018-09-04 13:34:32 UTC
Created attachment 78396 [details] [review]
Bug 18316: Ability to weight search fields

Weight can be defined between 1 and 99.
only search fields mapped with biblios can be weighted

Test plan:
  - apply this patch,
  - update schema file (perl misc/devel/update_dbix_class_files.pl),
  - go to Administration > Search engine configuration,
  - in search fields tab, define weights for some fields and save,
  - check weights has been saved,
  - reset some weights (empty) and save,
  - check weights has been reset

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 94 Nick Clemens 2018-09-04 13:34:38 UTC
Created attachment 78397 [details] [review]
Bug 18316: Add weighting/relevancy options to ES query on simple search

The idea is the following: if some search field(s) are weighted in
search engine config page, Koha will query ES on all fields plus those with
the coresponding weights. Else, search is done on the entire record with
no weighting. The advanced search page is unaffected by these changes

Test plan (having Koha working with Elasticsearch):
  - apply this patch
  - have some weights defined for various fields
  - try searches from the search bar and from the advanced search page
  - confirm weighting affects the relevancy (in expected ways)
      e.g.
      1. search for 'a' from advanced search, note results
      2. give 'title' a weight
      3. search for 'a' using the simple search bar
      4. results with 'a' in the title should now be more relevant
  - confirm search results on advanced search page are unaffected

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 95 Nick Clemens 2018-09-04 13:34:43 UTC
Created attachment 78398 [details] [review]
Bug 18316: String changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 96 Nick Clemens 2018-09-04 13:34:49 UTC
Created attachment 78399 [details] [review]
Bug 18316: (follow-up) Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 97 Nick Clemens 2018-09-04 13:34:54 UTC
Created attachment 78400 [details] [review]
Bug 18316: Change search field weight field to decimal
Comment 98 Nick Clemens 2018-09-04 13:34:59 UTC
Created attachment 78401 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm
Comment 99 Nick Clemens 2018-09-04 13:35:04 UTC
Created attachment 78402 [details] [review]
Bug 18316: Fix field weight validation/deletion bug
Comment 100 Nick Clemens 2018-09-04 13:35:09 UTC
Created attachment 78403 [details] [review]
Bug 18316: (follow-up) Fix tests and update statement
Comment 101 Nick Clemens 2018-09-04 17:28:01 UTC
(In reply to Joonas Kylmälä from comment #90)
> Instead of adding a new 'advsearch' parameter maybe checking whether the
> 'idx' parameter is present would work just fine?

I think having the search parameter to weight or not could be useful - advsearch should get weighting in future but may offer specific weighting. We also may want the ability to sometimes search without weights
Comment 102 Julian Maurice 2018-09-25 10:08:24 UTC
Comment on attachment 78397 [details] [review]
Bug 18316: Add weighting/relevancy options to ES query on simple search

Review of attachment 78397 [details] [review]:
-----------------------------------------------------------------

::: Koha/SearchEngine/Elasticsearch/QueryBuilder.pm
@@ +92,5 @@
>              default_operator => 'AND',
>              default_field    => '_all',
>              lenient          => JSON::true,
> +            fields           => $options{fields},
> +            use_dis_max      => JSON::true,

From https://www.elastic.co/guide/en/elasticsearch/reference/6.0/breaking_60_search_changes.html

> The use_dis_max parameter for the query_string query has been removed. If
> provided, it will be ignored and issue a deprecation warning. The tie_breaker
> parameter must be used instead.

In elasticsearch 5, use_dis_max is true by default. So maybe it's not a good idea to specify it.
Comment 103 Julian Maurice 2018-09-25 10:32:43 UTC
I found a potential bug. To reproduce:
1. Use advanced search
2. Go to the detail page of a result
3. Click on "Return to results" (in the left column)
4. The advsearch URL param is gone

Also, I think it should be implemented for OPAC as well. Or at least show a message in the configuration page saying that this feature only works for intranet.
Comment 104 Joonas Kylmälä 2018-10-09 12:20:32 UTC
(In reply to Julian Maurice from comment #103)
> I found a potential bug. To reproduce:
> 1. Use advanced search
> 2. Go to the detail page of a result
> 3. Click on "Return to results" (in the left column)
> 4. The advsearch URL param is gone
> 

This bug already happens even without this patch. Also for example if you have specified multiple keywords in the search only one will be left after clicking the return button.
Comment 105 Julian Maurice 2018-10-09 13:39:29 UTC
(In reply to Joonas Kylmälä from comment #104)
> This bug already happens even without this patch. Also for example if you
> have specified multiple keywords in the search only one will be left after
> clicking the return button.

Is there already a bug report open for this problem ?
Comment 106 Alex Arnaud 2018-10-09 14:30:06 UTC
Created attachment 80278 [details] [review]
Bug 18316: remove use_dis_max

It is true by default on ES 5 and has been removed on ES 6
Comment 107 Joonas Kylmälä 2018-10-10 06:23:25 UTC
(In reply to Alex Arnaud from comment #106)
> Created attachment 80278 [details] [review] [review]
> Bug 18316: remove use_dis_max

There should be a (follow-up) in the title. Otherwise looks good and will sign-off after the change.
Comment 108 Joonas Kylmälä 2018-10-10 08:13:29 UTC
(In reply to Julian Maurice from comment #105)
> (In reply to Joonas Kylmälä from comment #104)
> > This bug already happens even without this patch. Also for example if you
> > have specified multiple keywords in the search only one will be left after
> > clicking the return button.
> 
> Is there already a bug report open for this problem ?

yes: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20114
Comment 109 Julian Maurice 2018-10-10 08:31:03 UTC
(In reply to Joonas Kylmälä from comment #108)
> > Is there already a bug report open for this problem ?
> 
> yes: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20114

Thanks
Comment 110 Alex Arnaud 2018-10-10 12:03:43 UTC
Created attachment 80335 [details] [review]
Bug 18316: (follow-up) remove use_dis_max

It is true by default on ES 5 and has been removed on ES 6
Comment 111 Ere Maijala 2018-10-23 12:06:07 UTC
Created attachment 81004 [details] [review]
Bug 18316: Ability to weight search fields

Weight can be defined between 1 and 99.
only search fields mapped with biblios can be weighted

Test plan:
  - apply this patch,
  - update schema file (perl misc/devel/update_dbix_class_files.pl),
  - go to Administration > Search engine configuration,
  - in search fields tab, define weights for some fields and save,
  - check weights has been saved,
  - reset some weights (empty) and save,
  - check weights has been reset

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 112 Ere Maijala 2018-10-23 12:06:14 UTC
Created attachment 81005 [details] [review]
Bug 18316: Add weighting/relevancy options to ES query on simple search

The idea is the following: if some search field(s) are weighted in
search engine config page, Koha will query ES on all fields plus those with
the coresponding weights. Else, search is done on the entire record with
no weighting. The advanced search page is unaffected by these changes

Test plan (having Koha working with Elasticsearch):
  - apply this patch
  - have some weights defined for various fields
  - try searches from the search bar and from the advanced search page
  - confirm weighting affects the relevancy (in expected ways)
      e.g.
      1. search for 'a' from advanced search, note results
      2. give 'title' a weight
      3. search for 'a' using the simple search bar
      4. results with 'a' in the title should now be more relevant
  - confirm search results on advanced search page are unaffected

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 113 Ere Maijala 2018-10-23 12:06:19 UTC
Created attachment 81006 [details] [review]
Bug 18316: String changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 114 Ere Maijala 2018-10-23 12:06:26 UTC
Created attachment 81007 [details] [review]
Bug 18316: (follow-up) Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 115 Ere Maijala 2018-10-23 12:06:31 UTC
Created attachment 81008 [details] [review]
Bug 18316: Change search field weight field to decimal

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 116 Ere Maijala 2018-10-23 12:06:37 UTC
Created attachment 81009 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm

https://bugs.koha-community.org/show_bug.cgi?id=18316
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 117 Ere Maijala 2018-10-23 12:06:43 UTC
Created attachment 81010 [details] [review]
Bug 18316: Fix field weight validation/deletion bug

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 118 Ere Maijala 2018-10-23 12:06:49 UTC
Created attachment 81011 [details] [review]
Bug 18316: (follow-up) Fix tests and update statement

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 119 Ere Maijala 2018-10-23 12:06:55 UTC
Created attachment 81012 [details] [review]
Bug 18316: (follow-up) remove use_dis_max

It is true by default on ES 5 and has been removed on ES 6

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 120 Ere Maijala 2018-10-23 12:08:19 UTC
I took the liberty of fixing the md5sum of the schema file while signing off.
Comment 121 Martin Renvoize 2018-10-29 14:44:22 UTC
Created attachment 81507 [details] [review]
Bug 18316: Ability to weight search fields

Weight can be defined between 1 and 99.
only search fields mapped with biblios can be weighted

Test plan:
  - apply this patch,
  - update schema file (perl misc/devel/update_dbix_class_files.pl),
  - go to Administration > Search engine configuration,
  - in search fields tab, define weights for some fields and save,
  - check weights has been saved,
  - reset some weights (empty) and save,
  - check weights has been reset

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 122 Martin Renvoize 2018-10-29 14:44:29 UTC
Created attachment 81508 [details] [review]
Bug 18316: Add weighting/relevancy options to ES query on simple search

The idea is the following: if some search field(s) are weighted in
search engine config page, Koha will query ES on all fields plus those with
the coresponding weights. Else, search is done on the entire record with
no weighting. The advanced search page is unaffected by these changes

Test plan (having Koha working with Elasticsearch):
  - apply this patch
  - have some weights defined for various fields
  - try searches from the search bar and from the advanced search page
  - confirm weighting affects the relevancy (in expected ways)
      e.g.
      1. search for 'a' from advanced search, note results
      2. give 'title' a weight
      3. search for 'a' using the simple search bar
      4. results with 'a' in the title should now be more relevant
  - confirm search results on advanced search page are unaffected

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 123 Martin Renvoize 2018-10-29 14:44:34 UTC
Created attachment 81509 [details] [review]
Bug 18316: String changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 124 Martin Renvoize 2018-10-29 14:44:40 UTC
Created attachment 81510 [details] [review]
Bug 18316: (follow-up) Koha::SearchField::search_marc_maps return a result set - code refactoring for gettings weighted fields - Koha::SearchFields::weighted_fields return a result set

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Rebased-by: Alex Arnaud <alex.arnaud@biblibre.com>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 125 Martin Renvoize 2018-10-29 14:44:46 UTC
Created attachment 81511 [details] [review]
Bug 18316: Change search field weight field to decimal

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 126 Martin Renvoize 2018-10-29 14:44:52 UTC
Created attachment 81512 [details] [review]
Bug 18316: Update Koha/Schema/Result/SearchField.pm

https://bugs.koha-community.org/show_bug.cgi?id=18316
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 127 Martin Renvoize 2018-10-29 14:44:58 UTC
Created attachment 81513 [details] [review]
Bug 18316: Fix field weight validation/deletion bug

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 128 Martin Renvoize 2018-10-29 14:45:05 UTC
Created attachment 81514 [details] [review]
Bug 18316: (follow-up) Fix tests and update statement

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 129 Martin Renvoize 2018-10-29 14:45:11 UTC
Created attachment 81515 [details] [review]
Bug 18316: (follow-up) remove use_dis_max

It is true by default on ES 5 and has been removed on ES 6

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 130 Martin Renvoize 2018-10-29 14:45:17 UTC
Created attachment 81516 [details] [review]
Bug 18316: (QA follow-up) Add final missing filter

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 131 Martin Renvoize 2018-10-29 14:47:52 UTC
Passing QA!

I can't find any regressions and the code looks reasonable.. well done for sticking with it everyone and thank you for the perseverance. 

One minor note.. the bug claims it's sponsored but I can't see by whom... if there is a sponsor can we have it added to the commits with the Sponsored-by trailer?
Comment 132 Nick Clemens 2018-11-07 23:16:21 UTC
Awesome work all!

Pushed to master for 18.11
Comment 133 Nick Clemens 2018-11-08 01:51:21 UTC
Created attachment 82050 [details] [review]
Bug 18316: (follow-up) Don't let 'fields' be undef and adjust tests
Comment 134 Nick Clemens 2018-11-08 02:25:13 UTC
(In reply to Nick Clemens from comment #133)
> Created attachment 82050 [details] [review] [review]
> Bug 18316: (follow-up) Don't let 'fields' be undef and adjust tests

Followup pushed to master
Comment 135 David Gustafsson 2018-11-14 16:09:57 UTC
Basing my current work on bug 20589 on this I think I discovered a regression. 
In "Bug 18316: Update Koha/Schema/Result/SearchField.pm" the many-to-many relationship (which has previously been manually added) was removed. This causes a crash when resetting or adding new mappings. I can supply a patch for this and will submit in this issue.
Comment 136 David Gustafsson 2018-11-14 16:14:27 UTC
Hmm, no second thought this seems to have been done in Koha-master. So perhaps release-manager took care of this. I will fix this in my local branch then and RM will have to fix this manually again where regenerating the schema files for that in case it get merged.
Comment 137 Martin Renvoize 2018-11-16 12:52:36 UTC
Enhancement, will not be backported to 18.05.x series.