restoring a file from "Deleted files" (trashbin) is not possible if the original folder does not exist any-more

individual-it commented 5 years ago

Steps to reproduce

create a file in a folder
delete the file
rename the folder
try to restore the file

Expected behaviour

file should be restored

Actual behaviour

file is not restored. Error: Restoration of file failed The destination node is not found The user has no idea from where the file came from. see #1525 So its very hard to restore the file

PVince81 commented 4 years ago

I'd expect the trashbin API to know where the original location is as it's stored in the database.

The behavior in OC 10 was that if the target folder does not exist, we fall back to putting the restored item into the user's home root.

PVince81 commented 4 years ago

Maybe in Phoenix we can detect this situation, this is likely a 409 if the folder does not exist, so after that fall back to a MOVE to the root

PVince81 commented 4 years ago

in case we rely on the error code, need to make sure it's not happening also if the target destination file already exists like in https://github.com/owncloud/phoenix/issues/1730

individual-it commented 4 years ago

assigned @jasson99 and put into sprint for creating tests

haribhandari07 commented 4 years ago

@individual-it since tests have been added should we mark this issue as done and remove from current sprint?

individual-it commented 4 years ago

@HariBhandari07 the issue is not fixed, only tests were added, so I took off @jasson99 assignment, but it should stay on the sprint

pascalwengerter commented 2 years ago

Can confirm this still happens on https://ocis.ocis-web.latest.owncloud.works

kulmann commented 2 years ago

The trash (at least for oc10) has no parent file id, only an original location of the deleted file. We have no way of finding out where to restore the file/folder to if the original location doesn't exist anymore.

For a renamed original location, I'd expect restoring to the now renamed original location. We cannot do that as the information doesn't exist.
For a deleted original location, I'd expect restoring to fail with a fallback to be chosen by the user.

@tbsbdr what do you think about only informing the user that the original location doesn't exist anymore and then

a) restore to the users personal home (could require a confirmation upfront) as described by @PVince81 in https://github.com/owncloud/web/issues/1753#issuecomment-535862744
b) utilize the LocationPicker that we also have for copy and move operations for letting the user choose an alternative location.

If we choose option b, then we could also introduce an additional action restore to where the user can immediately choose a different restore location upfront.

tbsbdr commented 2 years ago

I would choose a) as it seems simpler (also implementation wise?) and does not require an immediate userchoice which always comes with some cognitive costs. Just my first thoughts

tbsbdr commented 2 years ago

Proposal

@kulmann we also need to think of restoring files in spaces - imo the usecase "original path vanished" could be not soooo cornerish in Spaces any more, as more hands will touch folders in Spaces.

I would still opt for option a) -> restore to user personal home.

In addition: If the file was deleted from a Space, it should be restored to the Space root.
Proposal: Since you are no longer alone in a Spaces, there is a risk that Spaces will become "messy", just like any good shared apartment kitchen. Therefore we could store restored files in a ..Spaces/Marketing/Restored-from-trashbin folder. Advantage:
1. Communication: Folder title communicates to other, not involved Space members where those files came from. Prevents confusion.
2. Tidy up: Space root is less unlikely to get messy

Also if I think of the user manual, I think its a bit more easy to comprehend written doc like: "If path changed: find all restored files in Restored-from-trashbin in personal/Sapcexy" rather than "find restored files in the root of personal/Spacexy"

@kulmann what do you think, also regarding Spaces?

Note - behaviour on Windows is:

Files get always restored to the path, where they where originally deleted from (even if the original path is deleted or renamed). That means, that a "restore" on windows could re-create the complete, original path even if it didn't exist any more.

lookacat commented 2 years ago

Would also prefer the windows method. @kulmann should I have a look if thats possible?

kulmann commented 2 years ago

Would also prefer the windows method. @kulmann should I have a look if thats possible?

Not yet. I'd like to hear @pmaier1's opinion on this first.

From a technical perspective we can only restore rebuilding the original path, because we don't have any parent folder file id via the API. The question at this time is if we want the windows behaviour (then you could implement it) or if we don't want it (then this ticket needs to remain on hold).

pmaier1 commented 2 years ago

Not yet. I'd like to hear @pmaier1's opinion on this first.

Intuitively I also prefer Windows behavior. Dropbox also follows this pattern. They even restore related parent folders in bulk (if those are still in the trash bin).

Interestingly, Box went the location picker way Screenshot from 2022-04-04 16-11-55

That certainly gives the user more control but is maybe more complex. If everyone agrees, I'd go for the simple Windows-based behavior and keep the location picker in mind as a potential improvement for later.

I think this works better than (arbitrarily) restoring to root. What do you think?

kulmann commented 2 years ago

If everyone agrees, I'd go for the simple Windows-based behavior and keep the location picker in mind as a potential improvement for later.

Fine by me. Let's do it this way 👍

individual-it commented 2 years ago

Just think of the corner case that you might not have the permissions anymore to recreate the original path

Alice creates "simple-folder/subfolder/lorem.txt" Alice shares "simple-folder" with "Brian" as editor Brian accepts share Brian deletes Shares/simple-folder/subfolder/lorem.txt Alice changes the permissions on the share to Brian to read-only Brian restores lorem.txt from the trashbin

Or there might be the case that the share is gone all-together, or its even a totally different Share from an other user with the same name

lookacat commented 2 years ago

args, restoring the folder structure isn't a problem but one case is really difficult.

If you have a file in a subfolder and you delete this file first and then the parent folder
Restore this file (Folder structure gets recreated)
Restore the deleted folder (All files are restored but the file restored previously because the folder get overwritten) I'm not sure if we have the possibility to do this via the client.

JammingBen commented 2 years ago

Hmm, I think it requires a product decision first. What should happen when you restore a folder with a name that already exists in your personal files?

a) Overwrite the folder b) Restore the folder as "new resource" e.g. by appending a suffix like "Folder (1)".

Currently we do a), which is No-Go IMO as folders can get overwritten easily. The overwritten folder then get's moved to the trash bin.

lookacat commented 2 years ago

Thx for the input, I would go for the suffix, i think that is how windows handles it as well

JammingBen commented 2 years ago

I also prefer this option, oC10 does it like this as well. But I'm not sure how easy it is to implement. Worst case scenario would be that we also need adjustments in backend and/or SDK.

@tbsbdr @pmaier1 see https://github.com/owncloud/web/issues/1753#issuecomment-1123762599 -> do you agree with b) as the preferred solution?

kulmann commented 2 years ago

Since we now have the resolve conflict dialog where the user gets prompted to choose one out of skip, overwrite and keep both we could use exactly the same pattern here as well. What do you think @lookacat @JammingBen ?

owncloud / web