From e1fae73e8533ac9a2eb146cb0e07909bc7f78f74 Mon Sep 17 00:00:00 2001 From: SezalAgrawal Date: Fri, 4 Oct 2019 20:00:04 +0530 Subject: [PATCH 01/22] [MM-19123] Migrate tests from "model/push_notification_test.go" to use testify (#12557) * [MM-19123] Migrate tests from model/push_notification_test.go to use testify * [MM-19123] Re-arranged the import package --- model/push_notification_test.go | 62 +++++++++------------------------ 1 file changed, 17 insertions(+), 45 deletions(-) diff --git a/model/push_notification_test.go b/model/push_notification_test.go index a6e158105b..e2d7de1a5e 100644 --- a/model/push_notification_test.go +++ b/model/push_notification_test.go @@ -6,6 +6,8 @@ package model import ( "strings" "testing" + + "github.com/stretchr/testify/require" ) func TestPushNotification(t *testing.T) { @@ -13,9 +15,7 @@ func TestPushNotification(t *testing.T) { json := msg.ToJson() result := PushNotificationFromJson(strings.NewReader(json)) - if msg.Platform != result.Platform { - t.Fatal("Ids do not match") - } + require.Equal(t, msg.Platform, result.Platform, "Ids do not match") } func TestPushNotificationDeviceId(t *testing.T) { @@ -23,72 +23,44 @@ func TestPushNotificationDeviceId(t *testing.T) { msg := PushNotification{Platform: "test"} msg.SetDeviceIdAndPlatform("android:12345") - if msg.Platform != "android" { - t.Fatal(msg.Platform) - } - if msg.DeviceId != "12345" { - t.Fatal(msg.DeviceId) - } + require.Equal(t, msg.Platform, "android", msg.Platform) + require.Equal(t, msg.DeviceId, "12345", msg.DeviceId) msg.Platform = "" msg.DeviceId = "" msg.SetDeviceIdAndPlatform("android:12:345") - if msg.Platform != "android" { - t.Fatal(msg.Platform) - } - if msg.DeviceId != "12:345" { - t.Fatal(msg.DeviceId) - } + require.Equal(t, msg.Platform, "android", msg.Platform) + require.Equal(t, msg.DeviceId, "12:345", msg.DeviceId) msg.Platform = "" msg.DeviceId = "" msg.SetDeviceIdAndPlatform("android::12345") - if msg.Platform != "android" { - t.Fatal(msg.Platform) - } - if msg.DeviceId != ":12345" { - t.Fatal(msg.DeviceId) - } + require.Equal(t, msg.Platform, "android", msg.Platform) + require.Equal(t, msg.DeviceId, ":12345", msg.DeviceId) msg.Platform = "" msg.DeviceId = "" msg.SetDeviceIdAndPlatform(":12345") - if msg.Platform != "" { - t.Fatal(msg.Platform) - } - if msg.DeviceId != "12345" { - t.Fatal(msg.DeviceId) - } + require.Equal(t, msg.Platform, "", msg.Platform) + require.Equal(t, msg.DeviceId, "12345", msg.DeviceId) msg.Platform = "" msg.DeviceId = "" msg.SetDeviceIdAndPlatform("android:") - if msg.Platform != "android" { - t.Fatal(msg.Platform) - } - if msg.DeviceId != "" { - t.Fatal(msg.DeviceId) - } + require.Equal(t, msg.Platform, "android", msg.Platform) + require.Equal(t, msg.DeviceId, "", msg.DeviceId) msg.Platform = "" msg.DeviceId = "" msg.SetDeviceIdAndPlatform("") - if msg.Platform != "" { - t.Fatal(msg.Platform) - } - if msg.DeviceId != "" { - t.Fatal(msg.DeviceId) - } + require.Equal(t, msg.Platform, "", msg.Platform) + require.Equal(t, msg.DeviceId, "", msg.DeviceId) msg.Platform = "" msg.DeviceId = "" msg.SetDeviceIdAndPlatform(":") - if msg.Platform != "" { - t.Fatal(msg.Platform) - } - if msg.DeviceId != "" { - t.Fatal(msg.DeviceId) - } + require.Equal(t, msg.Platform, "", msg.Platform) + require.Equal(t, msg.DeviceId, "", msg.DeviceId) msg.Platform = "" msg.DeviceId = "" } From fa166f13bf60e745d2e4bcc504fc9d194dd87146 Mon Sep 17 00:00:00 2001 From: Ogundele Olumide Date: Fri, 4 Oct 2019 15:43:29 +0100 Subject: [PATCH 02/22] chore: migrate test to testify (#12507) - replace calls to t.fatal with require assertion of testify toolkit --- cmd/mattermost/commands/channel_test.go | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/cmd/mattermost/commands/channel_test.go b/cmd/mattermost/commands/channel_test.go index ada50baf7a..e6109a7d7a 100644 --- a/cmd/mattermost/commands/channel_test.go +++ b/cmd/mattermost/commands/channel_test.go @@ -86,25 +86,17 @@ func TestListChannels(t *testing.T) { output := th.CheckCommand(t, "channel", "list", th.BasicTeam.Name) - if !strings.Contains(string(output), "town-square") { - t.Fatal("should have channels") - } + require.True(t, strings.Contains(string(output), "town-square"), "should have channels") - if !strings.Contains(string(output), channel.Name+" (archived)") { - t.Fatal("should have archived channel") - } + require.True(t, strings.Contains(string(output), channel.Name+" (archived)"), "should have archived channel") - if !strings.Contains(string(output), privateChannel.Name+" (private)") { - t.Fatal("should have private channel") - } + require.True(t, strings.Contains(string(output), privateChannel.Name+" (private)"), "should have private channel") th.Client.Must(th.Client.DeleteChannel(privateChannel.Id)) output = th.CheckCommand(t, "channel", "list", th.BasicTeam.Name) - if !strings.Contains(string(output), privateChannel.Name+" (archived) (private)") { - t.Fatal("should have a channel both archived and private") - } + require.True(t, strings.Contains(string(output), privateChannel.Name+" (archived) (private)"), "should have a channel both archived and private") } func TestRestoreChannel(t *testing.T) { From 293d4c762e45abc603e90fa6f6827333b9a54b15 Mon Sep 17 00:00:00 2001 From: Lev <1187448+levb@users.noreply.github.com> Date: Fri, 4 Oct 2019 07:46:37 -0700 Subject: [PATCH 03/22] Bumped GitHub plugin to 0.11.0 (#12613) --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index edef42be10..9c1217b860 100644 --- a/Makefile +++ b/Makefile @@ -85,7 +85,7 @@ PLUGIN_PACKAGES=mattermost-plugin-zoom-v1.1.1 PLUGIN_PACKAGES += mattermost-plugin-autolink-v1.1.1 PLUGIN_PACKAGES += mattermost-plugin-nps-v1.0.3 PLUGIN_PACKAGES += mattermost-plugin-custom-attributes-v1.0.2 -PLUGIN_PACKAGES += mattermost-plugin-github-v0.10.2 +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 From f97ed668ba0db37c9eee5cf861ba6bd89c44c974 Mon Sep 17 00:00:00 2001 From: George Felix Date: Fri, 4 Oct 2019 17:36:26 +0200 Subject: [PATCH 04/22] Migrated tests from "model/incoming_webhook_test.go" to use testify (#12602) * Replaced test assertions with testify * Changes based on suggestions https://github.com/mattermost/mattermost-server/pull/12602#pullrequestreview-296859312 * Unused variable replaced with blank identifier --- model/incoming_webhook_test.go | 104 +++++++++------------------------ 1 file changed, 28 insertions(+), 76 deletions(-) diff --git a/model/incoming_webhook_test.go b/model/incoming_webhook_test.go index 3f7d136952..784675f2e3 100644 --- a/model/incoming_webhook_test.go +++ b/model/incoming_webhook_test.go @@ -6,6 +6,8 @@ package model import ( "strings" "testing" + + "github.com/stretchr/testify/require" ) func TestIncomingWebhookJson(t *testing.T) { @@ -13,102 +15,64 @@ func TestIncomingWebhookJson(t *testing.T) { json := o.ToJson() ro := IncomingWebhookFromJson(strings.NewReader(json)) - if o.Id != ro.Id { - t.Fatal("Ids do not match") - } + require.Equal(t, o.Id, ro.Id) } func TestIncomingWebhookIsValid(t *testing.T) { o := IncomingWebhook{} - 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.UserId = "123" - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.UserId = NewId() - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.ChannelId = "123" - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.ChannelId = NewId() - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.TeamId = "123" - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.TeamId = NewId() - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + require.Nil(t, o.IsValid()) o.DisplayName = strings.Repeat("1", 65) - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.DisplayName = strings.Repeat("1", 64) - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + require.Nil(t, o.IsValid()) o.Description = strings.Repeat("1", 501) - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.Description = strings.Repeat("1", 500) - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + require.Nil(t, o.IsValid()) o.Username = strings.Repeat("1", 65) - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.Username = strings.Repeat("1", 64) - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + require.Nil(t, o.IsValid()) o.IconURL = strings.Repeat("1", 1025) - if err := o.IsValid(); err == nil { - t.Fatal("should be invalid") - } + require.Error(t, o.IsValid()) o.IconURL = strings.Repeat("1", 1024) - if err := o.IsValid(); err != nil { - t.Fatal(err) - } + require.Nil(t, o.IsValid()) } func TestIncomingWebhookPreSave(t *testing.T) { @@ -138,7 +102,7 @@ func TestIncomingWebhookRequestFromJson(t *testing.T) { `, } - for i, text := range texts { + for _, text := range texts { // build a sample payload with the text payload := `{ "text": "` + text + `", @@ -179,30 +143,18 @@ func TestIncomingWebhookRequestFromJson(t *testing.T) { // After it has been decoded, the JSON string won't contain the escape char anymore expected := strings.Replace(text, `\"`, `"`, -1) - if iwr == nil { - t.Fatal("IncomingWebhookRequest should not be nil") - } - if iwr.Text != expected { - t.Fatalf("Sample %d text should be: %s, got: %s", i, expected, iwr.Text) - } + require.NotNil(t, iwr) + require.Equal(t, expected, iwr.Text) attachment := iwr.Attachments[0] - if attachment.Text != expected { - t.Fatalf("Sample %d attachment text should be: %s, got: %s", i, expected, attachment.Text) - } + require.Equal(t, expected, attachment.Text) } } func TestIncomingWebhookNullArrayItems(t *testing.T) { payload := `{"attachments":[{"fields":[{"title":"foo","value":"bar","short":true}, null]}, null]}` iwr, _ := IncomingWebhookRequestFromJson(strings.NewReader(payload)) - if iwr == nil { - t.Fatal("IncomingWebhookRequest should not be nil") - } - if len(iwr.Attachments) != 1 { - t.Fatalf("expected one attachment") - } - if len(iwr.Attachments[0].Fields) != 1 { - t.Fatalf("expected one field") - } + require.NotNil(t, iwr) + require.Len(t, iwr.Attachments, 1) + require.Len(t, iwr.Attachments[0].Fields, 1) } From 8e86ec969e0c3ce8d5dd0920f8262341ef893f5d Mon Sep 17 00:00:00 2001 From: Sheshagiri Rao Mallipedhi Date: Fri, 4 Oct 2019 09:43:27 -0700 Subject: [PATCH 05/22] =?UTF-8?q?refactor=20tests=20in=20model/reaction=5F?= =?UTF-8?q?test.go=20and=20move=20to=20testify=20t=E2=80=A6=20(#12603)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- model/reaction_test.go | 218 ++++++++++++++++++++++++++++------------- 1 file changed, 149 insertions(+), 69 deletions(-) diff --git a/model/reaction_test.go b/model/reaction_test.go index a357504775..53f40494d7 100644 --- a/model/reaction_test.go +++ b/model/reaction_test.go @@ -6,79 +6,159 @@ package model import ( "strings" "testing" + + "github.com/stretchr/testify/require" ) func TestReactionIsValid(t *testing.T) { - reaction := Reaction{ - UserId: NewId(), - PostId: NewId(), - EmojiName: "emoji", - CreateAt: GetMillis(), + tests := []struct { + // reaction + reaction Reaction + // error message to print + errMsg string + // should there be an error + shouldErr bool + }{ + { + reaction: Reaction{ + UserId: NewId(), + PostId: NewId(), + EmojiName: "emoji", + CreateAt: GetMillis(), + }, + errMsg: "", + shouldErr: false, + }, + { + reaction: Reaction{ + UserId: "", + PostId: NewId(), + EmojiName: "emoji", + CreateAt: GetMillis(), + }, + errMsg: "user id should be invalid", + shouldErr: true, + }, + { + reaction: Reaction{ + UserId: "1234garbage", + PostId: NewId(), + EmojiName: "emoji", + CreateAt: GetMillis(), + }, + errMsg: "user id should be invalid", + shouldErr: true, + }, + { + reaction: Reaction{ + UserId: NewId(), + PostId: "", + EmojiName: "emoji", + CreateAt: GetMillis(), + }, + errMsg: "post id should be invalid", + shouldErr: true, + }, + { + reaction: Reaction{ + UserId: NewId(), + PostId: "1234garbage", + EmojiName: "emoji", + CreateAt: GetMillis(), + }, + errMsg: "post id should be invalid", + shouldErr: true, + }, + { + reaction: Reaction{ + UserId: NewId(), + PostId: NewId(), + EmojiName: strings.Repeat("a", 64), + CreateAt: GetMillis(), + }, + errMsg: "", + shouldErr: false, + }, + { + reaction: Reaction{ + UserId: NewId(), + PostId: NewId(), + EmojiName: "emoji-", + CreateAt: GetMillis(), + }, + errMsg: "", + shouldErr: false, + }, + { + reaction: Reaction{ + UserId: NewId(), + PostId: NewId(), + EmojiName: "emoji_", + CreateAt: GetMillis(), + }, + errMsg: "", + shouldErr: false, + }, + { + reaction: Reaction{ + UserId: NewId(), + PostId: NewId(), + EmojiName: "+1", + CreateAt: GetMillis(), + }, + errMsg: "", + shouldErr: false, + }, + { + reaction: Reaction{ + UserId: NewId(), + PostId: NewId(), + EmojiName: "emoji:", + CreateAt: GetMillis(), + }, + errMsg: "", + shouldErr: true, + }, + { + reaction: Reaction{ + UserId: NewId(), + PostId: NewId(), + EmojiName: "", + CreateAt: GetMillis(), + }, + errMsg: "emoji name should be invalid", + shouldErr: true, + }, + { + reaction: Reaction{ + UserId: NewId(), + PostId: NewId(), + EmojiName: strings.Repeat("a", 65), + CreateAt: GetMillis(), + }, + errMsg: "emoji name should be invalid", + shouldErr: true, + }, + { + reaction: Reaction{ + UserId: NewId(), + PostId: NewId(), + EmojiName: "emoji", + CreateAt: 0, + }, + errMsg: "create at should be invalid", + shouldErr: true, + }, } - if err := reaction.IsValid(); err != nil { - t.Fatal(err) - } - - reaction.UserId = "" - if err := reaction.IsValid(); err == nil { - t.Fatal("user id should be invalid") - } - - reaction.UserId = "1234garbage" - if err := reaction.IsValid(); err == nil { - t.Fatal("user id should be invalid") - } - - reaction.UserId = NewId() - reaction.PostId = "" - if err := reaction.IsValid(); err == nil { - t.Fatal("post id should be invalid") - } - - reaction.PostId = "1234garbage" - if err := reaction.IsValid(); err == nil { - t.Fatal("post id should be invalid") - } - - reaction.PostId = NewId() - reaction.EmojiName = strings.Repeat("a", 64) - if err := reaction.IsValid(); err != nil { - t.Fatal(err) - } - - reaction.EmojiName = "emoji-" - if err := reaction.IsValid(); err != nil { - t.Fatal(err) - } - - reaction.EmojiName = "emoji_" - if err := reaction.IsValid(); err != nil { - t.Fatal(err) - } - - reaction.EmojiName = "+1" - if err := reaction.IsValid(); err != nil { - t.Fatal(err) - } - - reaction.EmojiName = "emoji:" - if err := reaction.IsValid(); err == nil { - t.Fatal(err) - } - - reaction.EmojiName = "" - if err := reaction.IsValid(); err == nil { - t.Fatal("emoji name should be invalid") - } - - reaction.EmojiName = strings.Repeat("a", 65) - if err := reaction.IsValid(); err == nil { - t.Fatal("emoji name should be invalid") - } - - reaction.CreateAt = 0 - if err := reaction.IsValid(); err == nil { - t.Fatal("create at should be invalid") + for _, test := range tests { + err := test.reaction.IsValid() + if test.shouldErr { + // there should be an error here + require.NotNil(t, err, test.errMsg) + } else { + // err should be nil here + require.Nil(t, err, test.errMsg) + } } } From 51b5776fd0c14e18743251e1755da12a5336f17c Mon Sep 17 00:00:00 2001 From: Vitalii Ananichev Date: Fri, 4 Oct 2019 20:03:07 +0300 Subject: [PATCH 06/22] =?UTF-8?q?MM-19133=20Migrate=20tests=20from=20"mode?= =?UTF-8?q?l/terms=5Fof=5Fservice=5Ftest.go"=20t=E2=80=A6=20(#12578)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- model/terms_of_service_test.go | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/model/terms_of_service_test.go b/model/terms_of_service_test.go index b9f19d0c16..c52dfb28ea 100644 --- a/model/terms_of_service_test.go +++ b/model/terms_of_service_test.go @@ -13,39 +13,25 @@ import ( func TestTermsOfServiceIsValid(t *testing.T) { s := TermsOfService{} - if err := s.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.Error(t, s.IsValid(), "should be invalid") s.Id = NewId() - if err := s.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.Error(t, s.IsValid(), "should be invalid") s.CreateAt = GetMillis() - if err := s.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.Error(t, s.IsValid(), "should be invalid") s.UserId = NewId() - if err := s.IsValid(); err != nil { - t.Fatal("should be invalid") - } + assert.Error(t, s.IsValid(), "should be invalid") s.Text = strings.Repeat("0", POST_MESSAGE_MAX_RUNES_V2+1) - if err := s.IsValid(); err == nil { - t.Fatal("should be invalid") - } + assert.Error(t, s.IsValid(), "should be invalid") s.Text = strings.Repeat("0", POST_MESSAGE_MAX_RUNES_V2) - if err := s.IsValid(); err != nil { - t.Fatal(err) - } + assert.Nil(t, s.IsValid(), "should be valid") s.Text = "test" - if err := s.IsValid(); err != nil { - t.Fatal(err) - } + assert.Nil(t, s.IsValid(), "should be valid") } func TestTermsOfServiceJson(t *testing.T) { From cbb25838a61872b624ac512556d7bc932486a64c Mon Sep 17 00:00:00 2001 From: Karan Nadagoudar Date: Sat, 5 Oct 2019 11:31:50 +0530 Subject: [PATCH 07/22] =?UTF-8?q?[MM-19165]=20Migrate=20tests=20from=20"mo?= =?UTF-8?q?del/utils=5Ftest.go"=20to=20use=20tes=E2=80=A6=20(#12610)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- model/utils_test.go | 45 ++++++++++++--------------------------------- 1 file changed, 12 insertions(+), 33 deletions(-) diff --git a/model/utils_test.go b/model/utils_test.go index 22dc72859a..e3967f5b86 100644 --- a/model/utils_test.go +++ b/model/utils_test.go @@ -18,18 +18,14 @@ import ( func TestNewId(t *testing.T) { for i := 0; i < 1000; i++ { id := NewId() - if len(id) > 26 { - t.Fatal("ids shouldn't be longer than 26 chars") - } + require.LessOrEqual(t, len(id), 26, "ids shouldn't be longer than 26 chars") } } func TestRandomString(t *testing.T) { for i := 0; i < 1000; i++ { r := NewRandomString(32) - if len(r) != 32 { - t.Fatal("should be 32 chars") - } + require.Len(t, r, 32) } } @@ -39,9 +35,7 @@ func TestGetMillisForTime(t *testing.T) { result := GetMillisForTime(thisTime) - if thisTimeMillis != result { - t.Fatalf(fmt.Sprintf("millis are not the same: %d and %d", thisTimeMillis, result)) - } + require.Equalf(t, thisTimeMillis, result, "millis are not the same: %d and %d", thisTimeMillis, result) } func TestPadDateStringZeros(t *testing.T) { @@ -100,14 +94,10 @@ func TestMapJson(t *testing.T) { rm := MapFromJson(strings.NewReader(json)) - if rm["id"] != "test_id" { - t.Fatal("map should be valid") - } + require.Equal(t, rm["id"], "test_id", "map should be valid") rm2 := MapFromJson(strings.NewReader("")) - if len(rm2) > 0 { - t.Fatal("make should be ivalid") - } + require.LessOrEqual(t, len(rm2), 0, "make should be ivalid") } func TestIsValidEmail(t *testing.T) { @@ -295,9 +285,8 @@ func TestStringArray_Equal(t *testing.T) { func TestParseHashtags(t *testing.T) { for input, output := range hashtags { - if o, _ := ParseHashtags(input); o != output { - t.Fatal("failed to parse hashtags from input=" + input + " expected=" + output + " actual=" + o) - } + o, _ := ParseHashtags(input) + require.Equal(t, o, output, "failed to parse hashtags from input="+input+" expected="+output+" actual="+o) } } @@ -350,16 +339,12 @@ func TestIsValidAlphaNum(t *testing.T) { for _, tc := range cases { actual := IsValidAlphaNum(tc.Input) - if actual != tc.Result { - t.Fatalf("case: %v\tshould returned: %#v", tc, tc.Result) - } + require.Equalf(t, actual, tc.Result, "case: %v\tshould returned: %#v", tc, tc.Result) } } func TestGetServerIpAddress(t *testing.T) { - if len(GetServerIpAddress("")) == 0 { - t.Fatal("Should find local ip address") - } + require.NotEmpty(t, GetServerIpAddress(""), "Should find local ip address") } func TestIsValidAlphaNumHyphenUnderscore(t *testing.T) { @@ -419,9 +404,7 @@ func TestIsValidAlphaNumHyphenUnderscore(t *testing.T) { for _, tc := range casesWithFormat { actual := IsValidAlphaNumHyphenUnderscore(tc.Input, true) - if actual != tc.Result { - t.Fatalf("case: %v\tshould returned: %#v", tc, tc.Result) - } + require.Equalf(t, actual, tc.Result, "case: %v\tshould returned: %#v", tc, tc.Result) } casesWithoutFormat := []struct { @@ -489,9 +472,7 @@ func TestIsValidAlphaNumHyphenUnderscore(t *testing.T) { for _, tc := range casesWithoutFormat { actual := IsValidAlphaNumHyphenUnderscore(tc.Input, false) - if actual != tc.Result { - t.Fatalf("case: '%v'\tshould returned: %#v", tc.Input, tc.Result) - } + require.Equalf(t, actual, tc.Result, "case: '%v'\tshould returned: %#v", tc.Input, tc.Result) } } @@ -524,9 +505,7 @@ func TestIsValidId(t *testing.T) { for _, tc := range cases { actual := IsValidId(tc.Input) - if actual != tc.Result { - t.Fatalf("case: %v\tshould returned: %#v", tc, tc.Result) - } + require.Equalf(t, actual, tc.Result, "case: %v\tshould returned: %#v", tc, tc.Result) } } From 1a126b5133e5ec547159df445ad3dc3132f49780 Mon Sep 17 00:00:00 2001 From: Marc Argent Date: Sun, 6 Oct 2019 14:01:42 +0100 Subject: [PATCH 08/22] GH-12335 Refactor app/session.go to use structured logging (#12529) * GH-12335 Refactor app/session.go to use structured logging * Remove placeholders --- app/session.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/app/session.go b/app/session.go index 697b0e3ac4..dcfde84102 100644 --- a/app/session.go +++ b/app/session.go @@ -4,7 +4,6 @@ package app import ( - "fmt" "net/http" "github.com/mattermost/mattermost-server/mlog" @@ -96,7 +95,7 @@ func (a *App) GetSessions(userId string) ([]*model.Session, *model.AppError) { func (a *App) UpdateSessionsIsGuest(userId string, isGuest bool) { sessions, err := a.Srv.Store.Session().GetSessions(userId) if err != nil { - mlog.Error(fmt.Sprintf("Unable to get user sessions: userId=%s err=%s", userId, err.Error())) + mlog.Error("Unable to get user sessions", mlog.String("user_id", userId), mlog.Err(err)) } for _, session := range sessions { @@ -107,7 +106,7 @@ func (a *App) UpdateSessionsIsGuest(userId string, isGuest bool) { } err := a.Srv.Store.Session().UpdateProps(session) if err != nil { - mlog.Error(fmt.Sprintf("Unable to update isGuest session: %s", err.Error())) + mlog.Error("Unable to update isGuest session", mlog.Err(err)) continue } a.AddSessionToCache(session) @@ -214,7 +213,7 @@ func (a *App) RevokeSessionsForDeviceId(userId string, deviceId string, currentS } for _, session := range sessions { if session.DeviceId == deviceId && session.Id != currentSessionId { - mlog.Debug(fmt.Sprintf("Revoking sessionId=%v for userId=%v re-login with same device Id", session.Id, userId), mlog.String("user_id", userId)) + mlog.Debug("Revoking sessionId for userId. Re-login with the same device Id", mlog.String("session_id", session.Id), mlog.String("user_id", userId)) if err := a.RevokeSession(session); err != nil { // Soft error so we still remove the other sessions mlog.Error(err.Error()) @@ -279,7 +278,7 @@ func (a *App) UpdateLastActivityAtIfNeeded(session model.Session) { } if err := a.Srv.Store.Session().UpdateLastActivityAt(session.Id, now); err != nil { - mlog.Error(fmt.Sprintf("Failed to update LastActivityAt for user_id=%v and session_id=%v, err=%v", session.UserId, session.Id, err), mlog.String("user_id", session.UserId)) + mlog.Error("Failed to update LastActivityAt", mlog.String("user_id", session.UserId), mlog.String("session_id", session.Id), mlog.Err(err)) } session.LastActivityAt = now From 6c55f79ca721a2d4d01172a1135d7adb2d5146db Mon Sep 17 00:00:00 2001 From: Deepak Sah Date: Mon, 7 Oct 2019 00:52:30 +0530 Subject: [PATCH 09/22] [MM-19119] Migrate tests from "model/access_test.go" to use testify (#12611) * Changed tests to use require * Removed redundant messages --- model/access_test.go | 44 +++++++++++--------------------------------- 1 file changed, 11 insertions(+), 33 deletions(-) diff --git a/model/access_test.go b/model/access_test.go index 0f124a1072..775de8cf72 100644 --- a/model/access_test.go +++ b/model/access_test.go @@ -20,9 +20,7 @@ func TestAccessJson(t *testing.T) { json := a1.ToJson() ra1 := AccessDataFromJson(strings.NewReader(json)) - if a1.Token != ra1.Token { - t.Fatal("tokens didn't match") - } + require.Equal(t, a1.Token, ra1.Token) } func TestAccessIsValid(t *testing.T) { @@ -31,61 +29,41 @@ func TestAccessIsValid(t *testing.T) { require.NotNil(t, ad.IsValid()) ad.ClientId = NewRandomString(28) - if err := ad.IsValid(); err == nil { - t.Fatal("Should have failed Client Id") - } + require.Error(t, ad.IsValid()) ad.ClientId = "" - if err := ad.IsValid(); err == nil { - t.Fatal("Should have failed Client Id") - } + require.Error(t, ad.IsValid()) ad.ClientId = NewId() require.NotNil(t, ad.IsValid()) ad.UserId = NewRandomString(28) - if err := ad.IsValid(); err == nil { - t.Fatal("Should have failed User Id") - } + require.Error(t, ad.IsValid()) ad.UserId = "" - if err := ad.IsValid(); err == nil { - t.Fatal("Should have failed User Id") - } + require.Error(t, ad.IsValid()) ad.UserId = NewId() - if err := ad.IsValid(); err == nil { - t.Fatal("should have failed") - } + require.Error(t, ad.IsValid()) ad.Token = NewRandomString(22) - if err := ad.IsValid(); err == nil { - t.Fatal("Should have failed Token") - } + require.Error(t, ad.IsValid()) ad.Token = NewId() require.NotNil(t, ad.IsValid()) ad.RefreshToken = NewRandomString(28) - if err := ad.IsValid(); err == nil { - t.Fatal("Should have failed Refresh Token") - } + require.Error(t, ad.IsValid()) ad.RefreshToken = NewId() require.NotNil(t, ad.IsValid()) ad.RedirectUri = "" - if err := ad.IsValid(); err == nil { - t.Fatal("Should have failed Redirect URI not set") - } + require.Error(t, ad.IsValid()) ad.RedirectUri = NewRandomString(28) - if err := ad.IsValid(); err == nil { - t.Fatal("Should have failed invalid URL") - } + require.Error(t, ad.IsValid()) ad.RedirectUri = "http://example.com" - if err := ad.IsValid(); err != nil { - t.Fatal(err) - } + require.Error(t, ad.IsValid(), ad.IsValid()) } From 2ef5712a08b96910cd0f8a4a8ac4ceadb86ecd25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Collin?= Date: Mon, 7 Oct 2019 07:54:51 +0200 Subject: [PATCH 10/22] =?UTF-8?q?[MM-19120]=20Migrate=20tests=20from=20"mo?= =?UTF-8?q?del/security=5Fbulletin=5Ftest.go=E2=80=A6=20(#12567)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- model/security_bulletin_test.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/model/security_bulletin_test.go b/model/security_bulletin_test.go index a6e55fe1c4..356f405127 100644 --- a/model/security_bulletin_test.go +++ b/model/security_bulletin_test.go @@ -4,6 +4,7 @@ package model import ( + "github.com/stretchr/testify/require" "strings" "testing" ) @@ -17,16 +18,12 @@ func TestSecurityBulletinToFromJson(t *testing.T) { j := b.ToJson() b1 := SecurityBulletinFromJson(strings.NewReader(j)) - CheckString(t, b1.AppliesToVersion, b.AppliesToVersion) - CheckString(t, b1.Id, b.Id) + require.Equal(t, b, *b1) // Malformed JSON s2 := `{"wat"` b2 := SecurityBulletinFromJson(strings.NewReader(s2)) - - if b2 != nil { - t.Fatal("expected nil") - } + require.Nil(t, b2) } func TestSecurityBulletinsToFromJson(t *testing.T) { @@ -45,11 +42,11 @@ func TestSecurityBulletinsToFromJson(t *testing.T) { b1 := SecurityBulletinsFromJson(strings.NewReader(j)) - CheckInt(t, len(b1), 2) + require.Len(t, b1, 2) // Malformed JSON s2 := `{"wat"` b2 := SecurityBulletinsFromJson(strings.NewReader(s2)) - CheckInt(t, len(b2), 0) + require.Len(t, b2, 0) } From bb203bad4a8606b7709518080e354019eb977849 Mon Sep 17 00:00:00 2001 From: Victor Hugo Avelar Ossorio Date: Mon, 7 Oct 2019 15:27:53 +0200 Subject: [PATCH 11/22] MM-19163 - Refactored team search tests to testify (#12607) --- model/team_search_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/model/team_search_test.go b/model/team_search_test.go index 0f118c9ec2..e48896fa56 100644 --- a/model/team_search_test.go +++ b/model/team_search_test.go @@ -6,6 +6,8 @@ package model import ( "strings" "testing" + + "github.com/stretchr/testify/assert" ) func TestTeamSearchJson(t *testing.T) { @@ -13,7 +15,5 @@ func TestTeamSearchJson(t *testing.T) { json := teamSearch.ToJson() rteamSearch := ChannelSearchFromJson(strings.NewReader(json)) - if teamSearch.Term != rteamSearch.Term { - t.Fatal("Terms do not match") - } + assert.Equal(t, teamSearch.Term, rteamSearch.Term, "Terms do not match") } From f21b7618a4821fab5e40639dc8013fbace7832b0 Mon Sep 17 00:00:00 2001 From: Ogundele Olumide Date: Tue, 8 Oct 2019 07:09:39 +0100 Subject: [PATCH 12/22] =?UTF-8?q?MM-19015=20Migrate=20tests=20from=20"cmd/?= =?UTF-8?q?mattermost/commands/config=5Fte=E2=80=A6=20(#12627)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - change t.fatal to require package of the testify toolkit --- cmd/mattermost/commands/config_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cmd/mattermost/commands/config_test.go b/cmd/mattermost/commands/config_test.go index 607f4f4a70..1bd6c63c0a 100644 --- a/cmd/mattermost/commands/config_test.go +++ b/cmd/mattermost/commands/config_test.go @@ -453,9 +453,7 @@ func TestUpdateMap(t *testing.T) { t.Run(test.Name, func(t *testing.T) { err := UpdateMap(configMap, test.configSettings, test.newVal) - if err != nil { - t.Fatal("Wasn't expecting an error: ", err) - } + require.Nil(t, err, "Wasn't expecting an error") if !contains(configMap, test.expected, test.configSettings) { t.Error("update didn't happen") From 041c914de70e996b944c36cf4f69759b0a0fbb9f Mon Sep 17 00:00:00 2001 From: Aliaksandr Kantsevoi Date: Tue, 8 Oct 2019 08:27:53 +0200 Subject: [PATCH 13/22] =?UTF-8?q?Migrate=20tests=20from=20"model/switch=5F?= =?UTF-8?q?request=5Ftest.go"=20to=20use=20testi=E2=80=A6=20(#12554)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- model/switch_request_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/model/switch_request_test.go b/model/switch_request_test.go index 49302fba67..bbd33e66e8 100644 --- a/model/switch_request_test.go +++ b/model/switch_request_test.go @@ -6,6 +6,8 @@ package model import ( "strings" "testing" + + "github.com/stretchr/testify/require" ) func TestSwitchRequestJson(t *testing.T) { @@ -13,7 +15,5 @@ func TestSwitchRequestJson(t *testing.T) { json := o.ToJson() ro := SwitchRequestFromJson(strings.NewReader(json)) - if o.Email != ro.Email { - t.Fatal("Emails do not match") - } + require.Equal(t, o.Email, ro.Email, "Emails do not match") } From e5ea44afcbc3236dc2c4499bedd8269acf859882 Mon Sep 17 00:00:00 2001 From: boonwj <3538312+boonwj@users.noreply.github.com> Date: Tue, 8 Oct 2019 08:02:50 +0100 Subject: [PATCH 14/22] [MM-19158] Migrate to testify in model/cluster_message_test.go (#12615) --- model/cluster_message_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/model/cluster_message_test.go b/model/cluster_message_test.go index e9225a5c50..ba02eed78c 100644 --- a/model/cluster_message_test.go +++ b/model/cluster_message_test.go @@ -22,7 +22,6 @@ func TestClusterMessage(t *testing.T) { require.Equal(t, "hello", result.Data) badresult := ClusterMessageFromJson(strings.NewReader("junk")) - if badresult != nil { - t.Fatal("should not have parsed") - } + + require.Nil(t, badresult, "should not have parsed") } From ae3f112874fc100ea2952b3ad5b99053c53cfd24 Mon Sep 17 00:00:00 2001 From: Carlos Tadeu Panato Junior Date: Tue, 8 Oct 2019 09:32:51 +0200 Subject: [PATCH 15/22] upgrade db to 5.16 (#12260) * upgrade db to 5.16 * set current schema version --- store/sqlstore/upgrade.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/store/sqlstore/upgrade.go b/store/sqlstore/upgrade.go index b6e43f20c8..1247e3cc78 100644 --- a/store/sqlstore/upgrade.go +++ b/store/sqlstore/upgrade.go @@ -20,7 +20,7 @@ import ( ) const ( - CURRENT_SCHEMA_VERSION = VERSION_5_15_0 + CURRENT_SCHEMA_VERSION = VERSION_5_16_0 VERSION_5_16_0 = "5.16.0" VERSION_5_15_0 = "5.15.0" VERSION_5_14_0 = "5.14.0" @@ -736,15 +736,12 @@ func UpgradeDatabaseToVersion515(sqlStore SqlStore) { } func UpgradeDatabaseToVersion516(sqlStore SqlStore) { - // TODO: Uncomment following condition when version 5.16.0 is released - // if shouldPerformUpgrade(sqlStore, VERSION_5_15_0, VERSION_5_16_0) { - - if sqlStore.DriverName() == model.DATABASE_DRIVER_POSTGRES { - sqlStore.GetMaster().Exec("ALTER TABLE Tokens ALTER COLUMN Extra TYPE varchar(2048)") - } else if sqlStore.DriverName() == model.DATABASE_DRIVER_MYSQL { - sqlStore.GetMaster().Exec("ALTER TABLE Tokens MODIFY Extra text") + if shouldPerformUpgrade(sqlStore, VERSION_5_15_0, VERSION_5_16_0) { + if sqlStore.DriverName() == model.DATABASE_DRIVER_POSTGRES { + sqlStore.GetMaster().Exec("ALTER TABLE Tokens ALTER COLUMN Extra TYPE varchar(2048)") + } else if sqlStore.DriverName() == model.DATABASE_DRIVER_MYSQL { + sqlStore.GetMaster().Exec("ALTER TABLE Tokens MODIFY Extra text") + } + saveSchemaVersion(sqlStore, VERSION_5_16_0) } - - // saveSchemaVersion(sqlStore, VERSION_5_16_0) - // } } From 488ae1abad798f7937847261fadba3ee0090b26d Mon Sep 17 00:00:00 2001 From: Pradeep Murugesan Date: Tue, 8 Oct 2019 08:57:20 +0100 Subject: [PATCH 16/22] turned off email notification for the status DND (#12502) * turned off email notification for the status DND * extracted the userAllows email logic as a separate function * removed the unused function --- app/notification.go | 65 ++++++++++++++------------- app/notification_test.go | 97 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 30 deletions(-) diff --git a/app/notification.go b/app/notification.go index 1dc1fdbd95..c3b5fa99b9 100644 --- a/app/notification.go +++ b/app/notification.go @@ -164,42 +164,13 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod continue } - userAllowsEmails := profileMap[id].NotifyProps[model.EMAIL_NOTIFY_PROP] != "false" - if channelEmail, ok := channelMemberNotifyPropsMap[id][model.EMAIL_NOTIFY_PROP]; ok { - if channelEmail != model.CHANNEL_NOTIFY_DEFAULT { - userAllowsEmails = channelEmail != "false" - } - } - - // Remove the user as recipient when the user has muted the channel. - if channelMuted, ok := channelMemberNotifyPropsMap[id][model.MARK_UNREAD_NOTIFY_PROP]; ok { - if channelMuted == model.CHANNEL_MARK_UNREAD_MENTION { - mlog.Debug("Channel muted for user", mlog.String("user_id", id), mlog.String("channel_mute", channelMuted)) - userAllowsEmails = false - } - } - //If email verification is required and user email is not verified don't send email. if *a.Config().EmailSettings.RequireEmailVerification && !profileMap[id].EmailVerified { mlog.Error("Skipped sending notification email, address not verified.", mlog.String("user_email", profileMap[id].Email), mlog.String("user_id", id)) continue } - var status *model.Status - var err *model.AppError - if status, err = a.GetStatus(id); err != nil { - status = &model.Status{ - UserId: id, - Status: model.STATUS_OFFLINE, - Manual: false, - LastActivityAt: 0, - ActiveChannel: "", - } - } - - autoResponderRelated := status.Status == model.STATUS_OUT_OF_OFFICE || post.Type == model.POST_AUTO_RESPONDER - - if userAllowsEmails && status.Status != model.STATUS_ONLINE && profileMap[id].DeleteAt == 0 && !autoResponderRelated { + if a.userAllowsEmail(profileMap[id], channelMemberNotifyPropsMap[id], post) { a.sendNotificationEmail(notification, profileMap[id], team) } } @@ -377,6 +348,40 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod return mentionedUsersList, nil } +func (a *App) userAllowsEmail(user *model.User, channelMemberNotificationProps model.StringMap, post *model.Post) bool { + userAllowsEmails := user.NotifyProps[model.EMAIL_NOTIFY_PROP] != "false" + if channelEmail, ok := channelMemberNotificationProps[model.EMAIL_NOTIFY_PROP]; ok { + if channelEmail != model.CHANNEL_NOTIFY_DEFAULT { + userAllowsEmails = channelEmail != "false" + } + } + + // Remove the user as recipient when the user has muted the channel. + if channelMuted, ok := channelMemberNotificationProps[model.MARK_UNREAD_NOTIFY_PROP]; ok { + if channelMuted == model.CHANNEL_MARK_UNREAD_MENTION { + mlog.Debug("Channel muted for user", mlog.String("user_id", user.Id), mlog.String("channel_mute", channelMuted)) + userAllowsEmails = false + } + } + + var status *model.Status + var err *model.AppError + if status, err = a.GetStatus(user.Id); err != nil { + status = &model.Status{ + UserId: user.Id, + Status: model.STATUS_OFFLINE, + Manual: false, + LastActivityAt: 0, + ActiveChannel: "", + } + } + + autoResponderRelated := status.Status == model.STATUS_OUT_OF_OFFICE || post.Type == model.POST_AUTO_RESPONDER + emailNotificationsAllowedForStatus := status.Status != model.STATUS_ONLINE && status.Status != model.STATUS_DND + + return userAllowsEmails && emailNotificationsAllowedForStatus && user.DeleteAt == 0 && !autoResponderRelated +} + // sendOutOfChannelMentions sends an ephemeral post to the sender of a post if any of the given potential mentions // are outside of the post's channel. Returns whether or not an ephemeral post was sent. func (a *App) sendOutOfChannelMentions(sender *model.User, post *model.Post, channel *model.Channel, potentialMentions []string) (bool, error) { diff --git a/app/notification_test.go b/app/notification_test.go index 13ee5ca588..5b08bffd2e 100644 --- a/app/notification_test.go +++ b/app/notification_test.go @@ -1789,3 +1789,100 @@ func TestGetNotificationNameFormat(t *testing.T) { assert.Equal(t, model.SHOW_USERNAME, th.App.GetNotificationNameFormat(th.BasicUser)) }) } + +func TestUserAllowsEmail(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + t.Run("should return true", func(t *testing.T) { + user := th.CreateUser() + + th.App.SetStatusOffline(user.Id, true) + + channelMemberNotificationProps := model.StringMap{ + model.EMAIL_NOTIFY_PROP: model.CHANNEL_NOTIFY_DEFAULT, + model.MARK_UNREAD_NOTIFY_PROP: model.CHANNEL_MARK_UNREAD_ALL, + } + + assert.True(t, th.App.userAllowsEmail(user, channelMemberNotificationProps, &model.Post{Type: "some-post-type"})) + }) + + t.Run("should return false in case the status is ONLINE", func(t *testing.T) { + user := th.CreateUser() + + th.App.SetStatusOnline(user.Id, true) + + channelMemberNotificationProps := model.StringMap{ + model.EMAIL_NOTIFY_PROP: model.CHANNEL_NOTIFY_DEFAULT, + model.MARK_UNREAD_NOTIFY_PROP: model.CHANNEL_MARK_UNREAD_ALL, + } + + assert.False(t, th.App.userAllowsEmail(user, channelMemberNotificationProps, &model.Post{Type: "some-post-type"})) + }) + + t.Run("should return false in case the EMAIL_NOTIFY_PROP is false", func(t *testing.T) { + user := th.CreateUser() + + th.App.SetStatusOffline(user.Id, true) + + channelMemberNotificationProps := model.StringMap{ + model.EMAIL_NOTIFY_PROP: "false", + model.MARK_UNREAD_NOTIFY_PROP: model.CHANNEL_MARK_UNREAD_ALL, + } + + assert.False(t, th.App.userAllowsEmail(user, channelMemberNotificationProps, &model.Post{Type: "some-post-type"})) + }) + + t.Run("should return false in case the MARK_UNREAD_NOTIFY_PROP is CHANNEL_MARK_UNREAD_MENTION", func(t *testing.T) { + user := th.CreateUser() + + th.App.SetStatusOffline(user.Id, true) + + channelMemberNotificationProps := model.StringMap{ + model.EMAIL_NOTIFY_PROP: model.CHANNEL_NOTIFY_DEFAULT, + model.MARK_UNREAD_NOTIFY_PROP: model.CHANNEL_MARK_UNREAD_MENTION, + } + + assert.False(t, th.App.userAllowsEmail(user, channelMemberNotificationProps, &model.Post{Type: "some-post-type"})) + }) + + t.Run("should return false in case the Post type is POST_AUTO_RESPONDER", func(t *testing.T) { + user := th.CreateUser() + + th.App.SetStatusOffline(user.Id, true) + + channelMemberNotificationProps := model.StringMap{ + model.EMAIL_NOTIFY_PROP: model.CHANNEL_NOTIFY_DEFAULT, + model.MARK_UNREAD_NOTIFY_PROP: model.CHANNEL_MARK_UNREAD_ALL, + } + + assert.False(t, th.App.userAllowsEmail(user, channelMemberNotificationProps, &model.Post{Type: model.POST_AUTO_RESPONDER})) + }) + + t.Run("should return false in case the status is STATUS_OUT_OF_OFFICE", func(t *testing.T) { + user := th.CreateUser() + + th.App.SetStatusOutOfOffice(user.Id) + + channelMemberNotificationProps := model.StringMap{ + model.EMAIL_NOTIFY_PROP: model.CHANNEL_NOTIFY_DEFAULT, + model.MARK_UNREAD_NOTIFY_PROP: model.CHANNEL_MARK_UNREAD_ALL, + } + + assert.False(t, th.App.userAllowsEmail(user, channelMemberNotificationProps, &model.Post{Type: model.POST_AUTO_RESPONDER})) + }) + + t.Run("should return false in case the status is STATUS_ONLINE", func(t *testing.T) { + user := th.CreateUser() + + th.App.SetStatusDoNotDisturb(user.Id) + + channelMemberNotificationProps := model.StringMap{ + model.EMAIL_NOTIFY_PROP: model.CHANNEL_NOTIFY_DEFAULT, + model.MARK_UNREAD_NOTIFY_PROP: model.CHANNEL_MARK_UNREAD_ALL, + } + + assert.False(t, th.App.userAllowsEmail(user, channelMemberNotificationProps, &model.Post{Type: model.POST_AUTO_RESPONDER})) + }) + +} From 598f7c7255cc815991e09e4d320502753a0bb341 Mon Sep 17 00:00:00 2001 From: Martin Kraft Date: Tue, 8 Oct 2019 08:03:39 -0400 Subject: [PATCH 17/22] MM-17767: Remove ExperimentalLdapGroupSync config. (#12388) --- app/diagnostics.go | 1 - model/config.go | 5 ----- tests/test-config.json | 1 - 3 files changed, 7 deletions(-) diff --git a/app/diagnostics.go b/app/diagnostics.go index 758b25ace4..bb7fd0fac5 100644 --- a/app/diagnostics.go +++ b/app/diagnostics.go @@ -296,7 +296,6 @@ func (a *App) trackConfig() { "experimental_strict_csrf_enforcement": *cfg.ServiceSettings.ExperimentalStrictCSRFEnforcement, "enable_email_invitations": *cfg.ServiceSettings.EnableEmailInvitations, "experimental_channel_organization": *cfg.ServiceSettings.ExperimentalChannelOrganization, - "experimental_ldap_group_sync": *cfg.ServiceSettings.ExperimentalLdapGroupSync, "disable_bots_when_owner_is_deactivated": *cfg.ServiceSettings.DisableBotsWhenOwnerIsDeactivated, "enable_bot_account_creation": *cfg.ServiceSettings.EnableBotAccountCreation, "enable_svgs": *cfg.ServiceSettings.EnableSVGs, diff --git a/model/config.go b/model/config.go index acbd3d6f60..81e594f03d 100644 --- a/model/config.go +++ b/model/config.go @@ -320,7 +320,6 @@ type ServiceSettings struct { DisableLegacyMFA *bool `restricted:"true"` ExperimentalStrictCSRFEnforcement *bool `restricted:"true"` EnableEmailInvitations *bool - ExperimentalLdapGroupSync *bool DisableBotsWhenOwnerIsDeactivated *bool `restricted:"true"` EnableBotAccountCreation *bool EnableSVGs *bool @@ -669,10 +668,6 @@ func (s *ServiceSettings) SetDefaults(isUpdate bool) { s.DisableLegacyMFA = NewBool(!isUpdate) } - if s.ExperimentalLdapGroupSync == nil { - s.ExperimentalLdapGroupSync = NewBool(false) - } - if s.ExperimentalStrictCSRFEnforcement == nil { s.ExperimentalStrictCSRFEnforcement = NewBool(false) } diff --git a/tests/test-config.json b/tests/test-config.json index a2a7ef3a7c..64738c872a 100644 --- a/tests/test-config.json +++ b/tests/test-config.json @@ -65,7 +65,6 @@ "ImageProxyOptions": "", "EnableAPITeamDeletion": false, "ExperimentalEnableHardenedMode": false, - "ExperimentalLdapGroupSync": true }, "TeamSettings": { "SiteName": "Mattermost", From 13b28b7a208ab0ba7e3ca99fcc10d2354a282fe6 Mon Sep 17 00:00:00 2001 From: Vitalii Ananichev Date: Tue, 8 Oct 2019 17:22:07 +0300 Subject: [PATCH 18/22] =?UTF-8?q?MM-19132=20Migrate=20tests=20from=20"mode?= =?UTF-8?q?l/channel=5Fview=5Ftest.go"=20to=20us=E2=80=A6=20(#12579)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Migrate tests from "model/channel_view_test.go" to use testify * Replace require to assert --- model/channel_view_test.go | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/model/channel_view_test.go b/model/channel_view_test.go index fac455a742..7f99133b80 100644 --- a/model/channel_view_test.go +++ b/model/channel_view_test.go @@ -6,6 +6,8 @@ package model import ( "strings" "testing" + + "github.com/stretchr/testify/assert" ) func TestChannelViewJson(t *testing.T) { @@ -13,13 +15,8 @@ func TestChannelViewJson(t *testing.T) { json := o.ToJson() ro := ChannelViewFromJson(strings.NewReader(json)) - if o.ChannelId != ro.ChannelId { - t.Fatal("ChannelIdIds do not match") - } - - if o.PrevChannelId != ro.PrevChannelId { - t.Fatal("PrevChannelIds do not match") - } + assert.Equal(t, o.ChannelId, ro.ChannelId, "ChannelIdIds do not match") + assert.Equal(t, o.PrevChannelId, ro.PrevChannelId, "PrevChannelIds do not match") } func TestChannelViewResponseJson(t *testing.T) { @@ -28,11 +25,6 @@ func TestChannelViewResponseJson(t *testing.T) { json := o.ToJson() ro := ChannelViewResponseFromJson(strings.NewReader(json)) - if o.Status != ro.Status { - t.Fatal("ChannelIdIds do not match") - } - - if o.LastViewedAtTimes[id] != ro.LastViewedAtTimes[id] { - t.Fatal("LastViewedAtTimes do not match") - } + assert.Equal(t, o.Status, ro.Status, "ChannelIdIds do not match") + assert.Equal(t, o.LastViewedAtTimes[id], ro.LastViewedAtTimes[id], "LastViewedAtTimes do not match") } From 894b124bc6aae414820e5e44bf906e637372b575 Mon Sep 17 00:00:00 2001 From: Sheshagiri Rao Mallipedhi Date: Tue, 8 Oct 2019 07:36:15 -0700 Subject: [PATCH 19/22] Migrate tests from "model/analytics_row_test.go" to use testify (#12601) --- model/analytics_row_test.go | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/model/analytics_row_test.go b/model/analytics_row_test.go index bd4e96c7dc..31ad461041 100644 --- a/model/analytics_row_test.go +++ b/model/analytics_row_test.go @@ -6,32 +6,23 @@ package model import ( "strings" "testing" + + "github.com/stretchr/testify/require" ) -func TestAnalyticsRowJson(t *testing.T) { - a1 := AnalyticsRow{} - a1.Name = "2015-10-12" - a1.Value = 12345.0 - json := a1.ToJson() - ra1 := AnalyticsRowFromJson(strings.NewReader(json)) +var a1 = AnalyticsRow{ + Name: "2015-10-12", + Value: 12345.0, +} - if a1.Name != ra1.Name { - t.Fatal("days didn't match") - } +func TestAnalyticsRowJson(t *testing.T) { + ra1 := AnalyticsRowFromJson(strings.NewReader(a1.ToJson())) + require.Equal(t, a1.Name, ra1.Name, "days didn't match") } func TestAnalyticsRowsJson(t *testing.T) { - a1 := AnalyticsRow{} - a1.Name = "2015-10-12" - a1.Value = 12345.0 - var a1s AnalyticsRows = make([]*AnalyticsRow, 1) a1s[0] = &a1 - - ljson := a1s.ToJson() - results := AnalyticsRowsFromJson(strings.NewReader(ljson)) - - if a1s[0].Name != results[0].Name { - t.Fatal("Ids do not match") - } + results := AnalyticsRowsFromJson(strings.NewReader(a1s.ToJson())) + require.Equal(t, a1s[0].Name, results[0].Name, "Ids do not match") } From d2905342975bd1e04a53b75a9fb229af3fb82ad6 Mon Sep 17 00:00:00 2001 From: Phillip Ahereza Date: Tue, 8 Oct 2019 17:39:44 +0300 Subject: [PATCH 20/22] =?UTF-8?q?Migrate=20tests=20from=20"cmd/mattermost/?= =?UTF-8?q?commands/roles=5Ftest.go"=20to=E2=80=A6=20(#12421)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * migrated tests in roles_test.go to use testify * format file * format file and edit some test statements * change some require statements to assert * remove extra t.Fatal * test error on response * use require.NotEmpty --- cmd/mattermost/commands/command_test.go | 11 ++++++----- cmd/mattermost/commands/roles_test.go | 23 +++++++++-------------- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/cmd/mattermost/commands/command_test.go b/cmd/mattermost/commands/command_test.go index 205ff3f126..d6594cf585 100644 --- a/cmd/mattermost/commands/command_test.go +++ b/cmd/mattermost/commands/command_test.go @@ -114,16 +114,17 @@ func TestCreateCommand(t *testing.T) { t.Run(testCase.Description, func(t *testing.T) { actual, _ := th.RunCommandWithOutput(t, testCase.Args...) - cmds, _ := th.SystemAdminClient.ListCommands(team.Id, true) + cmds, response := th.SystemAdminClient.ListCommands(team.Id, true) + + require.Nil(t, response.Error, "Failed to list commands") if testCase.ExpectedErr == "" { - if len(cmds) == 0 || cmds[0].Trigger != "testcmd" { - t.Fatal("Failed to create command") - } + require.NotEmpty(t, cmds, "Failed to create command") + require.Equal(t, "testcmd", cmds[0].Trigger) assert.Contains(t, string(actual), "PASS") } else { if len(cmds) > 1 { - t.Fatal("Created command that shouldn't have been created") + require.Fail(t, "Created command that shouldn't have been created") } assert.Contains(t, string(actual), testCase.ExpectedErr) } diff --git a/cmd/mattermost/commands/roles_test.go b/cmd/mattermost/commands/roles_test.go index 4a010458d1..a727cb6563 100644 --- a/cmd/mattermost/commands/roles_test.go +++ b/cmd/mattermost/commands/roles_test.go @@ -4,6 +4,8 @@ package commands import ( + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "testing" ) @@ -13,21 +15,14 @@ func TestAssignRole(t *testing.T) { th.CheckCommand(t, "roles", "system_admin", th.BasicUser.Email) - if user, err := th.App.Srv.Store.User().GetByEmail(th.BasicUser.Email); err != nil { - t.Fatal(err) - } else { - if user.Roles != "system_user system_admin" { - t.Fatal("Got wrong roles:", user.Roles) - } - } + user, err := th.App.Srv.Store.User().GetByEmail(th.BasicUser.Email) + require.Nil(t, err) + assert.Equal(t, "system_user system_admin", user.Roles) th.CheckCommand(t, "roles", "member", th.BasicUser.Email) - if user, err := th.App.Srv.Store.User().GetByEmail(th.BasicUser.Email); err != nil { - t.Fatal(err) - } else { - if user.Roles != "system_user" { - t.Fatal("Got wrong roles:", user.Roles, user.Id) - } - } + user, err = th.App.Srv.Store.User().GetByEmail(th.BasicUser.Email) + require.Nil(t, err) + assert.Equal(t, "system_user", user.Roles) + } From 7ed2ac52a0e1c56791bfdd7fc9a7a69b5d0dc6be Mon Sep 17 00:00:00 2001 From: Carlos Tadeu Panato Junior Date: Tue, 8 Oct 2019 16:50:51 +0200 Subject: [PATCH 21/22] build docker first (#12623) --- .circleci/config.yml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 5f7446f0ce..372c333cf3 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -298,15 +298,9 @@ workflows: # - check-i18n: # requires: # - setup - - test: - requires: - - setup - - test-schema: - requires: - - setup - build: requires: - - test + - setup - upload-s3-sha: context: mattermost-ci-s3 requires: @@ -319,3 +313,9 @@ workflows: context: matterbuild-docker requires: - upload-s3-sha + - test: + requires: + - setup + - test-schema: + requires: + - setup From a6fdb72b19c95c5b1083e307f9fb2a6e24e0eece Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Tue, 8 Oct 2019 19:07:56 +0200 Subject: [PATCH 22/22] Not allowing to use team invite links by guest accounts (#12608) * Not allowing to use team invite links by guest accounts * Adding needed tests * Updating error text --- api4/team.go | 5 +++++ api4/team_test.go | 24 +++++++++++++++++++++++- i18n/en.json | 4 ++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/api4/team.go b/api4/team.go index da7a8f2da6..06e130ae3e 100644 --- a/api4/team.go +++ b/api4/team.go @@ -499,6 +499,11 @@ func addUserToTeamFromInvite(c *Context, w http.ResponseWriter, r *http.Request) var member *model.TeamMember var err *model.AppError + if c.App.Session.Props[model.SESSION_PROP_IS_GUEST] == "true" { + c.Err = model.NewAppError("addUserToTeamFromInvite", "api.team.add_user_to_team_from_invite.guest.app_error", nil, "", http.StatusForbidden) + return + } + if len(tokenId) > 0 { member, err = c.App.AddTeamMemberByToken(c.App.Session.UserId, tokenId) } else if len(inviteId) > 0 { diff --git a/api4/team_test.go b/api4/team_test.go index 7dfda587c8..20bf698e18 100644 --- a/api4/team_test.go +++ b/api4/team_test.go @@ -1359,13 +1359,26 @@ func TestAddTeamMember(t *testing.T) { team := th.BasicTeam otherUser := th.CreateUser() + th.App.SetLicense(model.NewTestLicense("")) + defer th.App.SetLicense(nil) + + enableGuestAccounts := *th.App.Config().GuestAccountsSettings.Enable + defer func() { + th.App.UpdateConfig(func(cfg *model.Config) { cfg.GuestAccountsSettings.Enable = &enableGuestAccounts }) + }() + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.GuestAccountsSettings.Enable = true }) + + guest := th.CreateUser() + _, resp := th.SystemAdminClient.DemoteUserToGuest(guest.Id) + CheckNoError(t, resp) + if err := th.App.RemoveUserFromTeam(th.BasicTeam.Id, th.BasicUser2.Id, ""); err != nil { t.Fatalf(err.Error()) } // Regular user can't add a member to a team they don't belong to. th.LoginBasic2() - _, resp := Client.AddTeamMember(team.Id, otherUser.Id) + _, resp = Client.AddTeamMember(team.Id, otherUser.Id) CheckForbiddenStatus(t, resp) if resp.Error == nil { t.Fatalf("Error is nil") @@ -1506,6 +1519,15 @@ func TestAddTeamMember(t *testing.T) { CheckNotFoundStatus(t, resp) th.App.DeleteToken(token) + // by invite_id + th.App.SetLicense(model.NewTestLicense("")) + defer th.App.SetLicense(nil) + _, resp = Client.Login(guest.Email, guest.Password) + CheckNoError(t, resp) + + tm, resp = Client.AddTeamMemberFromInvite("", team.InviteId) + CheckForbiddenStatus(t, resp) + // by invite_id Client.Login(otherUser.Email, otherUser.Password) diff --git a/i18n/en.json b/i18n/en.json index a8a78b1019..209e7c67d3 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1874,6 +1874,10 @@ "id": "api.team.add_user_to_team.missing_parameter.app_error", "translation": "Parameter required to add user to team." }, + { + "id": "api.team.add_user_to_team_from_invite.guest.app_error", + "translation": "Guests are restricted from joining a team with a invite link. Please request a guest email invitation to the team." + }, { "id": "api.team.demote_user_to_guest.disabled.error", "translation": "Guest accounts are disabled."