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 <mattermod@users.noreply.github.com>

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
 }

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"])
 	})
 }
 

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)
 }

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)
 }