Add priority-based notice bar with milestone and extrapolation messaging#1150
Add priority-based notice bar with milestone and extrapolation messaging#1150danieliser wants to merge 2 commits intodevelopfrom
Conversation
WalkthroughThe Upsell.php class undergoes refactoring to replace short array syntax ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@classes/Upsell.php`:
- Line 237: Remove the unused dead variable $pro_is_active from the method in
classes/Upsell.php: delete the assignment "$pro_is_active =
\PopupMaker\plugin()->is_pro_active();" (and any related unused references) so
the method no longer creates an unused local; if logic depended on that check,
instead reference \PopupMaker\plugin()->is_pro_active() inline where actually
needed or add a meaningful use, but if not required simply remove the
assignment.
🧹 Nitpick comments (1)
classes/Upsell.php (1)
488-500: Consider catching\Throwablefor broader error coverage.PHP 7+ can throw
\Errorin addition to\Exception. Using\Throwablewould catch both. However, since the fallback is graceful (returns 0), this is a minor concern.♻️ Optional improvement
private static function get_form_conversion_count() { try { $form_tracking = \PopupMaker\plugin( 'form_conversion_tracking' ); if ( $form_tracking && method_exists( $form_tracking, 'get_site_count' ) ) { return $form_tracking->get_site_count(); } - } catch ( \Exception $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch + } catch ( \Throwable $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch // Service not available, return 0. unset( $e ); // Prevent unused variable warning. } return 0; }
…messaging Implements comprehensive upsell enhancement system with 4 priority groups: **Priority System:** - Milestone Achievements (pri: 100) - Celebration messages for success - Conversion Extrapolation (pri: 80) - Data-driven "you could have X more" - Integration Detected (pri: 60) - Contextual plugin-based messages - Generic Upgrade (pri: 40) - Fallback messages **New Milestone Triggers:** - First form conversion celebration (🎉 with 300-500% increase stat) - High engagement tiers (1K, 5K, 10K+ popup views with analytics pitch) **New Extrapolation Messages:** - Ecommerce: "You've tracked X conversions, capture ~Y more with Exit Intent" - LMS: "You've tracked X enrollments, capture ~Y more with LMS targeting" - Uses 33% extrapolation for ecommerce, 40% for LMS **Integration:** - Connects to FormConversionTracking service for milestone data - Uses existing popup view count for engagement milestones - Maintains backward compatibility with existing integration detection **Technical Changes:** - Added get_notice_bar_triggers() with Review Request pattern - Added get_current_notice_bar_trigger() for priority selection - Added get_form_conversion_count() helper with graceful fallback - Added rsort_by_priority() for consistent sorting - Deprecated get_integration_messages(), get_pro_integration_message(), get_free_upgrade_message() - All messages use generate_upgrade_url() for UTM tracking **Impact:** - Success-triggered messaging (not frustration-based) - Data-driven value propositions with concrete numbers - Foundation for future A/B testing and message optimization
**Messaging Accuracy:** - Changed "conversions" → "form submissions" (free plugin doesn't track specific conversion types) - Changed "enrollments" → "form submissions" for LMS (specific tracking is Pro feature) - Updated translator comments to reflect accurate terminology **Milestone Cooldown:** - Added 90-day cooldown for milestone achievement messages (seasonal re-engagement) - Tracks last shown milestone via user meta: _pum_last_milestone_trigger, _pum_last_milestone_at - Prevents showing same milestone repeatedly within cooldown period - Allows different milestones to show (e.g., first form → high engagement) - Non-milestone messages (integration detected, generic upgrade) unaffected **Impact:** - More accurate messaging that doesn't over-promise on free plugin tracking - Quarterly milestone re-engagement prevents message fatigue - Natural progression: celebration → extrapolation → generic
danieliser
force-pushed
the
enhanced-notice-bar
branch
from
68c51ed to
0f210dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@classes/Upsell.php`:
- Around line 19-23: The file uses old array() syntax in the static hook
registrations (calls to add_filter/add_action) which triggers PHPCS
Generic.Arrays.DisallowLongArraySyntax; replace array( __CLASS__, 'addon_tabs' )
and similar occurrences (array( __CLASS__, 'popup_promotional_fields' ), array(
__CLASS__, 'theme_promotional_fields' ), array( __CLASS__, 'notice_bar_display'
)) with the short-array callable syntax [ __CLASS__, 'methodName' ] throughout
the Upsell class, or run vendor/bin/phpcbf classes/Upsell.php to auto-fix;
ensure all other array() usages in this file are converted to [] as well.
🧹 Nitpick comments (2)
classes/Upsell.php (2)
205-206: Side effect in getter method: consider separating read from write.
get_current_notice_bar_trigger()updates user meta as a side effect when returning a milestone trigger. This could cause unexpected behavior if the method is called multiple times or in contexts where side effects aren't expected (e.g., testing, previews).Consider separating the "get trigger" logic from "record that milestone was shown" into distinct methods, or document this behavior clearly in the method's docblock.
493-496: Minor: Simplify unused exception handling.Instead of
unset($e)to suppress the warning, you can avoid assigning the exception entirely:♻️ Suggested simplification
- } catch ( \Throwable $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch - // Service not available, return 0. - unset( $e ); // Prevent unused variable warning. - } + } catch ( \Throwable $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch -- Service not available, return 0. + }
| add_filter( 'views_edit-popup', array( __CLASS__, 'addon_tabs' ), 10, 1 ); | ||
| add_filter( 'views_edit-popup_theme', array( __CLASS__, 'addon_tabs' ), 10, 1 ); | ||
| add_filter( 'pum_popup_settings_fields', array( __CLASS__, 'popup_promotional_fields' ) ); | ||
| add_filter( 'pum_theme_settings_fields', array( __CLASS__, 'theme_promotional_fields' ) ); | ||
| add_action( 'in_admin_header', array( __CLASS__, 'notice_bar_display' ) ); |
There was a problem hiding this comment.
CI failure: PHPCS requires short array syntax [] throughout.
The pipeline is failing with 65 errors because the project's PHPCS configuration enforces Generic.Arrays.DisallowLongArraySyntax. This PR uses array() syntax, but short array syntax [] is required.
The error message indicates this can be auto-fixed:
vendor/bin/phpcbf classes/Upsell.phpAlternatively, revert to short array syntax manually throughout the file.
🧰 Tools
🪛 GitHub Actions: CI - Code Quality
[error] 19-687: PHPCS: Short array syntax must be used to define arrays (Generic.Arrays.DisallowLongArraySyntax.Found). 65 errors detected. The run exited with code 2. Can be auto-fixed with PHPCBF.
🪛 GitHub Check: PHP 8.2 - Code Quality
[failure] 23-23:
Short array syntax must be used to define arrays
[failure] 22-22:
Short array syntax must be used to define arrays
[failure] 21-21:
Short array syntax must be used to define arrays
[failure] 20-20:
Short array syntax must be used to define arrays
[failure] 19-19:
Short array syntax must be used to define arrays
🤖 Prompt for AI Agents
In `@classes/Upsell.php` around lines 19 - 23, The file uses old array() syntax in
the static hook registrations (calls to add_filter/add_action) which triggers
PHPCS Generic.Arrays.DisallowLongArraySyntax; replace array( __CLASS__,
'addon_tabs' ) and similar occurrences (array( __CLASS__,
'popup_promotional_fields' ), array( __CLASS__, 'theme_promotional_fields' ),
array( __CLASS__, 'notice_bar_display' )) with the short-array callable syntax [
__CLASS__, 'methodName' ] throughout the Upsell class, or run vendor/bin/phpcbf
classes/Upsell.php to auto-fix; ensure all other array() usages in this file are
converted to [] as well.
1 participant
Summary
Implements comprehensive upsell enhancement system with intelligent priority-based trigger selection inspired by the Review Request module pattern.
Priority System
4 Priority Groups (highest to lowest):
New Milestone Triggers
New Extrapolation Messages
Milestone Cooldown System
_pum_last_milestone_trigger,_pum_last_milestone_atTechnical Implementation
get_notice_bar_triggers()- Priority-based trigger definition systemget_current_notice_bar_trigger()- Intelligent trigger selection with cooldown logicget_form_conversion_count()- Helper with graceful fallbackrsort_by_priority()- Consistent priority sortinggenerate_upgrade_url()for UTM trackingImpact
Testing Scenarios
Files Changed
classes/Upsell.php- Complete refactor of message generation system (2 commits)Commits
b283d213- feat: add priority-based notice bar with milestone and extrapolation messaging68c51ed5- fix: improve messaging accuracy and add milestone cooldown system🤖 Ready for CodeRabbit review
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.