lms icon indicating copy to clipboard operation
lms copied to clipboard

"Save" button on admin pages turns all `None` settings to `False`

Open seanh opened this issue 3 years ago • 2 comments

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.

seanh avatar Dec 01 '21 16:12 seanh

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

seanh avatar Dec 01 '21 16:12 seanh

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.

robertknight avatar Feb 09 '24 14:02 robertknight