Bug 19482

Summary: Elasticsearch - prevent removal / editing of required indexes
Product: Koha Reporter: Nick Clemens <nick>
Component: Searching - ElasticsearchAssignee: Nick Clemens <nick>
Status: Signed Off --- QA Contact: Alex Arnaud <alex.arnaud>
Severity: enhancement    
Priority: P5 - low CC: alex.arnaud, bouzid.fergani, david.bourgault, e.betemps, jonathan.druart, marjorie.barry-vila, nicolas.legrand, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Who signed the patch off: Text to go in the release notes:
Version(s) released in:
Bug Depends on: 19481    
Bug Blocks:    
Attachments: Bug 19482 - DB changes
Bug 19482 - Add support for defining 'mandatory' mappings
Bug 19482 - Schema changes (DO NOT PUSH)
Bug 19482 - Make reset mappings reset also reset fields
Bug 19482 - DB changes
Bug 19482 - Add support for defining 'mandatory' mappings
Bug 19482 - Schema changes (DO NOT PUSH)
Bug 19482 - DB changes
Bug 19482 - Add support for defining 'mandatory' mappings
Bug 19482 - Schema changes (DO NOT PUSH)
Bug 19482 - DB changes
Bug 19482 - Add support for defining 'mandatory' mappings
Bug 19482: (follow-up) QA fixes
Bug 19482: DO NOT PUSH Schema changes
Bug 19482 - DB changes
Bug 19482 - Add support for defining 'mandatory' mappings
Bug 19482: (follow-up) QA fixes
Bug 19482: DO NOT PUSH Schema changes
Bug 19482 - DB changes
Bug 19482 - Add support for defining 'mandatory' mappings
Bug 19482: SCHEMA CHANGES - DO NOT PUSH
Bug 19482 - DB changes
Bug 19482 - Add support for defining 'mandatory' mappings
Bug 19482: SCHEMA CHANGES - DO NOT PUSH

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