Created attachment 163133 [details] [review]
Bug 35138: Make ES facetable fields configurable - POC

Move the ES facet fields that are currently hardcoded in the codebase to a new
ES_FACETS authorised value category.

Librarians can then add/remove facet fields and modify their lib/description.

Remaining work:
1. Adjust OPAC templates
2. AV is not ideal here, we should certainly have a dedicated DB table for that.
It will bring more flexibility.
We can image:
  * display or not (could be opac/staff/both/none)
  * define the size per field (number of facet to display)
  * order: move search_field.facet_order to this new table. In my
    testing this is not worker, they are displayed alpha (??)
3. Adjust the UI

Still no translatability however.

(In reply to Jonathan Druart from comment #1)
> Still no translatability however.

Translating using JS should be trivial as there will be an id on the label:

facets.inc
<span id="facet-[% facets_loo.type_link_value | html %]">[% facets_loo.label | html %]</span>

Thanks Jonathan for your proof of concept. 
We testet the patch that uses an authorized value for the configuration of ElasticSearch facets. 
Everything in the patch worked as expected.

(In reply to Jonathan Druart from comment #1)
>   * order: move search_field.facet_order to this new table. In my
>     testing this is not worker, they are displayed alpha (??)

*not working*

It's actually broken in master but is going to be fixed by bug 36190.

Created attachment 163680 [details] [review]
Bug 35138: Add the ability to manage ES facets

This new feature allows to manage facets for Elasticsearch from the
administration page.
Prior to this the facet fields were hardcoded in the codebase.

Librarians can then add/remove facet fields and modify their label.

Test plan:
1. Create a new search field and set it a label different than its name.
2. Save
3. Go the bibliographic mapping tab
4. Add 1+ mapping for this search field (Searchable and facetable must be "Yes")
5. Add, reorder, remove new facets
6. Save and reindex your records
7. Search and notice the new facet list

QA: There are several wrong things in this area (ES + facets code,
everything, pm, pl, tt AND on this administration page). I have done my
best to clean the code as much as I could and keep the code cleaner
after than before. But there are still a lot to do.
There are still inconsistencies on this page (like we need to save to
see the changes applied to the other tables), but this is clearly out of
the scope of this bug report.
Another enhancement would be to move the facet list to a different DB
table, that could bring more flexibility:
  * display or not (could be opac/staff/both/none)
  * define the size per field (number of facet to display)
  * order: move search_field.facet_order to this new table.
But, again, it's a lot more work.

More work is done in this area, please see related bug reports.

Sponsored-by: The Research University in the Helmholtz Association (KIT)

Created attachment 163681 [details] [review]
Bug 35138: Adjust existing data

We want the label from the DB displayed on the UI so we are adjusting
the yaml and the DB.

Sponsored-by: The Research University in the Helmholtz Association (KIT)

Created attachment 163682 [details] [review]
Bug 35138: Mark DisplayLibraryFacets as zebra only

We can know add/remove homelibrary and holdinglibray from the config
page, this syspref is no longer needed for ES

Sponsored-by: The Research University in the Helmholtz Association (KIT)

Created attachment 163683 [details] [review]
Bug 35138: UI changes to add/remove a facet

Sponsored-by: The Research University in the Helmholtz Association (KIT)

Created attachment 163684 [details] [review]
Bug 35138: A bit of cleaning - UI

This can be moved to its own bug report, it's only a bit of cleaning
trying to keep the code cleaner after than before.
No behaviour changes expected.

Sponsored-by: The Research University in the Helmholtz Association (KIT)

Created attachment 163896 [details] [review]
Bug 35138: Add the ability to manage ES facets

This new feature allows to manage facets for Elasticsearch from the
administration page.
Prior to this the facet fields were hardcoded in the codebase.

Librarians can then add/remove facet fields and modify their label.

Test plan:
1. Create a new search field and set it a label different than its name.
2. Save
3. Go the bibliographic mapping tab
4. Add 1+ mapping for this search field (Searchable and facetable must be "Yes")
5. Add, reorder, remove new facets
6. Save and reindex your records
7. Search and notice the new facet list

QA: There are several wrong things in this area (ES + facets code,
everything, pm, pl, tt AND on this administration page). I have done my
best to clean the code as much as I could and keep the code cleaner
after than before. But there are still a lot to do.
There are still inconsistencies on this page (like we need to save to
see the changes applied to the other tables), but this is clearly out of
the scope of this bug report.
Another enhancement would be to move the facet list to a different DB
table, that could bring more flexibility:
  * display or not (could be opac/staff/both/none)
  * define the size per field (number of facet to display)
  * order: move search_field.facet_order to this new table.
But, again, it's a lot more work.

More work is done in this area, please see related bug reports.

Sponsored-by: The Research University in the Helmholtz Association (KIT)
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>

Created attachment 163897 [details] [review]
Bug 35138: Adjust existing data

We want the label from the DB displayed on the UI so we are adjusting
the yaml and the DB.

Sponsored-by: The Research University in the Helmholtz Association (KIT)
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>

Created attachment 163898 [details] [review]
Bug 35138: Mark DisplayLibraryFacets as zebra only

We can know add/remove homelibrary and holdinglibray from the config
page, this syspref is no longer needed for ES

Sponsored-by: The Research University in the Helmholtz Association (KIT)
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>

Created attachment 163899 [details] [review]
Bug 35138: UI changes to add/remove a facet

Sponsored-by: The Research University in the Helmholtz Association (KIT)
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>

Created attachment 163900 [details] [review]
Bug 35138: A bit of cleaning - UI

This can be moved to its own bug report, it's only a bit of cleaning
trying to keep the code cleaner after than before.
No behaviour changes expected.

Sponsored-by: The Research University in the Helmholtz Association (KIT)
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>

Created attachment 164811 [details] [review]
Bug 35138: Add the ability to manage ES facets

This new feature allows to manage facets for Elasticsearch from the
administration page.
Prior to this the facet fields were hardcoded in the codebase.

Librarians can then add/remove facet fields and modify their label.

Test plan:
1. Create a new search field and set it a label different than its name.
2. Save
3. Go the bibliographic mapping tab
4. Add 1+ mapping for this search field (Searchable and facetable must be "Yes")
5. Add, reorder, remove new facets
6. Save and reindex your records
7. Search and notice the new facet list

QA: There are several wrong things in this area (ES + facets code,
everything, pm, pl, tt AND on this administration page). I have done my
best to clean the code as much as I could and keep the code cleaner
after than before. But there are still a lot to do.
There are still inconsistencies on this page (like we need to save to
see the changes applied to the other tables), but this is clearly out of
the scope of this bug report.
Another enhancement would be to move the facet list to a different DB
table, that could bring more flexibility:
  * display or not (could be opac/staff/both/none)
  * define the size per field (number of facet to display)
  * order: move search_field.facet_order to this new table.
But, again, it's a lot more work.

More work is done in this area, please see related bug reports.

Sponsored-by: The Research University in the Helmholtz Association (KIT)
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 164812 [details] [review]
Bug 35138: Adjust existing data

We want the label from the DB displayed on the UI so we are adjusting
the yaml and the DB.

Sponsored-by: The Research University in the Helmholtz Association (KIT)
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 164813 [details] [review]
Bug 35138: Mark DisplayLibraryFacets as zebra only

We can know add/remove homelibrary and holdinglibray from the config
page, this syspref is no longer needed for ES

Sponsored-by: The Research University in the Helmholtz Association (KIT)
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 164814 [details] [review]
Bug 35138: UI changes to add/remove a facet

Sponsored-by: The Research University in the Helmholtz Association (KIT)
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 164815 [details] [review]
Bug 35138: A bit of cleaning - UI

This can be moved to its own bug report, it's only a bit of cleaning
trying to keep the code cleaner after than before.
No behaviour changes expected.

Sponsored-by: The Research University in the Helmholtz Association (KIT)
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 164816 [details] [review]
Bug 35138: (QA followup) Tidy update

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

I think this patch makes the best of a difficult code area, and makes the management of facets in ES for more intuitive, and uses some of the things in the editor as they were intended.

The change to DisplayLibraryFacets will need to be highlighted in release notes so that libraries are aware they will need to reconfiugre

(In reply to Nick Clemens from comment #21)
> The change to DisplayLibraryFacets will need to be highlighted in release
> notes so that libraries are aware they will need to reconfiugre

They won't need to reconfigure anything. We are dealing with that in the atomic update. Or are you talking about something else?

I really want this for 24.05, please help me make it happen:

1) Failing unit test (blocker)

It looks like you forgot to adjust the t/db_dependent/Koha_Elasticsearch.t to the new name of the method. I tried to fix it, but even after changing the method names the test still keeps failing. I think some more is needed. Please provide a follow-up ASAP.

2) Labels cannot be edited (blocker)

The test plan states:

"Librarians can then add/remove facet fields and modify their label."

But I haven't been able to update the label for my own custom facets. The field remains "locked" and doesn't ever appear as an input field.


Tiny things I had fixed on my branch:

3) The database update is missing a success message. I had used:

+        say $out "Updated DisplayLibraryFacets and search field configuration";


4) Tiny typo: exist -> exists

-            alert(__("SearchField '%s' already exist".format(search_field_name)));
+            alert(__("SearchField '%s' already exists".format(search_field_name)));


For a separate bug maybe:

?) Show delete button only after saving?

At the moment the delete button shows for a newly added field, but it doesn't work. I think it would make sense to hide it, until the page has been saved.

(In reply to Katrin Fischer from comment #23)
> I really want this for 24.05, please help me make it happen:
> 
> 2) Labels cannot be edited (blocker)
> 
> The test plan states:
> 
> "Librarians can then add/remove facet fields and modify their label."
> 
> But I haven't been able to update the label for my own custom facets. The
> field remains "locked" and doesn't ever appear as an input field.


The Labels can be edited on the first tab 'Search fields' as it was before.

>The Labels can be edited on the first tab 'Search fields' as it was before.

I missed that, thanks!

Maybe someone could lend a hand with the test?

Created attachment 165811 [details] [review]
Bug 35138: Add the ability to manage ES facets

This new feature allows to manage facets for Elasticsearch from the
administration page.
Prior to this the facet fields were hardcoded in the codebase.

Librarians can then add/remove facet fields and modify their label.

Test plan:
1. Create a new search field and set it a label different than its name.
2. Save
3. Go the bibliographic mapping tab
4. Add 1+ mapping for this search field (Searchable and facetable must be "Yes")
5. Add, reorder, remove new facets
6. Save and reindex your records
7. Search and notice the new facet list

QA: There are several wrong things in this area (ES + facets code,
everything, pm, pl, tt AND on this administration page). I have done my
best to clean the code as much as I could and keep the code cleaner
after than before. But there are still a lot to do.
There are still inconsistencies on this page (like we need to save to
see the changes applied to the other tables), but this is clearly out of
the scope of this bug report.
Another enhancement would be to move the facet list to a different DB
table, that could bring more flexibility:
  * display or not (could be opac/staff/both/none)
  * define the size per field (number of facet to display)
  * order: move search_field.facet_order to this new table.
But, again, it's a lot more work.

More work is done in this area, please see related bug reports.

Sponsored-by: The Research University in the Helmholtz Association (KIT)
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 165812 [details] [review]
Bug 35138: Adjust existing data

We want the label from the DB displayed on the UI so we are adjusting
the yaml and the DB.

Sponsored-by: The Research University in the Helmholtz Association (KIT)
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 165813 [details] [review]
Bug 35138: Mark DisplayLibraryFacets as zebra only

We can know add/remove homelibrary and holdinglibray from the config
page, this syspref is no longer needed for ES

Sponsored-by: The Research University in the Helmholtz Association (KIT)
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 165814 [details] [review]
Bug 35138: UI changes to add/remove a facet

Sponsored-by: The Research University in the Helmholtz Association (KIT)
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 165815 [details] [review]
Bug 35138: A bit of cleaning - UI

This can be moved to its own bug report, it's only a bit of cleaning
trying to keep the code cleaner after than before.
No behaviour changes expected.

Sponsored-by: The Research University in the Helmholtz Association (KIT)
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 165816 [details] [review]
Bug 35138: (QA followup) Tidy update

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

Created attachment 165817 [details] [review]
Bug 35138: Fix Koha_Elasticsearch.t

Had to rebase because of previous tidy patches.

Adjusted the tests: fields are faceted when the facet_order is not null.

Created attachment 165818 [details] [review]
Bug 35138: Fix delete button for newly added facet

Pushed for 24.05!

Well done everyone, thank you!

Some facet heading labels are not displayed in the OPAC search results when using Elasticsearch.

I'm not sure if this is related to this bug, or is unrelated.

I've created bug 36750 for the issue.

Created attachment 166402 [details] [review]
Bug 35138: (follow-up) Mark DisplayLibraryFacets as zebra only

The tests are no longer relevant

Follow-up pushed as RM Assist.

Great feature

Not backported to 23.11.x