Bug 31364 - Saving multi-select system preference don't save when all checks are removed
Summary: Saving multi-select system preference don't save when all checks are removed
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Joonas Kylmälä
QA Contact: Katrin Fischer
URL:
Keywords:
: 31474 31524 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-08-15 21:36 UTC by Rhonda Kuiper
Modified: 2022-11-21 10:01 UTC (History)
7 users (show)

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


Attachments
Bug 31364: Allow multiple select system prefernces to saved when they are empty/null (1.37 KB, patch)
2022-09-01 22:10 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 31364: Allow multiple select system prefernces to saved when they are empty/null (1.42 KB, patch)
2022-09-02 11:04 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 31364: Check length of the multiple syspref array instead whether it is null (1.58 KB, patch)
2022-09-04 11:44 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 31364: Check length of the multiple syspref array instead whether it is null (1.62 KB, patch)
2022-09-06 10:57 UTC, David Nind
Details | Diff | Splinter Review
Bug 31364: Check length of the multiple syspref array instead whether it is null (1.68 KB, patch)
2022-09-07 11:15 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Rhonda Kuiper 2022-08-15 21:36:20 UTC
When updating a system preference with a dropdown that contains checkboxes, the changes are not saved in the case where there were items checked and then you try to remove all checks.  
An example of this is OPACHoldsIfAvailableAtPickupExceptions.  
Set a couple patron categories as exceptions.  
Save.  
Remove all the categories from the exception list.  
Save.  
You get a message that says 'Nothing to Save' and the changes are not made.
Comment 1 Katrin Fischer 2022-08-28 13:03:18 UTC
*** Bug 31474 has been marked as a duplicate of this bug. ***
Comment 2 Lucas Gass 2022-09-01 22:04:44 UTC
In preferences.js, if we are dealing with multiple select type system preference we check to make sure the value is not NULL before pushing anything to the unserialized array.  I think we should allow for that value to be empty/null.
Comment 3 Lucas Gass 2022-09-01 22:10:06 UTC
Created attachment 140064 [details] [review]
Bug 31364: Allow multiple select system prefernces to saved when they are empty/null

TO test:
1. Have a multiple select system preference like 'OPACHoldsIfAvailableAtPickupExceptions'.
2. Set a couple patron categories as exceptions and save.
3. Remove all the categories from the exception list and save.
4. You get a message that says 'Nothing to Save' and the changes are not made.
5. Apply patch
6. Try 2-4 again, excpet this time it should save.
Comment 4 Owen Leonard 2022-09-02 11:04:00 UTC
Created attachment 140102 [details] [review]
Bug 31364: Allow multiple select system prefernces to saved when they are empty/null

TO test:
1. Have a multiple select system preference like
   'OPACHoldsIfAvailableAtPickupExceptions'.
2. Set a couple patron categories as exceptions and save.
3. Remove all the categories from the exception list and save.
4. You get a message that says 'Nothing to Save' and the changes are not
   made.
5. Apply patch
6. Try 2-4 again, excpet this time it should save.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 5 Joonas Kylmälä 2022-09-04 11:33:04 UTC
Adds a comma in the end of the syspref list due to it sending an empty sysref value in addition to filled ones in the query string:

> MariaDB [koha_kohadev]> select * from systempreferences where variable = "OPACHoldsIfAvailableAtPickupExceptions"\G
> *************************** 1. row ***************************
>    variable: OPACHoldsIfAvailableAtPickupExceptions
>       value: B,HB,
>     options: 
> explanation: List the patron categories not affected by OPACHoldsIfAvailableAtPickup if off
>        type: Free

Since the sysref object value is an array we can just check instead whether the array is empty. Will send a patch shortly
Comment 6 Joonas Kylmälä 2022-09-04 11:44:58 UTC
Created attachment 140201 [details] [review]
Bug 31364: Check length of the multiple syspref array instead whether it is null

The multiple systempreferences are always defined as an empty array at
the minimum, so in order to determine whether we need to remove all
the selections from the systempreference we should check whether the
array is empty and not whether the array doesn't exist.

To test:
 1) Set at least 1 value for OPACHoldsIfAvailableAtPickupExceptions
 2) Remove all the values for OPACHoldsIfAvailableAtPickupExceptions
 and notice you get the message "Nothing to save"
 3) Apply patch and repeat, removing all the values should work now.
Comment 7 David Nind 2022-09-06 10:57:16 UTC
Created attachment 140220 [details] [review]
Bug 31364: Check length of the multiple syspref array instead whether it is null

The multiple systempreferences are always defined as an empty array at
the minimum, so in order to determine whether we need to remove all
the selections from the systempreference we should check whether the
array is empty and not whether the array doesn't exist.

To test:
 1) Set at least 1 value for OPACHoldsIfAvailableAtPickupExceptions
 2) Remove all the values for OPACHoldsIfAvailableAtPickupExceptions
 and notice you get the message "Nothing to save"
 3) Apply patch and repeat, removing all the values should work now.

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 Katrin Fischer 2022-09-07 07:44:47 UTC
*** Bug 31524 has been marked as a duplicate of this bug. ***
Comment 9 Katrin Fischer 2022-09-07 11:15:45 UTC
Created attachment 140269 [details] [review]
Bug 31364: Check length of the multiple syspref array instead whether it is null

The multiple systempreferences are always defined as an empty array at
the minimum, so in order to determine whether we need to remove all
the selections from the systempreference we should check whether the
array is empty and not whether the array doesn't exist.

To test:
 1) Set at least 1 value for OPACHoldsIfAvailableAtPickupExceptions
 2) Remove all the values for OPACHoldsIfAvailableAtPickupExceptions
 and notice you get the message "Nothing to save"
 3) Apply patch and repeat, removing all the values should work now.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Tomás Cohen Arazi 2022-09-07 20:21:43 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 11 Lucas Gass 2022-10-31 19:28:09 UTC
Backported to 22.05.x for upcoming 22.05.07 release
Comment 12 Arthur Suzuki 2022-11-14 13:27:55 UTC
applied to 21.11.x for 21.11.14
Comment 13 Victor Grousset/tuxayo 2022-11-18 06:19:37 UTC
Doesn't affect 21.05.x, not backported.

Nothing to document, marking resolved.
Comment 14 Arthur Suzuki 2022-11-21 10:01:38 UTC
revert on 21.11, bug not reproduced