Open a post with a remote auto-save, app must show a dialog asking to restore it

maxme commented 5 years ago

Description

When a user opens a published post with a remote auto-save, we must show a dialog “A more recent revision of this post exists. Restore?”. Like in Calypso:

Decision

we are splitting this from the discussion on conflict resolution.

Implementation logic: hasLocalChanges == true -> ignore autosave and just open the post hasLocalChanges == false && post.autosave == null -> just open the post hasLocalChanges == false && post.autosave != null -> open the post and show the dialog. If the user clicks on restore, load the autosave revision and set hasLocalChanges to true.

The service to query the latest autosave for a post is:
GET https://public-api.wordpress.com/rest/v1.1/sites/$site_id/posts/$post_id/autosave

Corresponding iOS Ticket

https://github.com/wordpress-mobile/WordPress-iOS/issues/11650

maxme commented 4 years ago

Now that https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/1255 is merged, we can start working on this one.

shiki commented 4 years ago

Subscribing @wordpress-mobile/ravenclaw

malinajirka commented 4 years ago

I've created a table with all the possible scenarios which we can encounter when we fetch a post from the server.

Post updated in Remote -> PostModel.lastModified has changed

Post's AutoSave object updated in Remote -> meta.data.autosave.modified has changed

hasLocalChanges -> flag indicating that the post was modified locally, but the changes were not synced with the server at all or were just remote-auto-saved.

Green (1c, 2c, 1d)
- We don't need to do anything as the user is about to edit (1c, 2c) or edited (1d) the most up-to-date version of the post.
Pink (2d)
- We can use the existing "Conflict resolution" dialog. However, I think this case will almost never happen -> the AutoSave object will be always modified when the post is edited in remote (see 3d and 4d).
Yellow (3c, 4c)
- We can update the post in the local db and show the "unpublished revision" dialog.
3d (This case is a bit more complicated)
- 3d-1. The user edited the post in the app and remote-auto-save was invoked -> the user went to Calypso and continued working on the autosaved version.
- 3d-2. The user edited the post in the app and remote-auto-save was invoked -> the user went to Calypso, but worked on the original (not autosaved version). The previous (app's) remote-auto-saved version was overridden.
- 3d-3. The user edited the post in the app but remote-auto-save was not invoked -> the user went to Calypso and worked on the original version.
- => 3d-1 and 3d-2 are fine, as Calpyso displayed the “unpublished revision” dialog and the user made their choice. We can’t currently differentiate 3d-3 from 3d-1 + 3d-2. Could we perhaps simply set "hasLocalChanges" to false when we perform successful remote-auto-save (downside is the remote-auto-save stores just "title, content and excerpt")? The 3d-1 and 3d-2 would become 3c and we could simply show the "unpublished revision" dialog. We said 3d-3 is an edge case which will probably never happen -> does it mean we'll simply discard local changes without user's explicit confirmation and show the remote post or we open our local version of the post and risk overriding Calypso's auto-save revision? I think an ideal solution would be to create a local revision from the local version + show the remote version of the post.
4d
- It’s very similar to 3d but more tricky from the UX point of view, as we should probably show both "conflict resolution" and "unpublished revision" dialogs. Let's keep it out of this discussions for now, until we resolve 3d.

I think it's quite complicated even if we want to keep things simple for the first iteration. I don't think we can just ignore its complexity and implement a simple solution without fully understanding impacts of our decisions. I also feel like it might be worth having a synchronous discussion about it. Wdyt?

malinajirka commented 4 years ago

I wish "GitHub" had an undo action 😞 . I posted it before it was finished.

I think the most straightforward solution would be to create a local revision when we detect 2d,3d,4d and just accept the data from the server (2d,3d,4d would become 2c,3c,4c). We would never need to show the "conflict resolution" dialog and the user could always go to revision history and restore their post..... But I'm not sure whether we want to/can introduce local revisions in the first iteration.

diegoreymendez commented 4 years ago

@malinajirka - That's an awesome analysis. Nice work!

Considering it's a much wider discussion than what this issue initially proposes: would you mind posting it as a new discussion issue? (it should still reference this one)

shiki commented 4 years ago

Great work, @malinajirka! Do we have information on how often these conflicts happen?

For 3d-3, would it be possible to show a dialog like "There is a modified version on this device and on the server, which one would you like to edit?". I realize this probably requires us to have local revisions. 🤔

I might have missed this. Is it possible to compare a local modifiedDate and remote modifiedDate and use the one that is the latest? Do we have these fields? This could probably be an interim solution until we get the complex solution sorted out.

I don't think we can just ignore its complexity and implement a simple solution without fully understanding impacts of our decisions.

I agree.

maxme commented 4 years ago

I think an ideal solution would be to create a local revision from the local version + show the remote version of the post.

It seems like the best technical solution (needs work but should be straight forward) and the best UX solution (present the history of changes when the user needs to select a revision).

I'm also wondering if we could create a remote revision from the local revision when the network is back (I'm not sure we have an endpoint to do this).

malinajirka commented 4 years ago

Considering it's a much wider discussion than what this issue initially proposes: would you mind posting it as a new discussion issue? (it should still reference this one)

Sorry @diegoreymendez , I was afk on Friday and I think moving it now would just result in more chaos.

Do we have information on how often these conflicts happen?

I don't think so:(.

I'm also wondering if we could create a remote revision from the local revision when the network is back (I'm not sure we have an endpoint to do this).

AFAIK this endpoint isn't available - we have the /autosave endpoint, but it doesn't always create a new revision and has side-effects.

The remote revisions support just "title, content and excerpt", if the local revisions would do the same I believe we wouldn't even need them, right? We already store changes made while offline. We could do the following

Fetch post from remote 2a. Conflict is not detected -> update our local database with the post we just fetched from remote 2b. Conflict is detected -> create a remote revision from the post in our local database and update our local database with the post we just fetched from remote

It wouldn't work on self-hosted sites though, so if we can make it work there introducing local revisions makes total sense.

malinajirka commented 4 years ago

I might have missed this. Is it possible to compare a local modifiedDate and remote modifiedDate and use the one that is the latest? Do we have these fields? This could probably be an interim solution until we get the complex solution sorted out.

I don't think we use local modified date - we use just a boolean flag "hasLocalChanges". The client's time isn't reliable in my opinion and we could quite easily lose all user changes. Wdyt?

For 3d-3, would it be possible to show a dialog like "There is a modified version on this device and on the server, which one would you like to edit?". I realize this probably requires us to have local revisions.

We basically already do that for post data conflicts (https://github.com/wordpress-mobile/WordPress-Android/pull/8989). But it gets a bit more tricky with introduction of the autosave - because you can edit one of remote post/remote autosave/local post. I agree with Maxime It seems like the best technical solution (needs work but should be straight forward) and the best UX solution (present the history of changes when the user needs to select a revision)..

osullivanchris commented 4 years ago

Thanks for the great breakdown @malinajirka . I'm still trying to make sure I understand all the scenarios.

Relating to "present the history of changes when the user needs to select a revision". Would we present this list in a way that also allows the user to view those changes? The user might remember the content they wrote, more so than the date/device it was written on. It would be nice while in this flow to be able to go and view the different posts to make an informed decision.

I'd also just like to mention that on the iOS version of this task, the same discussion might apply https://github.com/wordpress-mobile/WordPress-iOS/issues/11650 Does all the discussion above apply to iOS as well? If so, should we get talking across both platforms so we capture the same complexity on both platforms? @diegoreymendez

diegoreymendez commented 4 years ago

Does all the discussion above apply to iOS as well?

@osullivanchris - That's one of the limitations of having these discussions in GitHub. I also think this discussion should be happening in an issue of it's own, as this discussion should be parent to this issue and not vice-versa.

In a way having the wider discussion here adds a bit of noise because regardless of what states we have to deal with, this issue (the one initially reported) has a pretty narrow and well defined scope (which means this issue should be closed once that dialog is implemented and shown, regardless of the wider discussion).

I'd like to encourage us to sum up the bigger discussion in a separate issue (thread) and let this issue have a more limited scope, as initially intended.

malinajirka commented 4 years ago

I've created a new discussion issue: https://github.com/wordpress-mobile/WordPress-Android/issues/10008

Please post all comments which aren't directly related to this issue ("restore more recent revision dialog) into the discussion issue and not here.

yaelirub commented 4 years ago

@malinajirka , should this ticket still have "needs discussion" label? Is it blocked by https://github.com/wordpress-mobile/WordPress-Android/issues/10008 ?

malinajirka commented 4 years ago

should this ticket still have "needs discussion" label?

I think we can remove the discussion label since we moved the discussion to #10008

Is it blocked by #10008 ?

Yes, it is blocked as the solution for #10008 will most probably affect the implementation of the dialog. Most specifically what will happen with local changes if the user clicks on "restore" - will they be stored in a local revision, thrown away or something else?

I'd suggest pausing this ticket until #10008 is resolved and hopefully the solution for this ticket will be much clearer by then.

diegoreymendez commented 4 years ago

I spent some time thinking about this, because I felt we could still do progress while avoiding to get too deep into the conflict resolution discussion.

Is there any objection with implementing this for when hasLocalChanges == false case and leaving the rest for another issue about conflict resolution?

I think that should be fairly safe.

diegoreymendez commented 4 years ago

Also I wanted to add some documentation to aid in implementing this issue, since I actually went through the effort of looking this up today.

The service to query the latest autosave for a post is:

GET https://public-api.wordpress.com/rest/v1.1/sites/$site_id/posts/$post_id/autosave

In iOS I think this needs to be implemented.

malinajirka commented 4 years ago

I spent some time thinking about this, because I felt we could still do progress while avoiding to get >too deep into the conflict resolution discussion. Is there any objection with implementing this for when hasLocalChanges == false case and leaving >the rest for another issue about conflict resolution?

Good point, I guess we could do that.

Basically the logic would be following:

hasLocalChanges == true -> ignore autosave and just open the post
hasLocalChanges == false && post.autosave == null -> just open the post
hasLocalChanges == false && post.autosave != null -> open the post and show the dialog. If the user clicks on restore, load the autosave revision and set hasLocalChanges to true.

The only downside is that the users might expect that when the dialog is not shown, there isn't any newer version of the post. However, if hasLocalChanges == true, there might be a newer version. But it's probably not a big deal.

The service to query the latest autosave for a post is: GET https://public-api.wordpress.com/rest/v1.1/sites/$site_id/posts/$post_id/autosave

We are currently using GET /v1.1/sites/$SITE_ID/posts?context=edit&meta=autosave. I think we won't need to fetch an autosave for a single post. Or is there any scenario you can think of? Either case thank you for looking this up - it's good to know that the endpoint exists even if we don't use it for this particular ticket.

diegoreymendez commented 4 years ago

The only downside is that the users might expect that when the dialog is not shown, there isn't any newer version of the post. However, if hasLocalChanges == true, there might be a newer version. But it's probably not a big deal.

I agree we shouldn't think of it as a big deal currently.

My undestanding on autosaves, from what I saw in the APIs and the logic in there is that they're not really meant to be a safe storage mechanism, but rather something quite transient. They can really be dropped in many different scenarios, and I think this in part is due to how they're currently implemented (beyond just mobile).

We are currently using GET /v1.1/sites/$SITE_ID/posts?context=edit&meta=autosave. I think we won't need to fetch an autosave for a single post. Or is there any scenario you can think of? Either case thank you for looking this up - it's good to know that the endpoint exists even if we don't use it for this particular ticket.

That's fine!

The reason I added that info is because it took me a while to actually figure out how to retrieve autosave info for a single post for my manual tests. I would have though we'd be using the single post endpoint, but this is for no particular reason. Quite honestly whatever works for us is perfectly fine. Consider this my attempt to document something I'm not sure we have implemented in iOS.

diegoreymendez commented 4 years ago

@yaelirub - If you agree with the proposal above, this issue can be moved out of discovery, and can be actively worked on. The same applies for the matching iOS issue.

The proposal is:

Is there any objection with implementing this for when hasLocalChanges == false case and leaving the rest for another issue about conflict resolution?

yaelirub commented 4 years ago

I agree with the proposed solution. Updating the description here and on the iOS ticket to reflect the decision.

yaelirub commented 4 years ago

@malinajirka , from your comments:

I think we won't need to fetch an autosave for a single post and If the user clicks on restore, load the autosave revision

How are we fetching the autosave revision if not by using the GET endpoint?

malinajirka commented 4 years ago

How are we fetching the autosave revision if not by using the GET endpoint?

We are using just the following endpoints:

For fetching the post list GET /v1.1/sites/$SITE_ID/posts?context=edit&meta=autosave

For fetching a specific post GET /v1.1/sites/$SITE_ID/posts/$POST_ID?context=edit&meta=autosave

Both contain all the autosave data.

malinajirka commented 4 years ago

This issue is blocked by - https://github.com/wordpress-mobile/WordPress-Android/issues/10108. I though we'd simply show a regular system dialog. I assume we decided to consider designing our own solution so we wouldn't need to show a blocking dialog. Is that a correct assumption @yaelirub?

If that's the case I'd consider using a regular dialog in the first iteration and improving it later when we have some data about it's usage - I don't think it'll be shown that often so it might not be worth the effort. Wdyt?

cc @osullivanchris

yaelirub commented 4 years ago

I though we'd simply show a regular system dialog. I assume we decided to consider designing our own solution so we wouldn't need to show a blocking dialog. Is that a correct assumption @yaelirub?

Yes. I previously spoke to Megs and it sounded like we wanted to design our own. I agree that we can go a head and show a system dialog for the first iteration.

@osullivanchris, if you're good with showing a system dialog, please close https://github.com/wordpress-mobile/WordPress-Android/issues/10108.

yaelirub commented 4 years ago

@malinajirka , anyways, this in not blocked by the dialog design. Please proceed with the implementation.

malinajirka commented 4 years ago

@osullivanchris is considering showing the "version conflict" resolution flow even for this case. I'll wait for his decision before moving forward with this ticket.

osullivanchris commented 4 years ago

Proposal captured on paCBwp-bZ-p2

yaelirub commented 4 years ago

After discussions with @diegoreymendez , @maxme and @osullivanchris , we are closing this issue and #10008 in favor of implementing the simple approach which will be captured in this ticket

We intend to discuss any enhancements or other proposals to better handle conflict resolution in the future.

wordpress-mobile / WordPress-Android