Skip to content

Editor: Save post after deletion so that its status is refreshed and redirection happens.#17427

Merged
epiqueras merged 2 commits intomasterfrom
fix/move-to-trash-fails-to-redirect
Sep 13, 2019
Merged

Editor: Save post after deletion so that its status is refreshed and redirection happens.#17427
epiqueras merged 2 commits intomasterfrom
fix/move-to-trash-fails-to-redirect

Conversation

@epiqueras
Copy link
Copy Markdown
Contributor

@epiqueras epiqueras commented Sep 13, 2019

Fixes #17426

Description

This PR fixes the issue described in #17426.

How has this been tested?

It was verified that moving a post to the trash works as expected.

Types of Changes

Bug Fix: Redirect users after moving a post to the trash.

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.

@epiqueras epiqueras added [Type] Bug An existing feature does not function as intended [Priority] High Used to indicate top priority items that need quick attention [Package] Editor /packages/editor labels Sep 13, 2019
@epiqueras epiqueras added this to the Future milestone Sep 13, 2019
@epiqueras epiqueras requested a review from talldan as a code owner September 13, 2019 15:05
@epiqueras epiqueras self-assigned this Sep 13, 2019
@youknowriad youknowriad added the [Release] Do Not Punt Used to indicate the issue or pull request must not be moved from the assigned milestone label Sep 13, 2019
mcsf
mcsf reviewed Sep 13, 2019
View reviewed changes
packages/editor/src/store/actions.js
STORE_KEY,
'resetPost',
{ ...post, status: 'trash' }
'savePost'
Copy link
Copy Markdown
Contributor

@mcsf mcsf Sep 13, 2019

Choose a reason for hiding this comment

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

What does it mean to save a deleted post? My first impression is that we're abusing action creators a little in order to get a result not intended from them.

My second note is that the outbound comment mentions that editPost isn't used because of its ties to change detection, while savePost itself dispatches editPost.

Copy link
Copy Markdown
Contributor Author

@epiqueras epiqueras Sep 13, 2019

Choose a reason for hiding this comment

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

What does it mean to save a deleted post? My first impression is that we're abusing action creators a little in order to get a result not intended from them.

It saves and refetches the post, getting the new status.

My second note is that the outbound comment mentions that editPost isn't used because of its ties to change detection, while savePost itself dispatches editPost.

I can't think of any issues it could cause.

@epiqueras epiqueras merged commit 570a219 into master Sep 13, 2019
@epiqueras epiqueras deleted the fix/move-to-trash-fails-to-redirect branch September 13, 2019 20:27
@youknowriad youknowriad modified the milestones: Future, Gutenberg 6.5 Sep 14, 2019
@aduth
Copy link
Copy Markdown
Member

aduth commented Nov 4, 2019 •
edited
Loading

In my testing, "Move to Trash" still doesn't complete a redirect, at least not without first prompting the user there are unsaved changes. #18269 is partly related here, though it applies to non-metabox scenarios. I worry also that we try to save all changes, when it's the case that the REST API may reject some of those pending changes (evidenced by the error message in #18269 about editing trashed posts).

When I run wp.data.select( 'core/editor' ).getPostEdits() after seeing the prompt for redirecting, it always seems to return an object with the blocks value, regardless of whether I've made any changes to the post or its content. Could this stem from some of the recent refactoring? (#16932)

@epiqueras
Copy link
Copy Markdown
Contributor Author

epiqueras commented Nov 4, 2019

In my testing, "Move to Trash" still doesn't complete a redirect, at least not without first prompting the user there are unsaved changes. #18269 is partly related here, though it applies to non-metabox scenarios. I worry also that we try to save all changes, when it's the case that the REST API may reject some of those pending changes (evidenced by the error message in #18269 about editing trashed posts).

It was a nasty race condition. Fixed and explained here: #18275.

When I run wp.data.select( 'core/editor' ).getPostEdits() after seeing the prompt for redirecting, it always seems to return an object with the blocks value, regardless of whether I've made any changes to the post or its content. Could this stem from some of the recent refactoring? (#16932)

That's expected, blocks are a transient edit and are not part of change detection.

@aduth aduth mentioned this pull request Nov 4, 2019
Merged
5 tasks
@adamsilverstein adamsilverstein mentioned this pull request Jan 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcsf mcsf mcsf approved these changes

@talldan talldan Awaiting requested review from talldan

Assignees

@epiqueras epiqueras

Labels

[Package] Editor /packages/editor [Priority] High Used to indicate top priority items that need quick attention [Release] Do Not Punt Used to indicate the issue or pull request must not be moved from the assigned milestone [Type] Bug An existing feature does not function as intended

Projects

None yet

Milestone

Gutenberg 6.5

Development

Successfully merging this pull request may close these issues.

"Move to Trash" fails to redirect to posts page

5 participants

@epiqueras @aduth @mcsf @karmatosed @youknowriad