lms
lms copied to clipboard
"Save" button on admin pages turns all `None` settings to `False`
When you view an application instance on the admin pages (e.g. http://localhost:8001/admin/instance/Hypothesis14af0fe87c9deb2e461f88be4a8d5364/) it may have some settings missing from its application_instances.settings
column, for example:
{"blackboard": {"files_enabled": true, "groups_enabled": true}}
If you then click the Save button without changing any of the settings and submit the form, all of the application instance's settings that were previously missing will now be false
:
{"canvas": {"groups_enabled": false, "sections_enabled": false}, "blackboard": {"files_enabled": true, "groups_enabled": true}, "microsoft_onedrive": {"files_enabled": true}}
There's a potential unintended consequence or changes in behaviour here, depending on how the settings are used, because previously application_instance.settings.get("canvas", "groups_enabled")
(for example) would have returned None
whereas now it will return False
. None
and False
are two different values and could potentially be treated differently by the code.
I think the root of the problem here is probably that application_instance.settings.get("foo", "bar")
can either return a boolean or None
(or indeed any other JSON-serializable type if it got into the DB somehow), and that makes mistakes like this likely. I think we want these feature flags to always be booleans, never None
. For example an application_instance.groups_enabled
attribute that's always True
or False
Logically I think we want all boolean feature flags to be represented as tri-state on the settings page: On / off / default. This can accurately model how the data is stored.
When reading feature flags in the app, you'd use methods that map "default" to the global default.
This came up today in this thread where I wanted to change the default for a flag without affecting existing installations where the setting was explicitly configured.