-
Notifications
You must be signed in to change notification settings - Fork 812
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
Portfolio / Testimonials: on block themes remove settings and menu items when not in use #41714
Portfolio / Testimonials: on block themes remove settings and menu items when not in use #41714
Conversation
…ith no current usage
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 4 files.
Full summary · PHP report · JS report Add label
I don't care about code coverage for this PR
|
Code Coverage SummaryCoverage changed in 4 files.
Full summary · PHP report · JS report Coverage check overridden by
I don't care about code coverage for this PR
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing on WoA:
The theme I had previously enabled was Lodestar. I could see both Testimonials and Portfolio after enabling the branch for both Jetpack and WPcom Features plugins.
I switched to Twenty Twenty-Four after trashing all my testimonials (no Portfolio projects) but now I only see the Testimonials under Custom content types:
Testing on Simple
Similar behaviour. The theme I had previously enabled was Hola.
I trashed all my Testimonials and Portfolio projects.
I then switched to Twenty Twenty-Four but I can only see Testimonials available in Settings->Writing:
Is this expected? I guess I should also see Portfolio, right?
Thanks for testing! It would seem the Testimonials were still displaying due to stored transients, assuming they'd been published before being trashed. However I did more testing and couldn't locate the transients when testing on a Jurassic Ninja site, so couldn't think of a different and reliable way to store a temporary value for the count, vs testing locally where I was able to modify the transients in the database. I decided to pare things back to what was made in an original suggestion (P2 comment pfwV0U-ik-p2#comment-387), and only keep the feature enabled when switching to a block theme if it was already enabled, disregarding any previously published testimonials or portfolios. |
projects/packages/classic-theme-helper/src/custom-post-types/class-jetpack-testimonial.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚢
Great work, Karen! Tested on Simple and WoA and all seems to work as described in the test instructions. Also thanks for baring with me and explaining the expected functionality in detail!
Fixes https://github.com/Automattic/vulcan/issues/512
Proposed changes:
or either already have published posts. With the latter check, this relies on a transient, so is useful only as a temporary fallback for any accidental toggle switches.classic_theme_helper_should_display_portfolios
andclassic_theme_helper_should_display_testimonials
so that on a site by site basis the display of testimonial or portfolio settings can be overridden.Code coverage note: Nothing in the Classic Theme Helper package has any existing tests. I'll add an internal issue to look at this.
Other information:
Jetpack product discussion
https://github.com/Automattic/vulcan/issues/512
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
To test, on a self-hosted site testing locally or using the beta tester plugin, or an WoA site using the beta tester plugin and both branches (with just the Jetpack Mu Wpcom plugin using the branch, behavior should be as before). Also on Simple, using the commands in the generated comment below, for a sandboxed test site.
/wp-admin/admin.php?page=jetpack#writing
for WoA and self-hosted only, or Settings > Writing -/wp-admin/options-writing.php
)Now make sure you convert a testimonial or portfolio item to be published , then turn both testimonial and portfolios of with the toggles on either settings page. On WoA / Simple you may need to switch to a theme such as Twenty Fifteen as you won't be able to toggle off / on a CPT if the theme supports it.Switching back to a block based theme, you should now see the custom post type (testimonials or portfolios) as a toggle-able option on either settings page. As a disclaimer though, the 'count' functionality that checks if there are published testimonials or portfolios relies on a transient if that post type is not active. As such it isn't 100% reliable but is meant as a safety net if someone on a block theme accidentally toggles off portfolios or testimonials with published items in either.If you have access to run SQL commands on a test site, you can run 'DELETE FROMwp_options
WHEREoption_name
LIKE ('_transient_jetpack-testimonial-count-cache');' and 'DELETE FROMwp_options
WHEREoption_name
= '_transient_timeout_jetpack-testimonial-count-cache';' (and the same for portfolios) if you want to test behavior without the transients.To test the filter: