Bug 19482 - Elasticsearch - prevent removal / editing of required indexes
Summary: Elasticsearch - prevent removal / editing of required indexes
Status: Signed Off
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:
Whiteboard:
Keywords:
Depends on: 19481
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-18 10:52 UTC by Nick Clemens
Modified: 2018-10-18 14:38 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


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

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.