From afdceb0aff242c474385bb7351483a947113a659 Mon Sep 17 00:00:00 2001 From: clos Date: Wed, 1 Feb 2023 11:38:23 +0000 Subject: [PATCH] fix(list): when list background is removed, delete file from file system and DB (#1372) Co-authored-by: testinho.testador Reviewed-on: https://kolaente.dev/vikunja/api/pulls/1372 Reviewed-by: konrad Co-authored-by: clos Co-committed-by: clos --- pkg/db/fixtures/lists.yml | 11 +++ pkg/files/files.go | 2 +- pkg/models/list.go | 21 +++++ pkg/models/list_test.go | 82 +++++++++++++++++--- pkg/modules/background/handler/background.go | 5 ++ pkg/modules/background/upload/upload.go | 8 +- 6 files changed, 112 insertions(+), 17 deletions(-) diff --git a/pkg/db/fixtures/lists.yml b/pkg/db/fixtures/lists.yml index a1d5e84ee..356253955 100644 --- a/pkg/db/fixtures/lists.yml +++ b/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 diff --git a/pkg/files/files.go b/pkg/files/files.go index 96eb4bd46..12961f406 100644 --- a/pkg/files/files.go +++ b/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() } diff --git a/pkg/models/list.go b/pkg/models/list.go index 8949f9052..44812c7e9 100644 --- a/pkg/models/list.go +++ b/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{ diff --git a/pkg/models/list_test.go b/pkg/models/list_test.go index 7c085f9f6..f908ef88d 100644 --- a/pkg/models/list_test.go +++ b/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) - s := db.NewSession() - list := List{ - ID: 1, - } - err := list.Delete(s, &user.User{ID: 1}) - assert.NoError(t, err) - err = s.Commit() - assert.NoError(t, err) - db.AssertMissing(t, "lists", map[string]interface{}{ - "id": 1, + t.Run("normal", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + list := List{ + ID: 1, + } + err := list.Delete(s, &user.User{ID: 1}) + assert.NoError(t, err) + err = s.Commit() + assert.NoError(t, err) + 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) }) } diff --git a/pkg/modules/background/handler/background.go b/pkg/modules/background/handler/background.go index 6dfd74437..f5c3fc243 100644 --- a/pkg/modules/background/handler/background.go +++ b/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 = "" diff --git a/pkg/modules/background/upload/upload.go b/pkg/modules/background/upload/upload.go index 3bd3d5fea..dac3eb229 100644 --- a/pkg/modules/background/upload/upload.go +++ b/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 { - file := files.File{ID: list.BackgroundFileID} - if err := file.Delete(); err != nil { - return err - } + err = list.DeleteBackgroundFileIfExists() + if err != nil { + return err } file := &files.File{}