From 6de33799941edcb8ef4506d4e0fe0ee6ba4a431a Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Wed, 21 May 2025 16:35:00 +0200 Subject: [PATCH] [MM-61099] Fix errcheck issues in server/channels/app/brand.go (#30679) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [MM-28779] Fix errcheck issues in server/channels/app/brand.go Remove brand.go from the errcheck exclusion list in .golangci.yml and fixed the error by properly handling the return value from a.MoveFile(). 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude * [MM-28779] Add test to verify brand image backup functionality Add a new test that verifies backup of the original brand image happens when a new one is uploaded. This helps to ensure the fix for errcheck issues is working as expected. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude * use seperate temporary filestore for each test * Use FileSettings.Directory instead of finding the dir programatically * Fix another test * Fix defer * Update server/channels/api4/job_test.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Fix bad bot commit * Cleanup logs message * cleanup file path * Fix error variable names * WIP:cleanup panic ussage * Revert "WIP:cleanup panic ussage" This reverts commit c3284e4427a41c818acc161926cd2535dee9a6b9. * cleanup error checks --------- Co-authored-by: Claude Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Mattermost Build --- server/.golangci.yml | 1 - server/channels/api4/apitestlib.go | 95 ++++++++++++----------- server/channels/api4/brand_test.go | 71 +++++++++++++++++ server/channels/api4/emoji_test.go | 2 +- server/channels/api4/export_test.go | 23 +++--- server/channels/api4/import_test.go | 3 +- server/channels/api4/job_test.go | 12 +-- server/channels/api4/notify_admin_test.go | 18 ++--- server/channels/app/brand.go | 23 +++++- server/channels/app/file_test.go | 2 +- 10 files changed, 171 insertions(+), 79 deletions(-) diff --git a/server/.golangci.yml b/server/.golangci.yml index 544490d2d0..e31406f3ce 100644 --- a/server/.golangci.yml +++ b/server/.golangci.yml @@ -89,7 +89,6 @@ issues: channels/api4/team_local.go|\ channels/api4/websocket_test.go|\ channels/app/bot_test.go|\ - channels/app/brand.go|\ channels/app/file_test.go|\ channels/app/helper_test.go|\ channels/app/permissions_test.go|\ diff --git a/server/channels/api4/apitestlib.go b/server/channels/api4/apitestlib.go index 2b7529f3b9..54db2ee006 100644 --- a/server/channels/api4/apitestlib.go +++ b/server/channels/api4/apitestlib.go @@ -69,6 +69,8 @@ type TestHelper struct { LogBuffer *mlog.Buffer TestLogger *mlog.Logger + + workspace string } var mainHelper *testlib.MainHelper @@ -77,18 +79,14 @@ func SetMainHelper(mh *testlib.MainHelper) { mainHelper = mh } -func setupTestHelper(dbStore store.Store, searchEngine *searchengine.Broker, enterprise bool, includeCache bool, +func setupTestHelper(tb testing.TB, dbStore store.Store, searchEngine *searchengine.Broker, enterprise bool, includeCache bool, updateConfig func(*model.Config), options []app.Option, ) *TestHelper { tempWorkspace, err := os.MkdirTemp("", "apptest") - if err != nil { - panic(err) - } + require.NoError(tb, err) memoryStore, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{IgnoreEnvironmentOverrides: true}) - if err != nil { - panic("failed to initialize memory store: " + err.Error()) - } + require.NoError(tb, err, "failed to initialize memory store") memoryConfig := &model.Config{ SqlSettings: *mainHelper.GetSQLSettings(), @@ -96,7 +94,8 @@ func setupTestHelper(dbStore store.Store, searchEngine *searchengine.Broker, ent memoryConfig.SetDefaults() *memoryConfig.PluginSettings.Directory = filepath.Join(tempWorkspace, "plugins") *memoryConfig.PluginSettings.ClientDirectory = filepath.Join(tempWorkspace, "webapp") - memoryConfig.ServiceSettings.EnableLocalMode = model.NewPointer(true) + *memoryConfig.FileSettings.Directory = filepath.Join(tempWorkspace, "data") + *memoryConfig.ServiceSettings.EnableLocalMode = true *memoryConfig.ServiceSettings.LocalModeSocketLocation = filepath.Join(tempWorkspace, "mattermost_local.sock") *memoryConfig.LogSettings.EnableSentry = false // disable error reporting during tests *memoryConfig.LogSettings.ConsoleLevel = mlog.LvlStdLog.Name @@ -122,9 +121,7 @@ func setupTestHelper(dbStore store.Store, searchEngine *searchengine.Broker, ent memoryStore.Set(memoryConfig) configStore, err := config.NewStoreFromBacking(memoryStore, nil, false) - if err != nil { - panic(err) - } + require.NoError(tb, err) options = append(options, app.ConfigStore(configStore)) if includeCache { @@ -136,22 +133,20 @@ func setupTestHelper(dbStore store.Store, searchEngine *searchengine.Broker, ent buffer := &mlog.Buffer{} - testLogger, _ := mlog.NewLogger() - logCfg, _ := config.MloggerConfigFromLoggerConfig(&memoryConfig.LogSettings, nil, config.GetLogFileLocation) - if errCfg := testLogger.ConfigureTargets(logCfg, nil); errCfg != nil { - panic("failed to configure test logger: " + errCfg.Error()) - } - if errW := mlog.AddWriterTarget(testLogger, buffer, true, mlog.StdAll...); errW != nil { - panic("failed to add writer target to test logger: " + errW.Error()) - } + testLogger, err := mlog.NewLogger() + require.NoError(tb, err) + logCfg, err := config.MloggerConfigFromLoggerConfig(&memoryConfig.LogSettings, nil, config.GetLogFileLocation) + require.NoError(tb, err) + err = testLogger.ConfigureTargets(logCfg, nil) + require.NoError(tb, err, "failed to configure test logger") + err = mlog.AddWriterTarget(testLogger, buffer, true, mlog.StdAll...) + require.NoError(tb, err, "failed to add writer target to test logger") // lock logger config so server init cannot override it during testing. testLogger.LockConfiguration() options = append(options, app.SetLogger(testLogger)) s, err := app.NewServer(options...) - if err != nil { - panic(err) - } + require.NoError(tb, err) th := &TestHelper{ App: app.New(app.ServerConnector(s.Channels())), @@ -161,6 +156,7 @@ func setupTestHelper(dbStore store.Store, searchEngine *searchengine.Broker, ent IncludeCacheLayer: includeCache, TestLogger: testLogger, LogBuffer: buffer, + workspace: tempWorkspace, } if s.Platform().SearchEngine != nil && s.Platform().SearchEngine.BleveEngine != nil && searchEngine != nil { @@ -194,11 +190,11 @@ func setupTestHelper(dbStore store.Store, searchEngine *searchengine.Broker, ent *cfg.ServiceSettings.ListenAddress = "localhost:0" }) - if err := th.Server.Start(); err != nil { - panic(err) - } + err = th.Server.Start() + require.NoError(tb, err) - Init(th.App.Srv()) + _, err = Init(th.App.Srv()) + require.NoError(tb, err) web.New(th.App.Srv()) wsapi.Init(th.App.Srv()) @@ -249,8 +245,8 @@ func SetupEnterprise(tb testing.TB, options ...app.Option) *TestHelper { dbStore.MarkSystemRanUnitTests() mainHelper.PreloadMigrations() searchEngine := mainHelper.GetSearchEngine() - th := setupTestHelper(dbStore, searchEngine, true, true, nil, options) - th.InitLogin() + th := setupTestHelper(tb, dbStore, searchEngine, true, true, nil, options) + th.InitLogin(tb) return th } @@ -268,8 +264,8 @@ func Setup(tb testing.TB) *TestHelper { dbStore.MarkSystemRanUnitTests() mainHelper.PreloadMigrations() searchEngine := mainHelper.GetSearchEngine() - th := setupTestHelper(dbStore, searchEngine, false, true, nil, nil) - th.InitLogin() + th := setupTestHelper(tb, dbStore, searchEngine, false, true, nil, nil) + th.InitLogin(tb) return th } @@ -287,9 +283,9 @@ func SetupAndApplyConfigBeforeLogin(tb testing.TB, updateConfig func(cfg *model. dbStore.MarkSystemRanUnitTests() mainHelper.PreloadMigrations() searchEngine := mainHelper.GetSearchEngine() - th := setupTestHelper(dbStore, searchEngine, false, true, nil, nil) + th := setupTestHelper(tb, dbStore, searchEngine, false, true, nil, nil) th.App.UpdateConfig(updateConfig) - th.InitLogin() + th.InitLogin(tb) return th } @@ -307,13 +303,13 @@ func SetupConfig(tb testing.TB, updateConfig func(cfg *model.Config)) *TestHelpe dbStore.MarkSystemRanUnitTests() mainHelper.PreloadMigrations() searchEngine := mainHelper.GetSearchEngine() - th := setupTestHelper(dbStore, searchEngine, false, true, updateConfig, nil) - th.InitLogin() + th := setupTestHelper(tb, dbStore, searchEngine, false, true, updateConfig, nil) + th.InitLogin(tb) return th } func SetupConfigWithStoreMock(tb testing.TB, updateConfig func(cfg *model.Config)) *TestHelper { - th := setupTestHelper(testlib.GetMockStoreForSetupFunctions(), nil, false, false, updateConfig, nil) + th := setupTestHelper(tb, testlib.GetMockStoreForSetupFunctions(), nil, false, false, updateConfig, nil) statusMock := mocks.StatusStore{} statusMock.On("UpdateExpiredDNDStatuses").Return([]*model.Status{}, nil) statusMock.On("Get", "user1").Return(&model.Status{UserId: "user1", Status: model.StatusOnline}, nil) @@ -327,7 +323,7 @@ func SetupConfigWithStoreMock(tb testing.TB, updateConfig func(cfg *model.Config } func SetupWithStoreMock(tb testing.TB) *TestHelper { - th := setupTestHelper(testlib.GetMockStoreForSetupFunctions(), nil, false, false, nil, nil) + th := setupTestHelper(tb, testlib.GetMockStoreForSetupFunctions(), nil, false, false, nil, nil) statusMock := mocks.StatusStore{} statusMock.On("UpdateExpiredDNDStatuses").Return([]*model.Status{}, nil) statusMock.On("Get", "user1").Return(&model.Status{UserId: "user1", Status: model.StatusOnline}, nil) @@ -341,7 +337,7 @@ func SetupWithStoreMock(tb testing.TB) *TestHelper { } func SetupEnterpriseWithStoreMock(tb testing.TB, options ...app.Option) *TestHelper { - th := setupTestHelper(testlib.GetMockStoreForSetupFunctions(), nil, true, false, nil, options) + th := setupTestHelper(tb, testlib.GetMockStoreForSetupFunctions(), nil, true, false, nil, options) statusMock := mocks.StatusStore{} statusMock.On("UpdateExpiredDNDStatuses").Return([]*model.Status{}, nil) statusMock.On("Get", "user1").Return(&model.Status{UserId: "user1", Status: model.StatusOnline}, nil) @@ -368,8 +364,8 @@ func SetupWithServerOptions(tb testing.TB, options []app.Option) *TestHelper { dbStore.MarkSystemRanUnitTests() mainHelper.PreloadMigrations() searchEngine := mainHelper.GetSearchEngine() - th := setupTestHelper(dbStore, searchEngine, false, true, nil, options) - th.InitLogin() + th := setupTestHelper(tb, dbStore, searchEngine, false, true, nil, options) + th.InitLogin(tb) return th } @@ -387,8 +383,8 @@ func SetupEnterpriseWithServerOptions(tb testing.TB, options []app.Option) *Test dbStore.MarkSystemRanUnitTests() mainHelper.PreloadMigrations() searchEngine := mainHelper.GetSearchEngine() - th := setupTestHelper(dbStore, searchEngine, true, true, nil, options) - th.InitLogin() + th := setupTestHelper(tb, dbStore, searchEngine, true, true, nil, options) + th.InitLogin(tb) return th } @@ -413,7 +409,16 @@ func (th *TestHelper) TearDown() { // Clean all the caches th.App.Srv().InvalidateAllCaches() } + th.ShutdownApp() + + // Cleanup the workspace + if th.workspace != "" { + err := os.RemoveAll(th.workspace) + if err != nil { + panic(err) + } + } } func closeBody(r *http.Response) { @@ -434,8 +439,8 @@ var ( } ) -func (th *TestHelper) InitLogin() *TestHelper { - th.waitForConnectivity() +func (th *TestHelper) InitLogin(tb testing.TB) *TestHelper { + th.waitForConnectivity(tb) // create users once and cache them because password hashing is slow initBasicOnce.Do(func() { @@ -527,7 +532,7 @@ func (th *TestHelper) DeleteBots() *TestHelper { return th } -func (th *TestHelper) waitForConnectivity() { +func (th *TestHelper) waitForConnectivity(tb testing.TB) { for i := 0; i < 1000; i++ { conn, err := net.Dial("tcp", fmt.Sprintf("localhost:%v", th.App.Srv().ListenAddr.Port)) if err == nil { @@ -536,7 +541,7 @@ func (th *TestHelper) waitForConnectivity() { } time.Sleep(time.Millisecond * 20) } - panic("unable to connect") + tb.Fatal("unable to connect") } func (th *TestHelper) CreateClient() *model.Client4 { diff --git a/server/channels/api4/brand_test.go b/server/channels/api4/brand_test.go index fe16454043..e651cbd518 100644 --- a/server/channels/api4/brand_test.go +++ b/server/channels/api4/brand_test.go @@ -6,6 +6,7 @@ package api4 import ( "context" "net/http" + "strings" "testing" "github.com/stretchr/testify/require" @@ -64,6 +65,76 @@ func TestUploadBrandImage(t *testing.T) { CheckCreatedStatus(t, resp) } +func TestUploadBrandImageTwice(t *testing.T) { + th := Setup(t) + defer th.TearDown() + + data, err := testutils.ReadTestFile("test.png") + require.NoError(t, err) + + // First upload as system admin + resp, err := th.SystemAdminClient.UploadBrandImage(context.Background(), data) + require.NoError(t, err) + CheckCreatedStatus(t, resp) + + // Verify the image exists and contents match what was uploaded + receivedImg, resp, err := th.SystemAdminClient.GetBrandImage(context.Background()) + require.NoError(t, err) + require.NotNil(t, receivedImg) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.NotEmpty(t, receivedImg, "Received image data should not be empty") + + // Get the list of files in the brand directory + files, err := th.App.FileBackend().ListDirectory("brand/") + require.NoError(t, err) + require.Len(t, files, 1, "Expected only the original image file") + + // ListDirectory returns paths with the directory prefix included + fileName := files[0] + fileName = strings.TrimPrefix(fileName, "brand/") + require.Equal(t, "image.png", fileName, "Expected the original image file") + + // Second upload (which should back up the previous one) + data2, err := testutils.ReadTestFile("test.tiff") + require.NoError(t, err) + + resp, err = th.SystemAdminClient.UploadBrandImage(context.Background(), data2) + require.NoError(t, err) + CheckCreatedStatus(t, resp) + + // Get the list of files in the brand directory again + files, err = th.App.FileBackend().ListDirectory("brand/") + require.NoError(t, err) + + // Should now have the new image.png and a backup with timestamp + require.Len(t, files, 2, "Expected the original and backup files") + + // Check that one of the files is image.png + hasOriginal := false + hasBackup := false + for _, file := range files { + // ListDirectory returns paths with the directory prefix included + fileName := strings.TrimPrefix(file, "brand/") + + if fileName == "image.png" { + hasOriginal = true + } else if strings.HasSuffix(fileName, ".png") && strings.Contains(fileName, "-") { + // Backup file should have a timestamp format like 2006-01-02T15:04:05.png + hasBackup = true + } + } + + require.True(t, hasOriginal, "Original image.png file should exist") + require.True(t, hasBackup, "Backup image file should exist") + + // Verify the new image is available through the API and matches what was uploaded + receivedImg2, resp, err := th.SystemAdminClient.GetBrandImage(context.Background()) + require.NoError(t, err) + require.NotNil(t, receivedImg2) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.NotEmpty(t, receivedImg2, "Received image data should not be empty") +} + func TestDeleteBrandImage(t *testing.T) { th := Setup(t) defer th.TearDown() diff --git a/server/channels/api4/emoji_test.go b/server/channels/api4/emoji_test.go index c0cad849b0..12fd73af68 100644 --- a/server/channels/api4/emoji_test.go +++ b/server/channels/api4/emoji_test.go @@ -42,7 +42,7 @@ func TestCreateEmoji(t *testing.T) { emojiHeight := app.MaxEmojiHeight * 2 // check that emoji gets resized correctly, respecting proportions, and is of expected type checkEmojiFile := func(id, expectedImageType string) { - path, _ := fileutils.FindDir("data") + path := *th.App.Config().FileSettings.Directory file, fileErr := os.Open(filepath.Join(path, "/emoji/"+id+"/image")) require.NoError(t, fileErr) defer file.Close() diff --git a/server/channels/api4/export_test.go b/server/channels/api4/export_test.go index e11d622f89..9fab2b9b5c 100644 --- a/server/channels/api4/export_test.go +++ b/server/channels/api4/export_test.go @@ -12,7 +12,6 @@ import ( "testing" "github.com/mattermost/mattermost/server/public/model" - "github.com/mattermost/mattermost/server/v8/channels/utils/fileutils" "github.com/stretchr/testify/require" ) @@ -34,8 +33,7 @@ func TestListExports(t *testing.T) { require.Empty(t, exports) }, "no exports") - dataDir, found := fileutils.FindDir("data") - require.True(t, found) + dataDir := *th.App.Config().FileSettings.Directory th.TestForSystemAdminAndLocal(t, func(t *testing.T, c *model.Client4) { exportDir := filepath.Join(dataDir, *th.App.Config().ExportSettings.Directory) @@ -58,11 +56,13 @@ func TestListExports(t *testing.T) { }, "expected exports") th.TestForSystemAdminAndLocal(t, func(t *testing.T, c *model.Client4) { - value := *th.App.Config().ExportSettings.Directory - th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ExportSettings.Directory = value + "new" }) - defer th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ExportSettings.Directory = value }) + originalExportDir := *th.App.Config().ExportSettings.Directory + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ExportSettings.Directory = "new" }) + defer th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ExportSettings.Directory = originalExportDir + }) - exportDir := filepath.Join(dataDir, value+"new") + exportDir := filepath.Join(dataDir, *th.App.Config().ExportSettings.Directory) err := os.Mkdir(exportDir, 0700) require.NoError(t, err) defer func() { @@ -96,8 +96,7 @@ func TestDeleteExport(t *testing.T) { CheckErrorID(t, err, "api.context.permissions.app_error") }) - dataDir, found := fileutils.FindDir("data") - require.True(t, found) + dataDir := *th.App.Config().FileSettings.Directory exportDir := filepath.Join(dataDir, *th.App.Config().ExportSettings.Directory) th.TestForSystemAdminAndLocal(t, func(t *testing.T, c *model.Client4) { @@ -143,8 +142,7 @@ func TestDownloadExport(t *testing.T) { require.Zero(t, n) }) - dataDir, found := fileutils.FindDir("data") - require.True(t, found) + dataDir := *th.App.Config().FileSettings.Directory exportDir := filepath.Join(dataDir, *th.App.Config().ExportSettings.Directory) th.TestForSystemAdminAndLocal(t, func(t *testing.T, c *model.Client4) { @@ -201,8 +199,7 @@ func BenchmarkDownloadExport(b *testing.B) { th := Setup(b) defer th.TearDown() - dataDir, found := fileutils.FindDir("data") - require.True(b, found) + dataDir := *th.App.Config().FileSettings.Directory exportDir := filepath.Join(dataDir, *th.App.Config().ExportSettings.Directory) err := os.Mkdir(exportDir, 0700) diff --git a/server/channels/api4/import_test.go b/server/channels/api4/import_test.go index cfbe0ec4ab..7acb7a3377 100644 --- a/server/channels/api4/import_test.go +++ b/server/channels/api4/import_test.go @@ -61,8 +61,7 @@ func TestListImports(t *testing.T) { require.Nil(t, imports) }) - dataDir, found := fileutils.FindDir("data") - require.True(t, found) + dataDir := *th.App.Config().FileSettings.Directory th.TestForSystemAdminAndLocal(t, func(t *testing.T, c *model.Client4) { imports, _, err := c.ListImports(context.Background()) diff --git a/server/channels/api4/job_test.go b/server/channels/api4/job_test.go index 394f326080..d3ec4b9a5c 100644 --- a/server/channels/api4/job_test.go +++ b/server/channels/api4/job_test.go @@ -280,9 +280,9 @@ func TestDownloadJob(t *testing.T) { require.NoError(t, delErr, "Failed to delete job %s", job.Id) }() - filePath := "./data/export/" + job.Id + "/testdat.txt" - mkdirAllErr := os.MkdirAll(filepath.Dir(filePath), 0770) - require.NoError(t, mkdirAllErr) + filePath := filepath.Join(*th.App.Config().FileSettings.Directory, "export", job.Id+"/testdat.txt") + err = os.MkdirAll(filepath.Dir(filePath), 0770) + require.NoError(t, err) _, createErr := os.Create(filePath) require.NoError(t, createErr) @@ -314,9 +314,9 @@ func TestDownloadJob(t *testing.T) { // Now we stub the results of the job into the same directory and try to download it again // This time we should successfully retrieve the results without any error - filePath = "./data/export/" + job.Id + ".zip" - mkdirAllErr = os.MkdirAll(filepath.Dir(filePath), 0770) - require.NoError(t, mkdirAllErr) + filePath = filepath.Join(*th.App.Config().FileSettings.Directory, "export", job.Id+".zip") + err = os.MkdirAll(filepath.Dir(filePath), 0770) + require.NoError(t, err) _, createErr = os.Create(filePath) require.NoError(t, createErr) diff --git a/server/channels/api4/notify_admin_test.go b/server/channels/api4/notify_admin_test.go index 94f3c71670..085bd317cb 100644 --- a/server/channels/api4/notify_admin_test.go +++ b/server/channels/api4/notify_admin_test.go @@ -14,7 +14,7 @@ import ( func TestNotifyAdmin(t *testing.T) { t.Run("error when notifying with empty data", func(t *testing.T) { - th := Setup(t).InitBasic().InitLogin() + th := Setup(t).InitBasic() defer th.TearDown() statusCode, err := th.Client.NotifyAdmin(context.Background(), nil) @@ -24,7 +24,7 @@ func TestNotifyAdmin(t *testing.T) { }) t.Run("error when plan is unknown when notifying on upgrade", func(t *testing.T) { - th := Setup(t).InitBasic().InitLogin() + th := Setup(t).InitBasic() defer th.TearDown() statusCode, err := th.Client.NotifyAdmin(context.Background(), &model.NotifyAdminToUpgradeRequest{ @@ -38,7 +38,7 @@ func TestNotifyAdmin(t *testing.T) { }) t.Run("error when plan is unknown when notifying to trial", func(t *testing.T) { - th := Setup(t).InitBasic().InitLogin() + th := Setup(t).InitBasic() defer th.TearDown() statusCode, err := th.Client.NotifyAdmin(context.Background(), &model.NotifyAdminToUpgradeRequest{ @@ -53,7 +53,7 @@ func TestNotifyAdmin(t *testing.T) { }) t.Run("error when feature is unknown when notifying on upgrade", func(t *testing.T) { - th := Setup(t).InitBasic().InitLogin() + th := Setup(t).InitBasic() defer th.TearDown() statusCode, err := th.Client.NotifyAdmin(context.Background(), &model.NotifyAdminToUpgradeRequest{ @@ -67,7 +67,7 @@ func TestNotifyAdmin(t *testing.T) { }) t.Run("error when feature is unknown when notifying to trial", func(t *testing.T) { - th := Setup(t).InitBasic().InitLogin() + th := Setup(t).InitBasic() defer th.TearDown() statusCode, err := th.Client.NotifyAdmin(context.Background(), &model.NotifyAdminToUpgradeRequest{ @@ -82,7 +82,7 @@ func TestNotifyAdmin(t *testing.T) { }) t.Run("error when user tries to notify again on same feature within the cool off period", func(t *testing.T) { - th := Setup(t).InitBasic().InitLogin() + th := Setup(t).InitBasic() defer th.TearDown() statusCode, err := th.Client.NotifyAdmin(context.Background(), &model.NotifyAdminToUpgradeRequest{ @@ -104,7 +104,7 @@ func TestNotifyAdmin(t *testing.T) { }) t.Run("successfully save upgrade notification", func(t *testing.T) { - th := Setup(t).InitBasic().InitLogin() + th := Setup(t).InitBasic() defer th.TearDown() statusCode, err := th.Client.NotifyAdmin(context.Background(), &model.NotifyAdminToUpgradeRequest{ @@ -119,7 +119,7 @@ func TestNotifyAdmin(t *testing.T) { func TestTriggerNotifyAdmin(t *testing.T) { t.Run("error when EnableAPITriggerAdminNotifications is not true", func(t *testing.T) { - th := Setup(t).InitBasic().InitLogin() + th := Setup(t).InitBasic() defer th.TearDown() th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableAPITriggerAdminNotifications = false }) @@ -132,7 +132,7 @@ func TestTriggerNotifyAdmin(t *testing.T) { }) t.Run("error when non admins try to trigger notifications", func(t *testing.T) { - th := Setup(t).InitBasic().InitLogin() + th := Setup(t).InitBasic() defer th.TearDown() th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableAPITriggerAdminNotifications = true }) diff --git a/server/channels/app/brand.go b/server/channels/app/brand.go index 68dae47f3f..0fae9a974c 100644 --- a/server/channels/app/brand.go +++ b/server/channels/app/brand.go @@ -10,6 +10,7 @@ import ( "time" "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/shared/mlog" "github.com/mattermost/mattermost/server/public/shared/request" ) @@ -45,7 +46,27 @@ func (a *App) SaveBrandImage(rctx request.CTX, imageData *multipart.FileHeader) } t := time.Now() - a.MoveFile(BrandFilePath+BrandFileName, BrandFilePath+t.Format("2006-01-02T15:04:05")+".png") + // Try to backup the old brand image if it exists + oldPath := BrandFilePath + BrandFileName + newPath := BrandFilePath + t.Format("2006-01-02T15:04:05") + ".png" + + fileExists, appErr := a.FileExists(oldPath) + if appErr != nil { + rctx.Logger().Warn("Failed to check if brand image exists before backup", mlog.String("path", oldPath), mlog.Err(appErr)) + } + + if fileExists { + if err := a.MoveFile(oldPath, newPath); err != nil { + // Log the error but continue since this is a non-critical operation - we're just trying to + // backup the old brand image, but it's not a problem if we can't + rctx.Logger().Warn( + "Failed to backup old brand image", + mlog.Err(err), + mlog.String("oldPath", oldPath), + mlog.String("newPath", newPath), + ) + } + } if _, err := a.WriteFile(buf, BrandFilePath+BrandFileName); err != nil { return model.NewAppError("SaveBrandImage", "brand.save_brand_image.save_image.app_error", nil, "", http.StatusInternalServerError).Wrap(err) diff --git a/server/channels/app/file_test.go b/server/channels/app/file_test.go index a6ec59b4ff..a70af7fd6f 100644 --- a/server/channels/app/file_test.go +++ b/server/channels/app/file_test.go @@ -385,7 +385,7 @@ func TestGenerateThumbnailImage(t *testing.T) { th := Setup(t) defer th.TearDown() img := createDummyImage() - dataPath, _ := fileutils.FindDir("data") + dataPath := *th.App.Config().FileSettings.Directory thumbnailName := "thumb.jpg" thumbnailPath := filepath.Join(dataPath, thumbnailName)