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

clean up deleted files #762

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tarnung
Copy link
Collaborator

@tarnung tarnung commented Dec 6, 2021

adressing #602

Only tested with the dropbox backend.

There was a bug in the error handling in the dropbox backend implementation (trying to JSON.parse an already deserialized error object).

I'm not sure if there are cases where the backend call fails for other reasons that would also trigger this cleanup behaviour. The case i triggered with the dropbox backend explicitly checks for the error code "not_found" which seems reasonable.

I placed the trigger for the clean up logic in the live sync middleware. That's questionable but it's the place where we save to localstorage whether live sync is activated or not.

@munen
Copy link
Collaborator

munen commented Dec 9, 2021

@tarnung Sweet! That is a nice addition 🙏

I have tested two scenarios:

Scenario 1

  1. Create a new file with content outside of organice
  2. Edit it
  3. Delete it outside of organice
  4. Try to edit the file, again, in organice

This works perfectly. organice now doesn't try to write to a non-existent file anymore and properly shows an error message!

Scenario 2

  1. Create a new file with content outside of organice
  2. Add it to the file settings (with the option "Sync on startup" set)
  3. Edit it
  4. Delete it outside of organice
  5. Try to edit the file, again, in organice

This works well enough. I get a proper error as in "scenario 1". The fileSetting, however, stays in place. Arguably this is not wrong. Deleting a file outside of organice doesn't mean that related config in organice has to be automatically deleted.

Personally, I would merge. However, I saw in the code that you're trying to delete the fileSetting. We could delete the code that tries to delete the fileSetting or we could debug why the fileSetting is not deleted. Either is fine with me. But I didn't want to merge without getting your input.

@tarnung
Copy link
Collaborator Author

tarnung commented Dec 9, 2021

Thanks for testing. I will look at it again to check why the file setting survives the clean up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants