Bug 27316 - In mappings use yes/no for sortable
Summary: In mappings use yes/no for sortable
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Fridolin Somers
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 28279
  Show dependency treegraph
 
Reported: 2020-12-30 13:33 UTC by Fridolin Somers
Modified: 2021-12-13 21:11 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: String patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.04


Attachments
Bug 27316: In Elasticsearch mappings use yes/no for sortable (3.06 KB, patch)
2020-12-30 13:39 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 27316: In Elasticsearch mappings use yes/no for sortable (3.12 KB, patch)
2020-12-30 20:32 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 27316: [alternate] Convert 'undef' for sort to Yes and use Yes/No instead of 0/1 (4.71 KB, patch)
2021-01-07 20:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27316: (follow-up) Default to 1 if undef in mappings (974 bytes, patch)
2021-01-08 13:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27316: Convert 'undef' for sort to Yes and use Yes/No instead of 0/1 (4.80 KB, patch)
2021-01-28 14:57 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 27316: (follow-up) Default to 1 if undef in mappings (1.01 KB, patch)
2021-01-28 14:58 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 27316: Convert 'undef' for sort to Yes and use Yes/No instead of 0/1 (4.86 KB, patch)
2021-03-01 10:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27316: (follow-up) Default to 1 if undef in mappings (1.08 KB, patch)
2021-03-01 10:00 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2020-12-30 13:33:14 UTC
In search engine configuration (Elasticsearch), there is a colum 'sortable'.
It contains 'Undef', 0 or 1.
0 and 1 are not really friendly values for a boolean.
And also the translation seems to replace 0 by empty string (seen in fr-FR translation).

I propose to replace by Yes/No like other boolean.
Comment 1 Fridolin Somers 2020-12-30 13:39:25 UTC
Created attachment 114743 [details] [review]
Bug 27316: In Elasticsearch mappings use yes/no for sortable

In search engine configuration (Elasticsearch), there is a colum 'sortable'.
It contains 'Undef', 0 or 1.
0 and 1 are not really friendly values for a boolean.
And also the translation seems to replace 0 by empty string (seen in fr-FR translation).

I propose to replace by Yes/No like other boolean.

Test plan :
1) Without patch, look at Administration > Search engine configuration (Elasticsearch)
2) Look at tab 'Bibliographic records'
3) Look at a row containing 0 and a row containing 1
4) Apply patch
5) Refresh page
6) Check 0 is now 'No' and 1 is now 'Yes'
7) Check creating a new row with 'Yes' or 'No'
Comment 2 Lucas Gass 2020-12-30 20:31:28 UTC
much more user friendly, thank you
Comment 3 Lucas Gass 2020-12-30 20:32:20 UTC
Created attachment 114752 [details] [review]
Bug 27316: In Elasticsearch mappings use yes/no for sortable

In search engine configuration (Elasticsearch), there is a colum 'sortable'.
It contains 'Undef', 0 or 1.
0 and 1 are not really friendly values for a boolean.
And also the translation seems to replace 0 by empty string (seen in fr-FR translation).

I propose to replace by Yes/No like other boolean.

Test plan :
1) Without patch, look at Administration > Search engine configuration (Elasticsearch)
2) Look at tab 'Bibliographic records'
3) Look at a row containing 0 and a row containing 1
4) Apply patch
5) Refresh page
6) Check 0 is now 'No' and 1 is now 'Yes'
7) Check creating a new row with 'Yes' or 'No'

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 4 Nick Clemens 2021-01-07 20:18:27 UTC
Created attachment 114943 [details] [review]
Bug 27316: [alternate] Convert 'undef' for sort to Yes and use Yes/No instead of 0/1

This patch changes the interface to remove the undef option
We also update existing mappings and set default of 1

Comment previously indicated undef was treated differently, but in the code we treat undef as 1
That code is untoched to avoid creating problems for older mappings that may be reloaded

To test:
1 - View the ES mappings, note you can set columns to Undef, 0, 1
2 - Set some to undef
3 - Apply patches
4 - Update database
5 - Undef now show as 'Yes'
6 - Confirm you can set columns as Yes/no
7 - Confirm that mandatory columns display correct value when readonly (see issues field)
Comment 5 Nick Clemens 2021-01-07 20:19:00 UTC
Lets remove the ambiguity of 'Undef' which we treat as 'Yes'
Comment 6 Nick Clemens 2021-01-08 13:14:06 UTC
Created attachment 114955 [details] [review]
Bug 27316: (follow-up) Default to 1 if undef in mappings
Comment 7 Katrin Fischer 2021-01-10 22:25:47 UTC
It looks like the alternate patch hasn't been signed off. Nick, can you please clarify which patches need to be applied and maybe Lucas could test again?
Comment 8 Nick Clemens 2021-01-15 17:19:52 UTC
Frido, can you compare my patches and see if you approve?
Comment 9 Fridolin Somers 2021-01-27 14:46:38 UTC
(In reply to Nick Clemens from comment #5)
> Lets remove the ambiguity of 'Undef' which we treat as 'Yes'

Ok indeed, this is misleading.

I'll validate.
Comment 10 Fridolin Somers 2021-01-28 14:57:14 UTC
DEV atomic update: bug_23716_update_sort_mappings.perl
Upgrade to XXX done [15:35:00]: Bug 27316 - Description

I've set description : "In Elastisearch mappings convert NULL (Undef) for sort to 1 (Yes)"
Comment 11 Fridolin Somers 2021-01-28 14:57:50 UTC
Created attachment 115938 [details] [review]
Bug 27316: Convert 'undef' for sort to Yes and use Yes/No instead of 0/1

This patch changes the interface to remove the undef option
We also update existing mappings and set default of 1

Comment previously indicated undef was treated differently, but in the code we treat undef as 1
That code is untoched to avoid creating problems for older mappings that may be reloaded

To test:
1 - View the ES mappings, note you can set columns to Undef, 0, 1
2 - Set some to undef
3 - Apply patches
4 - Update database
5 - Undef now show as 'Yes'
6 - Confirm you can set columns as Yes/no
7 - Confirm that mandatory columns display correct value when readonly (see issues field)

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 12 Fridolin Somers 2021-01-28 14:58:18 UTC
Created attachment 115939 [details] [review]
Bug 27316: (follow-up) Default to 1 if undef in mappings

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 13 Fridolin Somers 2021-01-28 15:01:07 UTC
Should we also change mappings.yaml to use only 0 and 1 ?

With 1 on fields actually sortable :

https://git.koha-community.org/Koha-community/Koha/src/commit/6b23c0634b43a60f0a434d4c43a3e57718ad0806/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm#L694
Comment 14 Katrin Fischer 2021-02-07 12:08:12 UTC
(In reply to Fridolin Somers from comment #13)
> Should we also change mappings.yaml to use only 0 and 1 ?
> 
> With 1 on fields actually sortable :
> 
> https://git.koha-community.org/Koha-community/Koha/src/commit/
> 6b23c0634b43a60f0a434d4c43a3e57718ad0806/Koha/SearchEngine/Elasticsearch/
> QueryBuilder.pm#L694

Nick and others more familiar with Elasticsearch than me - can you answer Frido's question? :)
Comment 15 Nick Clemens 2021-02-22 16:02:20 UTC
(In reply to Katrin Fischer from comment #14)
> (In reply to Fridolin Somers from comment #13)
> > Should we also change mappings.yaml to use only 0 and 1 ?
> > 
> > With 1 on fields actually sortable :
> > 
> > https://git.koha-community.org/Koha-community/Koha/src/commit/
> > 6b23c0634b43a60f0a434d4c43a3e57718ad0806/Koha/SearchEngine/Elasticsearch/
> > QueryBuilder.pm#L694
> 
> Nick and others more familiar with Elasticsearch than me - can you answer
> Frido's question? :)

That would make sense, we should be able to simply apply the patches and generate the new mappings via export I believe
Comment 16 Fridolin Somers 2021-02-24 16:20:22 UTC
See Bug 24997
Comment 17 Martin Renvoize 2021-03-01 10:00:06 UTC
Created attachment 117429 [details] [review]
Bug 27316: Convert 'undef' for sort to Yes and use Yes/No instead of 0/1

This patch changes the interface to remove the undef option
We also update existing mappings and set default of 1

Comment previously indicated undef was treated differently, but in the code we treat undef as 1
That code is untoched to avoid creating problems for older mappings that may be reloaded

To test:
1 - View the ES mappings, note you can set columns to Undef, 0, 1
2 - Set some to undef
3 - Apply patches
4 - Update database
5 - Undef now show as 'Yes'
6 - Confirm you can set columns as Yes/no
7 - Confirm that mandatory columns display correct value when readonly (see issues field)

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2021-03-01 10:00:10 UTC
Created attachment 117430 [details] [review]
Bug 27316: (follow-up) Default to 1 if undef in mappings

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2021-03-01 10:00:48 UTC
Works nicely and is much cleaner/clearer.  No regressions found.. Passing QA
Comment 20 Jonathan Druart 2021-03-01 14:24:16 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 21 Fridolin Somers 2021-03-04 09:44:46 UTC
Pushed to 20.11.x for 20.11.04
Comment 22 Fridolin Somers 2021-03-04 10:13:27 UTC
(In reply to Nick Clemens from comment #15)
> (In reply to Katrin Fischer from comment #14)
> > (In reply to Fridolin Somers from comment #13)
> > > Should we also change mappings.yaml to use only 0 and 1 ?
> > > 
> > > With 1 on fields actually sortable :
> > > 
> > > https://git.koha-community.org/Koha-community/Koha/src/commit/
> > > 6b23c0634b43a60f0a434d4c43a3e57718ad0806/Koha/SearchEngine/Elasticsearch/
> > > QueryBuilder.pm#L694
> > 
> > Nick and others more familiar with Elasticsearch than me - can you answer
> > Frido's question? :)
> 
> That would make sense, we should be able to simply apply the patches and
> generate the new mappings via export I believe

Created Bug 27856
Comment 23 Andrew Fuerste-Henry 2021-03-14 18:28:06 UTC
Doesn't apply cleanly to 20.05. Please rebase if needed.