Bug 20388 - Elasticsearch - Ability to add search fields from UI
Summary: Elasticsearch - Ability to add search fields from UI
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Alex Arnaud
QA Contact:
URL:
Whiteboard:
Keywords:
: 18194 (view as bug list)
Depends on: 18316
Blocks: 20588
  Show dependency treegraph
 
Reported: 2018-03-13 14:12 UTC by Alex Arnaud
Modified: 2019-06-11 19:27 UTC (History)
13 users (show)

See Also:
Change sponsored?: 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:
Version(s) released in:


Attachments
Bug 20388 - Add/delete search fields from search engine configuration page (6.54 KB, patch)
2018-03-22 15:38 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388 - Add/delete search fields from search engine configuration page (6.60 KB, patch)
2018-03-23 09:32 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 20388 - Add/delete search fields from search engine configuration page (6.74 KB, patch)
2018-03-23 10:01 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 20388 - Add/delete search fields from search engine configuration page (6.80 KB, patch)
2018-03-30 12:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20388 - Add/delete search fields from search engine configuration page (6.42 KB, patch)
2018-04-04 13:10 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388 - Keep data consistency when adding/removing mapping (2.06 KB, patch)
2018-04-04 13:10 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388 - Keep data consistency when adding/removing mapping (2.28 KB, patch)
2018-04-04 13:48 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388 - Add/delete search fields from search engine configuration page (6.42 KB, patch)
2018-04-05 12:54 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 20388 - Keep data consistency when adding/removing mapping (2.33 KB, patch)
2018-04-05 12:54 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 20388 - Add/delete search fields from search engine configuration page (8.52 KB, patch)
2018-04-10 09:51 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388 - Keep data consistency when adding/removing mapping (2.33 KB, patch)
2018-04-10 09:51 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 18316 - Add weighting/relevancy options to ES query on simple search (6.98 KB, patch)
2018-04-10 14:36 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388 - Add/delete search fields from search engine configuration page (8.60 KB, patch)
2018-04-10 14:56 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388 - Keep data consistency when adding/removing mapping (2.33 KB, patch)
2018-04-10 14:56 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388 - Add/delete search fields from search engine configuration page (8.60 KB, patch)
2018-04-10 15:01 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388 - Keep data consistency when adding/removing mapping (2.33 KB, patch)
2018-04-10 15:01 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 11674: DBRev 17.12.00.031 (2.93 KB, patch)
2018-04-16 19:36 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 20388 - Add/delete search fields from search engine configuration page (8.56 KB, patch)
2018-04-23 12:45 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388 - Keep data consistency when adding/removing mapping (2.33 KB, patch)
2018-04-23 12:45 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388 - Add/delete search fields from search engine configuration page (8.42 KB, patch)
2018-09-14 13:43 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388 - Keep data consistency when adding/removing mapping (2.33 KB, patch)
2018-09-14 13:43 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388: Prevent user from using a search field for a mapping after removing it (1.84 KB, patch)
2018-10-17 13:52 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388: Use live handler for deleting elements (2.42 KB, patch)
2019-03-22 16:11 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388 - Add/delete search fields from search engine configuration page (8.53 KB, patch)
2019-03-29 12:57 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388 - Keep data consistency when adding/removing mapping (2.33 KB, patch)
2019-03-29 12:58 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388: Prevent user from using a search field for a mapping after removing it (1.84 KB, patch)
2019-03-29 12:58 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388: Use live handler for deleting elements (2.42 KB, patch)
2019-03-29 12:58 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388 - Add/delete search fields from search engine configuration page (8.58 KB, patch)
2019-03-29 13:23 UTC, Björn Nylén
Details | Diff | Splinter Review
Bug 20388 - Keep data consistency when adding/removing mapping (2.47 KB, patch)
2019-03-29 13:23 UTC, Björn Nylén
Details | Diff | Splinter Review
Bug 20388: Prevent user from using a search field for a mapping after removing it (1.98 KB, patch)
2019-03-29 13:23 UTC, Björn Nylén
Details | Diff | Splinter Review
Bug 20388: Use live handler for deleting elements (2.56 KB, patch)
2019-03-29 13:23 UTC, Björn Nylén
Details | Diff | Splinter Review
Bug 20388: QA fixes: - pass search field objects in template instead of unblessed ones - disable drag n drop (4.17 KB, patch)
2019-04-16 14:38 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20388: Add/delete search fields from search engine configuration page (8.64 KB, patch)
2019-04-16 15:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20388: Keep data consistency when adding/removing mapping (2.53 KB, patch)
2019-04-16 15:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20388: Prevent user from using a search field for a mapping after removing it (2.03 KB, patch)
2019-04-16 15:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20388: Use live handler for deleting elements (2.62 KB, patch)
2019-04-16 15:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20388: QA fixes: - pass search field objects in template instead of unblessed ones - disable drag n drop (4.22 KB, patch)
2019-04-16 15:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20388: QA fixe : delete line who add elements in biblios and authorities in the same time (4.50 KB, patch)
2019-04-30 14:19 UTC, axel Amghar
Details | Diff | Splinter Review
Bug 20388: QA fixe : delete line who add elements in biblios and authorities in the same time (4.50 KB, patch)
2019-04-30 14:20 UTC, axel Amghar
Details | Diff | Splinter Review
Bug 20388: QA fixe : delete line who add elements in biblios and authorities in the same time (1.75 KB, patch)
2019-04-30 14:52 UTC, axel Amghar
Details | Diff | Splinter Review
Bug 20388: add of types in the new entries dropdown of searchfiels (1.24 KB, patch)
2019-05-14 11:51 UTC, axel Amghar
Details | Diff | Splinter Review
Bug 20388: QA fixe : add searchfield only if it doesn't exist (2.24 KB, patch)
2019-05-17 12:32 UTC, axel Amghar
Details | Diff | Splinter Review
Bug 20388: Add/delete search fields from search engine configuration page (8.71 KB, patch)
2019-06-11 19:26 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20388: Keep data consistency when adding/removing mapping (2.58 KB, patch)
2019-06-11 19:26 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20388: Prevent user from using a search field for a mapping after removing it (2.09 KB, patch)
2019-06-11 19:26 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20388: Use live handler for deleting elements (2.67 KB, patch)
2019-06-11 19:27 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20388: QA fixes: - pass search field objects in template instead of unblessed ones - disable drag n drop (4.28 KB, patch)
2019-06-11 19:27 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20388: QA fixe : delete line who add elements in biblios and authorities in the same time (1.80 KB, patch)
2019-06-11 19:27 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20388: add of types in the new entries dropdown of searchfiels (1.29 KB, patch)
2019-06-11 19:27 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20388: QA fixe : add searchfield only if it doesn't exist (2.29 KB, patch)
2019-06-11 19:27 UTC, Michal Denar
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Arnaud 2018-03-13 14:12:52 UTC
Currently indexes only coming from mapping.yaml. We need to add some using UI (admin/searchengine/elasticsearch/mappings.pl)
Comment 1 claire.hernandez@biblibre.com 2018-03-16 14:13:50 UTC
* The add line could be at the begin of the page.
* A delete button could be on each line.
Comment 2 Alex Arnaud 2018-03-22 15:38:39 UTC Comment hidden (obsolete)
Comment 3 Nicolas Legrand 2018-03-23 09:32:25 UTC Comment hidden (obsolete)
Comment 4 Séverine Queune 2018-03-23 10:01:30 UTC Comment hidden (obsolete)
Comment 5 Séverine Queune 2018-03-23 10:04:11 UTC
This new feature will be very useful !
It also makes the "Search fields" tab clearer, for we now can see which index are unused.
Thanks Alex !
Comment 6 Alex Arnaud 2018-03-23 10:13:21 UTC
*** Bug 18194 has been marked as a duplicate of this bug. ***
Comment 7 Nick Clemens 2018-03-30 12:45:01 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2018-04-02 20:42:15 UTC
1. What's the point of the JS function valid_line?
I think you should double-check it, it does not do what you expect it to do.

2. You are reusing code to duplicate a new mapping but for something else. You should rename the variables to avoid confusion then.
For instance "search_fields" is not an index name

3. Bug report's title is wrong, we are not adding/removing indices

4. I did not implement that part on the original patch because it's hard to keep an interface consistent.
For instance:
- create a new search field
=> You cannot create a new mapping, you have to save first
- create a new search field, save
=> You can delete it. So it you create a mapping using it, you can still delete it. If you save => kaboom
=> If you have a search field already mapped and delete the mapping, you cannot delete the search field.
Comment 9 Alex Arnaud 2018-04-04 09:35:01 UTC
(In reply to Jonathan Druart from comment #8)
> 1. What's the point of the JS function valid_line?
> I think you should double-check it, it does not do what you expect it to do.
> 
> 2. You are reusing code to duplicate a new mapping but for something else.
> You should rename the variables to avoid confusion then.
> For instance "search_fields" is not an index name
I wanted to reuse the code as much as possible. It is not a problem for to add a js fonction dedicated to duplicate search fields only.
> 
> 3. Bug report's title is wrong, we are not adding/removing indices
Right!
> 
> 4. I did not implement that part on the original patch because it's hard to
> keep an interface consistent.
> For instance:
> - create a new search field
> => You cannot create a new mapping, you have to save first
> - create a new search field, save
> => You can delete it. So it you create a mapping using it, you can still
> delete it. If you save => kaboom
> => If you have a search field already mapped and delete the mapping, you
> cannot delete the search field.
I see 2 solutions:
  1) JS code for validating data when saving and warn the user if there is something inconsistent. May be quite tricky...
  2) Submit the form on clicking the "+add" button. There is a disavantage: page is reloaded between each adding. Not confortable for adding many search fields or mapping
Comment 10 Alex Arnaud 2018-04-04 13:10:08 UTC Comment hidden (obsolete)
Comment 11 Alex Arnaud 2018-04-04 13:10:18 UTC Comment hidden (obsolete)
Comment 12 Alex Arnaud 2018-04-04 13:48:37 UTC Comment hidden (obsolete)
Comment 13 Nicolas Legrand 2018-04-05 12:54:17 UTC Comment hidden (obsolete)
Comment 14 Nicolas Legrand 2018-04-05 12:54:23 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2018-04-05 15:13:13 UTC Comment hidden (obsolete)
Comment 16 Alex Arnaud 2018-04-06 06:48:26 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2018-04-06 14:37:27 UTC Comment hidden (obsolete)
Comment 18 Alex Arnaud 2018-04-10 09:51:29 UTC
Created attachment 73918 [details] [review]
Bug 20388 - Add/delete search fields from search engine configuration page

Test plan:

  - apply this patch,
  - check that SearchEngine system preference is set to Elasticsearch,
  - go to Admin > Search engine configuration,
  - on the search fields tab, fill a new line at the bottom of the table
    (name, label, type)
  - click on the "Add" button and save,
  - check that the new search field has been saved,
  - also test field deletions,
  - check that you can't delete already mapped fields.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Alex Arnaud 2018-04-10 09:51:36 UTC
Created attachment 73919 [details] [review]
Bug 20388 - Keep data consistency when adding/removing mapping

Test plan:
  - create a new search field, save,
  - create a mapping using it (don't save),
  - check that you can't delete the search field.

  - delete all the mapping related to a search field (biblios and
    authorities), don't save,
  - check that you can delete this search field.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 20 Alex Arnaud 2018-04-10 09:54:14 UTC
Made this BZ depends on 18316 because i'm reusing a subroutine created in.
So put this one back on needs signoff to be sure nothing is broken
Comment 21 Alex Arnaud 2018-04-10 14:36:14 UTC Comment hidden (obsolete)
Comment 22 Alex Arnaud 2018-04-10 14:56:07 UTC Comment hidden (obsolete)
Comment 23 Alex Arnaud 2018-04-10 14:56:24 UTC Comment hidden (obsolete)
Comment 24 Alex Arnaud 2018-04-10 15:01:38 UTC Comment hidden (obsolete)
Comment 25 Alex Arnaud 2018-04-10 15:01:59 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2018-04-13 15:46:15 UTC
I tested this one (when reviewing bug 18316).
We can have inconsistency when using the UI:
- Add acqdate to authority
- Add weight to acqdate for biblio
- Save
- Remove it for biblio
- Save

=> weight is still set in DB but not displayed on the UI

It's not that important, because not used. But if we continue that way we are going to have a lot of inconsistencies.

I built this interface (bug 14899, 2.5 years ago) as an example. Now we are going to add lot of information and maybe we should rethink the interface.

I can easily imagine somebody spending 2 hours switching from 1 tab to another and modifying everything, then finally save, and... everything is lost because an error occurred.

It seems obvious that the search fields form should be a separate page to force a save before modifying the mappings.
Maybe better would be to save via AJAX when the tab is changed (or force the user to save), and refresh the new tab to have updated info.
The "add" and "delete" buttons could also send AJAX requests.

I do not think it will be blocker for 18.05, but keep this comment in mind if you plan to add more and more stuffs to this view.
Comment 27 Jonathan Druart 2018-04-13 15:48:51 UTC
It would also has been fair to base your patches on top of the other ones in the queue (since 1 year), bug 18235 for instance
Comment 28 Séverine Queune 2018-04-16 19:36:58 UTC Comment hidden (obsolete)
Comment 29 Séverine Queune 2018-04-17 11:19:27 UTC
Something weird happened when I signed it.. Sorry about that.
Behaviour is ok for me.
Comment 30 Alex Arnaud 2018-04-23 12:45:05 UTC Comment hidden (obsolete)
Comment 31 Alex Arnaud 2018-04-23 12:45:12 UTC Comment hidden (obsolete)
Comment 32 Joonas Kylmälä 2018-08-16 06:57:27 UTC
I was trying to test these patches but they don't apply on top of latest master.
Comment 33 Alex Arnaud 2018-09-14 13:43:07 UTC Comment hidden (obsolete)
Comment 34 Alex Arnaud 2018-09-14 13:43:19 UTC Comment hidden (obsolete)
Comment 35 Julian Maurice 2018-09-27 10:19:21 UTC
There is a little problem that needs to be fixed:

1. Add a search field, save
2. Delete the search field, don't save
3. Go to "Bibliographic records" tab, add a new mapping for this search field, Save.
4. An error message appear:
> An error occurred when updating mappings (Can't call method
> "add_to_search_marc_maps" on an undefined value at
> /home/koha/env/master/src/admin/searchengine/elasticsearch/mappings.pl line
> 107. ).
Comment 36 Alex Arnaud 2018-10-17 13:52:31 UTC
Created attachment 80709 [details] [review]
Bug 20388: Prevent user from using a search field for a mapping after removing it

Test plan:

 - add a search field,
 - save,
 - remove this search field
 - check that it has been removed from the mapping lists (bottom
   select).
Comment 37 Josef Moravec 2018-12-05 16:16:44 UTC
(In reply to Alex Arnaud from comment #34)
> Created attachment 78659 [details] [review] [review]
> Bug 20388 - Keep data consistency when adding/removing mapping
> 
> Test plan:
>   - create a new search field, save,
>   - create a mapping using it (don't save),
>   - check that you can't delete the search field.
> 
>   - delete all the mapping related to a search field (biblios and
>     authorities), don't save,
>   - check that you can delete this search field.
> 

This does not work, first part is OK, but in second one, I still can't remove the searchfield...
Comment 38 Alex Arnaud 2019-03-21 16:24:58 UTC
(In reply to Josef Moravec from comment #37)
> (In reply to Alex Arnaud from comment #34)
> > Created attachment 78659 [details] [review] [review] [review]
> > Bug 20388 - Keep data consistency when adding/removing mapping
> > 
> > Test plan:
> >   - create a new search field, save,
> >   - create a mapping using it (don't save),
> >   - check that you can't delete the search field.
> > 
> >   - delete all the mapping related to a search field (biblios and
> >     authorities), don't save,
> >   - check that you can delete this search field.
> > 
> 
> This does not work, first part is OK, but in second one, I still can't
> remove the searchfield...

Hello,

You mean you deleted all mapping related to a search field and you can delete it ?
Comment 39 Alex Arnaud 2019-03-21 16:34:52 UTC
(In reply to Alex Arnaud from comment #38)
> (In reply to Josef Moravec from comment #37)
> > (In reply to Alex Arnaud from comment #34)
> > > Created attachment 78659 [details] [review] [review] [review] [review]
> > > Bug 20388 - Keep data consistency when adding/removing mapping
> > > 
> > > Test plan:
> > >   - create a new search field, save,
> > >   - create a mapping using it (don't save),
> > >   - check that you can't delete the search field.
> > > 
> > >   - delete all the mapping related to a search field (biblios and
> > >     authorities), don't save,
> > >   - check that you can delete this search field.
> > > 
> > 
> > This does not work, first part is OK, but in second one, I still can't
> > remove the searchfield...
> 
> Hello,
> 
> You mean you deleted all mapping related to a search field and you can
> delete it ?

Works for me...
Comment 40 Björn Nylén 2019-03-22 07:29:14 UTC
Hi!

We also tested the patch and can't remove the search field according to your instructions. If I save the mappings first it works ok. Tested in FF and Chrome

After a quick look it seems the newly created mappings is missing the onclick event to enable the delete button on the search field-tab.
Comment 41 Alex Arnaud 2019-03-22 16:11:50 UTC Comment hidden (obsolete)
Comment 42 Björn Nylén 2019-03-29 12:25:16 UTC
The patch doesnt apply for me on the current master.

Applying: Bug 20388 - Add/delete search fields from search engine configuration page
fatal: sha1 information is lacking or useless (admin/searchengine/elasticsearch/mappings.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 20388 - Add/delete search fields from search engine configuration page
Comment 43 Alex Arnaud 2019-03-29 12:57:53 UTC
Created attachment 87196 [details] [review]
Bug 20388 - Add/delete search fields from search engine configuration page

Test plan:

  - apply this patch,
  - check that SearchEngine system preference is set to Elasticsearch,
  - go to Admin > Search engine configuration,
  - on the search fields tab, fill a new line at the bottom of the table
    (name, label, type)
  - click on the "Add" button and save,
  - check that the new search field has been saved,
  - also test field deletions,
  - check that you can't delete already mapped fields.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 44 Alex Arnaud 2019-03-29 12:58:15 UTC
Created attachment 87197 [details] [review]
Bug 20388 - Keep data consistency when adding/removing mapping

Test plan:
  - create a new search field, save,
  - create a mapping using it (don't save),
  - check that you can't delete the search field.

  - delete all the mapping related to a search field (biblios and
    authorities), don't save,
  - check that you can delete this search field.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 45 Alex Arnaud 2019-03-29 12:58:28 UTC
Created attachment 87198 [details] [review]
Bug 20388: Prevent user from using a search field for a mapping after removing it

Test plan:

 - add a search field,
 - save,
 - remove this search field
 - check that it has been removed from the mapping lists (bottom
   select).
Comment 46 Alex Arnaud 2019-03-29 12:58:42 UTC
Created attachment 87199 [details] [review]
Bug 20388: Use live handler for deleting elements
Comment 47 Björn Nylén 2019-03-29 13:23:40 UTC
Created attachment 87201 [details] [review]
Bug 20388 - Add/delete search fields from search engine configuration page

Test plan:

  - apply this patch,
  - check that SearchEngine system preference is set to Elasticsearch,
  - go to Admin > Search engine configuration,
  - on the search fields tab, fill a new line at the bottom of the table
    (name, label, type)
  - click on the "Add" button and save,
  - check that the new search field has been saved,
  - also test field deletions,
  - check that you can't delete already mapped fields.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Comment 48 Björn Nylén 2019-03-29 13:23:45 UTC
Created attachment 87202 [details] [review]
Bug 20388 - Keep data consistency when adding/removing mapping

Test plan:
  - create a new search field, save,
  - create a mapping using it (don't save),
  - check that you can't delete the search field.

  - delete all the mapping related to a search field (biblios and
    authorities), don't save,
  - check that you can delete this search field.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Comment 49 Björn Nylén 2019-03-29 13:23:49 UTC
Created attachment 87203 [details] [review]
Bug 20388: Prevent user from using a search field for a mapping after removing it

Test plan:

 - add a search field,
 - save,
 - remove this search field
 - check that it has been removed from the mapping lists (bottom
   select).

Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Comment 50 Björn Nylén 2019-03-29 13:23:54 UTC
Created attachment 87204 [details] [review]
Bug 20388: Use live handler for deleting elements

Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Comment 51 Julian Maurice 2019-04-15 11:50:19 UTC
Comment on attachment 87201 [details] [review]
Bug 20388 - Add/delete search fields from search engine configuration page

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

::: admin/searchengine/elasticsearch/mappings.pl
@@ +217,5 @@
>      my $search_field_unblessed = $search_field->unblessed;
>      $search_field_unblessed->{mapped_biblios} = 1 if $search_field->is_mapped_biblios;
> +
> +    $search_field_unblessed->{is_mapped} = $schema->resultset('SearchMarcToField')->search(
> +        { search_field_id => $search_field->id })->count;

Can be replaced by

    $search_field_unblessed->{is_mapped} = $search_field->is_mapped;

But why is it unblessed in the first place, and not directly passed to template ?

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/searchengine/elasticsearch/mappings.tt
@@ +64,5 @@
> +            var name = $(line).find('input[data-id="search_field_name"]').val();
> +            var label = $(line).find('input[data-id="search_field_name"]').val();
> +            if ( name.length > 0 && label.length > 0 ) {
> +                var new_line = clone_line( line );
> +                new_line.appendTo($('table[data-index_name=search_fields]>tbody'));

You should probably use the already existing 'table' variable instead of searching in the whole DOM

@@ +72,5 @@
> +                clean_line(line);
> +
> +                $(table).tableDnD( {
> +                    onDragClass: "dragClass",
> +                } );

This is weird. Drag and Drop is not enabled on this table until you add a new field.
And it looks like it is useless to move lines around since they are automatically ordered alphabetically after a save

@@ +242,5 @@
> +                        [% IF search_field.is_mapped %]
> +                          <a class="btn btn-default btn-xs disabled delete" style="cursor: pointer;"><i class="fa fa-trash"></i> Delete</a>
> +                        [% ELSE %]
> +                          <a class="btn btn-default btn-xs delete" style="cursor: pointer;"><i class="fa fa-trash"></i> Delete</a>
> +                        [% END %]

It might be better to use a <button> here to avoid the inline style, and to use the disabled attribute instead of the disabled class

@@ +244,5 @@
> +                        [% ELSE %]
> +                          <a class="btn btn-default btn-xs delete" style="cursor: pointer;"><i class="fa fa-trash"></i> Delete</a>
> +                        [% END %]
> +                      </td>
> +                      </td>

Double end tag, should be removed
Comment 52 Julian Maurice 2019-04-15 11:58:47 UTC
Comment on attachment 87203 [details] [review]
Bug 20388: Prevent user from using a search field for a mapping after removing it

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/searchengine/elasticsearch/mappings.tt
@@ +33,4 @@
>              var line = $(this).closest("tr");
> +
> +            // We clicked delete button on search fields tab.
> +            if (name = $(line).find('input[name="search_field_name"]').val()) {

It looks like 'name' is a global variable. Declare it with 'var' to avoid that.
Comment 53 Julian Maurice 2019-04-15 13:09:18 UTC
Additional QA comments:

- I noticed a strange behaviour. After resetting all mappings ("Reset mappings" button), all "Delete" buttons are disabled until I click on "Save". What is happening here ?
- If I add a new field with the same name of an existing field, and move the line to the top, the existing field will be deleted and the new field will remain. It could be a problem if the types differ. There should be a control to avoid that.
Comment 54 Alex Arnaud 2019-04-16 14:38:09 UTC
Created attachment 88075 [details] [review]
Bug 20388: QA fixes: - pass search field objects in template instead of unblessed ones - disable drag n drop
Comment 55 Julian Maurice 2019-04-16 15:10:26 UTC
Created attachment 88079 [details] [review]
Bug 20388: Add/delete search fields from search engine configuration page

Test plan:

  - apply this patch,
  - check that SearchEngine system preference is set to Elasticsearch,
  - go to Admin > Search engine configuration,
  - on the search fields tab, fill a new line at the bottom of the table
    (name, label, type)
  - click on the "Add" button and save,
  - check that the new search field has been saved,
  - also test field deletions,
  - check that you can't delete already mapped fields.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 56 Julian Maurice 2019-04-16 15:10:37 UTC
Created attachment 88080 [details] [review]
Bug 20388: Keep data consistency when adding/removing mapping

Test plan:
  - create a new search field, save,
  - create a mapping using it (don't save),
  - check that you can't delete the search field.

  - delete all the mapping related to a search field (biblios and
    authorities), don't save,
  - check that you can delete this search field.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 57 Julian Maurice 2019-04-16 15:10:45 UTC
Created attachment 88081 [details] [review]
Bug 20388: Prevent user from using a search field for a mapping after removing it

Test plan:

 - add a search field,
 - save,
 - remove this search field
 - check that it has been removed from the mapping lists (bottom
   select).

Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 58 Julian Maurice 2019-04-16 15:10:53 UTC
Created attachment 88082 [details] [review]
Bug 20388: Use live handler for deleting elements

Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 59 Julian Maurice 2019-04-16 15:10:59 UTC
Created attachment 88083 [details] [review]
Bug 20388: QA fixes: - pass search field objects in template instead of unblessed ones - disable drag n drop

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 60 Nick Clemens 2019-04-19 14:11:44 UTC
If i add a field and then add a mapping, then delete the mapping, I am unable to the delete the field

I poked the DB a bit - it looks like the numberings are getting mixed up somewhere/somehow

The ids for search_field are rebuilt on every save?
Comment 61 Martin Renvoize 2019-04-30 12:46:46 UTC
Are we still hoping to move this along for 19.05?
Comment 62 Alex Arnaud 2019-04-30 12:51:20 UTC
(In reply to Martin Renvoize from comment #61)
> Are we still hoping to move this along for 19.05?

We (Biblibre) will send some fixes very soon. Work in progress...
Comment 63 axel Amghar 2019-04-30 14:19:01 UTC Comment hidden (obsolete)
Comment 64 axel Amghar 2019-04-30 14:20:38 UTC Comment hidden (obsolete)
Comment 65 axel Amghar 2019-04-30 14:52:03 UTC
Created attachment 89121 [details] [review]
Bug 20388: QA fixe : delete line who add elements in biblios  and authorities in the same time
Comment 66 Liz Rea 2019-04-30 18:21:53 UTC
Hi,

I've just tried to test this, and I don't see the row at the bottom to add new mappings on the search tab?

Terribly sorry, I'm quite sure I restarted the things and did all of the prerequisites, but as usual not sure if it was something I'm doing wrong or if it's the code.

Cheers,
Liz
Comment 67 axel Amghar 2019-05-02 07:51:36 UTC
(In reply to Liz Rea from comment #66)
> Hi,
> 
> I've just tried to test this, and I don't see the row at the bottom to add
> new mappings on the search tab?
> 
> Terribly sorry, I'm quite sure I restarted the things and did all of the
> prerequisites, but as usual not sure if it was something I'm doing wrong or
> if it's the code.
> 
> Cheers,
> Liz

Hi, 

Are you sure you are at the good place? 
You can add search fields at the biottom of the search field part, but to add new mappings you have to go at the bottom of bibliographic records.
Comment 68 Alex Arnaud 2019-05-06 13:42:53 UTC
Anyone else can reproduce Liz's issue ?
Comment 69 Liz Rea 2019-05-07 19:34:12 UTC
there were no entry boxes at the bottom of either tab, sorry.
Comment 70 axel Amghar 2019-05-09 07:56:33 UTC
(In reply to Liz Rea from comment #69)
> there were no entry boxes at the bottom of either tab, sorry.

If you realy are in search engine configuration. 

Remove the patch, so you should be in master.
- return to search engine configuration 
- go to bibliographic records or authorities
- at the bottom of the tab, you should see the add line because this patch just add the line at the bottom of search field
- if you don't, there is a issue on your side
Comment 71 Jonathan Druart 2019-05-09 15:24:55 UTC
The new "add" line appears for me, but the "Type" dropdown does not contain the same values as above. Please double check.

In case of misunderstanding it's always good to share a screenshot, so here it is:
https://snag.gy/2XiGEd.jpg # Scroll down!
Comment 72 Jonathan Druart 2019-05-09 15:27:50 UTC
Also, if you add a field that already exists, it is ignored (no warning, no updated).
Comment 73 Liz Rea 2019-05-10 01:20:33 UTC
Re-checked this today, and the fields showed up. Quite sure I didn't do anything differently, sorry for the noise.

As Jonathan noted, the types for the new entries don't match with the types in the existing entries. Is that intentional?

When I tested adding a name and label that already existed, it added it for me - I don't know if that is correct or incorrect behaviour.
Comment 74 axel Amghar 2019-05-14 11:51:00 UTC
Created attachment 89733 [details] [review]
Bug 20388: add of types in the new entries dropdown of searchfiels
Comment 75 axel Amghar 2019-05-14 12:00:49 UTC Comment hidden (obsolete)
Comment 76 axel Amghar 2019-05-14 12:00:49 UTC
(In reply to Liz Rea from comment #73)
> Re-checked this today, and the fields showed up. Quite sure I didn't do
> anything differently, sorry for the noise.
> 
> As Jonathan noted, the types for the new entries don't match with the types
> in the existing entries. Is that intentional?
> 
> When I tested adding a name and label that already existed, it added it for
> me - I don't know if that is correct or incorrect behaviour.

Hi, 
I fix the types for the new entries ( I don't think it was intentional).
But for the adding, I didn't succeed to reproduce your bug.
How did you do it?
Comment 77 Jonathan Druart 2019-05-15 22:25:03 UTC
Like that, adding a new 'uri' field: https://snag.gy/BpxcIq.jpg
Comment 78 axel Amghar 2019-05-17 12:32:52 UTC
Created attachment 89894 [details] [review]
Bug 20388: QA fixe : add searchfield only if it doesn't exist

To test:

Without the fix:
- in search field configuration -> search field
- add the search field "uri" at the bottom of the tab
- you should have two fields "uri" like that : https://snag.gy/BpxcIq.jpg

Then apply the fix
- try to add the search field uri
- this time an alert should appear with the message :
" SearchField uri already exist "
Comment 79 Michal Denar 2019-06-11 19:26:44 UTC
Created attachment 90517 [details] [review]
Bug 20388: Add/delete search fields from search engine configuration page

Test plan:

  - apply this patch,
  - check that SearchEngine system preference is set to Elasticsearch,
  - go to Admin > Search engine configuration,
  - on the search fields tab, fill a new line at the bottom of the table
    (name, label, type)
  - click on the "Add" button and save,
  - check that the new search field has been saved,
  - also test field deletions,
  - check that you can't delete already mapped fields.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Michal Denar <black23@gmail.com>
Comment 80 Michal Denar 2019-06-11 19:26:51 UTC
Created attachment 90518 [details] [review]
Bug 20388: Keep data consistency when adding/removing mapping

Test plan:
  - create a new search field, save,
  - create a mapping using it (don't save),
  - check that you can't delete the search field.

  - delete all the mapping related to a search field (biblios and
    authorities), don't save,
  - check that you can delete this search field.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Michal Denar <black23@gmail.com>
Comment 81 Michal Denar 2019-06-11 19:26:56 UTC
Created attachment 90519 [details] [review]
Bug 20388: Prevent user from using a search field for a mapping after removing it

Test plan:

 - add a search field,
 - save,
 - remove this search field
 - check that it has been removed from the mapping lists (bottom
   select).

Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Michal Denar <black23@gmail.com>
Comment 82 Michal Denar 2019-06-11 19:27:00 UTC
Created attachment 90520 [details] [review]
Bug 20388: Use live handler for deleting elements

Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Michal Denar <black23@gmail.com>
Comment 83 Michal Denar 2019-06-11 19:27:05 UTC
Created attachment 90521 [details] [review]
Bug 20388: QA fixes: - pass search field objects in template instead of unblessed ones - disable drag n drop

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Michal Denar <black23@gmail.com>
Comment 84 Michal Denar 2019-06-11 19:27:10 UTC
Created attachment 90522 [details] [review]
Bug 20388: QA fixe : delete line who add elements in biblios and authorities in the same time

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 85 Michal Denar 2019-06-11 19:27:14 UTC
Created attachment 90523 [details] [review]
Bug 20388: add of types in the new entries dropdown of searchfiels

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 86 Michal Denar 2019-06-11 19:27:19 UTC
Created attachment 90524 [details] [review]
Bug 20388: QA fixe : add searchfield only if it doesn't exist

To test:

Without the fix:
- in search field configuration -> search field
- add the search field "uri" at the bottom of the tab
- you should have two fields "uri" like that : https://snag.gy/BpxcIq.jpg

Then apply the fix
- try to add the search field uri
- this time an alert should appear with the message :
" SearchField uri already exist "

Signed-off-by: Michal Denar <black23@gmail.com>