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.
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'
much more user friendly, thank you
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>
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)
Lets remove the ambiguity of 'Undef' which we treat as 'Yes'
Created attachment 114955 [details] [review] Bug 27316: (follow-up) Default to 1 if undef in mappings
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?
Frido, can you compare my patches and see if you approve?
(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.
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)"
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>
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>
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
(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? :)
(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
See Bug 24997
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>
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>
Works nicely and is much cleaner/clearer. No regressions found.. Passing QA
Pushed to master for 21.05, thanks to everybody involved!
Pushed to 20.11.x for 20.11.04
(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
Doesn't apply cleanly to 20.05. Please rebase if needed.