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.

Sign up for GitHub

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

Fix for #33616: Draft posts not previewable #1

Closed
wants to merge 6 commits into from
Closed

Fix for #33616: Draft posts not previewable #1

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6626593
Fix for #33616: Draft posts not previewable
Nov 11, 2021
b37a2de
test: Add E2E test for issue #33616
Nov 13, 2021
230df3d
test: Update End to End test
Dec 12, 2021
617196b
docs: Add brief comment to provide context
Jan 10, 2022
8eaafbf
docs: Remove link to PR
Jan 13, 2022
160961d
docs: Add link to PR
Jan 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 65 additions & 1 deletion packages/e2e-tests/specs/editor/various/preview.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ describe( 'Preview', () => {
it( 'should not revert title during a preview right after a save draft', async () => {
const editorPage = page;

// Type aaaaa in the title filed.
// Type aaaaa in the title field.
await editorPage.type( '.editor-post-title__input', 'aaaaa' );
await editorPage.keyboard.press( 'Tab' );

Expand Down Expand Up @@ -248,6 +248,70 @@ describe( 'Preview', () => {

await previewPage.close();
} );

it( 'should not revert title during a preview right after switching from published to draft', async () => {
getdave marked this conversation as resolved.
Show resolved Hide resolved
const editorPage = page;

// Type Lorem in the title field.
await editorPage.type( '.editor-post-title__input', 'Lorem' );
getdave marked this conversation as resolved.
Show resolved Hide resolved

// Open the preview page.
const previewPage = await openPreviewPage( editorPage );
await previewPage.waitForSelector( '.entry-title' );

// Title in preview should match input.
let previewTitle = await previewPage.$eval(
'.entry-title',
( node ) => node.textContent
);
expect( previewTitle ).toBe( 'Lorem' );

// Return to editor and publish post.
await editorPage.bringToFront();
await publishPost();

// Close the panel.
await page.waitForSelector( '.editor-post-publish-panel' );
await page.click( '.editor-post-publish-panel__header button' );

// Change the title and preview again.
await editorPage.type( '.editor-post-title__input', ' Ipsum' );
await editorPage.keyboard.press( 'Tab' );
await waitForPreviewDropdownOpen( editorPage );
await waitForPreviewNavigation( previewPage );

// Title in preview should match updated input.
previewTitle = await previewPage.$eval(
'.entry-title',
( node ) => node.textContent
);

expect( previewTitle ).toBe( 'Lorem Ipsum' );

// Return to editor and switch to Draft.
await editorPage.bringToFront();
await editorPage.waitForSelector( '.editor-post-switch-to-draft' );
await editorPage.click( '.editor-post-switch-to-draft' );
await page.keyboard.press( 'Enter' );

// Change the title.
await editorPage.type( '.editor-post-title__input', 'Draft ' );
await editorPage.keyboard.press( 'Tab' );

// Open the preview page.
await waitForPreviewDropdownOpen( editorPage );
await waitForPreviewNavigation( previewPage );

// Title in preview should match updated input.
previewTitle = await previewPage.$eval(
'.entry-title',
( node ) => node.textContent
);

expect( previewTitle ).toBe( 'Draft Lorem Ipsum' );

await previewPage.close();
} );
} );

describe( 'Preview with Custom Fields enabled', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,7 @@ export function getEditedPostPreviewLink( state ) {
}

let previewLink = getAutosaveAttribute( state, 'preview_link' );
if ( ! previewLink ) {
if ( ! previewLink || 'draft' === getCurrentPost( state ).status ) {
Copy link

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

I'd also consider adding a reference to the Issue for context.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Screen Shot 2021-12-10 at 2 40 45 PM

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!

Copy link

Choose a reason for hiding this comment

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

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 🙇‍♂️

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

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.

Copy link

@getdave getdave Jan 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Exactly.

I have added a brief comment and a link to the PR and to the issue.

previewLink = getEditedPostAttribute( state, 'link' );
if ( previewLink ) {
previewLink = addQueryArgs( previewLink, { preview: true } );
Expand Down