Fix for #33616: Draft posts not previewable #1
Conversation
- Fixes issue with draft post not previwable if the draft was previously published. - During preview link creation, check if current post is in draft status.
- Add end to end test for issue WordPress#33616: Draft post not previewable if the draft was previously published.
There was a problem hiding this comment.
Thanks for the PR and nice work on providing an e2e test 👏 👏
From the PR description I'm not completely clear on how you've determined that the solution in this PR is the most appropriate.
I'm also not clear on what you believe causes the problem.
Some more context around how you've arrived at this solution would be much appreciated.
Thanks again for contributing!
🙇♂️
| @@ -839,7 +839,7 @@ export function getEditedPostPreviewLink( state ) { | |||
| } | |||
|
|
|||
| let previewLink = getAutosaveAttribute( state, 'preview_link' ); | |||
| if ( ! previewLink ) { | |||
| if ( ! previewLink || 'draft' === getCurrentPost( state ).status ) { | |||
There was a problem hiding this comment.
I'd like to understand more here about why "draft" status requires a unique case. What flow causes previewLink to be a falsy value? Is there no auto save? Does the autosave not get updated in certain circumstances? Is the REST API responding with the incorrect preview_link?
There was a problem hiding this comment.
I'd also consider adding a reference to the Issue for context.
There was a problem hiding this comment.
hi @getdave ! Thanks for the feedback!
One thing I noticed about Draft posts is that they do not update the Autosave record in the Database.
In the screenshot we can see the records for the post. The first row is the Autosave that was generated when the Post was published and previewed. The other 3 records are changes to the post that were made after switching to Draft mode. The previewLink seems to be generated based on the autosave record, so I thought adding this status check could be a good way to generate a valid previewLink.
Another observation is that after switching to Draft, and reloading the page. The autosave record gets deleted from the Database, and everything starts working fine again.
So based on these findings, I think other alternatives are:
a) Ensure that the autosave record gets updated also for Drafts, not only for Published.
b) Delete the autosave record asynchronously when switching to draft.
c) The getAutosaveAttribute is marked as deprecated since 5.6, so upgrading to the recommended getAutosave() would be worth exploring too.
As I am a new contributor, I don't have much understanding about the implications of any of these alternatives, or if we are talking about updates to the Wordpress core, so any advice is appreciated.
Thanks!
There was a problem hiding this comment.
hi @getdave ! Thanks for the feedback!
You're welcome!
The previewLink seems to be generated based on the autosave record
Can you validate this? What code path is used and where? I'd like to understand more about what actually generates the preview link?
...so I thought adding this status check could be a good way to generate a valid previewLink.
It does seem to but like you I'm also not 100% clear on this part of the system. When does the auto save record get created? Some possible avenues to explore
- does an autosave get generated when working on a post that has not been published (i.e. has always been a draft)?
- does an autosave get generated when working on a post that has been reverted from published to draft?
- what code paths force the creation of a new autosave record?
- is the autosave record "stale" when switching to draft?
Depending on the answers to the above, I wonder whether we are missing generation of an autosave record when switching from Publish to Draft status?
Ensure that the autosave record gets updated also for Drafts, not only for Published.
This is a good question. I'd need to understand more about the purpose of autosaves and their expected behaviour. Initially it would seem logical that autosaves would be generated for draft posts. After all if you've changed the post to "draft" status then you still want you changes to be autosaved right?
Delete the autosave record asynchronously when switching to draft.
I think the above above with updating the record sounds better.
The getAutosaveAttribute is marked as deprecated since 5.6, so upgrading to the recommended getAutosave() would be worth exploring too.
Worth a look. It would be nice to upgrade the code path to use non-deprecated methods. I would probably do this in a separate PR.
As I am a new contributor, I don't have much understanding about the implications of any of these alternatives, or if we are talking about updates to the Wordpress core, so any advice is appreciated.
I also don't have much context on autosaves so we'll have to dig into this together. I'll see if I can get some more advice on this as well.
Thanks for all your work here 🙇♂️
There was a problem hiding this comment.
hi @getdave,
Answering your questions:
does an autosave get generated when working on a post that has not been published (i.e. has always been a draft)?
No, autosave records get generated only for published posts.
does an autosave get generated when working on a post that has been reverted from published to draft?
No, an autosave record is not generated in this case. The autosave record that was created when the post was published remains in the Database. Subsequent updates to the draft post do not update the autosave record.
what code paths force the creation of a new autosave record?
New autosave record gets created only for published post, when clicking on the 'Preview in new tab' option from the dropdown. This is handled in the post-preview-button.js:
// Request an autosave. This happens asynchronously and causes the component
// to update when finished.
if ( this.props.isDraft ) {
this.props.savePost( { isPreview: true } );
} else {
this.props.autosave( { isPreview: true } );
}
is the autosave record "stale" when switching to draft?
Yes, the autosave record that was previously generated for the published post remains in the Database, but additional edits to the draft post do not get reflected into the autosave record. Any new edits to the draft are reflected into the original record (the parent post). Reloading the page deletes the stale record.
Thanks,
- Jorge.
There was a problem hiding this comment.
I'd like to understand more here about why "draft" status requires a unique case. What flow causes
previewLinkto be a falsy value? Is there no auto save? Does the autosave not get updated in certain circumstances? Is the REST API responding with the incorrectpreview_link?
What I'm observing is that there is no autosave for draft posts. The reason that the preview is not up to date with most recent edits is because there is a stale autosave record if the post switches from published to draft.
There was a problem hiding this comment.
Ok so is this correct?:
- the autosave exists because the post was first published and auto saves are only created for published posts.
- the post is switched to Draft which causes autosaves not to be updated on further post content "change"
- when requesting a preview, because the auto-save exists the code uses the preview link from that (stale) auto save
- the rendered preview is thus out of date with the current post content
So the solution is if the post is a draft don't use the autosave preview link from the autosave.
If i'm correct then let's add a code comment explaining why we're checking the Draft status. That comment should also contain a link to this PR.
There was a problem hiding this comment.
Exactly.
I have added a brief comment and a link to the PR and to the issue.
- Include a link to the issue that the test validates. - Use aria-label instead of css class as a selector for Title field. - Update test title to be more descriptive about the case under test.
|
This is now looking good. Thank you for all the hard work! I've requested a confidence check from @talldan just for a final 👍 then we should be good to merge. |
|
This looks great. Thanks for working on the fix, and for adding a test. The explanation in the comment makes sense to me, nice work tracking down this flaw and fixing it. Is the PR in the right repo? I think the PR should be created in the main WordPress/gutenberg repository. This is just a fork of that repo. If you could go ahead and create a PR there and drop a link to it, then Dave or I will be happy to approve. The comment in the code will have to be updated to reflect the URL of the new PR. I would also suggest summarising the explanation from the comments in the description of the new PR for anyone else that may have questions about it. |
|
Re-running the failing tests as I think the failures are erroneous. @talldan I think @jorgecontreras is following guidance from https://developer.wordpress.org/block-editor/contributors/code/git-workflow/#overview.
Will that not work here? |
|
@jorgecontreras Looks like it's as Dan said that the PR needs to be raised in the main Gutenberg repo and it should be your branch from this forked repo.
|
Got it. I have created the PR on the main wordpress/gutenberg repository: Please let me know if this looks good to you. |
|
Merged in WordPress#37952 |
Description
How has this been tested?
Screenshots
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).