Merge pull request 'test(avatar): deleting a user avatar is idempotent' (#4024) from earl-warren/forgejo:wip-delete-avatar into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4024
Reviewed-by: Victoria <efertone@noreply.codeberg.org>

modules/storage/helper.go

@@ -10,30 +10,30 @@ import (
 	"os"
 )
 
-var UninitializedStorage = discardStorage("uninitialized storage")
+var UninitializedStorage = DiscardStorage("uninitialized storage")
 
-type discardStorage string
+type DiscardStorage string
 
-func (s discardStorage) Open(_ string) (Object, error) {
+func (s DiscardStorage) Open(_ string) (Object, error) {
 	return nil, fmt.Errorf("%s", s)
 }
 
-func (s discardStorage) Save(_ string, _ io.Reader, _ int64) (int64, error) {
+func (s DiscardStorage) Save(_ string, _ io.Reader, _ int64) (int64, error) {
 	return 0, fmt.Errorf("%s", s)
 }
 
-func (s discardStorage) Stat(_ string) (os.FileInfo, error) {
+func (s DiscardStorage) Stat(_ string) (os.FileInfo, error) {
 	return nil, fmt.Errorf("%s", s)
 }
 
-func (s discardStorage) Delete(_ string) error {
+func (s DiscardStorage) Delete(_ string) error {
 	return fmt.Errorf("%s", s)
 }
 
-func (s discardStorage) URL(_, _ string) (*url.URL, error) {
+func (s DiscardStorage) URL(_, _ string) (*url.URL, error) {
 	return nil, fmt.Errorf("%s", s)
 }
 
-func (s discardStorage) IterateObjects(_ string, _ func(string, Object) error) error {
+func (s DiscardStorage) IterateObjects(_ string, _ func(string, Object) error) error {
 	return fmt.Errorf("%s", s)
 }

modules/storage/helper_test.go

@@ -11,9 +11,9 @@ import (
 )
 
 func Test_discardStorage(t *testing.T) {
-	tests := []discardStorage{
+	tests := []DiscardStorage{
 		UninitializedStorage,
-		discardStorage("empty"),
+		DiscardStorage("empty"),
 	}
 	for _, tt := range tests {
 		t.Run(string(tt), func(t *testing.T) {

modules/storage/storage.go

@@ -170,7 +170,7 @@ func initAvatars() (err error) {
 
 func initAttachments() (err error) {
 	if !setting.Attachment.Enabled {
-		Attachments = discardStorage("Attachment isn't enabled")
+		Attachments = DiscardStorage("Attachment isn't enabled")
 		return nil
 	}
 	log.Info("Initialising Attachment storage with type: %s", setting.Attachment.Storage.Type)
@@ -180,7 +180,7 @@ func initAttachments() (err error) {
 
 func initLFS() (err error) {
 	if !setting.LFS.StartServer {
-		LFS = discardStorage("LFS isn't enabled")
+		LFS = DiscardStorage("LFS isn't enabled")
 		return nil
 	}
 	log.Info("Initialising LFS storage with type: %s", setting.LFS.Storage.Type)
@@ -202,7 +202,7 @@ func initRepoArchives() (err error) {
 
 func initPackages() (err error) {
 	if !setting.Packages.Enabled {
-		Packages = discardStorage("Packages isn't enabled")
+		Packages = DiscardStorage("Packages isn't enabled")
 		return nil
 	}
 	log.Info("Initialising Packages storage with type: %s", setting.Packages.Storage.Type)
@@ -212,8 +212,8 @@ func initPackages() (err error) {
 
 func initActions() (err error) {
 	if !setting.Actions.Enabled {
-		Actions = discardStorage("Actions isn't enabled")
+		Actions = DiscardStorage("Actions isn't enabled")
-		ActionsArtifacts = discardStorage("ActionsArtifacts isn't enabled")
+		ActionsArtifacts = DiscardStorage("ActionsArtifacts isn't enabled")
 		return nil
 	}
 	log.Info("Initialising Actions storage with type: %s", setting.Actions.LogStorage.Type)

services/user/avatar_test.go

@@ -7,6 +7,7 @@ import (
 	"bytes"
 	"image"
 	"image/png"
+	"os"
 	"testing"
 
 	"code.gitea.io/gitea/models/db"
@@ -18,11 +19,22 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+type alreadyDeletedStorage struct {
+	storage.DiscardStorage
+}
+
+func (s alreadyDeletedStorage) Delete(_ string) error {
+	return os.ErrNotExist
+}
+
 func TestUserDeleteAvatar(t *testing.T) {
 	myImage := image.NewRGBA(image.Rect(0, 0, 1, 1))
 	var buff bytes.Buffer
 	png.Encode(&buff, myImage)
 
+	t.Run("AtomicStorageFailure", func(t *testing.T) {
+		defer test.MockProtect[storage.ObjectStorage](&storage.Avatars)()
+
 		assert.NoError(t, unittest.PrepareTestDatabase())
 		user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 
@@ -31,17 +43,38 @@ func TestUserDeleteAvatar(t *testing.T) {
 		verification := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 		assert.NotEqual(t, "", verification.Avatar)
 
-	t.Run("AtomicStorageFailure", func(t *testing.T) {
+		// fail to delete ...
-		defer test.MockVariableValue[storage.ObjectStorage](&storage.Avatars, storage.UninitializedStorage)()
+		storage.Avatars = storage.UninitializedStorage
 		err = DeleteAvatar(db.DefaultContext, user)
 		assert.Error(t, err)
-		verification := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+
+		// ... the avatar is not removed from the database
+		verification = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 		assert.True(t, verification.UseCustomAvatar)
+
+		// already deleted ...
+		storage.Avatars = alreadyDeletedStorage{}
+		err = DeleteAvatar(db.DefaultContext, user)
+		assert.NoError(t, err)
+
+		// ... the avatar is removed from the database
+		verification = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+		assert.Equal(t, "", verification.Avatar)
 	})
 
+	t.Run("Success", func(t *testing.T) {
+		assert.NoError(t, unittest.PrepareTestDatabase())
+		user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+
+		err := UploadAvatar(db.DefaultContext, user, buff.Bytes())
+		assert.NoError(t, err)
+		verification := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+		assert.NotEqual(t, "", verification.Avatar)
+
 		err = DeleteAvatar(db.DefaultContext, user)
 		assert.NoError(t, err)
 
 		verification = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 		assert.Equal(t, "", verification.Avatar)
+	})
 }