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

Discussion: Post and Post.Autosave conflict resolution #10008

Closed malinajirka closed 4 years ago

malinajirka commented 4 years ago

How are we going to handle conflicts between local version of a post and remote version of the post?

This is a continuations of a discussion which started here. We moved it into a new issue as its scope is wider than the scope of https://github.com/wordpress-mobile/WordPress-Android/issues/9812.

Subscribing @wordpress-mobile/ravenclaw

malinajirka commented 4 years ago

Reply to https://github.com/wordpress-mobile/WordPress-Android/issues/9812#issuecomment-499556670 Thanks @osullivanchris !

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 was thinking we could mostly reuse the visual representation of the "History" view. Wdyt? history

osullivanchris commented 4 years ago

@malinajirka Yeah that looks great. We might just need to think through the flow a little bit. How to get from a dialogue requiring a decision, to going to this space where we see the revisions, then returning to make the decision.

osullivanchris commented 4 years ago

Here is an idea I had for this flow. What if we forced the user through the revision history page, in cases of conflict that require resolution.

My rationale is that its hard to know which version is which without looking at them. And I don't think a complex action like reviewing different revisions can be resolved just through an alert dialogue.

In the below sketch, you'll see I'm using an adapted version of the history, with information above explaining to the user why they're here and what they need to do.

Some other numbered ideas/questions on the sketch

  1. Make sure we show some kind of contextual information on the post card, to give pre-emptive warning to the user that there is an issue
  2. Do you think we should keep the regular 'edit', 'publish', 'more' actions here? We could override with a single action like 'resolve conflict' because that is in fact the only thing we're allowing the user to do with this object
  3. I think it could be useful on the revision history to note what device or location the revision happened on next to the name of the person who made the revision
  4. Should we add some kind of label to the revision so that the user knows which one is live, and which are edits? Maybe that doesn't work. It depends. Are revisions about edits, or versioning? To me an edit just shows what changed, whereas a revision shows what the post looked like at a certain point in time. IMG_0696

cc @malinajirka @diegoreymendez @yaelirub

malinajirka commented 4 years ago

My rationale is that its hard to know which version is which without looking at them. And I don't think a complex action like reviewing different revisions can be resolved just through an alert dialogue.

πŸ’―

Make sure we show some kind of contextual information on the post card, to give pre-emptive warning to the user that there is an issue

πŸ‘ We currently show "Version conflict" label.

Do you think we should keep the regular 'edit', 'publish', 'more' actions here? We could override with a single action like 'resolve conflict' because that is in fact the only thing we're allowing the user to do with this object

I think replacing the other actions with "resolve conflict" makes sense.

I think it could be useful on the revision history to note what device or location the revision happened on next to the name of the person who made the revision

I like the idea, however, I'm a bit worried this is not supported by the api at the moment. If it were up to me, I'd omit it in the first iteration. We can create an enhancement ticket so we don't forget about it. Wdyt?

Should we add some kind of label to the revision so that the user knows which one is live, and which are edits? Maybe that doesn't work. It depends. Are revisions about edits, or versioning? To me an edit just shows what changed, whereas a revision shows what the post looked like at a certain point in time.

They are actually kind of both:D... They contain title, content and excerpt of a post at a certain point in time, but they also contain list of "diffs" from a certain revision. However, I like the idea and I believe it should be doable. ;)

This design proposal looks great to me! Good job and thank you;)

Technical point of view: We can have conflict in

a. Post content - remote vs local

b. AutoSave object - remote vs local

I think we have two options:

  1. Add full support for local revisions - have a table of local revisions in our local database and add support for loading them on the history screen (There is some work already done in Android's FluxC, but the logic of creating a revision from a post isn't there.).

  2. Add support for artificial local revisions - Somehow show the local content of the post (local autosave object) on the history screen - we'd basically create an artificial revision just on the UI layer.

yaelirub commented 4 years ago

Labeled "13" as its a discussion

osullivanchris commented 4 years ago

I like the idea, however, I'm a bit worried this is not supported by the api at the moment. If it were up to me, I'd omit it in the first iteration. We can create an enhancement ticket so we don't forget about it. Wdyt?

Yeah I agree @malinajirka that wouldn't be necessary as part of this particular issue, and could be captured separately as a potential improvement

Technical point of view:

do you need any input from me here or are you just capturing what you want to do?

How do we move this from discussion to solve it @yaelirub ? Should I go ahead and do proper UI mockups for the above sketch, seeing as it mostly makes @malinajirka agrees with the proposal? Any other open questions?

I also don't know how flexible the revision history page is, to be able to create a custom version of it with header and details per my idea.

malinajirka commented 4 years ago

do you need any input from me here or are you just capturing what you want to do?

Tbh I'm not sure if I should have posted this here. I'm not sure if the discussion tickets are platform in/dependant. Either case it's a technical detail which doesn't require any input from you. Thanks for checking!

malinajirka commented 4 years ago

I also don't know how flexible the revision history page is, to be able to create a custom version of it with header and details per my idea.

I believe this is a technical detail as well. We'll either re-use the existing page or create a brand new page. If you add some minor things into the design which will make it un-reusable, we'll let you know before we make the decision. From what I see now, I believe we can use the "Live" label and location/origin (Android, Web) even in the current history. So basically the only difference is the static header which won't be a problem.

diegoreymendez commented 4 years ago

I'm not sure if the discussion tickets are platform in/dependant

I don't want to digress, but I do want to quickly address this question: for now let's try to make sure discussion tickets accept any kind of input for either platform, since duplicating the issue would make it hard to keep track of things.

malinajirka commented 4 years ago

Summary of the current proposal

This table seems still up to date. Our proposed solution come downs to three cases

  1. There is no conflict or unpublished revision - we simply show the post
  2. There is no conflict, but there is an unpublished revision - we show the editor + unpublished revision dialog
  3. There is a conflict - we show "a copy" of Revision History screen with conflicted revisions "local post, local autosave, remote post, remote autosave".

Note for case 2: If I understand this correctly, we'll show the "unpublished revision" dialog, but the user won't be able to see the "unpublished revision" until they click on "Restore" (in other words we are not planning to show the revision history screen in this scenario, right?). I'm a bit concerned about two things

I guess we could solve both issues by creating a local revision before we restore the post. Wdyt?


@maxme What do you think about the Technical point of view: section in this comment? I feel like having full support for local revision would be nice at the same time I'm not sure we actually need it at the moment. Having said that if I had to decide I'd go with full support as it'll be more flexible and the chances it'll be useful are quite high imho. I also believe the implementation won't be complicated.

osullivanchris commented 4 years ago

Hi all I have picked up this issue and gone through some of the details of the idea I shared before. Hopefully the sketch is reasonably self-explanatory. It forces users through the 'conflict resolution' screen to decide which version to load. The main argument to do so is that it provides clarity into which exact version are they loading. I also have showed which device they made the change on. I think someone said that's not possible, but I saw we are doing it on the existing conflict resolution dialogue.

concept

When I saw it, one thing that occurred to me was that a user who hasn't dealt with the revision history screen before might not know the 'load' primary call to action. So I tried some alternatives where we show that action in the middle screen, so that the user doesn't have to go into the actual revision, only if they wish. Tapping anywhere else on the row would continue through to 'C' in the above flow. Tapping on load would skip 'C' Alts

Per #9812 it looks like some decisions were already made. As I understand that discussion is about which conflicts might occur where. The solution here is what can we show, in the case that there is a conflict that needs resolving.

So perhaps in reply to this concept, it would be good to purely discuss: is this a good solution when there is a conflict to be resolved?

malinajirka commented 4 years ago

❀️ I really like this flow. It makes the conflict resolution very clear.

Per #9812 it looks like some decisions were already made.

Just for completeness: The decision there was that we would ignore autosave when "hasLocalChanges==true" which is a pre-requisite for a conflict. So when we combine these two solutions we'll have a full support for loading an autosaved revision.

megsfulton commented 4 years ago

I really like this solution @osullivanchris!

Even though it's an extra tap, I like the first version you put together best because it's the most straight forward. In version 2, I wouldn't know that tapping the row outside of Load would load the full revision. Version 3 makes that a little clearer and don't mind it as an alternative.

My main feedback is about the screen content. I noticed in the normal version history we call the items "revisions." Which makes me wonder if we shouldn't do the same thing throughout this flow? I also think that the copy needs to clue the user into what they have to do (Version 3 does a better job of this than the others).

For the "Choose Version" pages, maybe something like:

App bar title: Resolve revision conflict

Screen text: Choose a revision to load and continue editing.

osullivanchris commented 4 years ago

@megsfulton thanks really glad you like the concept!

Appreciate the feedback, and very fair! My starting point was the error message 'Version conflict' so I continually referred to 'versions' throughout.

I will work through it again with your feedback and check with editorial for a double check if needed before going ahead with this.

yaelirub commented 4 years ago

@osullivanchris , I'm loving this solution! If you end up updating according to @megsfulton suggestions and/ or editorial, would you please update the mocks in your proposal above?

We will create another ticket for the implementation of this proposal once you are done and approve.

@malinajirka , thank you so so much for pushing this discussion <3

osullivanchris commented 4 years ago

Conceptv2

Update with the copy tweaks from Megs.

Tagging for editorial review and otherwise this is done from design side.

FAO editorial: When a user has a conflict with different versions of their post and we need them to choose which one to load, we're now bringing them through the revision history page to help them understand which versions we are referring to, and have more context to make a choice.

Most of the new copy is in the middle screen. Title: Resolve revision conflict Body: This post has 2 revisions that are in conflict. Choose a revision to load and continue editing. Heading: Older revisions

Also the error message on the post: Revision conflict (was previously "Version conflict")

Thanks!

michelleweber commented 4 years ago

Could we add more detail to the first screen? I don't know that I'd immediately know what "revision conflict" means, but something like "Multiple revisions available" or even "Conflicting post revisions" would be a wee bit clearer.

osullivanchris commented 4 years ago

@michelleweber good point. I think I prefer "conflicting post revisions", as it sounds more like it is an issue that needs to be resolved (which is true). "Multiple revisions available" sounds more neutral.

Is the content on the other screens ok for you?

michelleweber commented 4 years ago

On the second screen, let's tighten a smidge:

This post has two revisions that conflict. Choose one to load and continue editing.

Otherwise, looks fine!

osullivanchris commented 4 years ago

Thanks @michelleweber

Updating the mock with final copy!

Conceptv3

shiki commented 4 years ago

Conflict resolution seems like one of those issues that should be tackled as a bigger project.

I'd just like to add another side to this instead of creating another issue to avoid another discussion. 😬 One of the things that we also need to solve is when and how to detect possible conflicts. For example, these steps raised by @rachelmcr shows an invisible conflict (from p4a5px-2pK-p2):

  1. Create and save a draft in the app.
  2. Leave the app open to the Blog Posts list.
  3. Open the draft on the web, make edits, and publish it.
  4. In the app, open the post again in the editor. Note that the changes from the web aren’t synced.
  5. Close the editor (e.g. in an attempt to back out without saving anything).

Result: The editor autosaves the draft from the app, overwriting the changes (including published status) from the web.

The result is the published post is reverted to a draft. We can say that this is unlikely to happen but it can still happen.

There are many ways we can solve this but I'd just like to focus on my first point (paragraph) for now. πŸ™‚

malinajirka commented 4 years ago

Conflict resolution seems like one of those issues that should be tackled as a bigger project.

I agree that it's a bigger task, but it's kind of blocking the whole remote-auto-saving. I guess we could go with the simple dialog "use web vs local version" for all the scenarios. The UX wouldn't be great (quite bad tbh), but still better than not having any conflict resolution at all. WDYT?

osullivanchris commented 4 years ago

I guess we could go with the simple dialog "use web vs local version" for all the scenarios.

Can all scenarios be simplified down to that dialogue?

malinajirka commented 4 years ago

Can all scenarios be simplified down to that dialogue?

Sorry, my comment was a bit misleading. I believe it "can be simplified" down to two dialogs if we are fine the user might lose their changes in some cases.

There is a plan to improve the API - stop overriding auto-saved version when there are too many changes. It really depends on what is considered to be "too many changes", but it might decrease the risk of losing the changes.

So technically we can. First question is whether it's a good idea:). Second question is how much development time would we actually save.

malinajirka commented 4 years ago

I have been thinking about this and the user will lose their changes only in one of the following scenarios

  1. They edit not-the-most-recent version of a post. I believe they'll be aware that they are editing not-the-most-recent version.
  2. They choose a wrong option in the "conflict resolution" dialog and we permanently delete their local changes (we could perhaps introduce undo). (3. The app uploads a post which is out-of-date and overrides the most recent version in the remote. - this is out scope and a proper fix requires API change. Let's not discuss it here)

So all in all I think it'd be a good enough solution for v1. The UX won't be optimal as users won't even know what they are restoring/throwing away + moreover, two dialog in a row will be shown in some cases.. But the "auto-upload/save" is such a huge improvement that it's still worth it.

shiki commented 4 years ago

I agree that it's a bigger task, but it's kind of blocking the whole remote-auto-saving.

@malinajirka I've been staring at this and #9812 but I can't quite understand why the conflict resolution is blocking remote auto-saving. πŸ˜… I might have missed some conversations. Would you mind explaining more?

osullivanchris commented 4 years ago

@malinajirka I'm keen not to slow you down here and I believe the work I'm doing with conflict revisions is bigger than what is required to unblock your main tasks right now.

For v1 - showing a dialogue can be good enough. If that is quicker for you?

Conflict resolution seems like one of those issues that should be tackled as a bigger project.

Quoting @shiki here. Do you also think we should make a separate, broader task, to look at conflict resolution more broadly? It might get moved to the next project, if necessary. Cc @yaelirub

I have done some lightweight user testing on this issue. I'll try to write up the findings today. It might help

yaelirub commented 4 years ago

After discussions with @diegoreymendez , @maxme and @osullivanchris , we are closing this issue and #9812 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.