Hide Stories options and add deprecation notice #4219

pierlon · 2020-02-04T08:04:06Z

Summary

Stories experience enabled

The following table summarizes what happens when the Stories experience is enabled:

‏‏‎ ‎	With `amp_story` posts	Without `amp_story` posts
Story posts can be managed	✔️	❌
Stories settings are available	✔️	❌
Notice shown on AMP settings page

NB: In the case with existing amp_story posts, the deprecated notice is also shown on the Stories post list page:

And also in the Story editor:

And an admin pointer on non-story screens, when the Stories experience is enabled:

Stories experience disabled

The following table summarizes what happens when the Stories experience is disabled:

‏‏‎ ‎	With `amp_story` posts	Without `amp_story` posts
Story posts can be managed	❌	❌
Stories settings are available	✔️	❌
Notice shown on AMP settings page

Fixes #4201.
Fixes #4202.

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

includes/options/class-amp-options-manager.php

kienstra · 2020-02-05T01:58:15Z

It looks like this also fixes #4202, right?

As part of this, we need to notify users as soon as possible that they need to backup or export stories they have created since they are not likely to be automatically migrated to the new standalone plugin.

pierlon · 2020-02-05T04:10:10Z

Nope. The notice here is only applied to the AMP settings screen and is shown irregardless of Stories being enabled or not.

Should this PR also fix #4202, @westonruter?

kienstra · 2020-02-05T04:23:07Z

Ah, if this PR doesn't fix #4202, I don't think this PR needs to fix it.

westonruter · 2020-02-05T05:18:54Z

The entire experiences selection should be omitted entirely now:

So this code should not be run if Stories is not available:

amp-wp/includes/options/class-amp-options-menu.php

Lines 82 to 91 in 19d54bd

    
           add_settings_field( 
        
           	'experiences', 
        
           	__( 'Experiences', 'amp' ), 
        
           	[ $this, 'render_experiences' ], 
        
           	AMP_Options_Manager::OPTION_NAME, 
        
           	'general', 
        
           	[ 
        
           		'class' => 'experiences', 
        
           	] 
        
           );

westonruter · 2020-02-05T05:24:56Z

So this code should not be run if Stories is not available:

I mean, if no amp_story posts exist, then the experiences section checkbox should be hidden/omitted.

westonruter · 2020-02-05T05:26:33Z

Should this PR also fix #4202

Yes.

westonruter · 2020-02-05T05:28:09Z

~~So this PR can also fix #4203.~~

westonruter · 2020-02-05T05:29:45Z

Rather, the code should not yet be removed, but the experiences can be omitted. Issue #4203 would need to be fixed in a subsequent PR which removes all stories code.

includes/options/class-amp-options-manager.php

pierlon · 2020-02-05T08:14:38Z

I've also added the warning notice to the Story editor:

includes/options/class-amp-options-manager.php

westonruter · 2020-02-06T01:30:48Z

Please also link the Story export text to https://amp-wp.org/documentation/amp-stories/exporting-stories/

includes/options/class-amp-options-manager.php

westonruter

Another important change needed in this PR: if I have amp_story posts in my database and I have the stories experience enabled, if I then turn off stories the experience options should not all of a sudden disappear. I need to be able to re-enable the Stories experience to be able to export the stories if need be.

includes/options/class-amp-options-manager.php

westonruter · 2020-02-07T00:04:14Z

includes/options/class-amp-options-manager.php

+			printf(
+				'%s %s %s',
+				esc_html__( 'The Stories experience is being extracted from the AMP plugin into a separate standalone plugin which will be available soon. Please backup or', 'amp' ),
+				sprintf(
+					'<a href="%s">%s</a>',
+					esc_url( 'https://amp-wp.org/documentation/amp-stories/exporting-stories/' ),
+					esc_html__( 'export your existing Stories', 'amp' )
+				),
+				esc_html__( 'as they will not be available in the next version of the AMP plugin.', 'amp' )
+			);
+			echo '</p></div>';


Please refactor the warning to match the warning in the editor:

This will reduce the strings to translate.

Aside: Breaking up the strings in this way would make it hard to translate in some languages.

Good point, made the change in 8c14de5.

Co-Authored-By: Weston Ruter <westonruter@google.com>

…` posts Fixes #4202.

Co-Authored-By: Weston Ruter <westonruter@google.com>

pierlon · 2020-02-09T00:54:54Z

⚠️ Rebasing with latest changes from develop to resolve merge conflicts.

pierlon · 2020-02-09T01:45:30Z

The following can be observed when the Stories experience is disabled while editing a Story and refreshing the page:

Along with the following stack-trace:

[08-Feb-2020 23:55:26 UTC] PHP Notice:  map_meta_cap was called <strong>incorrectly</strong>. The post type amp_story is not registered, so it may not be reliable to check the capability "edit_post" against a post of that type. Please see <a href="https://wordpress.org/support/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 4.4.0.) in /app/public/core-dev/src/wp-includes/functions.php on line 5166
[08-Feb-2020 23:55:26 UTC] PHP Stack trace:
[08-Feb-2020 23:55:26 UTC] PHP   1. {main}() /app/public/core-dev/src/wp-admin/admin-ajax.php:0
[08-Feb-2020 23:55:26 UTC] PHP   2. do_action() /app/public/core-dev/src/wp-admin/admin-ajax.php:175
[08-Feb-2020 23:55:26 UTC] PHP   3. WP_Hook->do_action() /app/public/core-dev/src/wp-includes/plugin.php:478
[08-Feb-2020 23:55:26 UTC] PHP   4. WP_Hook->apply_filters() /app/public/core-dev/src/wp-includes/class-wp-hook.php:311
[08-Feb-2020 23:55:26 UTC] PHP   5. wp_ajax_wp_remove_post_lock() /app/public/core-dev/src/wp-includes/class-wp-hook.php:287
[08-Feb-2020 23:55:26 UTC] PHP   6. current_user_can() /app/public/core-dev/src/wp-admin/includes/ajax-actions.php:2816
[08-Feb-2020 23:55:26 UTC] PHP   7. WP_User->has_cap() /app/public/core-dev/src/wp-includes/capabilities.php:668
[08-Feb-2020 23:55:26 UTC] PHP   8. map_meta_cap() /app/public/core-dev/src/wp-includes/class-wp-user.php:749
[08-Feb-2020 23:55:26 UTC] PHP   9. _doing_it_wrong() /app/public/core-dev/src/wp-includes/capabilities.php:161
[08-Feb-2020 23:55:26 UTC] PHP  10. trigger_error() /app/public/core-dev/src/wp-includes/functions.php:5166

I don't think this needs to be looked into since it would be such a rare edge case, but just want it to be known.

pierlon · 2020-02-09T02:41:09Z

@westonruter how should the failing E2E tests be handled? All are related to the experiences section not being shown.

westonruter · 2020-02-09T02:47:12Z

@pierlon Just remove the offending tests. Everything will be removed with #4203 anyway.

…nter Also use 'back up' instead of 'backup' for verb.

westonruter · 2020-02-09T05:36:28Z

One minor thing that I found is that if I trash all the stories and then empty the trash, then I get wp_die() message saying the post type is invalid. This is because once there are no posts left, the post type then gets unregistered. I suppose that is fine, as most users will not try to empty their trash. And if they do, they can just go back to the WP Admin.

* Hide Stories options if no `amp_story` posts are found * Add Stories deprecation notice * Reword notice Co-Authored-By: Weston Ruter <westonruter@google.com> * Refactor how stories experience is considered enabled * Hide experiences section options if Stories not enabled * Always show the Stories deprecation notice if the user has `amp_story` posts Fixes #4202. * Show a different notice if the user has no Story posts * Show warning notice only on AMP settings screen & Stories list page * Show warning notice in Story editor * Reword deprecation notice Co-Authored-By: Weston Ruter <westonruter@google.com> * Use `wp_json_encode` to sanitize notice in editor * Add link to docs on how to export Stories * Rename 'Website Mode' to 'Template Mode' * Make action label translatable Co-Authored-By: Weston Ruter <westonruter@google.com> * Refactor Stories deprecation notice on AMP settings page * Get Story post count irregardless of the post type being registered * Handle story options only if the experience is enabled * Fix failing test * Remove failing E2E tests * Fix failing test * Add admin pointer for exporting stories; remove old Stories admin pointer Also use 'back up' instead of 'backup' for verb. * Use title case for 'Template Mode' Co-authored-by: Weston Ruter <westonruter@google.com>

* tag '1.4.3': (22 commits) Update readme and screenshots for Stories removal (#4259) Open story export instructions in a new window (#4258) Bump version to 1.4.3-RC1 Hide Stories options and add deprecation notice (#4219) Fix malformed conversion of relative action URLs for forms (#4250) Limit Stories experience to WP 5.3 & Gutenberg 7.1.0 (#4217) Prevent errors in admin bar filters from non-array arguments (#4207) Update @wordpress/e2e-test-utils dependency Revert update of mustache/mustache dependency Update composer.lock Update WP CLI to 2.4.0 For WordPress.tv embed, Use an oembed filter instead of block filter (#4164) Update readme to add FAQs section (#4159) Apply workaround to fix test__multiple_valid_image_files (#4034) Ignore Story editor tests (#4043) Update amp-video embed regex pattern to include other Vimeo URL formats (#4051) Update amp-instagram embed regex (#4053) Update wp-dev-lib package (#4029) Fix conversion of forms with relative action URLs (#4003) Improve release instructions (#3995) ...

googlebot added the cla: yes Signed the Google CLA label Feb 4, 2020

westonruter requested changes Feb 4, 2020

View reviewed changes

includes/options/class-amp-options-manager.php Outdated Show resolved Hide resolved

includes/options/class-amp-options-manager.php Outdated Show resolved Hide resolved

includes/options/class-amp-options-manager.php Outdated Show resolved Hide resolved

pierlon requested a review from westonruter February 5, 2020 04:13