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

Save meta on preview #11409

Merged
merged 17 commits into from Nov 20, 2018

Conversation

@adamsilverstein
Contributor

adamsilverstein commented Nov 2, 2018

Description

When clicking the preview button, save all metaboxes. Set preview URL only after metabox save is complete.

How has this been tested?

Using the sample code from #11280 (comment) I added a meta box. I tested changing the metabox value and clicking preview before blurring out of the field or letting a regular autosave trigger. I observed the correct meta on the preview page, and meta was saved to the post (as expected).

Types of changes

  • when previewing, call requestMetaBoxUpdates if we have metaboxes
  • defer setting the preview url until both autosave and metabox saevs have completed

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@adamsilverstein adamsilverstein requested a review from aduth Nov 2, 2018

// This relies on the window being responsible to unset itself when
// navigation occurs or a new preview window is opened, to avoid
// unintentional forceful redirects.
if ( previewLink && ! prevProps.previewLink ) {

This comment has been minimized.

@adamsilverstein

adamsilverstein Nov 2, 2018

Contributor

uncertain about this change: despite the inline comment, I don't understand how prevProps.previewLink is used here and I have removed it. instead, I am testing that both autosave and metabox save have completed.

@danielbachhuber

👍 Works as expected in my testing. I'll defer to @aduth for the underlying JavaScript.

@danielbachhuber

Approved from a functional perspective, see above comment. Deferring to @aduth on the JavaScript itself.

@aduth

This comment has been minimized.

Member

aduth commented Nov 2, 2018

To clarify, is a meta box update meant to occur in response to the interaction of the preview button press, or is it bound to the behavior of save/autosave?

This approach may be sensible for the former, but not the latter.

@adamsilverstein

This comment has been minimized.

Contributor

adamsilverstein commented Nov 2, 2018

To clarify, is a meta box update meant to occur in response to the interaction of the preview button press, or is it bound to the behavior of save/autosave?

the former, this alters the behavior for the preview button only and reintroduces the current (unfortunate) core behavior of saving metabox meta to the original post for previews. (see https://core.trac.wordpress.org/ticket/20299 -we can resolve this when we introduce meta revisioning to core)

const {
hasMetaBoxes,
isSavingMetaBoxes,
} = select( 'core/edit-post' );

This comment has been minimized.

@youknowriad

youknowriad Nov 2, 2018

Contributor

The editor package shouldn't know anything about the metaboxes, I don't know what this PR is trying to do exactly but we should try to hook into the saving mechanism from the edit-post package instead.

This comment has been minimized.

@youknowriad

youknowriad Nov 2, 2018

Contributor

I think you probably want to tweak this condition instead https://github.com/WordPress/gutenberg/blob/master/packages/edit-post/src/store/effects.js#L53

(Curious about the impact on revisions of all these save requests though)

This comment has been minimized.

@adamsilverstein

adamsilverstein Nov 4, 2018

Contributor

I don't know what this PR is trying to do exactly

@youknowriad
This PR is attempting to save metaboxes when a preview is triggered. That is, when the preview button is clicked, we save meta before displaying the preview so updated meta values will be included.

This comment has been minimized.

@adamsilverstein

adamsilverstein Nov 4, 2018

Contributor

I think you probably want to tweak this condition instead

Yes thanks for the suggestion... I looked at that originally without much luck because the previewing "condition" is not being tracked. It seemed more straightforward to add metabox saving as an effect of clicking the preview button only and code wise it is maybe simpler.

to move the logic into SET_META_BOXES_PER_LOCATIONS in effects, i think we need to track isPreviewing to know this is a preview autosave not a regular autosave - we don't want to save metabox meta for regular autosaves, only when the user clicks preview - does that sound right? or should i instead try to add a variable to the autosave call, eg autosave( true ) would be a preview autosave?.

(Curious about the impact on revisions of all these save requests though)

no revisions are stored for meta currently in core - the meta is saved directly to the post (unfortunately). only one autosave revision is saved per post (per user).

This comment has been minimized.

@youknowriad

youknowriad Nov 14, 2018

Contributor

Why the preview autosave is not just a regular autosave? Is there anything different in the backend or is this the actual different, the need to not trigger ant autosave but an actual save including metaboxes?

  • Could we just trigger a regular save (instead of autosave) when previewing drafts?
  • What should happen if we preview a published post and preview is clicked, should we save the metaboxes too? it seems like an unwanted changed could be published inadvertently?
  • If it's only for drafts, then I'd be fine adding a flag tracking that we're previwing.

This comment has been minimized.

@adamsilverstein

adamsilverstein Nov 14, 2018

Contributor

Why the preview autosave is not just a regular autosave?

to match core behavior:

regular autosaves do not include saving post meta - they only save the title, excerpt and content. previews also save all meta so that previews can include changes made in meta fields which may affect how the post displays. This fixes the issue raised in #11280. it also re-introduces the issue raised in https://core.trac.wordpress.org/ticket/20299.

I would love to work on a more complete solution - - perhaps as part of phase 2 - that would add meta revisioning to the autosave, so meta values would be associated with the autosave instead of the parent post, which would fix the unwanted saving of meta values to published posts when previewing. The code to achieve this is already available in a patch on the ticket and relies on my Post Meta Revisioning plugin.

Could we just trigger a regular save (instead of autosave) when previewing drafts?

Yes, probably - however that might require more refactoring of how the preview loads its data?

What should happen if we preview a published post and preview is clicked, should we save the metaboxes too? it seems like an unwanted changed could be published inadvertently?

Absolutely right - this is a tricky situation, if we save the metaboxes unwanted changes could be saved, if we don't save the metaboxes we risk the preview not showing the correct information. That is why adding meta revisioning and using that for the preview makes so much sense - it solves both problems. Short of that, we probably want to match the existing core behavior despite the possibility of unwanted data being saved. Since this is such a long standing and well documented bug it has (sadly) become the expected state of affairs.

If it's only for drafts, then I'd be fine adding a flag tracking that we're previwing.

its for drafts and published posts, both need to save meta when previewing.

This comment has been minimized.

@youknowriad

youknowriad Nov 14, 2018

Contributor

its for drafts and published posts, both need to save meta when previewing.

Gotcha, this means you're right that we need to distinguish the "preview save" from an "autosave" in the state to be able to trigger the metaboxes save in edit-post.

This comment has been minimized.

@adamsilverstein

adamsilverstein Nov 15, 2018

Contributor

👍 will try to refactor as you suggest

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 8, 2018

I'd love to get this in but it doesn't seem ready for 4.3.

@youknowriad youknowriad modified the milestones: 4.3, 4.4 Nov 8, 2018

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Nov 9, 2018

@adamsilverstein Still able to own this through to completion, or do you need to hand it off?

@lkraav

This comment has been minimized.

lkraav commented Nov 13, 2018

Curious, is post meta saved only to the Preview / autosave revision? If you close the editor without Publish / Update, does live post meta retain their original values?

EDIT OK this might be answered already

no revisions are stored for meta currently in core - the meta is saved directly to the post (unfortunately). only one autosave revision is saved per post (per user).

#11409 (comment)

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Nov 14, 2018

I don't know what this PR is trying to do exactly but we should try to hook into the saving mechanism from the edit-post package instead.

On a meta note, it would be neat if linting caught this. Do you think this is technically feasible?

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 14, 2018

On a meta note, it would be neat if linting caught this. Do you think this is technically feasible?

It should be possible with a custom linting rule, we'd have to check for package.json dependencies and have a mapping between package name and store name.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Nov 14, 2018

On a meta note, it would be neat if linting caught this. Do you think this is technically feasible?

It should be possible with a custom linting rule, we'd have to check for package.json dependencies and have a mapping between package name and store name.

Great. Created an issue with #11865

@danielbachhuber danielbachhuber self-requested a review Nov 14, 2018

@youknowriad youknowriad removed this from the 4.4 milestone Nov 15, 2018

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 19, 2018

This doesn't seem to work for me, not sure what's going wrong here. Also question: this doesn't save the featured image, categories ... Are these supposed to be fixed in another issue?

@adamsilverstein

This comment has been minimized.

Contributor

adamsilverstein commented Nov 19, 2018

This doesn't seem to work for me, not sure what's going wrong here. Also question: this doesn't save the featured image, categories ... Are these supposed to be fixed in another issue?

Thanks for testing. I'll dig in to see if I can tell what is happening. This addresses meta boxes so far. Featured image is being handles separately in #12037. I didn't realized categories/tags weren't included here, we should add them. One potential issue is I think the post endpoint accepts these directly, I'm not sure the autosave endpoint does.

*
* @return {boolean} Whether the post is being previewed.
*/
export function isPreviewingPost( state ) {

This comment has been minimized.

@mtias

mtias Nov 19, 2018

Contributor

It's slightly weird that preview would have this side-effect but it wouldn't be evident from selector name. Though I guess it matches the expectation of the action itself.

@youknowriad youknowriad force-pushed the fix/save-meta-on-preview branch from 5fb123c to 74035ca Nov 20, 2018


// Once popup redirect is evaluated, even if already closed, delete
// reference to avoid later assignment of location in post update.
delete this.previewWindow;

This comment has been minimized.

@youknowriad

youknowriad Nov 20, 2018

Contributor

@aduth I removed this to be able to refresh the window without an autosave (once the metaboxes finish saving). I'm not aware of any downsides but let me know if I missed something.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 20, 2018

I pushed some updates here, It's working better now.

youknowriad added some commits Nov 20, 2018

@youknowriad

LGTM 👍 But a second review would be good as I did work on this.

<PostPreviewButton />
<PostPreviewButton
forceIsAutosaveable={ hasActiveMetaboxes }
forcePreviewLink={ isSaving ? null : undefined }

This comment has been minimized.

@gziolo

gziolo Nov 20, 2018

Member

It could be refactored to:

forcePreviewLink={ isSaving }

and then we only would need to update the code later in the execution flow to:

previewLink: forcePreviewLink ? null : getAutosaveAttribute( 'preview_link' ),

This comment has been minimized.

@youknowriad

youknowriad Nov 20, 2018

Contributor

it can't because we're forcing the null value from outside. The inner component don't know what value to put unless we rename the prop something like forceEmptyPreviewLink

! wasAutosavingPost &&
! isSavingPost &&
! isAutosavingPost
hasActiveMetaBoxes && (

This comment has been minimized.

@gziolo

gziolo Nov 20, 2018

Member

I hope it works 🙏 😅

* @return {boolean} Whether the post is being previewed.
*/
export function isPreviewingPost( state ) {
return isSavingPost( state ) && !! state.saving.options.isPreview;

This comment has been minimized.

@gziolo

gziolo Nov 20, 2018

Member

Nit: I don't think we need !! in this condition. If we need it, we should probably be consistent and add it in the previous selector that is updated in this PR.

This comment has been minimized.

@youknowriad

youknowriad Nov 20, 2018

Contributor

Updated. it's better to have it to ensure the return value is always a boolean.

youknowriad added some commits Nov 20, 2018

@gziolo

gziolo approved these changes Nov 20, 2018

It works as expected in my testing using the snippet shared by @danielbachhuber in #11280 (comment).

@youknowriad youknowriad merged commit 2945a62 into master Nov 20, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@gziolo gziolo deleted the fix/save-meta-on-preview branch Nov 20, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Save meta on preview (WordPress#11409)
* Set the preview url once metabox and autosaves are complete.

* when previewing, requestMetaBoxUpdates

* move preview autosave logic into edit-post effects

* update docs & save logic

* Fix save metaboxes condition

* Refresh preview after metabox save

* Fix consecutive previews

* Fix unit tests

* Fix autosaving status

* Always return a boolean value from isAutosave selector.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment