fix(background): when list background is removed, delete file from file system and DB #1372
|
@ -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
|
||||
|
|
|
@ -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()
|
||||
}
|
||||
|
||||
|
|
|
@ -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{
|
||||
|
|
|
@ -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,6 +247,7 @@ func TestList_CreateOrUpdate(t *testing.T) {
|
|||
}
|
||||
|
||||
func TestList_Delete(t *testing.T) {
|
||||
t.Run("normal", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
list := List{
|
||||
|
@ -258,6 +260,64 @@ func TestList_Delete(t *testing.T) {
|
|||
db.AssertMissing(t, "lists", map[string]interface{}{
|
||||
"id": 1,
|
||||
})
|
||||
})
|
||||
t.Run("with background", func(t *testing.T) {
|
||||
konrad marked this conversation as resolved
Outdated
konrad
commented
Can you add a test where you remove a list with a background and then assert the file was removed? (check the fixtures for a list with a background) Can you add a test where you remove a list with a background and then assert the file was removed?
(check [the fixtures](https://kolaente.dev/vikunja/api/src/branch/main/pkg/db/fixtures/lists.yml) for a list with a background)
|
||||
db.LoadAndAssertFixtures(t)
|
||||
files.InitTestFileFixtures(t)
|
||||
s := db.NewSession()
|
||||
list := List{
|
||||
konrad
commented
Here you should not load the full list, instead create the list object like in the test before:
That way it's the same as when the web handler calls the delete method. Here you should not load the full list, instead create the list object like in the test before:
```
list := List{
ID: 25,
}
```
That way it's the same as when the web handler calls the delete method.
|
||||
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)
|
||||
})
|
||||
}
|
||||
|
||||
func TestList_ReadAll(t *testing.T) {
|
||||
|
|
|
@ -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 = ""
|
||||
|
|
|
@ -54,12 +54,10 @@ 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 {
|
||||
err = list.DeleteBackgroundFileIfExists()
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
||||
file := &files.File{}
|
||||
file.ID, err = strconv.ParseInt(img.ID, 10, 64)
|
||||
|
|
Loading…
Reference in New Issue
Can you move this code block to after when the list itself was deleted? This could otherwise lead to weird errors where the file is gone, but the list isn't (when deleting the list fails but deleting the background succeeded)
I had it that way initially, but then
GetListSimpleByID
fails to load the List because it has been deleted.I guess alternatively we could load the
fullList
here withGetListSimpleByID
, remove the call toGetListSimpleByID
fromDeleteBackgroundFileIfExists
, then down below we callfullList.DeleteBackgroundFileIfExists
?That makes a lot of sense. I Must have missed that.
I think that's the way to go. Can you check if this is required in the other place where
DeleteBackgroundFileIfExists
is called as well?The other places were working as expected per my previous testings, but I just verified in code that ultimately they have the full list loaded by that point.
The two other places are:
/lists/{id}/backgrounds/upload [put]
: already had the file deletion code, but the full list is coming from/lists/{id}/background [delete]