Better handle some defaults and setting keys #26991
Conversation
mm-cloud-bot
added
the
release-note
| @@ -47,7 +47,7 @@ function makeGetPluginSchema() { | |||
| let settings: Array<Partial<AdminDefinitionSetting>> = []; | |||
| if (plugin.settings_schema && plugin.settings_schema.settings) { | |||
| settings = plugin.settings_schema.settings.map((setting) => { | |||
| const key = setting.key.toLowerCase(); | |||
| const key = setting.key; | |||
There was a problem hiding this comment.
I don't understand why we were lowercasing this key, since it was not lowercased in all relevant places (specially the plugin settings that come from the server). Removing it so we always are able to get the right config for the right setting.
There was a problem hiding this comment.
I can't comment on this specific line of code, but I /seem/ to recall that plugin settings are supposed to be case insensitive. I can't remember if that's true for all configuration settings, or just plugins due to some old code and how it handled things in the past.
There was a problem hiding this comment.
They may have been, but I wans't able to find anything regarding that in the documentation (maybe I didn't look hard enough 😅 ).
What I found is that the config is reaching the state with the casing defined in the plugin (in the configuration model struct), and it was causing some data not to be properly gotten from the "current value".
I also checked the code of the MSTeams plugin and there was some test that was setting both in the initial state the camelCase version and the lowercase version (server/testutils/containere2e/containere2e.go line 113), which leads me to think this has been a "hidden issue" for a long time.
There was a problem hiding this comment.
I wonder if it's related to the old library that we used for the config file and environment variables, but I can't remember if we even use it any more. It had some odd behaviour where it lower cased names of things which I had to account for in the code involving the environmentConfig, so perhaps that's related to that. The plugin settings are a bit different than other settings because they're a map[string]any instead of a struct, so that might be why they were treated differently
There was a problem hiding this comment.
Is there any specific reason to make this change? With plugins the problem is that a change here that seems "right" can break something far away without warning or understanding.
There was a problem hiding this comment.
Yeah, I agree with leaving this as-is unless we fully understand why it works the way it is. Given that this PR is fixing a regression caused by cleaning up one of the various quirks that the system console code has, it would be safer to not change this unless there's a good reason to (Edit: I forgot that this PR was already split from the one about fixing the default values for other config settings)
There was a problem hiding this comment.
The specific reason is that apparently, any setting with uppercase in their name will not be picked by this code to set the initial value.
This was the quickest approach, but there are other options:
- search the config key lowercasing it, so we can have case insensitive configuration
- change the plugins (ms teams plugin in particular) so they don't use uppercase in their config names
Or we can leave it like this as a known issue. 0/5
| if (this.state[setting.key] === undefined && 'default' in setting) { | ||
| return setting.default; | ||
| } |
There was a problem hiding this comment.
This way we set all the values to the default if needed, instead of leaving them undefined.
amyblais
added
the
CherryPick/Approved
There was a problem hiding this comment.
Thanks, @larkox! I don't have a ton to offer besides historical context, but direction looks good :)
| @@ -47,7 +47,7 @@ function makeGetPluginSchema() { | |||
| let settings: Array<Partial<AdminDefinitionSetting>> = []; | |||
| if (plugin.settings_schema && plugin.settings_schema.settings) { | |||
| settings = plugin.settings_schema.settings.map((setting) => { | |||
| const key = setting.key.toLowerCase(); | |||
| const key = setting.key; | |||
There was a problem hiding this comment.
I can't comment on this specific line of code, but I /seem/ to recall that plugin settings are supposed to be case insensitive. I can't remember if that's true for all configuration settings, or just plugins due to some old code and how it handled things in the past.
webapp/channels/src/components/admin_console/schema_admin_settings.tsx
Outdated
Show resolved
Hide resolved
| @@ -331,7 +331,7 @@ function adminConsoleCustomComponents(state: {[pluginId: string]: Record<string, | |||
| } | |||
|
|
|||
| const pluginId = action.data.pluginId; | |||
| const key = action.data.key.toLowerCase(); | |||
| const key = action.data.key; | |||
There was a problem hiding this comment.
Similar to above, this could also be a source of future bugs...
amyblais
removed
the
CherryPick/Approved
Summary
During my initial PR, I changed how many settings we send on save, so we only send the modified settings.
I changed this so we send all the settings, with the default sets.
I also removed a couple of
toLowerCasethat were creating issues with setting keys with camelCase notation.Finally, I also changed how we deal with some falsy defaults, specially on number fields.
Ticket Link
NONE
Release Note