Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore autosave revision dialog #12947

Merged
merged 34 commits into from Dec 2, 2019

Conversation

@guarani
Copy link
Contributor

guarani commented Nov 14, 2019

Overview

Fixes #12141

On the post list screen, posts with an autosave revision are now marked with a new status label. When tapped, instead of just loading the post as before, users are now also given the option to load the autosaved revision. Loading the autosave revision works in both the Gutenberg and Aztec editors.

Demo

Testing Steps

  1. On WordPress.com, create a new post or open an existing post for editing
  2. Set a new arbitrary post title, content, and excerpt
  3. Publish the post
  4. Re-open the post and make an edit – to the title, content and/or excerpt – then click the preview button to trigger an autosave in the background
  5. On the iOS app, under the My Sites tab, navigate to the list of posts containing the newly created post
  6. Observe that there is now a status label – "You've made unsaved changes to this post" –indicating that an autosave revision is available
  7. Tap the post and observe a dialog giving an option to load the post directly or load the autosaved revision
  8. Tapping the option to load autosave revision ("From another device") should load the changes made in Step 4 into the editor (if there is an autosaved excerpt, it appears in the editor menu ➔ Post Settings ➔ Excerpt)
  9. Tapping "Update" should save the changes and if the user exits the editor, the changes should be shown on the post list screen
  10. Likewise, the option to load the post directly ("From this device") should load the post without the changes from Step 4
  11. If the autosave option is chosen and the user makes edits and then closes the editor without saving, the post list should still show the status label from Step 5 and subsequently tapping on the post should again show the dialog

Notes

  • The above steps must work for,
    • both the Gutenberg and Aztec editors, and switching between editors must work as expected
    • both published and scheduled posts
  • Posts with local changes must not offer load autosave functionality
  • An autosave can be created on the same device the post is later loaded on - so the dialog labels "From this device" and "From another device" don't always seem intuitive
    • Similarly, if the autosave is created on a device (e.g. changes are previewed), then the user exits the editor and discards the changes, the option to load the autosaved (discarded) changes is still available

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.
guarani added 3 commits Nov 10, 2019
A post is considered to have an autosave if the `autosaveModifiedDate`
is not nil.
When a post containing an autosave is tapped, a dialog is presented to
allow the user to make a choice between loading the post's current
published version and the post's autosave version.
@guarani

This comment has been minimized.

Copy link
Contributor Author

guarani commented Nov 14, 2019

Please note this is a Work in Progress (WIP) draft PR.
Submitted for review.

@guarani guarani changed the title Issue/12141 restore autosave Restore autosave revision dialog Nov 14, 2019
guarani added 7 commits Nov 17, 2019
When a post is edited, the editor is loaded with the original post and
then a revision is made. This order seems incorrect - the revision
should be made _before_ being loaded into the editor.

This change will allow autosave content that will be conditionally added
to a post revision to be loaded into the editor.
If the user chooses to load a post's autosave content, their choice is
propogated to both the Gutenberg and Aztec editors where it is loaded
into the post revision created by the editors. This revision
content is then loaded into the editors using the pre-existing editor
functionality.
Renamed variables and removed explicit `self` reference when
unnecessary.
Users are only asked if they wish to load a post's autosave changes if
the post has NO local changes.
A recent commit undid the code to load autosaves, this is now fixed.
@guarani guarani marked this pull request as ready for review Nov 22, 2019
@shiki shiki self-requested a review Nov 22, 2019
@shiki shiki added this to the 13.8 milestone Nov 22, 2019
Copy link
Member

shiki left a comment

Great work, @guarani!

  • Tested Aztec ✔️
  • Tested Gutenberg ✔️
  • Confirmed the labels are correct ✔️
  • Simulated a crash ⚠️
  • Tested the dialog will not show if there is a local change ✔️
  • Tested Pages

Possibly confusing labels

An autosave can be created on the same device the post is later loaded on - so the dialog labels "From this device" and "From another device" don't always seem intuitive
Similarly, if the autosave is created on a device (e.g. changes are previewed), then the user exits the editor and discards the changes, the option to load the autosaved (discarded) changes is still available

Good job finding this, @guarani.

Yeah, this seems like a flow that we did not anticipate. ☹️ This does not happen on Android because there is no Discard Changes option. WPAndroid immediately submits a new remote autosave when exiting the editor.

Do you have some suggestions to circumvent this?

@osullivanchris, what do you think about this scenario? Should we rename the buttons? Or should we make the Discard Changes button also discard the remote autosave (seems dangerous)? Or just leave this be for now?

Incorrect label after crash simulation

I simulated a crash using these steps:

  1. Create a post on the device and published it
  2. Created an autosave in the browser
  3. Edited the post on the device. Chose “From another device” in the dialog.
  4. After a few seconds that the editor is shown, I force-closed the app
  5. Reopened the app

In the Post List, the cell shows “You've made unsaved changes to this post”. However, when I click on the cell, the dialog is not shown. The expected message should most probably be “Local changes”.

We probably just need to add another condition in PostCardStatusViewModel.

Pages

It doesn't look like this works in pages. But I realized that the code is quite different. This should be a different task.

Others

I think this is worthy enough to add some release notes. 🙂

@guarani

This comment has been minimized.

Copy link
Contributor Author

guarani commented Nov 24, 2019

Thank you @shiki for the thorough review!

Possibly confusing labels

Yeah, this seems like a flow that we did not anticipate. ☹️ This does not happen on Android because there is no Discard Changes option. WPAndroid immediately submits a new remote autosave when exiting the editor.

Do you have some suggestions to circumvent this?

If Android ensures the autosave revision is always kept up-to-date with the editor contents - even when discarding content - while iOS currently does not then my suggestion would be to bring iOS behavior inline with Android (if this agrees with the intended user experience for iOS).
cc @osullivanchris

Incorrect label after crash simulation

In the Post List, the cell shows “You've made unsaved changes to this post”. However, when I click on the cell, the dialog is not shown. The expected message should most probably be “Local changes”.

If I'm understanding correctly, in this scenario you killed the app after simply opening the editor and viewing the autosave - you didn't make any changes to the content - is that correct? If that's the case, I would have thought that showing "You've made unsaved changes to this post" is correct because no local changes have been made. I agree of course that the autosave dialog should be shown though.

Pages

It doesn't look like this works in pages. But I realized that the code is quite different. This should be a different task.

Ok - I agree pages seem best suited to a different issue.

Others

I think this is worthy enough to add some release notes. 🙂

Yes, thanks for the reminder :) I've added a note for 13.8, which is the PR milestone.

@shiki

This comment has been minimized.

Copy link
Member

shiki commented Nov 27, 2019

"You've made unsaved changes to this post" is correct because no local changes have been made. I agree of course that the autosave dialog should be shown though.

That's correct. Please see this video. This is an edge case. If having a “Local changes” label is easier to implement, I think we can do that.

guarani added 6 commits Nov 29, 2019
Post autosave identifier is now persisted via core data to `AbstractPost`.
An autosave is available iff there is an autosave identifier greater
than zero.
To keep PostListViewController lean, move the logic for editing posts
(or loading their autosave revision) into a presenter.
All editors (Gutenberg and Aztec) support loading autosave revisions.
A required initializer is now defined in the PostEditor protocol to
ensure this dependency is always satisfied.
The autosave dialog was previously on the UIAlertController static
namespace, even though it has a one-off use case. Now it's in a custom
presenter and kept private.
@peril-wordpress-mobile

This comment has been minimized.

Copy link

peril-wordpress-mobile bot commented Nov 29, 2019

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

guarani added 2 commits Nov 29, 2019
Added check to post card status view model to ignore autosave status
label if post has local changes.
Using a "local" remote status for the post is wrong because it interfers
with the requirement that posts with local changes should not show the
status.
@guarani guarani requested a review from shiki Nov 29, 2019
@osullivanchris

This comment has been minimized.

Copy link

osullivanchris commented Nov 29, 2019

This does not happen on Android because there is no Discard Changes option. WPAndroid immediately submits a new remote autosave when exiting the editor.

I took a look at the 'leaving the editor' behaviour before. I prefer the Android solution. You can see a sketch or refer to paCBwp-d6-p2

if the autosave is created on a device (e.g. changes are previewed), then the user exits the editor and discards the changes, the option to load the autosaved (discarded) changes is still available

I had a similar conversation with Jirka on Android. I'm not sure if its the same scenario. But we basically found that each time the user opens a post with a conflict, until they make a committing change e.g. save/publish, we will have to show them the dialogue every time they open the post. The only way to avoid that is to force the user to make a decision. Keep one to move forward with.

should we make the Discard Changes button also discard the remote autosave (seems dangerous)?

Yeah that doesn't sound like what I want to do when I'm discarding changes. I would see 'discard changes' as meaning discard what I have done within this editing session. Agree?

If Android ensures the autosave revision is always kept up-to-date with the editor contents - even when discarding content - while iOS currently does not then my suggestion would be to bring iOS behavior inline with Android (if this agrees with the intended user experience for iOS).

Not 100% certain I understand but I feel like I agree with this. See my first comment about discarding on Android. Is this similar?

General point

Every time I jump back into something related to offline, it honestly takes me a huge context switch. Just reading this post has taken me about an hour! Its definitely possible that I'm just stupid, but I don't feel thats entirely to blame.

I think that all the flows around revisions and local changes, etc are far too complex. Sure, there is probably an inherent amount of complexity that we cannot get rid of. But it feels like we allow too much flexibility.

I think we are continually patching the current UX and the quicker we can break the paradigm the better. I think in the future I would in the system:

  • have smart defaults that require less user decisions. E.g. assume to save the post when the user leaves the editor, don't ask whether they want to discard.
  • when the user does have a choice, make it a committing one (e.g. when the user encounters a conflict create a flow where they must choose one, so that we don't have to have two concurrent versions after that point)
  • I think having both autosave and explicit save is another choice that creates confusion. Perhaps we should only have autosave.

As I'm struggling to understand the current system, I'm sorry if the answers prior to this general point are not conclusive enough

@shiki

This comment has been minimized.

Copy link
Member

shiki commented Nov 29, 2019

Every time I jump back into something related to offline, it honestly takes me a huge context switch.

I feel you. And I'm sorry for dragging you into this conversation. 😐 Thank you for your time! 🙏

The only way to avoid that is to force the user to make a decision. Keep one to move forward with.

That makes sense and it sounds related to this.

Yeah that doesn't sound like what I want to do when I'm discarding changes. I would see 'discard changes' as meaning discard what I have done within this editing session. Agree?

I agree. 👍

Not 100% certain I understand but I feel like I agree with this. See my first comment about discarding on Android. Is this similar?

As I understand it, yes. We should remove that dialog altogether. And I agree. Unfortunately, this was deemed out of the scope of the Offline Posting project in paCBwp-e5-p2.

I added a note of it in the description of #11435 to make the decision/recommendation discoverable.

Summary

@guarani, based on this discussion, we can leave the Possibly confusing labels alone for now.

Copy link
Member

shiki left a comment

Looking good, @guarani! I love the progression of the commits. Very easy to follow. I also appreciate the comments. Thank you for adding those!

I have a couple of questions from the recent changes. We also need to fix the conflicts. After that, we're good to go!

guarani added 3 commits Dec 2, 2019
An enum has the advantage of not being able to be instantiated, but does
hinder the readability of the code.
@guarani guarani requested a review from shiki Dec 2, 2019
@guarani

This comment has been minimized.

Copy link
Contributor Author

guarani commented Dec 2, 2019

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.
Generated by 🚫 dangerJS

This is due to the core data migration.

guarani added 2 commits Dec 2, 2019
I noticed that there was a line in release notes that shouldn't be there
(it duplicated an existing line):

> * Block editor: Images from Image Block can now be previewed full screen by tapping on them.

This line was introduced in 6716fc8 and moved in ffdbfdb. It
looks like a merge or conflict was not properly resolved at some point
on my feature branch resulting in the move commit not being cleanly
applied (line was added to a new line position but not deleted from its
old position).

This looks like the only inadvertent change - all else looks fine.
@shiki
shiki approved these changes Dec 2, 2019
Copy link
Member

shiki left a comment

Thank you, @guarani! 🎉

@shiki shiki merged commit 4408ad7 into wordpress-mobile:develop Dec 2, 2019
6 checks passed
6 checks passed
Hound No violations found. Woof!
Peril Found some issues. Don't worry, everything is fixable.
Details
ci/circleci: Build Tests Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPad Air 3rd generation) Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPhone 11) Your tests passed on CircleCI!
Details
ci/circleci: Unit Tests Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.