Add form conversion tracking service#1149
Conversation
WalkthroughRegisters and initializes a new FormConversionTracking service; adds the service class to record site-wide and per-popup form conversions (AJAX and non-AJAX); augments client analytics payloads with form eventData; accepts/sanitizes eventData in analytics REST; adds a telemetry comment. (33 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client (Browser)
participant SiteJS as Site JS\n(integration / pum-analytics)
participant AnalyticsAPI as PUM Analytics\nREST Endpoint
participant FormService as FormConversionTracking\nService
participant Storage as WP Options / Post Meta
Browser->>SiteJS: submit form
SiteJS->>SiteJS: collect eventData (provider,id,key,instance)
SiteJS->>AnalyticsAPI: POST conversion beacon (includes eventData, pid, event)
AnalyticsAPI->>FormService: forward event + popup_id
FormService->>FormService: validate, check duplicate
FormService->>Storage: increment site-wide count (option)
FormService->>Storage: increment popup meta count (post meta)
FormService-->>AnalyticsAPI: emit tracking action / ack
AnalyticsAPI-->>SiteJS: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes Possibly related PRs
🚥 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 |
danieliser
force-pushed
the
form-conversion-tracking
branch
from
654f8d8 to
dbb8dff
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @classes/Plugin/Core.php:
- Around line 295-305: The factory registered under
set('form_conversion_tracking') instantiates
\PopupMaker\Services\FormConversionTracking without the required Service
constructor argument; update the anonymous factory (the function returned for
'form_conversion_tracking') to pass the container instance to the
FormConversionTracking constructor (e.g., new
\PopupMaker\Services\FormConversionTracking($this) or the correct container
variable used in this class) so it matches the Service base class signature.
In @classes/Services/FormConversionTracking.php:
- Around line 64-88: The track_form_conversion method must defensively validate
its $args parameter before using it; add an early guard that returns if $args is
empty or not an array, then use isset() or array_key_exists for accessing
$args['tracked'], $args['popup_id'], and $args['form_provider'] (instead of
assuming array access) so you avoid warnings when non-array values are passed by
hook callbacks; keep the existing numeric check/cast for popup_id and preserve
the site and popup increment calls (increment_site_count and
increment_popup_count).
🧹 Nitpick comments (1)
classes/Services/FormConversionTracking.php (1)
97-103: Consider disabling autoload for analytical data.The site count option is analytical data not needed on every page load. Setting
autoloadtofalseprevents it from being loaded into memory on every request.♻️ Suggested improvement
protected function increment_site_count() { $count = (int) get_option( self::SITE_COUNT_KEY, 0 ); ++$count; - update_option( self::SITE_COUNT_KEY, $count ); + update_option( self::SITE_COUNT_KEY, $count, false ); return $count; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/Plugin/Core.phpclasses/Services/FormConversionTracking.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: Usepum_prefixes for all public functions and hooks in PHP
Use custom\PopupMaker\{ExtensionName}\namespace for classes/functions/hooks in PHP
Follow WordPress coding standards enforced by PHPCS
Use dependency injection over global access in PHP
Inline comments in PHP must end in full-stops, exclamation marks, or question marks (Squiz.Commenting.InlineComment.InvalidEndChar)
Never use strict PHP types on methods hooked to third-party actions/filters; use defensive validation instead
All PHP code must be compatible with PHP 7.4+; forbidden features include union types (e.g.,string|int),mixedtype, named arguments, and match expressions
Use%ifor the table name in $wpdb prepared queries
Check for Popup Maker existence before calling functions
Use defensive validation in hook callbacks for graceful handling of third-party plugin parameter changes
**/*.php: Use thepum_orPUM_prefix for your functions, classes, and hooks to maintain consistency with the core plugin's standards
DO NOT use the olderpopmake_prefix. This is for legacy code being phased out. Usepum_equivalent instead
Use thePopupMaker\namespace for new, modern integrations where appropriate, following the structure seen in theclasses/directory
Check for the existence of Popup Maker functions and classes before calling them usingfunction_exists()to prevent fatal errors if the core plugin is not active
DO NOT use deprecated functions or classes in Popup Maker integrations. Refer to the deprecation guide for alternatives
Use thepum_enqueue_scriptandpum_enqueue_stylefunctions for your assets to allow inclusion in Popup Maker's Asset Cache for optimized performance
DO NOT enqueue your own copies of libraries that Popup Maker already includes, such as jQuery or jQuery UI. Use the registered handles as dependencies
Use thepum_registered_triggers,pum_registered_cookies, andpum_registered_conditionsfilters to register custom items so they appea...
Files:
classes/Plugin/Core.phpclasses/Services/FormConversionTracking.php
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Hook into `pum_analytics_conversion` and `pum_analytics_open` actions to track and respond to popup analytics events
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `$popup->increase_event_count('open|conversion')` to increment popup analytics counters
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-best-practices.mdc:0-0
Timestamp: 2025-12-20T07:31:54.054Z
Learning: Applies to **/*.{js,jsx} : Fire the `pum.integration.form.success` JavaScript action after a successful form submission to integrate a new form plugin with Popup Maker's Form Submission trigger
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `$popup->increase_event_count('open|conversion')` to increment popup analytics counters
Applied to files:
classes/Services/FormConversionTracking.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Hook into `pum_analytics_conversion` and `pum_analytics_open` actions to track and respond to popup analytics events
Applied to files:
classes/Services/FormConversionTracking.php
🧬 Code graph analysis (2)
classes/Plugin/Core.php (3)
classes/Base/Container.php (2)
set(50-52)get(33-35)classes/Services/Globals.php (2)
set(71-75)get(59-61)classes/Services/FormConversionTracking.php (2)
FormConversionTracking(27-165)init(44-46)
classes/Services/FormConversionTracking.php (2)
classes/Base/Service.php (1)
Service(20-37)classes/Plugin/Core.php (1)
get_option(340-350)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: commitlint
🔇 Additional comments (6)
classes/Plugin/Core.php (1)
317-320: Initialization pattern looks correct.The service retrieval and explicit
init()call follows a clear pattern. Once the constructor issue in the factory is fixed, this will work as expected.classes/Services/FormConversionTracking.php (5)
1-37: File structure and constants are well-defined.The namespace, constants with
pum_prefix, and docblocks follow project conventions. The underscore prefix onPOPUP_META_KEYcorrectly indicates private/internal post meta.
39-46: Hook registration is correct.The
init()method cleanly registers the action hook with appropriate priority.
121-142: Getter methods are correctly implemented.Both methods properly cast values to integers and handle missing data gracefully by defaulting to 0.
144-164: Reset methods are appropriate for testing/cleanup.The implementation is correct. If these methods are later exposed via REST API or admin UI, ensure appropriate capability checks are added at that point.
113-119: No changes needed—separate form conversion tracking is intentional.FormConversionTracking is a specialized service that tracks form submission conversions specifically for milestone-based upsell triggers and future analytics. This is distinct from general popup conversions tracked via
increase_event_count('conversion')andpum_analytics_conversion. Form integrations already callincrease_event_count('conversion')for general analytics; FormConversionTracking provides an independent metric for form-specific business logic (e.g., upsell thresholds). The implementation appropriately maintains this separation.
❌ Commit Validation ErrorYour commits don't pass validation. Common issues: Format RequirementsCommits must follow conventional commit format: Valid types: feat, fix, improve, perf, refactor, docs, style, test, build, ci, chore, revert Examples:
|
danieliser
force-pushed
the
form-conversion-tracking
branch
from
d50b78e to
7c07684
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@classes/Services/FormConversionTracking.php`:
- Around line 107-120: The track_ajax_conversion method lacks defensive
validation of its $args parameter; add an early guard at the start of
track_ajax_conversion to ensure $args is an array (mirroring
track_form_conversion) and return early if not, before accessing
$args['eventData'], so the function does not trigger warnings when unexpected
types are passed.
In `@package.json`:
- Line 89: Remove the redundant dependency "commitlint" from package.json — keep
only the scoped "@commitlint/cli" entry; locate the duplicate "commitlint"
dependency (the unscoped package) and delete that line so the project relies
solely on "@commitlint/cli".
♻️ Duplicate comments (1)
classes/Services/FormConversionTracking.php (1)
67-88: Add defensive validation for$argsparameter.This method is hooked to
pum_integrated_form_submissionwhich third-party integrations may call. Per coding guidelines, hook callbacks should use defensive validation. If$argsis not an array, the array access on lines 69 and 74 will cause warnings.🛡️ Proposed fix
public function track_form_conversion( $args ) { + // Defensive validation for third-party callers. + if ( ! is_array( $args ) ) { + return; + } + // Skip if already tracked by another system to prevent duplicates. if ( ! empty( $args['tracked'] ) ) { return; }
🧹 Nitpick comments (2)
classes/Analytics.php (1)
170-192: Consider sanitizing nested values within eventData.The method correctly decodes JSON and returns arrays, but the nested values (
formProvider,formId, etc.) are not sanitized. Since this data comes from client requests, consider applyingsanitize_text_field()to string values within the array to prevent potential XSS if these values are ever output.♻️ Proposed enhancement for nested sanitization
public static function sanitize_event_data( $value ) { // If already an array, return as-is. if ( is_array( $value ) ) { - return $value; + return self::sanitize_event_data_array( $value ); } // Decode JSON string to array (matches Pro's Request::parse_tracking_data). if ( is_string( $value ) ) { $decoded = json_decode( $value, true ); - return is_array( $decoded ) ? $decoded : []; + return is_array( $decoded ) ? self::sanitize_event_data_array( $decoded ) : []; } return []; } +/** + * Sanitize nested eventData array values. + * + * `@param` array $data EventData array. + * + * `@return` array Sanitized array. + */ +private static function sanitize_event_data_array( $data ) { + $sanitized = []; + foreach ( $data as $key => $value ) { + $key = sanitize_key( $key ); + if ( is_string( $value ) ) { + $sanitized[ $key ] = sanitize_text_field( $value ); + } elseif ( is_array( $value ) ) { + $sanitized[ $key ] = self::sanitize_event_data_array( $value ); + } elseif ( is_int( $value ) || is_float( $value ) ) { + $sanitized[ $key ] = $value; + } + } + return $sanitized; +}classes/Services/FormConversionTracking.php (1)
153-159: Potential race condition in concurrent increment operations.The read-modify-write pattern (
get_option→ increment →update_option) is not atomic. Under concurrent form submissions, some increments could be lost. For milestone-based upsells this is likely acceptable, but worth noting if exact counts matter in the future.If precision becomes important, consider using a custom table with SQL
UPDATE ... SET count = count + 1for atomic increments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
assets/js/src/integration/fluentforms.jsassets/js/src/site/plugins/pum-analytics.jsclasses/Analytics.phpclasses/Plugin/Core.phpclasses/Services/FormConversionTracking.phpclasses/Telemetry.phppackage.json
🚧 Files skipped from review as they are similar to previous changes (1)
- assets/js/src/integration/fluentforms.js
🧰 Additional context used
📓 Path-based instructions (5)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: Usepum_prefixes for all public functions and hooks in PHP
Use custom\PopupMaker\{ExtensionName}\namespace for classes/functions/hooks in PHP
Follow WordPress coding standards enforced by PHPCS
Use dependency injection over global access in PHP
Inline comments in PHP must end in full-stops, exclamation marks, or question marks (Squiz.Commenting.InlineComment.InvalidEndChar)
Never use strict PHP types on methods hooked to third-party actions/filters; use defensive validation instead
All PHP code must be compatible with PHP 7.4+; forbidden features include union types (e.g.,string|int),mixedtype, named arguments, and match expressions
Use%ifor the table name in $wpdb prepared queries
Check for Popup Maker existence before calling functions
Use defensive validation in hook callbacks for graceful handling of third-party plugin parameter changes
**/*.php: Use thepum_orPUM_prefix for your functions, classes, and hooks to maintain consistency with the core plugin's standards
DO NOT use the olderpopmake_prefix. This is for legacy code being phased out. Usepum_equivalent instead
Use thePopupMaker\namespace for new, modern integrations where appropriate, following the structure seen in theclasses/directory
Check for the existence of Popup Maker functions and classes before calling them usingfunction_exists()to prevent fatal errors if the core plugin is not active
DO NOT use deprecated functions or classes in Popup Maker integrations. Refer to the deprecation guide for alternatives
Use thepum_enqueue_scriptandpum_enqueue_stylefunctions for your assets to allow inclusion in Popup Maker's Asset Cache for optimized performance
DO NOT enqueue your own copies of libraries that Popup Maker already includes, such as jQuery or jQuery UI. Use the registered handles as dependencies
Use thepum_registered_triggers,pum_registered_cookies, andpum_registered_conditionsfilters to register custom items so they appea...
Files:
classes/Plugin/Core.phpclasses/Services/FormConversionTracking.phpclasses/Telemetry.phpclasses/Analytics.php
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use PUM.hooks system for frontend extensibility in JavaScript/TypeScript
Files:
assets/js/src/site/plugins/pum-analytics.js
**/*.{js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/pm-best-practices.mdc)
**/*.{js,jsx}: Use thePUM.hooksevent manager for JavaScript actions and filters on any page where Popup Maker scripts are loaded
Use JavaScript hooks likepum.popup.beforeOpen,pum.popup.afterOpen,pum.popup.beforeClose,pum.popup.afterClose, andpum.integration.form.successfor extending Popup Maker functionality
Use the globalPUMobject to interact with popups via JavaScript methods likePUM.open(),PUM.close(), andPUM.getSettings()
DO NOT rely on the older$.fn.popmakemethods directly in JavaScript if aPUM.*equivalent exists, asPUM.*provides a more stable, public-facing API
Fire thepum.integration.form.successJavaScript action after a successful form submission to integrate a new form plugin with Popup Maker's Form Submission trigger
Files:
assets/js/src/site/plugins/pum-analytics.js
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/pm-js-api-reference.mdc)
**/*.{js,ts,tsx,jsx}: Use the globalwindow.PUMobject as the primary way to interact with popups on the frontend. Available methods include:PUM.open(),PUM.close(),PUM.getPopup(),PUM.getSettings(),PUM.getSetting(),PUM.getCookie(),PUM.setCookie(),PUM.clearCookie(),PUM.clearCookies(), andPUM.checkConditions()
Use the jQuerypopmake()method directly on popup jQuery objects as an alternative to the globalPUMobject. Available methods include:popmake('open', callback),popmake('close', callback),popmake('getSettings'), andpopmake('getContainer')
Use Popup Maker's hooks system (PUM.hooks) to extend frontend functionality in an event-driven manner. UsePUM.hooks.addAction()for actions andPUM.hooks.addFilter()for filters. Key actions include:pum.popup.beforeOpen,pum.popup.afterOpen,pum.popup.beforeClose,pum.popup.afterClose, andpum.integration.form.success
For form integrations that submit via AJAX, listen for the form's successful submission event and callwindow.PUM.integrations.formSubmission()with the form's jQuery object and an object containingformProviderandformId
Files:
assets/js/src/site/plugins/pum-analytics.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/pm-js-api-reference.mdc)
To extend the Call to Action editor in
@popup-maker/cta-editor, use the Registry API methods:registerEditorTab()to add new tabs,registerEditorHeaderAction()to add header buttons, andregisterEditorHeaderOption()to add menu items
Files:
assets/js/src/site/plugins/pum-analytics.js
🧠 Learnings (18)
📓 Common learnings
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Hook into `pum_analytics_conversion` and `pum_analytics_open` actions to track and respond to popup analytics events
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `$popup->increase_event_count('open|conversion')` to increment popup analytics counters
📚 Learning: 2025-12-20T07:31:54.054Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-best-practices.mdc:0-0
Timestamp: 2025-12-20T07:31:54.054Z
Learning: Applies to **/*.php : Use the `PopupMaker\` namespace for new, modern integrations where appropriate, following the structure seen in the `classes/` directory
Applied to files:
classes/Plugin/Core.phpclasses/Services/FormConversionTracking.php
📚 Learning: 2025-12-20T07:32:04.308Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-js-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:04.308Z
Learning: Applies to **/*.{js,ts,tsx,jsx} : For form integrations that submit via AJAX, listen for the form's successful submission event and call `window.PUM.integrations.formSubmission()` with the form's jQuery object and an object containing `formProvider` and `formId`
Applied to files:
assets/js/src/site/plugins/pum-analytics.js
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Hook into `pum_analytics_conversion` and `pum_analytics_open` actions to track and respond to popup analytics events
Applied to files:
assets/js/src/site/plugins/pum-analytics.jsclasses/Services/FormConversionTracking.phpclasses/Telemetry.php
📚 Learning: 2025-12-20T07:31:54.054Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-best-practices.mdc:0-0
Timestamp: 2025-12-20T07:31:54.054Z
Learning: Applies to **/*.{js,jsx} : Fire the `pum.integration.form.success` JavaScript action after a successful form submission to integrate a new form plugin with Popup Maker's Form Submission trigger
Applied to files:
assets/js/src/site/plugins/pum-analytics.js
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `$popup->increase_event_count('open|conversion')` to increment popup analytics counters
Applied to files:
assets/js/src/site/plugins/pum-analytics.jsclasses/Services/FormConversionTracking.php
📚 Learning: 2025-12-20T07:31:31.480Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T07:31:31.480Z
Learning: Applies to **/*.php : Use defensive validation in hook callbacks for graceful handling of third-party plugin parameter changes
Applied to files:
classes/Services/FormConversionTracking.php
📚 Learning: 2025-12-20T07:31:31.480Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T07:31:31.480Z
Learning: Applies to **/*.php : Never use strict PHP types on methods hooked to third-party actions/filters; use defensive validation instead
Applied to files:
classes/Services/FormConversionTracking.php
📚 Learning: 2025-12-20T07:31:31.480Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T07:31:31.480Z
Learning: Applies to tests/e2e/**/*.{ts,js} : Write E2E tests using Playwright in the `tests/e2e/` directory to test popup functionality and admin workflows against local WordPress environment
Applied to files:
package.json
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_get_option()` and `pum_update_option()` functions for accessing and modifying Popup Maker settings
Applied to files:
classes/Telemetry.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_get_options` and `pum_get_option` filters to modify Popup Maker's global settings before retrieval
Applied to files:
classes/Telemetry.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_popup_settings` filter to modify the settings array for a specific popup
Applied to files:
classes/Telemetry.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `$popup->get_settings()` and `$popup->get_setting('key', 'default')` methods on `PUM_Model_Popup` objects to access popup settings
Applied to files:
classes/Telemetry.php
📚 Learning: 2025-12-20T07:31:54.054Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-best-practices.mdc:0-0
Timestamp: 2025-12-20T07:31:54.054Z
Learning: Applies to **/*.php : Use the `pum_popup_settings_fields` and `pum_theme_settings_fields` filters to add new settings tabs, sections, and fields to the Popup and Theme editors
Applied to files:
classes/Telemetry.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_popup_data_attr` filter to modify the `data-popmake` attribute array before JSON conversion
Applied to files:
classes/Telemetry.php
📚 Learning: 2025-12-20T07:31:54.054Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-best-practices.mdc:0-0
Timestamp: 2025-12-20T07:31:54.054Z
Learning: Applies to **/*.php : Use the methods on popup and theme objects to get or update settings (e.g., `$popup->get_setting()`) rather than `get_post_meta` or `update_post_meta`
Applied to files:
classes/Telemetry.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Hook into `pum_save_popup`, `pum_save_theme`, or `pum_save_settings` actions to respond to data persistence events
Applied to files:
classes/Telemetry.php
📚 Learning: 2025-12-20T07:32:32.164Z
Learnt from: CR
Repo: PopupMaker/Popup-Maker PR: 0
File: .cursor/rules/pm-php-api-reference.mdc:0-0
Timestamp: 2025-12-20T07:32:32.164Z
Learning: Applies to *.php : Use `pum_settings_fields`, `pum_popup_settings_fields`, and `pum_theme_settings_fields` filters to modify admin field configurations
Applied to files:
classes/Telemetry.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: commitlint
🔇 Additional comments (10)
classes/Telemetry.php (1)
175-175: LGTM! Clear TODO for future telemetry integration.The comment correctly references the option key (
pum_form_conversion_count) that aligns with theFormConversionTrackingservice'sSITE_COUNT_KEYconstant. This provides a clear marker for adding the metric once the tracking feature is stable.assets/js/src/site/plugins/pum-analytics.js (1)
144-150: LGTM! Well-structured eventData payload for form conversion tracking.The
eventDataobject properly captures form submission metadata with sensible null defaults. This integrates cleanly with the backend'ssanitize_event_data()method which handles JSON decoding, and the existingsendBeaconlogic (lines 58-67) correctly stringifies nested objects before transmission.classes/Plugin/Core.php (2)
295-305: LGTM! Service registration follows established patterns.The factory correctly passes
$containerto the constructor, matching theServicebase class requirements and the pattern used by other services in this file (e.g.,options,license,logging).
317-320: Service initialization is correctly placed.The form conversion tracking service is properly fetched from the container and initialized. The
FormConversionTrackingclass exists atclasses/Services/FormConversionTracking.phpwith the correct namespacePopupMaker\Servicesand has the requiredinit()method. Service structure follows the guidelines and uses the properpum_prefix for hooks.classes/Analytics.php (1)
220-224: LGTM! REST endpoint correctly accepts eventData.The new
eventDataparameter is properly registered with the sanitization callback. Therequired: falsesetting correctly makes this optional for backward compatibility with existing analytics calls.classes/Services/FormConversionTracking.php (5)
1-36: LGTM!The file header, namespace usage, and constant definitions follow WordPress and Popup Maker conventions correctly. The
pum_prefix on option keys and underscore-prefixed meta key for hidden admin storage are appropriate.
43-49: LGTM!The
init()method correctly hooks into bothpum_integrated_form_submissionfor PHP-side tracking andpum_analytics_conversionfor AJAX beacon tracking. Based on learnings, hooking intopum_analytics_conversionis the appropriate pattern for tracking popup analytics events.
169-175: Same race condition consideration applies here.The per-popup increment has the same read-modify-write pattern. Noted for awareness—same trade-off as the site-wide counter.
184-198: LGTM!The getter methods are straightforward and correctly cast values to integers with sensible defaults.
207-220: LGTM!The reset methods are appropriate for testing and data cleanup purposes. Note that if these are ever exposed via REST/AJAX endpoints, capability checks should be added at the endpoint level.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Created FormConversionTracking service to track form submissions - Hooks into pum_integrated_form_submission action - Tracks both site-wide and per-popup conversion counts - Stores counts in wp_options (site) and post_meta (popup) - Registered service in DI container and initialized on plugin load - Includes helper methods: get_site_count(), get_popup_count() - Includes reset methods for testing/data cleanup - TODO: Send to Supabase v4 when tracking system is ready This enables: - Milestone-based upsell triggers (e.g., "congrats on first form!") - Future analytics dashboard showing form conversion rates - Per-popup performance metrics for power users Addresses: Quick win for Monday release - form conversion foundation
- Add early return when popup_id is empty or invalid - Prevents counting forms submitted outside of popups - Ensures site-wide count only includes popup-captured submissions - Simplifies logic by handling popup_id once instead of twice
- Change @SInCE tags from 1.21.3 to X.X.X for release versioning - Remove opt-in usage tracking reference from class docblock - Simplify class description to focus on core functionality
- Auto-installed during commit message validation - Required for commit message format enforcement
- Add hook to pum_analytics_conversion for AJAX submissions - Non-AJAX submissions continue via pum_integrated_form_submission - Ensures all form submissions are tracked regardless of method - Resolves issue where AJAX forms weren't being counted
…tern - Adopt Pro's eventData structure for form metadata - Filter AJAX tracking by eventData.type === 'form_submission' - Prevents counting CTAs and link clicks as form submissions - Maintains Pro compatibility (Pro removes handler entirely) - Non-AJAX forms still tracked via pum_integrated_form_submission
…ation - Add eventData parameter to Analytics REST endpoint with JSON decode sanitization (matches Pro's Request pattern) - Implement FluentForms integration with defensive handling of double-event firing - Clean FormConversionTracking implementation expecting pre-decoded eventData array - Add TODO in Telemetry for future usage tracking inclusion This completes the form conversion tracking feature with proper architecture matching Pro's proven eventData handling approach.
Prevents orphaned postmeta entries for deleted/invalid popups. Logs validation failures only in WP_DEBUG mode without breaking form submissions. Tracking is non-critical and should fail gracefully.
- Add defensive validation to track_form_conversion() and track_ajax_conversion() - Fix race condition: use atomic SQL updates for increment operations - Add cache flushing after direct wpdb updates - Set autoload=false for analytical data option - Remove duplicate commitlint dependency (keep @commitlint/cli)
danieliser
force-pushed
the
form-conversion-tracking
branch
from
81965c5 to
1155a06
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@classes/Services/FormConversionTracking.php`:
- Around line 67-100: In track_form_conversion replace the direct WP calls: stop
using get_post( $popup_id ) and get_post_type( $popup ) and instead call
pum_get_popup( $popup_id ) and validate with pum_is_popup( $popup ); if
pum_is_popup( $popup ) returns false keep the existing non-fatal logging and
return; update the $popup assignment to $popup = pum_get_popup( $popup_id ) and
use pum_is_popup( $popup ) in the conditional while preserving the WP_DEBUG
error_log message and subsequent increment_site_count() and
increment_popup_count( $popup_id ) calls.
🧹 Nitpick comments (2)
classes/Analytics.php (1)
179-192: Consider sanitizing nested values within the decoded array.The
sanitize_event_data()method correctly decodes JSON to an array but does not sanitize the individual values within that array. IfeventDatacontains user-controlled strings (e.g.,formProvider,formId), these should be sanitized before use downstream.♻️ Proposed fix to recursively sanitize array values
public static function sanitize_event_data( $value ) { // If already an array, return as-is. if ( is_array( $value ) ) { - return $value; + return self::sanitize_event_data_array( $value ); } // Decode JSON string to array (matches Pro's Request::parse_tracking_data). if ( is_string( $value ) ) { $decoded = json_decode( $value, true ); - return is_array( $decoded ) ? $decoded : []; + return is_array( $decoded ) ? self::sanitize_event_data_array( $decoded ) : []; } return []; } +/** + * Recursively sanitize event data array values. + * + * `@param` array $data Event data array. + * + * `@return` array Sanitized array. + */ +private static function sanitize_event_data_array( $data ) { + $sanitized = []; + foreach ( $data as $key => $value ) { + $key = sanitize_key( $key ); + if ( is_array( $value ) ) { + $sanitized[ $key ] = self::sanitize_event_data_array( $value ); + } elseif ( is_string( $value ) ) { + $sanitized[ $key ] = sanitize_text_field( $value ); + } elseif ( is_numeric( $value ) ) { + $sanitized[ $key ] = $value; + } elseif ( is_bool( $value ) ) { + $sanitized[ $key ] = $value; + } + // Skip other types for safety. + } + return $sanitized; +}classes/Services/FormConversionTracking.php (1)
259-283: Consider adding popup ID validation to public accessor methods.The
get_popup_count()andreset_popup_count()methods accept$popup_idwithout validation. While invalid IDs result in safe no-ops, adding validation would provide consistent behavior and clearer debug feedback.♻️ Optional: Add validation for consistency
public function get_popup_count( $popup_id ) { + if ( empty( $popup_id ) || ! is_numeric( $popup_id ) ) { + return 0; + } return (int) get_post_meta( $popup_id, self::POPUP_META_KEY, true ); } // ... public function reset_popup_count( $popup_id ) { + if ( empty( $popup_id ) || ! is_numeric( $popup_id ) ) { + return; + } delete_post_meta( $popup_id, self::POPUP_META_KEY ); }
- Replace get_post() with pum_get_popup() + pum_is_popup() for proper validation - Use %i placeholder for table names in SQL queries (modern wpdb pattern) - Add popup_maker/form_conversion_tracked action after tracking (extensibility)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@classes/Services/FormConversionTracking.php`:
- Around line 101-109: Replace the public action hook string
'popup_maker/form_conversion_tracked' with the pum_ prefixed name
'pum_form_conversion_tracked' in the do_action calls inside
Services/FormConversionTracking.php (the do_action(...) that fires after a form
conversion is tracked and the other occurrence around lines 165-173), updating
both occurrences so the plugin uses the public hook name
pum_form_conversion_tracked consistently.
🧹 Nitpick comments (2)
classes/Services/FormConversionTracking.php (2)
85-87: Guardpum_*calls withfunction_exists()for safety.Even though this is core, the guidelines call for defensive checks before calling Popup Maker functions to avoid fatals if loaded out of sequence.
♻️ Proposed change
+ if ( ! function_exists( 'pum_get_popup' ) || ! function_exists( 'pum_is_popup' ) ) { + return; + } $popup = pum_get_popup( $popup_id );As per coding guidelines, check for Popup Maker existence before calling functions.
Also applies to: 149-151
186-231: Consider injecting$wpdbinstead of using the global.This service can be made more testable and consistent with DI guidelines by accepting a
$wpdbdependency (defaulting to the global when not provided).As per coding guidelines, prefer dependency injection over global access.
1 participant
Summary
FormConversionTrackingservice to track form submission conversionspum_integrated_form_submissionactionChanges
Servicebase classwp_options['pum_form_conversion_count']post_meta['_pum_form_conversion_count']get_site_count(),get_popup_count()reset_site_count(),reset_popup_count()Impact
Testing
get_option('pum_form_conversion_count')increments_pum_form_conversion_countincrements🤖 Ready for CodeRabbit review
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.