-
Notifications
You must be signed in to change notification settings - Fork 799
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
Plan Checks: store less info locally and simplify logic #38937
base: trunk
Are you sure you want to change the base?
Conversation
Do not attempt to check if a block is available on WoA. We consider that they all are, out of the box. It simplifies a logic, improves performance, and avoids relying on the "available" part of the plan information stored locally in the "jetpack_active_plan" option. This will allow us to remove that part from the local option, since it isn't used elsewhere. Related conversation: p55Cj4-3q6-p2#comment-3642
Until now, the "jetpack_active_plan" option included everything that was retrieved from the API. That made for a really big option, with a lot of data that we do not actually use in the codebase today (namely, the "available" part of the feature list). With this commit, we remove that array from the data we store in the local option. Related conversation: p55Cj4-3q6-p2#comment-3642
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. |
@sbarbosa I'd like to ask you about the Blaze app. I see we pass this information to be used in the Blaze app:
However, I'm not sure where this is used in the app: Do you rely on |
It seems that some WoA sites, namely eCommerce trial sites, do not support all blocks so we need to check for that. Related conversation: p55Cj4-3q6-p2#comment-3667
@jeherve , I checked, and that features list is not used inside the Blaze dashboard. It is there because we based our code on the Stats Admin module (the stats app). I tested it, and this PR won't affect how Blaze works. We are not actively checking for the features on our page or in the components we use in our module. But, the It is used in other places, but it's hard to check if a rendered page like Blaze or the stats page uses those components. Let me tag @kangzj to see if this change may affect the stats app. (That app also passes the list of features to the Calypso app: Code link) |
Thanks for taking a look!
This should be fine. We're not making changes to the endpoint, just to the locally stored data.
That's a good point. I don't see |
I went looking for places that available features might be used and also found this: jetpack/projects/plugins/jetpack/_inc/client/my-plan/index.jsx Lines 40 to 42 in 66515fb
But I'm not sure if this would be impacted. |
This queries the jetpack/projects/plugins/jetpack/_inc/lib/core-api/class.jetpack-core-api-site-endpoints.php Line 39 in df21dd6
It must indeed be related to that. The VideoPress block was added as a feature available on free Jetpack plans in #28391 and D98224-code, but I don't know if we were supposed to have a free VideoPress tier on WordPress.com sites. Right now, on the Jetpack end that plan is supposed to behave like a Business site though:
@Automattic/jetpack-agora Do y'all know a bit more about this? Should the VideoPress block be available on free Atomic sites like trial eCommerce sites, or sites being migrated? I see some logic was added in D115819-code, but I'm not clear what the behavior is supposed to be for VideoPress and its block? |
Not much, sorry to say.
Off the top of my head, I'd say VideoPress block should be always available and manage its own restrictions for the "1 free upload" tier. That said, I see no reason for VideoPress not to be available on trial eCommerce sites? For sites being migrated I think the files are likely to be lost in the process (not lost, but remain attached to the old site?), need to confirm this though. Just mentioning it in case it makes it easier for us to make a decision on whether to make it available or not. |
Sorry for the late response. I looked up the Calypso codebase. It seems there's only one place that references the But I don't think Odyssey Stats is using it, so I can confirm removing it won't affect Stats. If Blaze is not using the selector, then I guess it'll be good as well. |
Proposed changes:
Until now, the
jetpack_active_plan
option included everything that was retrieved from the API. That made for a really big option, with a lot of data that we do not actually need to use in the codebase today (namely, theavailable
part of the feature list).With this PR, we remove that array from the data we store in the local option.
Of note, we were using the
available
part of the feature list in one place: starting in #19762, we introduced logic that would allow us to display an upgrade banner in the block editor, when site owners would try to add a block that was not available with their plan. The banner would tell in what plan the feature was available.That logic relied on the
available
array from thejetpack_active_plan
option, only to check for eligibility on WoA sites.Since WoA sites currently include all blocks, we do not currently need to gate access to any blocks on WoA, and thus we can remove that logic.Some WoA sites, namely eCommerce trial sites, do not support all blocks so we need that logic to remain. We should, however, be able to rely on the
get_minimum_plan_for_feature
method for WoA sites as well.Warning
Before we merge this, we want to be absolutely sure
available
isn't used anywhere.Other information:
Jetpack product discussion
See discussion here: p55Cj4-3q6-p2#comment-3642
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Note
I would recommend testing this on all 3 environments: WordPress.com Simple, WoA, and self-hosted.
Since it makes changes to data that is used across the products, I would also recommend diving through the different usages of the
Current_Plan
class in the monorepo, as well as usages of thejetpack_active_plan
option.