fix(list): when list background is removed, delete file from file system and DB (#1372)

Co-authored-by: testinho.testador <testinho.testador@noreply.kolaente.de>
Reviewed-on: https://kolaente.dev/vikunja/api/pulls/1372
Reviewed-by: konrad <k@knt.li>
Co-authored-by: clos <clos@noreply.kolaente.de>
Co-committed-by: clos <clos@noreply.kolaente.de>

pkg/db/fixtures/lists.yml

@@ -242,3 +242,14 @@
   position: 7
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
+-
+  id: 25
+  title: Test25 with background
+  description: Lorem Ipsum
+  identifier: test6
+  owner_id: 6
+  namespace_id: 6
+  background_file_id: 1
+  position: 8
+  updated: 2018-12-02 15:13:12
+  created: 2018-12-01 15:13:12

pkg/files/files.go

@@ -140,7 +140,7 @@ func (f *File) Delete() (err error) {
 		var perr *os.PathError
 		if errors.As(err, &perr) {
 			// Don't fail when removing the file failed
-			log.Errorf("Error deleting file %d: %w", err)
+			log.Errorf("Error deleting file %d: %w", f.ID, err)
 			return s.Commit()
 		}
 

pkg/models/list.go

@@ -795,6 +795,11 @@ func (l *List) Create(s *xorm.Session, a web.Auth) (err error) {
 // @Router /lists/{id} [delete]
 func (l *List) Delete(s *xorm.Session, a web.Auth) (err error) {
 
+	fullList, err := GetListSimpleByID(s, l.ID)
+	if err != nil {
+		return
+	}
+
 	// Delete the list
 	_, err = s.ID(l.ID).Delete(&List{})
 	if err != nil {
@@ -815,12 +820,28 @@ func (l *List) Delete(s *xorm.Session, a web.Auth) (err error) {
 		}
 	}
 
+	err = fullList.DeleteBackgroundFileIfExists()
+	if err != nil {
+		return
+	}
+
 	return events.Dispatch(&ListDeletedEvent{
 		List: l,
 		Doer: a,
 	})
 }
 
+// DeleteBackgroundFileIfExists deletes the list's background file from the db and the filesystem,
+// if one exists
+func (l *List) DeleteBackgroundFileIfExists() (err error) {
+	if l.BackgroundFileID == 0 {
+		return
+	}
+
+	file := files.File{ID: l.BackgroundFileID}
+	return file.Delete()
+}
+
 // SetListBackground sets a background file as list background in the db
 func SetListBackground(s *xorm.Session, listID int64, background *files.File, blurHash string) (err error) {
 	l := &List{

pkg/models/list_test.go

@@ -21,6 +21,7 @@ import (
 	"testing"
 
 	"code.vikunja.io/api/pkg/db"
+	"code.vikunja.io/api/pkg/files"
 	"code.vikunja.io/api/pkg/user"
 	"github.com/stretchr/testify/assert"
 )
@@ -246,17 +247,76 @@ func TestList_CreateOrUpdate(t *testing.T) {
 }
 
 func TestList_Delete(t *testing.T) {
-	db.LoadAndAssertFixtures(t)
+	t.Run("normal", func(t *testing.T) {
-	s := db.NewSession()
+		db.LoadAndAssertFixtures(t)
-	list := List{
+		s := db.NewSession()
-		ID: 1,
+		list := List{
-	}
+			ID: 1,
-	err := list.Delete(s, &user.User{ID: 1})
+		}
-	assert.NoError(t, err)
+		err := list.Delete(s, &user.User{ID: 1})
-	err = s.Commit()
+		assert.NoError(t, err)
-	assert.NoError(t, err)
+		err = s.Commit()
-	db.AssertMissing(t, "lists", map[string]interface{}{
+		assert.NoError(t, err)
-		"id": 1,
+		db.AssertMissing(t, "lists", map[string]interface{}{
+			"id": 1,
+		})
+	})
+	t.Run("with background", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		files.InitTestFileFixtures(t)
+		s := db.NewSession()
+		list := List{
+			ID: 25,
+		}
+		err := list.Delete(s, &user.User{ID: 6})
+		assert.NoError(t, err)
+		err = s.Commit()
+		assert.NoError(t, err)
+		db.AssertMissing(t, "lists", map[string]interface{}{
+			"id": 25,
+		})
+		db.AssertMissing(t, "files", map[string]interface{}{
+			"id": 1,
+		})
+	})
+}
+
+func TestList_DeleteBackgroundFileIfExists(t *testing.T) {
+	t.Run("list with background", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		files.InitTestFileFixtures(t)
+		s := db.NewSession()
+		file := &files.File{ID: 1}
+		list := List{
+			ID:               1,
+			BackgroundFileID: file.ID,
+		}
+		err := SetListBackground(s, list.ID, file, "")
+		assert.NoError(t, err)
+		err = list.DeleteBackgroundFileIfExists()
+		assert.NoError(t, err)
+	})
+	t.Run("list with invalid background", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		files.InitTestFileFixtures(t)
+		s := db.NewSession()
+		file := &files.File{ID: 9999}
+		list := List{
+			ID:               1,
+			BackgroundFileID: file.ID,
+		}
+		err := SetListBackground(s, list.ID, file, "")
+		assert.NoError(t, err)
+		err = list.DeleteBackgroundFileIfExists()
+		assert.Error(t, err)
+		assert.True(t, files.IsErrFileDoesNotExist(err))
+	})
+	t.Run("list without background", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		files.InitTestFileFixtures(t)
+		list := List{ID: 1}
+		err := list.DeleteBackgroundFileIfExists()
+		assert.NoError(t, err)
 	})
 }
 

pkg/modules/background/handler/background.go

@@ -341,6 +341,11 @@ func RemoveListBackground(c echo.Context) error {
 		return err
 	}
 
+	err = list.DeleteBackgroundFileIfExists()
+	if err != nil {
+		return err
+	}
+
 	list.BackgroundFileID = 0
 	list.BackgroundInformation = nil
 	list.BackgroundBlurHash = ""

pkg/modules/background/upload/upload.go

@@ -54,11 +54,9 @@ func (p *Provider) Search(s *xorm.Session, search string, page int64) (result []
 // @Router /lists/{id}/backgrounds/upload [put]
 func (p *Provider) Set(s *xorm.Session, img *background.Image, list *models.List, auth web.Auth) (err error) {
 	// Remove the old background if one exists
-	if list.BackgroundFileID != 0 {
+	err = list.DeleteBackgroundFileIfExists()
-		file := files.File{ID: list.BackgroundFileID}
+	if err != nil {
-		if err := file.Delete(); err != nil {
+		return err
-			return err
-		}
 	}
 
 	file := &files.File{}