farmOS / field-kit

A modular, offline-first companion app to farmOS.
https://farmOS.org
GNU General Public License v3.0
60 stars 39 forks source link

Logs do not re-sync after being deleted locally #349

Closed mstenta closed 4 years ago

mstenta commented 4 years ago

I forget if we have an issue open for this already or not...

I want to be able to "discard any changes I've made to a log in Field Kit" (on a log that I pulled in from the server.

It seems like the only way to do that is to delete the log in field kit, but then it doesn't get resynced from the server, until I log out and log back in.

jgaehring commented 4 years ago

Hm, lots of thoughts here.

It seems like the only way to do that is to delete the log in field kit, but then it doesn't get resynced from the server, until I log out and log back in.

I think there is a separate issue embedded in this, which we may want to track separately. That is, if someone deletes a log from their phone, but that log still meets the criteria to be synced from farmOS (eg, for My Logs, it is marked done: false and you are the log owner), then it should be resynced the next time you make a sync request. That's not happening now, but imo, it should, and it's a bug. See also #285 and #284, which are not quite the same, but related.

As to your original request...

I want to be able to "discard any changes I've made to a log in Field Kit" (on a log that I pulled in from the server.

I'm a little uneasy about how unbounded this criteria is: "discard any changes I've made to a log in Field Kit". That could mean a lot of different things. Ultimately, I think no matter how you parse it, it boils down to managing a log's version history. I'm tempted to label that wontfix, at least not without ample funding to match it, b/c that is a substantial task.

Sure, you may be thinking in terms our current implementation, in which syncing is still a manual task, and so there is in essence a fixed state, saved on the server, to which you can revert at any time up until the user hits sync. That's more or less trivial, and if we fix the "delete and resync" bug as I defined above, it's pretty much implemented. But that triviality is just an accident of our current implementation, and I don't want to enshrine that into some higher level UI component that promises to "Discard local changes", because I don't think we can maintain that feature as we move forward. Specifically, I think it is fundamentally at odds with #274, "Automatically sync logs whenever they change", and I think automatic syncing is a much higher priority than managing version history. Here's why...

We've acknowledged throughout development that Field Kit is ephemeral and farmOS is canonical. It's informed much of our design choices to-date, and I think it's the path we need to stay on for now. If we're taking that approach, we owe it to users to take every measure to reduce the risk of data loss, which I think means automating as much of the syncing process as possible and eliminating the possibility for user error. We always run the possibility that the user may forget to sync their data, then leave the app for a while, perhaps long enough for the browser's eviction criteria to come into effect, and they could lose that data.

I'm afraid if we enable "Discard local changes" now, it would be a feature we will only have to drop when we adopt atuomatic syncing later on. Otherwise it becomes incumbent on us to store a fallback point, or multiple points, in the log's version history, so the user can revert to that at any time. That introduces a lot of complexity that I don't think we're ready to tackle yet. And I don't want to introduce a feature to users, just to yank it away from their workflow later on.

In a truly local-first incarnation of farmOS/Field Kit, the conflict between automatic syncing and version history would totally go away, because we don't have to treat the local data as ephemeral. And it could be pretty simple, because CRDT's support version history and undo/redo right out of the box. Perhaps an intermediary step would be to enable the revision history on the server, so users could rollback changes that way.

mstenta commented 4 years ago

I think there is a separate issue embedded in this, which we may want to track separately. That is, if someone deletes a log from their phone, but that log still meets the criteria to be synced from farmOS (eg, for My Logs, it is marked done: false and you are the log owner), then it should be resynced the next time you make a sync request. That's not happening now, but imo, it should, and it's a bug.

Honestly, fixing this would accomplish the user story I described:

I want to be able to "discard any changes I've made to a log in Field Kit" (on a log that I pulled in from the server.

If deleting the log and re-syncing worked, that would work fine... at least for right now, given the way manual syncing works. No need for a dedicated feature around "discarding changes" more specifically.

I will change the title and labels of this issue to reflect that.

Great points regarding how this relates to other future plans, especially #274. I agree we should not implement a dedicated "discard changes" feature, with that in mind.

It does make me think more about #274, though...

Specifically: the reason this came up for me is that I was editing a log in Field Kit (at the same time that I discovered this bug: #348) and after I went back to "My Logs" I noticed that the date of my log changed (I don't remember changing it, but I must have while I was messing with the hour). This was a user mistake on my part. But if I wasn't specifically looking at the date I probably wouldn't have noticed it. If syncing is fully automated, that would have gone to the server.

I'm not saying that this is something we need to fix... it really might be an unavoidable problem (user makes a change accidentally to a log and it gets synced without them noticing)... and a dedicated "discard local changes" feature certainly wouldn't prevent that... just something that happened to me and so it made me think about these things...

jgaehring commented 4 years ago

I think this will be fixed if we get rid of the global syncDate that we're currently storing in localStorage for all logs, and instead use lastSync metadata on each log (a timestamp, which replaces wasPushedToServer, a bool). The syncDate is part of the determining criteria for how a local log is merged with a server log, but it's also used in a basic filter prior to merging, which was intended to reduce the amount of processing done to a log, but in hindsight seems misplaced:

https://github.com/farmOS/farmOS-client/blob/8a281f63d44e0006fe0e83d3d7c1b4a3adf326bd/src/core/store/http/module.js#L156-L162

I've already got a working implementation of farmLog with lastSync, which should help a lot of our issues. And I think instead of doing that filtering prior to merging, I can simply put a conditional around this block and only execute it if serverLog.changed > localLog[lastSync]:

https://github.com/farmOS/farmOS-client/blob/7459f27c195e3d1f8ab475136c4f6aa68c84ab41/src/utils/farmLog2.js#L219-L228

That will save us any unnecessary iterations but will not discard logs that were deleted locally and haven't changed on the server since the last sync event.

jgaehring commented 4 years ago

This is resolved with the new implementation of farmLog (#358), as explained in the last comment.