@feederbox826 , @DogmaDragon

Would one, or both, of you be willing to pull this down and test? I dont currently have any files on my dev system to move this around with. Sorry if I nag y'all too much for testing but I prefer to test with the regular userbase so it's easier for WP to merge comfortably lol.

I think the delete operation should fail if it fails to move the file to trash. If the user has set the trash directory, they would expect files to be moved there instead of being deleted. Deleting the file when it failed to move goes against this expectation and could potentially result in data loss.

I'm also more inclined to move the file into the trash location during the renameForDelete part in Deleter. This provides an immediate failure point if the file fails to be moved to the new location, and doesn't require any further operations during the Commit call.

@WithoutPants I anticipated you would want me to remove that so luckily I had most of the work done before I clock out for the night.

I removed the fallback. Now, the file should immediately get moved to Trash via renameForDelete. If it fails the tracked path is restored here trashedPaths["/some/path/here.mp4"]. If it is canceled for any reason, like an invalid location, then rollback kicks in to restore the file.

Would you like a machine to test on?

Would you like a machine to test on?

I have a machine just I moved all of my Dev work to my Mac laptop. Currently I don't have any files on it to test with.

I'll go ahead and set something up on my main server so I can test.

I might have... Broken it... A lot 👉👈

After trying to fail the trash, I can't get it to persist the deleteTrashPath, setting it in config.yml removes it it when its' started and the GQL goes through, but the response doesn't contain deleteTrashPath. The UI reflects it but it's not persisted or actually used

Okay, will look at it tomorrow after work.

Thanks for the test 🤙

So, I think this is an issue with the Mutation and query configuration resolvers. I forgot to add them there so it will never persists....... dur
Will try and get a new commit up here in the next little while.

So, these latest updates fix the issues that @feederbox826 was having.

• The trash path now persists.
• The response contains the DeleteTrashPath

Other Tests:

• Tested an "invalid path" and received an error stating that it was an invalid path to prevent user confusion.
• Tested a RO folder and received an error stating that it was a RO folder to prevent user confusion.

I also fixed the UI in the delete dialouge box. If the trash path is enabled then the warning now reflects the files will be moved to trash rather than permanently deleted.

Was able to test this by using /Users/gykes/.Trash as the trash path for mac and it worked accordingly.

I would love if others could test this on other operating systems and give feedback. I tried to break it but I can only do so much with my system.

My tests, for posterity

assert fail

• RW lib to RO trash
  • invalid cross-device link
• RO lib to RW trash
  • invalid cross-device link
• RO lib to RO trash
• RW lib to -x trash

assert success

• cross-device move
• subdirectory move
• path > 256ch
• nonexistent trash
  • created automatically
• windows $RECYCLE_BIN
  • access denied
• appends date-time if existing file

We need to be very careful about this and cover all our bases, because a mistake in this can lead to data loss. Case in point: I just deleted an image using this and it did not use the trash feature 😬

Having had a quick look around, it looks like only the scene mutators are using the trash feature. This needs to be added to the image, gallery and file mutation resolvers, along with the clean task. Basically anywhere the file.NewDeleter function is called.

Only complaint would be that since it uses SafeMove, it fails with error "invalid cross-device link" when instead it should be "access denied" in the first error.

Might be worth having a custom message intead of the generic one

scenesDestroy: input: scenesDestroy marking file "/library/062212-055.mp4" for deletion: failed to move to trash: copying file during SaveMove failed with: 'open /trash/062212-055.mp4: permission denied'; renaming file failed previously with: 'rename /library/062212-055.mp4 /trash/062212-055.mp4: permission denied'

I think that, in fsutil.SafeMove, if the rename operation fails with os.ErrPermission we should probably just fail then and there.

We need to be very careful about this and cover all our bases, because a mistake in this can lead to data loss. Case in point: I just deleted an image using this and it did not use the trash feature 😬

Having had a quick look around, it looks like only the scene mutators are using the trash feature. This needs to be added to the image, gallery and file mutation resolvers, along with the clean task. Basically anywhere the file.NewDeleter function is called.

Thats actually a very good point and I didn't even think to extend it to images or anything like that.

Good catch, will update tomorrow to include images and will look at groups as well (I think that calls scene mutations but will verify)

We need to be very careful about this and cover all our bases, because a mistake in this can lead to data loss. Case in point: I just deleted an image using this and it did not use the trash feature 😬

Having had a quick look around, it looks like only the scene mutators are using the trash feature. This needs to be added to the image, gallery and file mutation resolvers, along with the clean task. Basically anywhere the file.NewDeleter function is called.

Added in the support for this. Added for all scene, image,gallery, file, and clean task operations. Currently untested but will test when I get home unless our lord and tester FB wants to do it.

Having had another look through, I think there's another issue here that requires a bit more work. My expectation for this feature is that it would only apply to media that you've deleted, and shouldn't include things like generated files. It looks to me like this will use the trash directory for everything that gets deleted in a transaction, which I don't think is what we want.

Are we sure about that? I know I have previews generated for all my content and would be a pain to regenerate it all vs just returning them all to their appropriate location and calling it a day. I figured the trash support was a failsafe and, in my opinioin, extend to generated content as well.

From memory alone I believe the FileDeleter calls the Generated deleter as well. To go the route you want I think I would need to build a separate deleter that doesn't utilize Trash and then filter Generate delete through that

Hm but the generated files have a "random" filename, right? Very hard to tell which one goes with which actual file. And finding the right directory to restore to is also difficult, particularly since this implementation does not store that metadata.

Yea, fair. Will look at removing generated content from it then.

To go the route you want I think I would need to build a separate deleter that doesn't utilize Trash and then filter Generate delete through that

I don't think you need to go that far, I think you just went a bit overboard when adding all the types to trash handler