-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Small improve on deleting attachements #3145
Small improve on deleting attachements #3145
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3145 +/- ##
========================================
- Coverage 34.9% 34.7% -0.2%
========================================
Files 277 277
Lines 40108 40116 +8
========================================
- Hits 13998 13922 -76
- Misses 24061 24159 +98
+ Partials 2049 2035 -14
Continue to review full report at Codecov.
|
LGTM |
models/attachment.go
Outdated
return 0, nil | ||
} | ||
|
||
var ids = make([]int64, 0, len(attachments)) | ||
for i, a := range attachments { | ||
if remove { | ||
if err := os.Remove(a.LocalPath()); err != nil { | ||
return i, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, there are some attachments which we have already deleted from the filesystem. Should we also remove them from the DB, so that the DB doesn't contain non-existent attachments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
additional check could be added to check if error is about nonexistent file but if error is that file can not be deleted (access denied etc) than it should stay as it is now
ae1f337
to
2942b7a
Compare
@lafriks @ethantkoenig done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the change.
Could we please add a unit test for DeleteAttachments
?
@ethantkoenig the tests have existed in |
LGTM |
As title.