wordpress-mobile / WordPress-Android

WordPress for Android
http://android.wordpress.org
GNU General Public License v2.0
2.92k stars 1.3k forks source link

Possible content loss when a draft is published from elsewhere #5984

Closed oguzkocer closed 5 years ago

oguzkocer commented 6 years ago

A user has reported that they published a post via the desktop client and after she opened the app, it got converted back to draft and the changes she made on the desktop client were gone. She also mentioned that their revisions were deleted, which is pretty surprising since we don't really have revision management in the apps.

She was able to restore the post from the version histories from web (Calypso or wp-admin, I am not sure), which I guess means revisions are not really deleted, but she again said the revisions were wiped from the desktop client.

I have chatted with @aforcier as this should normally never happen. However, he came up with the following scenario which we should check out and also consider other alternatives:

1. write draft in app, save as remote draft
2. make local changes to the draft, exit the editor (be offline I guess so it doesn’t auto-update the draft)
3. edit that draft on the web and publish it
4. open the app - that published post won’t update because it has local changes
5. open the post and exit the editor
6. what happens?
catehstn commented 6 years ago

I have seen this happen with Simplenote (and TinyLetter) too. It can probably be handled by time edited vs time online with a prompt but there's not a clear solution when offline -> online gets involved.

(Background syncing would help).

tonyr59h commented 6 years ago

what happens?

Just ran through this scenario. Results: Local changes overwrote the published post but since it overwrote remote data it can be recovered from a previous revision.

hoverduck commented 6 years ago

I saw a flavor of this happen today as well. I had a post that I was editing both via Calypso web and Android (using Aztec), and I published it via the web.

I went back to the app and still had the post open, so I hit the back navigation arrow, but must have had some unsaved changes locally because it uploaded that to the web and converted the post from Published back to a draft.

SiobhyB commented 6 years ago

I see this come up fairly often in app support too. We're usually able to help people restore the "lost" version of the post from revisions, but it's still a stressful experience for people who think they've lost their content.

Recent tickets:

261889-h, 264047-h, 264064-h

SiobhyB commented 6 years ago

Related: #3381

oguzkocer commented 6 years ago

/cc @elibud since it's related to some of the discussions we had in Miami meetup.

SiobhyB commented 5 years ago

We recently received the following two star review which describes this issue:

I found a major problem with the app. I was writing a draft on the app and saved it to work on it later in the day. When I got to the office, I continued with my article and went on to publish it. However, when I opened up my app after this, it reverted my article to the last version that was worked on the app. This annoyed me greatly as the article was retracted from being published and reverted all the work I've done. I had to reinstate the article to the latest version on desktop but such things should not be happening. The app should not operate separately from the CMS because what's the point really if I can't seamlessly switch between desktop and mobile? I won't be using the app until this gets fixed.

elibud commented 5 years ago

@oguzkocer can you see if you can replicate that issue? Also, is this something that would be solved with what you are currently working on?

oguzkocer commented 5 years ago

@elibud The original steps still reproduces the issue. That's also exactly what the user describes in the review.

The post pagination project is not related to this issue.

Just to re-iterate, here are the steps I followed:

1. Create a draft while I am online in the app. That'll create the draft in remote.
2. Go offline
3. Make changes to the post and go back. This will save the changes as local draft
4. Go to web or any other client and make changes to the post.
5. Publish the post.
6. Open the post from the app
7. Go back
8. At this point the post will be saved as a draft and overwrite changes in the remote.

This will happen regardless of if the post is fetched again or not. Since there are local changes to the post, we'll simply ignore the response from remote.

elibud commented 5 years ago

Go it, I had missed the key part of being offline 🤦‍♀️ @aerych I believe you mentioned recently adding the ability to discard local changes in the Android, can we extend that to handle this case?

aerych commented 5 years ago

Seems like it would already be handled? The user should be prompted about the local changes in this case unless I'm misunderstanding. cc @theck13

theck13 commented 5 years ago

There will be a yellow "Local changes" label on any post that has local changes. A message will be shown when previewing the post, but not while editing the post. The only indication there are local changes while editing is the Discard Local Changes action in the overflow menu. See the screenshots below for illustration.

discard_changes

elibud commented 5 years ago

@SylvesterWilmott @iamthomasbishop What do you think about this? Should we show an alert once you enter the editor with local changes that would overwrite remote changes if the post is saved? The little orange message it's pretty easy to miss and the end result quite disruptive.

theck13 commented 5 years ago

One thing to keep in mind is that local changes are discarded only if there is a network connection since the post is reverted to the remote version. If we intend to allow discarding local changes offline, we need to rethink how local changes are discarded.

elibud commented 5 years ago

@theck13 can you please check the steps @oguzkocer posted above? That didn't sound to me like the local changes were discarded.

theck13 commented 5 years ago

The local changes are not discarded in those steps. The post is uploaded with the local changes, which effectively overwrites any remote changes. The local changes would be discarded if the device is online and the Discard Local Changes action is selected while editing or the Discard Changes action is selected while previewing.

elibud commented 5 years ago

We need to fix this, @iamthomasbishop @SylvesterWilmott either of you can propose a design for this? @aerych can you check how this affects iOS?

aerych commented 5 years ago

can you check how this affects iOS?

There is similar behavior. From the original steps:

1. write draft in app, save as remote draft
2. make local changes to the draft, exit the editor (be offline I guess so it doesn’t auto-update the draft)
3. edit that draft on the web and publish it
4. open the app - that published post won’t update because it has local changes
5. open the post and exit the editor
6. what happens?

When returning to the iOS app in step four the draft post remains in the draft filter and shows an upload failed status. A pull-to-refresh does nothing to change this. If I enter the editor and choose to dismiss changes then I finally see the published post (under the published filter). However, I can also choose to save from the editor, which overwrites the published post, setting it back to a draft.

mzorz commented 5 years ago

Current state: better than before

Following the steps as described by @oguzkocer here (adding a small clarification about when to switch the device online again, in step 6):

1. Create a draft while I am online in the app. That'll create the draft in remote.
2. Go offline
3. Make changes to the post and go back. This will save the changes as local draft
4. Go to web or any other client and make changes to the post.
5. Publish the post.
6. Open the post from the app, _and turn airplane mode off_,
7. Go back
8. At this point the post will be saved as a draft and overwrite changes in the remote.

With the current state of the app (now History is part of it), we can retrieve both versions (the one as published on the web in step 5, and the one that was made locally). So the user will have the tools to choose what to do and rebuild whatever they really want to get published from the different offered saved versions - except that at step 8 the user probably did not want the published Post to be converted back to a draft, with a different revision than previously published.

Proposal

To cover that specific case, I think we can make a check before uploading and, if there's a more recent version on the server then we could offer a Dialog saying so, and asking the user whether they want to continue and update the server-side copy with the current local changes or they prefer to load whatever there is server-side (along the lines of what @elibud came up with before in this comment https://github.com/wordpress-mobile/WordPress-Android/issues/5984#issuecomment-409319452).

I can put something together "quickly" but I guess there will be more cases where we want to give user more control. We should think of a proper design for this flow wdyt @iamthomasbishop @SylvesterWilmott ?

oguzkocer commented 5 years ago

@mzorz Thanks for bringing this up again. We actually have a bit more control over things from before which might help with the design.

When we fetch the post list, we compare the last modified dates of the remote and local posts. That means we can actually tell if a post has both local and remote changes during pull to refresh and give the option of either keeping the local version or the remote version. We could easily do this for multiple posts.

This will not handle every case, since the post list might not be fetched before the publish/override happens, but I think it'll handle vast majority of the cases.

@mzorz This would tie into your question in iamtryingtofigureitout. If we go with this, instead of ignoring the posts with local changes, we could dispatch an action to be handled in WPAndroid with the details of which posts have both local and remote changes. If that happens, we'll probably want to use ListStore for it, so we can look into it together if you wish.

mzorz commented 5 years ago

If we go with this, instead of ignoring the posts with local changes, we could dispatch an action to be handled in WPAndroid with the details of which posts have both local and remote changes. If that happens, we'll probably want to use ListStore for it, so we can look into it together if you wish.

I agree there's much more we can do with the information we have now available (with Posts attributes, History and such). I think it'd be better to wait for some design input about flows if possible, so we can come up with a more robust / complete implementation after that

megsfulton commented 5 years ago

Was chatting this through a bit with @iamthomasbishop trying to figure out when and how to alert the user to the issue.

One approach we could take that would be minimally disruptive but still communicate that there's an issue to be resolved:

How possbile is it to do something along those lines?

mzorz commented 5 years ago

Hey @megsfulton ! Thanks for chiming in 😄

When the post list is fetched, display a line on the post card indicating that there's a conflict. Similar to how "Local Changes" appears but with some sort of warning icon.

tl;dr: Definitely doable. Couple caveats though, it is important to note that all we know at the point of showing the Posts list is that the modification date of the Post is different (local vs remote) and / or we have a "locally modified" flag. That's as much as we can rely on at that part, without actually looking into each Posts content. Anyway, it should be a good enough indication that something is different locally vs remote. Also, we'd need to refresh the Posts list each time the user accesses it to make sure we've got the latest.

When the user opens the post ask them how they would like to resolve the conflict. Until they open the post to take an action both conflicted states would continue to exist.

I'm assuming conflict resolution here is something simple here: like "accept remote version" or "keep local version". Is that right?

oguzkocer commented 5 years ago

it is important to note that all we know at the point of showing the Posts list is that the modification date of the Post is different (local vs remote) and / or we have a "locally modified" flag. That's as much as we can rely on at that part, without actually looking into each Posts content. Anyway, it should be a good enough indication that something is different locally vs remote.

@mzorz I think that shouldn't be an issue for us. For the current request, it'll be definitely good enough, as we are completely relying on that information to fetch a newer version of that post in the post list and it has been working well. If we did require more information, we could just fetch the newer version of the post without saving in the DB and use the content field or something for more information. Having said that, we don't have a local revision that'd make things immediate, so you're right on not having a lot of information immediately.

Also, we'd need to refresh the Posts list each time the user accesses it to make sure we've got the latest.

This is already done, so shouldn't be an issue :)

I like this conflict resolution suggestion. If we go with this, it might be best to skip the handleFetchSinglePostCheckLocallyChanged of this PR: https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/1064#discussion_r247252981 and just rely on the change you have made in the handleFetchedPostList. That way, we can just dispatch an action to fetch the newest version of the post if the user want's to accept the remote version. We'll just want to check that we are not dispatching this action to update the post in the places like EditPostActivity since Post List will be responsible for it. If the user wants to keep the local version, we can simply dispatch an action to PUSH the local post to remote.

This ^ would not be an ideal solution as we'd rely on some assumptions, but it'd be easier to implement and try than dealing with a bunch of flags on when to update a post and when not to. If we end up liking this solution, we could iterate and improve on it. What do you think @mzorz ?

mzorz commented 5 years ago

I like this conflict resolution suggestion. If we go with this, it might be best to skip the handleFetchSinglePostCheckLocallyChanged of this PR: wordpress-mobile/WordPress-FluxC-Android#1064 (comment) and just rely on the change you have made in the handleFetchedPostList. That way, we can just dispatch an action to fetch the newest version of the post if the user want's to accept the remote version. We'll just want to check that we are not dispatching this action to update the post in the places like EditPostActivity since Post List will be responsible for it. If the user wants to keep the local version, we can simply dispatch an action to PUSH the local post to remote.

Agreed

This ^ would not be an ideal solution as we'd rely on some assumptions, but it'd be easier to implement and try than dealing with a bunch of flags on when to update a post and when not to. If we end up liking this solution, we could iterate and improve on it. What do you think @mzorz ?

Same thoughts here as well 👍

Will start working on a PR along the lines of https://github.com/wordpress-mobile/WordPress-Android/issues/5984#issuecomment-453706747

megsfulton commented 5 years ago

I'm assuming conflict resolution here is something simple here: like "accept remote version" or "keep local version". Is that right?

Yep! I'll work on writing up some text for the prompt in the next day or two. I think we'll also need to let the user know the time stamps of the different versions in that dialog/prompt so they can feel confident they're making the right choice.

oguzkocer commented 5 years ago

This issue has been addressed in #8989.