Provide a UI for resolving sync conflicts

jgaehring commented 5 years ago

The use case for this feature would be when a user has updated a field(s) in a log client-side, at some time since the last successful sync, while during the same period a user has updated the same fields in the same log from the server. We want to present the user with the option to choose the client or server revision over the other, and possibly provide the option to edit as well. This might be in a modal, or a special screen for sync conflicts, or perhaps a modified version of the Edit Log screen.

One issue I'm having with this is how exactly to inject this behavior into the existing sync procedure. As of now, I think the best way is to let the local changes override the server changes, which is how it's written currently, but to persist the server differences (specificaly, only the fields which differ from the local fields) in their own separate chunk of state in the Vuex store. Then when that state is present, provide the user with the option to open the special view for resolving the conflict, at which point we display the two versions side by side, with the option to select one or the other, which then updates the log in the store.

@alexadamsmith , I've got some questions for you regarding how you've implemented the sync procedure:

Is there a reason you chose to persist the last sync time rather than just comparing against the server logs .changed timestamp? (feel like you might have already explained this to me; apologies if I'm making you repeat yourself)
- I'm a little confused why you're using both a locLogBuilder and a servLogBuilder objects to construct updateParams.log; it seems a little redundant to me when the original objects are still in scope and could be used instead, or is there something I'm missing?

I think I'm going to punt on implementing this feature for today, while I work on #187 et al. I know you're busy, but if you get a chance tonight to answer those Q's I'd be deeply appreciative. This is going to be a tricky feature to implement I'm realizing, very involved on the UI side as well as the data side; might push me past my deadline of this Friday. :/

alexadamsmith commented 5 years ago

I'll be able to write and explain when I get in front of my computer, in about an hour

On Thu, Apr 11, 2019, 11:02 AM Jamie Gaehring notifications@github.com wrote:

The use case for this feature would be when a user has updated a field(s) in a log client-side, at some time since the last successful sync, while during the same period a user has updated the same fields in the same log from the server. We want to present the user with the option to choose the client or server revision over the other, and possibly provide the option to edit as well. This might be in a modal, or a special screen for sync conflicts, or perhaps a modified version of the Edit Log screen.

One issue I'm having with this is how exactly to inject this behavior into the existing sync procedure. As of now, I think the best way is to let the local changes override the server changes, which is how it's written currently, but to persist the server differences (specificaly, only the fields which differ from the local fields) in their own separate chunk of state in the Vuex store. Then when that state is present, provide the user with the option to open the special view for resolving the conflict, at which point we display the two versions side by side, with the option to select one or the other, which then updates the log in the store.

@alexadamsmith https://github.com/alexadamsmith , I've got some questions for you regarding how you've implemented the sync procedure:

Is there a reason you chose to persist the last sync time rather than just comparing against the server logs .changed timestamp? (feel like you might have already explained this to me; apologies if I'm making you repeat yourself)

I'm a little confused why you're using both a locLogBuilder and a servLogBuilder objects to construct updateParams.log; it seems a little redundant to me when the original objects are still in scope and could be used instead, or is there something I'm missing?

I think I'm going to punt on implementing this feature for today, while I work on #187 https://github.com/farmOS/farmOS-client/issues/187 et al. I know you're busy, but if you get a chance tonight to answer those Q's I'd be deeply appreciative. This is going to be a tricky feature to implement I'm realizing, very involved on the UI side as well as the data side; might push me past my deadline of this Friday. :/

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/farmOS/farmOS-client/issues/190, or mute the thread https://github.com/notifications/unsubscribe-auth/ARAC8LPMakFI6M7i1yCrQwNO5dyarIclks5vf05-gaJpZM4cp1Nb .

alexadamsmith commented 5 years ago

It's important to save the date of last sync (in localstorage as syncDate) because it is used to see if a log on the server has been changed since the last sync. See lines 175 and 176 of http/module We only want to over-write logs in the app store and database with those from the server if they have been changed on the server (and not on the app). Otherwise, we over-write nearly all logs every time we sync, creating many unnecessary store and database operations.

locLogBuilder and servLogBuilder are necessary because, in cases where a log has been changed on the server AND on the app, we want the option to retain changes that have been made on the app. If we are retaining local changes, we need to to construct a composite log that consists of attributes from the server who's changed date is < the syncDate, plus attributes from the local store who's changed date is > the sync date.

Looking at the code again, we probably don't need two builder objects; I think a single logBuilder would work fine.

Did you try out the suggestion on lines 245 - 260? I tested it out back when I was building this, and it seemed to work.

Gotta go now, but I might be able to talk tomorrow before 8:00 if that would be helpful.

On Thu, Apr 11, 2019 at 5:15 PM Alex Smith alexadamsmith@gmail.com wrote:

I'll be able to write and explain when I get in front of my computer, in about an hour

On Thu, Apr 11, 2019, 11:02 AM Jamie Gaehring notifications@github.com wrote:

The use case for this feature would be when a user has updated a field(s) in a log client-side, at some time since the last successful sync, while during the same period a user has updated the same fields in the same log from the server. We want to present the user with the option to choose the client or server revision over the other, and possibly provide the option to edit as well. This might be in a modal, or a special screen for sync conflicts, or perhaps a modified version of the Edit Log screen.

One issue I'm having with this is how exactly to inject this behavior into the existing sync procedure. As of now, I think the best way is to let the local changes override the server changes, which is how it's written currently, but to persist the server differences (specificaly, only the fields which differ from the local fields) in their own separate chunk of state in the Vuex store. Then when that state is present, provide the user with the option to open the special view for resolving the conflict, at which point we display the two versions side by side, with the option to select one or the other, which then updates the log in the store.

@alexadamsmith https://github.com/alexadamsmith , I've got some questions for you regarding how you've implemented the sync procedure:

Is there a reason you chose to persist the last sync time rather than just comparing against the server logs .changed timestamp? (feel like you might have already explained this to me; apologies if I'm making you repeat yourself)

I'm a little confused why you're using both a locLogBuilder and a servLogBuilder objects to construct updateParams.log; it seems a little redundant to me when the original objects are still in scope and could be used instead, or is there something I'm missing?

I think I'm going to punt on implementing this feature for today, while I work on #187 https://github.com/farmOS/farmOS-client/issues/187 et al. I know you're busy, but if you get a chance tonight to answer those Q's I'd be deeply appreciative. This is going to be a tricky feature to implement I'm realizing, very involved on the UI side as well as the data side; might push me past my deadline of this Friday. :/

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/farmOS/farmOS-client/issues/190, or mute the thread https://github.com/notifications/unsubscribe-auth/ARAC8LPMakFI6M7i1yCrQwNO5dyarIclks5vf05-gaJpZM4cp1Nb .

jgaehring commented 5 years ago

Otherwise, we over-write nearly all logs every time we sync, creating many unnecessary store and database operations.

Ohhhh, gotcha!

If we are retaining local changes, we need to to construct a composite log that consists of attributes from the server who's changed date is < the syncDate, plus attributes from the local store who's changed date is > the sync date. Looking at the code again, we probably don't need two builder objects; I think a single logBuilder would work fine.

Yea, I think I might refactor a little differently, with two sets of log properties: 1) the newest properties from the server which don't have any conflicts with the local copy, which we spread over the current log and then pass that as a payload to updateLogFromServer, and 2) a set of only the properties from the server which do conflict with the local log, which we pass into a special bit of state called mergeConflicts.logs or something like that.

Did you try out the suggestion on lines 245 - 260? I tested it out back when I was building this, and it seemed to work.

I haven't tried it yet. I'm guessing that I'll need to do something a little different, which will be sort of a combination of those two solutions, in order to propagate the conflict (as well as the decision of how to reconcile it) to the user.

Thanks for taking the time explain everything, @alexadamsmith , this was very helpful!

ludwa6 commented 5 years ago

n00b user here, having just got started with the app on iOS this afternoon, i created just a few log entries, one of which threw a 406 "failed to sync" error, and created a duplicate record -which is not an exact duplicate, as i'm seeing 3 diffs in the content:

log Category is changed to "animals" , vs what i had entered ("planting")
text of what i put in Note field is missing; and
a notation of "false" appears besides what i had put for Quantity (i.e. count = 120).

Now every time i sync, it creates another exact duplicate of the first dupe.... So only way i can see to fix this is: delete all the dupes AND the original in both iOS AND web view, and start over.

ps: i think this may have something to do with this error msg i see in the edit form (web view) on the original record:

Planting *
--
No items have been added yet. Click "Add items" to launch the widget.

Silly me (bad n00b!)... But still: i'd call the fact that this can happen a bug in need of squashing.

jgaehring commented 5 years ago

@ludwa6 Thanks for spotting this!

It may be useful to open up a separate issue for this bug, since it doesn't exactly have to do with how the UI could be redesigned to merge conflicts between the local device and the server, which is an enhancement we're working on (marking it as such now). Rather, this seems to be a bug where the server is responding that one of the logs is "not acceptable". I'll open that new issue thread and leave a few more comments on what that could mean.

jgaehring commented 4 years ago

In the process of working on #296, which involves a rewrite of makeLog (renamed farmLog for now), I'm thinking it might be the right time to at least add the underlying data structure for recording the merge conflicts, even if we're not ready to provide a UI for reconciling them yet.

This is the kind of structure I have in mind:

const log = {
  name: {
    changed: 1580497477,
    data: 'Harvest corn',
    conflicts: [
      {
        changed: 1580497477
        data: 'Harvest dent corn'
      }
    ],
  },
  type: {
    changed: 1580497477,
    type: 'farm_harvest',
    conflicts: [],
  },
  // etc, etc...
};

This way we'll be able to present the user with complete information about the conflict, or at least as good information as we have, namely the time of the remote change and its value.

A few assumptions:

The main value (ie, not the one listed under conflicts) will always revert to the local value, which is consistent with our merging strategy to date; in this respect we're only adding data to our existing model, not changing it otherwise.
There is only one source for conflicts, namely the server; since we can't do anything p2p yet, and there shouldn't be more than one server, this seems like a safe assumption; if this changes in the future, perhaps we'll want to add some sort of identifier for the source of the conflict.

Again, I'm not planning to do anything in the UI for the moment, so these conflicts will just go ignored, but that shouldn't present any problems, since they just represent additional metadata and won't disrupt how we otherwise access or modify logs.

I am, however, having second thoughts about this:

We only want to over-write logs in the app store and database with those from the server if they have been changed on the server (and not on the app). Otherwise, we over-write nearly all logs every time we sync, creating many unnecessary store and database operations.

I'm wondering, @alexadamsmith, if its worth the potential performance hit to simplify this process a little by not requiring the syncDate parameter when comparing the local log against the server log. As it is right now, I'm moving the main log-merging logic from the processLog() function in sync.js to the logFromServer() function in farmLog.js. Perhaps we could use the syncDate data differently, perhaps filtering before passing the local and server log into logFromServer(). I'm thinking syncDate could also be useful for implementing a sort of server "pulse" like we discussed in https://github.com/farmOS/farmOS-client/issues/287#issuecomment-573253901. Anyways, those are all ideas, I'll try to update here as it all solidifies.

jgaehring commented 4 years ago

Perhaps we could use the syncDate data differently, perhaps filtering before passing the local and server log into logFromServer().

Thinking about this more, I think this is a real possibility, and perhaps in fact the best way to go. Moving my comments over to https://github.com/farmOS/farmOS-client/issues/296#issuecomment-580976276, though, since it's more relevant to that issue.

jgaehring commented 3 years ago

See farmOS/farmOS.js#27.

jgaehring commented 3 years ago

Just a thought, but this will probably share a lot of implementation details with #459. Might be a good idea to hold off on both til the beta milestone.

jgaehring commented 3 years ago

Moving this to the beta milestone, since it should be part of core.

farmOS / field-kit

Provide a UI for resolving sync conflicts #190