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

Editor: Fix move to trash redirect save race conditions. #18275

Merged
merged 5 commits into from Nov 5, 2019

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Nov 4, 2019

Description

Two things were causing the unsaved changes warning to trigger when trashing a post:

  1. Posts are still dirty while saving and BrowserUrl was navigating away as soon as the new post status was received, causing the redirection to happen while the post was still technically dirty.

componentDidUpdate( prevProps ) {
const { postId, postStatus, postType } = this.props;
const { historyId } = this.state;
if ( postStatus === 'trash' ) {
this.setTrashURL( postId, postType );
return;
}

  1. Even after that was fixed by waiting for saving to finish completely before navigating away, the UnsavedChangesWarning listener fired immediately with stale values.

warnIfUnsavedChanges( event ) {
const { isDirty } = this.props;
if ( isDirty ) {
event.returnValue = __( 'You have unsaved changes. If you proceed, they will be lost.' );
return event.returnValue;
}
}

That was fixed by calling the selector from within the listener.

How has this been tested?

It was verified that trashing a post no longer triggers an unsaved changes warning.

Types of Changes

Bug Fix: Trashing a post no longer triggers an unsaved changes warning.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Nov 4, 2019

While I don't think it needs to be a blocker, we should definitely have an end-to-end test case covering this behavior.

My thinking was something in packages/e2e-tests/specs/editor/various/change-detection.test.js:

it( 'Should not prompt to confirm unsaved changes when trashing an existing post', async () => {
    await createNewPost( {
        title: 'My New Post',
    } );

    await saveDraft();
    
    // ...trash post and assert no dialog
} );
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Nov 4, 2019

Per #17427 (comment), do we still have the potential that a 500 server response can occur when the save occurs after trashing the post? My thinking is something along the lines of:

  1. Add title
  2. Save post
  3. Edit title
  4. Trash post

At step 4, isn't it true that since the save occurs after the trashing, the payload (the edited title) may cause a failed response?

yield apiFetch(
{
path: `/wp/v2/${ postType.rest_base }/${ post.id }`,
method: 'DELETE',
}
);
yield dispatch(
STORE_KEY,
'savePost'
);

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Nov 4, 2019

Even after that was fixed by waiting for saving to finish completely before navigating away, the UnsavedChangesWarning listener fired immediately with stale values.

Why are those values "stale" ? Is it that componentDidUpdate is triggered for BrowserURL before the same props can take effect for UnsavedChangesWarning ? It feels to me fragile or non-obvious, and at the very least I would suggest we add a comment explaining why we are calling the selector directly.

@epiqueras epiqueras requested review from ajitbohra, nerrad and ntwb as code owners Nov 4, 2019
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 4, 2019

While I don't think it needs to be a blocker, we should definitely have an end-to-end test case covering this behavior.

Done 😄

At step 4, isn't it true that since the save occurs after the trashing, the payload (the edited title) may cause a failed response?

Yeah, but you would want to know you have unsaved changes then.

Why are those values "stale" ? Is it that componentDidUpdate is triggered for BrowserURL before the same props can take effect for UnsavedChangesWarning ? It feels to me fragile or non-obvious, and at the very least I would suggest we add a comment explaining why we are calling the selector directly.

Yes, exactly. I already had a comment there, and I've made it clearer now.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Nov 5, 2019

Per #17427 (comment), do we still have the potential that a 500 server response can occur when the save occurs after trashing the post? My thinking is something along the lines of:

The current Travis test failure appears to suggest an issue exists:

FAIL packages/e2e-tests/specs/editor/various/change-detection.test.js (65.521s)
  ● Change detection › should not prompt to confirm unsaved changes when trashing an existing post
    expect(jest.fn()).not.toHaveErrored(expected)
    Expected mock function not to be called but it was called with:
    ["Failed to load resource: the server responded with a status of 500 (Internal Server Error)"]
      at Object.expect (../jest-console/build/@wordpress/jest-console/src/index.js:34:4)
          at runMicrotasks (<anonymous>)

It's also not surfaced to the user in any meaningful way: There is no notice, and the "Saving" button never changes back to a finished state.

await page.click( '.editor-post-trash.components-button' );

// Check that the dialog didn't show.
await assertIsDirty( false );

This comment has been minimized.

Copy link
@aduth

aduth Nov 5, 2019

Member

assertIsDirty does its check by performing a reload, which I sense would conflict with clicking the trash button. Rather, I think we probably want to press Trash and wait for the page to navigate to the post list (maybe check the success notice as well), optionally adding a 'dialog' even to short-cut the test failure when encountered.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 5, 2019

Author Contributor

This works because refreshing a page with a dialog doesn't get rid of the dialog. So if navigation didn't happen because the dialog stopped it, assertIsDirty wouldn't assert false.

await page.type( '.editor-post-title__input', 'Hello World' );

// Save
await Promise.all( [

This comment has been minimized.

Copy link
@aduth

aduth Nov 5, 2019

Member

Aside: There's a helper for this saveDraft. The file as a whole could do for some refactoring to use it.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 5, 2019

Author Contributor

Yes, good catch.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Nov 5, 2019

I suspect the 500 seen in the test log is a result of the fact that a draft save is issued via assertIsDirty (edit: this isn't the behavior that happens) at the same time the post is being trashed. Running through the behavior in my browser, it all works correctly, and I see no 500 error.

I will suggest that we revert the commit adding the test case, and reintroduce a fixed implementation in a subsequent pull request.

This reverts commit 4d58c79.
@aduth
aduth approved these changes Nov 5, 2019
@aduth aduth merged commit 3c98627 into master Nov 5, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@aduth aduth deleted the fix/move-to-trash-redirect-save-race-condition branch Nov 5, 2019
jorgefilipecosta added a commit that referenced this pull request Nov 5, 2019
* Editor: Fix move to trash redirect save race conditions.

* Editor: Add e2e test.

* Editor: Expand comment.

* Fix BrowserURL capitalization

* Revert "Editor: Add e2e test."

This reverts commit 4d58c79.
@epiqueras epiqueras mentioned this pull request Nov 5, 2019
5 of 5 tasks complete
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 5, 2019

We needed to wait for the post to save again before asserting:

#18290

This PR fixes and reintroduces the test case from #18275.

It also replaces some ad-hoc saving code with the correct e2e test util, saveDraft.

jorgefilipecosta added a commit that referenced this pull request Nov 5, 2019
* Editor: Fix move to trash redirect save race conditions.

* Editor: Add e2e test.

* Editor: Expand comment.

* Fix BrowserURL capitalization

* Revert "Editor: Add e2e test."

This reverts commit 4d58c79.
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Nov 5, 2019
The list of included fixes is:

WordPress/gutenberg#18183
WordPress/gutenberg#18194
WordPress/gutenberg#18230
WordPress/gutenberg#18275
WordPress/gutenberg#18287

Updated packages:
 - @wordpress/block-editor@3.2.4
 - @wordpress/block-library@2.9.5
 - @wordpress/edit-post@3.8.5
 - @wordpress/editor@9.7.5
 - @wordpress/format-library@1.9.4

Props @aduth, @mcsf, @youknowriad, @johnbillion.
Fixes #48502.

git-svn-id: https://develop.svn.wordpress.org/trunk@46656 602fd350-edb4-49c9-b593-d223f7449a82
nylen pushed a commit to nylen/wordpress-develop-svn that referenced this pull request Nov 5, 2019
The list of included fixes is:

WordPress/gutenberg#18183
WordPress/gutenberg#18194
WordPress/gutenberg#18230
WordPress/gutenberg#18275
WordPress/gutenberg#18287

Updated packages:
 - @wordpress/block-editor@3.2.4
 - @wordpress/block-library@2.9.5
 - @wordpress/edit-post@3.8.5
 - @wordpress/editor@9.7.5
 - @wordpress/format-library@1.9.4

Props @aduth, @mcsf, @youknowriad, @johnbillion.
Fixes #48502.

git-svn-id: https://develop.svn.wordpress.org/trunk@46656 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Nov 5, 2019
The list of included fixes is:

WordPress/gutenberg#18183
WordPress/gutenberg#18194
WordPress/gutenberg#18230
WordPress/gutenberg#18275
WordPress/gutenberg#18287

Updated packages:
 - @wordpress/block-editor@3.2.4
 - @wordpress/block-library@2.9.5
 - @wordpress/edit-post@3.8.5
 - @wordpress/editor@9.7.5
 - @wordpress/format-library@1.9.4

Props @aduth, @mcsf, @youknowriad, @johnbillion.
Fixes #48502.
Built from https://develop.svn.wordpress.org/trunk@46656


git-svn-id: http://core.svn.wordpress.org/trunk@46456 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott pushed a commit to gMagicScott/core.wordpress-mirror that referenced this pull request Nov 5, 2019
The list of included fixes is:

WordPress/gutenberg#18183
WordPress/gutenberg#18194
WordPress/gutenberg#18230
WordPress/gutenberg#18275
WordPress/gutenberg#18287

Updated packages:
 - @wordpress/block-editor@3.2.4
 - @wordpress/block-library@2.9.5
 - @wordpress/edit-post@3.8.5
 - @wordpress/editor@9.7.5
 - @wordpress/format-library@1.9.4

Props @aduth, @mcsf, @youknowriad, @johnbillion.
Fixes #48502.
Built from https://develop.svn.wordpress.org/trunk@46656


git-svn-id: https://core.svn.wordpress.org/trunk@46456 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@karmatosed karmatosed added this to Inbox in Tightening Up Nov 6, 2019
@karmatosed karmatosed moved this from Inbox to Ready to create in Tightening Up Nov 6, 2019
@karmatosed karmatosed moved this from Ready to create to Waiting (update, decision, block) in Tightening Up Nov 6, 2019
@karmatosed karmatosed moved this from Waiting (update, decision, block) to Done in Tightening Up Nov 6, 2019
@youknowriad youknowriad modified the milestones: Future, Gutenberg 6.9 Nov 11, 2019
CreativeDive added a commit to CreativeDive/gutenberg that referenced this pull request Nov 12, 2019
…8275)

* Editor: Fix move to trash redirect save race conditions.

* Editor: Add e2e test.

* Editor: Expand comment.

* Fix BrowserURL capitalization

* Revert "Editor: Add e2e test."

This reverts commit 4d58c79.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.