Skip to content
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

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

Closed
individual-it opened this issue Aug 9, 2019 · 20 comments · Fixed by #7635
Assignees
Labels
Estimation:M(3) feature:files Priority:p2-high Escalation, on top of current planning, release blocker Type:Bug Something isn't working

Comments

@individual-it
Copy link
Member

Steps to reproduce

  1. create a file in a folder
  2. delete the file
  3. rename the folder
  4. 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

@individual-it individual-it added the Type:Bug Something isn't working label Aug 9, 2019
@DeepDiver1975 DeepDiver1975 added the Priority:p2-high Escalation, on top of current planning, release blocker label Aug 16, 2019
@DeepDiver1975 DeepDiver1975 self-assigned this Aug 16, 2019
@LukasHirt LukasHirt added this to the backlog milestone Aug 23, 2019
@PVince81 PVince81 modified the milestones: backlog, Milestone 1: Phoenix for users Sep 24, 2019
@PVince81
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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 #1730

@individual-it
Copy link
Member Author

assigned @jasson99 and put into sprint for creating tests

@haribhandari07
Copy link
Contributor

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

@individual-it
Copy link
Member Author

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

@pascalwengerter
Copy link
Contributor

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

@kulmann
Copy link
Member

kulmann commented Mar 8, 2022

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

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
Copy link
Contributor

tbsbdr commented Mar 8, 2022

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
Copy link
Contributor

tbsbdr commented Mar 15, 2022

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
Copy link
Contributor

lookacat commented Apr 4, 2022

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

@kulmann
Copy link
Member

kulmann commented Apr 4, 2022

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
Copy link
Contributor

pmaier1 commented Apr 4, 2022

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
Copy link
Member

kulmann commented Apr 4, 2022

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
Copy link
Member Author

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 lookacat self-assigned this May 9, 2022
@lookacat
Copy link
Contributor

lookacat commented May 11, 2022

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

  1. If you have a file in a subfolder and you delete this file first and then the parent folder
  2. Restore this file (Folder structure gets recreated)
  3. 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
Copy link
Contributor

JammingBen commented May 11, 2022

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
Copy link
Contributor

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

@JammingBen
Copy link
Contributor

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 #1753 (comment) -> do you agree with b) as the preferred solution?

@kulmann
Copy link
Member

kulmann commented Jun 14, 2022

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Estimation:M(3) feature:files Priority:p2-high Escalation, on top of current planning, release blocker Type:Bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.