[MM-61099] Fix errcheck issues in server/channels/app/brand.go (#30679)

* [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 <noreply@anthropic.com>

* [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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Mattermost Build <build@mattermost.com>

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|\

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 {
+	require.NoError(tb, err)
-		panic(err)
-	}
 
 	memoryStore, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{IgnoreEnvironmentOverrides: true})
-	if err != nil {
+	require.NoError(tb, err, "failed to initialize memory store")
-		panic("failed to initialize memory store: " + err.Error())
-	}
 
 	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 {
+	require.NoError(tb, err)
-		panic(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()
+	testLogger, err := mlog.NewLogger()
-	logCfg, _ := config.MloggerConfigFromLoggerConfig(&memoryConfig.LogSettings, nil, config.GetLogFileLocation)
+	require.NoError(tb, err)
-	if errCfg := testLogger.ConfigureTargets(logCfg, nil); errCfg != nil {
+	logCfg, err := config.MloggerConfigFromLoggerConfig(&memoryConfig.LogSettings, nil, config.GetLogFileLocation)
-		panic("failed to configure test logger: " + errCfg.Error())
+	require.NoError(tb, err)
-	}
+	err = testLogger.ConfigureTargets(logCfg, nil)
-	if errW := mlog.AddWriterTarget(testLogger, buffer, true, mlog.StdAll...); errW != nil {
+	require.NoError(tb, err, "failed to configure test logger")
-		panic("failed to add writer target to test logger: " + errW.Error())
+	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 {
+	require.NoError(tb, err)
-		panic(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 {
+	err = th.Server.Start()
-		panic(err)
+	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 := setupTestHelper(tb, dbStore, searchEngine, true, true, nil, options)
-	th.InitLogin()
+	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 := setupTestHelper(tb, dbStore, searchEngine, false, true, nil, nil)
-	th.InitLogin()
+	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 := setupTestHelper(tb, dbStore, searchEngine, false, true, updateConfig, nil)
-	th.InitLogin()
+	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 := setupTestHelper(tb, dbStore, searchEngine, false, true, nil, options)
-	th.InitLogin()
+	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 := setupTestHelper(tb, dbStore, searchEngine, true, true, nil, options)
-	th.InitLogin()
+	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 {
+func (th *TestHelper) InitLogin(tb testing.TB) *TestHelper {
-	th.waitForConnectivity()
+	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 {

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

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

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")
+	dataDir := *th.App.Config().FileSettings.Directory
-	require.True(t, found)
 
 	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
+		originalExportDir := *th.App.Config().ExportSettings.Directory
-		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ExportSettings.Directory = value + "new" })
+		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ExportSettings.Directory = "new" })
-		defer th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ExportSettings.Directory = value })
+		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")
+	dataDir := *th.App.Config().FileSettings.Directory
-	require.True(t, found)
 	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")
+	dataDir := *th.App.Config().FileSettings.Directory
-	require.True(t, found)
 	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")
+	dataDir := *th.App.Config().FileSettings.Directory
-	require.True(b, found)
 	exportDir := filepath.Join(dataDir, *th.App.Config().ExportSettings.Directory)
 
 	err := os.Mkdir(exportDir, 0700)

server/channels/api4/import_test.go

@@ -61,8 +61,7 @@ func TestListImports(t *testing.T) {
 		require.Nil(t, imports)
 	})
 
-	dataDir, found := fileutils.FindDir("data")
+	dataDir := *th.App.Config().FileSettings.Directory
-	require.True(t, found)
 
 	th.TestForSystemAdminAndLocal(t, func(t *testing.T, c *model.Client4) {
 		imports, _, err := c.ListImports(context.Background())

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"
+	filePath := filepath.Join(*th.App.Config().FileSettings.Directory, "export", job.Id+"/testdat.txt")
-	mkdirAllErr := os.MkdirAll(filepath.Dir(filePath), 0770)
+	err = os.MkdirAll(filepath.Dir(filePath), 0770)
-	require.NoError(t, mkdirAllErr)
+	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"
+	filePath = filepath.Join(*th.App.Config().FileSettings.Directory, "export", job.Id+".zip")
-	mkdirAllErr = os.MkdirAll(filepath.Dir(filePath), 0770)
+	err = os.MkdirAll(filepath.Dir(filePath), 0770)
-	require.NoError(t, mkdirAllErr)
+	require.NoError(t, err)
 
 	_, createErr = os.Create(filePath)
 	require.NoError(t, createErr)

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

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)

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)