Skip to content

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