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

Store: Reset change detection on post update #6209

Merged
merged 4 commits into from
May 1, 2018
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 16, 2018

Fixes #6112

This pull request seeks to resolve an issue where changes made while a post is saving are not accurately reflected in change detection, resulting in the potential for data loss if the user attempts to navigate away after a save completes.

In doing so, it also introduces...

  • E2E tests for various "dirty post" prompt conditions.
  • A fix for the withChangeDetection higher-order reducer where ignored action types may have resulted in the isDirty value being lost.

Testing instructions:

Verify that end-to-end tests pass:

npm run test-e2e

Repeat steps to reproduce from #6112, verifying that the editor prompts about unsaved changes if they were introduced in the time between save start and save success.

@aduth aduth added the [Type] Bug An existing feature does not function as intended label Apr 16, 2018
@aduth
Copy link
Member Author

aduth commented Apr 17, 2018

Failing tests have been resolved in 23889c8 to use platform-dependent modifier key, which makes sense since our use of mod+s in EditorGlobalKeyboardShortcuts would be interpreted by Mousetrap as control.

But then it makes me wonder how meta+a works in multi-selection end-to-end tests (cc @youknowriad).

We have another instance of this in meta box tests. What I think is happening here (and also susceptible in this pull request) is that the Ctrl+S save isn't occurring, but the autosave kicks in 10 seconds later automatically, satisfying the previous waitForSelector. We might want to think of a way to account for this in tests (either disable autosave, check for it explicitly, shorten timeouts, etc).

@@ -88,7 +88,7 @@ export function isEditedPostNew( state ) {
* @return {boolean} Whether unsaved values exist.
*/
export function isEditedPostDirty( state ) {
return state.editor.isDirty;
return state.editor.isDirty || isSavingPost( state );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify why it should return true on mid-save?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify why it should return true on mid-save?

The fix here is moving the change detection reset from RESET_POST to UPDATE_POST, so immediately upon hitting save, before the request has finished. This means that we can track changes that occur while the save is in-flight. Conversely, we need to make sure that if the user attempts to navigate away while the save is still pending, they're notified.

Another (better?) way to express this might be to consider whether there are any transactions in redux-optimist where isDirty: true; i.e. is there a possibility that if the transaction (save attempt) was to be reverted (save fail), would the user be put back into a state of dirtiness?

I'd also considered if we could track changes more like what we had, with it being a condition of the current state: This is simple enough to check if there are any entries in state.editor.entries, but state.editor.blocksByUid and state.editor.blockOrder is harder to track for changes (without comparing to an initial value).

@aduth
Copy link
Member Author

aduth commented Apr 18, 2018

I thought meta would trigger "Ctrl" on windows?

Meta exists and has its own meaning in Windows (Windows key), though not always consistently applied by browsers apparently in treating event.metaKey.

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/metaKey

@aduth aduth force-pushed the fix/6112-dirty-in-flight branch from 23889c8 to 978f8d5 Compare April 25, 2018 19:41
@aduth
Copy link
Member Author

aduth commented Apr 25, 2018

Rebased.

e7f6b1c adds a new utility for end-to-end tests, pressWithModifier, which accepts "Mod" as an argument to translate to the platform-specific key. Existing usage has been updated, along with that in the newly-introduced tests.

978f8d5 avoids isSavingPost in the isEditedPostDirty selector, instead deferring to a new inSomeHistory selector which accepts state and a predicate function. Essentially this is the alternative behavior described in #6209 (comment), where dirtiness is a condition of whether the editor is currently dirty, or a pending-but-not-committed transaction could put the editor back into a dirty state.

@aduth aduth force-pushed the fix/6112-dirty-in-flight branch from 978f8d5 to 3a2e39f Compare May 1, 2018 19:56
@aduth aduth merged commit ac89ff3 into master May 1, 2018
@aduth aduth deleted the fix/6112-dirty-in-flight branch May 1, 2018 20:04
@aduth aduth added this to the 2.8 milestone May 1, 2018
@@ -89,7 +89,7 @@ export function isEditedPostNew( state ) {
* @return {boolean} Whether unsaved values exist.
*/
export function isEditedPostDirty( state ) {
return state.editor.isDirty;
return state.editor.isDirty || inSomeHistory( state, isEditedPostDirty );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is causing a regression for the post publish panel, it's getting closed right after publish

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dirty state is still cleared if post is edited while prolonged save request is made
2 participants