Bug 19482 - Elasticsearch - prevent removal / editing of required indexes
Summary: Elasticsearch - prevent removal / editing of required indexes
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Alex Arnaud
URL:
Keywords:
Depends on: 23204 19481
Blocks: 20388
  Show dependency treegraph
 
Reported: 2017-10-18 10:52 UTC by Nick Clemens
Modified: 2020-11-16 21:12 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Manual merge request:
Text to go in the release notes:
These patches add the option to define some fields as 'mandatory' in the mappings.yaml file for elasticsearch. A 'mandatory' field cannot be deleted from the staff interface and must be mapped to at least one MARC field. The intention is to prevent removal of search fields that are required for Koha functionality - built in sorting fields, issues count, etc. These patches add the marker to 'issues' and 'title' - more fields may be marked in the future
Version(s) released in:
20.11.00


Attachments
Bug 19482 - DB changes (2.02 KB, patch)
2017-10-18 13:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - Add support for defining 'mandatory' mappings (16.15 KB, patch)
2017-10-18 13:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - Schema changes (DO NOT PUSH) (1.47 KB, patch)
2017-10-18 13:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - Make reset mappings reset also reset fields (2.10 KB, patch)
2017-11-21 15:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - DB changes (2.04 KB, patch)
2018-03-14 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - Add support for defining 'mandatory' mappings (16.27 KB, patch)
2018-03-14 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - Schema changes (DO NOT PUSH) (1.47 KB, patch)
2018-03-14 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - DB changes (2.09 KB, patch)
2018-03-14 15:07 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 19482 - Add support for defining 'mandatory' mappings (16.37 KB, patch)
2018-03-14 15:07 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 19482 - Schema changes (DO NOT PUSH) (1.47 KB, patch)
2018-03-14 15:07 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 19482 - DB changes (2.11 KB, patch)
2018-07-16 15:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - Add support for defining 'mandatory' mappings (13.17 KB, patch)
2018-07-16 15:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: (follow-up) QA fixes (6.70 KB, patch)
2018-07-16 15:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: DO NOT PUSH Schema changes (1.40 KB, patch)
2018-07-16 15:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - DB changes (2.11 KB, patch)
2018-10-18 12:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - Add support for defining 'mandatory' mappings (13.56 KB, patch)
2018-10-18 12:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: (follow-up) QA fixes (7.13 KB, patch)
2018-10-18 12:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: DO NOT PUSH Schema changes (1.40 KB, patch)
2018-10-18 12:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - DB changes (2.07 KB, patch)
2019-10-01 13:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - Add support for defining 'mandatory' mappings (21.33 KB, patch)
2019-10-01 13:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: SCHEMA CHANGES - DO NOT PUSH (1.50 KB, patch)
2019-10-01 13:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - DB changes (2.05 KB, patch)
2019-10-01 14:08 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 19482 - Add support for defining 'mandatory' mappings (21.37 KB, patch)
2019-10-01 14:08 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 19482: SCHEMA CHANGES - DO NOT PUSH (1.54 KB, patch)
2019-10-01 14:08 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 19482 - DB changes (1.94 KB, patch)
2020-01-10 14:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - Add support for defining 'mandatory' mappings (22.77 KB, patch)
2020-01-10 14:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: SCHEMA CHANGES _ DO NOT PUSH (1.48 KB, patch)
2020-01-10 14:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - DB changes (2.04 KB, patch)
2020-03-20 18:24 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 19482 - Add support for defining 'mandatory' mappings (22.86 KB, patch)
2020-03-20 18:24 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 19482: SCHEMA CHANGES _ DO NOT PUSH (1.59 KB, patch)
2020-03-20 18:24 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 19482 - DB changes (2.05 KB, patch)
2020-08-18 11:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482 - Add support for defining 'mandatory' mappings (22.88 KB, patch)
2020-08-18 11:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: (follow-up) Fix select and adjust export of new field (2.62 KB, patch)
2020-08-18 11:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: (optional) Export mappings and update shipped file (70.87 KB, patch)
2020-08-18 11:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: DO NOT PUSH - Schema changes (1.48 KB, patch)
2020-08-18 11:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: DB changes (2.05 KB, patch)
2020-10-16 16:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: Add support for defining 'mandatory' mappings (23.47 KB, patch)
2020-10-16 16:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: (follow-up) Fix select and adjust export of new field (2.62 KB, patch)
2020-10-16 16:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: (follow-up) Add missing filters and boolean switch to schema (3.29 KB, patch)
2020-10-16 16:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: DO NOT PUSH: SCHEMA UPDATES (1.46 KB, patch)
2020-10-16 16:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: (follow-up) Don't set readonly as a variable, use conditionals (2.61 KB, patch)
2020-10-16 16:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: (follow-up) Fix update, add tests, restore closing </td> (6.01 KB, patch)
2020-10-25 02:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19482: DB changes (2.11 KB, patch)
2020-10-25 02:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19482: Add support for defining 'mandatory' mappings (23.53 KB, patch)
2020-10-25 02:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19482: (follow-up) Fix select and adjust export of new field (2.68 KB, patch)
2020-10-25 02:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19482: (follow-up) Add missing filters and boolean switch to schema (3.34 KB, patch)
2020-10-25 02:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19482: DO NOT PUSH: SCHEMA UPDATES (1.51 KB, patch)
2020-10-25 02:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19482: (follow-up) Don't set readonly as a variable, use conditionals (2.67 KB, patch)
2020-10-25 02:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19482: (follow-up) Fix update, add tests, restore closing </td> (6.07 KB, patch)
2020-10-25 02:29 UTC, Katrin Fischer
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-10-18 10:52:07 UTC
Sorting options are currently hardcoded, in view of future z39 support with ES and other interfaces it is probably best to provide a base set of fields that are indexed and sortable. Using the base set of sorting this patchset will prevent editing/removal of these fields by default
Comment 1 Nick Clemens 2017-10-18 13:19:39 UTC
Created attachment 68246 [details] [review]
Bug 19482 - DB changes
Comment 2 Nick Clemens 2017-10-18 13:19:41 UTC
Created attachment 68247 [details] [review]
Bug 19482 - Add support for defining 'mandatory' mappings

To test:
1 - Apply patch
2 - Reset Es mapping:
/cgi-bin/koha/admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1
3 - issues should now be uneditable and not removable
Comment 3 Nick Clemens 2017-10-18 13:19:43 UTC
Created attachment 68248 [details] [review]
Bug 19482 - Schema changes (DO NOT PUSH)
Comment 4 David Bourgault 2017-11-03 20:07:47 UTC
The 2nd patch does not apply on current master. (Lacking or useless SHA1)
Comment 5 David Bourgault 2017-11-08 16:40:00 UTC
Correction, patch does apply; I had simply overlooked the dependency.

I applied the patch and update the database (updatedatabase.pl) however, even after reset-ing the mappings I could not get issues to show up as readonly. I tried reset-ing again (twice) to no avail. 

Looking into the database I saw the the mandatory flag wasn't being set properly. After switching it manually it did show up as readonly, as expected.

However I'd suggest displaying the type as a <label> instead of a readonly text input.
Comment 6 Nick Clemens 2017-11-21 15:26:28 UTC
Created attachment 69250 [details] [review]
Bug 19482 - Make reset mappings reset also reset fields

I think this makes sense, we should reset to the mappings file
completely

To test:
1 - Change a field label in es
2 - Save it
3 - Reset mapping
4 - Label should be reset
Comment 7 Nick Clemens 2017-11-21 15:28:47 UTC

(In reply to David Bourgault from comment #5)
> I applied the patch and update the database (updatedatabase.pl) however,
> even after reset-ing the mappings I could not get issues to show up as
> readonly. I tried reset-ing again (twice) to no avail. 

Indeed, it seems a reset does not actually perform a full reset, it only resets the marc to field mappings

I added an initial patch to ensure that data would be updated, however, I am wondering if we should alter reset to do a full reload, deleting all existing and loading the file fresh? It hsould probably be its own bug if so
Comment 8 Nick Clemens 2018-03-14 12:27:18 UTC
Created attachment 72846 [details] [review]
Bug 19482 - DB changes
Comment 9 Nick Clemens 2018-03-14 12:27:21 UTC
Created attachment 72847 [details] [review]
Bug 19482 - Add support for defining 'mandatory' mappings

To test:
1 - Apply patch
2 - Clear all ES mappings form the db: DELETE FROM search_fields; DELETE
FROM search_marc_maps; DELETE FROM search_marc_to_fields;
3 - Reset Es mapping:
/cgi-bin/koha/admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1
4 - issues should now be uneditable and not removable
Comment 10 Nick Clemens 2018-03-14 12:27:28 UTC
Created attachment 72848 [details] [review]
Bug 19482 - Schema changes (DO NOT PUSH)
Comment 11 Nick Clemens 2018-03-14 12:28:19 UTC
I updated the instructions, this should work if the settings are entirely cleared - that should be dealt with on a separate bug though. This should be testable
Comment 12 Nicolas Legrand 2018-03-14 15:07:36 UTC
Created attachment 72878 [details] [review]
Bug 19482 - DB changes

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 13 Nicolas Legrand 2018-03-14 15:07:44 UTC
Created attachment 72879 [details] [review]
Bug 19482 - Add support for defining 'mandatory' mappings

To test:
1 - Apply patch
2 - ./installer/data/mysql/updatedatabase.pl
3 - Clear all ES mappings form the db: DELETE FROM search_field;
DELETE FROM search_marc_map; DELETE FROM search_marc_to_field;
4 - Reset ES mapping:
/cgi-bin/koha/admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1
5 - issues should now be uneditable and not removable

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 14 Nicolas Legrand 2018-03-14 15:07:49 UTC
Created attachment 72880 [details] [review]
Bug 19482 - Schema changes (DO NOT PUSH)
Comment 15 Nicolas Legrand 2018-03-14 15:10:12 UTC
I have slightly changed the test plan (added updatedatabase, and also correct table names with the DELETE part). Hope it's OK !
Comment 16 Alex Arnaud 2018-06-19 14:00:14 UTC
Comment on attachment 72879 [details] [review]
Bug 19482 - Add support for defining 'mandatory' mappings

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

::: Koha/SearchEngine/Elasticsearch.pm
@@ +286,5 @@
>          while ( my ( $field_name, $data ) = each %$fields ) {
>              my $field_type = $data->{type};
>              my $field_label = $data->{label};
> +            my $field_mandatory = $data->{mandatory};
> +            warn $field_mandatory;

Debug thing?

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/searchengine/elasticsearch/mappings.tt
@@ +143,3 @@
>                        <td>
> +                        [% IF is_readonly %]
> +                            <input type="text" name="search_field_type" value="[% search_field.type FILTER ucfirst%]" [% is_readonly %] />

Maybe this readonly input should be sized? This cause the input is larger than the selects and so expand the "Type" column. Or maybe use a <label> as suggested by David
Comment 17 Nick Clemens 2018-07-16 15:54:37 UTC
Created attachment 77007 [details] [review]
Bug 19482 - DB changes

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 18 Nick Clemens 2018-07-16 15:54:41 UTC
Created attachment 77008 [details] [review]
Bug 19482 - Add support for defining 'mandatory' mappings

To test:
1 - Apply patch
2 - ./installer/data/mysql/updatedatabase.pl
3 - Clear all ES mappings form the db: DELETE FROM search_field;
DELETE FROM search_marc_map; DELETE FROM search_marc_to_field;
4 - Reset ES mapping:
/cgi-bin/koha/admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1
5 - issues should now be uneditable and not removable

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 19 Nick Clemens 2018-07-16 15:54:44 UTC
Created attachment 77009 [details] [review]
Bug 19482: (follow-up) QA fixes

Remove debug statement
Use disaled select to match other fields
Comment 20 Nick Clemens 2018-07-16 15:54:48 UTC
Created attachment 77010 [details] [review]
Bug 19482: DO NOT PUSH Schema changes
Comment 21 Alex Arnaud 2018-10-18 08:21:11 UTC
Applying: Bug 19482 - DB changes
Applying: Bug 19482 - Add support for defining 'mandatory' mappings
Using index info to reconstruct a base tree...
M       admin/searchengine/elasticsearch/mappings.pl
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
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/searchengine/elasticsearch/mappings.tt
Auto-merging admin/searchengine/elasticsearch/mappings.pl
error: Failed to merge in the changes.
Patch failed at 0001 Bug 19482 - Add support for defining 'mandatory' mappings
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".
Patch left in /tmp/Bug-19482---Add-support-for-defining-mandatory-map-fo6gJt.patch
Comment 22 Nick Clemens 2018-10-18 12:25:31 UTC
Created attachment 80817 [details] [review]
Bug 19482 - DB changes

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 23 Nick Clemens 2018-10-18 12:25:36 UTC
Created attachment 80818 [details] [review]
Bug 19482 - Add support for defining 'mandatory' mappings

To test:
1 - Apply patch
2 - ./installer/data/mysql/updatedatabase.pl
3 - Clear all ES mappings form the db: DELETE FROM search_field;
DELETE FROM search_marc_map; DELETE FROM search_marc_to_field;
4 - Reset ES mapping:
/cgi-bin/koha/admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1
5 - issues should now be uneditable and not removable

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 24 Nick Clemens 2018-10-18 12:25:42 UTC
Created attachment 80819 [details] [review]
Bug 19482: (follow-up) QA fixes

Remove debug statement
Use disaled select to match other fields
Comment 25 Nick Clemens 2018-10-18 12:25:47 UTC
Created attachment 80820 [details] [review]
Bug 19482: DO NOT PUSH Schema changes
Comment 26 Alex Arnaud 2018-10-18 14:38:17 UTC
Here is a (late, sorry) fonctional point of view:

The current patches make all the mapping bloqued for a search field marked as mandatory. I think we need some mandatory mappings but not all. For exemple, title in 245$a should be a mandatory mapping but not 247$a (it's an example).

On my own instance i have about 40 mappings on title. So they are all uneditable/undeletable. Also, if i make a mistake by creating a wrong mapping for a mandatory field, i can't delete it.
Comment 27 Nick Clemens 2019-10-01 13:28:32 UTC
Created attachment 93366 [details] [review]
Bug 19482 - DB changes

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 28 Nick Clemens 2019-10-01 13:28:45 UTC
Created attachment 93367 [details] [review]
Bug 19482 - Add support for defining 'mandatory' mappings

To test:
1 - Apply patch
2 - ./installer/data/mysql/updatedatabase.pl
3 - Reset ES mapping: Administration->Search engine configuration , button at bottom of page
4 - 'issues' and 'title' mapping under 'search fields' should be mandatory and not editable
5 - On 'Bibliographic records' tab you should not be able to delete the single entry for issues
6 - You should be able to delete 'title' mappings, however, at the final one you should be stopped by javascript
7 - Bonus: force remove the last mapping from the page using developer tools - attempt to save and should be warned of missing mandatory mapping
Comment 29 Nick Clemens 2019-10-01 13:28:49 UTC
Created attachment 93368 [details] [review]
Bug 19482: SCHEMA CHANGES - DO NOT PUSH
Comment 30 Nicolas Legrand 2019-10-01 14:08:43 UTC
Created attachment 93371 [details] [review]
Bug 19482 - DB changes

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 31 Nicolas Legrand 2019-10-01 14:08:46 UTC
Created attachment 93372 [details] [review]
Bug 19482 - Add support for defining 'mandatory' mappings

To test:
1 - Apply patch
2 - ./installer/data/mysql/updatedatabase.pl
3 - Reset ES mapping: Administration->Search engine configuration , button at bottom of page
4 - 'issues' and 'title' mapping under 'search fields' should be mandatory and not editable
5 - On 'Bibliographic records' tab you should not be able to delete the single entry for issues
6 - You should be able to delete 'title' mappings, however, at the final one you should be stopped by javascript
7 - Bonus: force remove the last mapping from the page using developer tools - attempt to save and should be warned of missing mandatory mapping

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 32 Nicolas Legrand 2019-10-01 14:08:50 UTC
Created attachment 93373 [details] [review]
Bug 19482: SCHEMA CHANGES - DO NOT PUSH

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 33 Nicolas Legrand 2019-10-01 14:15:56 UTC
Thank you Nick!

Tested with Unimarc, so there isn't any issues index defined for the Bibliographic records section. Everything went as it should for the rest of it.

It could be puzzling for the Unimarc community, because our modifications will be blocked and we will see the message :

“You attempted to delete all mappings for a required index, you must leave at least one mapping”

until we add an issues index in Bibliographic records.

I'm not sure this should be fixed here or in a mapping.yaml patch. Listing the name of the required field missing in the error message could be a nice improvement.
Comment 34 Bouzid Fergani 2019-11-08 20:34:26 UTC
Hi,
We have conflict when applying.
Bug 19482 - Elasticsearch - prevent removal / editing of required indexes

93371 - Bug 19482 - DB changes
93372 - Bug 19482 - Add support for defining 'mandatory' mappings
93373 - Bug 19482: SCHEMA CHANGES - DO NOT PUSH

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 19482 - DB changes
Using index info to reconstruct a base tree...
M	installer/data/mysql/kohastructure.sql
Falling back to patching base and 3-way merge...
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 19482 - DB changes
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".
Patch left in /tmp/Bug-19482---DB-changes-FC4ixa.patch
Comment 35 Jonathan Druart 2020-01-10 14:09:48 UTC
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
Comment 36 Nick Clemens 2020-01-10 14:50:10 UTC
Created attachment 97226 [details] [review]
Bug 19482 - DB changes

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 37 Nick Clemens 2020-01-10 14:50:14 UTC
Created attachment 97227 [details] [review]
Bug 19482 - Add support for defining 'mandatory' mappings

To test:
1 - Apply patch
2 - ./installer/data/mysql/updatedatabase.pl
3 - Reset ES mapping: Administration->Search engine configuration , button at bottom of page
4 - 'issues' and 'title' mapping under 'search fields' should be mandatory and not editable
5 - On 'Bibliographic records' tab you should not be able to delete the single entry for issues
6 - You should be able to delete 'title' mappings, however, at the final one you should be stopped by javascript
7 - Bonus: force remove the last mapping from the page using developer tools - attempt to save and should be warned of missing mandatory mapping

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 38 Nick Clemens 2020-01-10 14:50:18 UTC
Created attachment 97228 [details] [review]
Bug 19482: SCHEMA CHANGES _ DO NOT PUSH
Comment 39 Bouzid Fergani 2020-03-20 18:24:15 UTC
Created attachment 101312 [details] [review]
Bug 19482 - DB changes

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

https://bugs.koha-community.org/show_bug.cgi?id=21357
Comment 40 Bouzid Fergani 2020-03-20 18:24:21 UTC
Created attachment 101313 [details] [review]
Bug 19482 - Add support for defining 'mandatory' mappings

To test:
1 - Apply patch
2 - ./installer/data/mysql/updatedatabase.pl
3 - Reset ES mapping: Administration->Search engine configuration , button at bottom of page
4 - 'issues' and 'title' mapping under 'search fields' should be mandatory and not editable
5 - On 'Bibliographic records' tab you should not be able to delete the single entry for issues
6 - You should be able to delete 'title' mappings, however, at the final one you should be stopped by javascript
7 - Bonus: force remove the last mapping from the page using developer tools - attempt to save and should be warned of missing mandatory mapping

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

https://bugs.koha-community.org/show_bug.cgi?id=21357
Comment 41 Bouzid Fergani 2020-03-20 18:24:27 UTC
Created attachment 101314 [details] [review]
Bug 19482: SCHEMA CHANGES _ DO NOT PUSH

Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

https://bugs.koha-community.org/show_bug.cgi?id=21357
Comment 42 Jonathan Druart 2020-03-26 15:14:42 UTC
Changes to mappings.yaml will need to be rebuilt on top of bug 23204 (sorry!)
Comment 43 Alex Arnaud 2020-06-12 14:44:10 UTC
Comment on attachment 101313 [details] [review]
Bug 19482 - Add support for defining 'mandatory' mappings

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/searchengine/elasticsearch/mappings.tt
@@ +200,5 @@
> +                              [% IF search_field.type == "string" %]
> +                                <option value="string" selected="selected">String</option>
> +                              [% ELSE %]
> +                                <option value="string">String</option>
> +                              [% END %]

That makes the list empty instead of only disabled.
Comment 44 Alex Arnaud 2020-06-12 14:45:34 UTC
As Jonathan said, Koha::SearchEngine::Elasticsearch::raw_elasticsearch_mappings need to be updated with the new mandatory field.
Comment 45 Nick Clemens 2020-08-18 11:37:36 UTC
Created attachment 108467 [details] [review]
Bug 19482 - DB changes

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

https://bugs.koha-community.org/show_bug.cgi?id=21357
Comment 46 Nick Clemens 2020-08-18 11:37:39 UTC
Created attachment 108468 [details] [review]
Bug 19482 - Add support for defining 'mandatory' mappings

To test:
1 - Apply patch
2 - ./installer/data/mysql/updatedatabase.pl
3 - Reset ES mapping: Administration->Search engine configuration , button at bottom of page
4 - 'issues' and 'title' mapping under 'search fields' should be mandatory and not editable
5 - On 'Bibliographic records' tab you should not be able to delete the single entry for issues
6 - You should be able to delete 'title' mappings, however, at the final one you should be stopped by javascript
7 - Bonus: force remove the last mapping from the page using developer tools - attempt to save and should be warned of missing mandatory mapping

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

https://bugs.koha-community.org/show_bug.cgi?id=21357
Comment 47 Nick Clemens 2020-08-18 11:37:43 UTC
Created attachment 108469 [details] [review]
Bug 19482: (follow-up) Fix select and adjust export of new field
Comment 48 Nick Clemens 2020-08-18 11:37:48 UTC
Created attachment 108470 [details] [review]
Bug 19482: (optional) Export mappings and update shipped file
Comment 49 Nick Clemens 2020-08-18 11:37:52 UTC
Created attachment 108471 [details] [review]
Bug 19482: DO NOT PUSH - Schema changes
Comment 50 Katrin Fischer 2020-08-22 16:43:44 UTC
Hi Nick,

the QA script found some problems, please fix:

AIL	installer/data/mysql/kohastructure.sql
   OK	  boolean_vs_tinyint
   OK	  charset_collate
   OK	  git manipulation
   FAIL	  tinyint_has_boolean_flag
		The new column search_field.mandatory is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/SearchField.pm), see the SQL12 coding guideline

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/admin/searchengine/elasticsearch/mappings.tt
   FAIL	  filters
		missing_filter at line 193 (                        <input type="text" name="search_field_name" value="[% search_field.name | html %]" [% is_readonly %]/>)
		missing_filter at line 196 (                        <input type="text" name="search_field_label" value="[% search_field.label | html %]" [% is_readonly %]/>)
		missing_filter at line 383 (                                    <td><a class="btn btn-default btn-xs delete mandatory" data-field_name="[% mapping.search_field_name %]" style="cursor: pointer;"><i class="fa fa-trash"></i> Delete</a></td>)
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  js_in_body
   OK	  spelling
   FAIL	  tt_valid
		lines 193, 196
   OK	  valid_template


* Commit title does not start with 'Bug XXXXX: ' - df373aaf1f
* Commit title does not start with 'Bug XXXXX: ' - 7c2fb8cbb1
Comment 51 Jonathan Druart 2020-10-15 12:56:47 UTC
ping Nick, those are easy QA issues to fix!
Comment 52 Nick Clemens 2020-10-16 16:06:17 UTC
Created attachment 111923 [details] [review]
Bug 19482: DB changes

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

https://bugs.koha-community.org/show_bug.cgi?id=21357
Comment 53 Nick Clemens 2020-10-16 16:06:23 UTC
Created attachment 111924 [details] [review]
Bug 19482: Add support for defining 'mandatory' mappings

To test:
1 - Apply patch
2 - ./installer/data/mysql/updatedatabase.pl
3 - Reset ES mapping: Administration->Search engine configuration , button at bottom of page
4 - 'issues' and 'title' mapping under 'search fields' should be mandatory and not editable
5 - On 'Bibliographic records' tab you should not be able to delete the single entry for issues
6 - You should be able to delete 'title' mappings, however, at the final one you should be stopped by javascript
7 - Bonus: force remove the last mapping from the page using developer tools - attempt to save and should be warned of missing mandatory mapping

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

https://bugs.koha-community.org/show_bug.cgi?id=21357
Comment 54 Nick Clemens 2020-10-16 16:06:31 UTC
Created attachment 111925 [details] [review]
Bug 19482: (follow-up) Fix select and adjust export of new field
Comment 55 Nick Clemens 2020-10-16 16:06:44 UTC
Created attachment 111926 [details] [review]
Bug 19482: (follow-up) Add missing filters and boolean switch to schema
Comment 56 Nick Clemens 2020-10-16 16:06:53 UTC
Created attachment 111927 [details] [review]
Bug 19482: DO NOT PUSH: SCHEMA UPDATES
Comment 57 Nick Clemens 2020-10-16 16:09:19 UTC
(In reply to Katrin Fischer from comment #50)
> Hi Nick,
> 
> the QA script found some problems, please fix:

Done

>    FAIL	  tt_valid
> 		lines 193, 196

This is a false positive - it doesn't like my unquoted TT variable in an html tag, but that's how readonly works
Comment 58 Nick Clemens 2020-10-16 16:14:32 UTC
Created attachment 111929 [details] [review]
Bug 19482: (follow-up) Don't set readonly as a variable, use conditionals
Comment 59 Nick Clemens 2020-10-16 16:15:09 UTC
(In reply to Nick Clemens from comment #57)
> 
> This is a false positive - it doesn't like my unquoted TT variable in an
> html tag, but that's how readonly works

I changed code and now is removed
Comment 60 Katrin Fischer 2020-10-22 22:44:48 UTC
Hi Nick,

I think there are some small things:

1) We are missing unit tests for the changes to SearchField.pm and Elasticsearch.pm

2) I believe the </tr> removed needs to be added back after the ELSE:

                       <td>
+                        [% IF search_field.mandatory %]
+                            <input type="text" name="search_field_label" value="[% search_field.label | html %]" readonly />
+                        [% ELSE %]
                             <input type="text" name="search_field_label" value="[% search_field.label | html %]" />
-                      </td>
+                        [% END %]
+
                       <td>

3) Database update

+    if( !column_exists( 'search_field', 'mandatory' ) ) {
+        $dbh->do( "ALTER TABLE search_field ADD COLUMN mandatory tinyint(1) NULL DEFAULT NULL" );
+    }

Please add AFTER opac to make sure we always end up with the same sequence of columsn :)

Functionality wise this appears to work ok. 

I think we'd need to work out which other fields we deem mandatory as a next step?
Comment 61 Nick Clemens 2020-10-25 02:19:44 UTC
Created attachment 112456 [details] [review]
Bug 19482: (follow-up) Fix update, add tests, restore closing </td>

To test:
1 - View the mappings page, confirm table is correctly structured
2 - prove -v t/db_dependent/Koha/SearchEngine/Elasticsearch/ExportConfig.t t/db_dependent/Koha/SearchEngine/Elasticsearch/Reset.t t/db_dependent/Koha/SearchField.t
3 - Confirm update statement works and table is ordered correctly
Comment 62 Katrin Fischer 2020-10-25 02:28:45 UTC
Created attachment 112459 [details] [review]
Bug 19482: DB changes

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 63 Katrin Fischer 2020-10-25 02:28:52 UTC
Created attachment 112460 [details] [review]
Bug 19482: Add support for defining 'mandatory' mappings

To test:
1 - Apply patch
2 - ./installer/data/mysql/updatedatabase.pl
3 - Reset ES mapping: Administration->Search engine configuration , button at bottom of page
4 - 'issues' and 'title' mapping under 'search fields' should be mandatory and not editable
5 - On 'Bibliographic records' tab you should not be able to delete the single entry for issues
6 - You should be able to delete 'title' mappings, however, at the final one you should be stopped by javascript
7 - Bonus: force remove the last mapping from the page using developer tools - attempt to save and should be warned of missing mandatory mapping

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 64 Katrin Fischer 2020-10-25 02:28:57 UTC
Created attachment 112461 [details] [review]
Bug 19482: (follow-up) Fix select and adjust export of new field

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 65 Katrin Fischer 2020-10-25 02:29:02 UTC
Created attachment 112462 [details] [review]
Bug 19482: (follow-up) Add missing filters and boolean switch to schema

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 66 Katrin Fischer 2020-10-25 02:29:10 UTC
Created attachment 112463 [details] [review]
Bug 19482: DO NOT PUSH: SCHEMA UPDATES

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 67 Katrin Fischer 2020-10-25 02:29:17 UTC
Created attachment 112464 [details] [review]
Bug 19482: (follow-up) Don't set readonly as a variable, use conditionals

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 68 Katrin Fischer 2020-10-25 02:29:24 UTC
Created attachment 112465 [details] [review]
Bug 19482: (follow-up) Fix update, add tests, restore closing </td>

To test:
1 - View the mappings page, confirm table is correctly structured
2 - prove -v t/db_dependent/Koha/SearchEngine/Elasticsearch/ExportConfig.t t/db_dependent/Koha/SearchEngine/Elasticsearch/Reset.t t/db_dependent/Koha/SearchField.t
3 - Confirm update statement works and table is ordered correctly

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 69 Jonathan Druart 2020-11-03 15:14:06 UTC
I am a bit reluctant regarding the 'if ($@ || @messages)' test, some messages could have been pushed outside of the eval.
But letting it as it for now.
Comment 70 Jonathan Druart 2020-11-03 15:14:45 UTC
This is what I had in mind.

@ admin/searchengine/elasticsearch/mappings.pl:119 @ if ( $op eq 'edit' ) {
             }
             elsif ($field_weight <= 0 || !looks_like_number($field_weight)) {
                 push @messages, { type => 'error', code => 'invalid_field_weight', 'weight' => $field_weight };
+                $errors++;
             }
             else {
                 $search_field->weight($field_weight);
@ admin/searchengine/elasticsearch/mappings.pl:165 @ if ( $op eq 'edit' ) {
                 search => $mapping_search
             });
         }
-        push @messages, { type => 'error', code => 'missing_mandatory_fields' } if $mandatory_after < $mandatory_before;
+        if ( $mandatory_after < $mandatory_before ) {
+            push @messages, { type => 'error', code => 'missing_mandatory_fields' };
+            $error++;
+        }
     };
-    if ($@ || @messages) {
+    if ($@ || $errors) {
         push @messages, { type => 'error', code => 'error_on_update', message => $@, };
         $schema->storage->txn_rollback;
     } else {

Will see later if we need it.
Comment 71 Jonathan Druart 2020-11-04 12:02:34 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 72 Lucas Gass 2020-11-16 21:12:08 UTC
enhancement will not be backported to 20.05.x