Premium Analytics: add conditional widget availability API#50031
Conversation
7f32b9a to
a404aaf
Compare
|
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 1 file.
1 file is newly checked for coverage.
If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
a404aaf to
9456084
Compare
9456084 to
86e11a2
Compare
nerrad
left a comment
There was a problem hiding this comment.
I left a few comments on the AI generated prototype.
| const WIDGET_REQUIREMENTS_FILTER = 'jetpack_premium_analytics_widget_requirements'; | ||
|
|
||
| /** | ||
| * Action name for registering widget types supplied outside Premium Analytics. | ||
| * | ||
| * @var string | ||
| */ | ||
| const REGISTER_WIDGET_TYPES_ACTION = 'jetpack_premium_analytics_register_widget_types'; |
There was a problem hiding this comment.
If these are used, should maybe prefix with __experimental as an indicator for third parties to avoid using for now?
There was a problem hiding this comment.
Also, if we want to make this forward compatible with potential upstream changes, we could have the filter and action names returned by function, that way we can swap out the string to whatever the upstream filter/action hook ends up being and it should "just work".
| return array( | ||
| 'jpa/bookings-by-device' => array( | ||
| array( | ||
| 'plugin_file' => 'woocommerce-bookings/woocommerce-bookings.php', |
There was a problem hiding this comment.
This assumes a plugin is installed at a specific path - which might not always be the case.
| 'active_constant' => 'WC_BOOKINGS_VERSION', | ||
| 'version_constant' => 'WC_BOOKINGS_VERSION', |
There was a problem hiding this comment.
What's the difference between these two?
|
|
||
| /** | ||
| * Returns Premium Analytics' built-in widget availability requirements. | ||
| * |
There was a problem hiding this comment.
Should document widget requirements array.
| return array( $requirements ); | ||
| } | ||
|
|
||
| return array_values( array_filter( $requirements, 'is_array' ) ); |
There was a problem hiding this comment.
I think the fallback should be to reject the value if it's not a valid requirements array and return just the default requirements? Otherwise it seems like this could still be incorrect.
| if ( isset( $requirements['plugin_file'] ) || isset( $requirements['active_class'] ) || isset( $requirements['active_function'] ) || isset( $requirements['active_constant'] ) || isset( $requirements['version_constant'] ) ) { | ||
| return array( $requirements ); | ||
| } |
There was a problem hiding this comment.
Should every property be required?
| if ( ! is_array( $requirement ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| if ( ! empty( $requirement['plugin_file'] ) && ! is_widget_requirement_plugin_file_available( $requirement['plugin_file'] ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| if ( has_widget_requirement_active_signal( $requirement ) && ! is_widget_requirement_active_signal_met( $requirement ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| if ( empty( $requirement['active_class'] ) && empty( $requirement['active_function'] ) && empty( $requirement['active_constant'] ) && ! empty( $requirement['plugin_file'] ) && ! is_widget_requirement_plugin_active( $requirement['plugin_file'] ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| if ( ! empty( $requirement['min_version'] ) ) { | ||
| if ( empty( $requirement['version_constant'] ) || ! defined( $requirement['version_constant'] ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| return version_compare( (string) constant( $requirement['version_constant'] ), (string) $requirement['min_version'], '>=' ); | ||
| } |
There was a problem hiding this comment.
Several of the conditional checks could be eliminated here is we made the shape of the requirements config array non-optional.
| * @param array $args Widget type arguments. | ||
| * @return Widget_Type|false The registered widget type on success, or false on failure/unavailable requirements. | ||
| */ | ||
| function register_widget_type( $name, $args = array() ) { |
There was a problem hiding this comment.
Definitely something that should live as a part of the potential upstream API.
There was a problem hiding this comment.
Also, the requirements check should be in Widget_Type_Registry::register() method? Everything that is in widet-availability also seems like something that should be in the Widget_Type_Registry class.
|
Thanks @nerrad. As we know, #50002 and this PR overlap, so I've updated it. It now exposes two neutral filters in core (the PA copy of the registry that could go upstream):
Here, PA uses the registry-time filter as a consumer to keep the dev widget out of production, so it's never registered there at all (as suggested by gate-at-registration @chihsuan). The runtime filter stays available for soft cases, e.g., a paid widget shown locked. The one thing I'd push back on is declaring availability directly on the widget. A widget should carry facts ( The line I'd hold: conditioning the registration and visibility of widgets is the consumer's job, not the infrastructure's. Core just exposes the filters; PA decides the policy (dev widgets, paid widgets, whatever). The two filters split the decision cleanly: registry-time = should it exist here? (hard: dev tools in prod), runtime = how is it shown? (soft: a paid widget rendered locked). So Bookings becomes a PA/Woo filter callback; core never knows "Bookings". Your extension-registration action fits this directly. I'd just keep the I hope it does make sense. Thanks again for this PR. |
Follow-up to #49701.
Note
This is AI generated based of an idea related to how we'd handle both other plugin requirement checks before widgets are available for users. This covers two scenarios: a. Widgets we bundle in Premium Analytics (defaults), and ; b. widgets extension developers can offer.
The latter is something that will likely still need some thought because of the sync for the data, so extensibility will require much more than just exposing the widget likely.
We will need something regardless for ensuring WooCommerce widgets (and other plugins supported for launch like WooCommerce Bookings) don't show widgets that aren't available. There is related existing work in #50002 / WOOA7S-1611 for a server-side widget type availability filter; this draft explores the requirements/version-gating layer that could build on that.
Another thing to keep in mind is that there is a potential opportunity for upsells in the future that could be controlled via this API as well.
Proposed changes
register_widget_type(), which checks requirements before adding widgets toWidget_Type_Registry.jetpack_premium_analytics_widget_requirementsso Premium Analytics or extensions can add requirements for bundled or extension-owned widgets.jetpack_premium_analytics_register_widget_typesso extensions can register widget types with Premium Analytics after bundled widgets are processed.jpa/bookings-by-devicerequires the WooCommerce Bookings plugin file (woocommerce-bookings/woocommerce-bookings.php) to exist and eitherWC_BookingsorWC_BOOKINGS_VERSIONto be present at runtime. No minimum Bookings version is required yet.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
composer phpunitfromprojects/packages/premium-analytics.vendor/bin/phpcs -p projects/packages/premium-analytics/src/widget-availability.php projects/packages/premium-analytics/src/widget-types.php projects/packages/premium-analytics/tests/php/Widget_Availability_Test.php projects/packages/premium-analytics/tests/php/fixtures/widget-modules-manifest.phpfrom the repo root.pnpm --filter @automattic/jetpack-premium-analytics typecheckfrom the repo root.pnpm --filter @automattic/jetpack-premium-analytics buildfrom the repo root.