Bug 18235 - Elastic search - Configurable facets
Summary: Elastic search - Configurable facets
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Jonathan Druart
QA Contact: Josef Moravec
URL:
Keywords: Manual
Depends on: 22495 14899 17169 18174
Blocks: 22636 18213
  Show dependency treegraph
 
Reported: 2017-03-08 18:57 UTC by Jonathan Druart
Modified: 2019-06-27 09:24 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
This new feature allows librarians with appropriate permissions to configure (show/hide/re-order) the search facets provided by elasticsearch.
Version(s) released in:


Attachments
Bug 18235: DBIx::Class schema changes (1.77 KB, patch)
2017-03-08 19:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: DB changes (2.11 KB, patch)
2017-03-08 19:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: ES - Facets configurable (16.20 KB, patch)
2017-03-08 19:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: DBIx::Class schema changes (1.83 KB, patch)
2017-03-23 12:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18235: DB changes (2.17 KB, patch)
2017-03-23 12:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18235: ES - Facets configurable (16.25 KB, patch)
2017-03-23 12:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18235: DBIx schema changes (1.53 KB, patch)
2017-06-30 08:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18235: DB changes (2.18 KB, patch)
2017-12-04 18:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: ES - Facets configurable (16.30 KB, patch)
2017-12-04 18:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: DBIx schema changes (1.50 KB, patch)
2017-12-04 18:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: DB changes (2.18 KB, patch)
2018-03-15 11:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18235: ES - Facets configurable (16.42 KB, patch)
2018-03-15 11:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18235: DBIx schema changes (1.49 KB, patch)
2018-03-15 11:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18235: DB changes (2.24 KB, patch)
2018-03-16 09:37 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 18235: ES - Facets configurable (16.31 KB, patch)
2018-03-16 09:37 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 18235: DBIx schema changes (1.55 KB, patch)
2018-03-16 09:37 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 18235 - Add unit tests for get_facetable_fields (4.19 KB, patch)
2018-03-23 15:56 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18235: Add unit tests for get_facetable_fields (3.74 KB, patch)
2018-04-20 13:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: Add missing POD to get_facetable_fields (864 bytes, patch)
2018-04-20 13:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: DB changes (2.39 KB, patch)
2018-06-12 14:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: Add unit tests for get_facetable_fields (3.72 KB, patch)
2018-06-12 14:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: ES - Facets configurable (16.27 KB, patch)
2019-02-16 19:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: Add missing POD to get_facetable_fields (864 bytes, patch)
2019-02-16 19:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: DB changes (2.34 KB, patch)
2019-02-16 19:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: Add unit tests for get_facetable_fields (4.20 KB, patch)
2019-02-16 19:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: DBIx schema changes (1.96 KB, patch)
2019-02-16 19:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: (QA follow-up) Fix POD (717 bytes, patch)
2019-03-01 07:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: (QA follow-up) Add missing filters (3.03 KB, patch)
2019-03-01 07:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: Update facet_order default value in atomic update (1.11 KB, patch)
2019-03-04 00:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: Do not call get_facetable_fields excessively (1.52 KB, patch)
2019-03-04 00:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18235: ES - Facets configurable (16.32 KB, patch)
2019-03-04 13:57 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: Add missing POD to get_facetable_fields (919 bytes, patch)
2019-03-04 13:57 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: DB changes (2.40 KB, patch)
2019-03-04 13:57 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: Add unit tests for get_facetable_fields (4.25 KB, patch)
2019-03-04 13:57 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: DBIx schema changes (2.02 KB, patch)
2019-03-04 13:57 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: (QA follow-up) Fix POD (772 bytes, patch)
2019-03-04 13:57 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: (QA follow-up) Add missing filters (3.08 KB, patch)
2019-03-04 13:57 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: Update facet_order default value in atomic update (1.16 KB, patch)
2019-03-04 13:57 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: Do not call get_facetable_fields excessively (1.57 KB, patch)
2019-03-04 13:58 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: (QA follow-up) Make db update idempotent (3.07 KB, patch)
2019-03-04 13:58 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: ES - Facets configurable (16.21 KB, patch)
2019-03-08 08:10 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: Add missing POD to get_facetable_fields (919 bytes, patch)
2019-03-08 08:11 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: DB changes (2.40 KB, patch)
2019-03-08 08:11 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: Add unit tests for get_facetable_fields (4.25 KB, patch)
2019-03-08 08:11 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: DBIx schema changes (2.02 KB, patch)
2019-03-08 08:11 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: (QA follow-up) Fix POD (772 bytes, patch)
2019-03-08 08:11 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: (QA follow-up) Add missing filters (3.08 KB, patch)
2019-03-08 08:11 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: Update facet_order default value in atomic update (1.16 KB, patch)
2019-03-08 08:11 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: Do not call get_facetable_fields excessively (1.57 KB, patch)
2019-03-08 08:11 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: (QA follow-up) Make db update idempotent (3.07 KB, patch)
2019-03-08 08:11 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: (QA follow-up) Series facet name after rebase (4.44 KB, patch)
2019-03-08 08:11 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: ES - Facets configurable (16.21 KB, patch)
2019-03-23 09:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: Add missing POD to get_facetable_fields (919 bytes, patch)
2019-03-23 09:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: DB changes (2.40 KB, patch)
2019-03-23 09:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: Add unit tests for get_facetable_fields (4.25 KB, patch)
2019-03-23 09:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: DBIx schema changes (2.02 KB, patch)
2019-03-23 09:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: (QA follow-up) Fix POD (772 bytes, patch)
2019-03-23 09:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: (QA follow-up) Add missing filters (3.08 KB, patch)
2019-03-23 09:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: Update facet_order default value in atomic update (1.16 KB, patch)
2019-03-23 09:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: Do not call get_facetable_fields excessively (1.57 KB, patch)
2019-03-23 09:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: (QA follow-up) Make db update idempotent (3.07 KB, patch)
2019-03-23 09:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: (QA follow-up) Series facet name after rebase (4.44 KB, patch)
2019-03-23 09:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18253: (QA follow-up) Restore facet_order for su-geo field in mappings file (874 bytes, patch)
2019-03-23 09:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: (QA follow-up) Restore facet_order for su-geo field in mappings file (819 bytes, patch)
2019-03-23 09:45 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18235: (RM follow-up) fix mappings reset (1.45 KB, patch)
2019-03-28 15:45 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 Jonathan Druart 2017-03-08 18:57:38 UTC
From the Elastic mappings configuration page, the librarians should be able to manage facets (display/hide and order).
Comment 1 Jonathan Druart 2017-03-08 19:07:34 UTC
Created attachment 60931 [details] [review]
Bug 18235: DBIx::Class schema changes
Comment 2 Jonathan Druart 2017-03-08 19:07:39 UTC
Created attachment 60932 [details] [review]
Bug 18235: DB changes
Comment 3 Jonathan Druart 2017-03-08 19:07:46 UTC
Created attachment 60933 [details] [review]
Bug 18235: ES - Facets configurable

This patch adds a new section 'Facet order' in the Biblio tab of the
'Search engine configuration' admin page of the Elastic mappings.
The idea is to let the librarians define the facet to display and order
them as their needs.

The ergonomic is not perfect and I am open to any suggestions.

Test plan:
Move up and down the field list to order the facets
Hide/show some facets
At the OPAC and the staff interface you should see the changes on the
search result page.
Comment 4 Nick Clemens 2017-03-23 12:11:28 UTC
Created attachment 61528 [details] [review]
Bug 18235: DBIx::Class schema changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Nick Clemens 2017-03-23 12:11:41 UTC
Created attachment 61529 [details] [review]
Bug 18235: DB changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 Nick Clemens 2017-03-23 12:11:45 UTC
Created attachment 61530 [details] [review]
Bug 18235: ES - Facets configurable

This patch adds a new section 'Facet order' in the Biblio tab of the
'Search engine configuration' admin page of the Elastic mappings.
The idea is to let the librarians define the facet to display and order
them as their needs.

The ergonomic is not perfect and I am open to any suggestions.

Test plan:
Move up and down the field list to order the facets
Hide/show some facets
At the OPAC and the staff interface you should see the changes on the
search result page.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Nick Clemens 2017-03-23 12:14:34 UTC
I like this alot - many libraries want to control the facets a bit more and this is helpful.

Still a few tings that would be nice (but I am not suggesting they are needed in this patchset)
-labels shouldn't be hardcoded - staff shoul eb able to set them
-we should add more options to what can be faceted- could be out of control but technically any field should be able to be used
-ES has better handling for faceting (can provide subsets of existing/current results and allow to step in/out of facets)

FYI not complaints
Comment 8 Jonathan Druart 2017-03-23 14:35:53 UTC
Nick, yes you are right of course. This is only the first step.

However there are two problems:
1. Translatability: if staff changes or add new labels, they will not be translated (we could cover some in the template)
2. Shared includes files: What I said for 2 years now... We are using the same files for elastic and Zebra, so we have to stick to ugly structure the template expects. That's very bad, we need to create separate files to allow more flexibility.
Comment 9 Jonathan Druart 2017-06-16 15:27:17 UTC
Marcel, They apply correctly, just skip the first patch and regenerate the schema files.
Comment 10 Marcel de Rooy 2017-06-30 08:44:50 UTC
If I select say Author and set them all to Facetable No in the Search fields list on the Biblios tab, I would not expect Author to be still in the Facet order list?

Changing status for feedback
Comment 11 Marcel de Rooy 2017-06-30 08:45:58 UTC
Created attachment 64747 [details] [review]
Bug 18235: DBIx schema changes

Fresh changes for SearchField, facet_order.
Comment 12 Nick Clemens 2017-07-03 16:32:14 UTC
(In reply to Marcel de Rooy from comment #10)
> If I select say Author and set them all to Facetable No in the Search fields
> list on the Biblios tab, I would not expect Author to be still in the Facet
> order list?

I agree, this is not ideal, but I think that comes when we stop hardcoding the displayable facets all together - I would say beyond the scope of this bug
Comment 13 Jonathan Druart 2017-07-05 18:28:51 UTC
Yes indeed you are right, this is not ideal and might be confusing.
We should add more JS code to handle that correctly, but I am not willing to write it.
However I am still thinking that this feature is needed.
Comment 14 David Bourgault 2017-11-10 21:50:48 UTC
Also patch does not apply.
Comment 15 Jonathan Druart 2017-12-04 18:30:29 UTC
Created attachment 69485 [details] [review]
Bug 18235: DB changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Jonathan Druart 2017-12-04 18:30:35 UTC
Created attachment 69486 [details] [review]
Bug 18235: ES - Facets configurable

This patch adds a new section 'Facet order' in the Biblio tab of the
'Search engine configuration' admin page of the Elastic mappings.
The idea is to let the librarians define the facet to display and order
them as their needs.

The ergonomic is not perfect and I am open to any suggestions.

Test plan:
Move up and down the field list to order the facets
Hide/show some facets
At the OPAC and the staff interface you should see the changes on the
search result page.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Jonathan Druart 2017-12-04 18:30:40 UTC
Created attachment 69487 [details] [review]
Bug 18235: DBIx schema changes

Fresh changes for SearchField, facet_order.
Comment 18 Nick Clemens 2018-03-15 11:56:31 UTC
Created attachment 72936 [details] [review]
Bug 18235: DB changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>


Current status: Signed Off
Comment 19 Nick Clemens 2018-03-15 11:56:37 UTC
Created attachment 72937 [details] [review]
Bug 18235: ES - Facets configurable

This patch adds a new section 'Facet order' in the Biblio tab of the
'Search engine configuration' admin page of the Elastic mappings.
The idea is to let the librarians define the facet to display and order
them as their needs.

The ergonomic is not perfect and I am open to any suggestions.

Test plan:
Move up and down the field list to order the facets
Hide/show some facets
At the OPAC and the staff interface you should see the changes on the
search result page.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 20 Nick Clemens 2018-03-15 11:56:41 UTC
Created attachment 72938 [details] [review]
Bug 18235: DBIx schema changes

Fresh changes for SearchField, facet_order.
Comment 21 Nicolas Legrand 2018-03-16 09:37:11 UTC
Created attachment 72999 [details] [review]
Bug 18235: DB changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 22 Nicolas Legrand 2018-03-16 09:37:15 UTC
Created attachment 73000 [details] [review]
Bug 18235: ES - Facets configurable

This patch adds a new section 'Facet order' in the Biblio tab of the
'Search engine configuration' admin page of the Elastic mappings.
The idea is to let the librarians define the facet to display and order
them as their needs.

The ergonomic is not perfect and I am open to any suggestions.

Test plan:
Move up and down the field list to order the facets
Hide/show some facets
Rebuild index
At the OPAC and the staff interface you should see the changes on the
search result page.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 23 Nicolas Legrand 2018-03-16 09:37:18 UTC
Created attachment 73001 [details] [review]
Bug 18235: DBIx schema changes

Fresh changes for SearchField, facet_order.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 24 Nicolas Legrand 2018-03-16 09:42:55 UTC
Added missing curly, removed a <<<<<HEAD thingy, added index rebuild in test plan.
Comment 25 Alex Arnaud 2018-03-22 09:14:45 UTC
Ergonomic suggestion:
  - move the the facet table in a new tab (discussed with some interested libraries). Currently it is not visible at the first glance.

QA suggestion:
  - rename bug_xxxxx.sql with the appropriate bug number,
  - add unit test for get_facetable_fields

Then, nice feature and i would like this to move forward.
Comment 26 Séverine Queune 2018-03-23 10:06:20 UTC
+1 for the new 'Facet order' tab !
Comment 27 Jonathan Druart 2018-03-23 13:30:12 UTC
I did not create a new tab because we already have 3: 
    Search fields
    Biblios
    Authorities
and I do not think it makes sense to have a 4th "Facet order" which will refer to biblio search only.
Comment 28 Alex Arnaud 2018-03-23 15:56:12 UTC
Created attachment 73193 [details] [review]
Bug 18235 - Add unit tests for get_facetable_fields

rename atomic update file
Comment 29 Marcel de Rooy 2018-04-13 07:59:46 UTC
QA: Looking here
Comment 30 Marcel de Rooy 2018-04-13 08:01:54 UTC
There is a lot of noise from YAML while running qa tools and a git manipulation fail too.
Comment 31 Jonathan Druart 2018-04-13 13:58:17 UTC
(In reply to Marcel de Rooy from comment #30)
> There is a lot of noise from YAML while running qa tools and a git
> manipulation fail too.

Noise appears before and after the patch, so not related.
The git manipulation is on the atomic update (it should not have been moved in the last patch), I will fix it on pushing.
Comment 32 Marcel de Rooy 2018-04-20 08:45:01 UTC
Resuming here
Comment 33 Marcel de Rooy 2018-04-20 08:49:24 UTC
* Commit title does not start with 'Bug XXXXX: ' - 9400c92
POD is missing for get_facetable_fields
Comment 34 Jonathan Druart 2018-04-20 13:38:47 UTC
(In reply to Marcel de Rooy from comment #33)
> * Commit title does not start with 'Bug XXXXX: ' - 9400c92
> POD is missing for get_facetable_fields

That's all?
Comment 35 Jonathan Druart 2018-04-20 13:45:26 UTC
Created attachment 74642 [details] [review]
Bug 18235: Add unit tests for get_facetable_fields

rename atomic update file
Comment 36 Jonathan Druart 2018-04-20 13:45:30 UTC
Created attachment 74643 [details] [review]
Bug 18235: Add missing POD to get_facetable_fields
Comment 37 Alex Arnaud 2018-06-12 14:17:50 UTC
(In reply to Jonathan Druart from comment #35)
> Created attachment 74642 [details] [review] [review]
> Bug 18235: Add unit tests for get_facetable_fields
> 
> rename atomic update file

Looks like this patch does not what it was named for. Atomic update file's name is still Bug_xxxxxxx.
Comment 38 Jonathan Druart 2018-06-12 14:38:26 UTC
Created attachment 75995 [details] [review]
Bug 18235: DB changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 39 Jonathan Druart 2018-06-12 14:39:35 UTC
Created attachment 75996 [details] [review]
Bug 18235: Add unit tests for get_facetable_fields
Comment 40 Jonathan Druart 2018-06-12 14:41:07 UTC
I have amended the last 2 patches: 1. rename atomic update file and 2. remove the line from commit message that was not related to the patch.
Comment 41 Katrin Fischer 2019-02-14 07:28:34 UTC
This patch doesn't apply right now. Might not be a hard fix if you are familiar with the Elasticsearch code. Can we get a rebase please?

@Ere - It'd also be great if you could have a look here.
Comment 42 Jonathan Druart 2019-02-16 19:50:48 UTC
Created attachment 85212 [details] [review]
Bug 18235: ES - Facets configurable

This patch adds a new section 'Facet order' in the Biblio tab of the
'Search engine configuration' admin page of the Elastic mappings.
The idea is to let the librarians define the facet to display and order
them as their needs.

The ergonomic is not perfect and I am open to any suggestions.

Test plan:
Move up and down the field list to order the facets
Hide/show some facets
Rebuild index
At the OPAC and the staff interface you should see the changes on the
search result page.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 43 Jonathan Druart 2019-02-16 19:50:55 UTC
Created attachment 85213 [details] [review]
Bug 18235: Add missing POD to get_facetable_fields
Comment 44 Jonathan Druart 2019-02-16 19:51:03 UTC
Created attachment 85214 [details] [review]
Bug 18235: DB changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 45 Jonathan Druart 2019-02-16 19:51:10 UTC
Created attachment 85215 [details] [review]
Bug 18235: Add unit tests for get_facetable_fields
Comment 46 Jonathan Druart 2019-02-16 19:51:17 UTC
Created attachment 85216 [details] [review]
Bug 18235: DBIx schema changes

Fresh changes for SearchField, facet_order.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 47 Jonathan Druart 2019-02-16 19:55:15 UTC
Those patches have been waiting for one year, only for a commit message problem.

As I do not plan to rebase them endlessly, I think it would be great for libraries using ES to get a green stamp from people involved in the ES development.
Comment 48 Josef Moravec 2019-03-01 07:28:34 UTC
Created attachment 85885 [details] [review]
Bug 18235: (QA follow-up) Fix POD
Comment 49 Josef Moravec 2019-03-01 07:28:39 UTC
Created attachment 85886 [details] [review]
Bug 18235: (QA follow-up) Add missing filters
Comment 50 Josef Moravec 2019-03-01 07:29:32 UTC
Comment on attachment 85214 [details] [review]
Bug 18235: DB changes

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

::: installer/data/mysql/atomicupdate/bug_18235-add-search_field-facet_order.sql
@@ +1,1 @@
> +ALTER TABLE search_field ADD COLUMN facet_order TINYINT(4) DEFAULT '0' AFTER weight;

Here is default 0

::: installer/data/mysql/kohastructure.sql
@@ +1493,4 @@
>    `label` varchar(255) NOT NULL COMMENT 'the human readable name of the field, for display',
>    `type` ENUM('', 'string', 'date', 'number', 'boolean', 'sum', 'isbn', 'stdno') NOT NULL COMMENT 'what type of data this holds, relevant when storing it in the search engine',
>    `weight` decimal(5,2) DEFAULT NULL,
> +  `facet_order` TINYINT(4) DEFAULT NULL COMMENT 'the order place of the field in facet list if faceted',

And here NULL
Comment 51 Josef Moravec 2019-03-01 07:35:48 UTC
Comment on attachment 85212 [details] [review]
Bug 18235: ES - Facets configurable

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

::: Koha/SearchEngine/Elasticsearch.pm
@@ +971,5 @@
> +    my @faceted_fields = Koha::SearchFields->search(
> +        { name => { -in => \@search_field_names }, facet_order => { '!=' => undef } }, { order_by => ['facet_order'] }
> +    );
> +    my @not_faceted_fields = Koha::SearchFields->search(
> +        { name => { -in => \@search_field_names }, facet_order => undef }, { order_by => ['facet_order'] }

Why do you retrieve fields from database in 2 queries, you always use them all.

::: admin/searchengine/elasticsearch/mappings.pl
@@ +215,4 @@
>      push @all_search_fields, $search_field_unblessed;
>  }
>  
> +my @facetable_fields = Koha::SearchEngine::Elasticsearch->get_facetable_fields();

get_facetable_fields is called 3 or 4 times every run of mappings.pl.
Comment 52 Josef Moravec 2019-03-01 07:38:35 UTC
Hi Jonathan, great job, I have just some small questions (see above)

I also adding Owen if he can advice us how to do the UI better, it is not obvious that the facet order could be changed, also the facet table is at the end of page and easy to oversee...
Comment 53 Owen Leonard 2019-03-01 15:31:34 UTC
(In reply to Josef Moravec from comment #52)
> the facet table is at
> the end of page and easy to oversee...

I wonder if it should be in a separate tab?
Comment 54 Jonathan Druart 2019-03-01 23:01:43 UTC
I would support an integration ASAP then we could discuss the ergonomic (patches submitted 2 years ago).

Unless somebody else is willing to make the changes ;)
Comment 55 Owen Leonard 2019-03-02 20:14:44 UTC
I've got some interface changes in process, but they don't need to hold this up.
Comment 56 Jonathan Druart 2019-03-04 00:35:50 UTC
(In reply to Josef Moravec from comment #50)
> Comment on attachment 85214 [details] [review] [review]
> Bug 18235: DB changes
> 
> Review of attachment 85214 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> :::
> installer/data/mysql/atomicupdate/bug_18235-add-search_field-facet_order.sql
> @@ +1,1 @@
> > +ALTER TABLE search_field ADD COLUMN facet_order TINYINT(4) DEFAULT '0' AFTER weight;
> 
> Here is default 0
> 
> ::: installer/data/mysql/kohastructure.sql
> @@ +1493,4 @@
> >    `label` varchar(255) NOT NULL COMMENT 'the human readable name of the field, for display',
> >    `type` ENUM('', 'string', 'date', 'number', 'boolean', 'sum', 'isbn', 'stdno') NOT NULL COMMENT 'what type of data this holds, relevant when storing it in the search engine',
> >    `weight` decimal(5,2) DEFAULT NULL,
> > +  `facet_order` TINYINT(4) DEFAULT NULL COMMENT 'the order place of the field in facet list if faceted',
> 
> And here NULL

Right, must be NULL in the atomic update file.

(In reply to Josef Moravec from comment #51)
> Comment on attachment 85212 [details] [review] [review]
> Bug 18235: ES - Facets configurable
> 
> Review of attachment 85212 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/SearchEngine/Elasticsearch.pm
> @@ +971,5 @@
> > +    my @faceted_fields = Koha::SearchFields->search(
> > +        { name => { -in => \@search_field_names }, facet_order => { '!=' => undef } }, { order_by => ['facet_order'] }
> > +    );
> > +    my @not_faceted_fields = Koha::SearchFields->search(
> > +        { name => { -in => \@search_field_names }, facet_order => undef }, { order_by => ['facet_order'] }
> 
> Why do you retrieve fields from database in 2 queries, you always use them
> all.

To have the not faceted fields at the end of the array. I agree it's a lazy way to handle that, but it works :)


> ::: admin/searchengine/elasticsearch/mappings.pl
> @@ +215,4 @@
> >      push @all_search_fields, $search_field_unblessed;
> >  }
> >  
> > +my @facetable_fields = Koha::SearchEngine::Elasticsearch->get_facetable_fields();
> 
> get_facetable_fields is called 3 or 4 times every run of mappings.pl.

Yep, could be lowered to 2.
Comment 57 Jonathan Druart 2019-03-04 00:37:11 UTC
Created attachment 85969 [details] [review]
Bug 18235: Update facet_order default value in atomic update
Comment 58 Jonathan Druart 2019-03-04 00:37:18 UTC
Created attachment 85970 [details] [review]
Bug 18235: Do not call get_facetable_fields excessively
Comment 59 Josef Moravec 2019-03-04 13:33:50 UTC
(In reply to Owen Leonard from comment #53)
> (In reply to Josef Moravec from comment #52)
> > the facet table is at
> > the end of page and easy to oversee...
> 
> I wonder if it should be in a separate tab?

I think it is good idea.
Comment 60 Josef Moravec 2019-03-04 13:37:06 UTC
(In reply to Jonathan Druart from comment #54)
> I would support an integration ASAP then we could discuss the ergonomic
> (patches submitted 2 years ago).
> 
> Unless somebody else is willing to make the changes ;)

(In reply to Owen Leonard from comment #55)
> I've got some interface changes in process, but they don't need to hold this
> up.

I agree, we could pass and push this soon and then work on UI
Comment 61 Josef Moravec 2019-03-04 13:57:17 UTC
Created attachment 85983 [details] [review]
Bug 18235: ES - Facets configurable

This patch adds a new section 'Facet order' in the Biblio tab of the
'Search engine configuration' admin page of the Elastic mappings.
The idea is to let the librarians define the facet to display and order
them as their needs.

The ergonomic is not perfect and I am open to any suggestions.

Test plan:
Move up and down the field list to order the facets
Hide/show some facets
Rebuild index
At the OPAC and the staff interface you should see the changes on the
search result page.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 62 Josef Moravec 2019-03-04 13:57:23 UTC
Created attachment 85984 [details] [review]
Bug 18235: Add missing POD to get_facetable_fields

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 63 Josef Moravec 2019-03-04 13:57:28 UTC
Created attachment 85985 [details] [review]
Bug 18235: DB changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 64 Josef Moravec 2019-03-04 13:57:34 UTC
Created attachment 85986 [details] [review]
Bug 18235: Add unit tests for get_facetable_fields

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 65 Josef Moravec 2019-03-04 13:57:39 UTC
Created attachment 85987 [details] [review]
Bug 18235: DBIx schema changes

Fresh changes for SearchField, facet_order.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 66 Josef Moravec 2019-03-04 13:57:45 UTC
Created attachment 85988 [details] [review]
Bug 18235: (QA follow-up) Fix POD

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 67 Josef Moravec 2019-03-04 13:57:50 UTC
Created attachment 85989 [details] [review]
Bug 18235: (QA follow-up) Add missing filters

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 68 Josef Moravec 2019-03-04 13:57:55 UTC
Created attachment 85990 [details] [review]
Bug 18235: Update facet_order default value in atomic update

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 69 Josef Moravec 2019-03-04 13:58:00 UTC
Created attachment 85991 [details] [review]
Bug 18235: Do not call get_facetable_fields excessively

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 70 Josef Moravec 2019-03-04 13:58:06 UTC
Created attachment 85992 [details] [review]
Bug 18235: (QA follow-up) Make db update idempotent

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 71 Nick Clemens 2019-03-07 21:23:01 UTC
Conflict in the mappings file, please rebase
Comment 72 Josef Moravec 2019-03-08 08:10:55 UTC
Created attachment 86340 [details] [review]
Bug 18235: ES - Facets configurable

This patch adds a new section 'Facet order' in the Biblio tab of the
'Search engine configuration' admin page of the Elastic mappings.
The idea is to let the librarians define the facet to display and order
them as their needs.

The ergonomic is not perfect and I am open to any suggestions.

Test plan:
Move up and down the field list to order the facets
Hide/show some facets
Rebuild index
At the OPAC and the staff interface you should see the changes on the
search result page.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 73 Josef Moravec 2019-03-08 08:11:01 UTC
Created attachment 86341 [details] [review]
Bug 18235: Add missing POD to get_facetable_fields

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 74 Josef Moravec 2019-03-08 08:11:06 UTC
Created attachment 86342 [details] [review]
Bug 18235: DB changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 75 Josef Moravec 2019-03-08 08:11:11 UTC
Created attachment 86343 [details] [review]
Bug 18235: Add unit tests for get_facetable_fields

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 76 Josef Moravec 2019-03-08 08:11:16 UTC
Created attachment 86344 [details] [review]
Bug 18235: DBIx schema changes

Fresh changes for SearchField, facet_order.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 77 Josef Moravec 2019-03-08 08:11:21 UTC
Created attachment 86345 [details] [review]
Bug 18235: (QA follow-up) Fix POD

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 78 Josef Moravec 2019-03-08 08:11:26 UTC
Created attachment 86346 [details] [review]
Bug 18235: (QA follow-up) Add missing filters

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 79 Josef Moravec 2019-03-08 08:11:31 UTC
Created attachment 86347 [details] [review]
Bug 18235: Update facet_order default value in atomic update

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 80 Josef Moravec 2019-03-08 08:11:36 UTC
Created attachment 86348 [details] [review]
Bug 18235: Do not call get_facetable_fields excessively

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 81 Josef Moravec 2019-03-08 08:11:41 UTC
Created attachment 86349 [details] [review]
Bug 18235: (QA follow-up) Make db update idempotent

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 82 Josef Moravec 2019-03-08 08:11:46 UTC
Created attachment 86350 [details] [review]
Bug 18235: (QA follow-up) Series facet name after rebase

Bug 19575 changes many field names so "se" facet field became "title-series"
Comment 83 Josef Moravec 2019-03-08 08:41:23 UTC
I rebased it and add a follow-up to adapt to 19575, just not sure what to do, with su-geo, look like it was removed in bug 1957...
Comment 84 Josef Moravec 2019-03-13 10:39:34 UTC
Bug 22495 is going to restore su-geo, so I think we should wait with this again :(
Comment 85 Josef Moravec 2019-03-23 09:44:02 UTC
Created attachment 86944 [details] [review]
Bug 18235: ES - Facets configurable

This patch adds a new section 'Facet order' in the Biblio tab of the
'Search engine configuration' admin page of the Elastic mappings.
The idea is to let the librarians define the facet to display and order
them as their needs.

The ergonomic is not perfect and I am open to any suggestions.

Test plan:
Move up and down the field list to order the facets
Hide/show some facets
Rebuild index
At the OPAC and the staff interface you should see the changes on the
search result page.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 86 Josef Moravec 2019-03-23 09:44:07 UTC
Created attachment 86945 [details] [review]
Bug 18235: Add missing POD to get_facetable_fields

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 87 Josef Moravec 2019-03-23 09:44:12 UTC
Created attachment 86946 [details] [review]
Bug 18235: DB changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 88 Josef Moravec 2019-03-23 09:44:17 UTC
Created attachment 86947 [details] [review]
Bug 18235: Add unit tests for get_facetable_fields

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 89 Josef Moravec 2019-03-23 09:44:22 UTC
Created attachment 86948 [details] [review]
Bug 18235: DBIx schema changes

Fresh changes for SearchField, facet_order.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 90 Josef Moravec 2019-03-23 09:44:27 UTC
Created attachment 86949 [details] [review]
Bug 18235: (QA follow-up) Fix POD

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 91 Josef Moravec 2019-03-23 09:44:32 UTC
Created attachment 86950 [details] [review]
Bug 18235: (QA follow-up) Add missing filters

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 92 Josef Moravec 2019-03-23 09:44:37 UTC
Created attachment 86951 [details] [review]
Bug 18235: Update facet_order default value in atomic update

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 93 Josef Moravec 2019-03-23 09:44:42 UTC
Created attachment 86952 [details] [review]
Bug 18235: Do not call get_facetable_fields excessively

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 94 Josef Moravec 2019-03-23 09:44:47 UTC
Created attachment 86953 [details] [review]
Bug 18235: (QA follow-up) Make db update idempotent

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 95 Josef Moravec 2019-03-23 09:44:53 UTC
Created attachment 86954 [details] [review]
Bug 18235: (QA follow-up) Series facet name after rebase

Bug 19575 changes many field names so "se" facet field became "title-series"
Comment 96 Josef Moravec 2019-03-23 09:44:58 UTC
Created attachment 86955 [details] [review]
Bug 18253: (QA follow-up) Restore facet_order for su-geo field in mappings file

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 97 Josef Moravec 2019-03-23 09:45:48 UTC
Created attachment 86956 [details] [review]
Bug 18235: (QA follow-up) Restore facet_order for su-geo field in mappings file

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 98 Josef Moravec 2019-03-23 09:46:43 UTC
Rebased and adapted to bug 22495.
Comment 99 Nick Clemens 2019-03-28 15:45:30 UTC
Created attachment 87141 [details] [review]
Bug 18235: (RM follow-up) fix mappings reset

Without this we could not recreate mappings as there are no defaults for
the additional fields - this restores behavoiur from before patches
Comment 100 Nick Clemens 2019-03-28 16:01:18 UTC
Awesome work all!

Pushed to master for 19.05
Comment 101 Tomás Cohen Arazi 2019-03-28 16:01:48 UTC
(In reply to Nick Clemens from comment #100)
> Awesome work all!
> 
> Pushed to master for 19.05

\o/
Comment 102 Martin Renvoize 2019-04-01 13:56:39 UTC
New feature will not be backported to 18.11.x series.