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

Site Editor: Revert templates to their original theme-provided files #27208

Closed
wants to merge 9 commits into from

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Nov 23, 2020

Description

Fixes #23421

  • Create a new core/edit-site action that reverts a template to its original theme-provided file.
  • Add a new "Revert" button in the Site Editor's document action for revertable templates.

WIP Notes

Technical Notes

"Reverting a template" means:

  • Delete the current template.
  • Let the templates sync run behind the scenes.
  • The sync eventually will notice that a template is missing, and will create a new auto-draft.
  • Get all auto-draft templates for the current theme, with the same slug of the deleted template.
  • Pick the first (typically the only one), and set it in the Site Editor context.

"Revertable template" indicates a template that:

  • Is not auto-draft.
  • Belongs to the current theme (its wp_theme taxonomy includes the current theme's slug).
  • Is _wp_file_based (its wp_theme taxonomy includes _wp_file_based).

Edge Case Issue (see 19951bd)

This approach relies on the templates sync that runs automatically whenever the site detects differences between the theme-provided file templates and the template posts.
As far as I understand, this should reliably work in all scenarios I could think of, except in one.

What if the theme used to provide a file template and it doesn't anymore?

For example, let's say a theme at version 1.0 provided an index template.
The sync creates an index auto-draft and tags it as _wp_file_based.
The user customizes that index template.
Then, the theme updates to version 2.0, dropping support for the index template by removing the index template file.

The index template post is still tagged as _wp_file_based, but it doesn't have an original file to revert to.

My first idea was to revert to the first revision of the template post, but as it turns out, auto-draft don't have revisions.
In other word, the first revision of a custom template would be the first time the user customized it.

Another idea could be to update the templates endpoint for _wp_file_based templates to check if the theme still has their original file to revert to.
I'm going to explore this, and see how complicated it would be.

How has this been tested?

To simplify this test, set up your site like this:

  • Use the 2021 Blocks theme.
  • Directly edit the Front Page autodraft (/wp-admin/edit.php?post_type=wp_template&post_status=auto-draft).
    Change its title in something like "CUSTOM FRONT PAGE", and make sure it's published.

Then:

  • Open the Site Editor.
  • Make sure it's showing the "CUSTOM FRONT PAGE".
  • Open the document actions (in the editor header), and click on "Revert".
  • Wait a little bit.
  • Make sure the editor is now showing the "Front Page" template (not custom).
  • Navigate to the templates CPT list (/wp-admin/edit.php?post_type=wp_template).
  • Check that the custom template is in Trash.
  • Check that there is a new Front Page template in Auto-Drafts (and make sure it's the only one!).

Harder test:

  • Pick an auto-draft template and customize it.
  • Make sure there is a "Revert" button in the document actions. (Don't revert!)
  • Head into the source files of the theme, and navigate to /wp-content/themes/THEME_NAME/block-templates.
  • Rename or delete the template file corresponding to the template you have customized.
  • Reload the Site Editor.
  • Make sure the "Revert" button in the document actions doesn't show up anymore.

Types of changes

New feature (non-breaking change which adds functionality)

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

github-actions bot commented Nov 23, 2020

Size Change: +1.73 kB (0%)

Total Size: 1.2 MB

Filename Size Change
build/block-directory/index.js 8.72 kB +9 B (0%)
build/block-editor/index.js 134 kB +369 B (0%)
build/block-editor/style-rtl.css 11.3 kB -26 B (0%)
build/block-editor/style.css 11.3 kB -25 B (0%)
build/block-library/editor-rtl.css 8.96 kB -4 B (0%)
build/block-library/editor.css 8.96 kB -2 B (0%)
build/block-library/index.js 148 kB +340 B (0%)
build/blocks/index.js 48.1 kB +19 B (0%)
build/components/index.js 172 kB +48 B (0%)
build/core-data/index.js 14.8 kB +2 B (0%)
build/data-controls/index.js 828 B +1 B
build/data/index.js 8.8 kB +4 B (0%)
build/date/index.js 11.2 kB -1 B
build/dom/index.js 4.95 kB +35 B (0%)
build/edit-navigation/index.js 11.1 kB -38 B (0%)
build/edit-post/index.js 306 kB +57 B (0%)
build/edit-post/style-rtl.css 6.42 kB -36 B (0%)
build/edit-post/style.css 6.4 kB -35 B (0%)
build/edit-site/index.js 24.6 kB +989 B (4%)
build/edit-site/style-rtl.css 3.93 kB +72 B (1%)
build/edit-site/style.css 3.93 kB +71 B (1%)
build/edit-widgets/index.js 26.3 kB -105 B (0%)
build/editor/index.js 43.3 kB -24 B (0%)
build/format-library/index.js 6.87 kB +3 B (0%)
build/list-reusable-blocks/index.js 3.1 kB +1 B
build/media-utils/index.js 5.32 kB +1 B
build/redux-routine/index.js 2.84 kB +1 B
build/reusable-blocks/index.js 2.92 kB +1 B
build/rich-text/index.js 13.3 kB -2 B (0%)
build/server-side-render/index.js 2.77 kB +1 B
build/url/index.js 4.06 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.8 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-library/style-rtl.css 8.23 kB 0 B
build/block-library/style.css 8.23 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.95 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.27 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.54 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/notices/index.js 1.81 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@Copons Copons added the [Status] In Progress Tracking issues with work in progress label Nov 23, 2020
@Copons Copons removed the [Status] In Progress Tracking issues with work in progress label Nov 24, 2020
Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

Going through these steps:

  • Use the 2021 Blocks theme.
  • Directly edit the Front Page autodraft (/wp-admin/edit.php?post_type=wp_template&post_status=auto-draft).
  • Change its title in something like "CUSTOM FRONT PAGE", and make sure it's published.
    Then:
  • Open the Site Editor.
  • Make sure it's showing the "CUSTOM FRONT PAGE".
  • Open the document actions (in the editor header), and click on "Revert".
  • Wait a little bit.

Im getting the notice that "this template is not revertable" 🤔 I have tried cleaning my environment and starting over and am still getting this message.

@Addison-Stavlo
Copy link
Contributor

@Copons
Fix incorrect theme parameter
560f903

Nice! That did it 😄

@Copons
Copy link
Contributor Author

Copons commented Nov 24, 2020

Im getting the notice that "this template is not revertable" 🤔 I have tried cleaning my environment and starting over and am still getting this message.

@Addison-Stavlo apologies, in one case I was passing the wrong parameter to the isTemplateRevertable function (it expects the theme slug/stylesheet, and I passed the full theme object instead).


Speaking of: is "revertable" the correct word?
According to random googling, it seems correct in a technical/programming environment, but it really sounds awkward to me, and I question its user-friendliness.
I'm not concerned about the utility function, but about the user-facing notices.

The "This template is not revertable" notice shows up for 2 different errors:

  1. If the isTemplateRevertable returned false (the template is not file based, or there is no original file).
  2. If, after deleting the custom template, the new auto-draft is not created (or — possibly? — the select happens before the auto-draft is created).

Now, the case 2 is not really testable, and would cause many headaches if it happens for real. Given how the sync mechanism work, it shouldn't be possible to happen, but I've still decided to add an error notice there just in case.
Maybe the notice could change into "An unexpected error occurred. Please reload."?

Case 1 could be reworded as "It's not possible to revert this template".

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

Testing works as expected! Just one question in the code.

Comment on lines 283 to 291
if ( ! fileTemplates.length ) {
yield controls.dispatch(
'core/notices',
'createErrorNotice',
__( 'This template is not revertable' ),
{ type: 'snackbar' }
);
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we can create this notice in 2 places:

  1. Prior to this step when isTemplateRevertable comes back falsy.
  2. Here after we have already sent the apiFetch to delete the template and fail to find the auto-draft to replace it with.

Should we be checking that autodraft and conditionally sending this notice before sending the apiFetch to delete?

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo Nov 24, 2020

Choose a reason for hiding this comment

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

Oh, Yeah I see why we have to do it this way and missed your recent note above:

Now, the case 2 is not really testable, and would cause many headaches if it happens for real. Given how the sync mechanism work, it shouldn't be possible to happen, but I've still decided to add an error notice there just in case.
Maybe the notice could change into "An unexpected error occurred. Please reload."

The unexpected error message may make more sense in this case. 🤔

Perhaps it would make sense to always create the auto-draft if the auto-draft doesn't exist. So the new auto-draft would be created as soon as the original one was customized and published, as opposed to right after it is deleted? Not a blocker for this PR, but a follow up if necessary.

@Addison-Stavlo
Copy link
Contributor

Speaking of: is "revertable" the correct word?

It seems correct to me. And "This template is not revertable" sounds better than "It is not possible to revert this template" from my perspective. Maybe design will have a different opinion, but updating the copy in the future could be pretty simple if there is an issue with it.

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

Approving, although noting it would probably make sense to change the phrasing on that second notice that we should never see. 🤣

@david-szabo97
Copy link
Member

david-szabo97 commented Nov 25, 2020

🛑 After clicking Revert the original Front Page is shown, but in the navigation sidebar, I still see the CUSTOM one. And I can open it.

Comment on lines 265 to 268
yield apiFetch( {
path: `/wp/v2/${ postType.rest_base }/${ template.id }`,
method: 'DELETE',
} );
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
yield apiFetch( {
path: `/wp/v2/${ postType.rest_base }/${ template.id }`,
method: 'DELETE',
} );
yield controls.dispatch(
'core',
'deleteEntityRecord',
'postType',
'wp_template',
template.id
);

Copy link
Member

Choose a reason for hiding this comment

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

Using the built-in deleteEntityRecord should take care of invalidating the cache and doing a rerequest.

Copy link
Contributor Author

@Copons Copons Nov 25, 2020

Choose a reason for hiding this comment

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

Nice catch @david-szabo97, I didn't think of checking the sidebar. ✨

Copy link
Member

@david-szabo97 david-szabo97 left a comment

Choose a reason for hiding this comment

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

LGTM 🚢 ✅

@Copons Copons added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Jan 12, 2021
@Copons
Copy link
Contributor Author

Copons commented Jan 12, 2021

Replaced by #28141

@Copons Copons closed this Jan 12, 2021
@Copons Copons deleted the add/templates-revert-to-theme-default branch January 12, 2021 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Blocked Used to indicate that a current effort isn't able to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edit Site: Add a "Revert to default" option for templates and template parts.
3 participants