feat: metered content countdown banner #4315
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,249 @@ | ||||||||||||
| <?php | ||||||||||||
| /** | ||||||||||||
| * WooCommerce Content Gate Metering. | ||||||||||||
| * | ||||||||||||
| * @package Newspack | ||||||||||||
| */ | ||||||||||||
|
|
||||||||||||
| namespace Newspack; | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * WooCommerce Content Gate Metering class. | ||||||||||||
| */ | ||||||||||||
| class Metering_Countdown { | ||||||||||||
|
|
||||||||||||
| const OPTION_PREFIX = 'np_countdown_banner_'; | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Initialize hooks. | ||||||||||||
| */ | ||||||||||||
| public static function init() { | ||||||||||||
| add_action( 'wp_enqueue_scripts', [ __CLASS__, 'enqueue_assets' ] ); | ||||||||||||
| add_action( 'wp_footer', [ __CLASS__, 'print_cta' ] ); | ||||||||||||
| add_filter( 'body_class', [ __CLASS__, 'filter_body_class' ] ); | ||||||||||||
| add_filter( 'newspack_ads_placement_data', [ __CLASS__, 'filter_ads_placement_data' ], 10, 2 ); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Get all settings with default values for the countdown banner. | ||||||||||||
| * | ||||||||||||
| * @return array Default countdown settings. | ||||||||||||
| */ | ||||||||||||
| public static function get_default_settings() { | ||||||||||||
| return [ | ||||||||||||
| 'enabled' => false, | ||||||||||||
| 'style' => 'light', | ||||||||||||
| 'cta_label' => __( 'Subscribe now and get unlimited access.', 'newspack' ), | ||||||||||||
| 'button_label' => __( 'Subscribe now', 'newspack' ), | ||||||||||||
|
||||||||||||
| 'cta_label' => __( 'Subscribe now and get unlimited access.', 'newspack' ), | |
| 'button_label' => __( 'Subscribe now', 'newspack' ), | |
| 'cta_label' => __( 'Subscribe now and get unlimited access.', 'newspack-plugin' ), | |
| 'button_label' => __( 'Subscribe now', 'newspack-plugin' ), |
Outdated
There was a problem hiding this comment.
The @return documentation states it returns an array but doesn't specify what type of value it could return when a $key parameter is provided. When $key is provided, it returns a single value (mixed type) from get_option(), not an array. The documentation should reflect both return types: @return array|mixed Array of all settings, or a single setting value if $key is provided.
| * @return array Countdown settings. | |
| * @return array|mixed Array of all settings, or a single setting value if $key is provided. |
There was a problem hiding this comment.
Not a strong opinion about this one
There was a problem hiding this comment.
The @return documentation states it returns array|\WP_Error but the function never returns a \WP_Error. It always returns the $current_settings array. Either the documentation should be updated to only indicate @return array, or error handling should be implemented to actually return a \WP_Error when appropriate.
| * @return array|\WP_Error Updated countdown settings or error if update fails. | |
| * @return array Updated countdown settings. |
There was a problem hiding this comment.
Fixed in dbb794b to actually return an error if there is one
Outdated
There was a problem hiding this comment.
The enabled setting should be handled specially like the style setting. Using empty( $value ) on line 75 will treat boolean false as empty, which means if someone tries to disable the banner by setting enabled to false, it will instead reset to the default value (also false), but the option won't be deleted. For boolean values, explicit type checking should be used instead of empty(). Consider:
if ( $key === 'enabled' ) {
update_option( self::OPTION_PREFIX . $key, (bool) $value );
$current_settings[ $key ] = (bool) $value;
continue;
}There was a problem hiding this comment.
This can cause issues in the future, but I'm not a fan of the proposed approach. We could either enforce the $settings to always include the entire schema and do ! isset() to delete, or skip this entirely and not handle deletion in this method. If needed, a separate delete_setting() could be implemented eventually.
There was a problem hiding this comment.
Agreed, in dbb794b I removed the delete_option from this for now.
Outdated
There was a problem hiding this comment.
Missing input sanitization. The update_settings() method receives unsanitized user input from the REST API and stores it directly using update_option(). While the style field has validation (lines 72-74), the cta_label, button_label, and cta_url fields are stored without sanitization. These should be sanitized before storing:
if ( $key === 'cta_label' || $key === 'button_label' || $key === 'cta_url' ) {
$value = sanitize_text_field( $value );
}Note that similar fields in the content gifting feature are properly sanitized (see lines 649, 652, 655 in the wizard file).
| } | |
| } | |
| if ( $key === 'cta_label' || $key === 'button_label' || $key === 'cta_url' ) { | |
| $value = sanitize_text_field( $value ); | |
| } |
There was a problem hiding this comment.
This could also be implemented as a separate sanitizer function.
There was a problem hiding this comment.
dbb794b implements a separate sanitize_setting method which should handle this as well as sanitize user input before saving to the DB.
Outdated
There was a problem hiding this comment.
Inconsistent return type. The function is declared to return void (no return type specified), but on line 163 it returns an empty string return ''. This should either return nothing or the function signature should document what it returns.
| return ''; | |
| return; |
There was a problem hiding this comment.
nit: @return void in the docblock
There was a problem hiding this comment.
Updated in dbb794b—added @return void in the docblock and made sure to return nothing instead of an empty string.
There was a problem hiding this comment.
Similar to what was implemented for content gifting, I expected this link to be "Create an account" (#register_modal) if there's registration metering.
There was a problem hiding this comment.
I think for the Countdown banner, the purpose of the countdown CTA is a bit different. It's to highlight that even though you have n "free" views remaining, you can skip ahead to get unlimited access by just purchasing a membership at anytime. The "sign in" link is for users who might already have an active subscription but just aren't logged in yet.
There was a problem hiding this comment.
But if they're not logged in, they may get more content by registering. It's a lead opportunity for readers who are not willing to subscribe.
I also just noticed that this should be behind a is_user_logged_in() condiitonal
There was a problem hiding this comment.
After some discussion, added some additional logic:
- If the user is NOT logged in and metering settings allow for free views for registered users, show a "Create account" link to allow the user to get additional metered views
- If the user is NOT logged in and metering settings do NOT allow for free registered views, show a "Sign in" link for users who might already have an account with a subscription
- If the user is logged in, show nothing
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.
The file header and class docblock describe this as "WooCommerce Content Gate Metering" but this class is actually for the countdown banner feature, not WooCommerce metering. The documentation should accurately describe the class purpose as "Content Gate Metering Countdown Banner" or similar.
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.
Updated in dbb794b