From 946a5c1417e659516363a866856a1ec10a624ff5 Mon Sep 17 00:00:00 2001 From: TPaschalis Date: Sat, 12 Oct 2019 00:00:23 +0300 Subject: [PATCH 01/23] Mm 19027 (#12498) * Migrate tests from model/websocket_request_test.go to use testifyy * Migrate tests from model/websocket_request_test.go to use testify * Update model/websocket_request_test.go Co-Authored-By: Miguel de la Cruz * Update model/websocket_request_test.go Co-Authored-By: Miguel de la Cruz --- model/websocket_request_test.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/model/websocket_request_test.go b/model/websocket_request_test.go index 0918fb7146..edf8f538fd 100644 --- a/model/websocket_request_test.go +++ b/model/websocket_request_test.go @@ -6,6 +6,8 @@ package model import ( "strings" "testing" + + "github.com/stretchr/testify/require" ) func TestWebSocketRequest(t *testing.T) { @@ -13,13 +15,9 @@ func TestWebSocketRequest(t *testing.T) { json := m.ToJson() result := WebSocketRequestFromJson(strings.NewReader(json)) - if result == nil { - t.Fatal("should not be nil") - } + require.NotNil(t, result) badresult := WebSocketRequestFromJson(strings.NewReader("junk")) - if badresult != nil { - t.Fatal("should have been nil") - } + require.Nil(t, badresult) } From fa8e57d91a0e9e66c300f132ad19ca850faa77b0 Mon Sep 17 00:00:00 2001 From: Karan Nadagoudar Date: Sat, 12 Oct 2019 14:39:27 +0530 Subject: [PATCH 02/23] [MM-19122] Migrate tests from "model/user_access_token_test.go" to use testify (#12581) * Migrated to testify/require and testify/assert. --- model/user_access_token_test.go | 34 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/model/user_access_token_test.go b/model/user_access_token_test.go index 1b4a9ccfd6..04ac3e96a7 100644 --- a/model/user_access_token_test.go +++ b/model/user_access_token_test.go @@ -6,6 +6,8 @@ package model import ( "strings" "testing" + + "github.com/stretchr/testify/require" ) func TestUserAccessTokenJson(t *testing.T) { @@ -16,43 +18,33 @@ func TestUserAccessTokenJson(t *testing.T) { json := a1.ToJson() ra1 := UserAccessTokenFromJson(strings.NewReader(json)) - if a1.Token != ra1.Token { - t.Fatal("tokens didn't match") - } + require.Equal(t, a1.Token, ra1.Token, "tokens didn't match") tokens := []*UserAccessToken{&a1} json = UserAccessTokenListToJson(tokens) tokens = UserAccessTokenListFromJson(strings.NewReader(json)) - if tokens[0].Token != a1.Token { - t.Fatal("tokens didn't match") - } + require.Equal(t, tokens[0].Token, ra1.Token, "tokens didn't match") } func TestUserAccessTokenIsValid(t *testing.T) { ad := UserAccessToken{} - if err := ad.IsValid(); err == nil || err.Id != "model.user_access_token.is_valid.id.app_error" { - t.Fatal(err) - } + err := ad.IsValid() + require.False(t, err == nil || err.Id != "model.user_access_token.is_valid.id.app_error") ad.Id = NewRandomString(26) - if err := ad.IsValid(); err == nil || err.Id != "model.user_access_token.is_valid.token.app_error" { - t.Fatal(err) - } + err = ad.IsValid() + require.False(t, err == nil || err.Id != "model.user_access_token.is_valid.token.app_error") ad.Token = NewRandomString(26) - if err := ad.IsValid(); err == nil || err.Id != "model.user_access_token.is_valid.user_id.app_error" { - t.Fatal(err) - } + err = ad.IsValid() + require.False(t, err == nil || err.Id != "model.user_access_token.is_valid.user_id.app_error") ad.UserId = NewRandomString(26) - if err := ad.IsValid(); err != nil { - t.Fatal(err) - } + require.Nil(t, ad.IsValid()) ad.Description = NewRandomString(256) - if err := ad.IsValid(); err == nil || err.Id != "model.user_access_token.is_valid.description.app_error" { - t.Fatal(err) - } + err = ad.IsValid() + require.False(t, err == nil || err.Id != "model.user_access_token.is_valid.description.app_error") } From 467141ab69c7c6038897bf87c54cd92cf2671864 Mon Sep 17 00:00:00 2001 From: Michael Kochell Date: Sun, 13 Oct 2019 12:37:34 -0600 Subject: [PATCH 03/23] [MM-19004] Change assertion in client4/system_test.go to be less flaky (#12586) * avoid swallowing errors from websocket_client.SendMessage * Revert "avoid swallowing errors from websocket_client.SendMessage" This reverts commit 5f223e0996f59f31792607c8a4fbf63bf42040ee. * test against app.TotalWebsocketConnections instead of hardcoding assertion --- api4/system_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api4/system_test.go b/api4/system_test.go index 6e58b56413..25d5cdc174 100644 --- a/api4/system_test.go +++ b/api4/system_test.go @@ -383,7 +383,7 @@ func TestGetAnalyticsOld(t *testing.T) { rows2, resp2 = th.SystemAdminClient.GetAnalyticsOld("standard", "") CheckNoError(t, resp2) assert.Equal(t, "total_websocket_connections", rows2[5].Name) - assert.Equal(t, float64(1), rows2[5].Value) + assert.Equal(t, float64(th.App.TotalWebsocketConnections()), rows2[5].Value) WebSocketClient.Close() From 2f5949d05eb65324ef5de697ea6df631d35a4ad1 Mon Sep 17 00:00:00 2001 From: Vitalii Ananichev Date: Sun, 13 Oct 2019 22:37:41 +0300 Subject: [PATCH 04/23] Migrate tests from "model/cluster_stats_test.go" to use testify (#12580) --- model/cluster_stats_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/model/cluster_stats_test.go b/model/cluster_stats_test.go index 82bacf5263..7aea9a9295 100644 --- a/model/cluster_stats_test.go +++ b/model/cluster_stats_test.go @@ -6,6 +6,8 @@ package model import ( "strings" "testing" + + "github.com/stretchr/testify/require" ) func TestClusterStatsJson(t *testing.T) { @@ -13,7 +15,5 @@ func TestClusterStatsJson(t *testing.T) { json := cluster.ToJson() result := ClusterStatsFromJson(strings.NewReader(json)) - if cluster.Id != result.Id { - t.Fatal("Ids do not match") - } + require.Equal(t, cluster.Id, result.Id, "Ids do not match") } From 705090999b5e91b0989d3be04f959c565f0321e1 Mon Sep 17 00:00:00 2001 From: Mike Vanbuskirk Date: Sun, 13 Oct 2019 15:38:06 -0400 Subject: [PATCH 05/23] migrate t.Fatal to require.Fail (#12530) * migrate t.Fatal to require.Fail * remove nested if/else logic in tests, use bool * correct bool eval on plugin tests --- config/unmarshal_test.go | 185 ++++++++++++++++++--------------------- 1 file changed, 84 insertions(+), 101 deletions(-) diff --git a/config/unmarshal_test.go b/config/unmarshal_test.go index 66cd4ec3e9..6880d8d5a7 100644 --- a/config/unmarshal_test.go +++ b/config/unmarshal_test.go @@ -161,19 +161,17 @@ func TestConfigFromEnviroVars(t *testing.T) { assert.Equal(t, "From Environment", *cfg.TeamSettings.SiteName) assert.Equal(t, "Custom Brand", *cfg.TeamSettings.CustomBrandText) - if teamSettings, ok := envCfg["TeamSettings"]; !ok { - t.Fatal("TeamSettings is missing from envConfig") - } else if teamSettingsAsMap, ok := teamSettings.(map[string]interface{}); !ok { - t.Fatal("TeamSettings is not a map in envConfig") - } else { - if siteNameInEnv, ok := teamSettingsAsMap["SiteName"].(bool); !ok || !siteNameInEnv { - t.Fatal("SiteName should be in envConfig") - } + teamSettings, ok := envCfg["TeamSettings"] + require.True(t, ok, "TeamSettings is missing from envConfig") - if customBrandTextInEnv, ok := teamSettingsAsMap["CustomBrandText"].(bool); !ok || !customBrandTextInEnv { - t.Fatal("SiteName should be in envConfig") - } - } + teamSettingsAsMap, ok := teamSettings.(map[string]interface{}) + require.True(t, ok, "TeamSettings is not a map in envConfig") + + siteNameInEnv, ok := teamSettingsAsMap["SiteName"].(bool) + require.True(t, ok || siteNameInEnv, "SiteName should be in envConfig") + + customBrandTextInEnv, ok := teamSettingsAsMap["CustomBrandText"].(bool) + require.True(t, ok || customBrandTextInEnv, "SiteName should be in envConfig") os.Unsetenv("MM_TEAMSETTINGS_SITENAME") os.Unsetenv("MM_TEAMSETTINGS_CUSTOMBRANDTEXT") @@ -183,9 +181,8 @@ func TestConfigFromEnviroVars(t *testing.T) { assert.Equal(t, "Mattermost", *cfg.TeamSettings.SiteName) - if _, ok := envCfg["TeamSettings"]; ok { - t.Fatal("TeamSettings should be missing from envConfig") - } + _, ok = envCfg["TeamSettings"] + require.False(t, ok, "TeamSettings should be missing from envConfig") }) t.Run("boolean setting", func(t *testing.T) { @@ -195,19 +192,16 @@ func TestConfigFromEnviroVars(t *testing.T) { cfg, envCfg, err := unmarshalConfig(strings.NewReader(config), true) require.Nil(t, err) - if *cfg.ServiceSettings.EnableCommands { - t.Fatal("Couldn't read config from environment var") - } + require.False(t, *cfg.ServiceSettings.EnableCommands, "Couldn't read config from environment var") - if serviceSettings, ok := envCfg["ServiceSettings"]; !ok { - t.Fatal("ServiceSettings is missing from envConfig") - } else if serviceSettingsAsMap, ok := serviceSettings.(map[string]interface{}); !ok { - t.Fatal("ServiceSettings is not a map in envConfig") - } else { - if enableCommandsInEnv, ok := serviceSettingsAsMap["EnableCommands"].(bool); !ok || !enableCommandsInEnv { - t.Fatal("EnableCommands should be in envConfig") - } - } + serviceSettings, ok := envCfg["ServiceSettings"] + require.True(t, ok, "ServiceSettings is missing from envConfig") + + serviceSettingsAsMap, ok := serviceSettings.(map[string]interface{}) + require.True(t, ok, "ServiceSettings is not a map in envConfig") + + enableCommandsInEnv, ok := serviceSettingsAsMap["EnableCommands"].(bool) + require.True(t, ok || enableCommandsInEnv, "EnableCommands should be in envConfig") }) t.Run("integer setting", func(t *testing.T) { @@ -219,15 +213,14 @@ func TestConfigFromEnviroVars(t *testing.T) { assert.Equal(t, 400, *cfg.ServiceSettings.ReadTimeout) - if serviceSettings, ok := envCfg["ServiceSettings"]; !ok { - t.Fatal("ServiceSettings is missing from envConfig") - } else if serviceSettingsAsMap, ok := serviceSettings.(map[string]interface{}); !ok { - t.Fatal("ServiceSettings is not a map in envConfig") - } else { - if readTimeoutInEnv, ok := serviceSettingsAsMap["ReadTimeout"].(bool); !ok || !readTimeoutInEnv { - t.Fatal("ReadTimeout should be in envConfig") - } - } + serviceSettings, ok := envCfg["ServiceSettings"] + require.True(t, ok, "ServiceSettings is missing from envConfig") + + serviceSettingsAsMap, ok := serviceSettings.(map[string]interface{}) + require.True(t, ok, "ServiceSettings is not a map in envConfig") + + readTimeoutInEnv, ok := serviceSettingsAsMap["ReadTimeout"].(bool) + require.True(t, ok || readTimeoutInEnv, "ReadTimeout should be in envConfig") }) t.Run("setting missing from config.json", func(t *testing.T) { @@ -239,15 +232,14 @@ func TestConfigFromEnviroVars(t *testing.T) { assert.Equal(t, "https://example.com", *cfg.ServiceSettings.SiteURL) - if serviceSettings, ok := envCfg["ServiceSettings"]; !ok { - t.Fatal("ServiceSettings is missing from envConfig") - } else if serviceSettingsAsMap, ok := serviceSettings.(map[string]interface{}); !ok { - t.Fatal("ServiceSettings is not a map in envConfig") - } else { - if siteURLInEnv, ok := serviceSettingsAsMap["SiteURL"].(bool); !ok || !siteURLInEnv { - t.Fatal("SiteURL should be in envConfig") - } - } + serviceSettings, ok := envCfg["ServiceSettings"] + require.True(t, ok, "ServiceSettings is missing from envConfig") + + serviceSettingsAsMap, ok := serviceSettings.(map[string]interface{}) + require.True(t, ok, "ServiceSettings is not a map in envConfig") + + siteURLInEnv, ok := serviceSettingsAsMap["SiteURL"].(bool) + require.True(t, ok || siteURLInEnv, "SiteURL should be in envConfig") }) t.Run("empty string setting", func(t *testing.T) { @@ -259,15 +251,14 @@ func TestConfigFromEnviroVars(t *testing.T) { assert.Empty(t, *cfg.SupportSettings.TermsOfServiceLink) - if supportSettings, ok := envCfg["SupportSettings"]; !ok { - t.Fatal("SupportSettings is missing from envConfig") - } else if supportSettingsAsMap, ok := supportSettings.(map[string]interface{}); !ok { - t.Fatal("SupportSettings is not a map in envConfig") - } else { - if termsOfServiceLinkInEnv, ok := supportSettingsAsMap["TermsOfServiceLink"].(bool); !ok || !termsOfServiceLinkInEnv { - t.Fatal("TermsOfServiceLink should be in envConfig") - } - } + supportSettings, ok := envCfg["SupportSettings"] + require.True(t, ok, "SupportSettings is missing from envConfig") + + supportSettingsAsMap, ok := supportSettings.(map[string]interface{}) + require.True(t, ok, "SupportSettings is not a map in envConfig") + + termsOfServiceLinkInEnv, ok := supportSettingsAsMap["TermsOfServiceLink"].(bool) + require.True(t, ok || termsOfServiceLinkInEnv, "TermsOfServiceLink should be in envConfig") }) t.Run("plugin directory settings", func(t *testing.T) { @@ -285,18 +276,17 @@ func TestConfigFromEnviroVars(t *testing.T) { assert.Equal(t, "/temp/plugins", *cfg.PluginSettings.Directory) assert.Equal(t, "/temp/clientplugins", *cfg.PluginSettings.ClientDirectory) - if pluginSettings, ok := envCfg["PluginSettings"]; !ok { - t.Fatal("PluginSettings is missing from envConfig") - } else if pluginSettingsAsMap, ok := pluginSettings.(map[string]interface{}); !ok { - t.Fatal("PluginSettings is not a map in envConfig") - } else { - if directory, ok := pluginSettingsAsMap["Directory"].(bool); !ok || !directory { - t.Fatal("Directory should be in envConfig") - } - if clientDirectory, ok := pluginSettingsAsMap["ClientDirectory"].(bool); !ok || !clientDirectory { - t.Fatal("ClientDirectory should be in envConfig") - } - } + pluginSettings, ok := envCfg["PluginSettings"] + require.True(t, ok, "PluginSettings is missing from envConfig") + + pluginSettingsAsMap, ok := pluginSettings.(map[string]interface{}) + require.True(t, ok, "PluginSettings is not a map in envConfig") + + directory, ok := pluginSettingsAsMap["Directory"].(bool) + require.True(t, ok || directory, "Directory should be in envConfig") + + clientDirectory, ok := pluginSettingsAsMap["ClientDirectory"].(bool) + require.True(t, ok || clientDirectory, "ClientDirectory should be in envConfig") }) t.Run("plugin specific settings cannot be overridden via environment", func(t *testing.T) { @@ -310,45 +300,38 @@ func TestConfigFromEnviroVars(t *testing.T) { cfg, envCfg, err := unmarshalConfig(strings.NewReader(config), true) require.Nil(t, err) - if pluginsJira, ok := cfg.PluginSettings.Plugins["jira"]; !ok { - t.Fatal("PluginSettings.Plugins.jira is missing from config") - } else { - if enabled, ok := pluginsJira["enabled"]; !ok { - t.Fatal("PluginSettings.Plugins.jira.enabled is missing from config") - } else { - assert.Equal(t, "true", enabled) - } + pluginsJira, ok := cfg.PluginSettings.Plugins["jira"] + require.True(t, ok, "PluginSettings.Plugins.jira is missing from config") - if secret, ok := pluginsJira["secret"]; !ok { - t.Fatal("PluginSettings.Plugins.jira.secret is missing from config") - } else { - assert.Equal(t, "config-secret", secret) - } - } + enabled, ok := pluginsJira["enabled"] + require.True(t, ok, "PluginSettings.Plugins.jira.enabled is missing from config") + assert.Equal(t, "true", enabled) - if pluginStatesJira, ok := cfg.PluginSettings.PluginStates["jira"]; !ok { - t.Fatal("PluginSettings.PluginStates.jira is missing from config") - } else { - require.Equal(t, true, pluginStatesJira.Enable) - } + secret, ok := pluginsJira["secret"] + require.True(t, ok, "PluginSettings.Plugins.jira.secret is missing from config") + assert.Equal(t, "config-secret", secret) - if pluginSettings, ok := envCfg["PluginSettings"]; !ok { - t.Fatal("PluginSettings is missing from envConfig") - } else if pluginSettingsAsMap, ok := pluginSettings.(map[string]interface{}); !ok { - t.Fatal("PluginSettings is not a map in envConfig") - } else { - if plugins, ok := pluginSettingsAsMap["Plugins"].(map[string]interface{}); !ok { - t.Fatal("PluginSettings.Plugins is not a map in envConfig") - } else if _, ok := plugins["jira"].(map[string]interface{}); ok { - t.Fatal("PluginSettings.Plugins.jira should not be a map in envConfig") - } + pluginStatesJira, ok := cfg.PluginSettings.PluginStates["jira"] + require.True(t, ok, "PluginSettings.PluginStates.jira is missing from config") + require.Equal(t, true, pluginStatesJira.Enable) - if pluginStates, ok := pluginSettingsAsMap["PluginStates"].(map[string]interface{}); !ok { - t.Fatal("PluginSettings.PluginStates is missing from envConfig") - } else if _, ok := pluginStates["jira"].(map[string]interface{}); ok { - t.Fatal("PluginSettings.PluginStates.jira should not be a map in envConfig") - } - } + pluginSettings, ok := envCfg["PluginSettings"] + require.True(t, ok, "PluginSettings is missing from envConfig") + + pluginSettingsAsMap, ok := pluginSettings.(map[string]interface{}) + require.True(t, ok, "PluginSettings is not a map in envConfig") + + plugins, ok := pluginSettingsAsMap["Plugins"].(map[string]interface{}) + require.True(t, ok, "PluginSettings.Plugins is not a map in envConfig") + + _, ok = plugins["jira"].(map[string]interface{}) + require.False(t, ok, "PluginSettings.Plugins.jira should not be a map in envConfig") + + pluginStates, ok := pluginSettingsAsMap["PluginStates"].(map[string]interface{}) + require.True(t, ok, "PluginSettings.PluginStates is missing from envConfig") + + _, ok = pluginStates["jira"].(map[string]interface{}) + require.False(t, ok, "PluginSettings.PluginStates.jira should not be a map in envConfig") }) } From babbe087ff44b8ba4302403acfe9795ef11ba3f5 Mon Sep 17 00:00:00 2001 From: Jairo Junior Date: Sun, 13 Oct 2019 16:38:28 -0300 Subject: [PATCH 06/23] Convert app/import_test.go t.Fatal calls into assert/require calls (#12228) --- app/import_test.go | 158 +++++++++++++++++---------------------------- 1 file changed, 58 insertions(+), 100 deletions(-) diff --git a/app/import_test.go b/app/import_test.go index 5bb4c1097d..c06aeb4e84 100644 --- a/app/import_test.go +++ b/app/import_test.go @@ -6,7 +6,6 @@ package app import ( "net/http" "path/filepath" - "runtime/debug" "strings" "testing" @@ -34,73 +33,43 @@ func ptrBool(b bool) *bool { } func checkPreference(t *testing.T, a *App, userId string, category string, name string, value string) { - if preferences, err := a.Srv.Store.Preference().GetCategory(userId, category); err != nil { - debug.PrintStack() - t.Fatalf("Failed to get preferences for user %v with category %v", userId, category) - } else { - found := false - for _, preference := range preferences { - if preference.Name == name { - found = true - if preference.Value != value { - debug.PrintStack() - t.Fatalf("Preference for user %v in category %v with name %v has value %v, expected %v", userId, category, name, preference.Value, value) - } - break - } - } - if !found { - debug.PrintStack() - t.Fatalf("Did not find preference for user %v in category %v with name %v", userId, category, name) + preferences, err := a.Srv.Store.Preference().GetCategory(userId, category) + require.Nilf(t, err, "Failed to get preferences for user %v with category %v", userId, category) + found := false + for _, preference := range preferences { + if preference.Name == name { + found = true + require.Equal(t, preference.Value, value, "Preference for user %v in category %v with name %v has value %v, expected %v", userId, category, name, preference.Value, value) + break } } + require.Truef(t, found, "Did not find preference for user %v in category %v with name %v", userId, category, name) } func checkNotifyProp(t *testing.T, user *model.User, key string, value string) { - if actual, ok := user.NotifyProps[key]; !ok { - debug.PrintStack() - t.Fatalf("Notify prop %v not found. User: %v", key, user.Id) - } else if actual != value { - debug.PrintStack() - t.Fatalf("Notify Prop %v was %v but expected %v. User: %v", key, actual, value, user.Id) - } + actual, ok := user.NotifyProps[key] + require.True(t, ok, "Notify prop %v not found. User: %v", key, user.Id) + require.Equalf(t, actual, value, "Notify Prop %v was %v but expected %v. User: %v", key, actual, value, user.Id) } func checkError(t *testing.T, err *model.AppError) { - if err == nil { - debug.PrintStack() - t.Fatal("Should have returned an error.") - } + require.NotNil(t, err, "Should have returned an error.") } func checkNoError(t *testing.T, err *model.AppError) { - if err != nil { - debug.PrintStack() - t.Fatalf("Unexpected Error: %v", err.Error()) - } + require.Nil(t, err, "Unexpected Error: %v", err) } func AssertAllPostsCount(t *testing.T, a *App, initialCount int64, change int64, teamName string) { - if result, err := a.Srv.Store.Post().AnalyticsPostCount(teamName, false, false); err != nil { - t.Fatal(err) - } else { - if initialCount+change != result { - debug.PrintStack() - t.Fatalf("Did not find the expected number of posts.") - } - } + result, err := a.Srv.Store.Post().AnalyticsPostCount(teamName, false, false) + require.Nil(t, err) + require.Equal(t, initialCount+change, result, "Did not find the expected number of posts.") } func AssertChannelCount(t *testing.T, a *App, channelType string, expectedCount int64) { - if count, err := a.Srv.Store.Channel().AnalyticsTypeCount("", channelType); err == nil { - if count != expectedCount { - debug.PrintStack() - t.Fatalf("Channel count of type: %v. Expected: %v, Got: %v", channelType, expectedCount, count) - } - } else { - debug.PrintStack() - t.Fatalf("Failed to get channel count.") - } + count, err := a.Srv.Store.Channel().AnalyticsTypeCount("", channelType) + require.Equalf(t, expectedCount, count, "Channel count of type: %v. Expected: %v, Got: %v", channelType, expectedCount, count) + require.Nil(t, err, "Failed to get channel count.") } func TestImportImportLine(t *testing.T) { @@ -112,51 +81,43 @@ func TestImportImportLine(t *testing.T) { Type: "gibberish", } - if err := th.App.ImportLine(line, false); err == nil { - t.Fatalf("Expected an error when importing a line with invalid type.") - } + err := th.App.ImportLine(line, false) + require.NotNil(t, err, "Expected an error when importing a line with invalid type.") // Try import line with team type but nil team. line.Type = "team" - if err := th.App.ImportLine(line, false); err == nil { - t.Fatalf("Expected an error when importing a line of type team with a nil team.") - } + err = th.App.ImportLine(line, false) + require.NotNil(t, err, "Expected an error when importing a line of type team with a nil team.") // Try import line with channel type but nil channel. line.Type = "channel" - if err := th.App.ImportLine(line, false); err == nil { - t.Fatalf("Expected an error when importing a line with type channel with a nil channel.") - } + err = th.App.ImportLine(line, false) + require.NotNil(t, err, "Expected an error when importing a line with type channel with a nil channel.") // Try import line with user type but nil user. line.Type = "user" - if err := th.App.ImportLine(line, false); err == nil { - t.Fatalf("Expected an error when importing a line with type uesr with a nil user.") - } + err = th.App.ImportLine(line, false) + require.NotNil(t, err, "Expected an error when importing a line with type user with a nil user.") // Try import line with post type but nil post. line.Type = "post" - if err := th.App.ImportLine(line, false); err == nil { - t.Fatalf("Expected an error when importing a line with type post with a nil post.") - } + err = th.App.ImportLine(line, false) + require.NotNil(t, err, "Expected an error when importing a line with type post with a nil post.") // Try import line with direct_channel type but nil direct_channel. line.Type = "direct_channel" - if err := th.App.ImportLine(line, false); err == nil { - t.Fatalf("Expected an error when importing a line with type direct_channel with a nil direct_channel.") - } + err = th.App.ImportLine(line, false) + require.NotNil(t, err, "Expected an error when importing a line with type direct_channel with a nil direct_channel.") // Try import line with direct_post type but nil direct_post. line.Type = "direct_post" - if err := th.App.ImportLine(line, false); err == nil { - t.Fatalf("Expected an error when importing a line with type direct_post with a nil direct_post.") - } + err = th.App.ImportLine(line, false) + require.NotNil(t, err, "Expected an error when importing a line with type direct_post with a nil direct_post.") // Try import line with scheme type but nil scheme. line.Type = "scheme" - if err := th.App.ImportLine(line, false); err == nil { - t.Fatalf("Expected an error when importing a line with type scheme with a nil scheme.") - } + err = th.App.ImportLine(line, false) + require.NotNil(t, err, "Expected an error when importing a line with type scheme with a nil scheme.") } func TestStopOnError(t *testing.T) { @@ -208,24 +169,24 @@ func TestImportBulkImport(t *testing.T) { {"type": "direct_post", "direct_post": {"channel_members": ["` + username + `", "` + username2 + `", "` + username3 + `"], "user": "` + username + `", "message": "Hello Group Channel", "create_at": 123456789015}} {"type": "emoji", "emoji": {"name": "` + emojiName + `", "image": "` + testImage + `"}}` - if err, line := th.App.BulkImport(strings.NewReader(data1), false, 2); err != nil || line != 0 { - t.Fatalf("BulkImport should have succeeded: %v, %v", err.Error(), line) - } + err, line := th.App.BulkImport(strings.NewReader(data1), false, 2) + require.Nil(t, err, "BulkImport should have succeeded") + require.Equal(t, 0, line, "BulkImport line should be 0") // Run bulk import using a string that contains a line with invalid json. data2 := `{"type": "version", "version": 1` - if err, line := th.App.BulkImport(strings.NewReader(data2), false, 2); err == nil || line != 1 { - t.Fatalf("Should have failed due to invalid JSON on line 1.") - } + err, line = th.App.BulkImport(strings.NewReader(data2), false, 2) + require.NotNil(t, err, "Should have failed due to invalid JSON on line 1.") + require.Equal(t, 1, line, "Should have failed due to invalid JSON on line 1.") // Run bulk import using valid JSON but missing version line at the start. data3 := `{"type": "team", "team": {"type": "O", "display_name": "lskmw2d7a5ao7ppwqh5ljchvr4", "name": "` + teamName + `"}} {"type": "channel", "channel": {"type": "O", "display_name": "xr6m6udffngark2uekvr3hoeny", "team": "` + teamName + `", "name": "` + channelName + `"}} {"type": "user", "user": {"username": "kufjgnkxkrhhfgbrip6qxkfsaa", "email": "kufjgnkxkrhhfgbrip6qxkfsaa@example.com"}} {"type": "user", "user": {"username": "bwshaim6qnc2ne7oqkd5b2s2rq", "email": "bwshaim6qnc2ne7oqkd5b2s2rq@example.com", "teams": [{"name": "` + teamName + `", "channels": [{"name": "` + channelName + `"}]}]}}` - if err, line := th.App.BulkImport(strings.NewReader(data3), false, 2); err == nil || line != 1 { - t.Fatalf("Should have failed due to missing version line on line 1.") - } + err, line = th.App.BulkImport(strings.NewReader(data3), false, 2) + require.NotNil(t, err, "Should have failed due to missing version line on line 1.") + require.Equal(t, 1, line, "Should have failed due to missing version line on line 1.") t.Run("First item after version without type", func(t *testing.T) { data := `{"type": "version", "version": 1} @@ -241,20 +202,18 @@ func TestImportProcessImportDataFileVersionLine(t *testing.T) { Type: "version", Version: ptrInt(1), } - if version, err := processImportDataFileVersionLine(data); err != nil || version != 1 { - t.Fatalf("Expected no error and version 1.") - } + version, err := processImportDataFileVersionLine(data) + require.Nil(t, err, "Expected no error") + require.Equal(t, 1, version, "Expected version 1") data.Type = "NotVersion" - if _, err := processImportDataFileVersionLine(data); err == nil { - t.Fatalf("Expected error on invalid version line.") - } + _, err = processImportDataFileVersionLine(data) + require.NotNil(t, err, "Expected error on invalid version line.") data.Type = "version" data.Version = nil - if _, err := processImportDataFileVersionLine(data); err == nil { - t.Fatalf("Expected error on invalid version line.") - } + _, err = processImportDataFileVersionLine(data) + require.NotNil(t, err, "Expected error on invalid version line.") } func GetAttachments(userId string, th *TestHelper, t *testing.T) []*model.FileInfo { @@ -267,12 +226,11 @@ func AssertFileIdsInPost(files []*model.FileInfo, th *TestHelper, t *testing.T) postId := files[0].PostId assert.NotNil(t, postId) - if posts, err := th.App.Srv.Store.Post().GetPostsByIds([]string{postId}); err != nil { - t.Fatal(err.Error()) - } else { - assert.Equal(t, len(posts), 1) - for _, file := range files { - assert.Contains(t, posts[0].FileIds, file.Id) - } + posts, err := th.App.Srv.Store.Post().GetPostsByIds([]string{postId}) + require.Nil(t, err) + + assert.Equal(t, len(posts), 1) + for _, file := range files { + assert.Contains(t, posts[0].FileIds, file.Id) } } From b860355f30b657cae8adf4a2f6ea15228d0a2fbf Mon Sep 17 00:00:00 2001 From: George Felix Date: Sun, 13 Oct 2019 21:38:58 +0200 Subject: [PATCH 07/23] Convert model/channel_test.go validation calls into assert/require calls (#12605) * Convert model/channel_test.go t.Fatal calls into assert/require calls * Removed blank spaces * Changes based on suggestions coming from related task https://github.com/mattermost/mattermost-server/pull/12602#pullrequestreview-296859312 --- model/channel_test.go | 100 +++++++++++------------------------------- 1 file changed, 26 insertions(+), 74 deletions(-) diff --git a/model/channel_test.go b/model/channel_test.go index 2a47cae0d2..1d2097ecfa 100644 --- a/model/channel_test.go +++ b/model/channel_test.go @@ -6,6 +6,8 @@ package model import ( "strings" "testing" + + "github.com/stretchr/testify/require" ) func TestChannelJson(t *testing.T) { @@ -13,27 +15,21 @@ func TestChannelJson(t *testing.T) { json := o.ToJson() ro := ChannelFromJson(strings.NewReader(json)) - if o.Id != ro.Id { - t.Fatal("Ids do not match") - } + require.Equal(t, o.Id, ro.Id) p := ChannelPatch{Name: new(string)} *p.Name = NewId() json = p.ToJson() rp := ChannelPatchFromJson(strings.NewReader(json)) - if *p.Name != *rp.Name { - t.Fatal("names do not match") - } + require.Equal(t, *p.Name, *rp.Name) } func TestChannelCopy(t *testing.T) { o := Channel{Id: NewId(), Name: NewId()} ro := o.DeepCopy() - if o.Id != ro.Id { - t.Fatal("Ids do not match") - } + require.Equal(t, o.Id, ro.Id, "Ids do not match") } func TestChannelPatch(t *testing.T) { @@ -47,97 +43,57 @@ func TestChannelPatch(t *testing.T) { o := Channel{Id: NewId(), Name: NewId()} o.Patch(p) - if *p.Name != o.Name { - t.Fatal("do not match") - } - if *p.DisplayName != o.DisplayName { - t.Fatal("do not match") - } - if *p.Header != o.Header { - t.Fatal("do not match") - } - if *p.Purpose != o.Purpose { - t.Fatal("do not match") - } - if *p.GroupConstrained != *o.GroupConstrained { - t.Fatalf("expected %v got %v", *p.GroupConstrained, *o.GroupConstrained) - } + require.Equal(t, *p.Name, o.Name) + require.Equal(t, *p.DisplayName, o.DisplayName) + require.Equal(t, *p.Header, o.Header) + require.Equal(t, *p.Purpose, o.Purpose) + require.Equal(t, *p.GroupConstrained, *o.GroupConstrained) } func TestChannelIsValid(t *testing.T) { o := Channel{} - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.Id = NewId() - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.CreateAt = GetMillis() - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.UpdateAt = GetMillis() - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.DisplayName = strings.Repeat("01234567890", 20) - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.DisplayName = "1234" o.Name = "ZZZZZZZ" - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.Name = "zzzzz" - - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.Type = "U" - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.Type = "P" - - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + require.Error(t, o.IsValid()) o.Header = strings.Repeat("01234567890", 100) - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.Header = "1234" - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + require.Nil(t, o.IsValid()) o.Purpose = strings.Repeat("01234567890", 30) - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.Purpose = "1234" - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + require.Nil(t, o.IsValid()) o.Purpose = strings.Repeat("0123456789", 25) - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + require.Nil(t, o.IsValid()) } func TestChannelPreSave(t *testing.T) { @@ -159,15 +115,11 @@ func TestGetGroupDisplayNameFromUsers(t *testing.T) { users[3] = &User{Username: NewId()} name := GetGroupDisplayNameFromUsers(users, true) - if len(name) > CHANNEL_NAME_MAX_LENGTH { - t.Fatal("name too long") - } + require.LessOrEqual(t, len(name), CHANNEL_NAME_MAX_LENGTH) } func TestGetGroupNameFromUserIds(t *testing.T) { name := GetGroupNameFromUserIds([]string{NewId(), NewId(), NewId(), NewId(), NewId()}) - if len(name) > CHANNEL_NAME_MAX_LENGTH { - t.Fatal("name too long") - } + require.LessOrEqual(t, len(name), CHANNEL_NAME_MAX_LENGTH) } From 6f4f06f8d4a586bf3deaabb8eaad9dde3fbe7465 Mon Sep 17 00:00:00 2001 From: Shodiq Muhammad <47969743+iDevoid@users.noreply.github.com> Date: Mon, 14 Oct 2019 02:39:30 +0700 Subject: [PATCH 08/23] change t.Fatal to assertions in model/outgoing_webhook_test.go (#12693) * change t.Fatal to assertions in model/outgoing_webhook_test.go * user assert NotNilf and Nilf, remove backticks --- model/outgoing_webhook_test.go | 122 +++++++++------------------------ 1 file changed, 33 insertions(+), 89 deletions(-) diff --git a/model/outgoing_webhook_test.go b/model/outgoing_webhook_test.go index bf61a67a78..16b05246b2 100644 --- a/model/outgoing_webhook_test.go +++ b/model/outgoing_webhook_test.go @@ -5,9 +5,10 @@ package model import ( "net/url" - "reflect" "strings" "testing" + + "github.com/stretchr/testify/assert" ) func TestOutgoingWebhookJson(t *testing.T) { @@ -15,132 +16,81 @@ func TestOutgoingWebhookJson(t *testing.T) { json := o.ToJson() ro := OutgoingWebhookFromJson(strings.NewReader(json)) - if o.Id != ro.Id { - t.Fatal("Ids do not match") - } + assert.Equal(t, o.Id, ro.Id, "Ids do not match") } func TestOutgoingWebhookIsValid(t *testing.T) { o := OutgoingWebhook{} - - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNil(t, o.IsValid(), "empty declaration should be invalid") o.Id = NewId() - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNilf(t, o.IsValid(), "Id = NewId; %s should be invalid", o.Id) o.CreateAt = GetMillis() - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNilf(t, o.IsValid(), "CreateAt = GetMillis; %d should be invalid", o.CreateAt) o.UpdateAt = GetMillis() - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNilf(t, o.IsValid(), "UpdateAt = GetMillis; %d should be invalid", o.UpdateAt) o.CreatorId = "123" - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNilf(t, o.IsValid(), "CreatorId %s should be invalid", o.CreatorId) o.CreatorId = NewId() - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNilf(t, o.IsValid(), "CreatorId = NewId; %s should be invalid", o.CreatorId) o.Token = "123" - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNilf(t, o.IsValid(), "Token %s should be invalid", o.Token) o.Token = NewId() - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNilf(t, o.IsValid(), "Token = NewId; %s should be invalid", o.Token) o.ChannelId = "123" - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNilf(t, o.IsValid(), "ChannelId %s should be invalid", o.ChannelId) o.ChannelId = NewId() - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNilf(t, o.IsValid(), "ChannelId = NewId; %s should be invalid", o.ChannelId) o.TeamId = "123" - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNilf(t, o.IsValid(), "TeamId %s should be invalid", o.TeamId) o.TeamId = NewId() - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNilf(t, o.IsValid(), "TeamId = NewId; %s should be invalid", o.TeamId) o.CallbackURLs = []string{"nowhere.com/"} - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNilf(t, o.IsValid(), "%v for CallbackURLs should be invalid", o.CallbackURLs) o.CallbackURLs = []string{"http://nowhere.com/"} - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + assert.Nilf(t, o.IsValid(), "%v for CallbackURLs should be valid", o.CallbackURLs) o.DisplayName = strings.Repeat("1", 65) - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNilf(t, o.IsValid(), "DisplayName length %d invalid, max length 64", len(o.DisplayName)) o.DisplayName = strings.Repeat("1", 64) - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + assert.Nilf(t, o.IsValid(), "DisplayName length %d should be valid, max length 64", len(o.DisplayName)) o.Description = strings.Repeat("1", 501) - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNilf(t, o.IsValid(), "Description length %d should be invalid, max length 500", len(o.Description)) o.Description = strings.Repeat("1", 500) - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + assert.Nilf(t, o.IsValid(), "Description length %d should be valid, max length 500", len(o.Description)) o.ContentType = strings.Repeat("1", 129) - if err := o.IsValid(); err == nil { - t.Fatal(err) - } + assert.NotNilf(t, o.IsValid(), "ContentType length %d should be invalid, max length 128", len(o.ContentType)) o.ContentType = strings.Repeat("1", 128) - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + assert.Nilf(t, o.IsValid(), "ContentType length %d should be valid", len(o.ContentType)) o.Username = strings.Repeat("1", 65) - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.NotNilf(t, o.IsValid(), "Username length %d should be invalid, max length 64", len(o.Username)) o.Username = strings.Repeat("1", 64) - if err := o.IsValid(); err != nil { - t.Fatal("should be invalid") - } + assert.Nilf(t, o.IsValid(), "Username length %d should be valid", len(o.Username)) o.IconURL = strings.Repeat("1", 1025) - if err := o.IsValid(); err == nil { - t.Fatal(err) - } + assert.NotNilf(t, o.IsValid(), "IconURL length %d should be invalid, max length 1024", len(o.IconURL)) o.IconURL = strings.Repeat("1", 1024) - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + assert.Nilf(t, o.IsValid(), "IconURL length %d should be valid", len(o.IconURL)) } func TestOutgoingWebhookPayloadToFormValues(t *testing.T) { @@ -171,9 +121,9 @@ func TestOutgoingWebhookPayloadToFormValues(t *testing.T) { v.Set("text", "Text") v.Set("trigger_word", "TriggerWord") v.Set("file_ids", "FileIds") - if got, want := p.ToFormValues(), v.Encode(); !reflect.DeepEqual(got, want) { - t.Fatalf("Got %+v, wanted %+v", got, want) - } + got := p.ToFormValues() + want := v.Encode() + assert.Equalf(t, got, want, "Got %+v, wanted %+v", got, want) } func TestOutgoingWebhookPreSave(t *testing.T) { @@ -189,12 +139,8 @@ func TestOutgoingWebhookPreUpdate(t *testing.T) { func TestOutgoingWebhookTriggerWordStartsWith(t *testing.T) { o := OutgoingWebhook{Id: NewId()} o.TriggerWords = append(o.TriggerWords, "foo") - if !o.TriggerWordStartsWith("foobar") { - t.Fatal("Should return true") - } - if o.TriggerWordStartsWith("barfoo") { - t.Fatal("Should return false") - } + assert.True(t, o.TriggerWordStartsWith("foobar"), "Should return true") + assert.False(t, o.TriggerWordStartsWith("barfoo"), "Should return false") } func TestOutgoingWebhookResponseJson(t *testing.T) { @@ -204,7 +150,5 @@ func TestOutgoingWebhookResponseJson(t *testing.T) { json := o.ToJson() ro, _ := OutgoingWebhookResponseFromJson(strings.NewReader(json)) - if *o.Text != *ro.Text { - t.Fatal("Text does not match") - } + assert.Equal(t, *o.Text, *ro.Text, "Text does not match") } From c5dcd85bc8720f71d462d7f33557a197d8f09668 Mon Sep 17 00:00:00 2001 From: Nikhil Ranjan Date: Mon, 14 Oct 2019 12:55:50 +0200 Subject: [PATCH 09/23] Converting to structured logging the file store/sqlstore/upgrade.go (#12628) * Converting to structured logging the file store/sqlstore/upgrade.go * changes as per review --- store/sqlstore/upgrade.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/store/sqlstore/upgrade.go b/store/sqlstore/upgrade.go index 1247e3cc78..d645282b95 100644 --- a/store/sqlstore/upgrade.go +++ b/store/sqlstore/upgrade.go @@ -6,7 +6,6 @@ package sqlstore import ( "database/sql" "encoding/json" - "fmt" "os" "strings" "time" @@ -106,7 +105,7 @@ func UpgradeDatabase(sqlStore SqlStore, currentModelVersionString string) error } currentSchemaVersion = ¤tModelVersion - mlog.Info(fmt.Sprintf("The database schema has been set to version %s", *currentSchemaVersion)) + mlog.Info("The database schema version has been set", mlog.String("version", currentSchemaVersion.String())) return nil } @@ -119,7 +118,7 @@ func UpgradeDatabase(sqlStore SqlStore, currentModelVersionString string) error if currentSchemaVersion.GTE(nextUnsupportedMajorVersion) { return errors.Errorf("Database schema version %s is not supported. This Mattermost server supports only >=%s, <%s. Please upgrade to at least version %s before continuing.", *currentSchemaVersion, currentModelVersion, nextUnsupportedMajorVersion, nextUnsupportedMajorVersion) } else if currentSchemaVersion.GT(currentModelVersion) { - mlog.Warn(fmt.Sprintf("The database schema with version %s is newer than Mattermost version %s.", currentSchemaVersion, currentModelVersion)) + mlog.Warn("The database schema version and model versions do not match", mlog.String("schema_version", currentSchemaVersion.String()), mlog.String("model_version", currentModelVersion.String())) } // Otherwise, apply any necessary migrations. Note that these methods currently invoke @@ -176,12 +175,12 @@ func saveSchemaVersion(sqlStore SqlStore, version string) { os.Exit(EXIT_VERSION_SAVE) } - mlog.Warn(fmt.Sprintf("The database schema has been upgraded to version %v", version)) + mlog.Warn("The database schema version has been upgraded", mlog.String("version", version)) } func shouldPerformUpgrade(sqlStore SqlStore, currentSchemaVersion string, expectedSchemaVersion string) bool { if sqlStore.GetCurrentSchemaVersion() == currentSchemaVersion { - mlog.Warn(fmt.Sprintf("Attempting to upgrade the database schema version from %s to %v", currentSchemaVersion, expectedSchemaVersion)) + mlog.Warn("Attempting to upgrade the database schema version", mlog.String("current_version", currentSchemaVersion), mlog.String("new_version", expectedSchemaVersion)) return true } @@ -205,7 +204,7 @@ func UpgradeDatabaseToVersion32(sqlStore SqlStore) { } func themeMigrationFailed(err error) { - mlog.Critical(fmt.Sprintf("Failed to migrate User.ThemeProps to Preferences table %v", err)) + mlog.Critical("Failed to migrate User.ThemeProps to Preferences table", mlog.Err(err)) time.Sleep(time.Second) os.Exit(EXIT_THEME_MIGRATION) } @@ -479,7 +478,7 @@ func UpgradeDatabaseToVersion49(sqlStore SqlStore) { defaultTimezone := timezones.DefaultUserTimezone() defaultTimezoneValue, err := json.Marshal(defaultTimezone) if err != nil { - mlog.Critical(fmt.Sprint(err)) + mlog.Critical(err.Error()) } sqlStore.CreateColumnIfNotExists("Users", "Timezone", "varchar(256)", "varchar(256)", string(defaultTimezoneValue)) sqlStore.RemoveIndexIfExists("idx_channels_displayname", "Channels") @@ -673,12 +672,12 @@ func UpgradeDatabaseToVersion511(sqlStore SqlStore) { // Enforce all teams have an InviteID set var teams []*model.Team if _, err := sqlStore.GetReplica().Select(&teams, "SELECT * FROM Teams WHERE InviteId = ''"); err != nil { - mlog.Error("Error fetching Teams without InviteID: " + err.Error()) + mlog.Error("Error fetching Teams without InviteID", mlog.Err(err)) } else { for _, team := range teams { team.InviteId = model.NewId() if _, err := sqlStore.Team().Update(team); err != nil { - mlog.Error("Error updating Team InviteIDs: " + err.Error()) + mlog.Error("Error updating Team InviteIDs", mlog.String("team_id", team.Id), mlog.Err(err)) } } } From 06866952b6134fc46347d8f86eb01464dc8fc4f2 Mon Sep 17 00:00:00 2001 From: Scott Bishel Date: Mon, 14 Oct 2019 11:38:17 -0600 Subject: [PATCH 10/23] MM-16861: Support Guest Authentication via AD/LDAP (#12690) * Add config settings for LDAPSettings GuestFilter * make error unique * Update model/config.go Co-Authored-By: Martin Kraft * add LdapSetting isempty_guest_attribute to diagnostics.go --- app/diagnostics.go | 1 + i18n/en.json | 4 ++ model/config.go | 11 +++++ model/config_test.go | 99 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 115 insertions(+) diff --git a/app/diagnostics.go b/app/diagnostics.go index 863af16592..ac7203156d 100644 --- a/app/diagnostics.go +++ b/app/diagnostics.go @@ -487,6 +487,7 @@ func (a *App) trackConfig() { "isempty_group_filter": isDefault(*cfg.LdapSettings.GroupFilter, ""), "isdefault_group_display_name_attribute": isDefault(*cfg.LdapSettings.GroupDisplayNameAttribute, model.LDAP_SETTINGS_DEFAULT_GROUP_DISPLAY_NAME_ATTRIBUTE), "isdefault_group_id_attribute": isDefault(*cfg.LdapSettings.GroupIdAttribute, model.LDAP_SETTINGS_DEFAULT_GROUP_ID_ATTRIBUTE), + "isempty_guest_filter": isDefault(*cfg.LdapSettings.GuestFilter, ""), }) a.SendDiagnostic(TRACK_CONFIG_COMPLIANCE, map[string]interface{}{ diff --git a/i18n/en.json b/i18n/en.json index f8841befc4..fa03c92154 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -3954,6 +3954,10 @@ "id": "ent.ldap.validate_filter.app_error", "translation": "Invalid AD/LDAP Filter" }, + { + "id": "ent.ldap.validate_guest_filter.app_error", + "translation": "Invalid AD/LDAP Guest Filter" + }, { "id": "ent.ldap_groups.group_search_error", "translation": "error retrieving ldap group" diff --git a/model/config.go b/model/config.go index 016477a3ab..ce7a4766e4 100644 --- a/model/config.go +++ b/model/config.go @@ -1687,6 +1687,7 @@ type LdapSettings struct { // Filtering UserFilter *string GroupFilter *string + GuestFilter *string // Group Mapping GroupDisplayNameAttribute *string @@ -1758,6 +1759,10 @@ func (s *LdapSettings) SetDefaults() { s.UserFilter = NewString("") } + if s.GuestFilter == nil { + s.GuestFilter = NewString("") + } + if s.GroupFilter == nil { s.GroupFilter = NewString("") } @@ -2784,6 +2789,12 @@ func (ls *LdapSettings) isValid() *AppError { return NewAppError("ValidateFilter", "ent.ldap.validate_filter.app_error", nil, err.Error(), http.StatusBadRequest) } } + + if *ls.GuestFilter != "" { + if _, err := ldap.CompileFilter(*ls.GuestFilter); err != nil { + return NewAppError("LdapSettings.isValid", "ent.ldap.validate_guest_filter.app_error", nil, err.Error(), http.StatusBadRequest) + } + } } return nil diff --git a/model/config_test.go b/model/config_test.go index 743b6a58ba..990ffa13dd 100644 --- a/model/config_test.go +++ b/model/config_test.go @@ -1018,6 +1018,105 @@ func TestLdapSettingsIsValid(t *testing.T) { }, ExpectError: true, }, + + { + Name: "valid guest filter #1", + LdapSettings: LdapSettings{ + Enable: NewBool(true), + LdapServer: NewString("server"), + BaseDN: NewString("basedn"), + EmailAttribute: NewString("email"), + UsernameAttribute: NewString("username"), + IdAttribute: NewString("id"), + LoginIdAttribute: NewString("loginid"), + GuestFilter: NewString("(property=value)"), + }, + ExpectError: false, + }, + { + Name: "invalid guest filter #1", + LdapSettings: LdapSettings{ + Enable: NewBool(true), + LdapServer: NewString("server"), + BaseDN: NewString("basedn"), + EmailAttribute: NewString("email"), + UsernameAttribute: NewString("username"), + IdAttribute: NewString("id"), + LoginIdAttribute: NewString("loginid"), + GuestFilter: NewString("("), + }, + ExpectError: true, + }, + { + Name: "invalid guest filter #2", + LdapSettings: LdapSettings{ + Enable: NewBool(true), + LdapServer: NewString("server"), + BaseDN: NewString("basedn"), + EmailAttribute: NewString("email"), + UsernameAttribute: NewString("username"), + IdAttribute: NewString("id"), + LoginIdAttribute: NewString("loginid"), + GuestFilter: NewString("()"), + }, + ExpectError: true, + }, + { + Name: "valid guest filter #2", + LdapSettings: LdapSettings{ + Enable: NewBool(true), + LdapServer: NewString("server"), + BaseDN: NewString("basedn"), + EmailAttribute: NewString("email"), + UsernameAttribute: NewString("username"), + IdAttribute: NewString("id"), + LoginIdAttribute: NewString("loginid"), + GuestFilter: NewString("(&(property=value)(otherthing=othervalue))"), + }, + ExpectError: false, + }, + { + Name: "valid guest filter #3", + LdapSettings: LdapSettings{ + Enable: NewBool(true), + LdapServer: NewString("server"), + BaseDN: NewString("basedn"), + EmailAttribute: NewString("email"), + UsernameAttribute: NewString("username"), + IdAttribute: NewString("id"), + LoginIdAttribute: NewString("loginid"), + GuestFilter: NewString("(&(property=value)(|(otherthing=othervalue)(other=thing)))"), + }, + ExpectError: false, + }, + { + Name: "invalid guest filter #3", + LdapSettings: LdapSettings{ + Enable: NewBool(true), + LdapServer: NewString("server"), + BaseDN: NewString("basedn"), + EmailAttribute: NewString("email"), + UsernameAttribute: NewString("username"), + IdAttribute: NewString("id"), + LoginIdAttribute: NewString("loginid"), + GuestFilter: NewString("(&(property=value)(|(otherthing=othervalue)(other=thing))"), + }, + ExpectError: true, + }, + { + Name: "invalid guest filter #4", + LdapSettings: LdapSettings{ + Enable: NewBool(true), + LdapServer: NewString("server"), + BaseDN: NewString("basedn"), + EmailAttribute: NewString("email"), + UsernameAttribute: NewString("username"), + IdAttribute: NewString("id"), + LoginIdAttribute: NewString("loginid"), + GuestFilter: NewString("(&(property=value)((otherthing=othervalue)(other=thing)))"), + }, + ExpectError: true, + }, } { t.Run(test.Name, func(t *testing.T) { test.LdapSettings.SetDefaults() From dc05e810128752652579c8736ed65ec564638b88 Mon Sep 17 00:00:00 2001 From: Santosh Desani Date: Mon, 14 Oct 2019 13:00:23 -0500 Subject: [PATCH 11/23] =?UTF-8?q?[MM-18277]=20Refactor=20plugin/health=5Fc?= =?UTF-8?q?heck.go=20to=20use=20structured=20l=E2=80=A6=20(#12734)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- plugin/health_check.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/plugin/health_check.go b/plugin/health_check.go index 85989e4775..3cab84b9da 100644 --- a/plugin/health_check.go +++ b/plugin/health_check.go @@ -4,7 +4,6 @@ package plugin import ( - "fmt" "sync" "time" @@ -88,7 +87,7 @@ func (job *PluginHealthCheckJob) checkPlugin(id string) { pluginErr := sup.PerformHealthCheck() if pluginErr != nil { - mlog.Error(fmt.Sprintf("Health check failed for plugin %s, error: %s", id, pluginErr.Error())) + mlog.Error("Health check failed for plugin", mlog.String("id", id), mlog.Err(pluginErr)) job.handleHealthCheckFail(id, pluginErr) } } @@ -107,13 +106,13 @@ func (job *PluginHealthCheckJob) handleHealthCheckFail(id string, err error) { if shouldDeactivatePlugin(p) { p.failTimeStamps = []time.Time{} - mlog.Debug(fmt.Sprintf("Deactivating plugin due to multiple crashes `%s`", id)) + mlog.Debug("Deactivating plugin due to multiple crashes", mlog.String("id", id)) job.env.Deactivate(id) job.env.SetPluginState(id, model.PluginStateFailedToStayRunning) } else { - mlog.Debug(fmt.Sprintf("Restarting plugin due to failed health check `%s`", id)) + mlog.Debug("Restarting plugin due to failed health check", mlog.String("id", id)) if err := job.env.RestartPlugin(id); err != nil { - mlog.Error(fmt.Sprintf("Failed to restart plugin `%s`: %s", id, err.Error())) + mlog.Error("Failed to restart plugin", mlog.String("id", id), mlog.Err(err)) } } } From d5fa1297a9845f4f9497e776784333d8523e37a4 Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Mon, 14 Oct 2019 22:25:44 +0300 Subject: [PATCH 12/23] services/filestore: migrate s3store_test to testify (#12685) * services/filestore: migrate s3store_test to testify * services/filestore: s3_store_test.go polished --- services/filesstore/s3store_test.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/services/filesstore/s3store_test.go b/services/filesstore/s3store_test.go index 02fb61438f..b6e423a427 100644 --- a/services/filesstore/s3store_test.go +++ b/services/filesstore/s3store_test.go @@ -7,26 +7,23 @@ import ( "testing" "github.com/mattermost/mattermost-server/model" + "github.com/stretchr/testify/require" ) func TestCheckMandatoryS3Fields(t *testing.T) { cfg := model.FileSettings{} err := CheckMandatoryS3Fields(&cfg) - if err == nil || err.Message != "api.admin.test_s3.missing_s3_bucket" { - t.Fatal("should've failed with missing s3 bucket") - } + require.NotNil(t, err) + require.Equal(t, err.Message, "api.admin.test_s3.missing_s3_bucket", "should've failed with missing s3 bucket") cfg.AmazonS3Bucket = model.NewString("test-mm") err = CheckMandatoryS3Fields(&cfg) - if err != nil { - t.Fatal("should've not failed") - } + require.Nil(t, err) cfg.AmazonS3Endpoint = model.NewString("") err = CheckMandatoryS3Fields(&cfg) - if err != nil || *cfg.AmazonS3Endpoint != "s3.amazonaws.com" { - t.Fatal("should've not failed because it should set the endpoint to the default") - } + require.Nil(t, err) + require.Equal(t, *cfg.AmazonS3Endpoint, "s3.amazonaws.com", "should've set the endpoint to the default") } From 7239f8da60ebe438e0e29da26dfac65719159d75 Mon Sep 17 00:00:00 2001 From: Lev <1187448+levb@users.noreply.github.com> Date: Mon, 14 Oct 2019 20:36:56 -0700 Subject: [PATCH 13/23] Bumped Jira plugin version to 2.2.2 (#12770) * Bumped the Jira plugin to v2.2.0 * Bumped Jira plugin version to 2.2.1 * Bumped Jira plugin version to 2.2.2 * Bumped Jira plugin version to v2.2.2 --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index d714aff153..f47b6de541 100644 --- a/Makefile +++ b/Makefile @@ -89,7 +89,7 @@ PLUGIN_PACKAGES += mattermost-plugin-github-v0.11.0 PLUGIN_PACKAGES += mattermost-plugin-welcomebot-v1.1.1 PLUGIN_PACKAGES += mattermost-plugin-aws-SNS-v1.0.2 PLUGIN_PACKAGES += mattermost-plugin-antivirus-v0.1.1 -PLUGIN_PACKAGES += mattermost-plugin-jira-v2.2.1 +PLUGIN_PACKAGES += mattermost-plugin-jira-v2.2.2 PLUGIN_PACKAGES += mattermost-plugin-gitlab-v1.0.1 PLUGIN_PACKAGES += mattermost-plugin-jenkins-v1.0.0 From ade6044441e901eccb4e01dd72a6f46f2bc2a189 Mon Sep 17 00:00:00 2001 From: Sascha Andres Date: Tue, 15 Oct 2019 10:48:06 +0200 Subject: [PATCH 14/23] =?UTF-8?q?MM-19135=20Migrate=20tests=20from=20"mode?= =?UTF-8?q?l/channel=5Fmember=5Ftest.go"=20to=E2=80=A6=20(#12651)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- model/channel_member_test.go | 47 ++++++++++-------------------------- 1 file changed, 13 insertions(+), 34 deletions(-) diff --git a/model/channel_member_test.go b/model/channel_member_test.go index 26c9c3ddfe..31f9e417e3 100644 --- a/model/channel_member_test.go +++ b/model/channel_member_test.go @@ -6,6 +6,8 @@ package model import ( "strings" "testing" + + "github.com/stretchr/testify/require" ) func TestChannelMemberJson(t *testing.T) { @@ -13,22 +15,16 @@ func TestChannelMemberJson(t *testing.T) { json := o.ToJson() ro := ChannelMemberFromJson(strings.NewReader(json)) - if o.ChannelId != ro.ChannelId { - t.Fatal("Ids do not match") - } + require.Equal(t, o.ChannelId, ro.ChannelId, "ids do not match") } func TestChannelMemberIsValid(t *testing.T) { o := ChannelMember{} - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid(), "should be invalid") o.ChannelId = NewId() - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid(), "should be invalid") o.NotifyProps = GetDefaultChannelNotifyProps() o.UserId = NewId() @@ -40,34 +36,22 @@ func TestChannelMemberIsValid(t *testing.T) { }*/ o.NotifyProps["desktop"] = "junk" - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid(), "should be invalid") o.NotifyProps["desktop"] = "123456789012345678901" - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid(), "should be invalid") o.NotifyProps["desktop"] = CHANNEL_NOTIFY_ALL - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + require.Error(t, o.IsValid(), "should be invalid") o.NotifyProps["mark_unread"] = "123456789012345678901" - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid(), "should be invalid") o.NotifyProps["mark_unread"] = CHANNEL_MARK_UNREAD_ALL - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + require.Error(t, o.IsValid(), "should be invalid") o.Roles = "" - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + require.Error(t, o.IsValid(), "should be invalid") } func TestChannelUnreadJson(t *testing.T) { @@ -75,11 +59,6 @@ func TestChannelUnreadJson(t *testing.T) { json := o.ToJson() ro := ChannelUnreadFromJson(strings.NewReader(json)) - if o.TeamId != ro.TeamId { - t.Fatal("Team Ids do not match") - } - - if o.MentionCount != ro.MentionCount { - t.Fatal("MentionCount do not match") - } + require.Equal(t, o.TeamId, ro.TeamId, "team Ids do not match") + require.Equal(t, o.MentionCount, ro.MentionCount, "mention count do not match") } From 441777a4e7b00cc53fea05a52235689c7e28c8ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Tue, 15 Oct 2019 11:16:31 +0200 Subject: [PATCH 15/23] Only include the avatar attachment if is going to show a message in the invitation (#12747) --- app/email.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/email.go b/app/email.go index 5499f18053..33edfbb116 100644 --- a/app/email.go +++ b/app/email.go @@ -445,9 +445,11 @@ func (a *App) SendGuestInviteEmails(team *model.Team, channels []*model.Channel, } embeddedFiles := make(map[string]io.Reader) - if senderProfileImage != nil { - embeddedFiles = map[string]io.Reader{ - "user-avatar.png": bytes.NewReader(senderProfileImage), + if message != "" { + if senderProfileImage != nil { + embeddedFiles = map[string]io.Reader{ + "user-avatar.png": bytes.NewReader(senderProfileImage), + } } } From 6b901773e6ed774408e6cb1de5b1c62660a4ac46 Mon Sep 17 00:00:00 2001 From: Ogundele Olumide Date: Tue, 15 Oct 2019 11:27:09 +0100 Subject: [PATCH 16/23] MM-18274 Refactor "app/email.go" to use structured logging (#12504) * chore: refactor to use structured logging - change fmt.sprintf method used in the logger to appropriate mlog method * implement suggested changes by removing added line during merge conflict --- app/email.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/email.go b/app/email.go index 33edfbb116..cf54a2af2d 100644 --- a/app/email.go +++ b/app/email.go @@ -347,13 +347,13 @@ func (a *App) SendInviteEmails(team *model.Team, senderName string, senderUserId data := model.MapToJson(props) if err := a.Srv.Store.Token().Save(token); err != nil { - mlog.Error(fmt.Sprintf("Failed to send invite email successfully err=%v", err)) + mlog.Error("Failed to send invite email successfully ", mlog.Err(err)) continue } bodyPage.Props["Link"] = fmt.Sprintf("%s/signup_user_complete/?d=%s&t=%s", siteURL, url.QueryEscape(data), url.QueryEscape(token.Token)) if err := a.SendMail(invite, subject, bodyPage.Render()); err != nil { - mlog.Error(fmt.Sprintf("Failed to send invite email successfully err=%v", err)) + mlog.Error("Failed to send invite email successfully ", mlog.Err(err)) } } } @@ -435,13 +435,13 @@ func (a *App) SendGuestInviteEmails(team *model.Team, channels []*model.Channel, data := model.MapToJson(props) if err := a.Srv.Store.Token().Save(token); err != nil { - mlog.Error(fmt.Sprintf("Failed to send invite email successfully err=%v", err)) + mlog.Error("Failed to send invite email successfully ", mlog.Err(err)) continue } bodyPage.Props["Link"] = fmt.Sprintf("%s/signup_user_complete/?d=%s&t=%s", siteURL, url.QueryEscape(data), url.QueryEscape(token.Token)) if !*a.Config().EmailSettings.SendEmailNotifications { - mlog.Info(fmt.Sprintf("sending invitation to %v %v", invite, bodyPage.Props["Link"])) + mlog.Info("sending invitation ", mlog.String("to", invite), mlog.String("link", bodyPage.Props["Link"].(string))) } embeddedFiles := make(map[string]io.Reader) @@ -454,7 +454,7 @@ func (a *App) SendGuestInviteEmails(team *model.Team, channels []*model.Channel, } if err := a.SendMailWithEmbeddedFiles(invite, subject, bodyPage.Render(), embeddedFiles); err != nil { - mlog.Error(fmt.Sprintf("Failed to send invite email successfully err=%v", err)) + mlog.Error("Failed to send invite email successfully", mlog.Err(err)) } } } From 99f343f4ce6301ba7110967aa99af4e4cc5ee22e Mon Sep 17 00:00:00 2001 From: Carlos Tadeu Panato Junior Date: Tue, 15 Oct 2019 13:24:46 +0200 Subject: [PATCH 17/23] remove -p=8 (#12799) --- .circleci/config.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 244b10533e..9ce3c3b4ee 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -66,8 +66,8 @@ jobs: cd mattermost-server make config-reset make check-style BUILD_NUMBER='${CIRCLE_BRANCH}-${CIRCLE_BUILD_NUM}' - GOFLAGS=-p=8 make build BUILD_NUMBER='${CIRCLE_BRANCH}-${CIRCLE_BUILD_NUM}' - GOFLAGS=-p=8 make package BUILD_NUMBER='${CIRCLE_BRANCH}-${CIRCLE_BUILD_NUM}' + make build BUILD_NUMBER='${CIRCLE_BRANCH}-${CIRCLE_BUILD_NUM}' + make package BUILD_NUMBER='${CIRCLE_BRANCH}-${CIRCLE_BUILD_NUM}' - store_artifacts: path: /go/src/github.com/mattermost/mattermost-server/dist/mattermost-team-linux-amd64.tar.gz - store_artifacts: From d97afa72cb4166dd62833c57551e9ba806a02690 Mon Sep 17 00:00:00 2001 From: Irzhy Ranaivoarivony Date: Tue, 15 Oct 2019 16:10:40 +0300 Subject: [PATCH 18/23] =?UTF-8?q?MM-19352=20-=20Migrated=20test=20in=20sto?= =?UTF-8?q?re/storetest/audit=5Fstore.go=20to=E2=80=A6=20(#12739)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- store/storetest/audit_store.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/store/storetest/audit_store.go b/store/storetest/audit_store.go index a9a9dbb012..1aa1956612 100644 --- a/store/storetest/audit_store.go +++ b/store/storetest/audit_store.go @@ -45,9 +45,7 @@ func testAuditStore(t *testing.T, ss store.Store) { audits, err = ss.Audit().Get("", 0, 100) require.Nil(t, err) - if len(audits) < 4 { - t.Fatal("Failed to save and retrieve 4 audit logs") - } + require.Len(t, audits, 4, "Failed to save and retrieve 4 audit logs") require.Nil(t, ss.Audit().PermanentDeleteByUser(audit.UserId)) } From 710c732349b301e356d53fc78f70a9bec1b0a547 Mon Sep 17 00:00:00 2001 From: Ben Sooraj Date: Tue, 15 Oct 2019 18:55:12 +0530 Subject: [PATCH 19/23] mlog standardisation for app/channel.go (#12738) --- app/channel.go | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/app/channel.go b/app/channel.go index 68851a477f..7ec1b5f61f 100644 --- a/app/channel.go +++ b/app/channel.go @@ -95,7 +95,7 @@ func (a *App) JoinDefaultChannels(teamId string, user *model.User, shouldBeAdmin _, err = a.Srv.Store.Channel().SaveMember(cm) if histErr := a.Srv.Store.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, model.GetMillis()); histErr != nil { - mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", histErr)) + mlog.Warn("Failed to update ChannelMemberHistory table", mlog.Err(histErr)) } if *a.Config().ServiceSettings.ExperimentalEnableDefaultChannelLeaveJoinMessages { @@ -126,21 +126,21 @@ func (a *App) postJoinMessageForDefaultChannel(user *model.User, requestor *mode if channel.Name == model.DEFAULT_CHANNEL { if requestor == nil { if err := a.postJoinTeamMessage(user, channel); err != nil { - mlog.Error(fmt.Sprint("Failed to post join/leave message", err)) + mlog.Error("Failed to post join/leave message", mlog.Err(err)) } } else { if err := a.postAddToTeamMessage(requestor, user, channel, ""); err != nil { - mlog.Error(fmt.Sprint("Failed to post join/leave message", err)) + mlog.Error("Failed to post join/leave message", mlog.Err(err)) } } } else { if requestor == nil { if err := a.postJoinChannelMessage(user, channel); err != nil { - mlog.Error(fmt.Sprint("Failed to post join/leave message", err)) + mlog.Error("Failed to post join/leave message", mlog.Err(err)) } } else { if err := a.PostAddToChannelMessage(requestor, user, channel, ""); err != nil { - mlog.Error(fmt.Sprint("Failed to post join/leave message", err)) + mlog.Error("Failed to post join/leave message", mlog.Err(err)) } } } @@ -247,7 +247,7 @@ func (a *App) CreateChannel(channel *model.Channel, addMember bool) (*model.Chan return nil, err } if err := a.Srv.Store.ChannelMemberHistory().LogJoinEvent(channel.CreatorId, sc.Id, model.GetMillis()); err != nil { - mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", err)) + mlog.Warn("Failed to update ChannelMemberHistory table", mlog.Err(err)) } a.InvalidateCacheForUser(channel.CreatorId) @@ -366,10 +366,10 @@ func (a *App) createDirectChannel(userId string, otherUserId string) (*model.Cha } if err = a.Srv.Store.ChannelMemberHistory().LogJoinEvent(userId, channel.Id, model.GetMillis()); err != nil { - mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", err)) + mlog.Warn("Failed to update ChannelMemberHistory table", mlog.Err(err)) } if err = a.Srv.Store.ChannelMemberHistory().LogJoinEvent(otherUserId, channel.Id, model.GetMillis()); err != nil { - mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", err)) + mlog.Warn("Failed to update ChannelMemberHistory table", mlog.Err(err)) } return channel, nil @@ -399,7 +399,7 @@ func (a *App) WaitForChannelMembership(channelId string, userId string) { } } - mlog.Error(fmt.Sprintf("WaitForChannelMembership giving up channelId=%v userId=%v", channelId, userId), mlog.String("user_id", userId)) + mlog.Error("WaitForChannelMembership giving up", mlog.String("channel_id", channelId), mlog.String("user_id", userId)) } func (a *App) CreateGroupChannel(userIds []string, creatorId string) (*model.Channel, *model.AppError) { @@ -477,7 +477,7 @@ func (a *App) createGroupChannel(userIds []string, creatorId string) (*model.Cha return nil, err } if err := a.Srv.Store.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, model.GetMillis()); err != nil { - mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", err)) + mlog.Warn("Failed to update ChannelMemberHistory table", mlog.Err(err)) } } @@ -861,21 +861,21 @@ func (a *App) DeleteChannel(channel *model.Channel, userId string) *model.AppErr } if _, err := a.CreatePost(post, channel, false); err != nil { - mlog.Error(fmt.Sprintf("Failed to post archive message %v", err)) + mlog.Error("Failed to post archive message", mlog.Err(err)) } } now := model.GetMillis() for _, hook := range incomingHooks { if err := a.Srv.Store.Webhook().DeleteIncoming(hook.Id, now); err != nil { - mlog.Error(fmt.Sprintf("Encountered error deleting incoming webhook, id=%v", hook.Id)) + mlog.Error("Encountered error deleting incoming webhook", mlog.String("hook_id", hook.Id), mlog.Err(err)) } a.InvalidateCacheForWebhook(hook.Id) } for _, hook := range outgoingHooks { if err := a.Srv.Store.Webhook().DeleteOutgoing(hook.Id, now); err != nil { - mlog.Error(fmt.Sprintf("Encountered error deleting outgoing webhook, id=%v", hook.Id)) + mlog.Error("Encountered error deleting outgoing webhook", mlog.String("hook_id", hook.Id), mlog.Err(err)) } } @@ -926,13 +926,13 @@ func (a *App) addUserToChannel(user *model.User, channel *model.Channel, teamMem SchemeUser: !user.IsGuest(), } if _, err = a.Srv.Store.Channel().SaveMember(newMember); err != nil { - mlog.Error(fmt.Sprintf("Failed to add member user_id=%v channel_id=%v err=%v", user.Id, channel.Id, err), mlog.String("user_id", user.Id)) + mlog.Error("Failed to add member", mlog.String("user_id", user.Id), mlog.String("channel_id", channel.Id), mlog.Err(err)) return nil, model.NewAppError("AddUserToChannel", "api.channel.add_user.to.channel.failed.app_error", nil, "", http.StatusInternalServerError) } a.WaitForChannelMembership(channel.Id, user.Id) if err = a.Srv.Store.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, model.GetMillis()); err != nil { - mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", err)) + mlog.Warn("Failed to update ChannelMemberHistory table", mlog.Err(err)) } a.InvalidateCacheForUser(user.Id) @@ -1884,13 +1884,13 @@ func (a *App) MarkChannelsAsViewed(channelIds []string, userId string, currentSe for _, channelId := range channelIds { channel, errCh := a.Srv.Store.Channel().Get(channelId, true) if errCh != nil { - mlog.Warn(fmt.Sprintf("Failed to get channel %v", errCh)) + mlog.Warn("Failed to get channel", mlog.Err(errCh)) continue } member, err := a.Srv.Store.Channel().GetMember(channelId, userId) if err != nil { - mlog.Warn(fmt.Sprintf("Failed to get membership %v", err)) + mlog.Warn("Failed to get membership", mlog.Err(err)) continue } From 65dc760f015c5363eed65c7383889c49b43c43c9 Mon Sep 17 00:00:00 2001 From: Saturnino Abril Date: Tue, 15 Oct 2019 21:25:31 +0800 Subject: [PATCH 20/23] add option to generate deactivated users via sampledata (#12772) --- cmd/mattermost/commands/sampledata.go | 78 ++++++++++++++++++--------- 1 file changed, 54 insertions(+), 24 deletions(-) diff --git a/cmd/mattermost/commands/sampledata.go b/cmd/mattermost/commands/sampledata.go index b46a341ed5..f6eb6cd99d 100644 --- a/cmd/mattermost/commands/sampledata.go +++ b/cmd/mattermost/commands/sampledata.go @@ -17,9 +17,15 @@ import ( "github.com/icrowley/fake" "github.com/mattermost/mattermost-server/app" + "github.com/mattermost/mattermost-server/model" "github.com/spf13/cobra" ) +const ( + DEACTIVATED_USER = "deactivated" + GUEST_USER = "guest" +) + var SampleDataCmd = &cobra.Command{ Use: "sampledata", Short: "Generate sample data", @@ -32,6 +38,7 @@ func init() { SampleDataCmd.Flags().Int("channels-per-team", 10, "The number of sample channels per team.") SampleDataCmd.Flags().IntP("users", "u", 15, "The number of sample users.") SampleDataCmd.Flags().IntP("guests", "g", 1, "The number of sample guests.") + SampleDataCmd.Flags().Int("deactivated-users", 0, "The number of deactivated users.") SampleDataCmd.Flags().Int("team-memberships", 2, "The number of sample team memberships per user.") SampleDataCmd.Flags().Int("channel-memberships", 5, "The number of sample channel memberships per user in a team.") SampleDataCmd.Flags().Int("posts-per-channel", 100, "The number of sample post per channel.") @@ -165,6 +172,10 @@ func sampleDataCmdF(command *cobra.Command, args []string) error { if err != nil || users < 0 { return errors.New("Invalid users parameter") } + deactivatedUsers, err := command.Flags().GetInt("deactivated-users") + if err != nil || deactivatedUsers < 0 { + return errors.New("Invalid deactivated-users parameter") + } guests, err := command.Flags().GetInt("guests") if err != nil || guests < 0 { return errors.New("Invalid guests parameter") @@ -283,12 +294,17 @@ func sampleDataCmdF(command *cobra.Command, args []string) error { allUsers := []string{} for i := 0; i < users; i++ { - userLine := createUser(i, teamMemberships, channelMemberships, teamsAndChannels, profileImages, false) + userLine := createUser(i, teamMemberships, channelMemberships, teamsAndChannels, profileImages, "") encoder.Encode(userLine) allUsers = append(allUsers, *userLine.User.Username) } for i := 0; i < guests; i++ { - userLine := createUser(i, teamMemberships, channelMemberships, teamsAndChannels, profileImages, true) + userLine := createUser(i, teamMemberships, channelMemberships, teamsAndChannels, profileImages, GUEST_USER) + encoder.Encode(userLine) + allUsers = append(allUsers, *userLine.User.Username) + } + for i := 0; i < deactivatedUsers; i++ { + userLine := createUser(i, teamMemberships, channelMemberships, teamsAndChannels, profileImages, DEACTIVATED_USER) encoder.Encode(userLine) allUsers = append(allUsers, *userLine.User.Username) } @@ -355,36 +371,44 @@ func sampleDataCmdF(command *cobra.Command, args []string) error { return nil } -func createUser(idx int, teamMemberships int, channelMemberships int, teamsAndChannels map[string][]string, profileImages []string, guest bool) app.LineImportData { - password := fmt.Sprintf("SampleUs@r-%d", idx) - email := fmt.Sprintf("user-%d@sample.mattermost.com", idx) - if guest { - password = fmt.Sprintf("SampleGu@st-%d", idx) - email = fmt.Sprintf("guest-%d@sample.mattermost.com", idx) - } +func createUser(idx int, teamMemberships int, channelMemberships int, teamsAndChannels map[string][]string, profileImages []string, userType string) app.LineImportData { firstName := fake.FirstName() lastName := fake.LastName() + position := fake.JobTitle() + username := fmt.Sprintf("%s.%s", strings.ToLower(firstName), strings.ToLower(lastName)) - if guest { + roles := "system_user" + + var password string + var email string + + switch userType { + case GUEST_USER: + password = fmt.Sprintf("SampleGu@st-%d", idx) + email = fmt.Sprintf("guest-%d@sample.mattermost.com", idx) + roles = "system_guest" if idx == 0 { username = "guest" password = "SampleGu@st1" email = "guest@sample.mattermost.com" } - } else if idx == 0 { - username = "sysadmin" - password = "Sys@dmin-sample1" - email = "sysadmin@sample.mattermost.com" - } else if idx == 1 { - username = "user-1" - } + case DEACTIVATED_USER: + password = fmt.Sprintf("SampleDe@ctivated-%d", idx) + email = fmt.Sprintf("deactivated-%d@sample.mattermost.com", idx) + default: + password = fmt.Sprintf("SampleUs@r-%d", idx) + email = fmt.Sprintf("user-%d@sample.mattermost.com", idx) + if idx == 0 { + username = "sysadmin" + password = "Sys@dmin-sample1" + email = "sysadmin@sample.mattermost.com" + } else if idx == 1 { + username = "user-1" + } - position := fake.JobTitle() - roles := "system_user" - if guest { - roles = "system_guest" - } else if idx%5 == 0 { - roles = "system_admin system_user" + if idx%5 == 0 { + roles = "system_admin system_user" + } } // The 75% of the users have custom profile image @@ -450,10 +474,15 @@ func createUser(idx int, teamMemberships int, channelMemberships int, teamsAndCh team := possibleTeams[position] possibleTeams = append(possibleTeams[:position], possibleTeams[position+1:]...) if teamChannels, err := teamsAndChannels[team]; err { - teams = append(teams, createTeamMembership(channelMemberships, teamChannels, &team, guest)) + teams = append(teams, createTeamMembership(channelMemberships, teamChannels, &team, userType == GUEST_USER)) } } + var deleteAt int64 + if userType == DEACTIVATED_USER { + deleteAt = model.GetMillis() + } + user := app.UserImportData{ ProfileImage: profileImage, Username: &username, @@ -470,6 +499,7 @@ func createUser(idx int, teamMemberships int, channelMemberships int, teamsAndCh MessageDisplay: &messageDisplay, ChannelDisplayMode: &channelDisplayMode, TutorialStep: &tutorialStep, + DeleteAt: &deleteAt, } return app.LineImportData{ Type: "user", From 69c8f89057838cb52e125b421fba7ae116b7030a Mon Sep 17 00:00:00 2001 From: Fares Rihani Date: Tue, 15 Oct 2019 09:33:41 -0400 Subject: [PATCH 21/23] Update Contributing Code link in README.md (#12735) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 5a3ac64d77..6ccc23bc54 100644 --- a/README.md +++ b/README.md @@ -46,7 +46,7 @@ Receive notifications of critical security updates. The sophistication of online ## Get Involved -- [Contribute Code](http://docs.mattermost.com/developer/contribution-guide.html) +- [Contribute Code](https://developers.mattermost.com/contribute/getting-started/) - [Find "Help Wanted" projects](https://github.com/mattermost/mattermost-server/issues?page=1&q=is%3Aissue+is%3Aopen+%22Help+Wanted%22&utf8=%E2%9C%93) - [Join Developer Discussion on a Mattermost Server for contributors](https://pre-release.mattermost.com/signup_user_complete/?id=f1924a8db44ff3bb41c96424cdc20676) - [File Bugs](http://www.mattermost.org/filing-issues/) From 0ba6a791ef3dac7d602a0624cd01456151877c90 Mon Sep 17 00:00:00 2001 From: jatinjtg <50952137+jatinjtg@users.noreply.github.com> Date: Tue, 15 Oct 2019 19:15:46 +0530 Subject: [PATCH 22/23] Fix typo (#12655) --- plugin/helpers_bots_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/helpers_bots_test.go b/plugin/helpers_bots_test.go index 776c32a10b..792f67e177 100644 --- a/plugin/helpers_bots_test.go +++ b/plugin/helpers_bots_test.go @@ -136,7 +136,7 @@ func TestEnsureBot(t *testing.T) { assert.Nil(t, err) }) - t.Run("shoudl fail if create bot fails", func(t *testing.T) { + t.Run("should fail if create bot fails", func(t *testing.T) { api := setupAPI() api.On("KVGet", plugin.BOT_USER_KEY).Return(nil, nil) api.On("GetUserByUsername", testbot.Username).Return(nil, nil) From d7d7216b0d6d76ca146c3ee12e5d446c66493c7a Mon Sep 17 00:00:00 2001 From: Amine Date: Tue, 15 Oct 2019 16:22:19 +0200 Subject: [PATCH 23/23] Migrate tests from store/storetest/cluster_discovery_store.go to use testify (#12757) --- store/storetest/cluster_discovery_store.go | 49 +++++++++------------- 1 file changed, 19 insertions(+), 30 deletions(-) diff --git a/store/storetest/cluster_discovery_store.go b/store/storetest/cluster_discovery_store.go index c2f039577e..ce5d068b92 100644 --- a/store/storetest/cluster_discovery_store.go +++ b/store/storetest/cluster_discovery_store.go @@ -29,13 +29,11 @@ func testClusterDiscoveryStore(t *testing.T, ss store.Store) { Type: "test_test", } - if err := ss.ClusterDiscovery().Save(discovery); err != nil { - t.Fatal(err) - } + err := ss.ClusterDiscovery().Save(discovery) + require.Nil(t, err) - if err := ss.ClusterDiscovery().Cleanup(); err != nil { - t.Fatal(err) - } + err = ss.ClusterDiscovery().Cleanup() + require.Nil(t, err) } func testClusterDiscoveryStoreDelete(t *testing.T, ss store.Store) { @@ -45,13 +43,11 @@ func testClusterDiscoveryStoreDelete(t *testing.T, ss store.Store) { Type: "test_test", } - if err := ss.ClusterDiscovery().Save(discovery); err != nil { - t.Fatal(err) - } + err := ss.ClusterDiscovery().Save(discovery) + require.Nil(t, err) - if _, err := ss.ClusterDiscovery().Delete(discovery); err != nil { - t.Fatal(err) - } + _, err = ss.ClusterDiscovery().Delete(discovery) + require.Nil(t, err) } func testClusterDiscoveryStoreLastPing(t *testing.T, ss store.Store) { @@ -61,29 +57,24 @@ func testClusterDiscoveryStoreLastPing(t *testing.T, ss store.Store) { Type: "test_test_lastPing" + model.NewId(), } - if err := ss.ClusterDiscovery().Save(discovery); err != nil { - t.Fatal(err) - } + err := ss.ClusterDiscovery().Save(discovery) + require.Nil(t, err) - if err := ss.ClusterDiscovery().SetLastPingAt(discovery); err != nil { - t.Fatal(err) - } + err = ss.ClusterDiscovery().SetLastPingAt(discovery) + require.Nil(t, err) ttime := model.GetMillis() time.Sleep(1 * time.Second) - if err := ss.ClusterDiscovery().SetLastPingAt(discovery); err != nil { - t.Fatal(err) - } + err = ss.ClusterDiscovery().SetLastPingAt(discovery) + require.Nil(t, err) list, err := ss.ClusterDiscovery().GetAll(discovery.Type, "cluster_name_lastPing") require.Nil(t, err) assert.Len(t, list, 1) - if list[0].LastPingAt-ttime < 500 { - t.Fatal("failed to set time") - } + require.Less(t, int64(500), list[0].LastPingAt-ttime) discovery2 := &model.ClusterDiscovery{ ClusterName: "cluster_name_missing", @@ -91,9 +82,8 @@ func testClusterDiscoveryStoreLastPing(t *testing.T, ss store.Store) { Type: "test_test_missing", } - if err := ss.ClusterDiscovery().SetLastPingAt(discovery2); err != nil { - t.Fatal(err) - } + err = ss.ClusterDiscovery().SetLastPingAt(discovery2) + require.Nil(t, err) } func testClusterDiscoveryStoreExists(t *testing.T, ss store.Store) { @@ -103,9 +93,8 @@ func testClusterDiscoveryStoreExists(t *testing.T, ss store.Store) { Type: "test_test_Exists" + model.NewId(), } - if err := ss.ClusterDiscovery().Save(discovery); err != nil { - t.Fatal(err) - } + err := ss.ClusterDiscovery().Save(discovery) + require.Nil(t, err) val, err := ss.ClusterDiscovery().Exists(discovery) require.Nil(t, err)