From f74eea11ba43624b38c7a9a27f3722465ac909b1 Mon Sep 17 00:00:00 2001 From: Scott Bishel Date: Tue, 9 Nov 2021 07:56:04 -0700 Subject: [PATCH] Fix for feature flag handling (#1753) * fix for handling feature flags changing * update tests * fix tests * lint fixes * feedback updates * use getConfig instead * linter fix * more linter * revert changes Co-authored-by: Mattermod --- mattermost-plugin/server/configuration.go | 26 +++-- .../server/configuration_test.go | 109 +++++++++--------- mattermost-plugin/server/plugin.go | 27 +++-- server/server/server.go | 11 +- 4 files changed, 84 insertions(+), 89 deletions(-) diff --git a/mattermost-plugin/server/configuration.go b/mattermost-plugin/server/configuration.go index a7b7dcfba..044ca8621 100644 --- a/mattermost-plugin/server/configuration.go +++ b/mattermost-plugin/server/configuration.go @@ -2,9 +2,6 @@ package main import ( "reflect" - - "github.com/mattermost/focalboard/server/utils" - "github.com/pkg/errors" ) // configuration captures the plugin's external configuration as exposed in the Mattermost server @@ -71,21 +68,28 @@ func (p *Plugin) setConfiguration(configuration *configuration) { } // OnConfigurationChange is invoked when configuration changes may have been made. -func (p *Plugin) OnConfigurationChange() error { +func (p *Plugin) OnConfigurationChange() error { //nolint // Have we been setup by OnActivate? if p.wsPluginAdapter == nil { return nil } - configuration := &configuration{} - // Load the public configuration fields from the Mattermost server configuration. - if err := p.API.LoadPluginConfiguration(&configuration); err != nil { - return errors.Wrap(err, "failed to load plugin configuration") + mmconfig := p.API.GetConfig() + + // handle plugin configuration settings + enableShareBoards := false + if mmconfig.PluginSettings.Plugins[pluginName][sharedBoardsName] == true { + enableShareBoards = true + } + configuration := &configuration{ + EnablePublicSharedBoards: enableShareBoards, } - p.setConfiguration(configuration) - p.server.UpdateClientConfig(utils.StructToMap(configuration)) - p.wsPluginAdapter.BroadcastConfigChange(*p.server.App().GetClientConfig()) + p.server.Config().EnablePublicSharedBoards = enableShareBoards + // handle feature flags + p.server.Config().FeatureFlags = parseFeatureFlags(mmconfig.FeatureFlags.ToMap()) + p.server.UpdateAppConfig() + p.wsPluginAdapter.BroadcastConfigChange(*p.server.App().GetClientConfig()) return nil } diff --git a/mattermost-plugin/server/configuration_test.go b/mattermost-plugin/server/configuration_test.go index 6c8aaacb2..04b19abaf 100644 --- a/mattermost-plugin/server/configuration_test.go +++ b/mattermost-plugin/server/configuration_test.go @@ -1,90 +1,85 @@ package main import ( - "errors" "testing" "github.com/mattermost/focalboard/server/model" + "github.com/mattermost/focalboard/server/server" + "github.com/mattermost/focalboard/server/services/config" "github.com/mattermost/focalboard/server/ws" + serverModel "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-server/v6/plugin/plugintest" - "github.com/mattermost/mattermost-server/v6/plugin/plugintest/mock" + "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) -var errLoadPluginConfiguration = errors.New("loadPluginConfiguration Error") +type TestHelper struct { + Server *server.Server +} -func TestConfiguration(t *testing.T) { - t.Run("null configuration", func(t *testing.T) { - plugin := &Plugin{} - assert.NotNil(t, plugin.getConfiguration()) +func SetupTestHelper() *TestHelper { + th := &TestHelper{} + th.Server = newTestServer() + return th +} + +func newTestServer() *server.Server { + srv, err := server.New(server.Params{ + Cfg: &config.Configuration{}, }) + if err != nil { + panic(err) + } - t.Run("changing configuration", func(t *testing.T) { - plugin := &Plugin{} + return srv +} - configuration1 := &configuration{EnablePublicSharedBoards: false} - plugin.setConfiguration(configuration1) - assert.Equal(t, configuration1, plugin.getConfiguration()) - - configuration2 := &configuration{EnablePublicSharedBoards: true} - plugin.setConfiguration(configuration2) - assert.Equal(t, configuration2, plugin.getConfiguration()) - assert.NotEqual(t, configuration1, plugin.getConfiguration()) - assert.False(t, plugin.getConfiguration() == configuration1) - assert.True(t, plugin.getConfiguration() == configuration2) - }) - - t.Run("setting same configuration", func(t *testing.T) { - plugin := &Plugin{} - - configuration1 := &configuration{} - plugin.setConfiguration(configuration1) - assert.Panics(t, func() { - plugin.setConfiguration(configuration1) - }) - }) - - t.Run("clearing configuration", func(t *testing.T) { - plugin := &Plugin{} - - configuration1 := &configuration{EnablePublicSharedBoards: true} - plugin.setConfiguration(configuration1) - assert.NotPanics(t, func() { - plugin.setConfiguration(nil) - }) - assert.NotNil(t, plugin.getConfiguration()) - assert.NotEqual(t, configuration1, plugin.getConfiguration()) - }) +func TestConfigurationNullConfiguration(t *testing.T) { + plugin := &Plugin{} + assert.NotNil(t, plugin.getConfiguration()) } func TestOnConfigurationChange(t *testing.T) { - t.Run("Test LoadPlugin Error", func(t *testing.T) { - api := &plugintest.API{} - api.On("LoadPluginConfiguration", - mock.Anything).Return(func(dest interface{}) error { - return errLoadPluginConfiguration - }) + stringRef := "" - p := Plugin{} - p.SetAPI(api) - p.wsPluginAdapter = &ws.PluginAdapter{} + basePlugins := make(map[string]map[string]interface{}) + basePlugins[pluginName] = make(map[string]interface{}) + basePlugins[pluginName][sharedBoardsName] = true - err := p.OnConfigurationChange() - assert.Error(t, err) - }) + baseFeatureFlags := &serverModel.FeatureFlags{ + BoardsFeatureFlags: "Feature1-Feature2", + } + basePluginSettings := &serverModel.PluginSettings{ + Directory: &stringRef, + Plugins: basePlugins, + } + + baseConfig := &serverModel.Config{ + FeatureFlags: baseFeatureFlags, + PluginSettings: *basePluginSettings, + } t.Run("Test Load Plugin Success", func(t *testing.T) { + th := SetupTestHelper() api := &plugintest.API{} - api.On("LoadPluginConfiguration", mock.AnythingOfType("**main.configuration")).Return(nil) + api.On("GetUnsanitizedConfig").Return(baseConfig) p := Plugin{} p.SetAPI(api) + p.server = th.Server p.wsPluginAdapter = &FakePluginAdapter{} err := p.OnConfigurationChange() - require.NoError(t, err) + assert.NoError(t, err) assert.Equal(t, 1, count) + + // make sure both App and Server got updated + assert.True(t, p.server.Config().EnablePublicSharedBoards) + assert.True(t, p.server.App().GetClientConfig().EnablePublicSharedBoards) + + assert.Equal(t, "true", p.server.Config().FeatureFlags["Feature1"]) + assert.Equal(t, "true", p.server.Config().FeatureFlags["Feature2"]) + assert.Equal(t, "", p.server.Config().FeatureFlags["Feature3"]) }) } diff --git a/mattermost-plugin/server/plugin.go b/mattermost-plugin/server/plugin.go index 8ea9146cd..82cfd1dbc 100644 --- a/mattermost-plugin/server/plugin.go +++ b/mattermost-plugin/server/plugin.go @@ -166,17 +166,7 @@ func (p *Plugin) createBoardsConfig(mmconfig mmModel.Config, baseURL string, ser enablePublicSharedBoards = true } - featureFlags := make(map[string]string) - for key, value := range mmconfig.FeatureFlags.ToMap() { - // Break out FeatureFlags and pass remaining - if key == boardsFeatureFlagName { - for _, flag := range strings.Split(value, "-") { - featureFlags[flag] = "true" - } - } else { - featureFlags[key] = value - } - } + featureFlags := parseFeatureFlags(mmconfig.FeatureFlags.ToMap()) return &config.Configuration{ ServerRoot: baseURL + "/plugins/focalboard", @@ -204,6 +194,21 @@ func (p *Plugin) createBoardsConfig(mmconfig mmModel.Config, baseURL string, ser } } +func parseFeatureFlags(configFeatureFlags map[string]string) map[string]string { + featureFlags := make(map[string]string) + for key, value := range configFeatureFlags { + // Break out FeatureFlags and pass remaining + if key == boardsFeatureFlagName { + for _, flag := range strings.Split(value, "-") { + featureFlags[flag] = "true" + } + } else { + featureFlags[key] = value + } + } + return featureFlags +} + func (p *Plugin) OnWebSocketConnect(webConnID, userID string) { p.wsPluginAdapter.OnWebSocketConnect(webConnID, userID) } diff --git a/server/server/server.go b/server/server/server.go index 8cc540655..dfe8fb8d4 100644 --- a/server/server/server.go +++ b/server/server/server.go @@ -352,16 +352,7 @@ func (s *Server) App() *app.App { return s.app } -func (s *Server) UpdateClientConfig(pluginConfig map[string]interface{}) { - for index, value := range pluginConfig { - if index == "EnablePublicSharedBoards" { - b, ok := value.(bool) - if !ok { - s.logger.Warn("Invalid value for config value", mlog.String(index, value.(string))) - } - s.config.EnablePublicSharedBoards = b - } - } +func (s *Server) UpdateAppConfig() { s.app.SetConfig(s.config) }