Bug 29893

Summary: ElasticSearch Config UI deletes mappings
Product: Koha Reporter: Thomas Klausner <domm>
Component: Searching - ElasticsearchAssignee: Jonathan Druart <jonathan.druart+koha>
Status: Pushed to stable --- QA Contact:
Severity: critical    
Priority: P5 - low CC: black23, david, fridolin.somers, jonathan.druart+koha, kyle, martin.renvoize, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.05
Bug Depends on:    
Bug Blocks: 30000, 30584    
Attachments: Bug 29893: Prevent ES mappings UI to remove hidden rows
sign-off
Bug 29893: Prevent ES mappings UI to remove hidden rows
Bug 29893: Prevent ES mappings UI to remove hidden rows
Bug 29893: Make sure the form will be submitted after the draw calls
Bug 29893: Redraw the tables from other tabs without paging
Bug 29893: Prevent ES mappings UI to remove hidden rows
Bug 29893: Make sure the form will be submitted after the draw calls
Bug 29893: Redraw the tables from other tabs without paging
Bug 29893: Prevent ES mappings UI to remove hidden rows
Bug 29893: Make sure the form will be submitted after the draw calls
Bug 29893: Redraw the tables from other tabs without paging

Description Thomas Klausner 2022-01-17 13:48:29 UTC
When using the Elasticsearch UI /cgi-bin/koha/admin/searchengine/elasticsearch/mappings.pl to edit search mappings one can quite easily delete a lot of mappings by mistake.

To reproduce:

* go to /cgi-bin/koha/admin/searchengine/elasticsearch/mappings.pl
* click on the "Authorities" Tab
* You should see ~ "67 entries"
* enter "Heading-Main" into the search field, you now see ~8 entries ("filtered from 67 entries"
* Click "save" Button
* Go again to the "Autorities" Tab
* You now see ~ "8 entries"
* All the other entries have been deleted!

What obviously happens is that all entries that where filtered due to the search are not submitted to the backend on save. The backend now interprets the missing entries as entries that shall be deleted. And happily deleted them!

This totally breaks the search index (after the next index run, or when adding single items), as (eg in the auth-index) "authtype" is no longer indexed.

I currently don't have a 21.11 or later version at hand, so maybe this has already been fixed?

To fix this bug we would either have to un-hide the filtered entries before submitting the form; change the backend to not deleted missing entries (which means that the delete-action would have to be implemented a bit different); something else?

I guess that the first fix is easier, and would also make sense with regard to how that form works.


Greetings,
domm
Comment 1 Jonathan Druart 2022-01-17 14:47:26 UTC Comment hidden (obsolete)
Comment 2 Thomas Klausner 2022-01-17 15:48:47 UTC
I can confirm that the patch provided by Jonathan fixed this bug.

Meta-Question: Can / should I set the status of this bug to "signed off" or RESOLVED? Or are status changes only "allowed" for proper team members?

Greetings,
domm
Comment 3 Jonathan Druart 2022-01-17 15:59:10 UTC
Ideally you should submit the patch with your "signed-off-by" line, then switch the status to "Signed Off".

https://wiki.koha-community.org/wiki/Sign_off_on_patches
Comment 4 Thomas Klausner 2022-01-18 13:35:52 UTC
Created attachment 129571 [details] [review]
sign-off

Sign-off for the patch, after applying it the bug is fixed.
Comment 5 Jonathan Druart 2022-02-07 16:11:47 UTC
Created attachment 130237 [details] [review]
Bug 29893: Prevent ES mappings UI to remove hidden rows

If you are editing ES mappings and you use the DataTable filtering option,
the table won't contain all the mappings you have in the DB.
If the form is submitted they will be removed and data will be lost!

Test plan:
Edit ES mappings, filter and submit the form.
Confirm that the table are redrawn before the form submission which
prevent data loss
Comment 6 Jonathan Druart 2022-02-07 16:14:18 UTC
Nick told me the previous patch didn't work for him. This method is using what is described on the DataTable's website https://datatables.net/examples/api/form.html and so might be a better approach.
See also https://www.gyrocode.com/articles/jquery-datatables-how-to-submit-all-pages-form-data/#solution-ajax

This seems quite ugly to me, especially the redirect, but I don't have anything else to suggest for now.
Comment 7 David Nind 2022-02-08 18:28:26 UTC
Issues from testing:

1. The patch works for me - it stops mappings being deleted as reported. I had to clear the browser cache for this to work.

2. However, with mappings either filtered or unfiltered, if you:
   2.1: Change one (for example: Authorities > Heading-Main, 155a - change the Yess to Nos), save, the change to the mapping is not saved.
   2.2: Delete one, save, the deleted mapping is not deleted.
   2.3: Add a new one (for example: Heading-Main, Yes for all options, 155b), the new mapping is not saved.

3. I also noted that when adding a new mapping, the options available for the sortable column are Undef, 0 and 1. For all the other columns the options are Yes and No. I'm not familiar enough with Elastic to know whether this is correct or not.

Tested using Firefox Developer Edition.
Comment 8 Jonathan Druart 2022-02-09 10:08:12 UTC
Created attachment 130348 [details] [review]
Bug 29893: Prevent ES mappings UI to remove hidden rows

If you are editing ES mappings and you use the DataTable filtering option,
the table won't contain all the mappings you have in the DB.
If the form is submitted they will be removed and data will be lost!

Test plan:
Edit ES mappings, filter and submit the form.
Confirm that the table are redrawn before the form submission which
prevent data loss
Comment 9 Jonathan Druart 2022-02-09 10:08:16 UTC
Created attachment 130349 [details] [review]
Bug 29893: Make sure the form will be submitted after the draw calls
Comment 10 Jonathan Druart 2022-02-09 10:09:04 UTC
Sorry David, the previous patch was terribly wrong.

I kept the first approach and fix what I think was a bug.
Comment 11 David Nind 2022-02-09 18:16:01 UTC
Thanks Jonathan!

However, I am now getting these messages in yellow boxes above the "Search engine configuration (Elasticsearch)" heading:

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

  "An error occurred when updating mappings: ." 

This occurs any time the save button is clicked.

On the positive side, the mappings are no longer deleted. 

On the downside, you can't edit, add or delete any mappings.

Consolidated test plan:
1. In the staff interface go to Administration > Catalog > Search engine configuration (Elasticsearch).
2. Select the authorities tab.
3. You should see ~67 entries.
4. Enter "Heading-Main" into the search field, you now see ~8 entries (filtered from 67 entries).
5. Cick the save button.
6. Go back to the authorities tab.
7. You will now see ~8 entries - all the other entries were deleted!
8. Apply the patch, flush_memcached, restart_all, and you may also need to clear your browser cache (or open a private window).
9. Repeat steps 1-7 - this time the expected number of entries should be visible after clicking save (~67), that is, all the entries are not deleted when you click save!
10. With mappings filtered and unfiltered, check that everything works as expected if you:
    . Change an entry (for example: Authorities > Heading-Main, 155a - change the Yess to Nos).
    . Delete an entry.
    . Add a new entry. (for example: Heading-Main, Yes for all options, 155b).
11. Sign off :D!
Comment 12 Nick Clemens 2022-02-17 15:58:02 UTC
When I follow the test plan all authority mappings except for heading-main are still deleted
Comment 13 Michal Denar 2022-02-17 16:00:52 UTC
Same for me. If I changed the values, the original values remained.
Comment 14 Jonathan Druart 2022-02-17 16:40:22 UTC
Created attachment 130764 [details] [review]
Bug 29893: Redraw the tables from other tabs without paging
Comment 15 Jonathan Druart 2022-02-17 16:42:24 UTC
I didn't recreate David or Michal problems.

Nick's problem should be fixed with the last patch.

Sorry guys, but what we are doing in this template is weird (we drop and redraw the table every time a tab is clicked, that's not efficient).
Comment 16 Nick Clemens 2022-02-17 19:01:04 UTC
Created attachment 130768 [details] [review]
Bug 29893: Prevent ES mappings UI to remove hidden rows

If you are editing ES mappings and you use the DataTable filtering option,
the table won't contain all the mappings you have in the DB.
If the form is submitted they will be removed and data will be lost!

Test plan:
Edit ES mappings, filter and submit the form.
Confirm that the table are redrawn before the form submission which
prevent data loss

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2022-02-17 19:01:09 UTC
Created attachment 130769 [details] [review]
Bug 29893: Make sure the form will be submitted after the draw calls

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Nick Clemens 2022-02-17 19:01:12 UTC
Created attachment 130770 [details] [review]
Bug 29893: Redraw the tables from other tabs without paging

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Jonathan Druart 2022-03-21 13:43:35 UTC
What is explaining a critical bug stuck in NQA for more than a month?
Comment 20 Martin Renvoize 2022-03-24 16:53:22 UTC
Created attachment 132185 [details] [review]
Bug 29893: Prevent ES mappings UI to remove hidden rows

If you are editing ES mappings and you use the DataTable filtering option,
the table won't contain all the mappings you have in the DB.
If the form is submitted they will be removed and data will be lost!

Test plan:
Edit ES mappings, filter and submit the form.
Confirm that the table are redrawn before the form submission which
prevent data loss

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2022-03-24 16:53:26 UTC
Created attachment 132186 [details] [review]
Bug 29893: Make sure the form will be submitted after the draw calls

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2022-03-24 16:53:30 UTC
Created attachment 132187 [details] [review]
Bug 29893: Redraw the tables from other tabs without paging

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2022-03-24 16:54:34 UTC
Apologies, not been on QA for a little while whilst I played catch up.

Patch works as expected, man the logic flow is weird here.

No regressions found.

Passing QA
Comment 24 Fridolin Somers 2022-03-25 00:25:21 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 25 Kyle M Hall 2022-03-25 14:00:02 UTC
Pushed to 21.11.x for 21.11.05