From 4f7bd2200d93d00b57d53c005100f96e9b616818 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Mon, 21 Oct 2019 14:56:08 -0300 Subject: [PATCH 01/45] MM-18651: no spurious mentions on leaving channels (#12527) Today, the server emits a system message containing one of ` left the channel.` or ` removed from the channel.` when such an event occurs. While the client interprets this special kind of message to take into account the enduser's username display settings (e.g. to render `First Last left the channel.`), the server also processes this "message" to look for mentions. With the user in question no longer in the channel, it incorrectly splits usernames containing a period into two tokens and tries to mentions users accordingly. So `christopher.poile` ends up trying to mention `christopher` and `poile`. If such a user exists, they receive a spurious mention for this event. Change the message rendered by the server to explicitly prepend an `@`, which in turn is detected by the mention logic to avoid splitting. As above, this has no effect on the client-side rendered message. --- api4/channel_test.go | 79 ++++++++++++++++++++++++++++++++++++++++---- app/channel.go | 18 ++++++---- 2 files changed, 84 insertions(+), 13 deletions(-) diff --git a/api4/channel_test.go b/api4/channel_test.go index 893c69bfcc..77ebcf93d1 100644 --- a/api4/channel_test.go +++ b/api4/channel_test.go @@ -10,6 +10,7 @@ import ( "sort" "strconv" "strings" + "sync" "testing" "time" @@ -2502,15 +2503,79 @@ func TestRemoveChannelMember(t *testing.T) { _, resp = Client.RemoveUserFromChannel(th.BasicChannel.Id, th.BasicUser.Id) CheckForbiddenStatus(t, resp) - th.App.AddUserToChannel(th.BasicUser2, th.BasicChannel) - _, resp = Client.RemoveUserFromChannel(th.BasicChannel.Id, th.BasicUser2.Id) - CheckNoError(t, resp) + t.Run("success", func(t *testing.T) { + // Setup the system administrator to listen for websocket events from the channels. + th.LinkUserToTeam(th.SystemAdminUser, th.BasicTeam) + _, err := th.App.AddUserToChannel(th.SystemAdminUser, th.BasicChannel) + _, err = th.App.AddUserToChannel(th.SystemAdminUser, th.BasicChannel2) + require.Nil(t, err) + props := map[string]string{} + props[model.DESKTOP_NOTIFY_PROP] = model.CHANNEL_NOTIFY_ALL + _, resp = th.SystemAdminClient.UpdateChannelNotifyProps(th.BasicChannel.Id, th.SystemAdminUser.Id, props) + _, resp = th.SystemAdminClient.UpdateChannelNotifyProps(th.BasicChannel2.Id, th.SystemAdminUser.Id, props) + CheckNoError(t, resp) - _, resp = Client.RemoveUserFromChannel(th.BasicChannel2.Id, th.BasicUser.Id) - CheckNoError(t, resp) + wsClient, err := th.CreateWebSocketSystemAdminClient() + require.Nil(t, err) + wsClient.Listen() + var closeWsClient sync.Once + defer closeWsClient.Do(func() { + wsClient.Close() + }) - _, resp = th.SystemAdminClient.RemoveUserFromChannel(th.BasicChannel.Id, th.BasicUser.Id) - CheckNoError(t, resp) + wsr := <-wsClient.EventChannel + require.Equal(t, wsr.Event, model.WEBSOCKET_EVENT_HELLO) + + // requirePost listens for websocket events and tries to find the post matching + // the expected post's channel and message. + requirePost := func(expectedPost *model.Post) { + t.Helper() + for { + select { + case event := <-wsClient.EventChannel: + postData, ok := event.Data["post"] + if !ok { + continue + } + + post := model.PostFromJson(strings.NewReader(postData.(string))) + if post.ChannelId == expectedPost.ChannelId && post.Message == expectedPost.Message { + return + } + case <-time.After(5 * time.Second): + t.Fatal("failed to find expected post after 5 seconds") + return + } + } + } + + th.App.AddUserToChannel(th.BasicUser2, th.BasicChannel) + _, resp = Client.RemoveUserFromChannel(th.BasicChannel.Id, th.BasicUser2.Id) + CheckNoError(t, resp) + + requirePost(&model.Post{ + Message: fmt.Sprintf("@%s left the channel.", th.BasicUser2.Username), + ChannelId: th.BasicChannel.Id, + }) + + _, resp = Client.RemoveUserFromChannel(th.BasicChannel2.Id, th.BasicUser.Id) + CheckNoError(t, resp) + requirePost(&model.Post{ + Message: fmt.Sprintf("@%s removed from the channel.", th.BasicUser.Username), + ChannelId: th.BasicChannel2.Id, + }) + + _, resp = th.SystemAdminClient.RemoveUserFromChannel(th.BasicChannel.Id, th.BasicUser.Id) + CheckNoError(t, resp) + requirePost(&model.Post{ + Message: fmt.Sprintf("@%s removed from the channel.", th.BasicUser.Username), + ChannelId: th.BasicChannel.Id, + }) + + closeWsClient.Do(func() { + wsClient.Close() + }) + }) // Leave deleted channel th.LoginBasic() diff --git a/app/channel.go b/app/channel.go index 7021cd4192..aa0e54bbb8 100644 --- a/app/channel.go +++ b/app/channel.go @@ -1525,9 +1525,12 @@ func (a *App) LeaveChannel(channelId string, userId string) *model.AppError { func (a *App) postLeaveChannelMessage(user *model.User, channel *model.Channel) *model.AppError { post := &model.Post{ ChannelId: channel.Id, - Message: fmt.Sprintf(utils.T("api.channel.leave.left"), user.Username), - Type: model.POST_LEAVE_CHANNEL, - UserId: user.Id, + // Message here embeds `@username`, not just `username`, to ensure that mentions + // treat this as a username mention even though the user has now left the channel. + // The client renders its own system message, ignoring this value altogether. + Message: fmt.Sprintf(utils.T("api.channel.leave.left"), fmt.Sprintf("@%s", user.Username)), + Type: model.POST_LEAVE_CHANNEL, + UserId: user.Id, Props: model.StringInterface{ "username": user.Username, }, @@ -1595,9 +1598,12 @@ func (a *App) postAddToTeamMessage(user *model.User, addedUser *model.User, chan func (a *App) postRemoveFromChannelMessage(removerUserId string, removedUser *model.User, channel *model.Channel) *model.AppError { post := &model.Post{ ChannelId: channel.Id, - Message: fmt.Sprintf(utils.T("api.channel.remove_member.removed"), removedUser.Username), - Type: model.POST_REMOVE_FROM_CHANNEL, - UserId: removerUserId, + // Message here embeds `@username`, not just `username`, to ensure that mentions + // treat this as a username mention even though the user has now left the channel. + // The client renders its own system message, ignoring this value altogether. + Message: fmt.Sprintf(utils.T("api.channel.remove_member.removed"), fmt.Sprintf("@%s", removedUser.Username)), + Type: model.POST_REMOVE_FROM_CHANNEL, + UserId: removerUserId, Props: model.StringInterface{ "removedUserId": removedUser.Id, "removedUsername": removedUser.Username, From 94db07f65fa60b3d2e28377ba4bdd2ddee4e236c Mon Sep 17 00:00:00 2001 From: Shobhit Gupta Date: Mon, 21 Oct 2019 14:00:11 -0700 Subject: [PATCH 02/45] Migrate tests from "store/storetest/command_webhook_store.go" to use testify (#12773) * Migrate tests to use testify * Remove dangling semicolon * Fix failing test * Change from assert to require.Nil --- store/storetest/command_webhook_store.go | 49 ++++++++++-------------- 1 file changed, 20 insertions(+), 29 deletions(-) diff --git a/store/storetest/command_webhook_store.go b/store/storetest/command_webhook_store.go index 9fdcb319fe..77cef61999 100644 --- a/store/storetest/command_webhook_store.go +++ b/store/storetest/command_webhook_store.go @@ -4,14 +4,13 @@ package storetest import ( - "testing" - - "github.com/stretchr/testify/require" - "net/http" + "testing" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/store" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestCommandWebhookStore(t *testing.T, ss store.Store) { @@ -29,17 +28,12 @@ func testCommandWebhookStore(t *testing.T, ss store.Store) { require.Nil(t, err) var r1 *model.CommandWebhook - if r1, err = cws.Get(h1.Id); err != nil { - t.Fatal(err) - } else { - if *r1 != *h1 { - t.Fatal("invalid returned webhook") - } - } + r1, err = cws.Get(h1.Id) + require.Nil(t, err) + assert.Equal(t, *r1, *h1, "invalid returned webhook") - if _, err = cws.Get("123"); err.StatusCode != http.StatusNotFound { - t.Fatal("Should have set the status as not found for missing id") - } + _, err = cws.Get("123") + assert.Equal(t, err.StatusCode, http.StatusNotFound, "Should have set the status as not found for missing id") h2 := &model.CommandWebhook{} h2.CreateAt = model.GetMillis() - 2*model.COMMAND_WEBHOOK_LIFETIME @@ -49,25 +43,22 @@ func testCommandWebhookStore(t *testing.T, ss store.Store) { h2, err = cws.Save(h2) require.Nil(t, err) - if _, err := cws.Get(h2.Id); err == nil || err.StatusCode != http.StatusNotFound { - t.Fatal("Should have set the status as not found for expired webhook") - } + _, err = cws.Get(h2.Id) + require.NotNil(t, err, "Should have set the status as not found for expired webhook") + assert.Equal(t, err.StatusCode, http.StatusNotFound, "Should have set the status as not found for expired webhook") cws.Cleanup() - if _, err := cws.Get(h1.Id); err != nil { - t.Fatal("Should have no error getting unexpired webhook") - } + _, err = cws.Get(h1.Id) + require.Nil(t, err, "Should have no error getting unexpired webhook") - if _, err := cws.Get(h2.Id); err.StatusCode != http.StatusNotFound { - t.Fatal("Should have set the status as not found for expired webhook") - } + _, err = cws.Get(h2.Id) + assert.Equal(t, err.StatusCode, http.StatusNotFound, "Should have set the status as not found for expired webhook") - if err := cws.TryUse(h1.Id, 1); err != nil { - t.Fatal("Should be able to use webhook once") - } + err = cws.TryUse(h1.Id, 1) + require.Nil(t, err, "Should be able to use webhook once") - if err := cws.TryUse(h1.Id, 1); err == nil || err.StatusCode != http.StatusBadRequest { - t.Fatal("Should be able to use webhook once") - } + err = cws.TryUse(h1.Id, 1) + require.NotNil(t, err, "Should be able to use webhook once") + assert.Equal(t, err.StatusCode, http.StatusBadRequest, "Should be able to use webhook once") } From c12c585fb8a15012aebbdd48f00bbc97b3500f72 Mon Sep 17 00:00:00 2001 From: Christopher Poile Date: Tue, 22 Oct 2019 08:39:49 -0400 Subject: [PATCH 03/45] [MM-19526] /code is rendering HTML incorrectly (#12840) * text from /code command needs to be treated differently when processed * PR comments --- app/command.go | 22 ++++++++++++++++------ app/command_test.go | 19 ++++++++++++++++++- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/app/command.go b/app/command.go index c7f83c5b1e..678031dbbb 100644 --- a/app/command.go +++ b/app/command.go @@ -39,8 +39,13 @@ func GetCommandProvider(name string) CommandProvider { return nil } -func (a *App) CreateCommandPost(post *model.Post, teamId string, response *model.CommandResponse) (*model.Post, *model.AppError) { - post.Message = model.ParseSlackLinksToMarkdown(response.Text) +func (a *App) CreateCommandPost(post *model.Post, teamId string, response *model.CommandResponse, skipSlackParsing bool) (*model.Post, *model.AppError) { + if skipSlackParsing { + post.Message = response.Text + } else { + post.Message = model.ParseSlackLinksToMarkdown(response.Text) + } + post.CreateAt = model.GetMillis() if strings.HasPrefix(post.Type, model.POST_SYSTEM_MESSAGE_PREFIX) { @@ -430,11 +435,16 @@ func (a *App) HandleCommandResponsePost(command *model.Command, args *model.Comm post.AddProp("from_webhook", "true") } - // Process Slack text replacements - response.Text = a.ProcessSlackText(response.Text) - response.Attachments = a.ProcessSlackAttachments(response.Attachments) + // Do not process text if this is a code block + skipSlackParsing := command.Trigger == "code" - if _, err := a.CreateCommandPost(post, args.TeamId, response); err != nil { + // Process Slack text replacements + if !skipSlackParsing { + response.Text = a.ProcessSlackText(response.Text) + response.Attachments = a.ProcessSlackAttachments(response.Attachments) + } + + if _, err := a.CreateCommandPost(post, args.TeamId, response, skipSlackParsing); err != nil { return post, err } diff --git a/app/command_test.go b/app/command_test.go index 85e614d4f6..5f34935298 100644 --- a/app/command_test.go +++ b/app/command_test.go @@ -68,7 +68,8 @@ func TestCreateCommandPost(t *testing.T) { Text: "some message", } - _, err := th.App.CreateCommandPost(post, th.BasicTeam.Id, resp) + skipSlackParsing := false + _, err := th.App.CreateCommandPost(post, th.BasicTeam.Id, resp, skipSlackParsing) if err == nil || err.Id != "api.context.invalid_param.app_error" { t.Fatal("should have failed - bad post type") } @@ -205,7 +206,23 @@ func TestHandleCommandResponsePost(t *testing.T) { if err == nil || err.Id != "api.command.command_post.forbidden.app_error" { t.Fatal("should have failed - forbidden channel post") } + + // Test that /code text is not converted with the Slack text conversion. + command.Trigger = "code" + resp.ChannelId = "" + resp.Text = "" + resp.Attachments = []*model.SlackAttachment{ + &model.SlackAttachment{ + Text: "", + }, + } + post, err = th.App.HandleCommandResponsePost(command, args, resp, builtIn) + + assert.Nil(t, err) + assert.Equal(t, resp.Text, post.Message, "/code text should not be converted to Slack links") + assert.Equal(t, "", resp.Attachments[0].Text) } + func TestHandleCommandResponse(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() From 60572a00c83ecb896d27b29f5dcdfd17662c1edb Mon Sep 17 00:00:00 2001 From: Phillip Ahereza Date: Tue, 22 Oct 2019 15:40:50 +0300 Subject: [PATCH 04/45] Migrate tests from services/httpservice/client_test.go to testify (#12743) --- services/httpservice/client_test.go | 59 +++++++++++------------------ 1 file changed, 23 insertions(+), 36 deletions(-) diff --git a/services/httpservice/client_test.go b/services/httpservice/client_test.go index f24d5cce14..61c0132975 100644 --- a/services/httpservice/client_test.go +++ b/services/httpservice/client_test.go @@ -6,6 +6,7 @@ package httpservice import ( "context" "fmt" + "github.com/stretchr/testify/assert" "io/ioutil" "net" "net/http" @@ -35,9 +36,7 @@ func TestHTTPClient(t *testing.T) { } { _, err := c.Get(tc.URL) if !tc.IsInternal { - if err != nil { - t.Fatal("google is down?") - } + require.Nil(t, err, "google is down?") } else { allowed := !tc.IsInternal || allowInternal success := err == nil @@ -47,9 +46,7 @@ func TestHTTPClient(t *testing.T) { case *url.Error: success = e.Err != AddressForbidden } - if success != allowed { - t.Fatalf("failed for %v. allowed: %v, success %v", tc.URL, allowed, success) - } + require.Equalf(t, success, allowed, "failed for %v. allowed: %v, success %v", tc.URL, allowed, success) } } } @@ -64,18 +61,12 @@ func TestHTTPClientWithProxy(t *testing.T) { c.Transport.(*MattermostTransport).Transport.(*http.Transport).Proxy = http.ProxyURL(purl) resp, err := c.Get("http://acme.com") - if err != nil { - t.Fatal(err) - } + require.Nil(t, err) defer resp.Body.Close() body, err := ioutil.ReadAll(resp.Body) - if err != nil { - t.Fatal(err) - } - if string(body) != "proxy" { - t.FailNow() - } + require.Nil(t, err) + require.Equal(t, "proxy", string(body)) } func createProxyServer() *httptest.Server { @@ -113,11 +104,14 @@ func TestDialContextFilter(t *testing.T) { return nil, nil }, func(host string) bool { return host == "10.0.0.1" }, func(ip net.IP) bool { return !IsReservedIP(ip) }) _, err := filter(context.Background(), "", tc.Addr) - switch { - case tc.IsValid == (err == AddressForbidden) || (err != nil && err != AddressForbidden): - t.Errorf("unexpected err for %v (%v)", tc.Addr, err) - case tc.IsValid != didDial: - t.Errorf("unexpected didDial for %v", tc.Addr) + + if tc.IsValid { + require.Nil(t, err) + require.True(t, didDial) + } else { + require.NotNil(t, err) + require.Equal(t, err, AddressForbidden) + require.False(t, didDial) } } } @@ -127,19 +121,15 @@ func TestUserAgentIsSet(t *testing.T) { defaultUserAgent = testUserAgent ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { ua := req.UserAgent() - if ua == "" { - t.Error("expected user-agent to be non-empty") - } - if ua != testUserAgent { - t.Errorf("expected user-agent to be %q but was %q", testUserAgent, ua) - } + assert.NotEqual(t, "", ua, "expected user-agent to be non-empty") + assert.Equalf(t, testUserAgent, ua, "expected user-agent to be %q but was %q", testUserAgent, ua) })) defer ts.Close() client := NewHTTPClient(NewTransport(true, nil, nil)) req, err := http.NewRequest("GET", ts.URL, nil) - if err != nil { - t.Fatal("NewRequest failed", err) - } + + require.Nil(t, err, "NewRequest failed", err) + client.Do(req) } @@ -164,9 +154,8 @@ func TestIsReservedIP(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := IsReservedIP(tt.ip); got != tt.want { - t.Errorf("IsReservedIP() = %v, want %v", got, tt.want) - } + got := IsReservedIP(tt.ip) + assert.Equalf(t, tt.want, got, "IsReservedIP() = %v, want %v", got, tt.want) }) } } @@ -182,10 +171,8 @@ func TestIsOwnIP(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got, _ := IsOwnIP(tt.ip); got != tt.want { - t.Errorf("IsOwnIP() = %v, want %v", got, tt.want) - t.Errorf(tt.ip.String()) - } + got, _ := IsOwnIP(tt.ip) + assert.Equalf(t, tt.want, got, "IsOwnIP() = %v, want %v for IP %s", got, tt.want, tt.ip.String()) }) } } From c8814b4219c4de814f94a12104adfca966265e9e Mon Sep 17 00:00:00 2001 From: Rahul Verma Date: Tue, 22 Oct 2019 18:52:06 +0530 Subject: [PATCH 05/45] Converted store/storetest/bot_store.go t.Fatal calls into assert/require calls (#12838) --- store/storetest/bot_store.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/store/storetest/bot_store.go b/store/storetest/bot_store.go index 683eaf4e7b..37f07d45d2 100644 --- a/store/storetest/bot_store.go +++ b/store/storetest/bot_store.go @@ -175,13 +175,13 @@ func testBotStoreGetAll(t *testing.T, ss store.Store) { Email: MakeEmail(), Username: model.NewId(), } - if _, err := ss.User().Save(&deletedUser); err != nil { - t.Fatal("couldn't save user", err) - } + _, err1 := ss.User().Save(&deletedUser) + require.Nil(t, err1, "couldn't save user") + deletedUser.DeleteAt = model.GetMillis() - if _, err := ss.User().Update(&deletedUser, true); err != nil { - t.Fatal("couldn't delete user", err) - } + _, err2 := ss.User().Update(&deletedUser, true) + require.Nil(t, err2, "couldn't delete user") + defer func() { require.Nil(t, ss.User().PermanentDelete(deletedUser.Id)) }() ob5, _ := makeBotWithUser(t, ss, &model.Bot{ Username: "ob5", From fbf4d6d139475c290a2d149ce17f312e6752ff34 Mon Sep 17 00:00:00 2001 From: Nikhil Ranjan Date: Tue, 22 Oct 2019 15:47:15 +0200 Subject: [PATCH 06/45] Convert store/storetest/user_access_token_store.go t.Fatal calls into assert/require calls (#12801) * Convert store/storetest/user_access_token_store.go t.Fatal calls into assert/require calls * more t.Fatal Calls converted --- store/storetest/user_access_token_store.go | 127 ++++++++------------- 1 file changed, 50 insertions(+), 77 deletions(-) diff --git a/store/storetest/user_access_token_store.go b/store/storetest/user_access_token_store.go index 0a3c82f47c..0e4e6ce390 100644 --- a/store/storetest/user_access_token_store.go +++ b/store/storetest/user_access_token_store.go @@ -31,49 +31,36 @@ func testUserAccessTokenSaveGetDelete(t *testing.T, ss store.Store) { s1, err := ss.Session().Save(s1) require.Nil(t, err) - if _, err = ss.UserAccessToken().Save(uat); err != nil { - t.Fatal(err) - } + _, err = ss.UserAccessToken().Save(uat) + require.Nil(t, err) - if result, terr := ss.UserAccessToken().Get(uat.Id); terr != nil { - t.Fatal(terr) - } else if result.Token != uat.Token { - t.Fatal("received incorrect token after save") - } + result, terr := ss.UserAccessToken().Get(uat.Id) + require.Nil(t, terr) + require.Equal(t, result.Token, uat.Token, "received incorrect token after save") - if received, err2 := ss.UserAccessToken().GetByToken(uat.Token); err2 != nil { - t.Fatal(err2) - } else if received.Token != uat.Token { - t.Fatal("received incorrect token after save") - } + received, err2 := ss.UserAccessToken().GetByToken(uat.Token) + require.Nil(t, err2) + require.Equal(t, received.Token, uat.Token, "received incorrect token after save") - if _, err = ss.UserAccessToken().GetByToken("notarealtoken"); err == nil { - t.Fatal("should have failed on bad token") - } + _, err = ss.UserAccessToken().GetByToken("notarealtoken") + require.NotNil(t, err, "should have failed on bad token") - if received, err2 := ss.UserAccessToken().GetByUser(uat.UserId, 0, 100); err2 != nil { - t.Fatal(err2) - } else if len(received) != 1 { - t.Fatal("received incorrect number of tokens after save") - } + received2, err2 := ss.UserAccessToken().GetByUser(uat.UserId, 0, 100) + require.Nil(t, err2) + require.Equal(t, 1, len(received2), "received incorrect number of tokens after save") - if result, appError := ss.UserAccessToken().GetAll(0, 100); appError != nil { - t.Fatal(appError) - } else if len(result) != 1 { - t.Fatal("received incorrect number of tokens after save") - } + result2, appError := ss.UserAccessToken().GetAll(0, 100) + require.Nil(t, appError) + require.Equal(t, 1, len(result2), "received incorrect number of tokens after save") - if err = ss.UserAccessToken().Delete(uat.Id); err != nil { - t.Fatal(err) - } + err = ss.UserAccessToken().Delete(uat.Id) + require.Nil(t, err) - if _, err = ss.Session().Get(s1.Token); err == nil { - t.Fatal("should error - session should be deleted") - } + _, err = ss.Session().Get(s1.Token) + require.NotNil(t, err, "should error - session should be deleted") - if _, err = ss.UserAccessToken().GetByToken(s1.Token); err == nil { - t.Fatal("should error - access token should be deleted") - } + _, err = ss.UserAccessToken().GetByToken(s1.Token) + require.NotNil(t, err, "should error - access token should be deleted") s2 := &model.Session{} s2.UserId = uat.UserId @@ -82,21 +69,17 @@ func testUserAccessTokenSaveGetDelete(t *testing.T, ss store.Store) { s2, err = ss.Session().Save(s2) require.Nil(t, err) - if _, err = ss.UserAccessToken().Save(uat); err != nil { - t.Fatal(err) - } + _, err = ss.UserAccessToken().Save(uat) + require.Nil(t, err) - if err := ss.UserAccessToken().DeleteAllForUser(uat.UserId); err != nil { - t.Fatal(err) - } + err = ss.UserAccessToken().DeleteAllForUser(uat.UserId) + require.Nil(t, err) - if _, err := ss.Session().Get(s2.Token); err == nil { - t.Fatal("should error - session should be deleted") - } + _, err = ss.Session().Get(s2.Token) + require.NotNil(t, err, "should error - session should be deleted") - if _, err := ss.UserAccessToken().GetByToken(s2.Token); err == nil { - t.Fatal("should error - access token should be deleted") - } + _, err = ss.UserAccessToken().GetByToken(s2.Token) + require.NotNil(t, err, "should error - access token should be deleted") } func testUserAccessTokenDisableEnable(t *testing.T, ss store.Store) { @@ -113,17 +96,14 @@ func testUserAccessTokenDisableEnable(t *testing.T, ss store.Store) { s1, err := ss.Session().Save(s1) require.Nil(t, err) - if _, err = ss.UserAccessToken().Save(uat); err != nil { - t.Fatal(err) - } + _, err = ss.UserAccessToken().Save(uat) + require.Nil(t, err) - if err = ss.UserAccessToken().UpdateTokenDisable(uat.Id); err != nil { - t.Fatal(err) - } + err = ss.UserAccessToken().UpdateTokenDisable(uat.Id) + require.Nil(t, err) - if _, err = ss.Session().Get(s1.Token); err == nil { - t.Fatal("should error - session should be deleted") - } + _, err = ss.Session().Get(s1.Token) + require.NotNil(t, err, "should error - session should be deleted") s2 := &model.Session{} s2.UserId = uat.UserId @@ -132,9 +112,8 @@ func testUserAccessTokenDisableEnable(t *testing.T, ss store.Store) { s2, err = ss.Session().Save(s2) require.Nil(t, err) - if err = ss.UserAccessToken().UpdateTokenEnable(uat.Id); err != nil { - t.Fatal(err) - } + err = ss.UserAccessToken().UpdateTokenEnable(uat.Id) + require.Nil(t, err) } func testUserAccessTokenSearch(t *testing.T, ss store.Store) { @@ -158,25 +137,19 @@ func testUserAccessTokenSearch(t *testing.T, ss store.Store) { s1, err = ss.Session().Save(s1) require.Nil(t, err) - if _, err = ss.UserAccessToken().Save(uat); err != nil { - t.Fatal(err) - } + _, err = ss.UserAccessToken().Save(uat) + require.Nil(t, err) - if received, err := ss.UserAccessToken().Search(uat.Id); err != nil { - t.Fatal(err) - } else if len(received) != 1 { - t.Fatal("received incorrect number of tokens after search") - } + received, err := ss.UserAccessToken().Search(uat.Id) + require.Nil(t, err) - if received, err := ss.UserAccessToken().Search(uat.UserId); err != nil { - t.Fatal(err) - } else if len(received) != 1 { - t.Fatal("received incorrect number of tokens after search") - } + require.Equal(t, 1, len(received), "received incorrect number of tokens after search") - if received, err := ss.UserAccessToken().Search(u1.Username); err != nil { - t.Fatal(err) - } else if len(received) != 1 { - t.Fatal("received incorrect number of tokens after search") - } + received, err = ss.UserAccessToken().Search(uat.UserId) + require.Nil(t, err) + require.Equal(t, 1, len(received), "received incorrect number of tokens after search") + + received, err = ss.UserAccessToken().Search(u1.Username) + require.Nil(t, err) + require.Equal(t, 1, len(received), "received incorrect number of tokens after search") } From 9307f75fe9df82fbf9fa8cabdef20cce71e9ae16 Mon Sep 17 00:00:00 2001 From: Joram Wilander Date: Tue, 22 Oct 2019 14:38:08 -0400 Subject: [PATCH 07/45] Add some group plugin APIs (#12180) * Add group store GetByUser * Add group store GetByName * Add group plugin APIs * Minor naming fixes * Add minimum version comments --- app/group.go | 8 +++ app/plugin_api.go | 12 ++++ plugin/api.go | 15 ++++ plugin/client_rpc_generated.go | 87 +++++++++++++++++++++++ plugin/plugintest/api.go | 75 ++++++++++++++++++++ store/sqlstore/group_store.go | 32 +++++++++ store/store.go | 2 + store/storetest/group_store.go | 104 ++++++++++++++++++++++++++++ store/storetest/mocks/GroupStore.go | 50 +++++++++++++ store/timer_layer.go | 34 +++++++++ 10 files changed, 419 insertions(+) diff --git a/app/group.go b/app/group.go index e84cd34d9e..690e5bf582 100644 --- a/app/group.go +++ b/app/group.go @@ -11,6 +11,10 @@ func (a *App) GetGroup(id string) (*model.Group, *model.AppError) { return a.Srv.Store.Group().Get(id) } +func (a *App) GetGroupByName(name string) (*model.Group, *model.AppError) { + return a.Srv.Store.Group().GetByName(name) +} + func (a *App) GetGroupByRemoteID(remoteID string, groupSource model.GroupSource) (*model.Group, *model.AppError) { return a.Srv.Store.Group().GetByRemoteID(remoteID, groupSource) } @@ -19,6 +23,10 @@ func (a *App) GetGroupsBySource(groupSource model.GroupSource) ([]*model.Group, return a.Srv.Store.Group().GetAllBySource(groupSource) } +func (a *App) GetGroupsByUserId(userId string) ([]*model.Group, *model.AppError) { + return a.Srv.Store.Group().GetByUser(userId) +} + func (a *App) CreateGroup(group *model.Group) (*model.Group, *model.AppError) { return a.Srv.Store.Group().Create(group) } diff --git a/app/plugin_api.go b/app/plugin_api.go index dda5e1ae10..d228a88d0d 100644 --- a/app/plugin_api.go +++ b/app/plugin_api.go @@ -445,6 +445,18 @@ func (api *PluginAPI) DeleteChannelMember(channelId, userId string) *model.AppEr return api.app.LeaveChannel(channelId, userId) } +func (api *PluginAPI) GetGroup(groupId string) (*model.Group, *model.AppError) { + return api.app.GetGroup(groupId) +} + +func (api *PluginAPI) GetGroupByName(name string) (*model.Group, *model.AppError) { + return api.app.GetGroupByName(name) +} + +func (api *PluginAPI) GetGroupsForUser(userId string) ([]*model.Group, *model.AppError) { + return api.app.GetGroupsByUserId(userId) +} + func (api *PluginAPI) CreatePost(post *model.Post) (*model.Post, *model.AppError) { return api.app.CreatePostMissingChannel(post, true) } diff --git a/plugin/api.go b/plugin/api.go index 1afadc2b89..49ac0ebf8f 100644 --- a/plugin/api.go +++ b/plugin/api.go @@ -378,6 +378,21 @@ type API interface { // Minimum server version: 5.2 UpdateChannelMemberNotifications(channelId, userId string, notifications map[string]string) (*model.ChannelMember, *model.AppError) + // GetGroup gets a group by ID. + // + // Minimum server version: 5.18 + GetGroup(groupId string) (*model.Group, *model.AppError) + + // GetGroupByName gets a group by name. + // + // Minimum server version: 5.18 + GetGroupByName(name string) (*model.Group, *model.AppError) + + // GetGroupsForUser gets the groups a user is in. + // + // Minimum server version: 5.18 + GetGroupsForUser(userId string) ([]*model.Group, *model.AppError) + // DeleteChannelMember deletes a channel membership for a user. // // Minimum server version: 5.2 diff --git a/plugin/client_rpc_generated.go b/plugin/client_rpc_generated.go index c50730fbef..d91ae36db6 100644 --- a/plugin/client_rpc_generated.go +++ b/plugin/client_rpc_generated.go @@ -2498,6 +2498,93 @@ func (s *apiRPCServer) UpdateChannelMemberNotifications(args *Z_UpdateChannelMem return nil } +type Z_GetGroupArgs struct { + A string +} + +type Z_GetGroupReturns struct { + A *model.Group + B *model.AppError +} + +func (g *apiRPCClient) GetGroup(groupId string) (*model.Group, *model.AppError) { + _args := &Z_GetGroupArgs{groupId} + _returns := &Z_GetGroupReturns{} + if err := g.client.Call("Plugin.GetGroup", _args, _returns); err != nil { + log.Printf("RPC call to GetGroup API failed: %s", err.Error()) + } + return _returns.A, _returns.B +} + +func (s *apiRPCServer) GetGroup(args *Z_GetGroupArgs, returns *Z_GetGroupReturns) error { + if hook, ok := s.impl.(interface { + GetGroup(groupId string) (*model.Group, *model.AppError) + }); ok { + returns.A, returns.B = hook.GetGroup(args.A) + } else { + return encodableError(fmt.Errorf("API GetGroup called but not implemented.")) + } + return nil +} + +type Z_GetGroupByNameArgs struct { + A string +} + +type Z_GetGroupByNameReturns struct { + A *model.Group + B *model.AppError +} + +func (g *apiRPCClient) GetGroupByName(name string) (*model.Group, *model.AppError) { + _args := &Z_GetGroupByNameArgs{name} + _returns := &Z_GetGroupByNameReturns{} + if err := g.client.Call("Plugin.GetGroupByName", _args, _returns); err != nil { + log.Printf("RPC call to GetGroupByName API failed: %s", err.Error()) + } + return _returns.A, _returns.B +} + +func (s *apiRPCServer) GetGroupByName(args *Z_GetGroupByNameArgs, returns *Z_GetGroupByNameReturns) error { + if hook, ok := s.impl.(interface { + GetGroupByName(name string) (*model.Group, *model.AppError) + }); ok { + returns.A, returns.B = hook.GetGroupByName(args.A) + } else { + return encodableError(fmt.Errorf("API GetGroupByName called but not implemented.")) + } + return nil +} + +type Z_GetGroupsForUserArgs struct { + A string +} + +type Z_GetGroupsForUserReturns struct { + A []*model.Group + B *model.AppError +} + +func (g *apiRPCClient) GetGroupsForUser(userId string) ([]*model.Group, *model.AppError) { + _args := &Z_GetGroupsForUserArgs{userId} + _returns := &Z_GetGroupsForUserReturns{} + if err := g.client.Call("Plugin.GetGroupsForUser", _args, _returns); err != nil { + log.Printf("RPC call to GetGroupsForUser API failed: %s", err.Error()) + } + return _returns.A, _returns.B +} + +func (s *apiRPCServer) GetGroupsForUser(args *Z_GetGroupsForUserArgs, returns *Z_GetGroupsForUserReturns) error { + if hook, ok := s.impl.(interface { + GetGroupsForUser(userId string) ([]*model.Group, *model.AppError) + }); ok { + returns.A, returns.B = hook.GetGroupsForUser(args.A) + } else { + return encodableError(fmt.Errorf("API GetGroupsForUser called but not implemented.")) + } + return nil +} + type Z_DeleteChannelMemberArgs struct { A string B string diff --git a/plugin/plugintest/api.go b/plugin/plugintest/api.go index f308746374..4db7d5e5e2 100644 --- a/plugin/plugintest/api.go +++ b/plugin/plugintest/api.go @@ -996,6 +996,56 @@ func (_m *API) GetFileLink(fileId string) (string, *model.AppError) { return r0, r1 } +// GetGroup provides a mock function with given fields: groupId +func (_m *API) GetGroup(groupId string) (*model.Group, *model.AppError) { + ret := _m.Called(groupId) + + var r0 *model.Group + if rf, ok := ret.Get(0).(func(string) *model.Group); ok { + r0 = rf(groupId) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*model.Group) + } + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string) *model.AppError); ok { + r1 = rf(groupId) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 +} + +// GetGroupByName provides a mock function with given fields: name +func (_m *API) GetGroupByName(name string) (*model.Group, *model.AppError) { + ret := _m.Called(name) + + var r0 *model.Group + if rf, ok := ret.Get(0).(func(string) *model.Group); ok { + r0 = rf(name) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*model.Group) + } + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string) *model.AppError); ok { + r1 = rf(name) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 +} + // GetGroupChannel provides a mock function with given fields: userIds func (_m *API) GetGroupChannel(userIds []string) (*model.Channel, *model.AppError) { ret := _m.Called(userIds) @@ -1021,6 +1071,31 @@ func (_m *API) GetGroupChannel(userIds []string) (*model.Channel, *model.AppErro return r0, r1 } +// GetGroupsForUser provides a mock function with given fields: userId +func (_m *API) GetGroupsForUser(userId string) ([]*model.Group, *model.AppError) { + ret := _m.Called(userId) + + var r0 []*model.Group + if rf, ok := ret.Get(0).(func(string) []*model.Group); ok { + r0 = rf(userId) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*model.Group) + } + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string) *model.AppError); ok { + r1 = rf(userId) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 +} + // GetLDAPUserAttributes provides a mock function with given fields: userId, attributes func (_m *API) GetLDAPUserAttributes(userId string, attributes []string) (map[string]string, *model.AppError) { ret := _m.Called(userId, attributes) diff --git a/store/sqlstore/group_store.go b/store/sqlstore/group_store.go index 0895ef0ae9..7ce65b23c4 100644 --- a/store/sqlstore/group_store.go +++ b/store/sqlstore/group_store.go @@ -123,6 +123,18 @@ func (s *SqlGroupStore) Get(groupId string) (*model.Group, *model.AppError) { return group, nil } +func (s *SqlGroupStore) GetByName(name string) (*model.Group, *model.AppError) { + var group *model.Group + if err := s.GetReplica().SelectOne(&group, "SELECT * from UserGroups WHERE Name = :Name", map[string]interface{}{"Name": name}); err != nil { + if err == sql.ErrNoRows { + return nil, model.NewAppError("SqlGroupStore.GroupGetByName", "store.sql_group.no_rows", nil, err.Error(), http.StatusNotFound) + } + return nil, model.NewAppError("SqlGroupStore.GroupGetByName", "store.select_error", nil, err.Error(), http.StatusInternalServerError) + } + + return group, nil +} + func (s *SqlGroupStore) GetByIDs(groupIDs []string) ([]*model.Group, *model.AppError) { var groups []*model.Group query := s.getQueryBuilder().Select("*").From("UserGroups").Where(sq.Eq{"Id": groupIDs}) @@ -158,6 +170,26 @@ func (s *SqlGroupStore) GetAllBySource(groupSource model.GroupSource) ([]*model. return groups, nil } +func (s *SqlGroupStore) GetByUser(userId string) ([]*model.Group, *model.AppError) { + var groups []*model.Group + + query := ` + SELECT + UserGroups.* + FROM + GroupMembers + JOIN UserGroups ON UserGroups.Id = GroupMembers.GroupId + WHERE + GroupMembers.DeleteAt = 0 + AND UserId = :UserId` + + if _, err := s.GetReplica().Select(&groups, query, map[string]interface{}{"UserId": userId}); err != nil { + return nil, model.NewAppError("SqlGroupStore.GetByUser", "store.select_error", nil, err.Error(), http.StatusInternalServerError) + } + + return groups, nil +} + func (s *SqlGroupStore) Update(group *model.Group) (*model.Group, *model.AppError) { var retrievedGroup *model.Group if err := s.GetMaster().SelectOne(&retrievedGroup, "SELECT * FROM UserGroups WHERE Id = :Id", map[string]interface{}{"Id": group.Id}); err != nil { diff --git a/store/store.go b/store/store.go index 9ad3324a02..ab28c0f588 100644 --- a/store/store.go +++ b/store/store.go @@ -569,9 +569,11 @@ type UserTermsOfServiceStore interface { type GroupStore interface { Create(group *model.Group) (*model.Group, *model.AppError) Get(groupID string) (*model.Group, *model.AppError) + GetByName(name string) (*model.Group, *model.AppError) GetByIDs(groupIDs []string) ([]*model.Group, *model.AppError) GetByRemoteID(remoteID string, groupSource model.GroupSource) (*model.Group, *model.AppError) GetAllBySource(groupSource model.GroupSource) ([]*model.Group, *model.AppError) + GetByUser(userId string) ([]*model.Group, *model.AppError) Update(group *model.Group) (*model.Group, *model.AppError) Delete(groupID string) (*model.Group, *model.AppError) diff --git a/store/storetest/group_store.go b/store/storetest/group_store.go index 11afeb3fe8..0a289cc3ef 100644 --- a/store/storetest/group_store.go +++ b/store/storetest/group_store.go @@ -11,6 +11,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/mattermost/mattermost-server/model" @@ -20,9 +21,11 @@ import ( func TestGroupStore(t *testing.T, ss store.Store) { t.Run("Create", func(t *testing.T) { testGroupStoreCreate(t, ss) }) t.Run("Get", func(t *testing.T) { testGroupStoreGet(t, ss) }) + t.Run("GetByName", func(t *testing.T) { testGroupStoreGetByName(t, ss) }) t.Run("GetByIDs", func(t *testing.T) { testGroupStoreGetByIDs(t, ss) }) t.Run("GetByRemoteID", func(t *testing.T) { testGroupStoreGetByRemoteID(t, ss) }) t.Run("GetAllBySource", func(t *testing.T) { testGroupStoreGetAllByType(t, ss) }) + t.Run("GetByUser", func(t *testing.T) { testGroupStoreGetByUser(t, ss) }) t.Run("Update", func(t *testing.T) { testGroupStoreUpdate(t, ss) }) t.Run("Delete", func(t *testing.T) { testGroupStoreDelete(t, ss) }) @@ -181,6 +184,37 @@ func testGroupStoreGet(t *testing.T, ss store.Store) { require.Equal(t, err.Id, "store.sql_group.no_rows") } +func testGroupStoreGetByName(t *testing.T, ss store.Store) { + // Create a group + g1 := &model.Group{ + Name: model.NewId(), + DisplayName: model.NewId(), + Description: model.NewId(), + Source: model.GroupSourceLdap, + RemoteId: model.NewId(), + } + d1, err := ss.Group().Create(g1) + require.Nil(t, err) + require.Len(t, d1.Id, 26) + + // Get the group + d2, err := ss.Group().GetByName(d1.Name) + require.Nil(t, err) + require.Equal(t, d1.Id, d2.Id) + require.Equal(t, d1.Name, d2.Name) + require.Equal(t, d1.DisplayName, d2.DisplayName) + require.Equal(t, d1.Description, d2.Description) + require.Equal(t, d1.RemoteId, d2.RemoteId) + require.Equal(t, d1.CreateAt, d2.CreateAt) + require.Equal(t, d1.UpdateAt, d2.UpdateAt) + require.Equal(t, d1.DeleteAt, d2.DeleteAt) + + // Get an invalid group + _, err = ss.Group().GetByName(model.NewId()) + require.NotNil(t, err) + require.Equal(t, err.Id, "store.sql_group.no_rows") +} + func testGroupStoreGetByIDs(t *testing.T, ss store.Store) { var group1 *model.Group var group2 *model.Group @@ -280,6 +314,76 @@ func testGroupStoreGetAllByType(t *testing.T, ss store.Store) { } } +func testGroupStoreGetByUser(t *testing.T, ss store.Store) { + // Save a group + g1 := &model.Group{ + Name: model.NewId(), + DisplayName: model.NewId(), + Description: model.NewId(), + Source: model.GroupSourceLdap, + RemoteId: model.NewId(), + } + g1, err := ss.Group().Create(g1) + require.Nil(t, err) + + g2 := &model.Group{ + Name: model.NewId(), + DisplayName: model.NewId(), + Description: model.NewId(), + Source: model.GroupSourceLdap, + RemoteId: model.NewId(), + } + g2, err = ss.Group().Create(g2) + require.Nil(t, err) + + u1 := &model.User{ + Email: MakeEmail(), + Username: model.NewId(), + } + u1, err = ss.User().Save(u1) + require.Nil(t, err) + + _, err = ss.Group().UpsertMember(g1.Id, u1.Id) + require.Nil(t, err) + _, err = ss.Group().UpsertMember(g2.Id, u1.Id) + require.Nil(t, err) + + u2 := &model.User{ + Email: MakeEmail(), + Username: model.NewId(), + } + u2, err = ss.User().Save(u2) + require.Nil(t, err) + + _, err = ss.Group().UpsertMember(g2.Id, u2.Id) + require.Nil(t, err) + + groups, err := ss.Group().GetByUser(u1.Id) + require.Nil(t, err) + assert.Equal(t, 2, len(groups)) + found1 := false + found2 := false + for _, g := range groups { + if g.Id == g1.Id { + found1 = true + } + if g.Id == g2.Id { + found2 = true + } + } + assert.True(t, found1) + assert.True(t, found2) + + groups, err = ss.Group().GetByUser(u2.Id) + require.Nil(t, err) + require.Equal(t, 1, len(groups)) + assert.Equal(t, g2.Id, groups[0].Id) + + groups, err = ss.Group().GetByUser(model.NewId()) + require.Nil(t, err) + assert.Equal(t, 0, len(groups)) +} + func testGroupStoreUpdate(t *testing.T, ss store.Store) { // Save a new group g1 := &model.Group{ diff --git a/store/storetest/mocks/GroupStore.go b/store/storetest/mocks/GroupStore.go index f630d27581..e5328e4e50 100644 --- a/store/storetest/mocks/GroupStore.go +++ b/store/storetest/mocks/GroupStore.go @@ -406,6 +406,31 @@ func (_m *GroupStore) GetByIDs(groupIDs []string) ([]*model.Group, *model.AppErr return r0, r1 } +// GetByName provides a mock function with given fields: name +func (_m *GroupStore) GetByName(name string) (*model.Group, *model.AppError) { + ret := _m.Called(name) + + var r0 *model.Group + if rf, ok := ret.Get(0).(func(string) *model.Group); ok { + r0 = rf(name) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*model.Group) + } + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string) *model.AppError); ok { + r1 = rf(name) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 +} + // GetByRemoteID provides a mock function with given fields: remoteID, groupSource func (_m *GroupStore) GetByRemoteID(remoteID string, groupSource model.GroupSource) (*model.Group, *model.AppError) { ret := _m.Called(remoteID, groupSource) @@ -431,6 +456,31 @@ func (_m *GroupStore) GetByRemoteID(remoteID string, groupSource model.GroupSour return r0, r1 } +// GetByUser provides a mock function with given fields: userId +func (_m *GroupStore) GetByUser(userId string) ([]*model.Group, *model.AppError) { + ret := _m.Called(userId) + + var r0 []*model.Group + if rf, ok := ret.Get(0).(func(string) []*model.Group); ok { + r0 = rf(userId) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*model.Group) + } + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string) *model.AppError); ok { + r1 = rf(userId) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 +} + // GetGroupSyncable provides a mock function with given fields: groupID, syncableID, syncableType func (_m *GroupStore) GetGroupSyncable(groupID string, syncableID string, syncableType model.GroupSyncableType) (*model.GroupSyncable, *model.AppError) { ret := _m.Called(groupID, syncableID, syncableType) diff --git a/store/timer_layer.go b/store/timer_layer.go index 3632de3d81..366150b79a 100644 --- a/store/timer_layer.go +++ b/store/timer_layer.go @@ -2776,6 +2776,23 @@ func (s *TimerLayerGroupStore) GetByIDs(groupIDs []string) ([]*model.Group, *mod return resultVar0, resultVar1 } +func (s *TimerLayerGroupStore) GetByName(name string) (*model.Group, *model.AppError) { + start := timemodule.Now() + + resultVar0, resultVar1 := s.GroupStore.GetByName(name) + + t := timemodule.Now() + elapsed := t.Sub(start) + if s.Root.Metrics != nil { + success := "false" + if resultVar1 == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("GroupStore.GetByName", success, float64(elapsed)) + } + return resultVar0, resultVar1 +} + func (s *TimerLayerGroupStore) GetByRemoteID(remoteID string, groupSource model.GroupSource) (*model.Group, *model.AppError) { start := timemodule.Now() @@ -2792,6 +2809,23 @@ func (s *TimerLayerGroupStore) GetByRemoteID(remoteID string, groupSource model. return resultVar0, resultVar1 } +func (s *TimerLayerGroupStore) GetByUser(userId string) ([]*model.Group, *model.AppError) { + start := timemodule.Now() + + resultVar0, resultVar1 := s.GroupStore.GetByUser(userId) + + t := timemodule.Now() + elapsed := t.Sub(start) + if s.Root.Metrics != nil { + success := "false" + if resultVar1 == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("GroupStore.GetByUser", success, float64(elapsed)) + } + return resultVar0, resultVar1 +} + func (s *TimerLayerGroupStore) GetGroupSyncable(groupID string, syncableID string, syncableType model.GroupSyncableType) (*model.GroupSyncable, *model.AppError) { start := timemodule.Now() From a96565671df005962fd0b2d7efaeb0a79bb9dad6 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Wed, 23 Oct 2019 04:45:58 -0400 Subject: [PATCH 08/45] MM-19477: model extra manifest data (#12823) Model `HomepageURL` and `SupportURL` for use in listing plugins in the marketplace. --- model/manifest.go | 6 ++++++ model/manifest_test.go | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/model/manifest.go b/model/manifest.go index 92565e2d1f..c0eced6920 100644 --- a/model/manifest.go +++ b/model/manifest.go @@ -130,6 +130,12 @@ type Manifest struct { // A description of what your plugin is and does. Description string `json:"description,omitempty" yaml:"description,omitempty"` + // HomepageURL is an optional link to learn more about the plugin. + HomepageURL string `json:"homepage_url,omitempty" yaml:"homepage_url,omitempty"` + + // SupportURL is an optional URL where plugin issues can be reported. + SupportURL string `json:"support_url,omitempty" yaml:"support_url,omitempty"` + // A relative file path in the bundle that points to the plugins svg icon for use with the Plugin Marketplace. // This should be relative to the root of your bundle and the location of the manifest file. Bitmap image formats are not supported. IconPath string `json:"icon_path,omitempty" yaml:"icon_path,omitempty"` diff --git a/model/manifest_test.go b/model/manifest_test.go index 53e2370af0..2a36fbe19c 100644 --- a/model/manifest_test.go +++ b/model/manifest_test.go @@ -64,6 +64,8 @@ func TestFindManifest(t *testing.T) { func TestManifestUnmarshal(t *testing.T) { expected := Manifest{ Id: "theid", + HomepageURL: "https://example.com", + SupportURL: "https://example.com/support", IconPath: "assets/icon.svg", MinServerVersion: "5.6.0", Server: &ManifestServer{ @@ -103,6 +105,8 @@ func TestManifestUnmarshal(t *testing.T) { var yamlResult Manifest require.NoError(t, yaml.Unmarshal([]byte(` id: theid +homepage_url: https://example.com +support_url: https://example.com/support icon_path: assets/icon.svg min_server_version: 5.6.0 server: @@ -133,6 +137,8 @@ settings_schema: var jsonResult Manifest require.NoError(t, json.Unmarshal([]byte(`{ "id": "theid", + "homepage_url": "https://example.com", + "support_url": "https://example.com/support", "icon_path": "assets/icon.svg", "min_server_version": "5.6.0", "server": { From 23aa193f28f3e3dd431198bccc644b80c507daf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maur=C3=ADcio=20Linhares?= Date: Wed, 23 Oct 2019 08:45:15 -0400 Subject: [PATCH 09/45] =?UTF-8?q?Changes=20tests=20to=20use=20testify.requ?= =?UTF-8?q?ire=20at=20api4/file=5Ftest.go=20fixe=E2=80=A6=20(#12814)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- api4/apitestlib.go | 5 + api4/file_test.go | 228 +++++++++++++++++---------------------------- 2 files changed, 88 insertions(+), 145 deletions(-) diff --git a/api4/apitestlib.go b/api4/apitestlib.go index ba0a3fb24f..1746168e5f 100644 --- a/api4/apitestlib.go +++ b/api4/apitestlib.go @@ -5,6 +5,7 @@ package api4 import ( "fmt" + "github.com/stretchr/testify/require" "io/ioutil" "net" "net/http" @@ -673,6 +674,10 @@ func CheckErrorMessage(t *testing.T, resp *model.Response, errorId string) { } } +func CheckStartsWith(t *testing.T, value, prefix, message string) { + require.True(t, strings.HasPrefix(value, prefix), message, value) +} + // Similar to s3.New() but allows initialization of signature v2 or signature v4 client. // If signV2 input is false, function always returns signature v4. // diff --git a/api4/file_test.go b/api4/file_test.go index a12015294a..ed4afeafc1 100644 --- a/api4/file_test.go +++ b/api4/file_test.go @@ -40,26 +40,20 @@ func escapeQuotes(s string) string { return quoteEscaper.Replace(s) } -func randomBytes(n int) []byte { +func randomBytes(t *testing.T, n int) []byte { bb := make([]byte, n) _, err := rand.Read(bb) - if err != nil { - panic(err.Error()) - } + require.NoError(t, err) return bb } -func fileBytes(path string) []byte { +func fileBytes(t *testing.T, path string) []byte { path = filepath.Join(testDir, path) f, err := os.Open(path) - if err != nil { - panic(err.Error()) - } + require.NoError(t, err) defer f.Close() bb, err := ioutil.ReadAll(f) - if err != nil { - panic(err.Error()) - } + require.NoError(t, err) return bb } @@ -183,7 +177,7 @@ func testUploadFilesMultipart( require.Nil(t, err) } - mw.Close() + require.NoError(t, mw.Close()) return testDoUploadFileRequest(t, c, "", mwBody.Bytes(), mw.FormDataContentType(), -1) } @@ -251,7 +245,7 @@ func TestUploadFiles(t *testing.T) { { title: "Happy invalid image", names: []string{"testgif.gif"}, - blobs: [][]byte{fileBytes("test-search.md")}, + blobs: [][]byte{fileBytes(t, "test-search.md")}, skipPayloadValidation: true, expectedCreatorId: th.BasicUser.Id, }, @@ -382,7 +376,7 @@ func TestUploadFiles(t *testing.T) { useChunkedInSimplePost: true, skipPayloadValidation: true, names: []string{"1Mb-stream"}, - blobs: [][]byte{randomBytes(1024 * 1024)}, + blobs: [][]byte{randomBytes(t, 1024*1024)}, setupConfig: func(a *app.App) func(a *app.App) { maxFileSize := *a.Config().FileSettings.MaxFileSize a.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.MaxFileSize = 1024 * 1024 }) @@ -480,7 +474,7 @@ func TestUploadFiles(t *testing.T) { title: "Error stream too large", skipPayloadValidation: true, names: []string{"1Mb-stream"}, - blobs: [][]byte{randomBytes(1024 * 1024)}, + blobs: [][]byte{randomBytes(t, 1024*1024)}, skipSuccessValidation: true, checkResponse: CheckRequestEntityTooLargeStatus, setupConfig: func(a *app.App) func(a *app.App) { @@ -532,7 +526,7 @@ func TestUploadFiles(t *testing.T) { blobs := tc.blobs if len(blobs) == 0 { for _, name := range tc.names { - blobs = append(blobs, fileBytes(name)) + blobs = append(blobs, fileBytes(t, name)) } } @@ -555,9 +549,9 @@ func TestUploadFiles(t *testing.T) { return } - if fileResp == nil || len(fileResp.FileInfos) == 0 || len(fileResp.FileInfos) != len(tc.names) { - t.Fatal("Empty or mismatched actual or expected FileInfos") - } + require.NotNil(t, fileResp, "Nil fileResp") + require.NotEqual(t, 0, len(fileResp.FileInfos), "Empty FileInfos") + require.Equal(t, len(tc.names), len(fileResp.FileInfos), "Mismatched actual or expected FileInfos") for i, ri := range fileResp.FileInfos { // The returned file info from the upload call will be missing some fields that will be stored in the database @@ -653,29 +647,20 @@ func TestGetFile(t *testing.T) { t.Skip("skipping because no file driver is enabled") } - fileId := "" - var sent []byte - var err error - if sent, err = testutils.ReadTestFile("test.png"); err != nil { - t.Fatal(err) - } else { - fileResp, resp := Client.UploadFile(sent, channel.Id, "test.png") - CheckNoError(t, resp) + sent, err := testutils.ReadTestFile("test.png") + require.NoError(t, err) - fileId = fileResp.FileInfos[0].Id - } + fileResp, resp := Client.UploadFile(sent, channel.Id, "test.png") + CheckNoError(t, resp) + + fileId := fileResp.FileInfos[0].Id data, resp := Client.GetFile(fileId) CheckNoError(t, resp) - - if len(data) == 0 { - t.Fatal("should not be empty") - } + require.NotEqual(t, 0, len(data), "should not be empty") for i := range data { - if data[i] != sent[i] { - t.Fatal("received file didn't match sent one") - } + require.Equal(t, sent[i], data[i], "received file didn't match sent one") } _, resp = Client.GetFile("junk") @@ -713,30 +698,19 @@ func TestGetFileHeaders(t *testing.T) { _, resp = Client.GetFile(fileId) CheckNoError(t, resp) - if contentType := resp.Header.Get("Content-Type"); !strings.HasPrefix(contentType, expectedContentType) { - t.Fatal("returned incorrect Content-Type", contentType) - } + CheckStartsWith(t, resp.Header.Get("Content-Type"), expectedContentType, "returned incorrect Content-Type") if getInline { - if contentDisposition := resp.Header.Get("Content-Disposition"); !strings.HasPrefix(contentDisposition, "inline") { - t.Fatal("returned incorrect Content-Disposition", contentDisposition) - } + CheckStartsWith(t, resp.Header.Get("Content-Disposition"), "inline", "returned incorrect Content-Disposition") } else { - if contentDisposition := resp.Header.Get("Content-Disposition"); !strings.HasPrefix(contentDisposition, "attachment") { - t.Fatal("returned incorrect Content-Disposition", contentDisposition) - } + CheckStartsWith(t, resp.Header.Get("Content-Disposition"), "attachment", "returned incorrect Content-Disposition") } _, resp = Client.DownloadFile(fileId, true) CheckNoError(t, resp) - if contentType := resp.Header.Get("Content-Type"); !strings.HasPrefix(contentType, expectedContentType) { - t.Fatal("returned incorrect Content-Type", contentType) - } - - if contentDisposition := resp.Header.Get("Content-Disposition"); !strings.HasPrefix(contentDisposition, "attachment") { - t.Fatal("returned incorrect Content-Disposition", contentDisposition) - } + CheckStartsWith(t, resp.Header.Get("Content-Type"), expectedContentType, "returned incorrect Content-Type") + CheckStartsWith(t, resp.Header.Get("Content-Disposition"), "attachment", "returned incorrect Content-Disposition") } } @@ -768,27 +742,20 @@ func TestGetFileThumbnail(t *testing.T) { t.Skip("skipping because no file driver is enabled") } - fileId := "" - var sent []byte - var err error - if sent, err = testutils.ReadTestFile("test.png"); err != nil { - t.Fatal(err) - } else { - fileResp, resp := Client.UploadFile(sent, channel.Id, "test.png") - CheckNoError(t, resp) + sent, err := testutils.ReadTestFile("test.png") + require.NoError(t, err) - fileId = fileResp.FileInfos[0].Id - } + fileResp, resp := Client.UploadFile(sent, channel.Id, "test.png") + CheckNoError(t, resp) + + fileId := fileResp.FileInfos[0].Id // Wait a bit for files to ready time.Sleep(2 * time.Second) data, resp := Client.GetFileThumbnail(fileId) CheckNoError(t, resp) - - if len(data) == 0 { - t.Fatal("should not be empty") - } + require.NotEqual(t, 0, len(data), "should not be empty") _, resp = Client.GetFileThumbnail("junk") CheckBadRequestStatus(t, resp) @@ -823,21 +790,19 @@ func TestGetFileLink(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.EnablePublicLink = true }) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.PublicLinkSalt = model.NewRandomString(32) }) - fileId := "" - if data, err := testutils.ReadTestFile("test.png"); err != nil { - t.Fatal(err) - } else { - fileResp, resp := Client.UploadFile(data, channel.Id, "test.png") - CheckNoError(t, resp) + data, err := testutils.ReadTestFile("test.png") + require.NoError(t, err) - fileId = fileResp.FileInfos[0].Id - } + fileResp, uploadResp := Client.UploadFile(data, channel.Id, "test.png") + CheckNoError(t, uploadResp) + + fileId := fileResp.FileInfos[0].Id _, resp := Client.GetFileLink(fileId) CheckBadRequestStatus(t, resp) // Hacky way to assign file to a post (usually would be done by CreatePost call) - err := th.App.Srv.Store.FileInfo().AttachToPost(fileId, th.BasicPost.Id, th.BasicUser.Id) + err = th.App.Srv.Store.FileInfo().AttachToPost(fileId, th.BasicPost.Id, th.BasicUser.Id) require.Nil(t, err) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.EnablePublicLink = false }) @@ -850,10 +815,7 @@ func TestGetFileLink(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.EnablePublicLink = true }) link, resp := Client.GetFileLink(fileId) CheckNoError(t, resp) - - if link == "" { - t.Fatal("should've received public link") - } + require.NotEqual(t, "", link, "should've received public link") _, resp = Client.GetFileLink("junk") CheckBadRequestStatus(t, resp) @@ -889,27 +851,19 @@ func TestGetFilePreview(t *testing.T) { t.Skip("skipping because no file driver is enabled") } - fileId := "" - var sent []byte - var err error - if sent, err = testutils.ReadTestFile("test.png"); err != nil { - t.Fatal(err) - } else { - fileResp, resp := Client.UploadFile(sent, channel.Id, "test.png") - CheckNoError(t, resp) + sent, err := testutils.ReadTestFile("test.png") + require.NoError(t, err) - fileId = fileResp.FileInfos[0].Id - } + fileResp, resp := Client.UploadFile(sent, channel.Id, "test.png") + CheckNoError(t, resp) + fileId := fileResp.FileInfos[0].Id // Wait a bit for files to ready time.Sleep(2 * time.Second) data, resp := Client.GetFilePreview(fileId) CheckNoError(t, resp) - - if len(data) == 0 { - t.Fatal("should not be empty") - } + require.NotEqual(t, 0, len(data), "should not be empty") _, resp = Client.GetFilePreview("junk") CheckBadRequestStatus(t, resp) @@ -942,17 +896,12 @@ func TestGetFileInfo(t *testing.T) { t.Skip("skipping because no file driver is enabled") } - fileId := "" - var sent []byte - var err error - if sent, err = testutils.ReadTestFile("test.png"); err != nil { - t.Fatal(err) - } else { - fileResp, resp := Client.UploadFile(sent, channel.Id, "test.png") - CheckNoError(t, resp) + sent, err := testutils.ReadTestFile("test.png") + require.NoError(t, err) - fileId = fileResp.FileInfos[0].Id - } + fileResp, resp := Client.UploadFile(sent, channel.Id, "test.png") + CheckNoError(t, resp) + fileId := fileResp.FileInfos[0].Id // Wait a bit for files to ready time.Sleep(2 * time.Second) @@ -960,23 +909,14 @@ func TestGetFileInfo(t *testing.T) { info, resp := Client.GetFileInfo(fileId) CheckNoError(t, resp) - if err != nil { - t.Fatal(err) - } else if info.Id != fileId { - t.Fatal("got incorrect file") - } else if info.CreatorId != user.Id { - t.Fatal("file should be assigned to user") - } else if info.PostId != "" { - t.Fatal("file shouldn't have a post") - } else if info.Path != "" { - t.Fatal("file path shouldn't have been returned to client") - } else if info.ThumbnailPath != "" { - t.Fatal("file thumbnail path shouldn't have been returned to client") - } else if info.PreviewPath != "" { - t.Fatal("file preview path shouldn't have been returned to client") - } else if info.MimeType != "image/png" { - t.Fatal("mime type should've been image/png") - } + require.NoError(t, err) + require.Equal(t, fileId, info.Id, "got incorrect file") + require.Equal(t, user.Id, info.CreatorId, "file should be assigned to user") + require.Equal(t, "", info.PostId, "file shouldn't have a post") + require.Equal(t, "", info.Path, "file path shouldn't have been returned to client") + require.Equal(t, "", info.ThumbnailPath, "file thumbnail path shouldn't have been returned to client") + require.Equal(t, "", info.PreviewPath, "file preview path shouldn't have been returned to client") + require.Equal(t, "image/png", info.MimeType, "mime type should've been image/png") _, resp = Client.GetFileInfo("junk") CheckBadRequestStatus(t, resp) @@ -1007,18 +947,16 @@ func TestGetPublicFile(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.EnablePublicLink = true }) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.PublicLinkSalt = model.NewRandomString(32) }) - fileId := "" - if data, err := testutils.ReadTestFile("test.png"); err != nil { - t.Fatal(err) - } else { - fileResp, resp := Client.UploadFile(data, channel.Id, "test.png") - CheckNoError(t, resp) + data, err := testutils.ReadTestFile("test.png") + require.NoError(t, err) - fileId = fileResp.FileInfos[0].Id - } + fileResp, httpResp := Client.UploadFile(data, channel.Id, "test.png") + CheckNoError(t, httpResp) + + fileId := fileResp.FileInfos[0].Id // Hacky way to assign file to a post (usually would be done by CreatePost call) - err := th.App.Srv.Store.FileInfo().AttachToPost(fileId, th.BasicPost.Id, th.BasicUser.Id) + err = th.App.Srv.Store.FileInfo().AttachToPost(fileId, th.BasicPost.Id, th.BasicUser.Id) require.Nil(t, err) info, err := th.App.Srv.Store.FileInfo().Get(fileId) @@ -1028,31 +966,31 @@ func TestGetPublicFile(t *testing.T) { // Wait a bit for files to ready time.Sleep(2 * time.Second) - if resp, err := http.Get(link); err != nil || resp.StatusCode != http.StatusOK { - t.Log(link) - t.Fatal("failed to get image with public link", err) - } + resp, err := http.Get(link) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode, "failed to get image with public link") - if resp, err := http.Get(link[:strings.LastIndex(link, "?")]); err == nil && resp.StatusCode != http.StatusBadRequest { - t.Fatal("should've failed to get image with public link without hash", resp.Status) - } + resp, err = http.Get(link[:strings.LastIndex(link, "?")]) + require.NoError(t, err) + require.Equal(t, http.StatusBadRequest, resp.StatusCode, "should've failed to get image with public link without hash", resp.Status) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.EnablePublicLink = false }) - if resp, err := http.Get(link); err == nil && resp.StatusCode != http.StatusNotImplemented { - t.Fatal("should've failed to get image with disabled public link") - } + + resp, err = http.Get(link) + require.NoError(t, err) + require.Equal(t, http.StatusNotImplemented, resp.StatusCode, "should've failed to get image with disabled public link") // test after the salt has changed th.App.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.EnablePublicLink = true }) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.PublicLinkSalt = model.NewRandomString(32) }) - if resp, err := http.Get(link); err == nil && resp.StatusCode != http.StatusBadRequest { - t.Fatal("should've failed to get image with public link after salt changed") - } + resp, err = http.Get(link) + require.NoError(t, err) + require.Equal(t, http.StatusBadRequest, resp.StatusCode, "should've failed to get image with public link after salt changed") - if resp, err := http.Get(link); err == nil && resp.StatusCode != http.StatusBadRequest { - t.Fatal("should've failed to get image with public link after salt changed") - } + resp, err = http.Get(link) + require.NoError(t, err) + require.Equal(t, http.StatusBadRequest, resp.StatusCode, "should've failed to get image with public link after salt changed") fileInfo, err := th.App.Srv.Store.FileInfo().Get(fileId) require.Nil(t, err) From 1234e685755af8ccb1ddb2c1a9b4d82e671eef4e Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Wed, 23 Oct 2019 14:31:22 -0400 Subject: [PATCH 10/45] MM-19574 Don't return metadata for deleted posts (#12878) --- app/post_metadata.go | 5 +++++ app/post_metadata_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/app/post_metadata.go b/app/post_metadata.go index f5d751bbf5..233bd4a841 100644 --- a/app/post_metadata.go +++ b/app/post_metadata.go @@ -87,6 +87,11 @@ func (a *App) PreparePostForClient(originalPost *model.Post, isNewPost bool, isE post.Metadata = &model.PostMetadata{} + if post.DeleteAt > 0 { + // Don't fill out metadata for deleted posts + return post + } + // Emojis and reaction counts if emojis, reactions, err := a.getEmojisAndReactionsForPost(post); err != nil { mlog.Warn("Failed to get emojis and reactions for a post", mlog.String("post_id", post.Id), mlog.Err(err)) diff --git a/app/post_metadata_test.go b/app/post_metadata_test.go index 240f18df1d..367ec99372 100644 --- a/app/post_metadata_test.go +++ b/app/post_metadata_test.go @@ -477,6 +477,36 @@ func TestPreparePostForClient(t *testing.T) { }, imageDimensions[server.URL+"/test-image1.png"]) }) }) + + t.Run("no metadata for deleted posts", func(t *testing.T) { + th := setup() + defer th.TearDown() + + fileInfo, err := th.App.DoUploadFile(time.Now(), th.BasicTeam.Id, th.BasicChannel.Id, th.BasicUser.Id, "test.txt", []byte("test")) + require.Nil(t, err) + + post, err := th.App.CreatePost(&model.Post{ + Message: "test", + FileIds: []string{fileInfo.Id}, + UserId: th.BasicUser.Id, + ChannelId: th.BasicChannel.Id, + }, th.BasicChannel, false) + require.Nil(t, err) + + th.AddReactionToPost(post, th.BasicUser, "taco") + + post, err = th.App.DeletePost(post.Id, th.BasicUser.Id) + require.Nil(t, err) + + // DeleteAt isn't set on the post returned by App.DeletePost + post.DeleteAt = model.GetMillis() + + clientPost := th.App.PreparePostForClient(post, false, false) + + assert.NotEqual(t, nil, clientPost.Metadata, "should've populated Metadata“") + assert.Nil(t, clientPost.Metadata.Reactions, "should not have populated Reactions") + assert.Nil(t, clientPost.Metadata.Files, "should not have populated Files") + }) } func TestPreparePostForClientWithImageProxy(t *testing.T) { From 9fa6e80e55db6f32fd0194186d14359c004abe4f Mon Sep 17 00:00:00 2001 From: Andre Polykanine Date: Thu, 24 Oct 2019 08:44:01 +0300 Subject: [PATCH 11/45] Refactor app/notification_email.go for structure logging (#12740) --- app/notification_email.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/notification_email.go b/app/notification_email.go index a2fbb6766d..cdd7ae7ef9 100644 --- a/app/notification_email.go +++ b/app/notification_email.go @@ -100,7 +100,7 @@ func (a *App) sendNotificationEmail(notification *postNotification, user *model. a.Srv.Go(func() { if err := a.SendNotificationMail(user.Email, html.UnescapeString(subjectText), bodyText); err != nil { - mlog.Error(fmt.Sprint("Error to send the email", user.Email, err)) + mlog.Error("Error while sending the email", mlog.String("email", user.Email), mlog.Err(err)) } }) @@ -320,7 +320,7 @@ func (a *App) GetMessageForNotification(post *model.Post, translateFunc i18n.Tra // extract the filenames from their paths and determine what type of files are attached infos, err := a.Srv.Store.FileInfo().GetForPost(post.Id, true, false, true) if err != nil { - mlog.Warn(fmt.Sprintf("Encountered error when getting files for notification message, post_id=%v, err=%v", post.Id, err), mlog.String("post_id", post.Id)) + mlog.Warn("Encountered error when getting files for notification message", mlog.String("post_id", post.Id), mlog.Err(err)) } filenames := make([]string, len(infos)) From 07cd32779dbecf5d1e2177fbcc00928cf64671df Mon Sep 17 00:00:00 2001 From: Ioannis Mavroukakis Date: Thu, 24 Oct 2019 08:01:53 +0100 Subject: [PATCH 12/45] MM-394 convert remaining mailservice tests to require/assert (#12813) --- services/mailservice/mail_test.go | 122 +++++++++++++----------------- 1 file changed, 51 insertions(+), 71 deletions(-) diff --git a/services/mailservice/mail_test.go b/services/mailservice/mail_test.go index 0b62fcab15..2e2b87302f 100644 --- a/services/mailservice/mail_test.go +++ b/services/mailservice/mail_test.go @@ -9,7 +9,6 @@ import ( "io" "io/ioutil" "os" - "strings" "testing" "time" @@ -29,24 +28,19 @@ func TestMailConnectionFromConfig(t *testing.T) { require.Nil(t, err) cfg := fs.Get() + conn, err := ConnectToSMTPServer(cfg) + require.Nil(t, err, "Should connect to the SMTP Server %v", err) - if conn, err := ConnectToSMTPServer(cfg); err != nil { - t.Log(err) - t.Fatal("Should connect to the STMP Server") - } else { - if _, err1 := NewSMTPClient(conn, cfg); err1 != nil { - t.Log(err) - t.Fatal("Should get new smtp client") - } - } + _, err = NewSMTPClient(conn, cfg) + + require.Nil(t, err, "Should get new SMTP client") *cfg.EmailSettings.SMTPServer = "wrongServer" *cfg.EmailSettings.SMTPPort = "553" - if _, err := ConnectToSMTPServer(cfg); err == nil { - t.Log(err) - t.Fatal("Should not to the STMP Server") - } + _, err = ConnectToSMTPServer(cfg) + + require.NotNil(t, err, "Should not connect to the SMTP Server") } func TestMailConnectionAdvanced(t *testing.T) { @@ -55,7 +49,7 @@ func TestMailConnectionAdvanced(t *testing.T) { cfg := fs.Get() - if conn, err := ConnectToSMTPServerAdvanced( + conn, err := ConnectToSMTPServerAdvanced( &SmtpConnectionInfo{ ConnectionSecurity: *cfg.EmailSettings.ConnectionSecurity, SkipCertVerification: *cfg.EmailSettings.SkipServerCertificateVerification, @@ -63,30 +57,28 @@ func TestMailConnectionAdvanced(t *testing.T) { SmtpServerHost: *cfg.EmailSettings.SMTPServer, SmtpPort: *cfg.EmailSettings.SMTPPort, }, - ); err != nil { - t.Log(err) - t.Fatal("Should connect to the STMP Server") - } else { - if _, err1 := NewSMTPClientAdvanced( - conn, - utils.GetHostnameFromSiteURL(*cfg.ServiceSettings.SiteURL), - &SmtpConnectionInfo{ - ConnectionSecurity: *cfg.EmailSettings.ConnectionSecurity, - SkipCertVerification: *cfg.EmailSettings.SkipServerCertificateVerification, - SmtpServerName: *cfg.EmailSettings.SMTPServer, - SmtpServerHost: *cfg.EmailSettings.SMTPServer, - SmtpPort: *cfg.EmailSettings.SMTPPort, - Auth: *cfg.EmailSettings.EnableSMTPAuth, - SmtpUsername: *cfg.EmailSettings.SMTPUsername, - SmtpPassword: *cfg.EmailSettings.SMTPPassword, - }, - ); err1 != nil { - t.Log(err) - t.Fatal("Should get new smtp client") - } - } + ) - if _, err := ConnectToSMTPServerAdvanced( + require.Nil(t, err, "Should connect to the SMTP Server") + + _, err2 := NewSMTPClientAdvanced( + conn, + utils.GetHostnameFromSiteURL(*cfg.ServiceSettings.SiteURL), + &SmtpConnectionInfo{ + ConnectionSecurity: *cfg.EmailSettings.ConnectionSecurity, + SkipCertVerification: *cfg.EmailSettings.SkipServerCertificateVerification, + SmtpServerName: *cfg.EmailSettings.SMTPServer, + SmtpServerHost: *cfg.EmailSettings.SMTPServer, + SmtpPort: *cfg.EmailSettings.SMTPPort, + Auth: *cfg.EmailSettings.EnableSMTPAuth, + SmtpUsername: *cfg.EmailSettings.SMTPUsername, + SmtpPassword: *cfg.EmailSettings.SMTPPassword, + }, + ) + + require.Nil(t, err2, "Should get new SMTP client") + + _, err3 := ConnectToSMTPServerAdvanced( &SmtpConnectionInfo{ ConnectionSecurity: *cfg.EmailSettings.ConnectionSecurity, SkipCertVerification: *cfg.EmailSettings.SkipServerCertificateVerification, @@ -94,11 +86,8 @@ func TestMailConnectionAdvanced(t *testing.T) { SmtpServerHost: "wrongServer", SmtpPort: "553", }, - ); err == nil { - t.Log(err) - t.Fatal("Should not to the STMP Server") - } - + ) + require.NotNil(t, err3, "Should not connect to the SMTP Server") } func TestSendMailUsingConfig(t *testing.T) { @@ -116,32 +105,25 @@ func TestSendMailUsingConfig(t *testing.T) { //Delete all the messages before check the sample email DeleteMailBox(emailTo) - if err := SendMailUsingConfig(emailTo, emailSubject, emailBody, cfg, true); err != nil { - t.Log(err) - t.Fatal("Should connect to the STMP Server") + err2 := SendMailUsingConfig(emailTo, emailSubject, emailBody, cfg, true) + require.Nil(t, err2, "Should connect to the SMTP Server") + + //Check if the email was send to the right email address + var resultsMailbox JSONMessageHeaderInbucket + err3 := RetryInbucket(5, func() error { + var err error + resultsMailbox, err = GetMailBox(emailTo) + return err + }) + if err3 != nil { + t.Log(err3) + t.Log("No email was received, maybe due load on the server. Skipping this verification") } else { - //Check if the email was send to the right email address - var resultsMailbox JSONMessageHeaderInbucket - err := RetryInbucket(5, func() error { - var err error - resultsMailbox, err = GetMailBox(emailTo) - return err - }) - if err != nil { - t.Log(err) - t.Log("No email was received, maybe due load on the server. Disabling this verification") - } - if err == nil && len(resultsMailbox) > 0 { - if !strings.ContainsAny(resultsMailbox[0].To[0], emailTo) { - t.Fatal("Wrong To recipient") - } else { - if resultsEmail, err := GetMessageFromMailbox(emailTo, resultsMailbox[0].ID); err == nil { - if !strings.Contains(resultsEmail.Body.Text, emailBody) { - t.Log(resultsEmail.Body.Text) - t.Fatal("Received message") - } - } - } + if len(resultsMailbox) > 0 { + require.Contains(t, resultsMailbox[0].To[0], emailTo, "Wrong To: recipient") + resultsEmail, err := GetMessageFromMailbox(emailTo, resultsMailbox[0].ID) + require.Nil(t, err, "Could not get message from mailbox") + require.Contains(t, emailBody, resultsEmail.Body.Text, "Wrong received message %s", resultsEmail.Body.Text) } } } @@ -299,9 +281,7 @@ func TestAuthMethods(t *testing.T) { if err != nil { got = err.Error() } - if got != test.err { - t.Errorf("%d. got error = %q; want %q", i, got, test.err) - } + assert.True(t, got == test.err, "%d. got error = %q; want %q", i, got, test.err) }) } } From 0652e121ec3cd298dbf8afb9d4980ef13419612b Mon Sep 17 00:00:00 2001 From: Mrigank Krishan Date: Thu, 24 Oct 2019 14:34:50 +0530 Subject: [PATCH 13/45] use testify in "model/session_test.go" (#12644) Migrate tests from "model/session_test.go" to use testify --- model/session_test.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/model/session_test.go b/model/session_test.go index fb89f89633..c232db8d66 100644 --- a/model/session_test.go +++ b/model/session_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/assert" ) @@ -45,21 +46,16 @@ func TestSessionJson(t *testing.T) { json := session.ToJson() rsession := SessionFromJson(strings.NewReader(json)) - if rsession.Id != session.Id { - t.Fatal("Ids do not match") - } + require.Equal(t, rsession.Id, session.Id, "Ids do not match") session.Sanitize() - if session.IsExpired() { - t.Fatal("Shouldn't expire") - } + require.False(t, session.IsExpired(), "Shouldn't expire") session.ExpiresAt = GetMillis() time.Sleep(10 * time.Millisecond) - if !session.IsExpired() { - t.Fatal("Should expire") - } + + require.True(t, session.IsExpired(), "Should expire") session.SetExpireInDays(10) } From cedaee99b8b060c56545dfaae6cd35b8dd10e091 Mon Sep 17 00:00:00 2001 From: Shobhit Gupta Date: Thu, 24 Oct 2019 06:04:06 -0700 Subject: [PATCH 14/45] Migrate tests from "api4/websocket_test.go" to use testify (#12896) * Migrate tests to use testify * Fix build * Fix tests --- api4/websocket_test.go | 229 ++++++++++++++--------------------------- 1 file changed, 76 insertions(+), 153 deletions(-) diff --git a/api4/websocket_test.go b/api4/websocket_test.go index f6f2ae8795..e4e1517000 100644 --- a/api4/websocket_test.go +++ b/api4/websocket_test.go @@ -20,62 +20,50 @@ func TestWebSocket(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() WebSocketClient, err := th.CreateWebSocketClient() - if err != nil { - t.Fatal(err) - } + require.Nil(t, err) defer WebSocketClient.Close() time.Sleep(300 * time.Millisecond) // Test closing and reconnecting WebSocketClient.Close() - if err := WebSocketClient.Connect(); err != nil { - t.Fatal(err) - } + err = WebSocketClient.Connect() + require.Nil(t, err) WebSocketClient.Listen() time.Sleep(300 * time.Millisecond) - if resp := <-WebSocketClient.ResponseChannel; resp.Status != model.STATUS_OK { - t.Fatal("should have responded OK to authentication challenge") - } + resp := <-WebSocketClient.ResponseChannel + require.Equal(t, resp.Status, model.STATUS_OK, "should have responded OK to authentication challenge") WebSocketClient.SendMessage("ping", nil) time.Sleep(300 * time.Millisecond) - if resp := <-WebSocketClient.ResponseChannel; resp.Data["text"].(string) != "pong" { - t.Fatal("wrong response") - } + resp = <-WebSocketClient.ResponseChannel + require.Equal(t, resp.Data["text"].(string),"pong","wrong response") WebSocketClient.SendMessage("", nil) time.Sleep(300 * time.Millisecond) - if resp := <-WebSocketClient.ResponseChannel; resp.Error.Id != "api.web_socket_router.no_action.app_error" { - t.Fatal("should have been no action response") - } + resp = <-WebSocketClient.ResponseChannel + require.Equal(t, resp.Error.Id, "api.web_socket_router.no_action.app_error", "should have been no action response") WebSocketClient.SendMessage("junk", nil) time.Sleep(300 * time.Millisecond) - if resp := <-WebSocketClient.ResponseChannel; resp.Error.Id != "api.web_socket_router.bad_action.app_error" { - t.Fatal("should have been bad action response") - } + resp = <-WebSocketClient.ResponseChannel + require.Equal(t, resp.Error.Id, "api.web_socket_router.bad_action.app_error", "should have been bad action response") req := &model.WebSocketRequest{} req.Seq = 0 req.Action = "ping" WebSocketClient.Conn.WriteJSON(req) time.Sleep(300 * time.Millisecond) - if resp := <-WebSocketClient.ResponseChannel; resp.Error.Id != "api.web_socket_router.bad_seq.app_error" { - t.Fatal("should have been bad action response") - } + resp = <-WebSocketClient.ResponseChannel + require.Equal(t, resp.Error.Id, "api.web_socket_router.bad_seq.app_error", "should have been bad action response") WebSocketClient.UserTyping("", "") time.Sleep(300 * time.Millisecond) - if resp := <-WebSocketClient.ResponseChannel; resp.Error.Id != "api.websocket_handler.invalid_param.app_error" { - t.Fatal("should have been invalid param response") - } else { - if resp.Error.DetailedError != "" { - t.Fatal("detailed error not cleared") - } - } + resp = <-WebSocketClient.ResponseChannel + require.Equal(t, resp.Error.Id, "api.websocket_handler.invalid_param.app_error", "should have been invalid param response") + require.Equal(t, resp.Error.DetailedError, "", "detailed error not cleared") } func TestWebSocketEvent(t *testing.T) { @@ -83,17 +71,14 @@ func TestWebSocketEvent(t *testing.T) { defer th.TearDown() WebSocketClient, err := th.CreateWebSocketClient() - if err != nil { - t.Fatal(err) - } + require.Nil(t, err) defer WebSocketClient.Close() WebSocketClient.Listen() time.Sleep(300 * time.Millisecond) - if resp := <-WebSocketClient.ResponseChannel; resp.Status != model.STATUS_OK { - t.Fatal("should have responded OK to authentication challenge") - } + resp := <-WebSocketClient.ResponseChannel + require.Equal(t, resp.Status, model.STATUS_OK, "should have responded OK to authentication challenge") omitUser := make(map[string]bool, 1) omitUser["somerandomid"] = true @@ -123,9 +108,7 @@ func TestWebSocketEvent(t *testing.T) { stop <- true - if !eventHit { - t.Fatal("did not receive typing event") - } + require.True(t, eventHit, "did not receive typing event") evt2 := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_TYPING, "", "somerandomid", "", nil) th.App.Publish(evt2) @@ -150,9 +133,7 @@ func TestWebSocketEvent(t *testing.T) { stop <- true - if eventHit { - t.Fatal("got typing event for bad channel id") - } + require.False(t, eventHit, "got typing event for bad channel id") } func TestCreateDirectChannelWithSocket(t *testing.T) { @@ -170,21 +151,16 @@ func TestCreateDirectChannelWithSocket(t *testing.T) { } WebSocketClient, err := th.CreateWebSocketClient() - if err != nil { - t.Fatal(err) - } + require.Nil(t, err) defer WebSocketClient.Close() WebSocketClient.Listen() time.Sleep(300 * time.Millisecond) - if resp := <-WebSocketClient.ResponseChannel; resp.Status != model.STATUS_OK { - t.Fatal("should have responded OK to authentication challenge") - } + resp := <-WebSocketClient.ResponseChannel + require.Equal(t, resp.Status, model.STATUS_OK, "should have responded OK to authentication challenge") wsr := <-WebSocketClient.EventChannel - if wsr.Event != model.WEBSOCKET_EVENT_HELLO { - t.Fatal("missing hello") - } + require.Equal(t, wsr.Event, model.WEBSOCKET_EVENT_HELLO, "missing hello") stop := make(chan bool) count := 0 @@ -205,19 +181,15 @@ func TestCreateDirectChannelWithSocket(t *testing.T) { for _, user := range users { time.Sleep(100 * time.Millisecond) - if _, resp := Client.CreateDirectChannel(th.BasicUser.Id, user.Id); resp.Error != nil { - t.Fatal("failed to create DM channel") - } + _, resp := Client.CreateDirectChannel(th.BasicUser.Id, user.Id) + require.Nil(t, resp.Error, "failed to create DM channel") } time.Sleep(5000 * time.Millisecond) stop <- true - if count != len(users) { - t.Fatal("We didn't get the proper amount of direct_added messages") - } - + require.Equal(t, count, len(users), "We didn't get the proper amount of direct_added messages") } func TestWebsocketOriginSecurity(t *testing.T) { @@ -230,53 +202,42 @@ func TestWebsocketOriginSecurity(t *testing.T) { _, _, err := websocket.DefaultDialer.Dial(url+model.API_URL_SUFFIX+"/websocket", http.Header{ "Origin": []string{"http://www.evil.com"}, }) - if err == nil { - t.Fatal("Should have errored because Origin does not match host! SECURITY ISSUE!") - } + + require.NotNil(t, err, "Should have errored because Origin does not match host! SECURITY ISSUE!") // We are not a browser so we can spoof this just fine _, _, err = websocket.DefaultDialer.Dial(url+model.API_URL_SUFFIX+"/websocket", http.Header{ "Origin": []string{fmt.Sprintf("http://localhost:%v", th.App.Srv.ListenAddr.Port)}, }) - if err != nil { - t.Fatal(err) - } + require.Nil(t, err, err) // Should succeed now because open CORS th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowCorsFrom = "*" }) _, _, err = websocket.DefaultDialer.Dial(url+model.API_URL_SUFFIX+"/websocket", http.Header{ "Origin": []string{"http://www.evil.com"}, }) - if err != nil { - t.Fatal(err) - } + require.Nil(t, err, err) // Should succeed now because matching CORS th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowCorsFrom = "http://www.evil.com" }) _, _, err = websocket.DefaultDialer.Dial(url+model.API_URL_SUFFIX+"/websocket", http.Header{ "Origin": []string{"http://www.evil.com"}, }) - if err != nil { - t.Fatal(err) - } + require.Nil(t, err, err) // Should fail because non-matching CORS th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowCorsFrom = "http://www.good.com" }) _, _, err = websocket.DefaultDialer.Dial(url+model.API_URL_SUFFIX+"/websocket", http.Header{ "Origin": []string{"http://www.evil.com"}, }) - if err == nil { - t.Fatal("Should have errored because Origin contain AllowCorsFrom") - } + require.NotNil(t, err, "Should have errored because Origin contain AllowCorsFrom") // Should fail because non-matching CORS th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowCorsFrom = "http://www.good.com" }) _, _, err = websocket.DefaultDialer.Dial(url+model.API_URL_SUFFIX+"/websocket", http.Header{ "Origin": []string{"http://www.good.co"}, }) - if err == nil { - t.Fatal("Should have errored because Origin does not match host! SECURITY ISSUE!") - } + require.NotNil(t, err, "Should have errored because Origin does not match host! SECURITY ISSUE!") th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowCorsFrom = "" }) } @@ -287,16 +248,13 @@ func TestWebSocketStatuses(t *testing.T) { Client := th.Client WebSocketClient, err := th.CreateWebSocketClient() - if err != nil { - t.Fatal(err) - } + require.Nil(t, err, err) defer WebSocketClient.Close() WebSocketClient.Listen() time.Sleep(300 * time.Millisecond) - if resp := <-WebSocketClient.ResponseChannel; resp.Status != model.STATUS_OK { - t.Fatal("should have responded OK to authentication challenge") - } + resp := <-WebSocketClient.ResponseChannel + require.Equal(t, resp.Status, model.STATUS_OK, "should have responded OK to authentication challenge") team := model.Team{DisplayName: "Name", Name: "z-z-" + model.NewId() + "a", Email: "test@nowhere.com", Type: model.TEAM_OPEN} rteam, _ := Client.CreateTeam(&team) @@ -318,79 +276,53 @@ func TestWebSocketStatuses(t *testing.T) { th.LoginBasic2() WebSocketClient2, err2 := th.CreateWebSocketClient() - if err2 != nil { - t.Fatal(err2) - } + require.Nil(t, err2, err2) time.Sleep(1000 * time.Millisecond) WebSocketClient.GetStatuses() - if resp := <-WebSocketClient.ResponseChannel; resp.Error != nil { - t.Fatal(resp.Error) - } else { - if resp.SeqReply != WebSocketClient.Sequence-1 { - t.Fatal("bad sequence number") - } + resp = <-WebSocketClient.ResponseChannel + require.Nil(t, resp.Error, resp.Error) - for _, status := range resp.Data { - if status != model.STATUS_OFFLINE && status != model.STATUS_AWAY && status != model.STATUS_ONLINE && status != model.STATUS_DND { - t.Fatalf("one of the statuses had an invalid value status=%v", status) - } - } + require.Equal(t, resp.SeqReply, WebSocketClient.Sequence-1, "bad sequence number") - if status, ok := resp.Data[th.BasicUser2.Id]; !ok { - t.Log(resp.Data) - t.Fatal("should have had user status") - } else if status != model.STATUS_ONLINE { - t.Log(status) - t.Fatal("status should have been online") - } + allowedValues := [4]string{model.STATUS_OFFLINE, model.STATUS_AWAY, model.STATUS_ONLINE, model.STATUS_DND} + for _, status := range resp.Data { + require.Containsf(t, allowedValues, status, "one of the statuses had an invalid value status=%v", status) } + status, ok := resp.Data[th.BasicUser2.Id] + require.True(t, ok, "should have had user status") + + require.Equal(t, status, model.STATUS_ONLINE, "status should have been online status=%v", status) + WebSocketClient.GetStatusesByIds([]string{th.BasicUser2.Id}) - if resp := <-WebSocketClient.ResponseChannel; resp.Error != nil { - t.Fatal(resp.Error) - } else { - if resp.SeqReply != WebSocketClient.Sequence-1 { - t.Fatal("bad sequence number") - } + resp = <-WebSocketClient.ResponseChannel + require.Nil(t, resp.Error, resp.Error) - for _, status := range resp.Data { - if status != model.STATUS_OFFLINE && status != model.STATUS_AWAY && status != model.STATUS_ONLINE { - t.Fatal("one of the statuses had an invalid value") - } - } + require.Equal(t, resp.SeqReply, WebSocketClient.Sequence-1, "bad sequence number") - if status, ok := resp.Data[th.BasicUser2.Id]; !ok { - t.Log(len(resp.Data)) - t.Fatal("should have had user status") - } else if status != model.STATUS_ONLINE { - t.Log(status) - t.Fatal("status should have been online") - } else if len(resp.Data) != 1 { - t.Fatal("only 1 status should be returned") - } + allowedValues = [4]string{model.STATUS_OFFLINE, model.STATUS_AWAY, model.STATUS_ONLINE} + for _, status := range resp.Data { + require.Containsf(t, allowedValues, status, "one of the statuses had an invalid value status") } + status, ok = resp.Data[th.BasicUser2.Id] + require.True(t, ok, "should have had user status") + + require.Equal(t, status, model.STATUS_ONLINE, "status should have been online status=%v", status) + require.Equal(t, len(resp.Data), 1, "only 1 status should be returned") + WebSocketClient.GetStatusesByIds([]string{ruser2.Id, "junk"}) - if resp := <-WebSocketClient.ResponseChannel; resp.Error != nil { - t.Fatal(resp.Error) - } else { - if resp.SeqReply != WebSocketClient.Sequence-1 { - t.Fatal("bad sequence number") - } - - if len(resp.Data) != 2 { - t.Fatal("2 statuses should be returned") - } - } + resp = <-WebSocketClient.ResponseChannel + require.Nil(t, resp.Error, resp.Error) + require.Equal(t, resp.SeqReply, WebSocketClient.Sequence-1, "bad sequence number") + require.Equal(t, len(resp.Data), 2, "2 statuses should be returned") WebSocketClient.GetStatusesByIds([]string{}) - if resp := <-WebSocketClient.ResponseChannel; resp.Error == nil { - if resp.SeqReply != WebSocketClient.Sequence-1 { - t.Fatal("bad sequence number") - } - t.Fatal("should have errored - empty user ids") + if resp2 := <-WebSocketClient.ResponseChannel; resp2.Error == nil { + require.Equal(t, resp2.SeqReply, WebSocketClient.Sequence-1, "bad sequence number") + require.NotNil(t, resp2.Error, "should have errored - empty user ids") } WebSocketClient2.Close() @@ -411,17 +343,12 @@ func TestWebSocketStatuses(t *testing.T) { time.Sleep(1500 * time.Millisecond) WebSocketClient.GetStatuses() - if resp := <-WebSocketClient.ResponseChannel; resp.Error != nil { - t.Fatal(resp.Error) - } else { - if resp.SeqReply != WebSocketClient.Sequence-1 { - t.Fatal("bad sequence number") - } + resp = <-WebSocketClient.ResponseChannel + require.Nil(t, resp.Error) - if _, ok := resp.Data[th.BasicUser2.Id]; ok { - t.Fatal("should not have had user status") - } - } + require.Equal(t, resp.SeqReply, WebSocketClient.Sequence-1, "bad sequence number") + _, ok = resp.Data[th.BasicUser2.Id] + require.False(t, ok, "should not have had user status") stop := make(chan bool) onlineHit := false @@ -449,12 +376,8 @@ func TestWebSocketStatuses(t *testing.T) { stop <- true - if !onlineHit { - t.Fatal("didn't get online event") - } - if !awayHit { - t.Fatal("didn't get away event") - } + require.True(t, onlineHit, "didn't get online event") + require.True(t, awayHit, "didn't get away event") time.Sleep(500 * time.Millisecond) From caeadbec889101ea977aea0d1bd90eb55cb81c42 Mon Sep 17 00:00:00 2001 From: Simon <38186045+simross@users.noreply.github.com> Date: Thu, 24 Oct 2019 16:32:31 +0300 Subject: [PATCH 15/45] MM-19468 Migrate tests from "api4/command_test.go" to use testify (#12809) * MM-19468 Migrate tests from "api4/command_test.go" to use testify * swap expected and actual values during token regeneration test --- api4/command_test.go | 138 +++++++++++++------------------------------ 1 file changed, 42 insertions(+), 96 deletions(-) diff --git a/api4/command_test.go b/api4/command_test.go index f091c0dbe1..7dddd313b1 100644 --- a/api4/command_test.go +++ b/api4/command_test.go @@ -39,12 +39,8 @@ func TestCreateCommand(t *testing.T) { createdCmd, resp := th.SystemAdminClient.CreateCommand(newCmd) CheckNoError(t, resp) CheckCreatedStatus(t, resp) - if createdCmd.CreatorId != th.SystemAdminUser.Id { - t.Fatal("user ids didn't match") - } - if createdCmd.TeamId != th.BasicTeam.Id { - t.Fatal("team ids didn't match") - } + require.Equal(t, th.SystemAdminUser.Id, createdCmd.CreatorId, "user ids didn't match") + require.Equal(t, th.BasicTeam.Id, createdCmd.TeamId, "team ids didn't match") _, resp = th.SystemAdminClient.CreateCommand(newCmd) CheckBadRequestStatus(t, resp) @@ -100,34 +96,22 @@ func TestUpdateCommand(t *testing.T) { rcmd, resp := Client.UpdateCommand(cmd2) CheckNoError(t, resp) - if rcmd.Trigger != cmd2.Trigger { - t.Fatal("Trigger should have updated") - } + require.Equal(t, cmd2.Trigger, rcmd.Trigger, "Trigger should have updated") - if rcmd.Method != cmd2.Method { - t.Fatal("Method should have updated") - } + require.Equal(t, cmd2.Method, rcmd.Method, "Method should have updated") - if rcmd.URL != cmd2.URL { - t.Fatal("URL should have updated") - } + require.Equal(t, cmd2.URL, rcmd.URL, "URL should have updated") - if rcmd.CreatorId != cmd1.CreatorId { - t.Fatal("CreatorId should have not updated") - } + require.Equal(t, cmd1.CreatorId, rcmd.CreatorId, "CreatorId should have not updated") - if rcmd.Token != cmd1.Token { - t.Fatal("Token should have not updated") - } + require.Equal(t, cmd1.Token, rcmd.Token, "Token should have not updated") cmd2.Id = GenerateTestId() rcmd, resp = Client.UpdateCommand(cmd2) CheckNotFoundStatus(t, resp) - if rcmd != nil { - t.Fatal("should be empty") - } + require.Nil(t, rcmd, "should be empty") cmd2.Id = "junk" @@ -176,21 +160,15 @@ func TestDeleteCommand(t *testing.T) { ok, resp := Client.DeleteCommand(rcmd1.Id) CheckNoError(t, resp) - if !ok { - t.Fatal("should have returned true") - } + require.True(t, ok) rcmd1, _ = th.App.GetCommand(rcmd1.Id) - if rcmd1 != nil { - t.Fatal("should be nil") - } + require.Nil(t, rcmd1) ok, resp = Client.DeleteCommand("junk") CheckBadRequestStatus(t, resp) - if ok { - t.Fatal("should have returned false") - } + require.False(t, ok) _, resp = Client.DeleteCommand(GenerateTestId()) CheckNotFoundStatus(t, resp) @@ -248,24 +226,16 @@ func TestListCommands(t *testing.T) { foundCustom = true } } - if !foundEcho { - t.Fatal("Couldn't find echo command") - } - if !foundCustom { - t.Fatal("Should list the custom command") - } + require.True(t, foundEcho, "Couldn't find echo command") + require.True(t, foundCustom, "Should list the custom command") }) t.Run("ListCustomOnlyCommands", func(t *testing.T) { listCommands, resp := th.SystemAdminClient.ListCommands(th.BasicTeam.Id, true) CheckNoError(t, resp) - if len(listCommands) > 1 { - t.Fatal("Should list just one custom command") - } - if listCommands[0].Trigger != "custom_command" { - t.Fatal("Wrong custom command trigger") - } + require.Len(t, listCommands, 1, "Should list just one custom command") + require.Equal(t, listCommands[0].Trigger, "custom_command", "Wrong custom command trigger") }) t.Run("UserWithNoPermissionForCustomCommands", func(t *testing.T) { @@ -287,12 +257,8 @@ func TestListCommands(t *testing.T) { foundCustom = true } } - if !foundEcho { - t.Fatal("Couldn't find echo command") - } - if foundCustom { - t.Fatal("Should not list the custom command") - } + require.True(t, foundEcho, "Couldn't find echo command") + require.False(t, foundCustom, "Should not list the custom command") }) t.Run("NoMember", func(t *testing.T) { @@ -344,12 +310,8 @@ func TestListAutocompleteCommands(t *testing.T) { foundCustom = true } } - if !foundEcho { - t.Fatal("Couldn't find echo command") - } - if foundCustom { - t.Fatal("Should not list the custom command") - } + require.True(t, foundEcho, "Couldn't find echo command") + require.False(t, foundCustom, "Should not list the custom command") }) t.Run("RegularUserCanListOnlySystemCommands", func(t *testing.T) { @@ -366,12 +328,8 @@ func TestListAutocompleteCommands(t *testing.T) { foundCustom = true } } - if !foundEcho { - t.Fatal("Couldn't find echo command") - } - if foundCustom { - t.Fatal("Should not list the custom command") - } + require.True(t, foundEcho, "Couldn't find echo command") + require.False(t, foundCustom, "Should not list the custom command") }) t.Run("NoMember", func(t *testing.T) { @@ -414,15 +372,11 @@ func TestRegenToken(t *testing.T) { token, resp := th.SystemAdminClient.RegenCommandToken(createdCmd.Id) CheckNoError(t, resp) - if token == createdCmd.Token { - t.Fatal("should update the token") - } + require.NotEqual(t, createdCmd.Token, token, "should update the token") token, resp = Client.RegenCommandToken(createdCmd.Id) CheckForbiddenStatus(t, resp) - if token != "" { - t.Fatal("should not return the token") - } + require.Empty(t, token, "should not return the token") } func TestExecuteInvalidCommand(t *testing.T) { @@ -457,9 +411,8 @@ func TestExecuteInvalidCommand(t *testing.T) { Trigger: "getcommand", } - if _, err := th.App.CreateCommand(getCmd); err != nil { - t.Fatal("failed to create get command") - } + _, err := th.App.CreateCommand(getCmd) + require.Nil(t, err, "failed to create get command") _, resp := Client.ExecuteCommand(channel.Id, "") CheckBadRequestStatus(t, resp) @@ -537,9 +490,8 @@ func TestExecuteGetCommand(t *testing.T) { Token: token, } - if _, err := th.App.CreateCommand(getCmd); err != nil { - t.Fatal("failed to create get command") - } + _, err := th.App.CreateCommand(getCmd) + require.Nil(t, err, "failed to create get command") commandResponse, resp := Client.ExecuteCommand(channel.Id, "/getcommand") CheckNoError(t, resp) @@ -597,9 +549,8 @@ func TestExecutePostCommand(t *testing.T) { Token: token, } - if _, err := th.App.CreateCommand(postCmd); err != nil { - t.Fatal("failed to create get command") - } + _, err := th.App.CreateCommand(postCmd) + require.Nil(t, err, "failed to create get command") commandResponse, resp := Client.ExecuteCommand(channel.Id, "/postcommand") CheckNoError(t, resp) @@ -652,9 +603,8 @@ func TestExecuteCommandAgainstChannelOnAnotherTeam(t *testing.T) { Method: model.COMMAND_METHOD_POST, Trigger: "postcommand", } - if _, err := th.App.CreateCommand(postCmd); err != nil { - t.Fatal("failed to create post command") - } + _, err := th.App.CreateCommand(postCmd) + require.Nil(t, err, "failed to create post command") // the execute command endpoint will always search for the command by trigger and team id, inferring team id from the // channel id, so there is no way to use that slash command on a channel that belongs to some other team @@ -702,15 +652,13 @@ func TestExecuteCommandAgainstChannelUserIsNotIn(t *testing.T) { Method: model.COMMAND_METHOD_POST, Trigger: "postcommand", } - if _, err := th.App.CreateCommand(postCmd); err != nil { - t.Fatal("failed to create post command") - } + _, err := th.App.CreateCommand(postCmd) + require.Nil(t, err, "failed to create post command") // make a channel on that team, ensuring that our test user isn't in it channel2 := th.CreateChannelWithClientAndTeam(client, model.CHANNEL_OPEN, team2.Id) - if success, _ := client.RemoveUserFromChannel(channel2.Id, th.BasicUser.Id); !success { - t.Fatal("Failed to remove user from channel") - } + success, _ := client.RemoveUserFromChannel(channel2.Id, th.BasicUser.Id) + require.True(t, success, "Failed to remove user from channel") // we should not be able to run the slash command in channel2, because we aren't in it _, resp := client.ExecuteCommandWithTeam(channel2.Id, team2.Id, "/postcommand") @@ -760,9 +708,8 @@ func TestExecuteCommandInDirectMessageChannel(t *testing.T) { Method: model.COMMAND_METHOD_POST, Trigger: "postcommand", } - if _, err := th.App.CreateCommand(postCmd); err != nil { - t.Fatal("failed to create post command") - } + _, err := th.App.CreateCommand(postCmd) + require.Nil(t, err, "failed to create post command") // make a direct message channel dmChannel, response := client.CreateDirectChannel(th.BasicUser.Id, th.BasicUser2.Id) @@ -823,9 +770,8 @@ func TestExecuteCommandInTeamUserIsNotOn(t *testing.T) { Method: model.COMMAND_METHOD_POST, Trigger: "postcommand", } - if _, err := th.App.CreateCommand(postCmd); err != nil { - t.Fatal("failed to create post command") - } + _, err := th.App.CreateCommand(postCmd) + require.Nil(t, err, "failed to create post command") // make a direct message channel dmChannel, response := client.CreateDirectChannel(th.BasicUser.Id, th.BasicUser2.Id) @@ -836,9 +782,9 @@ func TestExecuteCommandInTeamUserIsNotOn(t *testing.T) { CheckOKStatus(t, resp) // if the user is removed from the team, they should NOT be able to run the slash command in the DM channel - if success, _ := client.RemoveTeamMember(team2.Id, th.BasicUser.Id); !success { - t.Fatal("Failed to remove user from team") - } + success, _ := client.RemoveTeamMember(team2.Id, th.BasicUser.Id) + require.True(t, success, "Failed to remove user from team") + _, resp = client.ExecuteCommandWithTeam(dmChannel.Id, team2.Id, "/postcommand") CheckForbiddenStatus(t, resp) From 4f8fd3e59e4ed31e6840bbf901352201ee0d08ae Mon Sep 17 00:00:00 2001 From: Laura Pareja Date: Thu, 24 Oct 2019 16:59:18 +0200 Subject: [PATCH 16/45] migrate tests from api4/status_test.go to testify (#12906) --- api4/status_test.go | 65 ++++++++++++--------------------------------- 1 file changed, 17 insertions(+), 48 deletions(-) diff --git a/api4/status_test.go b/api4/status_test.go index ced87c0d7b..5d47a9ead3 100644 --- a/api4/status_test.go +++ b/api4/status_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/mattermost/mattermost-server/model" + "github.com/stretchr/testify/assert" ) func TestGetUserStatus(t *testing.T) { @@ -13,44 +14,32 @@ func TestGetUserStatus(t *testing.T) { userStatus, resp := Client.GetUserStatus(th.BasicUser.Id, "") CheckNoError(t, resp) - if userStatus.Status != "offline" { - t.Fatal("Should return offline status") - } + assert.Equal(t, "offline", userStatus.Status) th.App.SetStatusOnline(th.BasicUser.Id, true) userStatus, resp = Client.GetUserStatus(th.BasicUser.Id, "") CheckNoError(t, resp) - if userStatus.Status != "online" { - t.Fatal("Should return online status") - } + assert.Equal(t, "online", userStatus.Status) th.App.SetStatusAwayIfNeeded(th.BasicUser.Id, true) userStatus, resp = Client.GetUserStatus(th.BasicUser.Id, "") CheckNoError(t, resp) - if userStatus.Status != "away" { - t.Fatal("Should return away status") - } + assert.Equal(t, "away", userStatus.Status) th.App.SetStatusDoNotDisturb(th.BasicUser.Id) userStatus, resp = Client.GetUserStatus(th.BasicUser.Id, "") CheckNoError(t, resp) - if userStatus.Status != "dnd" { - t.Fatal("Should return dnd status") - } + assert.Equal(t, "dnd", userStatus.Status) th.App.SetStatusOffline(th.BasicUser.Id, true) userStatus, resp = Client.GetUserStatus(th.BasicUser.Id, "") CheckNoError(t, resp) - if userStatus.Status != "offline" { - t.Fatal("Should return offline status") - } + assert.Equal(t, "offline", userStatus.Status) //Get user2 status logged as user1 userStatus, resp = Client.GetUserStatus(th.BasicUser2.Id, "") CheckNoError(t, resp) - if userStatus.Status != "offline" { - t.Fatal("Should return offline status") - } + assert.Equal(t, "offline", userStatus.Status) Client.Logout() @@ -60,9 +49,7 @@ func TestGetUserStatus(t *testing.T) { th.LoginBasic2() userStatus, resp = Client.GetUserStatus(th.BasicUser2.Id, "") CheckNoError(t, resp) - if userStatus.Status != "offline" { - t.Fatal("Should return offline status") - } + assert.Equal(t, "offline", userStatus.Status) } func TestGetUsersStatusesByIds(t *testing.T) { @@ -75,9 +62,7 @@ func TestGetUsersStatusesByIds(t *testing.T) { usersStatuses, resp := Client.GetUsersStatusesByIds(usersIds) CheckNoError(t, resp) for _, userStatus := range usersStatuses { - if userStatus.Status != "offline" { - t.Fatal("Status should be offline") - } + assert.Equal(t, "offline", userStatus.Status) } th.App.SetStatusOnline(th.BasicUser.Id, true) @@ -85,9 +70,7 @@ func TestGetUsersStatusesByIds(t *testing.T) { usersStatuses, resp = Client.GetUsersStatusesByIds(usersIds) CheckNoError(t, resp) for _, userStatus := range usersStatuses { - if userStatus.Status != "online" { - t.Fatal("Status should be offline") - } + assert.Equal(t, "online", userStatus.Status) } th.App.SetStatusAwayIfNeeded(th.BasicUser.Id, true) @@ -95,9 +78,7 @@ func TestGetUsersStatusesByIds(t *testing.T) { usersStatuses, resp = Client.GetUsersStatusesByIds(usersIds) CheckNoError(t, resp) for _, userStatus := range usersStatuses { - if userStatus.Status != "away" { - t.Fatal("Status should be offline") - } + assert.Equal(t, "away", userStatus.Status) } th.App.SetStatusDoNotDisturb(th.BasicUser.Id) @@ -105,9 +86,7 @@ func TestGetUsersStatusesByIds(t *testing.T) { usersStatuses, resp = Client.GetUsersStatusesByIds(usersIds) CheckNoError(t, resp) for _, userStatus := range usersStatuses { - if userStatus.Status != "dnd" { - t.Fatal("Status should be offline") - } + assert.Equal(t, "dnd", userStatus.Status) } Client.Logout() @@ -124,30 +103,22 @@ func TestUpdateUserStatus(t *testing.T) { toUpdateUserStatus := &model.Status{Status: "online", UserId: th.BasicUser.Id} updateUserStatus, resp := Client.UpdateUserStatus(th.BasicUser.Id, toUpdateUserStatus) CheckNoError(t, resp) - if updateUserStatus.Status != "online" { - t.Fatal("Should return online status") - } + assert.Equal(t, "online", updateUserStatus.Status) toUpdateUserStatus.Status = "away" updateUserStatus, resp = Client.UpdateUserStatus(th.BasicUser.Id, toUpdateUserStatus) CheckNoError(t, resp) - if updateUserStatus.Status != "away" { - t.Fatal("Should return away status") - } + assert.Equal(t, "away", updateUserStatus.Status) toUpdateUserStatus.Status = "dnd" updateUserStatus, resp = Client.UpdateUserStatus(th.BasicUser.Id, toUpdateUserStatus) CheckNoError(t, resp) - if updateUserStatus.Status != "dnd" { - t.Fatal("Should return dnd status") - } + assert.Equal(t, "dnd", updateUserStatus.Status) toUpdateUserStatus.Status = "offline" updateUserStatus, resp = Client.UpdateUserStatus(th.BasicUser.Id, toUpdateUserStatus) CheckNoError(t, resp) - if updateUserStatus.Status != "offline" { - t.Fatal("Should return offline status") - } + assert.Equal(t, "offline", updateUserStatus.Status) toUpdateUserStatus.Status = "online" toUpdateUserStatus.UserId = th.BasicUser2.Id @@ -156,9 +127,7 @@ func TestUpdateUserStatus(t *testing.T) { toUpdateUserStatus.Status = "online" updateUserStatus, _ = th.SystemAdminClient.UpdateUserStatus(th.BasicUser2.Id, toUpdateUserStatus) - if updateUserStatus.Status != "online" { - t.Fatal("Should return online status") - } + assert.Equal(t, "online", updateUserStatus.Status) _, resp = Client.UpdateUserStatus(th.BasicUser.Id, toUpdateUserStatus) CheckBadRequestStatus(t, resp) From b9329d19841fb4e5f6e2f674ddea31d5d1e94cd8 Mon Sep 17 00:00:00 2001 From: Hossein Ahmadian-Yazdi Date: Thu, 24 Oct 2019 17:31:17 -0400 Subject: [PATCH 17/45] [MM-13704] Updated CLI "deleter user" to also permanently delete GroupMembers associated to user (#12858) * [MM-13704] Updated CLI to set DeleteAt in groupmemebrs * [MM-13704] Updated Mock Test * [MM-13704] Refactored to use current pattern to write function * [MM-13704] Small text change * [MM-13704] updated translation * [MM-13704] Updated params of error function * [MM-13704] Fixed confusion between channel and group store * [MM-13704] Addressed PR comments * [MM-13704] Added test case * [MM-13704] Fixed err declaration --- app/user.go | 4 ++++ i18n/en.json | 4 ++++ store/sqlstore/channel_store.go | 2 +- store/sqlstore/group_store.go | 7 ++++++ store/store.go | 1 + store/storetest/group_store.go | 37 +++++++++++++++++++++++++++++ store/storetest/mocks/GroupStore.go | 16 +++++++++++++ 7 files changed, 70 insertions(+), 1 deletion(-) diff --git a/app/user.go b/app/user.go index da2e016c21..878ce66a31 100644 --- a/app/user.go +++ b/app/user.go @@ -1456,6 +1456,10 @@ func (a *App) PermanentDeleteUser(user *model.User) *model.AppError { return err } + if err := a.Srv.Store.Group().PermanentDeleteMembersByUser(user.Id); err != nil { + return err + } + if err := a.Srv.Store.Post().PermanentDeleteByUser(user.Id); err != nil { return err } diff --git a/i18n/en.json b/i18n/en.json index fa03c92154..6a89c055f4 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -6126,6 +6126,10 @@ "id": "store.sql_group.no_rows_changed", "translation": "no rows changed" }, + { + "id": "store.sql_group.permanent_delete_members_by_user.app_error", + "translation": "Unable to remove the group members with UserID \"{{.UserId}}\"" + }, { "id": "store.sql_group.unique_constraint", "translation": "a group with that name already exists" diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index 308b8bc727..aba5d38d77 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -1790,7 +1790,7 @@ func (s SqlChannelStore) RemoveAllDeactivatedMembers(channelId string) *model.Ap func (s SqlChannelStore) PermanentDeleteMembersByUser(userId string) *model.AppError { if _, err := s.GetMaster().Exec("DELETE FROM ChannelMembers WHERE UserId = :UserId", map[string]interface{}{"UserId": userId}); err != nil { - return model.NewAppError("SqlChannelStore.RemoveMember", "store.sql_channel.permanent_delete_members_by_user.app_error", nil, "user_id="+userId+", "+err.Error(), http.StatusInternalServerError) + return model.NewAppError("SqlChannelStore.ChannelPermanentDeleteMembersByUser", "store.sql_channel.permanent_delete_members_by_user.app_error", nil, "user_id="+userId+", "+err.Error(), http.StatusInternalServerError) } return nil } diff --git a/store/sqlstore/group_store.go b/store/sqlstore/group_store.go index 7ce65b23c4..f0e4555ff3 100644 --- a/store/sqlstore/group_store.go +++ b/store/sqlstore/group_store.go @@ -373,6 +373,13 @@ func (s *SqlGroupStore) DeleteMember(groupID string, userID string) (*model.Grou return retrievedMember, nil } +func (s *SqlGroupStore) PermanentDeleteMembersByUser(userId string) *model.AppError { + if _, err := s.GetMaster().Exec("DELETE FROM GroupMembers WHERE UserId = :UserId", map[string]interface{}{"UserId": userId}); err != nil { + return model.NewAppError("SqlGroupStore.GroupPermanentDeleteMembersByUser", "store.sql_group.permanent_delete_members_by_user.app_error", map[string]interface{}{"UserId": userId}, "", http.StatusInternalServerError) + } + return nil +} + func (s *SqlGroupStore) CreateGroupSyncable(groupSyncable *model.GroupSyncable) (*model.GroupSyncable, *model.AppError) { if err := groupSyncable.IsValid(); err != nil { return nil, err diff --git a/store/store.go b/store/store.go index ab28c0f588..26fb519a7e 100644 --- a/store/store.go +++ b/store/store.go @@ -582,6 +582,7 @@ type GroupStore interface { GetMemberCount(groupID string) (int64, *model.AppError) UpsertMember(groupID string, userID string) (*model.GroupMember, *model.AppError) DeleteMember(groupID string, userID string) (*model.GroupMember, *model.AppError) + PermanentDeleteMembersByUser(userId string) *model.AppError CreateGroupSyncable(groupSyncable *model.GroupSyncable) (*model.GroupSyncable, *model.AppError) GetGroupSyncable(groupID string, syncableID string, syncableType model.GroupSyncableType) (*model.GroupSyncable, *model.AppError) diff --git a/store/storetest/group_store.go b/store/storetest/group_store.go index 0a289cc3ef..125173ef23 100644 --- a/store/storetest/group_store.go +++ b/store/storetest/group_store.go @@ -33,6 +33,7 @@ func TestGroupStore(t *testing.T, ss store.Store) { t.Run("GetMemberUsersPage", func(t *testing.T) { testGroupGetMemberUsersPage(t, ss) }) t.Run("UpsertMember", func(t *testing.T) { testUpsertMember(t, ss) }) t.Run("DeleteMember", func(t *testing.T) { testGroupDeleteMember(t, ss) }) + t.Run("PermanentDeleteMembersByUser", func(t *testing.T) { testGroupPermanentDeleteMembersByUser(t, ss) }) t.Run("CreateGroupSyncable", func(t *testing.T) { testCreateGroupSyncable(t, ss) }) t.Run("GetGroupSyncable", func(t *testing.T) { testGetGroupSyncable(t, ss) }) @@ -762,6 +763,42 @@ func testGroupDeleteMember(t *testing.T, ss store.Store) { require.Equal(t, err.Id, "store.sql_group.no_rows") } +func testGroupPermanentDeleteMembersByUser(t *testing.T, ss store.Store) { + var g *model.Group + var groups []*model.Group + numberOfGroups := 5 + + for i := 0; i < numberOfGroups; i++ { + g = &model.Group{ + Name: model.NewId(), + DisplayName: model.NewId(), + Source: model.GroupSourceLdap, + RemoteId: model.NewId(), + } + group, err := ss.Group().Create(g) + groups = append(groups, group) + require.Nil(t, err) + } + + // Create user + u1 := &model.User{ + Email: MakeEmail(), + Username: model.NewId(), + } + user, err := ss.User().Save(u1) + require.Nil(t, err) + + // Create members + for _, group := range groups { + _, err = ss.Group().UpsertMember(group.Id, user.Id) + require.Nil(t, err) + } + + // Happy path + err = ss.Group().PermanentDeleteMembersByUser(user.Id) + require.Nil(t, err) +} + func testCreateGroupSyncable(t *testing.T, ss store.Store) { // Invalid GroupID _, err := ss.Group().CreateGroupSyncable(model.NewGroupTeam("x", model.NewId(), false)) diff --git a/store/storetest/mocks/GroupStore.go b/store/storetest/mocks/GroupStore.go index e5328e4e50..2c1a7fbefc 100644 --- a/store/storetest/mocks/GroupStore.go +++ b/store/storetest/mocks/GroupStore.go @@ -654,6 +654,22 @@ func (_m *GroupStore) GetMemberUsersPage(groupID string, page int, perPage int) return r0, r1 } +// PermanentDeleteMembersByUser provides a mock function with given fields: userId +func (_m *GroupStore) PermanentDeleteMembersByUser(userId string) *model.AppError { + ret := _m.Called(userId) + + var r0 *model.AppError + if rf, ok := ret.Get(0).(func(string) *model.AppError); ok { + r0 = rf(userId) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*model.AppError) + } + } + + return r0 +} + // TeamMembersMinusGroupMembers provides a mock function with given fields: teamID, groupIDs, page, perPage func (_m *GroupStore) TeamMembersMinusGroupMembers(teamID string, groupIDs []string, page int, perPage int) ([]*model.UserWithGroups, *model.AppError) { ret := _m.Called(teamID, groupIDs, page, perPage) From 6ce97648f4490d410b70f08b984a7286dcadcbb7 Mon Sep 17 00:00:00 2001 From: ptisserand Date: Fri, 25 Oct 2019 04:55:58 +0200 Subject: [PATCH 18/45] MM-19670: Migrate tests to use testify (#12883) (#12898) * MM-19670: Migrate tests to use testify (#12883) * MM-19670: (#12883) Applied changes requested in PR #12898 * MM-19670: (#12883) assert.Equal parameters are 'expected' and then 'actual'. --- utils/time_test.go | 20 ++++++++------------ utils/utils_test.go | 15 ++++----------- 2 files changed, 12 insertions(+), 23 deletions(-) diff --git a/utils/time_test.go b/utils/time_test.go index 451ceb5aa3..8f151ccc2e 100644 --- a/utils/time_test.go +++ b/utils/time_test.go @@ -6,6 +6,8 @@ package utils import ( "testing" "time" + + "github.com/stretchr/testify/assert" ) var format = "2006-01-02 15:04:05.000000000" @@ -15,18 +17,16 @@ func TestMillisFromTime(t *testing.T) { actual := MillisFromTime(input) expected := int64(1420115640000) - if actual != expected { - t.Fatalf("TestMillisFromTime failed, %v=%v", expected, actual) - } + assert.Equal(t, expected, actual) } func TestYesterday(t *testing.T) { actual := Yesterday() expected := time.Now().AddDate(0, 0, -1) - if actual.Year() != expected.Year() || actual.Day() != expected.Day() || actual.Month() != expected.Month() { - t.Fatalf("TestYesterday failed, %v=%v", expected, actual) - } + assert.Equal(t, expected.Year(), actual.Year()) + assert.Equal(t, expected.Day(), actual.Day()) + assert.Equal(t, expected.Month(), actual.Month()) } func TestStartOfDay(t *testing.T) { @@ -34,9 +34,7 @@ func TestStartOfDay(t *testing.T) { actual := StartOfDay(input) expected, _ := time.Parse(format, "2015-01-01 00:00:00.000000000") - if actual != expected { - t.Fatalf("TestStartOfDay failed, %v=%v", expected, actual) - } + assert.Equal(t, expected, actual) } func TestEndOfDay(t *testing.T) { @@ -44,7 +42,5 @@ func TestEndOfDay(t *testing.T) { actual := EndOfDay(input) expected, _ := time.Parse(format, "2015-01-01 23:59:59.999999999") - if actual != expected { - t.Fatalf("TestEndOfDay failed, %v=%v", expected, actual) - } + assert.Equal(t, expected, actual) } diff --git a/utils/utils_test.go b/utils/utils_test.go index 32c7b46acd..b33868b345 100644 --- a/utils/utils_test.go +++ b/utils/utils_test.go @@ -23,13 +23,8 @@ func TestStringArrayIntersection(t *testing.T) { "def", } - if len(StringArrayIntersection(a, b)) != 0 { - t.Fatal("should be 0") - } - - if len(StringArrayIntersection(a, c)) != 1 { - t.Fatal("should be 1") - } + assert.Len(t, StringArrayIntersection(a, b), 0) + assert.Len(t, StringArrayIntersection(a, c), 1) } func TestRemoveDuplicatesFromStringArray(t *testing.T) { @@ -43,9 +38,7 @@ func TestRemoveDuplicatesFromStringArray(t *testing.T) { "a", } - if len(RemoveDuplicatesFromStringArray(a)) != 3 { - t.Fatal("should be 3") - } + assert.Len(t, RemoveDuplicatesFromStringArray(a), 3) } func TestStringSliceDiff(t *testing.T) { @@ -53,7 +46,7 @@ func TestStringSliceDiff(t *testing.T) { b := []string{"two", "seven", "four", "six"} expected := []string{"one", "three", "five"} - assert.Equal(t, StringSliceDiff(a, b), expected) + assert.Equal(t, expected, StringSliceDiff(a, b)) } func TestGetIpAddress(t *testing.T) { From 7c230d542aac40abcb8fc99e679ba0df3d799cc5 Mon Sep 17 00:00:00 2001 From: Veda Depatla Date: Thu, 24 Oct 2019 20:26:37 -0700 Subject: [PATCH 19/45] replace fmt.sprintf (#12776) * replace fmt.sprintf * update error message * Update app/ldap.go Co-Authored-By: Jesse Hallam * Update app/ldap.go Co-Authored-By: Jesse Hallam --- app/ldap.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/ldap.go b/app/ldap.go index 5219ef8249..22a8568a68 100644 --- a/app/ldap.go +++ b/app/ldap.go @@ -52,7 +52,7 @@ func (a *App) GetLdapGroup(ldapGroupID string) (*model.Group, *model.AppError) { } } else { ae := model.NewAppError("GetLdapGroup", "ent.ldap.app_error", nil, "", http.StatusNotImplemented) - mlog.Error(fmt.Sprintf("%v", ae.Error())) + mlog.Error("Unable to use ldap", mlog.String("ldap_group_id", ldapGroupID), mlog.Err(ae)) return nil, ae } @@ -73,7 +73,7 @@ func (a *App) GetAllLdapGroupsPage(page int, perPage int, opts model.LdapGroupSe } } else { ae := model.NewAppError("GetAllLdapGroupsPage", "ent.ldap.app_error", nil, "", http.StatusNotImplemented) - mlog.Error(fmt.Sprintf("%v", ae.Error())) + mlog.Error("Unable to use ldap", mlog.Err(ae)) return nil, 0, ae } From 7cf7e7f2380165ebc553de2ab34d10d49d685b32 Mon Sep 17 00:00:00 2001 From: Rajat Varyani <37879062+RajatVaryani@users.noreply.github.com> Date: Fri, 25 Oct 2019 14:13:13 +0530 Subject: [PATCH 20/45] [MM-18871] Update session in app instance in app layer (#12540) --- api4/user.go | 2 -- app/login.go | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/api4/user.go b/api4/user.go index 315c36e970..81fda670f5 100644 --- a/api4/user.go +++ b/api4/user.go @@ -1422,8 +1422,6 @@ func login(c *Context, w http.ResponseWriter, r *http.Request) { user.TermsOfServiceCreateAt = userTermsOfService.CreateAt } - c.App.Session = *session - user.Sanitize(map[string]bool{}) w.Write([]byte(user.ToJson())) diff --git a/app/login.go b/app/login.go index 8b4a7fcaf7..31f1126fdd 100644 --- a/app/login.go +++ b/app/login.go @@ -162,6 +162,7 @@ func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, } w.Header().Set(model.HEADER_TOKEN, session.Token) + a.Session = *session if pluginsEnvironment := a.GetPluginsEnvironment(); pluginsEnvironment != nil { a.Srv.Go(func() { From 64a377c60c179e43eb4c697e2ae8e66982bf8c0b Mon Sep 17 00:00:00 2001 From: Carlos Tadeu Panato Junior Date: Fri, 25 Oct 2019 10:43:27 +0200 Subject: [PATCH 21/45] use the branch to tag the docker image and also build team edition (#12903) --- build/Jenkinsfile.branch | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/build/Jenkinsfile.branch b/build/Jenkinsfile.branch index 2446270ae2..339c9e3b81 100644 --- a/build/Jenkinsfile.branch +++ b/build/Jenkinsfile.branch @@ -124,7 +124,7 @@ pipeline { withCredentials([usernamePassword(credentialsId: 'matterbuild-docker-hub', usernameVariable: 'DOCKER_USER', passwordVariable: 'DOCKER_PASS')]) { sh 'docker login --username ${DOCKER_USER} --password ${DOCKER_PASS}' sh """ - export TAG='${env.BRANCH_NAME}.${env.BUILD_NUMBER}.${SERVER_GIT_COMMIT}' + export TAG='${env.BRANCH_NAME}' docker build --no-cache --build-arg MM_PACKAGE=https://releases.mattermost.com/mattermost-platform/${env.BRANCH_NAME}/mattermost-enterprise-linux-amd64.tar.gz -t mattermost/mattermost-enterprise-edition:\${TAG} build docker push mattermost/mattermost-enterprise-edition:\${TAG} docker logout @@ -133,6 +133,25 @@ pipeline { } } } + + stage ('Build Docker mattermost-team-edition') { + agent { + label 'default-mm-builder' + } + steps { + dir('src/github.com/mattermost/mattermost-server') { + withCredentials([usernamePassword(credentialsId: 'matterbuild-docker-hub', usernameVariable: 'DOCKER_USER', passwordVariable: 'DOCKER_PASS')]) { + sh 'docker login --username ${DOCKER_USER} --password ${DOCKER_PASS}' + sh """ + export TAG='${env.BRANCH_NAME}' + docker build --no-cache --build-arg MM_PACKAGE=https://releases.mattermost.com/mattermost-platform/${env.BRANCH_NAME}/mattermost-team-linux-amd64.tar.gz -t mattermost/mattermost-team-edition:\${TAG} build + docker push mattermost/mattermost-team-edition:\${TAG} + docker logout + """ + } + } + } + } } post { From 89317487b87ca165c47f4f47051ad58ecc0f950f Mon Sep 17 00:00:00 2001 From: Christopher Speller Date: Fri, 25 Oct 2019 02:00:43 -0700 Subject: [PATCH 22/45] Fixing inconsistant docker build containers. (#12911) --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index d2c96cf9d8..229f271f01 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -232,7 +232,7 @@ jobs: --env MM_ELASTICSEARCHSETTINGS_CONNECTIONURL=http://elasticsearch:9200 \ -v ~/go/src:/go/src \ -w /go/src/github.com/mattermost/mattermost-server \ - mattermost/mattermost-build-server:feb-28-2019 \ + mattermost/mattermost-build-server:oct-18-2019 \ bash -c 'ulimit -n 8096; make ARGS="version" run-cli && make MM_SQLSETTINGS_DATASOURCE="mmuser:mostest@tcp(mysql:3306)/latest?charset=utf8mb4,utf8&readTimeout=30s&writeTimeout=30s" ARGS="version" run-cli' echo "Ignoring known MySQL mismatch: ChannelMembers.SchemeGuest" From a622ba412e5831c7b1142decd2c93a8507943ade Mon Sep 17 00:00:00 2001 From: Adrian Date: Fri, 25 Oct 2019 11:19:56 +0200 Subject: [PATCH 23/45] Use locale-agnostic Firefox download link (#12251) This link redirects to the most suitable language for the user based on their browser's `Accept-Language` header. --- web/unsupported_browser.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/unsupported_browser.go b/web/unsupported_browser.go index 8e5f09d64e..db02ab8a39 100644 --- a/web/unsupported_browser.go +++ b/web/unsupported_browser.go @@ -127,7 +127,7 @@ func renderBrowserFirefox(app *app.App) Browser { "/static/images/browser-icons/firefox.svg", app.T("web.error.unsupported_browser.browser_title.firefox"), app.T("web.error.unsupported_browser.min_browser_version.firefox"), - "https://www.mozilla.org/en-CA/firefox/new/", + "https://www.mozilla.org/firefox/new/", app.T("web.error.unsupported_browser.browser_get_latest.firefox"), } } From 187e4ac7659ce03e16eedb1bca133ecdb5ae389a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Fri, 25 Oct 2019 18:10:34 +0200 Subject: [PATCH 24/45] Unbinding the permission INVITE_GUEST from INVITE_USER (#12908) --- app/permissions_migrations.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/permissions_migrations.go b/app/permissions_migrations.go index 31ecc8318f..d4fe8019fe 100644 --- a/app/permissions_migrations.go +++ b/app/permissions_migrations.go @@ -282,11 +282,7 @@ func getAddManageGuestsPermissionsMigration() permissionsMap { return permissionsMap{ permissionTransformation{ On: isRole(model.SYSTEM_ADMIN_ROLE_ID), - Add: []string{PERMISSION_PROMOTE_GUEST, PERMISSION_DEMOTE_TO_GUEST}, - }, - permissionTransformation{ - On: permissionExists(PERMISSION_INVITE_USER), - Add: []string{PERMISSION_INVITE_GUEST}, + Add: []string{PERMISSION_PROMOTE_GUEST, PERMISSION_DEMOTE_TO_GUEST, PERMISSION_INVITE_GUEST}, }, } } From c0186cef1a62d275a6074641d13490839a5553ff Mon Sep 17 00:00:00 2001 From: Abu Hanifa Date: Fri, 25 Oct 2019 23:49:33 +0600 Subject: [PATCH 25/45] Conv emoji to testify (#12900) * convert fatal to requrie Signed-off-by: hanif * finish convert Signed-off-by: hanif * change require.Errof to require.Failf Signed-off-by: hanif * simplify conditions Signed-off-by: hanif --- utils/emoji.go | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/utils/emoji.go b/utils/emoji.go index d7e8a8b126..6797e5e0a2 100644 --- a/utils/emoji.go +++ b/utils/emoji.go @@ -11,14 +11,15 @@ import ( "image/jpeg" "image/png" "testing" + + "github.com/stretchr/testify/require" ) func CreateTestGif(t *testing.T, width int, height int) []byte { var buffer bytes.Buffer - if err := gif.Encode(&buffer, image.NewRGBA(image.Rect(0, 0, width, height)), nil); err != nil { - t.Fatalf("failed to create gif: %v", err.Error()) - } + err := gif.Encode(&buffer, image.NewRGBA(image.Rect(0, 0, width, height)), nil) + require.NoErrorf(t, err, "failed to create gif: %v", err) return buffer.Bytes() } @@ -34,9 +35,8 @@ func CreateTestAnimatedGif(t *testing.T, width int, height int, frames int) []by img.Image[i] = image.NewPaletted(image.Rect(0, 0, width, height), color.Palette{color.Black}) img.Delay[i] = 0 } - if err := gif.EncodeAll(&buffer, &img); err != nil { - t.Fatalf("failed to create animated gif: %v", err.Error()) - } + err := gif.EncodeAll(&buffer, &img) + require.NoErrorf(t, err, "failed to create animated gif: %v", err) return buffer.Bytes() } @@ -44,9 +44,8 @@ func CreateTestAnimatedGif(t *testing.T, width int, height int, frames int) []by func CreateTestJpeg(t *testing.T, width int, height int) []byte { var buffer bytes.Buffer - if err := jpeg.Encode(&buffer, image.NewRGBA(image.Rect(0, 0, width, height)), nil); err != nil { - t.Fatalf("failed to create jpeg: %v", err.Error()) - } + err := jpeg.Encode(&buffer, image.NewRGBA(image.Rect(0, 0, width, height)), nil) + require.NoErrorf(t, err, "failed to create jpeg: %v", err) return buffer.Bytes() } @@ -54,9 +53,8 @@ func CreateTestJpeg(t *testing.T, width int, height int) []byte { func CreateTestPng(t *testing.T, width int, height int) []byte { var buffer bytes.Buffer - if err := png.Encode(&buffer, image.NewRGBA(image.Rect(0, 0, width, height))); err != nil { - t.Fatalf("failed to create png: %v", err.Error()) - } + err := png.Encode(&buffer, image.NewRGBA(image.Rect(0, 0, width, height))) + require.NoErrorf(t, err, "failed to create png: %v", err) return buffer.Bytes() } From f58e2ffd0f31c6c003d64a9d363905f94bfe5481 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Guillermo=20Vay=C3=A1?= Date: Fri, 25 Oct 2019 19:58:48 +0200 Subject: [PATCH 26/45] [MM-19344] add option to render latex (#12907) --- app/diagnostics.go | 1 + config/client.go | 1 + model/config.go | 9 +++++++++ 3 files changed, 11 insertions(+) diff --git a/app/diagnostics.go b/app/diagnostics.go index ac7203156d..08d9c150e3 100644 --- a/app/diagnostics.go +++ b/app/diagnostics.go @@ -300,6 +300,7 @@ func (a *App) trackConfig() { "disable_bots_when_owner_is_deactivated": *cfg.ServiceSettings.DisableBotsWhenOwnerIsDeactivated, "enable_bot_account_creation": *cfg.ServiceSettings.EnableBotAccountCreation, "enable_svgs": *cfg.ServiceSettings.EnableSVGs, + "enable_latex": *cfg.ServiceSettings.EnableLatex, }) a.SendDiagnostic(TRACK_CONFIG_TEAM, map[string]interface{}{ diff --git a/config/client.go b/config/client.go index 3c1eca98cb..a0f47f86a4 100644 --- a/config/client.go +++ b/config/client.go @@ -44,6 +44,7 @@ func GenerateClientConfig(c *model.Config, diagnosticId string, license *model.L props["ExperimentalGroupUnreadChannels"] = *c.ServiceSettings.ExperimentalGroupUnreadChannels props["EnableSVGs"] = strconv.FormatBool(*c.ServiceSettings.EnableSVGs) props["EnableMarketplace"] = strconv.FormatBool(*c.PluginSettings.EnableMarketplace) + props["EnableLatex"] = strconv.FormatBool(*c.ServiceSettings.EnableLatex) // This setting is only temporary, so keep using the old setting name for the mobile and web apps props["ExperimentalEnablePostMetadata"] = "true" diff --git a/model/config.go b/model/config.go index 9fd2d291e3..12079f4126 100644 --- a/model/config.go +++ b/model/config.go @@ -325,6 +325,7 @@ type ServiceSettings struct { DisableBotsWhenOwnerIsDeactivated *bool `restricted:"true"` EnableBotAccountCreation *bool EnableSVGs *bool + EnableLatex *bool } func (s *ServiceSettings) SetDefaults(isUpdate bool) { @@ -689,6 +690,14 @@ func (s *ServiceSettings) SetDefaults(isUpdate bool) { s.EnableSVGs = NewBool(false) } } + + if s.EnableLatex == nil { + if isUpdate { + s.EnableLatex = NewBool(true) + } else { + s.EnableLatex = NewBool(false) + } + } } type ClusterSettings struct { From 37e042497717fab807fc8742c4ae771af9f5c255 Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Fri, 25 Oct 2019 20:02:18 +0200 Subject: [PATCH 27/45] Include request_id in HTTP request logs (#12880) --- web/handlers.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/web/handlers.go b/web/handlers.go index a666501f6d..ea2885ba13 100644 --- a/web/handlers.go +++ b/web/handlers.go @@ -72,14 +72,16 @@ type Handler struct { func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { now := time.Now() - mlog.Debug("request:", mlog.String("method", r.Method), mlog.String("url", r.URL.Path)) + + requestID := model.NewId() + mlog.Debug("Received HTTP request", mlog.String("method", r.Method), mlog.String("url", r.URL.Path), mlog.String("request_id", requestID)) c := &Context{} c.App = app.New( h.GetGlobalAppOptions()..., ) c.App.T, _ = utils.GetTranslationsAndLocale(w, r) - c.App.RequestId = model.NewId() + c.App.RequestId = requestID c.App.IpAddress = utils.GetIpAddress(r, c.App.Config().ServiceSettings.TrustedProxyIPHeader) c.App.UserAgent = r.UserAgent() c.App.AcceptLanguage = r.Header.Get("Accept-Language") From 3be92856b5810cfc4df981a489e34ed0c43bb2a5 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Sat, 26 Oct 2019 12:42:07 +0530 Subject: [PATCH 28/45] MM-19172: Add golangci-lint to the CI pipeline (#12909) To start off with, we are using the new-from-rev=HEAD~ option which just checks the current commit. This allows us to quickly integrate golangci-lint and not spend time in fixing all the outstanding issues. Things pending: - Slowly fix the existing issues. To test them, just uncomment the "new-from-rev: HEAD~" line from .golangci.yml and have at it. - There are a number of unused functions and methods which are only invoked from enterprise code. We are ignoring them for now because removing them will stop enterprise build from working. The correct solution here is to use a build tag to separate TE and EE code. As a long term goal, we would want to use that build tag throughout the EE codebase and remove the TE_PACKAGES and EE_PACKAGES variables in the Makefile and just use the build tag. That makes things a lot cleaner and avoids the need to spawn a "go list" every time to get the correct list of packages. --- .golangci.yml | 30 ++++++++++++++++++++++++++++++ Makefile | 13 +++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 .golangci.yml diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000000..0f14da58b6 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,30 @@ +run: + timeout: 5m + modules-download-mode: vendor + +linters-settings: + govet: + check-shadowing: true + +linters: + disable-all: true + enable: + - deadcode + - gofmt + - gosimple + - govet + - ineffassign + - structcheck + - unconvert + - unused + - varcheck + # TODO: enable this later + # - errcheck + +issues: + exclude-rules: + - linters: + # ignore unused warnings from enterprise code + # add more as required. + - unused + text: "RedisSupplier|LocalCacheSupplier|Enterprise" diff --git a/Makefile b/Makefile index f799b8e966..5b159471d0 100644 --- a/Makefile +++ b/Makefile @@ -173,6 +173,18 @@ gofmt: ## Runs gofmt against all packages. done @echo "gofmt success"; \ +golangci-lint: +# https://stackoverflow.com/a/677212/1027058 (check if a command exists or not) +# https://github.com/golangci/golangci-lint#binary-release +# It is recommended to NOT use go get, but instead use a binary release pinned to a version. + @if ! [ -x "$$(command -v golangci-lint)" ]; then \ + echo "golangci-lint is not installed. Please run: curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s -- -b $(GOPATH)/bin v1.21.0"; \ + exit 1; \ + fi; \ + + @echo Running golangci-lint + $(GOPATH)/bin/golangci-lint run + megacheck: ## Run megacheck on codebasis env GO111MODULE=off go get -u honnef.co/go/tools/cmd/megacheck $(GOPATH)/bin/megacheck $(TE_PACKAGES) @@ -219,6 +231,7 @@ check-licenses: ## Checks license status. check-prereqs: ## Checks prerequisite software status. ./scripts/prereq-check.sh +# TODO: remove govet and gofmt checks once golangci-lint is being enforced. check-style: govet gofmt check-licenses ## Runs govet and gofmt against all packages. test-te-race: ## Checks for race conditions in the team edition. From 78c2780bc79e79c11f9adde58d8e53f79a789c86 Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Sat, 26 Oct 2019 23:38:07 +0900 Subject: [PATCH 29/45] preference_test: use testify (#12933) --- api4/preference_test.go | 51 +++++++++++++++-------------------------- 1 file changed, 19 insertions(+), 32 deletions(-) diff --git a/api4/preference_test.go b/api4/preference_test.go index 41681ff69c..114c73ff4e 100644 --- a/api4/preference_test.go +++ b/api4/preference_test.go @@ -8,6 +8,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + "github.com/mattermost/mattermost-server/model" ) @@ -42,14 +44,10 @@ func TestGetPreferences(t *testing.T) { prefs, resp := Client.GetPreferences(user1.Id) CheckNoError(t, resp) - if len(prefs) != 4 { - t.Fatal("received the wrong number of preferences") - } + require.Equal(t, len(prefs), 4, "received the wrong number of preferences") for _, preference := range prefs { - if preference.UserId != th.BasicUser.Id { - t.Fatal("user id does not match") - } + require.Equal(t, preference.UserId, th.BasicUser.Id, "user id does not match") } th.LoginBasic2() @@ -57,9 +55,7 @@ func TestGetPreferences(t *testing.T) { prefs, resp = Client.GetPreferences(th.BasicUser2.Id) CheckNoError(t, resp) - if len(prefs) == 0 { - t.Fatal("received the wrong number of preferences") - } + require.Greater(t, len(prefs), 0, "received the wrong number of preferences") _, resp = Client.GetPreferences(th.BasicUser.Id) CheckForbiddenStatus(t, resp) @@ -101,9 +97,7 @@ func TestGetPreferencesByCategory(t *testing.T) { prefs, resp := Client.GetPreferencesByCategory(user1.Id, category) CheckNoError(t, resp) - if len(prefs) != 2 { - t.Fatalf("received the wrong number of preferences %v:%v", len(prefs), 2) - } + require.Equal(t, len(prefs), 2, "received the wrong number of preferences") _, resp = Client.GetPreferencesByCategory(user1.Id, "junk") CheckNotFoundStatus(t, resp) @@ -119,9 +113,7 @@ func TestGetPreferencesByCategory(t *testing.T) { prefs, resp = Client.GetPreferencesByCategory(th.BasicUser2.Id, "junk") CheckNotFoundStatus(t, resp) - if len(prefs) != 0 { - t.Fatal("received the wrong number of preferences") - } + require.Equal(t, len(prefs), 0, "received the wrong number of preferences") Client.Logout() _, resp = Client.GetPreferencesByCategory(th.BasicUser2.Id, category) @@ -158,9 +150,9 @@ func TestGetPreferenceByCategoryAndName(t *testing.T) { pref, resp := Client.GetPreferenceByCategoryAndName(user.Id, model.PREFERENCE_CATEGORY_DIRECT_CHANNEL_SHOW, name) CheckNoError(t, resp) - if (pref.UserId != preferences[0].UserId) && (pref.Category != preferences[0].Category) && (pref.Name != preferences[0].Name) { - t.Fatal("preference saved incorrectly") - } + require.Equal(t, preferences[0].UserId, pref.UserId, "UserId preference not saved") + require.Equal(t, preferences[0].Category, pref.Category, "Category preference not saved") + require.Equal(t, preferences[0].Name, pref.Name, "Name preference not saved") preferences[0].Value = model.NewId() Client.UpdatePreferences(user.Id, &preferences) @@ -247,15 +239,12 @@ func TestUpdatePreferencesWebsocket(t *testing.T) { defer th.TearDown() WebSocketClient, err := th.CreateWebSocketClient() - if err != nil { - t.Fatal(err) - } + require.Nil(t, err) WebSocketClient.Listen() time.Sleep(300 * time.Millisecond) - if resp := <-WebSocketClient.ResponseChannel; resp.Status != model.STATUS_OK { - t.Fatal("should have responded OK to authentication challenge") - } + wsResp := <-WebSocketClient.ResponseChannel + require.Equal(t, wsResp.Status, model.STATUS_OK, "expected OK from auth challenge") userId := th.BasicUser.Id preferences := &model.Preferences{ @@ -285,19 +274,17 @@ func TestUpdatePreferencesWebsocket(t *testing.T) { } received, err := model.PreferencesFromJson(strings.NewReader(event.Data["preferences"].(string))) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) - for i, preference := range *preferences { - if preference.UserId != received[i].UserId || preference.Category != received[i].Category || preference.Name != received[i].Name { - t.Fatal("received incorrect preference") - } + for i, p := range *preferences { + require.Equal(t, received[i].UserId, p.UserId, "received incorrect UserId") + require.Equal(t, received[i].Category, p.Category, "received incorrect Category") + require.Equal(t, received[i].Name, p.Name, "received incorrect Name") } waiting = false case <-timeout: - t.Fatal("timed out waiting for preference update event") + require.Fail(t, "timed timed out waiting for preference update event") } } } From cf6496bb8f78cf8b23d53ab0d8a24832971ef265 Mon Sep 17 00:00:00 2001 From: chahat arora <42025842+chahat-arora@users.noreply.github.com> Date: Sun, 27 Oct 2019 16:12:21 +0530 Subject: [PATCH 30/45] Migrate tests from app/command_test.go to use testify (#12659) * Migrate tests from app/command_test.go to use testify * minor fix * minor fix * minor fix --- cmd/mattermost/commands/command_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/cmd/mattermost/commands/command_test.go b/cmd/mattermost/commands/command_test.go index b3e304c4da..63227457f6 100644 --- a/cmd/mattermost/commands/command_test.go +++ b/cmd/mattermost/commands/command_test.go @@ -119,13 +119,11 @@ func TestCreateCommand(t *testing.T) { require.Nil(t, response.Error, "Failed to list commands") if testCase.ExpectedErr == "" { - require.NotEmpty(t, cmds, "Failed to create command") - require.Equal(t, "testcmd", cmds[0].Trigger) + assert.NotZero(t, len(cmds), "Failed to create command") + assert.Equal(t, cmds[0].Trigger, "testcmd", "Failed to create command") assert.Contains(t, string(actual), "PASS") } else { - if len(cmds) > 1 { - require.Fail(t, "Created command that shouldn't have been created") - } + assert.LessOrEqual(t, len(cmds), 1, "Created command that shouldn't have been created") assert.Contains(t, string(actual), testCase.ExpectedErr) } }) From 4d962aaade34b03ab56e55b29d49f65d69e6fc98 Mon Sep 17 00:00:00 2001 From: Igor Zibarev Date: Sun, 27 Oct 2019 10:47:15 +0000 Subject: [PATCH 31/45] Migrate tests from "api4/job_test.go" to use testify (#12897) Fixes #12887 --- api4/job_test.go | 52 ++++++++++++++++++------------------------------ 1 file changed, 19 insertions(+), 33 deletions(-) diff --git a/api4/job_test.go b/api4/job_test.go index 61172897de..e2426c5478 100644 --- a/api4/job_test.go +++ b/api4/job_test.go @@ -23,7 +23,7 @@ func TestCreateJob(t *testing.T) { } received, resp := th.SystemAdminClient.CreateJob(job) - CheckNoError(t, resp) + require.Nil(t, resp.Error) defer th.App.Srv.Store.Job().Delete(received.Id) @@ -52,11 +52,10 @@ func TestGetJob(t *testing.T) { defer th.App.Srv.Store.Job().Delete(job.Id) received, resp := th.SystemAdminClient.GetJob(job.Id) - CheckNoError(t, resp) + require.Nil(t, resp.Error) - if received.Id != job.Id || received.Status != job.Status { - t.Fatal("incorrect job received") - } + require.Equal(t, job.Id, received.Id, "incorrect job received") + require.Equal(t, job.Status, received.Status, "incorrect job received") _, resp = th.SystemAdminClient.GetJob("1234") CheckBadRequestStatus(t, resp) @@ -100,22 +99,16 @@ func TestGetJobs(t *testing.T) { } received, resp := th.SystemAdminClient.GetJobs(0, 2) - CheckNoError(t, resp) + require.Nil(t, resp.Error) - if len(received) != 2 { - t.Fatal("received wrong number of jobs") - } else if received[0].Id != jobs[2].Id { - t.Fatal("should've received newest job first") - } else if received[1].Id != jobs[0].Id { - t.Fatal("should've received second newest job second") - } + require.Len(t, received, 2, "received wrong number of jobs") + require.Equal(t, jobs[2].Id, received[0].Id, "should've received newest job first") + require.Equal(t, jobs[0].Id, received[1].Id, "should've received second newest job second") received, resp = th.SystemAdminClient.GetJobs(1, 2) - CheckNoError(t, resp) + require.Nil(t, resp.Error) - if received[0].Id != jobs[1].Id { - t.Fatal("should've received oldest job last") - } + require.Equal(t,jobs[1].Id, received[0].Id, "should've received oldest job last") _, resp = th.Client.GetJobs(0, 60) CheckForbiddenStatus(t, resp) @@ -157,24 +150,17 @@ func TestGetJobsByType(t *testing.T) { } received, resp := th.SystemAdminClient.GetJobsByType(jobType, 0, 2) - CheckNoError(t, resp) + require.Nil(t, resp.Error) - if len(received) != 2 { - t.Fatal("received wrong number of jobs") - } else if received[0].Id != jobs[2].Id { - t.Fatal("should've received newest job first") - } else if received[1].Id != jobs[0].Id { - t.Fatal("should've received second newest job second") - } + require.Len(t, received, 2, "received wrong number of jobs") + require.Equal(t, jobs[2].Id, received[0].Id, "should've received newest job first") + require.Equal(t, jobs[0].Id, received[1].Id, "should've received second newest job second") received, resp = th.SystemAdminClient.GetJobsByType(jobType, 1, 2) - CheckNoError(t, resp) + require.Nil(t, resp.Error) - if len(received) != 1 { - t.Fatal("received wrong number of jobs") - } else if received[0].Id != jobs[1].Id { - t.Fatal("should've received oldest job last") - } + require.Len(t, received, 1, "received wrong number of jobs") + require.Equal(t, jobs[1].Id, received[0].Id, "should've received oldest job last") _, resp = th.SystemAdminClient.GetJobsByType("", 0, 60) CheckNotFoundStatus(t, resp) @@ -218,10 +204,10 @@ func TestCancelJob(t *testing.T) { CheckForbiddenStatus(t, resp) _, resp = th.SystemAdminClient.CancelJob(jobs[0].Id) - CheckNoError(t, resp) + require.Nil(t, resp.Error) _, resp = th.SystemAdminClient.CancelJob(jobs[1].Id) - CheckNoError(t, resp) + require.Nil(t, resp.Error) _, resp = th.SystemAdminClient.CancelJob(jobs[2].Id) CheckInternalErrorStatus(t, resp) From 3701a393e57f48208cbdf81d61735df800637e1a Mon Sep 17 00:00:00 2001 From: Agus Mistiawan Date: Sun, 27 Oct 2019 18:54:19 +0700 Subject: [PATCH 32/45] Migrate tests from "api4/apitestlib.go" to use testify (#12817) * migrate tests to use testify * check resp error * require empty auth * fix based on comment * change to require * change all assert to require * remove line --- api4/apitestlib.go | 63 +++++++++++----------------------------------- 1 file changed, 15 insertions(+), 48 deletions(-) diff --git a/api4/apitestlib.go b/api4/apitestlib.go index 1746168e5f..8be1af367f 100644 --- a/api4/apitestlib.go +++ b/api4/apitestlib.go @@ -5,13 +5,10 @@ package api4 import ( "fmt" - "github.com/stretchr/testify/require" "io/ioutil" "net" "net/http" "os" - "reflect" - "strconv" "strings" "testing" "time" @@ -27,6 +24,7 @@ import ( s3 "github.com/minio/minio-go/v6" "github.com/minio/minio-go/v6/pkg/credentials" + "github.com/stretchr/testify/require" ) type TestHelper struct { @@ -561,57 +559,34 @@ func GenerateTestId() string { func CheckUserSanitization(t *testing.T, user *model.User) { t.Helper() - if user.Password != "" { - t.Fatal("password wasn't blank") - } - - if user.AuthData != nil && *user.AuthData != "" { - t.Fatal("auth data wasn't blank") - } - - if user.MfaSecret != "" { - t.Fatal("mfa secret wasn't blank") - } + require.Equal(t, "", user.Password, "password wasn't blank") + require.Empty(t, user.AuthData, "auth data wasn't blank") + require.Equal(t, "", user.MfaSecret, "mfa secret wasn't blank") } func CheckEtag(t *testing.T, data interface{}, resp *model.Response) { t.Helper() - if !reflect.ValueOf(data).IsNil() { - t.Fatal("etag data was not nil") - } - - if resp.StatusCode != http.StatusNotModified { - t.Log("actual: " + strconv.Itoa(resp.StatusCode)) - t.Log("expected: " + strconv.Itoa(http.StatusNotModified)) - t.Fatal("wrong status code for etag") - } + require.Empty(t, data) + require.Equal(t, resp.StatusCode, http.StatusNotModified, "wrong status code for etag") } func CheckNoError(t *testing.T, resp *model.Response) { t.Helper() - if resp.Error != nil { - t.Fatalf("Expected no error, got %q", resp.Error.Error()) - } + require.Nil(t, resp.Error) } func checkHTTPStatus(t *testing.T, resp *model.Response, expectedStatus int, expectError bool) { t.Helper() - switch { - case resp == nil: - t.Fatalf("Unexpected nil response, expected http:%v, expectError:%v)", expectedStatus, expectError) - - case expectError && resp.Error == nil: - t.Fatalf("Expected a non-nil error and http status:%v, got nil, %v", expectedStatus, resp.StatusCode) - - case !expectError && resp.Error != nil: - t.Fatalf("Expected no error and http status:%v, got %q, http:%v", expectedStatus, resp.Error, resp.StatusCode) - - case resp.StatusCode != expectedStatus: - t.Fatalf("Expected http status:%v, got %v (err: %q)", expectedStatus, resp.StatusCode, resp.Error) + require.NotNilf(t, resp, "Unexpected nil response, expected http:%v, expectError:%v", expectedStatus, expectError) + if expectError { + require.NotNil(t, resp.Error, "Expected a non-nil error and http status:%v, got nil, %v", expectedStatus, resp.StatusCode) + } else { + require.Nil(t, resp.Error, "Expected no error and http status:%v, got %q, http:%v", expectedStatus, resp.Error, resp.StatusCode) } + require.Equalf(t, expectedStatus, resp.StatusCode, "Expected http status:%v, got %v (err: %q)", expectedStatus, resp.StatusCode, resp.Error) } func CheckOKStatus(t *testing.T, resp *model.Response) { @@ -662,16 +637,8 @@ func CheckInternalErrorStatus(t *testing.T, resp *model.Response) { func CheckErrorMessage(t *testing.T, resp *model.Response, errorId string) { t.Helper() - if resp.Error == nil { - t.Fatal("should have errored with message:" + errorId) - return - } - - if resp.Error.Id != errorId { - t.Log("actual: " + resp.Error.Id) - t.Log("expected: " + errorId) - t.Fatal("incorrect error message") - } + require.NotNil(t, resp.Error) + require.Equal(t, resp.Error.Id, errorId, "incorrect error message") } func CheckStartsWith(t *testing.T, value, prefix, message string) { From 8a72ed28330b789b4cc1d244c44d54779b23094e Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Mon, 28 Oct 2019 10:57:21 +0100 Subject: [PATCH 33/45] Fix unnecessary type conversions (#12927) --- api4/post.go | 2 +- api4/team.go | 2 +- app/web_hub_test.go | 4 +- cmd/mattermost/commands/channel_test.go | 8 ++-- cmd/mattermost/commands/command_test.go | 56 ++++++++++++------------- cmd/mattermost/commands/config_test.go | 26 ++++++------ cmd/mattermost/commands/group_test.go | 16 +++---- mlog/stdlog.go | 2 +- model/integration_action.go | 2 +- services/imageproxy/local.go | 3 +- store/storetest/group_store.go | 10 ++--- 11 files changed, 65 insertions(+), 66 deletions(-) diff --git a/api4/post.go b/api4/post.go index 1441a1dd9f..095e4d4fbb 100644 --- a/api4/post.go +++ b/api4/post.go @@ -475,7 +475,7 @@ func searchPosts(c *Context, w http.ResponseWriter, r *http.Request) { startTime := time.Now() - results, err := c.App.SearchPostsInTeamForUser(terms, c.App.Session.UserId, c.Params.TeamId, isOrSearch, includeDeletedChannels, int(timeZoneOffset), page, perPage) + results, err := c.App.SearchPostsInTeamForUser(terms, c.App.Session.UserId, c.Params.TeamId, isOrSearch, includeDeletedChannels, timeZoneOffset, page, perPage) elapsedTime := float64(time.Since(startTime)) / float64(time.Second) metrics := c.App.Metrics diff --git a/api4/team.go b/api4/team.go index 7891f68074..391376e332 100644 --- a/api4/team.go +++ b/api4/team.go @@ -911,7 +911,7 @@ func importTeam(c *Context, w http.ResponseWriter, r *http.Request) { } data := map[string]string{} - data["results"] = base64.StdEncoding.EncodeToString([]byte(log.Bytes())) + data["results"] = base64.StdEncoding.EncodeToString(log.Bytes()) if c.Err != nil { w.WriteHeader(c.Err.StatusCode) } diff --git a/app/web_hub_test.go b/app/web_hub_test.go index 27b922a3a3..780c10b6e7 100644 --- a/app/web_hub_test.go +++ b/app/web_hub_test.go @@ -50,7 +50,7 @@ func TestHubStopWithMultipleConnections(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() - s := httptest.NewServer(http.HandlerFunc(dummyWebsocketHandler(t))) + s := httptest.NewServer(dummyWebsocketHandler(t)) defer s.Close() th.App.HubStart() @@ -68,7 +68,7 @@ func TestHubStopRaceCondition(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() - s := httptest.NewServer(http.HandlerFunc(dummyWebsocketHandler(t))) + s := httptest.NewServer(dummyWebsocketHandler(t)) th.App.HubStart() wc1 := registerDummyWebConn(t, th.App, s.Listener.Addr(), th.BasicUser.Id) diff --git a/cmd/mattermost/commands/channel_test.go b/cmd/mattermost/commands/channel_test.go index e6109a7d7a..d6003adc3b 100644 --- a/cmd/mattermost/commands/channel_test.go +++ b/cmd/mattermost/commands/channel_test.go @@ -86,17 +86,17 @@ func TestListChannels(t *testing.T) { output := th.CheckCommand(t, "channel", "list", th.BasicTeam.Name) - require.True(t, strings.Contains(string(output), "town-square"), "should have channels") + require.True(t, strings.Contains(output, "town-square"), "should have channels") - require.True(t, strings.Contains(string(output), channel.Name+" (archived)"), "should have archived channel") + require.True(t, strings.Contains(output, channel.Name+" (archived)"), "should have archived channel") - require.True(t, strings.Contains(string(output), privateChannel.Name+" (private)"), "should have private channel") + require.True(t, strings.Contains(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) - require.True(t, strings.Contains(string(output), privateChannel.Name+" (archived) (private)"), "should have a channel both archived and private") + require.True(t, strings.Contains(output, privateChannel.Name+" (archived) (private)"), "should have a channel both archived and private") } func TestRestoreChannel(t *testing.T) { diff --git a/cmd/mattermost/commands/command_test.go b/cmd/mattermost/commands/command_test.go index 63227457f6..3d599e0136 100644 --- a/cmd/mattermost/commands/command_test.go +++ b/cmd/mattermost/commands/command_test.go @@ -121,10 +121,10 @@ func TestCreateCommand(t *testing.T) { if testCase.ExpectedErr == "" { assert.NotZero(t, len(cmds), "Failed to create command") assert.Equal(t, cmds[0].Trigger, "testcmd", "Failed to create command") - assert.Contains(t, string(actual), "PASS") + assert.Contains(t, actual, "PASS") } else { assert.LessOrEqual(t, len(cmds), 1, "Created command that shouldn't have been created") - assert.Contains(t, string(actual), testCase.ExpectedErr) + assert.Contains(t, actual, testCase.ExpectedErr) } }) } @@ -159,11 +159,11 @@ func TestShowCommand(t *testing.T) { assert.Equal(t, len(commands), 1) output := th.CheckCommand(t, "command", "show", command.Id) - assert.Contains(t, string(output), command.Id) - assert.Contains(t, string(output), command.TeamId) - assert.Contains(t, string(output), trigger) - assert.Contains(t, string(output), displayName) - assert.Contains(t, string(output), user.Username) + assert.Contains(t, output, command.Id) + assert.Contains(t, output, command.TeamId) + assert.Contains(t, output, trigger) + assert.Contains(t, output, displayName) + assert.Contains(t, output, user.Username) }) t.Run("not existing command", func(t *testing.T) { @@ -253,14 +253,14 @@ func TestModifyCommand(t *testing.T) { t.Run("command not specified", func(t *testing.T) { args := []string{"command", "", command.Id, "--trigger-word", "sometrigger"} output, _ := th.RunCommandWithOutput(t, args...) - assert.Contains(t, string(output), "Error: unknown flag: --trigger-word") + assert.Contains(t, output, "Error: unknown flag: --trigger-word") }) t.Run("modify command unchanged", func(t *testing.T) { args := []string{"command", "modify", command.Id} output, _ := th.RunCommandWithOutput(t, args...) cmd, _ := th.App.GetCommand(command.Id) - assert.Contains(t, string(output), "PASS") + assert.Contains(t, output, "PASS") assert.Equal(t, cmd.DisplayName, command.DisplayName) assert.Equal(t, cmd.Method, command.Method) assert.Equal(t, cmd.TeamId, command.TeamId) @@ -277,7 +277,7 @@ func TestModifyCommand(t *testing.T) { t.Run("misspelled flag", func(t *testing.T) { args := []string{"command", "", command.Id, "--trigger-wor", "sometrigger"} output, _ := th.RunCommandWithOutput(t, args...) - assert.Contains(t, string(output), "Error: unknown flag:") + assert.Contains(t, output, "Error: unknown flag:") }) t.Run("multiple flags nil error", func(t *testing.T) { @@ -287,7 +287,7 @@ func TestModifyCommand(t *testing.T) { args := []string{"command", "modify", command.Id, "--trigger-word", testName, "--url", testURL, "--description", testDescription} output, _ := th.RunCommandWithOutput(t, args...) cmd, _ := th.App.GetCommand(command.Id) - assert.Contains(t, string(output), "PASS") + assert.Contains(t, output, "PASS") assert.Equal(t, cmd.Trigger, testName) assert.Equal(t, cmd.URL, testURL) assert.Equal(t, cmd.Description, testDescription) @@ -298,7 +298,7 @@ func TestModifyCommand(t *testing.T) { args := []string{"command", "modify", command.Id, "--title", testVal} output, _ := th.RunCommandWithOutput(t, args...) cmd, _ := th.App.GetCommand(command.Id) - assert.Contains(t, string(output), "PASS") + assert.Contains(t, output, "PASS") assert.Equal(t, cmd.DisplayName, testVal) }) @@ -307,7 +307,7 @@ func TestModifyCommand(t *testing.T) { args := []string{"command", "modify", command.Id, "--description", testVal} output, _ := th.RunCommandWithOutput(t, args...) cmd, _ := th.App.GetCommand(command.Id) - assert.Contains(t, string(output), "PASS") + assert.Contains(t, output, "PASS") assert.Equal(t, cmd.Description, testVal) }) @@ -316,7 +316,7 @@ func TestModifyCommand(t *testing.T) { args := []string{"command", "modify", command.Id, "--trigger-word", testVal} output, _ := th.RunCommandWithOutput(t, args...) cmd, _ := th.App.GetCommand(command.Id) - assert.Contains(t, string(output), "PASS") + assert.Contains(t, output, "PASS") assert.Equal(t, cmd.Trigger, testVal) }) @@ -324,14 +324,14 @@ func TestModifyCommand(t *testing.T) { testVal := "bad trigger" args := []string{"command", "modify", command.Id, "--trigger-word", testVal} output, _ := th.RunCommandWithOutput(t, args...) - assert.Contains(t, string(output), "Error: a trigger word must not contain spaces") + assert.Contains(t, output, "Error: a trigger word must not contain spaces") }) t.Run("trigger with leading /", func(t *testing.T) { testVal := "/bad-trigger" args := []string{"command", "modify", command.Id, "--trigger-word", testVal} output, _ := th.RunCommandWithOutput(t, args...) - assert.Contains(t, string(output), "Error: a trigger word cannot begin with a /") + assert.Contains(t, output, "Error: a trigger word cannot begin with a /") }) t.Run("blank trigger", func(t *testing.T) { @@ -341,7 +341,7 @@ func TestModifyCommand(t *testing.T) { cmd_modified, _ := th.App.GetCommand(command.Id) // assert trigger remains unchanged - assert.Contains(t, string(output), "PASS") + assert.Contains(t, output, "PASS") assert.Equal(t, cmd_unmodified.Trigger, cmd_modified.Trigger) }) @@ -351,7 +351,7 @@ func TestModifyCommand(t *testing.T) { args := []string{"command", "modify", command.Id, "--url", testVal} output, _ := th.RunCommandWithOutput(t, args...) cmd, _ := th.App.GetCommand(command.Id) - assert.Contains(t, string(output), "PASS") + assert.Contains(t, output, "PASS") assert.Equal(t, cmd.URL, testVal) }) @@ -362,7 +362,7 @@ func TestModifyCommand(t *testing.T) { cmd_modified, _ := th.App.GetCommand(command.Id) //assert URL remains unchanged - assert.Contains(t, string(output), "PASS") + assert.Contains(t, output, "PASS") assert.Equal(t, cmd_unmodified.URL, cmd_modified.URL) }) @@ -371,7 +371,7 @@ func TestModifyCommand(t *testing.T) { args := []string{"command", "modify", command.Id, "--icon", testVal} output, _ := th.RunCommandWithOutput(t, args...) cmd, _ := th.App.GetCommand(command.Id) - assert.Contains(t, string(output), "PASS") + assert.Contains(t, output, "PASS") assert.Equal(t, cmd.IconURL, testVal) }) @@ -380,7 +380,7 @@ func TestModifyCommand(t *testing.T) { args := []string{"command", "modify", command.Id, "--creator", testVal.Username} output, _ := th.RunCommandWithOutput(t, args...) cmd, _ := th.App.GetCommand(command.Id) - assert.Contains(t, string(output), "PASS") + assert.Contains(t, output, "PASS") assert.Equal(t, cmd.CreatorId, testVal.Id) }) @@ -388,14 +388,14 @@ func TestModifyCommand(t *testing.T) { testVal := "fakeuser" args := []string{"command", "modify", command.Id, "--creator", testVal} output, _ := th.RunCommandWithOutput(t, args...) - assert.Contains(t, string(output), "unable to find user") + assert.Contains(t, output, "unable to find user") }) t.Run("creator not admin user", func(t *testing.T) { testVal := user.Username args := []string{"command", "modify", command.Id, "--creator", testVal} output, _ := th.RunCommandWithOutput(t, args...) - assert.Contains(t, string(output), "the creator must be a user who has permissions to manage slash commands") + assert.Contains(t, output, "the creator must be a user who has permissions to manage slash commands") }) t.Run("response username nil error", func(t *testing.T) { @@ -403,7 +403,7 @@ func TestModifyCommand(t *testing.T) { args := []string{"command", "modify", command.Id, "--response-username", testVal} output, _ := th.RunCommandWithOutput(t, args...) cmd, _ := th.App.GetCommand(command.Id) - assert.Contains(t, string(output), "PASS") + assert.Contains(t, output, "PASS") assert.Equal(t, cmd.Username, testVal) }) @@ -414,13 +414,13 @@ func TestModifyCommand(t *testing.T) { // set post and check output_set, _ := th.RunCommandWithOutput(t, args_set...) cmd_set, _ := th.App.GetCommand(command.Id) - assert.Contains(t, string(output_set), "PASS") + assert.Contains(t, output_set, "PASS") assert.Equal(t, cmd_set.Method, "P") // unset post and check output_unset, _ := th.RunCommandWithOutput(t, args_unset...) cmd_unset, _ := th.App.GetCommand(command.Id) - assert.Contains(t, string(output_unset), "PASS") + assert.Contains(t, output_unset, "PASS") assert.Equal(t, cmd_unset.Method, "G") }) @@ -431,13 +431,13 @@ func TestModifyCommand(t *testing.T) { // set autocomplete and check output_set, _ := th.RunCommandWithOutput(t, args_set...) cmd_set, _ := th.App.GetCommand(command.Id) - assert.Contains(t, string(output_set), "PASS") + assert.Contains(t, output_set, "PASS") assert.Equal(t, cmd_set.AutoComplete, true) // unset autocomplete and check output_unset, _ := th.RunCommandWithOutput(t, args_unset...) cmd_unset, _ := th.App.GetCommand(command.Id) - assert.Contains(t, string(output_unset), "PASS") + assert.Contains(t, output_unset, "PASS") assert.Equal(t, cmd_unset.AutoComplete, false) }) } diff --git a/cmd/mattermost/commands/config_test.go b/cmd/mattermost/commands/config_test.go index 1bd6c63c0a..3268066084 100644 --- a/cmd/mattermost/commands/config_test.go +++ b/cmd/mattermost/commands/config_test.go @@ -111,10 +111,10 @@ func TestConfigGet(t *testing.T) { t.Run("check output", func(t *testing.T) { output := th.CheckCommand(t, "config", "get", "MessageExportSettings") - assert.Contains(t, string(output), "EnableExport") - assert.Contains(t, string(output), "ExportFormat") - assert.Contains(t, string(output), "DailyRunTime") - assert.Contains(t, string(output), "ExportFromTimestamp") + assert.Contains(t, output, "EnableExport") + assert.Contains(t, output, "ExportFormat") + assert.Contains(t, output, "DailyRunTime") + assert.Contains(t, output, "ExportFromTimestamp") }) } @@ -138,33 +138,33 @@ func TestConfigSet(t *testing.T) { t.Run("Error when the wrong value is set", func(t *testing.T) { assert.Error(t, th.RunCommand(t, "config", "set", "EmailSettings.ConnectionSecurity", "invalid-key")) output := th.CheckCommand(t, "config", "get", "EmailSettings.ConnectionSecurity") - assert.NotContains(t, string(output), "invalid-key") + assert.NotContains(t, output, "invalid-key") }) t.Run("Error when the parameter of an unknown plugin is set", func(t *testing.T) { output, err := th.RunCommandWithOutput(t, "config", "set", "PluginSettings.Plugins.someplugin", "true") assert.Error(t, err) - assert.NotContains(t, string(output), "panic") + assert.NotContains(t, output, "panic") }) t.Run("Error when the wrong locale is set", func(t *testing.T) { th.CheckCommand(t, "config", "set", "LocalizationSettings.DefaultServerLocale", "es") assert.Error(t, th.RunCommand(t, "config", "set", "LocalizationSettings.DefaultServerLocale", "invalid-key")) output := th.CheckCommand(t, "config", "get", "LocalizationSettings.DefaultServerLocale") - assert.NotContains(t, string(output), "invalid-key") - assert.NotContains(t, string(output), "\"en\"") + assert.NotContains(t, output, "invalid-key") + assert.NotContains(t, output, "\"en\"") }) t.Run("Success when a valid value is set", func(t *testing.T) { assert.NoError(t, th.RunCommand(t, "config", "set", "EmailSettings.ConnectionSecurity", "TLS")) output := th.CheckCommand(t, "config", "get", "EmailSettings.ConnectionSecurity") - assert.Contains(t, string(output), "TLS") + assert.Contains(t, output, "TLS") }) t.Run("Success when a valid locale is set", func(t *testing.T) { assert.NoError(t, th.RunCommand(t, "config", "set", "LocalizationSettings.DefaultServerLocale", "es")) output := th.CheckCommand(t, "config", "get", "LocalizationSettings.DefaultServerLocale") - assert.Contains(t, string(output), "\"es\"") + assert.Contains(t, output, "\"es\"") }) } @@ -339,9 +339,9 @@ func TestConfigShow(t *testing.T) { t.Run("successfully dumping config", func(t *testing.T) { output := th.CheckCommand(t, "config", "show") - assert.Contains(t, string(output), "SqlSettings") - assert.Contains(t, string(output), "MessageExportSettings") - assert.Contains(t, string(output), "AnnouncementSettings") + assert.Contains(t, output, "SqlSettings") + assert.Contains(t, output, "MessageExportSettings") + assert.Contains(t, output, "AnnouncementSettings") }) t.Run("successfully dumping config as json", func(t *testing.T) { diff --git a/cmd/mattermost/commands/group_test.go b/cmd/mattermost/commands/group_test.go index 7569de2a13..2f2bee18de 100644 --- a/cmd/mattermost/commands/group_test.go +++ b/cmd/mattermost/commands/group_test.go @@ -114,7 +114,7 @@ func TestChannelGroupStatus(t *testing.T) { // get status, should be Disabled output := th.CheckCommand(t, "group", "channel", "status", th.BasicTeam.Name+":"+channel.Name) - require.Contains(t, string(output), "Disabled") + require.Contains(t, output, "Disabled") // add group and enable id := model.NewId() @@ -143,7 +143,7 @@ func TestChannelGroupStatus(t *testing.T) { // get status, should be enabled output = th.CheckCommand(t, "group", "channel", "status", th.BasicTeam.Name+":"+channel.Name) - require.Contains(t, string(output), "Enabled") + require.Contains(t, output, "Enabled") // try to get status of nonexistent channel, should fail require.Error(t, th.RunCommand(t, "group", "channel", "status", th.BasicTeam.Name+":"+channel.Name+"asdf")) @@ -204,8 +204,8 @@ func TestChannelGroupList(t *testing.T) { // list groups output := th.CheckCommand(t, "group", "channel", "list", th.BasicTeam.Name+":"+channel.Name) - require.Contains(t, string(output), g1.DisplayName) - require.Contains(t, string(output), g2.DisplayName) + require.Contains(t, output, g1.DisplayName) + require.Contains(t, output, g2.DisplayName) // try to get list of nonexistent channel, should fail require.Error(t, th.RunCommand(t, "group", "channel", "list", th.BasicTeam.Name+":"+channel.Name+"asdf")) @@ -301,7 +301,7 @@ func TestTeamGroupStatus(t *testing.T) { // get status, should be Disabled output := th.CheckCommand(t, "group", "team", "status", th.BasicTeam.Name) - require.Contains(t, string(output), "Disabled") + require.Contains(t, output, "Disabled") // add group and enable id := model.NewId() @@ -330,7 +330,7 @@ func TestTeamGroupStatus(t *testing.T) { // get status, should be enabled output = th.CheckCommand(t, "group", "team", "status", th.BasicTeam.Name) - require.Contains(t, string(output), "Enabled") + require.Contains(t, output, "Enabled") // try to get status of nonexistent channel, should fail require.Error(t, th.RunCommand(t, "group", "team", "status", th.BasicTeam.Name+"asdf")) @@ -388,8 +388,8 @@ func TestTeamGroupList(t *testing.T) { // list groups output := th.CheckCommand(t, "group", "team", "list", th.BasicTeam.Name) - require.Contains(t, string(output), g1.DisplayName) - require.Contains(t, string(output), g2.DisplayName) + require.Contains(t, output, g1.DisplayName) + require.Contains(t, output, g2.DisplayName) // try to get list of nonexistent team, should fail require.Error(t, th.RunCommand(t, "group", "team", "list", th.BasicTeam.Name+"asdf")) diff --git a/mlog/stdlog.go b/mlog/stdlog.go index 7839ddfa05..1abf30b2ca 100644 --- a/mlog/stdlog.go +++ b/mlog/stdlog.go @@ -81,7 +81,7 @@ type loggerWriter struct { func (l *loggerWriter) Write(p []byte) (int, error) { trimmed := string(bytes.TrimSpace(p)) for _, line := range strings.Split(trimmed, "\n") { - l.logFunc(string(line)) + l.logFunc(line) } return len(p), nil } diff --git a/model/integration_action.go b/model/integration_action.go index f57611f6d6..cef7256abb 100644 --- a/model/integration_action.go +++ b/model/integration_action.go @@ -284,7 +284,7 @@ func DecodeAndVerifyTriggerId(triggerId string, s *ecdsa.PrivateKey) (string, st R, S *big.Int } - if _, err := asn1.Unmarshal([]byte(signature), &esig); err != nil { + if _, err := asn1.Unmarshal(signature, &esig); err != nil { return "", "", NewAppError("DecodeAndVerifyTriggerId", "interactive_message.decode_trigger_id.signature_decode_failed", nil, err.Error(), http.StatusBadRequest) } diff --git a/services/imageproxy/local.go b/services/imageproxy/local.go index 0ffc4dc025..ad282502a9 100644 --- a/services/imageproxy/local.go +++ b/services/imageproxy/local.go @@ -10,7 +10,6 @@ import ( "net/http" "net/http/httptest" "net/url" - "time" "github.com/mattermost/mattermost-server/mlog" "github.com/mattermost/mattermost-server/services/httpservice" @@ -58,7 +57,7 @@ func makeLocalBackend(proxy *ImageProxy) *LocalBackend { impl.DefaultBaseURL = baseURL } - impl.Timeout = time.Duration(httpservice.RequestTimeout) + impl.Timeout = httpservice.RequestTimeout impl.ContentTypes = imageContentTypes return &LocalBackend{ diff --git a/store/storetest/group_store.go b/store/storetest/group_store.go index 125173ef23..d91103cde0 100644 --- a/store/storetest/group_store.go +++ b/store/storetest/group_store.go @@ -1038,7 +1038,7 @@ func testDeleteGroupSyncable(t *testing.T, ss store.Store) { require.Equal(t, err.Id, "store.sql_group.no_rows") // Non-existent Team - _, err = ss.Group().DeleteGroupSyncable(groupTeam.GroupId, string(model.NewId()), model.GroupSyncableTypeTeam) + _, err = ss.Group().DeleteGroupSyncable(groupTeam.GroupId, model.NewId(), model.GroupSyncableTypeTeam) require.Equal(t, err.Id, "store.sql_group.no_rows") // Happy path... @@ -1795,10 +1795,10 @@ func testGetGroupsByChannel(t *testing.T, ss store.Store) { _, err = ss.User().Update(user2, true) require.Nil(t, err) - group1WithMemberCount := model.Group(*group1) + group1WithMemberCount := *group1 group1WithMemberCount.MemberCount = model.NewInt(1) - group2WithMemberCount := model.Group(*group2) + group2WithMemberCount := *group2 group2WithMemberCount.MemberCount = model.NewInt(0) testCases := []struct { @@ -2009,10 +2009,10 @@ func testGetGroupsByTeam(t *testing.T, ss store.Store) { _, err = ss.User().Update(user2, true) require.Nil(t, err) - group1WithMemberCount := model.Group(*group1) + group1WithMemberCount := *group1 group1WithMemberCount.MemberCount = model.NewInt(1) - group2WithMemberCount := model.Group(*group2) + group2WithMemberCount := *group2 group2WithMemberCount.MemberCount = model.NewInt(0) testCases := []struct { From e61340c54e81c7d9038a9a874b0ac6739ed0e54e Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Mon, 28 Oct 2019 14:08:08 +0100 Subject: [PATCH 34/45] Run gofmt -s against codebase (#12943) --- .golangci.yml | 2 + api4/integration_action_test.go | 2 +- api4/plugin_test.go | 8 +-- api4/websocket_test.go | 2 +- app/app_test.go | 44 +++++++------- app/command_test.go | 12 ++-- app/diagnostics_test.go | 6 +- app/email_batching_test.go | 10 ++-- app/import_validators_test.go | 11 ++-- app/integration_action_test.go | 2 +- app/plugin_hooks_test.go | 2 +- app/webhook_test.go | 12 ++-- cmd/mattermost/commands/user_test.go | 1 - config/database_test.go | 2 +- config/memory_test.go | 2 +- model/client4_test.go | 8 +-- model/command_response_test.go | 4 +- model/link_metadata_test.go | 4 +- model/manifest_test.go | 12 ++-- model/session_test.go | 4 +- model/user_test.go | 2 +- store/sqlstore/integrity_test.go | 86 ++++++++++++++-------------- utils/password_test.go | 44 +++++++------- 23 files changed, 141 insertions(+), 141 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 0f14da58b6..6788712517 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -5,6 +5,8 @@ run: linters-settings: govet: check-shadowing: true + gofmt: + simplify: true linters: disable-all: true diff --git a/api4/integration_action_test.go b/api4/integration_action_test.go index 3838d321fb..7b629f4a04 100644 --- a/api4/integration_action_test.go +++ b/api4/integration_action_test.go @@ -112,7 +112,7 @@ func TestOpenDialog(t *testing.T) { CallbackId: "callbackid", Title: "Some Title", Elements: []model.DialogElement{ - model.DialogElement{ + { DisplayName: "Element Name", Name: "element_name", Type: "text", diff --git a/api4/plugin_test.go b/api4/plugin_test.go index 210ec1b722..a1732b4db1 100644 --- a/api4/plugin_test.go +++ b/api4/plugin_test.go @@ -387,7 +387,7 @@ func TestDisableOnRemove(t *testing.T) { pluginsResp, resp := th.SystemAdminClient.GetPlugins() CheckNoError(t, resp) require.Len(t, pluginsResp.Active, 0) - require.Equal(t, pluginsResp.Inactive, []*model.PluginInfo{&model.PluginInfo{ + require.Equal(t, pluginsResp.Inactive, []*model.PluginInfo{{ Manifest: *manifest, }}) @@ -400,7 +400,7 @@ func TestDisableOnRemove(t *testing.T) { pluginsResp, resp = th.SystemAdminClient.GetPlugins() CheckNoError(t, resp) require.Len(t, pluginsResp.Inactive, 0) - require.Equal(t, pluginsResp.Active, []*model.PluginInfo{&model.PluginInfo{ + require.Equal(t, pluginsResp.Active, []*model.PluginInfo{{ Manifest: *manifest, }}) @@ -414,7 +414,7 @@ func TestDisableOnRemove(t *testing.T) { pluginsResp, resp = th.SystemAdminClient.GetPlugins() CheckNoError(t, resp) require.Len(t, pluginsResp.Inactive, 0) - require.Equal(t, pluginsResp.Active, []*model.PluginInfo{&model.PluginInfo{ + require.Equal(t, pluginsResp.Active, []*model.PluginInfo{{ Manifest: *manifest, }}) } @@ -439,7 +439,7 @@ func TestDisableOnRemove(t *testing.T) { pluginsResp, resp = th.SystemAdminClient.GetPlugins() CheckNoError(t, resp) require.Len(t, pluginsResp.Active, 0) - require.Equal(t, pluginsResp.Inactive, []*model.PluginInfo{&model.PluginInfo{ + require.Equal(t, pluginsResp.Inactive, []*model.PluginInfo{{ Manifest: *manifest, }}) diff --git a/api4/websocket_test.go b/api4/websocket_test.go index e4e1517000..504a7095a3 100644 --- a/api4/websocket_test.go +++ b/api4/websocket_test.go @@ -39,7 +39,7 @@ func TestWebSocket(t *testing.T) { WebSocketClient.SendMessage("ping", nil) time.Sleep(300 * time.Millisecond) resp = <-WebSocketClient.ResponseChannel - require.Equal(t, resp.Data["text"].(string),"pong","wrong response") + require.Equal(t, resp.Data["text"].(string), "pong", "wrong response") WebSocketClient.SendMessage("", nil) time.Sleep(300 * time.Millisecond) diff --git a/app/app_test.go b/app/app_test.go index 647d2dc4b3..2701829a34 100644 --- a/app/app_test.go +++ b/app/app_test.go @@ -69,7 +69,7 @@ func TestDoAdvancedPermissionsMigration(t *testing.T) { assert.Equal(t, len(roles1), len(roleNames)) expected1 := map[string][]string{ - "channel_user": []string{ + "channel_user": { model.PERMISSION_READ_CHANNEL.Id, model.PERMISSION_ADD_REACTION.Id, model.PERMISSION_REMOVE_REACTION.Id, @@ -86,10 +86,10 @@ func TestDoAdvancedPermissionsMigration(t *testing.T) { model.PERMISSION_DELETE_POST.Id, model.PERMISSION_EDIT_POST.Id, }, - "channel_admin": []string{ + "channel_admin": { model.PERMISSION_MANAGE_CHANNEL_ROLES.Id, }, - "team_user": []string{ + "team_user": { model.PERMISSION_LIST_TEAM_CHANNELS.Id, model.PERMISSION_JOIN_PUBLIC_CHANNELS.Id, model.PERMISSION_READ_PUBLIC_CHANNEL.Id, @@ -99,13 +99,13 @@ func TestDoAdvancedPermissionsMigration(t *testing.T) { model.PERMISSION_INVITE_USER.Id, model.PERMISSION_ADD_USER_TO_TEAM.Id, }, - "team_post_all": []string{ + "team_post_all": { model.PERMISSION_CREATE_POST.Id, }, - "team_post_all_public": []string{ + "team_post_all_public": { model.PERMISSION_CREATE_POST_PUBLIC.Id, }, - "team_admin": []string{ + "team_admin": { model.PERMISSION_REMOVE_USER_FROM_TEAM.Id, model.PERMISSION_MANAGE_TEAM.Id, model.PERMISSION_IMPORT_TEAM.Id, @@ -120,7 +120,7 @@ func TestDoAdvancedPermissionsMigration(t *testing.T) { model.PERMISSION_DELETE_POST.Id, model.PERMISSION_DELETE_OTHERS_POSTS.Id, }, - "system_user": []string{ + "system_user": { model.PERMISSION_LIST_PUBLIC_TEAMS.Id, model.PERMISSION_JOIN_PUBLIC_TEAMS.Id, model.PERMISSION_CREATE_DIRECT_CHANNEL.Id, @@ -128,18 +128,18 @@ func TestDoAdvancedPermissionsMigration(t *testing.T) { model.PERMISSION_VIEW_MEMBERS.Id, model.PERMISSION_CREATE_TEAM.Id, }, - "system_post_all": []string{ + "system_post_all": { model.PERMISSION_CREATE_POST.Id, }, - "system_post_all_public": []string{ + "system_post_all_public": { model.PERMISSION_CREATE_POST_PUBLIC.Id, }, - "system_user_access_token": []string{ + "system_user_access_token": { model.PERMISSION_CREATE_USER_ACCESS_TOKEN.Id, model.PERMISSION_READ_USER_ACCESS_TOKEN.Id, model.PERMISSION_REVOKE_USER_ACCESS_TOKEN.Id, }, - "system_admin": []string{ + "system_admin": { model.PERMISSION_ASSIGN_SYSTEM_ADMIN_ROLE.Id, model.PERMISSION_MANAGE_SYSTEM.Id, model.PERMISSION_MANAGE_ROLES.Id, @@ -254,7 +254,7 @@ func TestDoAdvancedPermissionsMigration(t *testing.T) { // Check the role permissions. expected2 := map[string][]string{ - "channel_user": []string{ + "channel_user": { model.PERMISSION_READ_CHANNEL.Id, model.PERMISSION_ADD_REACTION.Id, model.PERMISSION_REMOVE_REACTION.Id, @@ -269,10 +269,10 @@ func TestDoAdvancedPermissionsMigration(t *testing.T) { model.PERMISSION_DELETE_POST.Id, model.PERMISSION_EDIT_POST.Id, }, - "channel_admin": []string{ + "channel_admin": { model.PERMISSION_MANAGE_CHANNEL_ROLES.Id, }, - "team_user": []string{ + "team_user": { model.PERMISSION_LIST_TEAM_CHANNELS.Id, model.PERMISSION_JOIN_PUBLIC_CHANNELS.Id, model.PERMISSION_READ_PUBLIC_CHANNEL.Id, @@ -282,13 +282,13 @@ func TestDoAdvancedPermissionsMigration(t *testing.T) { model.PERMISSION_INVITE_USER.Id, model.PERMISSION_ADD_USER_TO_TEAM.Id, }, - "team_post_all": []string{ + "team_post_all": { model.PERMISSION_CREATE_POST.Id, }, - "team_post_all_public": []string{ + "team_post_all_public": { model.PERMISSION_CREATE_POST_PUBLIC.Id, }, - "team_admin": []string{ + "team_admin": { model.PERMISSION_REMOVE_USER_FROM_TEAM.Id, model.PERMISSION_MANAGE_TEAM.Id, model.PERMISSION_IMPORT_TEAM.Id, @@ -305,7 +305,7 @@ func TestDoAdvancedPermissionsMigration(t *testing.T) { model.PERMISSION_DELETE_POST.Id, model.PERMISSION_DELETE_OTHERS_POSTS.Id, }, - "system_user": []string{ + "system_user": { model.PERMISSION_LIST_PUBLIC_TEAMS.Id, model.PERMISSION_JOIN_PUBLIC_TEAMS.Id, model.PERMISSION_CREATE_DIRECT_CHANNEL.Id, @@ -313,18 +313,18 @@ func TestDoAdvancedPermissionsMigration(t *testing.T) { model.PERMISSION_VIEW_MEMBERS.Id, model.PERMISSION_CREATE_TEAM.Id, }, - "system_post_all": []string{ + "system_post_all": { model.PERMISSION_CREATE_POST.Id, }, - "system_post_all_public": []string{ + "system_post_all_public": { model.PERMISSION_CREATE_POST_PUBLIC.Id, }, - "system_user_access_token": []string{ + "system_user_access_token": { model.PERMISSION_CREATE_USER_ACCESS_TOKEN.Id, model.PERMISSION_READ_USER_ACCESS_TOKEN.Id, model.PERMISSION_REVOKE_USER_ACCESS_TOKEN.Id, }, - "system_admin": []string{ + "system_admin": { model.PERMISSION_ASSIGN_SYSTEM_ADMIN_ROLE.Id, model.PERMISSION_MANAGE_SYSTEM.Id, model.PERMISSION_MANAGE_ROLES.Id, diff --git a/app/command_test.go b/app/command_test.go index 5f34935298..03e0bda7f3 100644 --- a/app/command_test.go +++ b/app/command_test.go @@ -189,7 +189,7 @@ func TestHandleCommandResponsePost(t *testing.T) { // Test Slack attachments text conversion. resp.Attachments = []*model.SlackAttachment{ - &model.SlackAttachment{ + { Text: "", }, } @@ -212,7 +212,7 @@ func TestHandleCommandResponsePost(t *testing.T) { resp.ChannelId = "" resp.Text = "" resp.Attachments = []*model.SlackAttachment{ - &model.SlackAttachment{ + { Text: "", }, } @@ -257,10 +257,10 @@ func TestHandleCommandResponse(t *testing.T) { resp = &model.CommandResponse{ Text: "message 1", ExtraResponses: []*model.CommandResponse{ - &model.CommandResponse{ + { Text: "message 2", }, - &model.CommandResponse{ + { Type: model.POST_SYSTEM_GENERIC, Text: "message 3", }, @@ -274,8 +274,8 @@ func TestHandleCommandResponse(t *testing.T) { resp = &model.CommandResponse{ ExtraResponses: []*model.CommandResponse{ - &model.CommandResponse{}, - &model.CommandResponse{}, + {}, + {}, }, } diff --git a/app/diagnostics_test.go b/app/diagnostics_test.go index 8d9b51da1c..50d8b85a18 100644 --- a/app/diagnostics_test.go +++ b/app/diagnostics_test.go @@ -20,7 +20,7 @@ import ( func TestPluginSetting(t *testing.T) { settings := &model.PluginSettings{ Plugins: map[string]map[string]interface{}{ - "test": map[string]interface{}{ + "test": { "foo": "bar", }, }, @@ -31,10 +31,10 @@ func TestPluginSetting(t *testing.T) { func TestPluginActivated(t *testing.T) { states := map[string]*model.PluginState{ - "foo": &model.PluginState{ + "foo": { Enable: true, }, - "bar": &model.PluginState{ + "bar": { Enable: false, }, } diff --git a/app/email_batching_test.go b/app/email_batching_test.go index 09c2511ccf..aed286f0d9 100644 --- a/app/email_batching_test.go +++ b/app/email_batching_test.go @@ -64,11 +64,11 @@ func TestHandleNewNotifications(t *testing.T) { job.Add(&model.User{Id: id1}, &model.Post{UserId: id1, Message: "test4"}, &model.Team{Name: "team"}) job.Add(&model.User{Id: id2}, &model.Post{UserId: id1, Message: "test5"}, &model.Team{Name: "team"}) job.handleNewNotifications() - assert.Equal(t, job.pendingNotifications[id1][0].post.Message, "test1", "incorrect order of received posts for user1"); - assert.Equal(t, job.pendingNotifications[id1][1].post.Message, "test2", "incorrect order of received posts for user1"); - assert.Equal(t, job.pendingNotifications[id1][2].post.Message, "test4", "incorrect order of received posts for user1"); - assert.Equal(t, job.pendingNotifications[id2][0].post.Message, "test3", "incorrect order of received posts for user2"); - assert.Equal(t, job.pendingNotifications[id2][1].post.Message, "test5", "incorrect order of received posts for user2"); + assert.Equal(t, job.pendingNotifications[id1][0].post.Message, "test1", "incorrect order of received posts for user1") + assert.Equal(t, job.pendingNotifications[id1][1].post.Message, "test2", "incorrect order of received posts for user1") + assert.Equal(t, job.pendingNotifications[id1][2].post.Message, "test4", "incorrect order of received posts for user1") + assert.Equal(t, job.pendingNotifications[id2][0].post.Message, "test3", "incorrect order of received posts for user2") + assert.Equal(t, job.pendingNotifications[id2][1].post.Message, "test5", "incorrect order of received posts for user2") } func TestCheckPendingNotifications(t *testing.T) { diff --git a/app/import_validators_test.go b/app/import_validators_test.go index 48de801350..59d8188cc5 100644 --- a/app/import_validators_test.go +++ b/app/import_validators_test.go @@ -487,7 +487,7 @@ func TestImportValidateUserImportData(t *testing.T) { data.Username = ptrStr("i am a username with spaces and !!!") err = validateUserImportData(&data) - require.NotNil(t, err, "Validation should have failed due to invalid characters in Username."); + require.NotNil(t, err, "Validation should have failed due to invalid characters in Username.") data.Username = ptrStr("bob") @@ -759,7 +759,6 @@ func TestImportValidateReactionImportData(t *testing.T) { err = validateReactionImportData(&data, parentCreateAt) require.NotNil(t, err, "Should have failed due to missing required property.") - data = ReactionImportData{ User: ptrStr("username"), EmojiName: ptrStr("emoji"), @@ -938,13 +937,13 @@ func TestImportValidatePostImportData(t *testing.T) { require.NotNil(t, err, "Should have failed due to 0 create-at value.") // Test with valid all optional parameters. - reactions := []ReactionImportData{ReactionImportData{ + reactions := []ReactionImportData{{ User: ptrStr("username"), EmojiName: ptrStr("emoji"), CreateAt: ptrInt64(model.GetMillis()), }} - replies := []ReplyImportData{ReplyImportData{ + replies := []ReplyImportData{{ User: ptrStr("username"), Message: ptrStr("message"), CreateAt: ptrInt64(model.GetMillis()), @@ -1240,13 +1239,13 @@ func TestImportValidateDirectPostImportData(t *testing.T) { require.Nil(t, err, "Validation should succeed with post flagged by members") // Test with valid all optional parameters. - reactions := []ReactionImportData{ReactionImportData{ + reactions := []ReactionImportData{{ User: ptrStr("username"), EmojiName: ptrStr("emoji"), CreateAt: ptrInt64(model.GetMillis()), }} - replies := []ReplyImportData{ReplyImportData{ + replies := []ReplyImportData{{ User: ptrStr("username"), Message: ptrStr("message"), CreateAt: ptrInt64(model.GetMillis()), diff --git a/app/integration_action_test.go b/app/integration_action_test.go index d204efcd23..6474a16b29 100644 --- a/app/integration_action_test.go +++ b/app/integration_action_test.go @@ -430,7 +430,7 @@ func TestSubmitInteractiveDialog(t *testing.T) { assert.Equal(t, "value1", val) resp := model.SubmitDialogResponse{ - Error: "some generic error", + Error: "some generic error", Errors: map[string]string{"name1": "some error"}, } diff --git a/app/plugin_hooks_test.go b/app/plugin_hooks_test.go index df88b99b9b..2feabc26de 100644 --- a/app/plugin_hooks_test.go +++ b/app/plugin_hooks_test.go @@ -571,7 +571,7 @@ func TestHookFileWillBeUploaded(t *testing.T) { assert.Nil(t, err) assert.NotNil(t, response) assert.Equal(t, 1, len(response.FileInfos)) - + fileId := response.FileInfos[0].Id fileInfo, err := th.App.GetFileInfo(fileId) assert.Nil(t, err) diff --git a/app/webhook_test.go b/app/webhook_test.go index d56f45f14a..3822d16c79 100644 --- a/app/webhook_test.go +++ b/app/webhook_test.go @@ -389,13 +389,13 @@ func TestSplitWebhookPost(t *testing.T) { Message: strings.Repeat("本", maxPostSize*3/2), Props: map[string]interface{}{ "attachments": []*model.SlackAttachment{ - &model.SlackAttachment{ + { Text: strings.Repeat("本", 1000), }, - &model.SlackAttachment{ + { Text: strings.Repeat("本", 2000), }, - &model.SlackAttachment{ + { Text: strings.Repeat("本", model.POST_PROPS_MAX_USER_RUNES-1000), }, }, @@ -409,10 +409,10 @@ func TestSplitWebhookPost(t *testing.T) { Message: strings.Repeat("本", maxPostSize/2), Props: map[string]interface{}{ "attachments": []*model.SlackAttachment{ - &model.SlackAttachment{ + { Text: strings.Repeat("本", 1000), }, - &model.SlackAttachment{ + { Text: strings.Repeat("本", 2000), }, }, @@ -421,7 +421,7 @@ func TestSplitWebhookPost(t *testing.T) { { Props: map[string]interface{}{ "attachments": []*model.SlackAttachment{ - &model.SlackAttachment{ + { Text: strings.Repeat("本", model.POST_PROPS_MAX_USER_RUNES-1000), }, }, diff --git a/cmd/mattermost/commands/user_test.go b/cmd/mattermost/commands/user_test.go index 1c5e3d8acc..e7a5de50af 100644 --- a/cmd/mattermost/commands/user_test.go +++ b/cmd/mattermost/commands/user_test.go @@ -89,7 +89,6 @@ func TestChangeUserEmail(t *testing.T) { require.Equal(t, user.Email, newEmail, "should've updated to the new email") - // should fail because using an invalid email require.Error(t, th.RunCommand(t, "user", "email", th.BasicUser.Username, "wrong$email.com")) diff --git a/config/database_test.go b/config/database_test.go index ead6be3571..0ce33316aa 100644 --- a/config/database_test.go +++ b/config/database_test.go @@ -764,7 +764,7 @@ func TestDatabaseStoreLoad(t *testing.T) { func TestDatabaseGetFile(t *testing.T) { _, tearDown := setupConfigDatabase(t, minimalConfig, map[string][]byte{ - "empty-file": []byte{}, + "empty-file": {}, "test-file": []byte("test"), }) defer tearDown() diff --git a/config/memory_test.go b/config/memory_test.go index ede50fe549..bc1df36f97 100644 --- a/config/memory_test.go +++ b/config/memory_test.go @@ -282,7 +282,7 @@ func TestMemoryGetFile(t *testing.T) { ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{ InitialConfig: minimalConfig, InitialFiles: map[string][]byte{ - "empty-file": []byte{}, + "empty-file": {}, "test-file": []byte("test"), }, }) diff --git a/model/client4_test.go b/model/client4_test.go index b165e6b610..87f2c285da 100644 --- a/model/client4_test.go +++ b/model/client4_test.go @@ -17,9 +17,9 @@ func TestClient4CreatePost(t *testing.T) { post := &Post{ Props: map[string]interface{}{ "attachments": []*SlackAttachment{ - &SlackAttachment{ + { Actions: []*PostAction{ - &PostAction{ + { Integration: &PostActionIntegration{ Context: map[string]interface{}{ "foo": "bar", @@ -37,9 +37,9 @@ func TestClient4CreatePost(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { attachments := PostFromJson(r.Body).Attachments() assert.Equal(t, []*SlackAttachment{ - &SlackAttachment{ + { Actions: []*PostAction{ - &PostAction{ + { Integration: &PostActionIntegration{ Context: map[string]interface{}{ "foo": "bar", diff --git a/model/command_response_test.go b/model/command_response_test.go index 071cd4129a..76d46da0c3 100644 --- a/model/command_response_test.go +++ b/model/command_response_test.go @@ -146,7 +146,7 @@ func TestCommandResponseFromJson(t *testing.T) { &CommandResponse{ Text: "message 1", ExtraResponses: []*CommandResponse{ - &CommandResponse{ + { Text: "message 2", }, }, @@ -180,7 +180,7 @@ func TestCommandResponseFromJson(t *testing.T) { }, }, ExtraResponses: []*CommandResponse{ - &CommandResponse{ + { Text: "message 2", Attachments: []*SlackAttachment{ { diff --git a/model/link_metadata_test.go b/model/link_metadata_test.go index a3f63c506e..61c8acb09a 100644 --- a/model/link_metadata_test.go +++ b/model/link_metadata_test.go @@ -305,8 +305,8 @@ func TestTruncateOpenGraph(t *testing.T) { sampleImage("metoo.gif"), sampleImage("fifth.ico"), sampleImage("notme.tiff")}, - Audios: []*opengraph.Audio{&opengraph.Audio{}}, - Videos: []*opengraph.Video{&opengraph.Video{}}, + Audios: []*opengraph.Audio{{}}, + Videos: []*opengraph.Video{{}}, Article: &opengraph.Article{}, Book: &opengraph.Book{}, Profile: &opengraph.Profile{}, diff --git a/model/manifest_test.go b/model/manifest_test.go index 2a36fbe19c..f93b642acc 100644 --- a/model/manifest_test.go +++ b/model/manifest_test.go @@ -83,7 +83,7 @@ func TestManifestUnmarshal(t *testing.T) { Header: "theheadertext", Footer: "thefootertext", Settings: []*PluginSetting{ - &PluginSetting{ + { Key: "thesetting", DisplayName: "thedisplayname", Type: "dropdown", @@ -91,7 +91,7 @@ func TestManifestUnmarshal(t *testing.T) { RegenerateHelpText: "theregeneratehelptext", Placeholder: "theplaceholder", Options: []*PluginOption{ - &PluginOption{ + { DisplayName: "theoptiondisplayname", Value: "thevalue", }, @@ -232,7 +232,7 @@ func TestManifestJson(t *testing.T) { Header: "theheadertext", Footer: "thefootertext", Settings: []*PluginSetting{ - &PluginSetting{ + { Key: "thesetting", DisplayName: "thedisplayname", Type: "dropdown", @@ -240,7 +240,7 @@ func TestManifestJson(t *testing.T) { RegenerateHelpText: "theregeneratehelptext", Placeholder: "theplaceholder", Options: []*PluginOption{ - &PluginOption{ + { DisplayName: "theoptiondisplayname", Value: "thevalue", }, @@ -299,7 +299,7 @@ func TestManifestClientManifest(t *testing.T) { Header: "theheadertext", Footer: "thefootertext", Settings: []*PluginSetting{ - &PluginSetting{ + { Key: "thesetting", DisplayName: "thedisplayname", Type: "dropdown", @@ -307,7 +307,7 @@ func TestManifestClientManifest(t *testing.T) { RegenerateHelpText: "theregeneratehelptext", Placeholder: "theplaceholder", Options: []*PluginOption{ - &PluginOption{ + { DisplayName: "theoptiondisplayname", Value: "thevalue", }, diff --git a/model/session_test.go b/model/session_test.go index c232db8d66..0171f9e0a9 100644 --- a/model/session_test.go +++ b/model/session_test.go @@ -8,8 +8,8 @@ import ( "testing" "time" - "github.com/stretchr/testify/require" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestSessionDeepCopy(t *testing.T) { @@ -18,7 +18,7 @@ func TestSessionDeepCopy(t *testing.T) { mapKey := "key" mapValue := "val" - session := &Session{Id: sessionId, Props: map[string]string{mapKey: mapValue}, TeamMembers: []*TeamMember{&TeamMember{UserId: userId, TeamId: "someteamId"}}} + session := &Session{Id: sessionId, Props: map[string]string{mapKey: mapValue}, TeamMembers: []*TeamMember{{UserId: userId, TeamId: "someteamId"}}} copySession := session.DeepCopy() copySession.Id = "changed" diff --git a/model/user_test.go b/model/user_test.go index d3d64da78b..5fe39917c6 100644 --- a/model/user_test.go +++ b/model/user_test.go @@ -63,7 +63,7 @@ func TestUserPreSave(t *testing.T) { user.PreSave() user.Etag(true, true) assert.NotNil(t, user.Timezone, "Timezone is nil") - assert.Equal(t, user.Timezone["useAutomaticTimezone"], "true", "Timezone is not set to default"); + assert.Equal(t, user.Timezone["useAutomaticTimezone"], "true", "Timezone is not set to default") } func TestUserPreUpdate(t *testing.T) { diff --git a/store/sqlstore/integrity_test.go b/store/sqlstore/integrity_test.go index 02125a4cee..2f01ed3613 100644 --- a/store/sqlstore/integrity_test.go +++ b/store/sqlstore/integrity_test.go @@ -14,10 +14,10 @@ import ( func createAudit(ss store.Store, userId, sessionId string) *model.Audit { audit := model.Audit{ - UserId: userId, + UserId: userId, SessionId: sessionId, IpAddress: "ipaddress", - Action: "Action", + Action: "Action", } ss.Audit().Save(&audit) return &audit @@ -46,7 +46,7 @@ func createChannelWithSchemeId(ss store.Store, schemeId *string) *model.Channel return c } -func createCommand(ss store.Store, userId, teamId string) * model.Command { +func createCommand(ss store.Store, userId, teamId string) *model.Command { m := model.Command{} m.CreatorId = userId m.Method = model.COMMAND_METHOD_POST @@ -75,19 +75,19 @@ func createChannelMemberHistory(ss store.Store, channelId, userId string) *model } func createChannelWithTeamId(ss store.Store, id string) *model.Channel { - return createChannel(ss, id, model.NewId()); + return createChannel(ss, id, model.NewId()) } func createChannelWithCreatorId(ss store.Store, id string) *model.Channel { - return createChannel(ss, model.NewId(), id); + return createChannel(ss, model.NewId(), id) } func createChannelMemberWithChannelId(ss store.Store, id string) *model.ChannelMember { - return createChannelMember(ss, id, model.NewId()); + return createChannelMember(ss, id, model.NewId()) } func createChannelMemberWithUserId(ss store.Store, id string) *model.ChannelMember { - return createChannelMember(ss, model.NewId(), id); + return createChannelMember(ss, model.NewId(), id) } func createCommandWebhook(ss store.Store, commandId, userId, channelId string) *model.CommandWebhook { @@ -192,20 +192,20 @@ func createPost(ss store.Store, channelId, userId, rootId, parentId string) *mod } func createPostWithChannelId(ss store.Store, id string) *model.Post { - return createPost(ss, id, model.NewId(), "", ""); + return createPost(ss, id, model.NewId(), "", "") } func createPostWithUserId(ss store.Store, id string) *model.Post { - return createPost(ss, model.NewId(), id, "", ""); + return createPost(ss, model.NewId(), id, "", "") } func createPreferences(ss store.Store, userId string) *model.Preferences { preferences := model.Preferences{ { - UserId: userId, - Name: model.NewId(), + UserId: userId, + Name: model.NewId(), Category: model.PREFERENCE_CATEGORY_DIRECT_CHANNEL_SHOW, - Value: "somevalue", + Value: "somevalue", }, } ss.Preference().Save(&preferences) @@ -369,10 +369,10 @@ func TestCheckParentChildIntegrity(t *testing.T) { supplier := ss.(*store.LayeredStore).DatabaseLayer.(*SqlSupplier) t.Run("should receive an error", func(t *testing.T) { config := relationalCheckConfig{ - parentName: "NotValid", + parentName: "NotValid", parentIdAttr: "NotValid", - childName: "NotValid", - childIdAttr: "NotValid", + childName: "NotValid", + childIdAttr: "NotValid", } result := checkParentChildIntegrity(supplier, config) require.NotNil(t, result.Err) @@ -402,7 +402,7 @@ func TestCheckChannelsCommandWebhooksIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: channelId, - ChildId: cwh.Id, + ChildId: cwh.Id, }, data.Records[0]) dbmap.Delete(cwh) }) @@ -488,7 +488,7 @@ func TestCheckChannelsIncomingWebhooksIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: channelId, - ChildId: wh.Id, + ChildId: wh.Id, }, data.Records[0]) dbmap.Delete(wh) }) @@ -518,7 +518,7 @@ func TestCheckChannelsOutgoingWebhooksIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: channelId, - ChildId: wh.Id, + ChildId: wh.Id, }, data.Records[0]) dbmap.Delete(wh) }) @@ -545,7 +545,7 @@ func TestCheckChannelsPostsIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: post.ChannelId, - ChildId: post.Id, + ChildId: post.Id, }, data.Records[0]) dbmap.Delete(post) }) @@ -573,7 +573,7 @@ func TestCheckCommandsCommandWebhooksIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: commandId, - ChildId: cwh.Id, + ChildId: cwh.Id, }, data.Records[0]) dbmap.Delete(cwh) }) @@ -631,7 +631,7 @@ func TestCheckPostsPostsParentIdIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: parentId, - ChildId: post.Id, + ChildId: post.Id, }, data.Records[0]) dbmap.Delete(root) dbmap.Delete(post) @@ -662,7 +662,7 @@ func TestCheckPostsPostsRootIdIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: rootId, - ChildId: post.Id, + ChildId: post.Id, }, data.Records[0]) dbmap.Delete(post) }) @@ -720,7 +720,7 @@ func TestCheckSchemesChannelsIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: schemeId, - ChildId: channel.Id, + ChildId: channel.Id, }, data.Records[0]) dbmap.Delete(channel) }) @@ -751,7 +751,7 @@ func TestCheckSchemesTeamsIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: schemeId, - ChildId: team.Id, + ChildId: team.Id, }, data.Records[0]) dbmap.Delete(team) }) @@ -782,7 +782,7 @@ func TestCheckSessionsAuditsIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: sessionId, - ChildId: audit.Id, + ChildId: audit.Id, }, data.Records[0]) ss.Audit().PermanentDeleteByUser(userId) }) @@ -809,7 +809,7 @@ func TestCheckTeamsChannelsIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: channel.TeamId, - ChildId: channel.Id, + ChildId: channel.Id, }, data.Records[0]) dbmap.Delete(channel) }) @@ -837,7 +837,7 @@ func TestCheckTeamsCommandsIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: teamId, - ChildId: cmd.Id, + ChildId: cmd.Id, }, data.Records[0]) dbmap.Delete(cmd) }) @@ -865,7 +865,7 @@ func TestCheckTeamsIncomingWebhooksIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: teamId, - ChildId: wh.Id, + ChildId: wh.Id, }, data.Records[0]) dbmap.Delete(wh) }) @@ -893,7 +893,7 @@ func TestCheckTeamsOutgoingWebhooksIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: teamId, - ChildId: wh.Id, + ChildId: wh.Id, }, data.Records[0]) dbmap.Delete(wh) }) @@ -951,7 +951,7 @@ func TestCheckUsersAuditsIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: userId, - ChildId: audit.Id, + ChildId: audit.Id, }, data.Records[0]) ss.Audit().PermanentDeleteByUser(userId) }) @@ -979,7 +979,7 @@ func TestCheckUsersCommandWebhooksIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: userId, - ChildId: cwh.Id, + ChildId: cwh.Id, }, data.Records[0]) dbmap.Delete(cwh) }) @@ -1006,7 +1006,7 @@ func TestCheckUsersChannelsIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: channel.CreatorId, - ChildId: channel.Id, + ChildId: channel.Id, }, data.Records[0]) dbmap.Delete(channel) }) @@ -1094,7 +1094,7 @@ func TestCheckUsersCommandsIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: userId, - ChildId: cmd.Id, + ChildId: cmd.Id, }, data.Records[0]) dbmap.Delete(cmd) }) @@ -1124,7 +1124,7 @@ func TestCheckUsersCompliancesIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: userId, - ChildId: compliance.Id, + ChildId: compliance.Id, }, data.Records[0]) dbmap.Delete(compliance) }) @@ -1154,7 +1154,7 @@ func TestCheckUsersEmojiIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: userId, - ChildId: emoji.Id, + ChildId: emoji.Id, }, data.Records[0]) dbmap.Delete(emoji) }) @@ -1211,7 +1211,7 @@ func TestCheckUsersIncomingWebhooksIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: userId, - ChildId: wh.Id, + ChildId: wh.Id, }, data.Records[0]) dbmap.Delete(wh) }) @@ -1241,7 +1241,7 @@ func TestCheckUsersOAuthAccessDataIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: userId, - ChildId: ad.Token, + ChildId: ad.Token, }, data.Records[0]) ss.OAuth().RemoveAccessData(ad.Token) }) @@ -1271,7 +1271,7 @@ func TestCheckUsersOAuthAppsIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: userId, - ChildId: app.Id, + ChildId: app.Id, }, data.Records[0]) ss.OAuth().DeleteApp(app.Id) }) @@ -1301,7 +1301,7 @@ func TestCheckUsersOAuthAuthDataIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: userId, - ChildId: ad.Code, + ChildId: ad.Code, }, data.Records[0]) ss.OAuth().RemoveAuthData(ad.Code) }) @@ -1329,7 +1329,7 @@ func TestCheckUsersOutgoingWebhooksIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: userId, - ChildId: wh.Id, + ChildId: wh.Id, }, data.Records[0]) dbmap.Delete(wh) }) @@ -1356,7 +1356,7 @@ func TestCheckUsersPostsIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: post.UserId, - ChildId: post.Id, + ChildId: post.Id, }, data.Records[0]) dbmap.Delete(post) }) @@ -1442,7 +1442,7 @@ func TestCheckUsersSessionsIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: userId, - ChildId: session.Id, + ChildId: session.Id, }, data.Records[0]) dbmap.Delete(session) }) @@ -1531,7 +1531,7 @@ func TestCheckUsersUserAccessTokensIntegrity(t *testing.T) { require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ ParentId: userId, - ChildId: uat.Id, + ChildId: uat.Id, }, data.Records[0]) ss.UserAccessToken().Delete(uat.Id) }) diff --git a/utils/password_test.go b/utils/password_test.go index e0d120eed4..f5ef44f81d 100644 --- a/utils/password_test.go +++ b/utils/password_test.go @@ -19,10 +19,10 @@ func TestIsPasswordValidWithSettings(t *testing.T) { Password: strings.Repeat("x", 3), Settings: &model.PasswordSettings{ MinimumLength: model.NewInt(3), - Lowercase: model.NewBool(false), - Uppercase: model.NewBool(false), - Number: model.NewBool(false), - Symbol: model.NewBool(false), + Lowercase: model.NewBool(false), + Uppercase: model.NewBool(false), + Number: model.NewBool(false), + Symbol: model.NewBool(false), }, }, "Long": { @@ -30,28 +30,28 @@ func TestIsPasswordValidWithSettings(t *testing.T) { Settings: &model.PasswordSettings{ Lowercase: model.NewBool(false), Uppercase: model.NewBool(false), - Number: model.NewBool(false), - Symbol: model.NewBool(false), + Number: model.NewBool(false), + Symbol: model.NewBool(false), }, }, "TooShort": { Password: strings.Repeat("x", 2), Settings: &model.PasswordSettings{ MinimumLength: model.NewInt(3), - Lowercase: model.NewBool(false), - Uppercase: model.NewBool(false), - Number: model.NewBool(false), - Symbol: model.NewBool(false), + Lowercase: model.NewBool(false), + Uppercase: model.NewBool(false), + Number: model.NewBool(false), + Symbol: model.NewBool(false), }, ExpectedError: "model.user.is_valid.pwd.app_error", }, "TooLong": { - Password: strings.Repeat("x", model.PASSWORD_MAXIMUM_LENGTH+1), - Settings: &model.PasswordSettings{ + Password: strings.Repeat("x", model.PASSWORD_MAXIMUM_LENGTH+1), + Settings: &model.PasswordSettings{ Lowercase: model.NewBool(false), Uppercase: model.NewBool(false), - Number: model.NewBool(false), - Symbol: model.NewBool(false), + Number: model.NewBool(false), + Symbol: model.NewBool(false), }, ExpectedError: "model.user.is_valid.pwd.app_error", }, @@ -60,8 +60,8 @@ func TestIsPasswordValidWithSettings(t *testing.T) { Settings: &model.PasswordSettings{ Lowercase: model.NewBool(true), Uppercase: model.NewBool(false), - Number: model.NewBool(false), - Symbol: model.NewBool(false), + Number: model.NewBool(false), + Symbol: model.NewBool(false), }, ExpectedError: "model.user.is_valid.pwd_lowercase.app_error", }, @@ -70,28 +70,28 @@ func TestIsPasswordValidWithSettings(t *testing.T) { Settings: &model.PasswordSettings{ Uppercase: model.NewBool(true), Lowercase: model.NewBool(false), - Number: model.NewBool(false), - Symbol: model.NewBool(false), + Number: model.NewBool(false), + Symbol: model.NewBool(false), }, ExpectedError: "model.user.is_valid.pwd_uppercase.app_error", }, "MissingNumber": { Password: "asasdasdsadASD!@#", Settings: &model.PasswordSettings{ - Number: model.NewBool(true), + Number: model.NewBool(true), Lowercase: model.NewBool(false), Uppercase: model.NewBool(false), - Symbol: model.NewBool(false), + Symbol: model.NewBool(false), }, ExpectedError: "model.user.is_valid.pwd_number.app_error", }, "MissingSymbol": { Password: "asdasdasdasdasdASD123", Settings: &model.PasswordSettings{ - Symbol: model.NewBool(true), + Symbol: model.NewBool(true), Lowercase: model.NewBool(false), Uppercase: model.NewBool(false), - Number: model.NewBool(false), + Number: model.NewBool(false), }, ExpectedError: "model.user.is_valid.pwd_symbol.app_error", }, From 0736433d472ac18a26c1a844d00c8d438089d306 Mon Sep 17 00:00:00 2001 From: Eli Yukelzon Date: Mon, 28 Oct 2019 15:51:18 +0200 Subject: [PATCH 35/45] fix npm ci step (#12937) --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 229f271f01..fc344c5fce 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -30,7 +30,7 @@ jobs: if [ $CURL_FAILED -eq 1 ] then - npm ci && cd node_modules/mattermost-redux && npm install && npm run build && cd ../.. && make build + npm ci && make build fi - persist_to_workspace: root: /go/src/github.com/mattermost From d6f8ea293b05aee5fd0102419e6fed5b1029f590 Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Mon, 28 Oct 2019 16:41:41 +0100 Subject: [PATCH 36/45] Remove unused variables (#12926) --- api4/group.go | 5 ----- utils/imgutils/gif.go | 1 - utils/subpath_test.go | 2 -- 3 files changed, 8 deletions(-) diff --git a/api4/group.go b/api4/group.go index 24c7869551..4f799c3b58 100644 --- a/api4/group.go +++ b/api4/group.go @@ -13,11 +13,6 @@ import ( "github.com/mattermost/mattermost-server/model" ) -const ( - groupMemberActionCreate = iota - groupMemberActionDelete -) - func (api *API) InitGroup() { // GET /api/v4/groups api.BaseRoutes.Groups.Handle("", api.ApiSessionRequired(getGroups)).Methods("GET") diff --git a/utils/imgutils/gif.go b/utils/imgutils/gif.go index 0e48f25ece..48aa988b2f 100644 --- a/utils/imgutils/gif.go +++ b/utils/imgutils/gif.go @@ -32,7 +32,6 @@ type reader interface { const ( // Fields. fColorTable = 1 << 7 - fInterlace = 1 << 6 fColorTableBitsMask = 7 // Graphic control flags. diff --git a/utils/subpath_test.go b/utils/subpath_test.go index 0c76bd7368..ceb063fc40 100644 --- a/utils/subpath_test.go +++ b/utils/subpath_test.go @@ -281,8 +281,6 @@ const newSubpathRootHtml = ` Mattermost

Cannot connect to Mattermost

We're having trouble connecting to Mattermost. If refreshing this page (Ctrl+R or Command+R) does not work, please verify that your computer is connected to the internet.


` - const baseManifestJson = `{ "icons": [ { From ef5a5d574ff218e49fa251043428e4edfaea5ac5 Mon Sep 17 00:00:00 2001 From: 413x Date: Mon, 28 Oct 2019 17:44:20 +0200 Subject: [PATCH 37/45] GH-12630 Migrate tests from model/file_info_test.go to use testify (#12647) * Rewrite tests on testify for model/file_info_test.go --- model/file_info_test.go | 208 ++++++++++++++-------------------------- 1 file changed, 70 insertions(+), 138 deletions(-) diff --git a/model/file_info_test.go b/model/file_info_test.go index f87dd46404..bf2e449ac2 100644 --- a/model/file_info_test.go +++ b/model/file_info_test.go @@ -5,6 +5,8 @@ package model import ( "encoding/base64" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" _ "image/gif" _ "image/png" "io/ioutil" @@ -22,179 +24,109 @@ func TestFileInfoIsValid(t *testing.T) { Path: "fake/path.png", } - if err := info.IsValid(); err != nil { - t.Fatal(err) - } + require.Nil(t, info.IsValid()) info.Id = "" - if err := info.IsValid(); err == nil { - t.Fatal("empty Id isn't valid") - } + require.NotNil(t, info.IsValid(), "empty Id isn't valid") info.Id = NewId() info.CreateAt = 0 - if err := info.IsValid(); err == nil { - t.Fatal("empty CreateAt isn't valid") - } + require.NotNil(t, info.IsValid(), "empty CreateAt isn't valid") info.CreateAt = 1234 info.UpdateAt = 0 - if err := info.IsValid(); err == nil { - t.Fatal("empty UpdateAt isn't valid") - } + require.NotNil(t, info.IsValid(), "empty UpdateAt isn't valid") info.UpdateAt = 1234 info.PostId = NewId() - if err := info.IsValid(); err != nil { - t.Fatal(err) - } + require.Nil(t, info.IsValid()) info.Path = "" - if err := info.IsValid(); err == nil { - t.Fatal("empty Path isn't valid") - } + require.NotNil(t, info.IsValid(), "empty Path isn't valid") info.Path = "fake/path.png" - if err := info.IsValid(); err != nil { - t.Fatal(err) - } + require.Nil(t, info.IsValid()) } func TestFileInfoIsImage(t *testing.T) { - info := &FileInfo{ - MimeType: "image/png", - } - - if !info.IsImage() { - t.Fatal("file is an image") - } + info := &FileInfo{MimeType: "image/png"} + assert.True(t, info.IsImage(), "file is an image") info.MimeType = "text/plain" - if info.IsImage() { - t.Fatal("file is not an image") - } + assert.False(t, info.IsImage(), "file is not an image") } func TestGetInfoForFile(t *testing.T) { fakeFile := make([]byte, 1000) - if info, err := GetInfoForBytes("file.txt", fakeFile); err != nil { - t.Fatal(err) - } else if info.Name != "file.txt" { - t.Fatalf("Got incorrect filename: %v", info.Name) - } else if info.Extension != "txt" { - t.Fatalf("Got incorrect extension: %v", info.Extension) - } else if info.Size != 1000 { - t.Fatalf("Got incorrect size: %v", info.Size) - } else if !strings.HasPrefix(info.MimeType, "text/plain") { - t.Fatalf("Got incorrect mime type: %v", info.MimeType) - } else if info.Width != 0 { - t.Fatalf("Got incorrect width: %v", info.Width) - } else if info.Height != 0 { - t.Fatalf("Got incorrect height: %v", info.Height) - } else if info.HasPreviewImage { - t.Fatalf("Got incorrect has preview image: %v", info.HasPreviewImage) - } + info, errApp := GetInfoForBytes("file.txt", fakeFile) + require.Nil(t, errApp) + assert.Equalf(t, info.Name, "file.txt", "Got incorrect filename: %v", info.Name) + assert.Equalf(t, info.Extension, "txt", "Got incorrect extension: %v", info.Extension) + assert.EqualValuesf(t, info.Size, 1000, "Got incorrect size: %v", info.Size) + assert.Truef(t, strings.HasPrefix(info.MimeType, "text/plain"), "Got incorrect mime type: %v", info.MimeType) + assert.Equalf(t, info.Width, 0, "Got incorrect width: %v", info.Width) + assert.Equalf(t, info.Height, 0, "Got incorrect height: %v", info.Height) + assert.Falsef(t, info.HasPreviewImage, "Got incorrect has preview image: %v", info.HasPreviewImage) pngFile, err := ioutil.ReadFile("../tests/test.png") - if err != nil { - t.Fatalf("Failed to load test.png: %v", err.Error()) - } - if info, err := GetInfoForBytes("test.png", pngFile); err != nil { - t.Fatal(err) - } else if info.Name != "test.png" { - t.Fatalf("Got incorrect filename: %v", info.Name) - } else if info.Extension != "png" { - t.Fatalf("Got incorrect extension: %v", info.Extension) - } else if info.Size != 279591 { - t.Fatalf("Got incorrect size: %v", info.Size) - } else if info.MimeType != "image/png" { - t.Fatalf("Got incorrect mime type: %v", info.MimeType) - } else if info.Width != 408 { - t.Fatalf("Got incorrect width: %v", info.Width) - } else if info.Height != 336 { - t.Fatalf("Got incorrect height: %v", info.Height) - } else if !info.HasPreviewImage { - t.Fatalf("Got incorrect has preview image: %v", info.HasPreviewImage) - } + require.Nilf(t, err, "Failed to load test.png") + + info, err = GetInfoForBytes("test.png", pngFile) + require.Nil(t, err) + assert.Equalf(t, info.Name, "test.png", "Got incorrect filename: %v", info.Name) + assert.Equalf(t, info.Extension, "png", "Got incorrect extension: %v", info.Extension) + assert.EqualValues(t, info.Size, 279591, "Got incorrect size: %v", info.Size) + assert.Equalf(t, info.MimeType, "image/png", "Got incorrect mime type: %v", info.MimeType) + assert.Equalf(t, info.Width, 408, "Got incorrect width: %v", info.Width) + assert.Equalf(t, info.Height, 336, "Got incorrect height: %v", info.Height) + assert.Truef(t, info.HasPreviewImage, "Got incorrect has preview image: %v", info.HasPreviewImage) // base 64 encoded version of handtinywhite.gif from http://probablyprogramming.com/2009/03/15/the-tiniest-gif-ever gifFile, _ := base64.StdEncoding.DecodeString("R0lGODlhAQABAIABAP///wAAACwAAAAAAQABAAACAkQBADs=") - if info, err := GetInfoForBytes("handtinywhite.gif", gifFile); err != nil { - t.Fatal(err) - } else if info.Name != "handtinywhite.gif" { - t.Fatalf("Got incorrect filename: %v", info.Name) - } else if info.Extension != "gif" { - t.Fatalf("Got incorrect extension: %v", info.Extension) - } else if info.Size != 35 { - t.Fatalf("Got incorrect size: %v", info.Size) - } else if info.MimeType != "image/gif" { - t.Fatalf("Got incorrect mime type: %v", info.MimeType) - } else if info.Width != 1 { - t.Fatalf("Got incorrect width: %v", info.Width) - } else if info.Height != 1 { - t.Fatalf("Got incorrect height: %v", info.Height) - } else if !info.HasPreviewImage { - t.Fatalf("Got incorrect has preview image: %v", info.HasPreviewImage) - } + info, err = GetInfoForBytes("handtinywhite.gif", gifFile) + require.Nil(t, err) + assert.Equalf(t, info.Name, "handtinywhite.gif", "Got incorrect filename: %v", info.Name) + assert.Equalf(t, info.Extension, "gif", "Got incorrect extension: %v", info.Extension) + assert.EqualValuesf(t, info.Size, 35, "Got incorrect size: %v", info.Size) + assert.Equalf(t, info.MimeType, "image/gif", "Got incorrect mime type: %v", info.MimeType) + assert.Equalf(t, info.Width, 1, "Got incorrect width: %v", info.Width) + assert.Equalf(t, info.Height, 1, "Got incorrect height: %v", info.Height) + assert.Truef(t, info.HasPreviewImage, "Got incorrect has preview image: %v", info.HasPreviewImage) animatedGifFile, err := ioutil.ReadFile("../tests/testgif.gif") - if err != nil { - t.Fatalf("Failed to load testgif.gif: %v", err.Error()) - } - if info, err := GetInfoForBytes("testgif.gif", animatedGifFile); err != nil { - t.Fatal(err) - } else if info.Name != "testgif.gif" { - t.Fatalf("Got incorrect filename: %v", info.Name) - } else if info.Extension != "gif" { - t.Fatalf("Got incorrect extension: %v", info.Extension) - } else if info.Size != 38689 { - t.Fatalf("Got incorrect size: %v", info.Size) - } else if info.MimeType != "image/gif" { - t.Fatalf("Got incorrect mime type: %v", info.MimeType) - } else if info.Width != 118 { - t.Fatalf("Got incorrect width: %v", info.Width) - } else if info.Height != 118 { - t.Fatalf("Got incorrect height: %v", info.Height) - } else if info.HasPreviewImage { - t.Fatalf("Got incorrect has preview image: %v", info.HasPreviewImage) - } + require.Nilf(t, err, "Failed to load testgif.gif") - if info, err := GetInfoForBytes("filewithoutextension", fakeFile); err != nil { - t.Fatal(err) - } else if info.Name != "filewithoutextension" { - t.Fatalf("Got incorrect filename: %v", info.Name) - } else if info.Extension != "" { - t.Fatalf("Got incorrect extension: %v", info.Extension) - } else if info.Size != 1000 { - t.Fatalf("Got incorrect size: %v", info.Size) - } else if info.MimeType != "" { - t.Fatalf("Got incorrect mime type: %v", info.MimeType) - } else if info.Width != 0 { - t.Fatalf("Got incorrect width: %v", info.Width) - } else if info.Height != 0 { - t.Fatalf("Got incorrect height: %v", info.Height) - } else if info.HasPreviewImage { - t.Fatalf("Got incorrect has preview image: %v", info.HasPreviewImage) - } + info, err = GetInfoForBytes("testgif.gif", animatedGifFile) + require.Nil(t, err) + assert.Equalf(t, info.Name, "testgif.gif", "Got incorrect filename: %v", info.Name) + assert.Equalf(t, info.Extension, "gif", "Got incorrect extension: %v", info.Extension) + assert.EqualValuesf(t, info.Size, 38689, "Got incorrect size: %v", info.Size) + assert.Equalf(t, info.MimeType, "image/gif", "Got incorrect mime type: %v", info.MimeType) + assert.Equalf(t, info.Width, 118, "Got incorrect width: %v", info.Width) + assert.Equalf(t, info.Height, 118, "Got incorrect height: %v", info.Height) + assert.Falsef(t, info.HasPreviewImage, "Got incorrect has preview image: %v", info.HasPreviewImage) + + info, err = GetInfoForBytes("filewithoutextension", fakeFile) + require.Nil(t, err) + assert.Equalf(t, info.Name, "filewithoutextension", "Got incorrect filename: %v", info.Name) + assert.Equalf(t, info.Extension, "", "Got incorrect extension: %v", info.Extension) + assert.EqualValuesf(t, info.Size, 1000, "Got incorrect size: %v", info.Size) + assert.Equalf(t, info.MimeType, "", "Got incorrect mime type: %v", info.MimeType) + assert.Equalf(t, info.Width, 0, "Got incorrect width: %v", info.Width) + assert.Equalf(t, info.Height, 0, "Got incorrect height: %v", info.Height) + assert.Falsef(t, info.HasPreviewImage, "Got incorrect has preview image: %v", info.HasPreviewImage) // Always make the extension lower case to make it easier to use in other places - if info, err := GetInfoForBytes("file.TXT", fakeFile); err != nil { - t.Fatal(err) - } else if info.Name != "file.TXT" { - t.Fatalf("Got incorrect filename: %v", info.Name) - } else if info.Extension != "txt" { - t.Fatalf("Got incorrect extension: %v", info.Extension) - } + info, err = GetInfoForBytes("file.TXT", fakeFile) + require.Nil(t, err) + assert.Equalf(t, info.Name, "file.TXT", "Got incorrect filename: %v", info.Name) + assert.Equalf(t, info.Extension, "txt", "Got incorrect extension: %v", info.Extension) // Don't error out for image formats we don't support - if info, err := GetInfoForBytes("file.tif", fakeFile); err != nil { - t.Fatal(err) - } else if info.Name != "file.tif" { - t.Fatalf("Got incorrect filename: %v", info.Name) - } else if info.Extension != "tif" { - t.Fatalf("Got incorrect extension: %v", info.Extension) - } else if info.MimeType != "image/tiff" && info.MimeType != "image/x-tiff" { - t.Fatalf("Got incorrect mime type: %v", info.MimeType) - } + info, err = GetInfoForBytes("file.tif", fakeFile) + require.Nil(t, err) + assert.Equalf(t, info.Name, "file.tif", "Got incorrect filename: %v", info.Name) + assert.Equalf(t, info.Extension, "tif", "Got incorrect extension: %v", info.Extension) + assert.True(t, info.MimeType == "image/x-tiff" || info.MimeType == "image/tiff", "Got incorrect mime type: %v", info.MimeType) } From aaf0bf1cddc8b1e3866ef40c67946a0f64be1c85 Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Tue, 29 Oct 2019 00:52:51 +0900 Subject: [PATCH 38/45] command_help_test, cors_test: use testify (#12941) * command_help_test, cors_test: use testify * cors_test: remove extraneous lines * command_help_test: remove extraneous lines * command_help_test: set checks to nonterminal --- api4/command_help_test.go | 9 +++------ api4/cors_test.go | 10 +++------- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/api4/command_help_test.go b/api4/command_help_test.go index c5ff819b3f..87a223e8dd 100644 --- a/api4/command_help_test.go +++ b/api4/command_help_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/mattermost/mattermost-server/model" + "github.com/stretchr/testify/assert" ) func TestHelpCommand(t *testing.T) { @@ -23,15 +24,11 @@ func TestHelpCommand(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.SupportSettings.HelpLink = "" }) rs1, _ := Client.ExecuteCommand(channel.Id, "/help ") - if rs1.GotoLocation != model.SUPPORT_SETTINGS_DEFAULT_HELP_LINK { - t.Fatal("failed to default help link") - } + assert.Equal(t, rs1.GotoLocation, model.SUPPORT_SETTINGS_DEFAULT_HELP_LINK, "failed to default help link") th.App.UpdateConfig(func(cfg *model.Config) { *cfg.SupportSettings.HelpLink = "https://docs.mattermost.com/guides/user.html" }) rs2, _ := Client.ExecuteCommand(channel.Id, "/help ") - if rs2.GotoLocation != "https://docs.mattermost.com/guides/user.html" { - t.Fatal("failed to help link") - } + assert.Equal(t, rs2.GotoLocation, "https://docs.mattermost.com/guides/user.html", "failed to help link") } diff --git a/api4/cors_test.go b/api4/cors_test.go index 74702b284e..34dd0fd4b6 100644 --- a/api4/cors_test.go +++ b/api4/cors_test.go @@ -7,6 +7,7 @@ import ( "github.com/mattermost/mattermost-server/model" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) const ( @@ -127,16 +128,12 @@ func TestCORSRequestHandling(t *testing.T) { url := fmt.Sprintf("%v/api/v4/system/ping", host) req, err := http.NewRequest("GET", url, nil) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) testcase.ModifyRequest(req) client := &http.Client{} resp, err := client.Do(req) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) assert.Equal(t, http.StatusOK, resp.StatusCode) assert.Equal(t, testcase.ExpectedAllowOrigin, resp.Header.Get(acAllowOrigin)) assert.Equal(t, testcase.ExpectedExposeHeaders, resp.Header.Get(acExposeHeaders)) @@ -146,5 +143,4 @@ func TestCORSRequestHandling(t *testing.T) { assert.Equal(t, "", resp.Header.Get(acAllowHeaders)) }) } - } From 71d5f7dc64c48218de36033656448e8937ee4e95 Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Mon, 28 Oct 2019 18:04:50 +0100 Subject: [PATCH 39/45] Remove unused struct fields (#12924) --- app/notification_push.go | 1 - cmd/mattermost/commands/server_test.go | 1 - 2 files changed, 2 deletions(-) diff --git a/app/notification_push.go b/app/notification_push.go index eb26910774..df1b0384dd 100644 --- a/app/notification_push.go +++ b/app/notification_push.go @@ -29,7 +29,6 @@ type PushNotificationsHub struct { } type PushNotification struct { - id string notificationType NotificationType currentSessionId string userId string diff --git a/cmd/mattermost/commands/server_test.go b/cmd/mattermost/commands/server_test.go index c34a3358b8..dfcf78caf1 100644 --- a/cmd/mattermost/commands/server_test.go +++ b/cmd/mattermost/commands/server_test.go @@ -16,7 +16,6 @@ import ( ) type ServerTestHelper struct { - configStore config.Store disableConfigWatch bool interruptChan chan os.Signal originalInterval int From fdcda20fe462b3d6f726bfb83982f30e057d80e5 Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Mon, 28 Oct 2019 19:12:50 +0100 Subject: [PATCH 40/45] Cleanup dead code (#12929) * Cleanup dead code * Remove unneeded translation string --- api4/file.go | 86 -------------------------------- config/utils_test.go | 4 -- i18n/en.json | 4 -- store/sqlstore/integrity_test.go | 4 -- store/sqlstore/upgrade.go | 28 ----------- store/storetest/oauth_store.go | 2 +- store/storetest/webhook_store.go | 2 +- utils/test_files_compiler.go | 10 ---- 8 files changed, 2 insertions(+), 138 deletions(-) diff --git a/api4/file.go b/api4/file.go index 3426a04844..ef3f410d4b 100644 --- a/api4/file.go +++ b/api4/file.go @@ -65,92 +65,6 @@ func (api *API) InitFile() { } -func uploadFile(c *Context, w http.ResponseWriter, r *http.Request) { - defer io.Copy(ioutil.Discard, r.Body) - - if !*c.App.Config().FileSettings.EnableFileAttachments { - c.Err = model.NewAppError("uploadFile", "api.file.attachments.disabled.app_error", nil, "", http.StatusNotImplemented) - return - } - - if r.ContentLength > *c.App.Config().FileSettings.MaxFileSize { - c.Err = model.NewAppError("uploadFile", "api.file.upload_file.too_large.app_error", nil, "", http.StatusRequestEntityTooLarge) - return - } - - now := time.Now() - var resStruct *model.FileUploadResponse - var appErr *model.AppError - - if err := r.ParseMultipartForm(*c.App.Config().FileSettings.MaxFileSize); err != nil && err != http.ErrNotMultipart { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } else if err == http.ErrNotMultipart { - defer r.Body.Close() - - c.RequireChannelId() - c.RequireFilename() - - if c.Err != nil { - return - } - - channelId := c.Params.ChannelId - filename := c.Params.Filename - - if !c.App.SessionHasPermissionToChannel(c.App.Session, channelId, model.PERMISSION_UPLOAD_FILE) { - c.SetPermissionError(model.PERMISSION_UPLOAD_FILE) - return - } - - resStruct, appErr = c.App.UploadFiles( - FILE_TEAM_ID, - channelId, - c.App.Session.UserId, - []io.ReadCloser{r.Body}, - []string{filename}, - []string{}, - now, - ) - } else { - m := r.MultipartForm - - props := m.Value - if len(props["channel_id"]) == 0 { - c.SetInvalidParam("channel_id") - return - } - channelId := props["channel_id"][0] - c.Params.ChannelId = channelId - c.RequireChannelId() - if c.Err != nil { - return - } - - if !c.App.SessionHasPermissionToChannel(c.App.Session, channelId, model.PERMISSION_UPLOAD_FILE) { - c.SetPermissionError(model.PERMISSION_UPLOAD_FILE) - return - } - - resStruct, appErr = c.App.UploadMultipartFiles( - FILE_TEAM_ID, - channelId, - c.App.Session.UserId, - m.File["files"], - m.Value["client_ids"], - now, - ) - } - - if appErr != nil { - c.Err = appErr - return - } - - w.WriteHeader(http.StatusCreated) - w.Write([]byte(resStruct.ToJson())) -} - func parseMultipartRequestHeader(req *http.Request) (boundary string, err error) { v := req.Header.Get("Content-Type") if v == "" { diff --git a/config/utils_test.go b/config/utils_test.go index 8bfc2eb33b..131c843e7c 100644 --- a/config/utils_test.go +++ b/config/utils_test.go @@ -204,7 +204,3 @@ func sToP(s string) *string { func bToP(b bool) *bool { return &b } - -func iToP(i int) *int { - return &i -} diff --git a/i18n/en.json b/i18n/en.json index 6a89c055f4..4ecfb9dfe6 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1320,10 +1320,6 @@ "id": "api.file.upload_file.storage.app_error", "translation": "Unable to upload file. Image storage is not configured." }, - { - "id": "api.file.upload_file.too_large.app_error", - "translation": "Unable to upload file. File is too large." - }, { "id": "api.file.upload_file.too_large_detailed.app_error", "translation": "Unable to upload file {{.Filename}}. {{.Length}} bytes exceeds the maximum allowed {{.Limit}} bytes." diff --git a/store/sqlstore/integrity_test.go b/store/sqlstore/integrity_test.go index 2f01ed3613..d511f32f83 100644 --- a/store/sqlstore/integrity_test.go +++ b/store/sqlstore/integrity_test.go @@ -86,10 +86,6 @@ func createChannelMemberWithChannelId(ss store.Store, id string) *model.ChannelM return createChannelMember(ss, id, model.NewId()) } -func createChannelMemberWithUserId(ss store.Store, id string) *model.ChannelMember { - return createChannelMember(ss, model.NewId(), id) -} - func createCommandWebhook(ss store.Store, commandId, userId, channelId string) *model.CommandWebhook { m := model.CommandWebhook{} m.CommandId = commandId diff --git a/store/sqlstore/upgrade.go b/store/sqlstore/upgrade.go index 4b216a4a7a..fe29f711e7 100644 --- a/store/sqlstore/upgrade.go +++ b/store/sqlstore/upgrade.go @@ -4,7 +4,6 @@ package sqlstore import ( - "database/sql" "encoding/json" "os" "strings" @@ -602,33 +601,6 @@ func UpgradeDatabaseToVersion57(sqlStore SqlStore) { } } -func getRole(sqlStore SqlStore, name string) (*model.Role, error) { - var dbRole Role - - if err := sqlStore.GetReplica().SelectOne(&dbRole, "SELECT * from Roles WHERE Name = :Name", map[string]interface{}{"Name": name}); err != nil { - if err == sql.ErrNoRows { - return nil, errors.Wrapf(err, "failed to find role %s", name) - } else { - return nil, errors.Wrapf(err, "failed to query role %s", name) - } - } - - return dbRole.ToModel(), nil -} - -func saveRole(sqlStore SqlStore, role *model.Role) error { - dbRole := NewRoleFromModel(role) - - dbRole.UpdateAt = model.GetMillis() - if rowsChanged, err := sqlStore.GetMaster().Update(dbRole); err != nil { - return errors.Wrap(err, "failed to update role") - } else if rowsChanged != 1 { - return errors.New("found no role to update") - } - - return nil -} - func UpgradeDatabaseToVersion58(sqlStore SqlStore) { if shouldPerformUpgrade(sqlStore, VERSION_5_7_0, VERSION_5_8_0) { // idx_channels_txt was removed in `UpgradeDatabaseToVersion50`, but merged as part of diff --git a/store/storetest/oauth_store.go b/store/storetest/oauth_store.go index c754a2c277..6d3298c718 100644 --- a/store/storetest/oauth_store.go +++ b/store/storetest/oauth_store.go @@ -213,7 +213,7 @@ func testOAuthStoreRemoveAccessData(t *testing.T, ss store.Store) { require.Nil(t, result, "did not delete access token") } -func testOAuthStoreRemoveAllAccessData(t *testing.T, ss store.Store) { +func TestOAuthStoreRemoveAllAccessData(t *testing.T, ss store.Store) { a1 := model.AccessData{} a1.ClientId = model.NewId() a1.UserId = model.NewId() diff --git a/store/storetest/webhook_store.go b/store/storetest/webhook_store.go index d9aff7048b..810a37224b 100644 --- a/store/storetest/webhook_store.go +++ b/store/storetest/webhook_store.go @@ -228,7 +228,7 @@ func TestWebhookStoreGetIncomingByTeamByUser(t *testing.T, ss store.Store) { }) } -func testWebhookStoreGetIncomingByChannel(t *testing.T, ss store.Store) { +func TestWebhookStoreGetIncomingByChannel(t *testing.T, ss store.Store) { o1 := buildIncomingWebhook() o1, err := ss.Webhook().SaveIncoming(o1) diff --git a/utils/test_files_compiler.go b/utils/test_files_compiler.go index b58ad98bd2..6bf6ad05f0 100644 --- a/utils/test_files_compiler.go +++ b/utils/test_files_compiler.go @@ -10,21 +10,11 @@ import ( "os/exec" "path/filepath" "runtime" - "strings" "testing" "github.com/stretchr/testify/require" ) -func goMod(t *testing.T, dir string, args ...string) { - cmd := exec.Command("go", append([]string{"mod"}, args...)...) - cmd.Dir = dir - output, err := cmd.CombinedOutput() - if err != nil { - t.Fatalf("Failed to %s: %s", strings.Join(args, " "), string(output)) - } -} - func CompileGo(t *testing.T, sourceCode, outputPath string) { dir, err := ioutil.TempDir(".", "") require.NoError(t, err) From 7bcb835f374b3a9dd949abfe6adbf05d67270a1b Mon Sep 17 00:00:00 2001 From: Nikhil Ranjan Date: Mon, 28 Oct 2019 22:23:55 +0100 Subject: [PATCH 41/45] Convert store/storetest/preference_store.go t.Fatal calls into assert/require calls (#12800) * Convert store/storetest/preference_store.go t.Fatal calls into assert/require calls * fix test --- store/storetest/preference_store.go | 94 ++++++++++++----------------- 1 file changed, 39 insertions(+), 55 deletions(-) diff --git a/store/storetest/preference_store.go b/store/storetest/preference_store.go index 3456ce2f33..247f84cfb9 100644 --- a/store/storetest/preference_store.go +++ b/store/storetest/preference_store.go @@ -42,26 +42,22 @@ func testPreferenceSave(t *testing.T, ss store.Store) { Value: "value1b", }, } - if err := ss.Preference().Save(&preferences); err != nil { - t.Fatal("saving preference returned error") - } + err := ss.Preference().Save(&preferences) + require.Nil(t, err, "saving preference returned error") for _, preference := range preferences { - if data, _ := ss.Preference().Get(preference.UserId, preference.Category, preference.Name); preference.ToJson() != data.ToJson() { - t.Fatal("got incorrect preference after first Save") - } + data, _ := ss.Preference().Get(preference.UserId, preference.Category, preference.Name) + require.Equal(t, data.ToJson(), preference.ToJson(), "got incorrect preference after first Save") } preferences[0].Value = "value2a" preferences[1].Value = "value2b" - if err := ss.Preference().Save(&preferences); err != nil { - t.Fatal("saving preference returned error") - } + err = ss.Preference().Save(&preferences) + require.Nil(t, err, "saving preference returned error") for _, preference := range preferences { - if data, _ := ss.Preference().Get(preference.UserId, preference.Category, preference.Name); preference.ToJson() != data.ToJson() { - t.Fatal("got incorrect preference after second Save") - } + data, _ := ss.Preference().Get(preference.UserId, preference.Category, preference.Name) + require.Equal(t, data.ToJson(), preference.ToJson(), "got incorrect preference after second Save") } } @@ -96,16 +92,13 @@ func testPreferenceGet(t *testing.T, ss store.Store) { err := ss.Preference().Save(&preferences) require.Nil(t, err) - if data, err := ss.Preference().Get(userId, category, name); err != nil { - t.Fatal(err) - } else if data.ToJson() != preferences[0].ToJson() { - t.Fatal("got incorrect preference") - } + data, err := ss.Preference().Get(userId, category, name) + require.Nil(t, err) + require.Equal(t, preferences[0].ToJson(), data.ToJson(), "got incorrect preference") // make sure getting a missing preference fails - if _, err := ss.Preference().Get(model.NewId(), model.NewId(), model.NewId()); err == nil { - t.Fatal("no error on getting a missing preference") - } + _, err = ss.Preference().Get(model.NewId(), model.NewId(), model.NewId()) + require.NotNil(t, err, "no error on getting a missing preference") } func testPreferenceGetCategory(t *testing.T, ss store.Store) { @@ -142,20 +135,19 @@ func testPreferenceGetCategory(t *testing.T, ss store.Store) { err := ss.Preference().Save(&preferences) require.Nil(t, err) - if preferencesByCategory, err := ss.Preference().GetCategory(userId, category); err != nil { - t.Fatal(err) - } else if len(preferencesByCategory) != 2 { - t.Fatal("got the wrong number of preferences") - } else if !((preferencesByCategory[0] == preferences[0] && preferencesByCategory[1] == preferences[1]) || (preferencesByCategory[0] == preferences[1] && preferencesByCategory[1] == preferences[0])) { - t.Fatal("got incorrect preferences") - } + preferencesByCategory, err := ss.Preference().GetCategory(userId, category) + require.Nil(t, err) + require.Equal(t, 2, len(preferencesByCategory), "got the wrong number of preferences") + require.True( + t, + ((preferencesByCategory[0] == preferences[0] && preferencesByCategory[1] == preferences[1]) || (preferencesByCategory[0] == preferences[1] && preferencesByCategory[1] == preferences[0])), + "got incorrect preferences", + ) // make sure getting a missing preference category doesn't fail - if preferencesByCategory, err := ss.Preference().GetCategory(model.NewId(), model.NewId()); err != nil { - t.Fatal(err) - } else if len(preferencesByCategory) != 0 { - t.Fatal("shouldn't have got any preferences") - } + preferencesByCategory, err = ss.Preference().GetCategory(model.NewId(), model.NewId()) + require.Nil(t, err) + require.Equal(t, 0, len(preferencesByCategory), "shouldn't have got any preferences") } func testPreferenceGetAll(t *testing.T, ss store.Store) { @@ -192,17 +184,14 @@ func testPreferenceGetAll(t *testing.T, ss store.Store) { err := ss.Preference().Save(&preferences) require.Nil(t, err) - if result, err := ss.Preference().GetAll(userId); err != nil { - t.Fatal(err) - } else if len(result) != 3 { - t.Fatal("got the wrong number of preferences") - } else { - for i := 0; i < 3; i++ { - if result[0] != preferences[i] && result[1] != preferences[i] && result[2] != preferences[i] { - t.Fatal("got incorrect preferences") - } - } + result, err := ss.Preference().GetAll(userId) + require.Nil(t, err) + require.Equal(t, 3, len(result), "got the wrong number of preferences") + + for i := 0; i < 3; i++ { + assert.Falsef(t, result[0] != preferences[i] && result[1] != preferences[i] && result[2] != preferences[i], "got incorrect preferences") } + } func testPreferenceDeleteByUser(t *testing.T, ss store.Store) { @@ -239,9 +228,8 @@ func testPreferenceDeleteByUser(t *testing.T, ss store.Store) { err := ss.Preference().Save(&preferences) require.Nil(t, err) - if err := ss.Preference().PermanentDeleteByUser(userId); err != nil { - t.Fatal(err) - } + err = ss.Preference().PermanentDeleteByUser(userId) + require.Nil(t, err) } func testPreferenceDelete(t *testing.T, ss store.Store) { @@ -259,13 +247,11 @@ func testPreferenceDelete(t *testing.T, ss store.Store) { require.Nil(t, err) assert.Len(t, preferences, 1, "should've returned 1 preference") - if err = ss.Preference().Delete(preference.UserId, preference.Category, preference.Name); err != nil { - t.Fatal(err) - } + err = ss.Preference().Delete(preference.UserId, preference.Category, preference.Name) + require.Nil(t, err) preferences, err = ss.Preference().GetAll(preference.UserId) require.Nil(t, err) assert.Len(t, preferences, 0, "should've returned no preferences") - } func testPreferenceDeleteCategory(t *testing.T, ss store.Store) { @@ -293,9 +279,8 @@ func testPreferenceDeleteCategory(t *testing.T, ss store.Store) { require.Nil(t, err) assert.Len(t, preferences, 2, "should've returned 2 preferences") - if err = ss.Preference().DeleteCategory(userId, category); err != nil { - t.Fatal(err) - } + err = ss.Preference().DeleteCategory(userId, category) + require.Nil(t, err) preferences, err = ss.Preference().GetAll(userId) require.Nil(t, err) @@ -333,9 +318,8 @@ func testPreferenceDeleteCategoryAndName(t *testing.T, ss store.Store) { require.Nil(t, err) assert.Len(t, preferences, 1, "should've returned 1 preference") - if err = ss.Preference().DeleteCategoryAndName(category, name); err != nil { - t.Fatal(err) - } + err = ss.Preference().DeleteCategoryAndName(category, name) + require.Nil(t, err) preferences, err = ss.Preference().GetAll(userId) require.Nil(t, err) From d7649fbf31b4d757fbb5307bbe198acf7b444feb Mon Sep 17 00:00:00 2001 From: Nikhil Ranjan Date: Tue, 29 Oct 2019 01:02:28 +0100 Subject: [PATCH 42/45] Migrate tests from store/storetest/terms_of_service_store.go to use testify (#12874) --- store/storetest/terms_of_service_store.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/store/storetest/terms_of_service_store.go b/store/storetest/terms_of_service_store.go index 4e309bf213..af20c5327e 100644 --- a/store/storetest/terms_of_service_store.go +++ b/store/storetest/terms_of_service_store.go @@ -30,13 +30,9 @@ func testSaveTermsOfService(t *testing.T, ss store.Store) { savedTermsOfService, err := ss.TermsOfService().Save(termsOfService) require.Nil(t, err) - if len(savedTermsOfService.Id) != 26 { - t.Fatal("Id should have been populated") - } + require.Len(t, savedTermsOfService.Id, 26, "Id should have been populated") - if savedTermsOfService.CreateAt == 0 { - t.Fatal("Create at should have been populated") - } + require.NotEqual(t, savedTermsOfService.CreateAt, 0, "Create at should have been populated") } func testGetLatestTermsOfService(t *testing.T, ss store.Store) { From 7a665aacdd2f238b11cc0a8b7d12b0bf37dc606a Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Tue, 29 Oct 2019 07:45:09 +0100 Subject: [PATCH 43/45] Run gosimple against codebase (#12928) --- api4/file_test.go | 3 +-- api4/system.go | 2 -- api4/user_test.go | 5 +++-- app/notification_push.go | 4 ++-- app/post_metadata.go | 2 -- app/post_metadata_test.go | 12 ++++++++---- app/server.go | 2 +- app/web_conn.go | 12 ++---------- cmd/mattermost/commands/command.go | 4 ++-- mlog/human/parser.go | 4 ++-- model/config.go | 2 +- model/slack_attachment.go | 6 +----- plugin/helpers_kv_test.go | 10 +++++----- store/localcachelayer/role_layer.go | 7 +++---- store/sqlstore/user_store.go | 3 +-- testlib/resources.go | 2 +- 16 files changed, 33 insertions(+), 47 deletions(-) diff --git a/api4/file_test.go b/api4/file_test.go index ed4afeafc1..b451155715 100644 --- a/api4/file_test.go +++ b/api4/file_test.go @@ -607,8 +607,7 @@ func TestUploadFiles(t *testing.T) { expected, err := ioutil.ReadFile(filepath.Join(testDir, name)) require.Nil(t, err) - - if bytes.Compare(data, expected) != 0 { + if !bytes.Equal(data, expected) { tf, err := ioutil.TempFile("", fmt.Sprintf("test_%v_*_%s", i, name)) require.Nil(t, err) _, _ = io.Copy(tf, bytes.NewReader(data)) diff --git a/api4/system.go b/api4/system.go index 878d407d63..6e3a8866b5 100644 --- a/api4/system.go +++ b/api4/system.go @@ -397,7 +397,6 @@ func getRedirectLocation(c *Context, w http.ResponseWriter, r *http.Request) { m["location"] = location w.Write([]byte(model.MapToJson(m))) - return } func pushNotificationAck(c *Context, w http.ResponseWriter, r *http.Request) { @@ -415,5 +414,4 @@ func pushNotificationAck(c *Context, w http.ResponseWriter, r *http.Request) { } ReturnStatusOK(w) - return } diff --git a/api4/user_test.go b/api4/user_test.go index ea710dc8f3..452a045e21 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -1369,7 +1369,8 @@ func TestGetUsersByGroupChannelIds(t *testing.T) { usersByChannelId, resp := th.Client.GetUsersByGroupChannelIds([]string{gc1.Id}) CheckNoError(t, resp) - users, _ := usersByChannelId[gc1.Id] + users, ok := usersByChannelId[gc1.Id] + assert.True(t, ok) userIds := []string{} for _, user := range users { userIds = append(userIds, user.Id) @@ -1381,7 +1382,7 @@ func TestGetUsersByGroupChannelIds(t *testing.T) { usersByChannelId, resp = th.Client.GetUsersByGroupChannelIds([]string{gc1.Id}) CheckNoError(t, resp) - _, ok := usersByChannelId[gc1.Id] + _, ok = usersByChannelId[gc1.Id] require.False(t, ok) th.Client.Logout() diff --git a/app/notification_push.go b/app/notification_push.go index df1b0384dd..32596eb804 100644 --- a/app/notification_push.go +++ b/app/notification_push.go @@ -366,8 +366,8 @@ func ShouldSendPushNotification(user *model.User, channelNotifyProps model.Strin func DoesNotifyPropsAllowPushNotification(user *model.User, channelNotifyProps model.StringMap, post *model.Post, wasMentioned bool) bool { userNotifyProps := user.NotifyProps userNotify := userNotifyProps[model.PUSH_NOTIFY_PROP] - channelNotify, _ := channelNotifyProps[model.PUSH_NOTIFY_PROP] - if channelNotify == "" { + channelNotify, ok := channelNotifyProps[model.PUSH_NOTIFY_PROP] + if !ok || channelNotify == "" { channelNotify = model.CHANNEL_NOTIFY_DEFAULT } diff --git a/app/post_metadata.go b/app/post_metadata.go index 233bd4a841..b67f6f1ca4 100644 --- a/app/post_metadata.go +++ b/app/post_metadata.go @@ -73,8 +73,6 @@ func (a *App) OverrideIconURLIfEmoji(post *model.Post) { } else { mlog.Warn("Failed to retrieve URL for overriden profile icon (emoji)", mlog.String("emojiName", emojiName), mlog.Err(err)) } - - return } func (a *App) PreparePostForClient(originalPost *model.Post, isNewPost bool, isEditPost bool) *model.Post { diff --git a/app/post_metadata_test.go b/app/post_metadata_test.go index 367ec99372..fa3840dd73 100644 --- a/app/post_metadata_test.go +++ b/app/post_metadata_test.go @@ -307,18 +307,22 @@ func TestPreparePostForClient(t *testing.T) { t.Run("does not override icon URL", func(t *testing.T) { clientPost := prepare(false, url, emoji) - s, _ := clientPost.Props[model.POST_PROPS_OVERRIDE_ICON_URL] + s, ok := clientPost.Props[model.POST_PROPS_OVERRIDE_ICON_URL] + assert.True(t, ok) assert.EqualValues(t, url, s) - s, _ = clientPost.Props[model.POST_PROPS_OVERRIDE_ICON_EMOJI] + s, ok = clientPost.Props[model.POST_PROPS_OVERRIDE_ICON_EMOJI] + assert.True(t, ok) assert.EqualValues(t, emoji, s) }) t.Run("overrides icon URL", func(t *testing.T) { clientPost := prepare(true, url, emoji) - s, _ := clientPost.Props[model.POST_PROPS_OVERRIDE_ICON_URL] + s, ok := clientPost.Props[model.POST_PROPS_OVERRIDE_ICON_URL] + assert.True(t, ok) assert.EqualValues(t, overridenUrl, s) - s, _ = clientPost.Props[model.POST_PROPS_OVERRIDE_ICON_EMOJI] + s, ok = clientPost.Props[model.POST_PROPS_OVERRIDE_ICON_EMOJI] + assert.True(t, ok) assert.EqualValues(t, emoji, s) }) diff --git a/app/server.go b/app/server.go index 827367dc06..fb7ec94f25 100644 --- a/app/server.go +++ b/app/server.go @@ -633,7 +633,7 @@ func (a *App) OriginChecker() func(*http.Request) bool { func (s *Server) checkPushNotificationServerUrl() { notificationServer := *s.Config().EmailSettings.PushNotificationServer - if strings.HasPrefix(notificationServer, "http://") == true { + if strings.HasPrefix(notificationServer, "http://") { mlog.Warn("Your push notification server is configured with HTTP. For improved security, update to HTTPS in your configuration.") } } diff --git a/app/web_conn.go b/app/web_conn.go index bc014371ca..bb12e7074b 100644 --- a/app/web_conn.go +++ b/app/web_conn.go @@ -331,10 +331,7 @@ func (webCon *WebConn) ShouldSendEvent(msg *model.WebSocketEvent) bool { // If the event is destined to a specific user if len(msg.Broadcast.UserId) > 0 { - if webCon.UserId == msg.Broadcast.UserId { - return true - } - return false + return webCon.UserId == msg.Broadcast.UserId } // if the user is omitted don't send the message @@ -397,10 +394,5 @@ func (webCon *WebConn) IsMemberOfTeam(teamId string) bool { currentSession = session } - member := currentSession.GetTeamByTeamId(teamId) - - if member != nil { - return true - } - return false + return currentSession.GetTeamByTeamId(teamId) != nil } diff --git a/cmd/mattermost/commands/command.go b/cmd/mattermost/commands/command.go index c264a7e772..a7637f3ba1 100644 --- a/cmd/mattermost/commands/command.go +++ b/cmd/mattermost/commands/command.go @@ -153,7 +153,7 @@ func createCommandCmdF(command *cobra.Command, args []string) error { autocompleteHint, _ := command.Flags().GetString("autocompleteHint") post, errp := command.Flags().GetBool("post") method := "P" - if errp != nil || post == false { + if errp != nil || !post { method = "G" } @@ -371,7 +371,7 @@ func modifyCommandCmdF(command *cobra.Command, args []string) error { post, err := command.Flags().GetBool("post") method := "P" - if err != nil || post == false { + if err != nil || !post { method = "G" } modifiedCommand.Method = method diff --git a/mlog/human/parser.go b/mlog/human/parser.go index f279ecf672..1aa738aeaa 100644 --- a/mlog/human/parser.go +++ b/mlog/human/parser.go @@ -41,7 +41,7 @@ func parseLogMessage(msg string) (result LogEntry, err error) { } else { d, ok := token.(json.Delim) if !ok || d != '{' { - return result, errors.New(fmt.Sprintf("input is not a JSON object, found: %v", token)) + return result, fmt.Errorf("input is not a JSON object, found: %v", token) } } @@ -110,7 +110,7 @@ func parseLogMessage(msg string) (result LogEntry, err error) { } else { d, ok := token.(json.Delim) if !ok || d != '}' { - return result, errors.New(fmt.Sprintf("failed to read '}', read: %v", token)) + return result, fmt.Errorf("failed to read '}', read: %v", token) } } diff --git a/model/config.go b/model/config.go index 12079f4126..8d61e1ec8f 100644 --- a/model/config.go +++ b/model/config.go @@ -2880,7 +2880,7 @@ func (ss *ServiceSettings) isValid() *AppError { return NewAppError("Config.IsValid", "model.config.is_valid.webserver_security.app_error", nil, "", http.StatusBadRequest) } - if *ss.ConnectionSecurity == CONN_SECURITY_TLS && *ss.UseLetsEncrypt == false { + if *ss.ConnectionSecurity == CONN_SECURITY_TLS && !*ss.UseLetsEncrypt { appErr := NewAppError("Config.IsValid", "model.config.is_valid.tls_cert_file.app_error", nil, "", http.StatusBadRequest) if *ss.TLSCertFile == "" { diff --git a/model/slack_attachment.go b/model/slack_attachment.go index 0f1b96fb50..30757061e6 100644 --- a/model/slack_attachment.go +++ b/model/slack_attachment.go @@ -111,11 +111,7 @@ func (s *SlackAttachment) Equals(input *SlackAttachment) bool { } } - if s.Timestamp != input.Timestamp { - return false - } - - return true + return s.Timestamp == input.Timestamp } type SlackAttachmentField struct { diff --git a/plugin/helpers_kv_test.go b/plugin/helpers_kv_test.go index f61033dacb..3c63137401 100644 --- a/plugin/helpers_kv_test.go +++ b/plugin/helpers_kv_test.go @@ -84,7 +84,7 @@ func TestKVSetJSON(t *testing.T) { p := &plugin.HelpersImpl{API: api} - err := p.KVSetJSON("test-key", func() { return }) + err := p.KVSetJSON("test-key", func() {}) api.AssertExpectations(t) assert.Error(t, err) }) @@ -124,7 +124,7 @@ func TestKVCompareAndSetJSON(t *testing.T) { api.AssertNotCalled(t, "KVCompareAndSet") p := &plugin.HelpersImpl{API: api} - ok, err := p.KVCompareAndSetJSON("test-key", func() { return }, map[string]interface{}{}) + ok, err := p.KVCompareAndSetJSON("test-key", func() {}, map[string]interface{}{}) api.AssertExpectations(t) assert.Equal(t, false, ok) @@ -137,7 +137,7 @@ func TestKVCompareAndSetJSON(t *testing.T) { p := &plugin.HelpersImpl{API: api} - ok, err := p.KVCompareAndSetJSON("test-key", map[string]interface{}{}, func() { return }) + ok, err := p.KVCompareAndSetJSON("test-key", map[string]interface{}{}, func() {}) api.AssertExpectations(t) assert.False(t, ok) @@ -211,7 +211,7 @@ func TestKVCompareAndDeleteJSON(t *testing.T) { api.AssertNotCalled(t, "KVCompareAndDelete") p := &plugin.HelpersImpl{API: api} - ok, err := p.KVCompareAndDeleteJSON("test-key", func() { return }) + ok, err := p.KVCompareAndDeleteJSON("test-key", func() {}) api.AssertExpectations(t) assert.Equal(t, false, ok) @@ -266,7 +266,7 @@ func TestKVSetWithExpiryJSON(t *testing.T) { p := &plugin.HelpersImpl{API: api} - err := p.KVSetWithExpiryJSON("test-key", func() { return }, 100) + err := p.KVSetWithExpiryJSON("test-key", func() {}, 100) api.AssertExpectations(t) assert.Error(t, err) diff --git a/store/localcachelayer/role_layer.go b/store/localcachelayer/role_layer.go index 0cd03c1427..33aaa2a7c6 100644 --- a/store/localcachelayer/role_layer.go +++ b/store/localcachelayer/role_layer.go @@ -55,11 +55,10 @@ func (s LocalCacheRoleStore) GetByNames(names []string) ([]*model.Role, *model.A roles, _ := s.RoleStore.GetByNames(rolesToQuery) - if roles != nil { - for _, role := range roles { - s.rootStore.doStandardAddToCache(s.rootStore.roleCache, role.Name, role) - } + for _, role := range roles { + s.rootStore.doStandardAddToCache(s.rootStore.roleCache, role.Name, role) } + return append(foundRoles, roles...), nil } diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index 5febcc88e0..9a20318009 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -1359,8 +1359,7 @@ func (us SqlUserStore) GetProfilesNotInTeam(teamId string, groupConstrained bool } func (us SqlUserStore) GetEtagForProfilesNotInTeam(teamId string) string { - var querystr string - querystr = ` + querystr := ` SELECT CONCAT(MAX(UpdateAt), '.', COUNT(Id)) as etag FROM diff --git a/testlib/resources.go b/testlib/resources.go index 8f058df58d..698e20197f 100644 --- a/testlib/resources.go +++ b/testlib/resources.go @@ -84,7 +84,7 @@ func getTestResourcesToSetup() []testResourceDetails { testResourcesToSetup[i].src = srcPath } else if testResource.resType == resourceTypeFolder { srcPath, found = findDir(testResource.src) - if found == false { + if !found { panic(fmt.Sprintf("Failed to find folder %s", testResource.src)) } From 4cc29734837f8b448a90da6d55a96d637f0171bc Mon Sep 17 00:00:00 2001 From: Ali Farooq Date: Tue, 29 Oct 2019 06:44:32 -0400 Subject: [PATCH 44/45] MM-18827 - Can not use api/v4/websocket with trailing slash (#12931) --- api4/websocket.go | 3 ++- api4/websocket_test.go | 9 +++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/api4/websocket.go b/api4/websocket.go index 164ae83b79..85eaf87d39 100644 --- a/api4/websocket.go +++ b/api4/websocket.go @@ -12,7 +12,8 @@ import ( ) func (api *API) InitWebSocket() { - api.BaseRoutes.ApiRoot.Handle("/websocket", api.ApiHandlerTrustRequester(connectWebSocket)).Methods("GET") + // Optionally supports a trailing slash + api.BaseRoutes.ApiRoot.Handle("/{websocket:websocket(?:\\/)?}", api.ApiHandlerTrustRequester(connectWebSocket)).Methods("GET") } func connectWebSocket(c *Context, w http.ResponseWriter, r *http.Request) { diff --git a/api4/websocket_test.go b/api4/websocket_test.go index 504a7095a3..8450684f7a 100644 --- a/api4/websocket_test.go +++ b/api4/websocket_test.go @@ -66,6 +66,15 @@ func TestWebSocket(t *testing.T) { require.Equal(t, resp.Error.DetailedError, "", "detailed error not cleared") } +func TestWebSocketTrailingSlash(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + url := fmt.Sprintf("ws://localhost:%v", th.App.Srv.ListenAddr.Port) + _, _, err := websocket.DefaultDialer.Dial(url+model.API_URL_SUFFIX+"/websocket/", nil) + require.NoError(t, err) +} + func TestWebSocketEvent(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() From 38c0bde7f8fb39ea285f7dde4d28ebec76a5da5b Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Tue, 29 Oct 2019 15:04:28 +0100 Subject: [PATCH 45/45] Run ineffassign against codebase (#12925) --- api4/channel_test.go | 1 + app/migrations.go | 6 +++--- model/config_test.go | 1 - store/sqlstore/post_store.go | 3 +-- store/sqlstore/supplier.go | 2 +- store/sqlstore/user_store.go | 2 +- utils/merge_test.go | 3 ++- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/api4/channel_test.go b/api4/channel_test.go index 77ebcf93d1..3b634cd84a 100644 --- a/api4/channel_test.go +++ b/api4/channel_test.go @@ -2507,6 +2507,7 @@ func TestRemoveChannelMember(t *testing.T) { // Setup the system administrator to listen for websocket events from the channels. th.LinkUserToTeam(th.SystemAdminUser, th.BasicTeam) _, err := th.App.AddUserToChannel(th.SystemAdminUser, th.BasicChannel) + require.Nil(t, err) _, err = th.App.AddUserToChannel(th.SystemAdminUser, th.BasicChannel2) require.Nil(t, err) props := map[string]string{} diff --git a/app/migrations.go b/app/migrations.go index 1503e44604..8c4b5fe0c5 100644 --- a/app/migrations.go +++ b/app/migrations.go @@ -95,9 +95,9 @@ func (a *App) DoEmojisPermissionsMigration() { return } - var role *model.Role = nil - var systemAdminRole *model.Role = nil - var err *model.AppError = nil + var role *model.Role + var systemAdminRole *model.Role + var err *model.AppError mlog.Info("Migrating emojis config to database.") switch *a.Config().ServiceSettings.DEPRECATED_DO_NOT_USE_RestrictCustomEmojiCreation { diff --git a/model/config_test.go b/model/config_test.go index dde02b517b..bc88b18937 100644 --- a/model/config_test.go +++ b/model/config_test.go @@ -186,7 +186,6 @@ func TestConfigIsValidFakeAlgorithm(t *testing.T) { require.Equal(t, "model.config.is_valid.saml_digest_algorithm.app_error", err.Message) *c1.SamlSettings.DigestAlgorithm = temp - temp = *c1.SamlSettings.SignatureAlgorithm *c1.SamlSettings.SignatureAlgorithm = "Fake Algorithm" err = c1.SamlSettings.isValid() if err == nil { diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index a58d63bcc0..3560fd61c7 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -1346,7 +1346,6 @@ func (s *SqlPostStore) GetOldest() (*model.Post, *model.AppError) { } func (s *SqlPostStore) determineMaxPostSize() int { - var maxPostSize int = model.POST_MESSAGE_MAX_RUNES_V1 var maxPostSizeBytes int32 if s.DriverName() == model.DATABASE_DRIVER_POSTGRES { @@ -1384,7 +1383,7 @@ func (s *SqlPostStore) determineMaxPostSize() int { } // Assume a worst-case representation of four bytes per rune. - maxPostSize = int(maxPostSizeBytes) / 4 + maxPostSize := int(maxPostSizeBytes) / 4 // To maintain backwards compatibility, don't yield a maximum post // size smaller than the previous limit, even though it wasn't diff --git a/store/sqlstore/supplier.go b/store/sqlstore/supplier.go index 9d04cedf7b..3db37c90d3 100644 --- a/store/sqlstore/supplier.go +++ b/store/sqlstore/supplier.go @@ -699,7 +699,7 @@ func (ss *SqlSupplier) AlterColumnDefaultIfExists(tableName string, columnName s return false } - var defaultValue = "" + var defaultValue string if ss.DriverName() == model.DATABASE_DRIVER_MYSQL { // Some column types in MySQL cannot have defaults, so don't try to configure anything. if mySqlColDefault == nil { diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index 9a20318009..5f15cf23b1 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -1267,7 +1267,7 @@ func generateSearchQuery(query sq.SelectBuilder, terms []string, fields []string func (us SqlUserStore) performSearch(query sq.SelectBuilder, term string, options *model.UserSearchOptions) ([]*model.User, *model.AppError) { term = sanitizeSearchTerm(term, "*") - searchType := USER_SEARCH_TYPE_NAMES_NO_FULL_NAME + var searchType []string if options.AllowEmails { if options.AllowFullNames { searchType = USER_SEARCH_TYPE_ALL diff --git a/utils/merge_test.go b/utils/merge_test.go index a002a33797..a2b5af7e57 100644 --- a/utils/merge_test.go +++ b/utils/merge_test.go @@ -513,7 +513,8 @@ func TestMergeWithSlices(t *testing.T) { assert.Equal(t, expected, merged) // of course this won't change merged, even if it did copy... but just in case. m2 = append(m2, "test") - assert.Equal(t, 0, len(merged)) + assert.Len(t, m2, 1) + assert.Len(t, merged, 0) }) t.Run("slice is not copied. change in patch will not affect merged", func(t *testing.T) {