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
Created attachment 68246 [details] [review] Bug 19482 - DB changes
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
Created attachment 68248 [details] [review] Bug 19482 - Schema changes (DO NOT PUSH)
The 2nd patch does not apply on current master. (Lacking or useless SHA1)
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.
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
(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
Created attachment 72846 [details] [review] Bug 19482 - DB changes
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
Created attachment 72848 [details] [review] Bug 19482 - Schema changes (DO NOT PUSH)
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
Created attachment 72878 [details] [review] Bug 19482 - DB changes Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
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>
Created attachment 72880 [details] [review] Bug 19482 - Schema changes (DO NOT PUSH)
I have slightly changed the test plan (added updatedatabase, and also correct table names with the DELETE part). Hope it's OK !
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
Created attachment 77007 [details] [review] Bug 19482 - DB changes Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
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>
Created attachment 77009 [details] [review] Bug 19482: (follow-up) QA fixes Remove debug statement Use disaled select to match other fields
Created attachment 77010 [details] [review] Bug 19482: DO NOT PUSH Schema changes
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
Created attachment 80817 [details] [review] Bug 19482 - DB changes Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
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>
Created attachment 80819 [details] [review] Bug 19482: (follow-up) QA fixes Remove debug statement Use disaled select to match other fields
Created attachment 80820 [details] [review] Bug 19482: DO NOT PUSH Schema changes
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.
Created attachment 93366 [details] [review] Bug 19482 - DB changes Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
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
Created attachment 93368 [details] [review] Bug 19482: SCHEMA CHANGES - DO NOT PUSH
Created attachment 93371 [details] [review] Bug 19482 - DB changes Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
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>
Created attachment 93373 [details] [review] Bug 19482: SCHEMA CHANGES - DO NOT PUSH Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
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.
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
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
Created attachment 97226 [details] [review] Bug 19482 - DB changes Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
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>
Created attachment 97228 [details] [review] Bug 19482: SCHEMA CHANGES _ DO NOT PUSH
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
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
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
Changes to mappings.yaml will need to be rebuilt on top of bug 23204 (sorry!)
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.
As Jonathan said, Koha::SearchEngine::Elasticsearch::raw_elasticsearch_mappings need to be updated with the new mandatory field.
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
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
Created attachment 108469 [details] [review] Bug 19482: (follow-up) Fix select and adjust export of new field
Created attachment 108470 [details] [review] Bug 19482: (optional) Export mappings and update shipped file
Created attachment 108471 [details] [review] Bug 19482: DO NOT PUSH - Schema changes
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
ping Nick, those are easy QA issues to fix!
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
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
Created attachment 111925 [details] [review] Bug 19482: (follow-up) Fix select and adjust export of new field
Created attachment 111926 [details] [review] Bug 19482: (follow-up) Add missing filters and boolean switch to schema
Created attachment 111927 [details] [review] Bug 19482: DO NOT PUSH: SCHEMA UPDATES
(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
Created attachment 111929 [details] [review] Bug 19482: (follow-up) Don't set readonly as a variable, use conditionals
(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
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?
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
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>
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>
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>
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>
Created attachment 112463 [details] [review] Bug 19482: DO NOT PUSH: SCHEMA UPDATES Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
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>
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>
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.
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.
Pushed to master for 20.11, thanks to everybody involved!
enhancement will not be backported to 20.05.x