From a6f20cc71572635191809e33c7829ed44eef42c4 Mon Sep 17 00:00:00 2001 From: Vansh Kapoor Date: Thu, 20 Aug 2026 19:55:43 +0530 Subject: [PATCH 01/50] Feat: Views & Conversion Rate columns in the Forms table (#2973) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports the work from #2975 onto next-release. That branch was 288 commits behind dev and could not be updated: merging dev in is rejected by the org required_signatures ruleset, because the merge introduces a6346487a (an unsigned commit from 7 July) into the branch. It predates the rule and already sits on dev, next-release and master, but the rule will not let it enter a branch that does not have it — the same rejection comes from git push, gh pr update-branch and the REST update-branch endpoint, and re-signing it would rewrite 220 commits across three protected branches. Squashes the five original signed commits; the net diff is identical to #2975 at 13 files, +849/-37. - inc/form-views.php: Form_Views, the public sureforms/v1/forms/track-view endpoint (HMAC Submit_Token rather than a nonce, so it survives page caching), exclusion re-checks, a fail-closed per-IP+form rate limit, and an atomic $wpdb increment of _srfm_form_views. - assets/js/unminified/form-submit.js: IntersectionObserver beacon, once per form, so views count on fully cached pages. - inc/forms-data.php: views and conversion_rate in prepare_form_for_listing(), plus the compute-sort-paginate path for sorting on a derived metric. - src/admin/forms/components/FormsTable.js: the two sortable columns. - General settings toggle for the feature; _srfm_form_views registered in post-types.php; orderby enum extended in rest-api.php. - tests/unit/inc/test-form-views.php. Verified on this branch by exit code: PHPCS 0, PHPStan level 9 0, phpinsights --min-quality=100 --min-architecture=100 --min-style=100 0, ESLint 0, and 11 Form_Views tests (26 assertions) pass. Manual smoke test and a review of the unauthenticated track-view endpoint are still outstanding — noted in the PR description. --- assets/js/unminified/form-submit.js | 77 +++++ .../settings/get-global-settings.php | 11 +- .../settings/update-global-settings.php | 3 +- inc/form-views.php | 286 ++++++++++++++++++ inc/forms-data.php | 144 +++++++-- inc/global-settings/global-settings.php | 27 +- inc/post-types.php | 5 + inc/rest-api.php | 2 +- plugin-loader.php | 2 + src/admin/forms/components/FormsTable.js | 41 +++ src/admin/settings/Component.js | 4 + src/admin/settings/pages/General.js | 45 +++ tests/unit/inc/test-form-views.php | 239 +++++++++++++++ 13 files changed, 849 insertions(+), 37 deletions(-) create mode 100644 inc/form-views.php create mode 100644 tests/unit/inc/test-form-views.php diff --git a/assets/js/unminified/form-submit.js b/assets/js/unminified/form-submit.js index cca718ce4..ec31637ce 100644 --- a/assets/js/unminified/form-submit.js +++ b/assets/js/unminified/form-submit.js @@ -214,6 +214,83 @@ function initializeFormHandlers() { } ) ); } + + // Record a cache-safe view (impression) for each form once it becomes visible. + trackFormViews( forms ); +} + +/** + * Cache-safe form view (impression) tracking. + * + * Fires a single beacon per form the first time it scrolls into view, so views are + * counted even on fully page-cached pages (the beacon runs client-side). Non-JS + * crawlers never execute this, so they are excluded naturally. Privileged users and + * builder/Instant Form previews are excluded via the server-set + * `window.srfm_view_beacon.enabled` flag. + * + * @param {Array} forms - The `.srfm-form` elements collected during initialization. + */ +function trackFormViews( forms ) { + // Respect the server-side exclusion flag (admins/editors, previews). + if ( ! window.srfm_view_beacon || ! window.srfm_view_beacon.enabled ) { + return; + } + + if ( typeof window.IntersectionObserver === 'undefined' ) { + return; + } + + for ( const form of forms ) { + // Count each form once per page load. + if ( form.hasAttribute( 'data-srfm-view-tracked' ) ) { + continue; + } + + const formId = form.getAttribute( 'form-id' ); + const submitToken = form.getAttribute( 'data-submit-token' ); + + if ( ! formId || ! submitToken ) { + continue; + } + + const observer = new window.IntersectionObserver( + ( entries, obs ) => { + for ( const entry of entries ) { + if ( ! entry.isIntersecting ) { + continue; + } + + // Guard again inside the callback: if initializeFormHandlers() + // re-ran and attached a second observer to a still-unviewed form, + // this prevents both observers from counting the same impression. + if ( form.hasAttribute( 'data-srfm-view-tracked' ) ) { + obs.disconnect(); + continue; + } + + // Mark + stop observing before the request so it fires only once. + form.setAttribute( 'data-srfm-view-tracked', '1' ); + obs.disconnect(); + + wp.apiFetch( { + path: 'sureforms/v1/forms/track-view', + method: 'POST', + data: { form_id: parseInt( formId, 10 ) }, + headers: { + 'X-WP-Submit-Token': submitToken, + }, + } ).catch( () => { + // Beacon is best-effort; never disrupt the page on failure. + } ); + } + }, + // threshold 0 → count as soon as any part of the form is visible, + // so tall forms on short viewports are not missed. + { threshold: 0 } + ); + + observer.observe( form ); + } } document.addEventListener( 'DOMContentLoaded', function () { diff --git a/inc/abilities/settings/get-global-settings.php b/inc/abilities/settings/get-global-settings.php index 8c75ff1f0..49477fdc8 100644 --- a/inc/abilities/settings/get-global-settings.php +++ b/inc/abilities/settings/get-global-settings.php @@ -145,9 +145,10 @@ private function get_general_settings() { if ( empty( $settings ) || ! is_array( $settings ) ) { $settings = [ - 'srfm_ip_log' => false, - 'srfm_form_analytics' => false, - 'srfm_admin_notification' => true, + 'srfm_ip_log' => false, + 'srfm_form_analytics' => false, + 'srfm_admin_notification' => true, + 'srfm_form_views_tracking' => true, ]; } @@ -155,6 +156,10 @@ private function get_general_settings() { $settings['srfm_admin_notification'] = true; } + if ( ! isset( $settings['srfm_form_views_tracking'] ) ) { + $settings['srfm_form_views_tracking'] = true; + } + $settings['srfm_bsf_analytics'] = 'yes' === get_option( 'sureforms_usage_optin', false ); return $settings; diff --git a/inc/abilities/settings/update-global-settings.php b/inc/abilities/settings/update-global-settings.php index 3d88a2af3..67bda9d47 100644 --- a/inc/abilities/settings/update-global-settings.php +++ b/inc/abilities/settings/update-global-settings.php @@ -47,6 +47,7 @@ class Update_Global_Settings extends Abstract_Ability { 'srfm_form_analytics', 'srfm_bsf_analytics', 'srfm_admin_notification', + 'srfm_form_views_tracking', ], 'validation-messages' => [ 'srfm_url_block_required_text', @@ -283,7 +284,7 @@ private function sanitize_settings( $category, $settings ) { * @return array */ private function sanitize_general_settings( $settings ) { - $boolean_keys = [ 'srfm_ip_log', 'srfm_form_analytics', 'srfm_bsf_analytics', 'srfm_admin_notification' ]; + $boolean_keys = [ 'srfm_ip_log', 'srfm_form_analytics', 'srfm_bsf_analytics', 'srfm_admin_notification', 'srfm_form_views_tracking' ]; foreach ( $boolean_keys as $key ) { if ( isset( $settings[ $key ] ) ) { diff --git a/inc/form-views.php b/inc/form-views.php new file mode 100644 index 000000000..7c3dbe20d --- /dev/null +++ b/inc/form-views.php @@ -0,0 +1,286 @@ + $endpoints Existing endpoint definitions. + * @since x.x.x + * @return array Endpoints with the track-view route added. + */ + public function register_route( $endpoints ) { + if ( ! is_array( $endpoints ) ) { + return $endpoints; + } + + $endpoints['forms/track-view'] = [ + 'methods' => 'POST', + 'callback' => [ $this, 'track_view' ], + 'permission_callback' => [ $this, 'permissions_check' ], + 'args' => [ + 'form_id' => [ + 'type' => 'integer', + 'required' => true, + 'minimum' => 1, + ], + ], + ]; + + return $endpoints; + } + + /** + * Permission check: proof-of-origin via the HMAC Submit_Token header (cache-safe). + * + * @param WP_REST_Request> $request REST request. + * @since x.x.x + * @return true|WP_Error + */ + public function permissions_check( $request ) { + $token = Helper::get_string_value( $request->get_header( 'X-WP-Submit-Token' ) ); + $form_id = absint( Helper::get_integer_value( $request->get_param( 'form_id' ) ) ); + + if ( ! Submit_Token::verify( $token, $form_id ) ) { + return new WP_Error( + 'srfm_view_token_invalid', + __( 'Security verification failed.', 'sureforms' ), + [ 'status' => 403 ] + ); + } + + return true; + } + + /** + * Record a view for the given form. + * + * Silently no-ops (200) for excluded contexts so the beacon never surfaces an + * error to visitors; returns 429 only when the IP + form rate limit is exceeded. + * + * @param WP_REST_Request> $request REST request. + * @since x.x.x + * @return WP_REST_Response + */ + public function track_view( $request ) { + $form_id = absint( Helper::get_integer_value( $request->get_param( 'form_id' ) ) ); + + if ( ! $form_id || SRFM_FORMS_POST_TYPE !== get_post_type( $form_id ) ) { + return new WP_REST_Response( [ 'counted' => false ], 200 ); + } + + // Exclude previews and logged-in privileged users (author/editor/admin). + if ( ! $this->should_track() ) { + return new WP_REST_Response( [ 'counted' => false ], 200 ); + } + + if ( $this->is_rate_limited( $form_id ) ) { + return new WP_REST_Response( [ 'counted' => false ], 429 ); + } + + $this->increment_views( $form_id ); + + return new WP_REST_Response( [ 'counted' => true ], 200 ); + } + + /** + * Localize the beacon enable flag onto the (already enqueued) form-submit script. + * + * @since x.x.x + * @return void + */ + public function localize_beacon() { + if ( ! wp_script_is( 'srfm-form-submit', 'enqueued' ) ) { + return; + } + + wp_localize_script( + 'srfm-form-submit', + 'srfm_view_beacon', + [ 'enabled' => $this->should_track() ] + ); + } + + /** + * Read the current view count for a form. + * + * @param int $form_id Form post ID. + * @since x.x.x + * @return int + */ + public function get_views( $form_id ) { + return max( 0, Helper::get_integer_value( get_post_meta( $form_id, self::META_KEY, true ) ) ); + } + + /** + * Whether view + conversion-rate tracking is enabled in the global General + * settings. Defaults to true when the setting has never been saved so existing + * installs keep tracking without an explicit opt-in. + * + * @since x.x.x + * @return bool + */ + public function is_tracking_enabled() { + $general = get_option( 'srfm_general_settings_options', [] ); + + if ( ! is_array( $general ) || ! isset( $general[ self::SETTING_KEY ] ) ) { + return true; + } + + return (bool) $general[ self::SETTING_KEY ]; + } + + /** + * Whether the current request should be counted as a view. + * + * Excludes requests when tracking is disabled globally, logged-in users who can + * edit content (author/editor/admin), and form-builder / Instant Form live previews. + * + * @since x.x.x + * @return bool + */ + private function should_track() { + if ( ! $this->is_tracking_enabled() ) { + return false; + } + + if ( is_user_logged_in() && current_user_can( 'edit_posts' ) ) { + return false; + } + + if ( ! empty( Helper::get_instant_form_live_data() ) ) { + return false; + } + + return true; + } + + /** + * Atomically increment the stored view count for a form. + * + * @param int $form_id Form post ID. + * @since x.x.x + * @return void + */ + private function increment_views( $form_id ) { + global $wpdb; + + // Atomic increment so concurrent beacons don't clobber each other. + // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching + $updated = $wpdb->query( + $wpdb->prepare( + "UPDATE {$wpdb->postmeta} SET meta_value = meta_value + 1 WHERE post_id = %d AND meta_key = %s", + $form_id, + self::META_KEY + ) + ); + + if ( empty( $updated ) ) { + // Row didn't exist yet — create it (unique so a racing insert can't duplicate). + add_post_meta( $form_id, self::META_KEY, 1, true ); + } + + // Keep the post-meta cache consistent after the direct write. + wp_cache_delete( $form_id, 'post_meta' ); + } + + /** + * Per visitor-IP + form rate limit. Fails closed when the IP is undeterminable. + * + * @param int $form_id Form post ID. + * @since x.x.x + * @return bool True when the request should be blocked. + */ + private function is_rate_limited( $form_id ) { + // Use the connection's REMOTE_ADDR, not Helper::get_visitor_ip() (which trusts + // client-supplied X-Forwarded-For / Client-IP headers). Otherwise an attacker + // could send a fresh spoofed forwarded IP per request, get a new rate-limit + // bucket each time, and inflate the view counter without bound. + $ip = isset( $_SERVER['REMOTE_ADDR'] ) ? sanitize_text_field( wp_unslash( $_SERVER['REMOTE_ADDR'] ) ) : ''; + + if ( empty( $ip ) || ! filter_var( $ip, FILTER_VALIDATE_IP ) ) { + return true; // Fail closed if IP cannot be determined. + } + + $transient_key = 'srfm_view_' . md5( $ip . '_' . $form_id ); + $attempts = get_transient( $transient_key ); + + if ( false === $attempts ) { + set_transient( $transient_key, 1, MINUTE_IN_SECONDS ); + return false; + } + + $attempts_count = Helper::get_integer_value( $attempts ); + + if ( $attempts_count >= self::RATE_LIMIT_MAX ) { + return true; + } + + set_transient( $transient_key, $attempts_count + 1, MINUTE_IN_SECONDS ); + return false; + } +} diff --git a/inc/forms-data.php b/inc/forms-data.php index e7fb75210..4c3bc3616 100644 --- a/inc/forms-data.php +++ b/inc/forms-data.php @@ -202,32 +202,124 @@ public function get_forms_list( $request ) { // Execute query — use try/finally to guarantee filter cleanup. try { - $query = new \WP_Query( $args ); + if ( in_array( $orderby, [ 'views', 'conversion_rate' ], true ) ) { + // Derived metrics can't be sorted by WP_Query; compute across all + // matching forms, sort, then paginate. + $response_data = $this->get_forms_sorted_by_metric( $args, $orderby, $order, $page, Helper::get_integer_value( $per_page ) ); + } else { + $query = new \WP_Query( $args ); + + $forms = []; + /** + * Post object from the query. + * + * @var \WP_Post $post */ + foreach ( $query->posts as $post ) { + $forms[] = $this->prepare_form_for_listing( $post ); + } + + $response_data = [ + 'forms' => $forms, + 'total' => Helper::get_integer_value( $query->found_posts ), + 'total_pages' => Helper::get_integer_value( $query->max_num_pages ), + 'current_page' => $page, + 'per_page' => $per_page, + ]; + } } finally { if ( ! empty( $search ) && is_numeric( $search ) && isset( $where_filter ) ) { remove_filter( 'posts_where', $where_filter, 10 ); } } + return new WP_REST_Response( $response_data, 200 ); + } + + /** + * Sort the forms list by a derived metric (views or conversion rate) and paginate. + * + * WP_Query can't order by views (missing-meta forms would drop out) or by the + * derived conversion rate at all, so we fetch every matching form id, compute the + * metric, sort in PHP, then slice the requested page. Form counts are small in + * practice; revisit with a grouped query if a site accumulates thousands of forms. + * + * @param array $args Base WP_Query args (filters/search), pagination ignored. + * @param string $orderby Either 'views' or 'conversion_rate'. + * @param string $order 'asc' or 'desc'. + * @param int $page Current page (1-based). + * @param int $per_page Items per page. + * @since x.x.x + * @return array Response payload matching get_forms_list(). + */ + private function get_forms_sorted_by_metric( $args, $orderby, $order, $page, $per_page ) { + $id_args = $args; + $id_args['posts_per_page'] = -1; + $id_args['paged'] = 1; + $id_args['fields'] = 'ids'; + $id_args['orderby'] = 'ID'; + $id_args['order'] = 'DESC'; + + $id_query = new \WP_Query( $id_args ); + + // `fields => ids` skips the post-meta cache priming that a normal WP_Query does, + // so prime it once for all matched forms — otherwise each get_views() below is a + // separate get_post_meta() query (N+1). Entry counts are still one COUNT per form; + // acceptable for typical form volumes, revisit with a grouped query if needed. + if ( ! empty( $id_query->posts ) ) { + $prime_ids = array_map( + static function ( $post ) { + return (int) ( $post instanceof \WP_Post ? $post->ID : $post ); + }, + $id_query->posts + ); + update_meta_cache( 'post', $prime_ids ); + } + + $rows = []; + foreach ( $id_query->posts as $post_id ) { + $form_id = Helper::get_integer_value( $post_id ); + $views = Form_Views::get_instance()->get_views( $form_id ); + $entries = Helper::get_integer_value( Entries::get_total_entries_by_status( 'all', $form_id ) ); + $metric = 'views' === $orderby ? (float) $views : ( $views > 0 ? $entries / $views * 100 : 0.0 ); + + $rows[] = [ + 'id' => $form_id, + 'metric' => $metric, + ]; + } + + // Sort by metric, tie-break on id (desc) for a stable order. + $direction = 'asc' === strtolower( $order ) ? 1 : -1; + usort( + $rows, + static function ( $a, $b ) use ( $direction ) { + if ( $a['metric'] === $b['metric'] ) { + return $b['id'] <=> $a['id']; + } + return ( $a['metric'] <=> $b['metric'] ) * $direction; + } + ); + + $total = count( $rows ); + $total_pages = $per_page > 0 ? (int) ceil( $total / $per_page ) : 1; + $offset = ( $page - 1 ) * $per_page; + $page_rows = array_slice( $rows, max( 0, $offset ), $per_page ); + $forms = []; - /** - * Post object from the query. - * - * @var \WP_Post $post */ - foreach ( $query->posts as $post ) { - $forms[] = $this->prepare_form_for_listing( $post ); + foreach ( $page_rows as $row ) { + $post = get_post( $row['id'] ); + if ( $post instanceof \WP_Post ) { + $forms[] = $this->prepare_form_for_listing( $post ); + } } - // Prepare response. - $response_data = [ + return [ 'forms' => $forms, - 'total' => Helper::get_integer_value( $query->found_posts ), - 'total_pages' => Helper::get_integer_value( $query->max_num_pages ), + 'total' => $total, + 'total_pages' => $total_pages, 'current_page' => $page, 'per_page' => $per_page, ]; - - return new WP_REST_Response( $response_data, 200 ); } /** @@ -243,16 +335,24 @@ private function prepare_form_for_listing( $post ) { // Get entries count. $entries_count = Helper::get_integer_value( Entries::get_total_entries_by_status( 'all', $form_id ) ); + // Views (impressions) and derived conversion rate. Clamp to 100%: entries are + // all-time while views only accrue from when this feature shipped, so early on a + // form can have more entries than counted views (which would read >100%). + $views = Form_Views::get_instance()->get_views( $form_id ); + $conversion_rate = $views > 0 ? round( min( 100, $entries_count / $views * 100 ), 1 ) : 0.0; + return [ - 'id' => $form_id, - 'title' => $post->post_title, - 'status' => $post->post_status, - 'date_created' => mysql_to_rfc3339( $post->post_date ), - 'date_modified' => mysql_to_rfc3339( $post->post_modified ), - 'entries_count' => $entries_count, - 'shortcode' => "[sureforms id='{$form_id}']", - 'edit_url' => admin_url( "post.php?post={$form_id}&action=edit" ), - 'frontend_url' => get_permalink( $form_id ), + 'id' => $form_id, + 'title' => $post->post_title, + 'status' => $post->post_status, + 'date_created' => mysql_to_rfc3339( $post->post_date ), + 'date_modified' => mysql_to_rfc3339( $post->post_modified ), + 'entries_count' => $entries_count, + 'views' => $views, + 'conversion_rate' => $conversion_rate, + 'shortcode' => "[sureforms id='{$form_id}']", + 'edit_url' => admin_url( "post.php?post={$form_id}&action=edit" ), + 'frontend_url' => get_permalink( $form_id ), ]; } } diff --git a/inc/global-settings/global-settings.php b/inc/global-settings/global-settings.php index 14856005a..0e39c4480 100644 --- a/inc/global-settings/global-settings.php +++ b/inc/global-settings/global-settings.php @@ -148,15 +148,17 @@ public static function srfm_save_global_settings( $request ) { */ public static function srfm_save_general_settings( $setting_options ) { - $srfm_ip_log = $setting_options['srfm_ip_log'] ?? false; - $srfm_form_analytics = $setting_options['srfm_form_analytics'] ?? false; - $srfm_bsf_analytics = $setting_options['srfm_bsf_analytics'] ?? false; - $srfm_admin_notification = isset( $setting_options['srfm_admin_notification'] ) ? (bool) $setting_options['srfm_admin_notification'] : true; + $srfm_ip_log = $setting_options['srfm_ip_log'] ?? false; + $srfm_form_analytics = $setting_options['srfm_form_analytics'] ?? false; + $srfm_bsf_analytics = $setting_options['srfm_bsf_analytics'] ?? false; + $srfm_admin_notification = isset( $setting_options['srfm_admin_notification'] ) ? (bool) $setting_options['srfm_admin_notification'] : true; + $srfm_form_views_tracking = isset( $setting_options['srfm_form_views_tracking'] ) ? (bool) $setting_options['srfm_form_views_tracking'] : true; $settings = [ - 'srfm_ip_log' => $srfm_ip_log, - 'srfm_form_analytics' => $srfm_form_analytics, - 'srfm_admin_notification' => $srfm_admin_notification, + 'srfm_ip_log' => $srfm_ip_log, + 'srfm_form_analytics' => $srfm_form_analytics, + 'srfm_admin_notification' => $srfm_admin_notification, + 'srfm_form_views_tracking' => $srfm_form_views_tracking, ]; /** @@ -660,9 +662,10 @@ public static function srfm_get_general_settings( $request ) { if ( empty( $global_setting_options['srfm_general_settings_options'] ) || ! is_array( $global_setting_options['srfm_general_settings_options'] ) ) { $global_setting_options['srfm_general_settings_options'] = [ - 'srfm_ip_log' => false, - 'srfm_form_analytics' => false, - 'srfm_admin_notification' => true, + 'srfm_ip_log' => false, + 'srfm_form_analytics' => false, + 'srfm_admin_notification' => true, + 'srfm_form_views_tracking' => true, ]; } @@ -670,6 +673,10 @@ public static function srfm_get_general_settings( $request ) { $global_setting_options['srfm_general_settings_options']['srfm_admin_notification'] = true; } + if ( ! isset( $global_setting_options['srfm_general_settings_options']['srfm_form_views_tracking'] ) ) { + $global_setting_options['srfm_general_settings_options']['srfm_form_views_tracking'] = true; + } + /** * We have introduced toggle for analytics optin in the general settings. * Hence retrieving the option sureforms_analytics_optin to get current status. diff --git a/inc/post-types.php b/inc/post-types.php index d3af48d85..b98eb6afb 100644 --- a/inc/post-types.php +++ b/inc/post-types.php @@ -448,6 +448,11 @@ public function register_post_metas() { ] ); + // NOTE: `_srfm_form_views` is intentionally NOT registered here. It is a + // server-side counter written only by Form_Views (get/add_post_meta + atomic + // SQL). Exposing it to the block editor via show_in_rest let a form save / + // autosave round-trip a stale value and clobber the live count back to 0. + // Form Custom CSS meta. register_post_meta( 'sureforms_form', diff --git a/inc/rest-api.php b/inc/rest-api.php index 3a5674df1..5e6c9739f 100644 --- a/inc/rest-api.php +++ b/inc/rest-api.php @@ -1765,7 +1765,7 @@ private function get_endpoints() { 'orderby' => [ 'type' => 'string', 'default' => 'date', - 'enum' => [ 'date', 'id', 'title', 'modified' ], + 'enum' => [ 'date', 'id', 'title', 'modified', 'views', 'conversion_rate' ], ], 'order' => [ 'type' => 'string', diff --git a/plugin-loader.php b/plugin-loader.php index a0972c062..6e3c82de0 100644 --- a/plugin-loader.php +++ b/plugin-loader.php @@ -37,6 +37,7 @@ use SRFM\Inc\Export; use SRFM\Inc\Form_Restriction; use SRFM\Inc\Form_Submit; +use SRFM\Inc\Form_Views; use SRFM\Inc\Forms_Data; use SRFM\Inc\Frontend_Assets; use SRFM\Inc\Generate_Form_Markup; @@ -305,6 +306,7 @@ public function load_plugin() { Activator::get_instance(); Admin_Ajax::get_instance(); Forms_Data::get_instance(); + Form_Views::get_instance(); Export::get_instance(); Smart_Tags::get_instance(); Generate_Form_Markup::get_instance(); diff --git a/src/admin/forms/components/FormsTable.js b/src/admin/forms/components/FormsTable.js index fad44a06e..644ee0ce9 100644 --- a/src/admin/forms/components/FormsTable.js +++ b/src/admin/forms/components/FormsTable.js @@ -178,6 +178,47 @@ const FormsTable = ( { ), }, + { + label: __( 'Views', 'sureforms' ), + key: 'views', + sortable: true, + headerClassName: 'w-[8%]', + render: ( form ) => ( + + { Number( form.views ?? 0 ).toLocaleString() } + + ), + }, + { + label: __( 'Conversion Rate', 'sureforms' ), + key: 'conversion_rate', + sortable: true, + headerClassName: 'w-[10%]', + render: ( form ) => { + const views = Number( form.views ?? 0 ); + if ( ! views ) { + // No views yet — nothing to convert against. + return ( + + — + + ); + } + return ( + + { `${ Number( + form.conversion_rate ?? 0 + ).toLocaleString() }%` } + + ); + }, + }, { label: __( 'Date & Time', 'sureforms' ), key: 'date', diff --git a/src/admin/settings/Component.js b/src/admin/settings/Component.js index 9115c2fb9..a8b06461f 100644 --- a/src/admin/settings/Component.js +++ b/src/admin/settings/Component.js @@ -40,6 +40,7 @@ const Component = ( { path, subpage } ) => { srfm_form_analytics: false, srfm_bsf_analytics: false, srfm_admin_notification: true, + srfm_form_views_tracking: true, } ); const [ emailTabOptions, setEmailTabOptions ] = useState( { srfm_email_summary: false, @@ -268,12 +269,15 @@ const Component = ( { path, subpage } ) => { srfm_form_analytics, srfm_bsf_analytics, srfm_admin_notification, + // Default ON for installs saved before this toggle existed. + srfm_form_views_tracking = true, } = data.srfm_general_settings_options; const snap = { srfm_ip_log, srfm_form_analytics, srfm_bsf_analytics, srfm_admin_notification, + srfm_form_views_tracking, }; setGeneralTabOptions( snap ); nextBaselines[ 'general-settings' ] = snap; diff --git a/src/admin/settings/pages/General.js b/src/admin/settings/pages/General.js index 3d1b2714a..6f1612e33 100644 --- a/src/admin/settings/pages/General.js +++ b/src/admin/settings/pages/General.js @@ -215,6 +215,41 @@ const AdminNotificationContent = ( { ); }; +/** + * Form Views & Conversion tracking settings section. + * + * @param {Object} props + * @param {Object} props.generalTabOptions - General settings. + * @param {Function} props.updateGlobalSettings - Settings update handler. + */ +const FormViewsTrackingContent = ( { + generalTabOptions, + updateGlobalSettings, +} ) => { + return ( + + updateGlobalSettings( + 'srfm_form_views_tracking', + value, + 'general-settings' + ) + } + /> + ); +}; + /** * Usage Tracking / Analytics settings section. * @@ -328,6 +363,16 @@ const GeneralPage = ( { /> } /> + + } + /> markTestSkipped( 'Form_Views class not available.' ); + } + + $this->views = Form_Views::get_instance(); + $_SERVER['REMOTE_ADDR'] = '127.0.0.1'; + unset( $_GET['live_mode'] ); + wp_set_current_user( 0 ); + } + + protected function tearDown(): void { + wp_set_current_user( 0 ); + parent::tearDown(); + } + + /** + * Helper to call private methods. + */ + private function call_private_method( $object, $method_name, $parameters = [] ) { + $reflection = new \ReflectionClass( get_class( $object ) ); + $method = $reflection->getMethod( $method_name ); + $method->setAccessible( true ); + return $method->invokeArgs( $object, $parameters ); + } + + private function make_form() { + return wp_insert_post( + [ + 'post_type' => SRFM_FORMS_POST_TYPE, + 'post_title' => 'Views Test Form', + 'post_status' => 'publish', + ] + ); + } + + // ────────────────────────────────────────────── + // get_views / increment_views + // ────────────────────────────────────────────── + + public function test_get_views_defaults_to_zero() { + $form_id = $this->make_form(); + $this->assertSame( 0, $this->views->get_views( $form_id ) ); + wp_delete_post( $form_id, true ); + } + + public function test_increment_views_creates_then_increments() { + $form_id = $this->make_form(); + + $this->call_private_method( $this->views, 'increment_views', [ $form_id ] ); + $this->assertSame( 1, $this->views->get_views( $form_id ), 'First increment should create the meta at 1.' ); + + $this->call_private_method( $this->views, 'increment_views', [ $form_id ] ); + $this->assertSame( 2, $this->views->get_views( $form_id ), 'Second increment should bump to 2.' ); + + wp_delete_post( $form_id, true ); + } + + // ────────────────────────────────────────────── + // should_track (exclusions) + // ────────────────────────────────────────────── + + public function test_should_track_true_for_anonymous_visitor() { + wp_set_current_user( 0 ); + $this->assertTrue( $this->call_private_method( $this->views, 'should_track' ) ); + } + + public function test_should_track_excludes_privileged_user() { + $editor = wp_insert_user( + [ + 'user_login' => 'srfm_views_editor_' . wp_rand(), + 'user_pass' => 'password', + 'user_email' => 'srfm_views_editor_' . wp_rand() . '@example.com', + 'role' => 'editor', + ] + ); + wp_set_current_user( $editor ); + + $this->assertFalse( $this->call_private_method( $this->views, 'should_track' ) ); + + wp_set_current_user( 0 ); + wp_delete_user( $editor ); + } + + // ────────────────────────────────────────────── + // is_tracking_enabled (global General-settings toggle) + // ────────────────────────────────────────────── + + public function test_is_tracking_enabled() { + // Default ON when the option/key has never been saved. + delete_option( 'srfm_general_settings_options' ); + $this->assertTrue( $this->views->is_tracking_enabled(), 'Absent option should default to enabled.' ); + + // Key present but false → disabled. + update_option( 'srfm_general_settings_options', [ 'srfm_form_views_tracking' => false ] ); + $this->assertFalse( $this->views->is_tracking_enabled(), 'Explicit false should disable tracking.' ); + // Disabling tracking also short-circuits should_track for anonymous visitors. + wp_set_current_user( 0 ); + $this->assertFalse( $this->call_private_method( $this->views, 'should_track' ), 'should_track must respect the global toggle.' ); + + // Key present and true → enabled. + update_option( 'srfm_general_settings_options', [ 'srfm_form_views_tracking' => true ] ); + $this->assertTrue( $this->views->is_tracking_enabled(), 'Explicit true should enable tracking.' ); + + delete_option( 'srfm_general_settings_options' ); + } + + // ────────────────────────────────────────────── + // permissions_check (HMAC token) + // ────────────────────────────────────────────── + + public function test_permissions_check_rejects_missing_token() { + $form_id = $this->make_form(); + $request = new WP_REST_Request( 'POST', '/sureforms/v1/forms/track-view' ); + $request->set_param( 'form_id', $form_id ); + + $result = $this->views->permissions_check( $request ); + $this->assertInstanceOf( 'WP_Error', $result ); + + wp_delete_post( $form_id, true ); + } + + public function test_permissions_check_accepts_valid_token() { + $form_id = $this->make_form(); + $token = Submit_Token::generate( (int) $form_id ); + + $request = new WP_REST_Request( 'POST', '/sureforms/v1/forms/track-view' ); + $request->set_param( 'form_id', $form_id ); + $request->set_header( 'X-WP-Submit-Token', $token ); + + $this->assertTrue( $this->views->permissions_check( $request ) ); + + wp_delete_post( $form_id, true ); + } + + // ────────────────────────────────────────────── + // track_view (end to end) + // ────────────────────────────────────────────── + + public function test_track_view_counts_for_anonymous_visitor() { + $form_id = $this->make_form(); + wp_set_current_user( 0 ); + + $request = new WP_REST_Request( 'POST', '/sureforms/v1/forms/track-view' ); + $request->set_param( 'form_id', $form_id ); + + $response = $this->views->track_view( $request ); + $this->assertInstanceOf( 'WP_REST_Response', $response ); + $this->assertSame( 200, $response->get_status() ); + $this->assertSame( 1, $this->views->get_views( $form_id ) ); + + wp_delete_post( $form_id, true ); + } + + public function test_track_view_does_not_count_privileged_user() { + $form_id = $this->make_form(); + $admin = wp_insert_user( + [ + 'user_login' => 'srfm_views_admin_' . wp_rand(), + 'user_pass' => 'password', + 'user_email' => 'srfm_views_admin_' . wp_rand() . '@example.com', + 'role' => 'administrator', + ] + ); + wp_set_current_user( $admin ); + + $request = new WP_REST_Request( 'POST', '/sureforms/v1/forms/track-view' ); + $request->set_param( 'form_id', $form_id ); + + $response = $this->views->track_view( $request ); + $this->assertSame( 200, $response->get_status() ); + $this->assertSame( 0, $this->views->get_views( $form_id ), 'Privileged users must not be counted.' ); + + wp_set_current_user( 0 ); + wp_delete_user( $admin ); + wp_delete_post( $form_id, true ); + } + + // ────────────────────────────────────────────── + // register_route / localize_beacon + // ────────────────────────────────────────────── + + /** + * register_route adds the public forms/track-view POST route to the endpoints array. + */ + public function test_register_route() { + $endpoints = $this->views->register_route( [] ); + + $this->assertIsArray( $endpoints ); + $this->assertArrayHasKey( 'forms/track-view', $endpoints ); + $this->assertSame( 'POST', $endpoints['forms/track-view']['methods'] ); + $this->assertArrayHasKey( 'callback', $endpoints['forms/track-view'] ); + $this->assertArrayHasKey( 'permission_callback', $endpoints['forms/track-view'] ); + $this->assertArrayHasKey( 'form_id', $endpoints['forms/track-view']['args'] ); + + // Non-array input is returned unchanged (defensive guard). + $this->assertSame( 'unchanged', $this->views->register_route( 'unchanged' ) ); + } + + /** + * localize_beacon is a safe no-op when the form-submit script is not enqueued, + * and localizes the beacon flag onto it when it is. + */ + public function test_localize_beacon() { + // Not enqueued → early return, nothing localized, no error. + wp_dequeue_script( 'srfm-form-submit' ); + $this->views->localize_beacon(); + $this->assertFalse( wp_script_is( 'srfm-form-submit', 'enqueued' ) ); + + // Enqueued → beacon data is attached to the script. + wp_register_script( 'srfm-form-submit', '', [], '1.0.0', true ); + wp_enqueue_script( 'srfm-form-submit' ); + $this->views->localize_beacon(); + + $data = wp_scripts()->get_data( 'srfm-form-submit', 'data' ); + $this->assertIsString( $data ); + $this->assertStringContainsString( 'srfm_view_beacon', $data ); + + wp_dequeue_script( 'srfm-form-submit' ); + wp_deregister_script( 'srfm-form-submit' ); + } +} From a7a2b70ef5d264fe52fae73a096cef49630819e7 Mon Sep 17 00:00:00 2001 From: Vansh Kapoor Date: Thu, 20 Aug 2026 20:12:05 +0530 Subject: [PATCH 02/50] fix: hide the Views and Conversion Rate columns when tracking is off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The General-settings toggle only gated the beacon and the REST endpoint, so switching tracking off left both columns on the Forms table. With no new views being recorded they would sit frozen at whatever had been counted before, which reads as broken data rather than a disabled feature. - admin/admin.php: expose the same setting to the admin bundle as srfm_admin.form_views_tracking, resolved through Form_Views::is_tracking_enabled() so there is one source of truth rather than a second copy of the option read. - FormsTable.js: spread the two column definitions in only when the flag is not false. Absent flag means shown, matching the server-side default for installs that predate the setting. - inc/forms-data.php: stop computing the metrics when tracking is off. Hiding the columns alone would have left a per-form meta read on every listing request for values nothing renders, and would still have honoured orderby=views / conversion_rate — running the compute-sort-paginate pass to order rows nobody can see. That request now falls through to the default ordering. Verified against the real option rather than by inspection: is_tracking_enabled() returns true / false / true for on, off and key-absent, and prepare_form_for_listing() returns views=42 conversion_rate=2.4 with tracking on and views=0 conversion_rate=0.0 with it off, for the same seeded meta. PHPCS 0, PHPStan level 9 0, phpinsights --min-quality=100 0, ESLint 0, build:script 0, 19 tests (46 assertions) pass. --- admin/admin.php | 6 ++ inc/forms-data.php | 19 ++++- src/admin/forms/components/FormsTable.js | 89 +++++++++++++----------- 3 files changed, 71 insertions(+), 43 deletions(-) diff --git a/admin/admin.php b/admin/admin.php index 35e38aeea..465e63417 100644 --- a/admin/admin.php +++ b/admin/admin.php @@ -10,6 +10,7 @@ use Astra_Notices; use SRFM\Inc\AI_Form_Builder\AI_Helper; use SRFM\Inc\Database\Tables\Entries; +use SRFM\Inc\Form_Views; use SRFM\Inc\Global_Settings\Global_Settings; use SRFM\Inc\Helper; use SRFM\Inc\Onboarding; @@ -1700,6 +1701,11 @@ public function enqueue_scripts() { $current_user = wp_get_current_user(); $localization_data = [ + // Gates the Views / Conversion Rate columns on the Forms table. The + // columns are meaningless when tracking is off — no new views are being + // recorded — so they are hidden rather than shown frozen at their last + // value. Same setting the beacon and the REST endpoint check. + 'form_views_tracking' => Form_Views::get_instance()->is_tracking_enabled(), 'site_url' => get_site_url(), 'current_user_login' => $current_user->user_login ?? '', 'website_lead_details' => [ diff --git a/inc/forms-data.php b/inc/forms-data.php index 4c3bc3616..31e45eb13 100644 --- a/inc/forms-data.php +++ b/inc/forms-data.php @@ -202,7 +202,11 @@ public function get_forms_list( $request ) { // Execute query — use try/finally to guarantee filter cleanup. try { - if ( in_array( $orderby, [ 'views', 'conversion_rate' ], true ) ) { + // Only take the derived-metric path while tracking is on. With the feature + // off the columns are hidden, so a stored or hand-crafted request to sort by + // them would run the expensive compute-sort-paginate pass to order rows + // nobody can see; fall through to the default ordering instead. + if ( in_array( $orderby, [ 'views', 'conversion_rate' ], true ) && Form_Views::get_instance()->is_tracking_enabled() ) { // Derived metrics can't be sorted by WP_Query; compute across all // matching forms, sort, then paginate. $response_data = $this->get_forms_sorted_by_metric( $args, $orderby, $order, $page, Helper::get_integer_value( $per_page ) ); @@ -338,8 +342,17 @@ private function prepare_form_for_listing( $post ) { // Views (impressions) and derived conversion rate. Clamp to 100%: entries are // all-time while views only accrue from when this feature shipped, so early on a // form can have more entries than counted views (which would read >100%). - $views = Form_Views::get_instance()->get_views( $form_id ); - $conversion_rate = $views > 0 ? round( min( 100, $entries_count / $views * 100 ), 1 ) : 0.0; + // + // Skipped entirely when tracking is off — the columns are hidden then, so this + // would be a meta read per form on every listing request for values nothing + // renders. + $views = 0; + $conversion_rate = 0.0; + + if ( Form_Views::get_instance()->is_tracking_enabled() ) { + $views = Form_Views::get_instance()->get_views( $form_id ); + $conversion_rate = $views > 0 ? round( min( 100, $entries_count / $views * 100 ), 1 ) : 0.0; + } return [ 'id' => $form_id, diff --git a/src/admin/forms/components/FormsTable.js b/src/admin/forms/components/FormsTable.js index 644ee0ce9..a5e82d4a4 100644 --- a/src/admin/forms/components/FormsTable.js +++ b/src/admin/forms/components/FormsTable.js @@ -178,47 +178,56 @@ const FormsTable = ( { ), }, - { - label: __( 'Views', 'sureforms' ), - key: 'views', - sortable: true, - headerClassName: 'w-[8%]', - render: ( form ) => ( - - { Number( form.views ?? 0 ).toLocaleString() } - - ), - }, - { - label: __( 'Conversion Rate', 'sureforms' ), - key: 'conversion_rate', - sortable: true, - headerClassName: 'w-[10%]', - render: ( form ) => { - const views = Number( form.views ?? 0 ); - if ( ! views ) { - // No views yet — nothing to convert against. - return ( - - — + // Views and Conversion Rate are hidden when view tracking is switched off in + // General settings: with the beacon disabled no new views are recorded, so the + // columns would sit frozen at whatever was counted before and read as broken. + // Defaults to shown when the flag is absent, matching the server-side default + // for installs that predate the setting. + ...( false === window.srfm_admin?.form_views_tracking + ? [] + : [ + { + label: __( 'Views', 'sureforms' ), + key: 'views', + sortable: true, + headerClassName: 'w-[8%]', + render: ( form ) => ( + + { Number( form.views ?? 0 ).toLocaleString() } - ); - } - return ( - - { `${ Number( - form.conversion_rate ?? 0 - ).toLocaleString() }%` } - - ); - }, - }, + ), + }, + { + label: __( 'Conversion Rate', 'sureforms' ), + key: 'conversion_rate', + sortable: true, + headerClassName: 'w-[10%]', + render: ( form ) => { + const views = Number( form.views ?? 0 ); + if ( ! views ) { + // No views yet — nothing to convert against. + return ( + + — + + ); + } + return ( + + { `${ Number( + form.conversion_rate ?? 0 + ).toLocaleString() }%` } + + ); + }, + }, + ] ), { label: __( 'Date & Time', 'sureforms' ), key: 'date', From 4173b16d9688316eac47be21cd41514a72d6a937 Mon Sep 17 00:00:00 2001 From: Vansh Kapoor Date: Thu, 20 Aug 2026 20:55:08 +0530 Subject: [PATCH 03/50] =?UTF-8?q?fix:=20actually=20hide=20the=20columns=20?= =?UTF-8?q?=E2=80=94=20the=20flag=20crossed=20the=20PHP/JS=20boundary=20as?= =?UTF-8?q?=20a=20string?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit did not work. WP_Scripts::localize() casts every scalar in the payload with (string) (wp-includes/class-wp-scripts.php), so the PHP boolean false reached JS as an empty string and the guard false === window.srfm_admin?.form_views_tracking could never match. The columns stayed visible with tracking off. My error was verifying one side and assuming the boundary: I checked is_tracking_enabled() and prepare_form_for_listing() against the real option and reported it working, without ever reading the value that actually arrives in the browser. The contract is now explicit on both sides. PHP sends '1'/'0' rather than a boolean, with the reason recorded at the call site so nobody 'tidies' it back, and the JS compares as a string, defaulting a missing flag to '1' so the columns stay visible if the PHP predates the setting. Verified in the browser on both states this time, reading the rendered table headers rather than the source: tracking OFF -> flag '0' (typeof string) -> headers: Title, Shortcode, Entries, Date & Time, Actions tracking ON -> flag '1' -> headers: ... Entries, Views, Conversion Rate, Date & Time, Actions PHPCS 0, PHPStan 0, phpinsights --min-quality=100 0, ESLint 0, build 0. --- admin/admin.php | 6 +++++- src/admin/forms/components/FormsTable.js | 9 ++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/admin/admin.php b/admin/admin.php index 465e63417..7333fac8a 100644 --- a/admin/admin.php +++ b/admin/admin.php @@ -1705,7 +1705,11 @@ public function enqueue_scripts() { // columns are meaningless when tracking is off — no new views are being // recorded — so they are hidden rather than shown frozen at their last // value. Same setting the beacon and the REST endpoint check. - 'form_views_tracking' => Form_Views::get_instance()->is_tracking_enabled(), + // + // Sent as '1'/'0' rather than a boolean on purpose: WP_Scripts::localize() + // casts every scalar with (string), so `false` would reach JS as an empty + // string and an identity check against `false` there could never match. + 'form_views_tracking' => Form_Views::get_instance()->is_tracking_enabled() ? '1' : '0', 'site_url' => get_site_url(), 'current_user_login' => $current_user->user_login ?? '', 'website_lead_details' => [ diff --git a/src/admin/forms/components/FormsTable.js b/src/admin/forms/components/FormsTable.js index a5e82d4a4..d0401abf0 100644 --- a/src/admin/forms/components/FormsTable.js +++ b/src/admin/forms/components/FormsTable.js @@ -181,9 +181,12 @@ const FormsTable = ( { // Views and Conversion Rate are hidden when view tracking is switched off in // General settings: with the beacon disabled no new views are recorded, so the // columns would sit frozen at whatever was counted before and read as broken. - // Defaults to shown when the flag is absent, matching the server-side default - // for installs that predate the setting. - ...( false === window.srfm_admin?.form_views_tracking + // + // Compared as a string because wp_localize_script casts every scalar through + // (string) — a PHP boolean arrives here as '1' or '', never true/false. PHP + // sends '1'/'0' explicitly; a missing flag falls back to '1' so the columns + // stay visible on an install whose PHP predates this setting. + ...( '0' === String( window.srfm_admin?.form_views_tracking ?? '1' ) ? [] : [ { From de06e243ce25ede1d056e36a8001d1c002919417 Mon Sep 17 00:00:00 2001 From: Vansh Kapoor Date: Fri, 21 Aug 2026 10:09:50 +0530 Subject: [PATCH 04/50] feat: measure conversion from when view tracking started MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Conversion rate divided all-time entries by views that only began accruing when this feature shipped, so any form that already existed reported a rate built from two different periods. The min( 100, ... ) clamp hid the absurdity rather than the error: a form with 500 entries and 3 views displayed a confident 100%, and since the column sorts, those forms sorted above genuinely good ones. Entries are now counted from the same moment as views. - Form_Views::TRACKING_STARTED_OPTION records when counting opened. Written once, on first use, and never rewritten — tracking is on by default so there is no toggle event to hang it on, and add_option() means a concurrent request cannot move a window that is already open. Kept in its own option rather than inside srfm_general_settings_options, because that array is rebuilt from an allowlist on save and an unrecognised key would be silently dropped. - Forms_Data counts entries with created_at >= the window, reusing the where_clause the entries table already accepts and the (form_id, created_at, status) index. A form created after the window opened measures from its own creation instead, so its first days are not diluted by a window it did not exist for. - conversion_rate is now nullable. More entries than views is impossible, so rather than clamping, the value is null and the table shows the dash it already uses for 'no data yet'. The Entries column is untouched and stays all-time. Not re-stamped when tracking is switched off and on: the earlier view counts are kept, and moving the start forward would measure them against a shorter entry window. Entries arriving while tracking is off skew the rate slightly, which the entries-exceed-views guard covers when it becomes meaningless. Verified against real data rather than by inspection. The timestamp is stable across repeated calls and not autoloaded. With the window opened now, a legacy form's older entry is excluded and the rate reads 0.0 instead of the previous 100.0; with the window backdated two years the same entry is counted and the rate reads 25.0 against 4 views; zero views yields null. PHPCS 0, PHPStan level 9 0, phpinsights --min-quality=100 0, ESLint 0, build 0, 19 tests (46 assertions). --- inc/form-views.php | 53 ++++++++++++++++++++++++ inc/forms-data.php | 44 ++++++++++++++++++-- src/admin/forms/components/FormsTable.js | 4 +- 3 files changed, 96 insertions(+), 5 deletions(-) diff --git a/inc/form-views.php b/inc/form-views.php index 7c3dbe20d..ddf3271b2 100644 --- a/inc/form-views.php +++ b/inc/form-views.php @@ -47,6 +47,21 @@ class Form_Views { */ public const SETTING_KEY = 'srfm_form_views_tracking'; + /** + * Option holding the unix time from which view counting has been running. + * + * Conversion rate divides entries by views, and views only start accruing when + * tracking is switched on — so counting entries from the beginning of time would + * compare two different periods and report a wildly inflated rate on any form + * that existed beforehand. Entries are therefore counted from this moment too. + * + * Deliberately its own option rather than a key inside + * `srfm_general_settings_options`: that array is rebuilt from an allowlist when + * settings are saved, so an unrecognised key added to it would be silently + * dropped on the next save. + */ + public const TRACKING_STARTED_OPTION = 'srfm_form_views_tracking_started_at'; + /** * Max beacon hits accepted per visitor IP + form within the rate-limit window. */ @@ -66,6 +81,44 @@ public function __construct() { add_action( 'wp_footer', [ $this, 'localize_beacon' ], 5 ); } + /** + * Unix time from which views have been counted. + * + * Written once, on first use, and never rewritten. Tracking is on by default, so + * there is no toggle event to hang this on for the majority of installs — the + * window simply opens the first time anything asks for it, which on an upgrade is + * the moment the feature becomes live. + * + * Not re-stamped when tracking is switched off and on again: the stored view + * counts from before the gap are kept, so moving the start forward would measure + * those views against a shorter entry window. Entries that arrive while tracking + * is off do skew the rate slightly, and the entries-exceed-views guard in + * Forms_Data covers the case where that skew makes the number meaningless. + * + * add_option() rather than update_option() so a concurrent request cannot move a + * window that is already open. + * + * @since x.x.x + * @return int Unix timestamp. + */ + public function get_tracking_started_at() { + $started = Helper::get_integer_value( get_option( self::TRACKING_STARTED_OPTION, 0 ) ); + + if ( $started > 0 ) { + return $started; + } + + $started = time(); + + // Only creates the option when it does not already exist, so the first writer + // wins and later calls read that value back. + if ( ! add_option( self::TRACKING_STARTED_OPTION, $started, '', false ) ) { + $started = Helper::get_integer_value( get_option( self::TRACKING_STARTED_OPTION, $started ) ); + } + + return $started; + } + /** * Register the public `forms/track-view` REST route on the SureForms endpoints array. * diff --git a/inc/forms-data.php b/inc/forms-data.php index 31e45eb13..b66d53470 100644 --- a/inc/forms-data.php +++ b/inc/forms-data.php @@ -346,12 +346,48 @@ private function prepare_form_for_listing( $post ) { // Skipped entirely when tracking is off — the columns are hidden then, so this // would be a meta read per form on every listing request for values nothing // renders. - $views = 0; - $conversion_rate = 0.0; + $views = 0; + // null means "not computable yet", which the table renders as a dash. 0.0 would + // claim a real measurement of zero. + $conversion_rate = null; if ( Form_Views::get_instance()->is_tracking_enabled() ) { - $views = Form_Views::get_instance()->get_views( $form_id ); - $conversion_rate = $views > 0 ? round( min( 100, $entries_count / $views * 100 ), 1 ) : 0.0; + $views = Form_Views::get_instance()->get_views( $form_id ); + + // Compare like with like. The Entries column is all-time, but views only + // start accruing when tracking opens, so the rate counts entries from that + // same moment — otherwise a form that existed beforehand divides years of + // entries by days of views and reports a rate that is pure noise. + // + // A form created after the window opened measures from its own creation + // instead, so its first days are not diluted by a window it did not exist for. + $window_start = Form_Views::get_instance()->get_tracking_started_at(); + $form_created = strtotime( (string) $post->post_date_gmt ); + $window_start = $form_created && $form_created > $window_start ? $form_created : $window_start; + $entries_since = Helper::get_integer_value( + Entries::get_total_entries_by_status( + 'all', + $form_id, + [ + [ + [ + 'key' => 'created_at', + 'compare' => '>=', + 'value' => gmdate( 'Y-m-d H:i:s', $window_start ), + ], + ], + ] + ) + ); + + // More entries than views is impossible — every entry needs a view first — + // so the count is incomplete and any percentage would be invented. null tells + // the table to show its "no data yet" dash rather than a clamped 100%. + if ( $views > 0 && $entries_since <= $views ) { + $conversion_rate = round( $entries_since / $views * 100, 1 ); + } elseif ( $views > 0 ) { + $conversion_rate = null; + } } return [ diff --git a/src/admin/forms/components/FormsTable.js b/src/admin/forms/components/FormsTable.js index d0401abf0..b7313d469 100644 --- a/src/admin/forms/components/FormsTable.js +++ b/src/admin/forms/components/FormsTable.js @@ -207,7 +207,9 @@ const FormsTable = ( { headerClassName: 'w-[10%]', render: ( form ) => { const views = Number( form.views ?? 0 ); - if ( ! views ) { + // No views, or a rate the server could not compute (entries + // predate the tracking window, so the ratio would be invented). + if ( ! views || null === form.conversion_rate ) { // No views yet — nothing to convert against. return ( Date: Fri, 21 Aug 2026 11:05:18 +0530 Subject: [PATCH 05/50] fix: keep counting views while the columns are hidden MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The General-settings toggle now governs display only. should_track() no longer consults it, so the beacon and REST endpoint keep counting while the Views and Conversion Rate columns are hidden — switching them back on reveals the period rather than a gap. The server-side gates in forms-data.php (payload + orderby) stay, since they are what hides the data. Settings copy and the is_tracking_enabled() docblock rewritten to describe display-only semantics, and the test_is_tracking_enabled assertion inverted: it now fails if the toggle check is ever put back into should_track(). --- inc/form-views.php | 24 +++++++++++++++--------- src/admin/settings/pages/General.js | 4 ++-- tests/unit/inc/test-form-views.php | 7 ++++--- 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/inc/form-views.php b/inc/form-views.php index ddf3271b2..fcc29e86f 100644 --- a/inc/form-views.php +++ b/inc/form-views.php @@ -230,9 +230,14 @@ public function get_views( $form_id ) { } /** - * Whether view + conversion-rate tracking is enabled in the global General - * settings. Defaults to true when the setting has never been saved so existing - * installs keep tracking without an explicit opt-in. + * Whether the Views and Conversion Rate columns are shown on the Forms list. + * + * Despite the setting key's name this governs display only — counting runs + * regardless, so switching the columns back on reveals the period they were + * hidden for rather than a gap. See should_track(). + * + * Defaults to true when the setting has never been saved, so existing installs + * get the columns without an explicit opt-in. * * @since x.x.x * @return bool @@ -250,17 +255,18 @@ public function is_tracking_enabled() { /** * Whether the current request should be counted as a view. * - * Excludes requests when tracking is disabled globally, logged-in users who can - * edit content (author/editor/admin), and form-builder / Instant Form live previews. + * Excludes logged-in users who can edit content (author/editor/admin) and + * form-builder / Instant Form live previews. + * + * Deliberately does not consult the General-settings toggle: that setting governs + * whether the Views and Conversion Rate columns are shown, not whether counting + * happens. Counting continues in the background so switching the columns back on + * reveals the period they were hidden for, rather than a gap. * * @since x.x.x * @return bool */ private function should_track() { - if ( ! $this->is_tracking_enabled() ) { - return false; - } - if ( is_user_logged_in() && current_user_can( 'edit_posts' ) ) { return false; } diff --git a/src/admin/settings/pages/General.js b/src/admin/settings/pages/General.js index 6f1612e33..b3da83a32 100644 --- a/src/admin/settings/pages/General.js +++ b/src/admin/settings/pages/General.js @@ -230,11 +230,11 @@ const FormViewsTrackingContent = ( { false ] ); - $this->assertFalse( $this->views->is_tracking_enabled(), 'Explicit false should disable tracking.' ); - // Disabling tracking also short-circuits should_track for anonymous visitors. + $this->assertFalse( $this->views->is_tracking_enabled(), 'Explicit false should hide the columns.' ); + // The toggle governs display only — counting must continue while the columns are hidden, + // so switching them back on reveals the period rather than a gap. wp_set_current_user( 0 ); - $this->assertFalse( $this->call_private_method( $this->views, 'should_track' ), 'should_track must respect the global toggle.' ); + $this->assertTrue( $this->call_private_method( $this->views, 'should_track' ), 'should_track must ignore the display toggle and keep counting.' ); // Key present and true → enabled. update_option( 'srfm_general_settings_options', [ 'srfm_form_views_tracking' => true ] ); From f56aaa6cfa7185dd8e3758450fe817d49fc7db3a Mon Sep 17 00:00:00 2001 From: Vansh Kapoor Date: Fri, 21 Aug 2026 11:14:07 +0530 Subject: [PATCH 06/50] test: cover get_tracking_started_at() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the check-test-coverage gap on the conversion-rate window stamp. Pins the three invariants the rate depends on: the stamp is written once and never drifts, an existing stamp is returned verbatim, and the option is not autoloaded. Verified by revert — flipping autoload to true fails the third assertion, dropping the stored-value early return fails the second. Also drops the "switched off and on again" paragraph from the docblock: counting no longer stops, so there is no gap for entries to skew. --- inc/form-views.php | 8 +++---- tests/unit/inc/test-form-views.php | 38 ++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/inc/form-views.php b/inc/form-views.php index fcc29e86f..9d7b79dce 100644 --- a/inc/form-views.php +++ b/inc/form-views.php @@ -89,11 +89,9 @@ public function __construct() { * window simply opens the first time anything asks for it, which on an upgrade is * the moment the feature becomes live. * - * Not re-stamped when tracking is switched off and on again: the stored view - * counts from before the gap are kept, so moving the start forward would measure - * those views against a shorter entry window. Entries that arrive while tracking - * is off do skew the rate slightly, and the entries-exceed-views guard in - * Forms_Data covers the case where that skew makes the number meaningless. + * Never re-stamped. Counting runs regardless of the display toggle, so the stamp + * always matches the period the stored view counts cover; moving it forward would + * measure those views against a shorter entry window. * * add_option() rather than update_option() so a concurrent request cannot move a * window that is already open. diff --git a/tests/unit/inc/test-form-views.php b/tests/unit/inc/test-form-views.php index 776f3a848..efe4ce855 100644 --- a/tests/unit/inc/test-form-views.php +++ b/tests/unit/inc/test-form-views.php @@ -123,6 +123,44 @@ public function test_is_tracking_enabled() { delete_option( 'srfm_general_settings_options' ); } + // ────────────────────────────────────────────── + // get_tracking_started_at (conversion-rate window) + // ────────────────────────────────────────────── + + public function test_get_tracking_started_at() { + delete_option( Form_Views::TRACKING_STARTED_OPTION ); + + $before = time(); + $started = $this->views->get_tracking_started_at(); + $after = time(); + + $this->assertGreaterThanOrEqual( $before, $started, 'First call should stamp the current time.' ); + $this->assertLessThanOrEqual( $after, $started, 'First call should stamp the current time.' ); + + // Write-once: the stamp must not drift on later calls, or the conversion-rate + // window would keep sliding forward and hide entries it already counted. + $this->assertSame( $started, $this->views->get_tracking_started_at(), 'Second call must return the stored stamp.' ); + $this->assertSame( $started, (int) get_option( Form_Views::TRACKING_STARTED_OPTION ), 'Stamp must be persisted.' ); + + // Not autoloaded — this is read only on the Forms list screen. + global $wpdb; + $autoload = $wpdb->get_var( + $wpdb->prepare( + "SELECT autoload FROM {$wpdb->options} WHERE option_name = %s", // phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- Core table name from $wpdb. + Form_Views::TRACKING_STARTED_OPTION + ) + ); + // WP 6.6+ stores 'off' for an explicit false; older cores store 'no'. + $this->assertContains( $autoload, [ 'no', 'off' ], 'Stamp must not be autoloaded on every request.' ); + + // An existing stamp is returned verbatim, never re-stamped. + delete_option( Form_Views::TRACKING_STARTED_OPTION ); + add_option( Form_Views::TRACKING_STARTED_OPTION, 1000000000, '', false ); + $this->assertSame( 1000000000, $this->views->get_tracking_started_at(), 'Existing stamp must be preserved.' ); + + delete_option( Form_Views::TRACKING_STARTED_OPTION ); + } + // ────────────────────────────────────────────── // permissions_check (HMAC token) // ────────────────────────────────────────────── From 6940a88131e1c3d2caf5e9bbe38d8f8d1c3967a6 Mon Sep 17 00:00:00 2001 From: Vansh Kapoor Date: Fri, 21 Aug 2026 11:19:33 +0530 Subject: [PATCH 07/50] i18n: drop the version placeholder from the tracking setting description MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The placeholder sat inside a translatable string, and Gruntfile's plugin_function_comment task rewrites a bare /x\.x\.x/gi across src/**/*.js at release. That changes the msgid on every version bump, so every translation of this string would be lost each release. No other user-facing string in src/ does this — the remaining placeholders are all @since docblocks. "when this feature first runs on your site" is also the more accurate statement: get_tracking_started_at() stamps the window on first use, which on a site that upgrades late is not the release date. --- src/admin/settings/pages/General.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/admin/settings/pages/General.js b/src/admin/settings/pages/General.js index b3da83a32..d8e22bb83 100644 --- a/src/admin/settings/pages/General.js +++ b/src/admin/settings/pages/General.js @@ -234,7 +234,7 @@ const FormViewsTrackingContent = ( { 'sureforms' ), description: __( - 'Adds the Views and Conversion Rate columns to the Forms list. A view is counted once per page visit when the form appears on screen, and the conversion rate is the share of those views that ended in a submission. Counting starts from version x.x.x, so submissions received before then are not counted towards the rate. Turning this off hides the columns but keeps counting, so the figures are up to date if you switch it back on.', + 'Adds the Views and Conversion Rate columns to the Forms list. A view is counted once per page visit when the form appears on screen, and the conversion rate is the share of those views that ended in a submission. Counting begins when this feature first runs on your site, so submissions received before then are not counted towards the rate. Turning this off hides the columns but keeps counting, so the figures are up to date if you switch it back on.', 'sureforms' ), } } From 9cdf5183b670084a3ff41a7f1de5af22e6e4f7b4 Mon Sep 17 00:00:00 2001 From: Vansh Kapoor Date: Mon, 24 Aug 2026 08:38:01 +0530 Subject: [PATCH 08/50] fix: stop the Edit Form pill overlapping fields, and attribute its clicks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pill was absolutely positioned at the container's top-right, which only clears the fields when the container happens to have enough top padding. With default styling it landed on top of the first row's last field. It now renders in normal flow on its own right-aligned row immediately above the
, so it cannot overlap anything at any width with any theme. The form shifts down by the pill's height, but only for users who can edit it — the markup is still absent from the DOM for everyone else. Analytics: the edit link carries an `srfm_edit_src=embed` marker that Admin::maybe_track_edit_form_button_click() reads on load-post.php and counts into `edit_form_button_clicks`, emitted as the cumulative `edit_form_button_clicked` event. Attributing via the URL keeps the front end script-free and adds no AJAX endpoint, and measures the editor actually opening rather than a click that may not land. Every decision is taken from server state: the post type comes from the resolved post and the capability from `edit_post` on that form, so an absent, misspelled or reused marker, a non-form post, and a user without the capability all fall through to no-op. Also fixes a latent fatal in the same function: the Elementor guard checked only `class_exists( '\Elementor\Plugin' )`, but Elementor declares `public static $instance = null` and populates it on boot, so `null->editor->is_edit_mode()` was reachable. That is a fatal Error, not a warning, and it was already breaking two tests in this class on dev. --- admin/admin.php | 55 ++++++++++ inc/generate-form-markup.php | 96 +++++++++++------ tests/unit/admin/test-admin.php | 106 +++++++++++++++++++ tests/unit/inc/test-generate-form-markup.php | 35 +++++- 4 files changed, 258 insertions(+), 34 deletions(-) diff --git a/admin/admin.php b/admin/admin.php index 35e38aeea..01d8e8602 100644 --- a/admin/admin.php +++ b/admin/admin.php @@ -10,6 +10,7 @@ use Astra_Notices; use SRFM\Inc\AI_Form_Builder\AI_Helper; use SRFM\Inc\Database\Tables\Entries; +use SRFM\Inc\Generate_Form_Markup; use SRFM\Inc\Global_Settings\Global_Settings; use SRFM\Inc\Helper; use SRFM\Inc\Onboarding; @@ -181,6 +182,7 @@ public function __construct() { add_action( 'wp_ajax_sureforms_accept_cta', [ $this, 'pointer_accepted_cta' ] ); add_action( 'wp_ajax_srfm_notice_response', [ $this, 'handle_notice_response' ] ); add_action( 'wp_ajax_srfm_ai_widget_usage', [ $this, 'track_ai_widget_usage' ] ); + add_action( 'load-post.php', [ $this, 'maybe_track_edit_form_button_click' ] ); // Register dashboard widget only if there are recent entries. add_action( 'admin_init', [ $this, 'maybe_register_dashboard_widget' ] ); @@ -2827,6 +2829,59 @@ public function enqueue_ai_dashboard_widget_assets( $hook_suffix ) { wp_add_inline_script( 'srfm-ai-dashboard-widget', $inline_script ); } + /** + * Count an editor visit that came from the front-end "Edit Form" pill. + * + * The pill is a plain link, so the click is attributed by the marker query arg + * it carries rather than by a front-end click handler. That keeps the front end + * script-free and adds no AJAX endpoint: the only thing on the page is still an + * anchor. It also measures the outcome that matters — the editor actually + * opening — instead of a click that may never land. + * + * Every decision here comes from server state. The query arg selects the code + * path; what gets counted is derived from the resolved post and the current + * user's capability on it. An absent, empty, misspelled or reused arg, a post + * that is not a SureForms form, and a user without `edit_post` on that form all + * fall through to no-op without an explicit branch. + * + * No nonce, deliberately: the pill is rendered into front-end HTML that may be + * page-cached, so a nonce would either be baked into the cache or be stale on + * arrival. Nothing here is worth protecting with one — the effect is a private + * usage counter for a user who can already edit the form, so the worst a forged + * link achieves is inflating our own telemetry by one. + * + * @return void + * @since x.x.x + */ + public function maybe_track_edit_form_button_click() { + // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Read-only attribution marker; see docblock for why a nonce is neither possible nor needed. + $source = isset( $_GET[ Generate_Form_Markup::EDIT_FORM_BUTTON_SOURCE_ARG ] ) ? sanitize_key( wp_unslash( $_GET[ Generate_Form_Markup::EDIT_FORM_BUTTON_SOURCE_ARG ] ) ) : ''; + + if ( 'embed' !== $source ) { + return; + } + + // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Same read-only path as above. + $post_id = isset( $_GET['post'] ) ? absint( wp_unslash( $_GET['post'] ) ) : 0; + + // Resolve the post type from the stored post, never from the request, and + // confirm the visitor may edit that specific form before counting anything. + if ( 0 === $post_id || SRFM_FORMS_POST_TYPE !== get_post_type( $post_id ) ) { + return; + } + + if ( ! current_user_can( 'edit_post', $post_id ) ) { + return; + } + + $count = Helper::get_integer_value( Helper::get_srfm_option( 'edit_form_button_clicks', 0 ) ) + 1; + Helper::update_srfm_option( 'edit_form_button_clicks', $count ); + + // $force = true because this is a cumulative counter, not a one-time event — + // it must re-send the latest count each cycle (bypasses one-time dedup). + Analytics::events()->track( 'edit_form_button_clicked', (string) $count, [], true ); + } + /** * Track AI dashboard widget usage. * diff --git a/inc/generate-form-markup.php b/inc/generate-form-markup.php index f0f55cc5a..0ceef5719 100644 --- a/inc/generate-form-markup.php +++ b/inc/generate-form-markup.php @@ -24,6 +24,14 @@ class Generate_Form_Markup { use Get_Instance; + /** + * Query arg marking an editor visit as arriving from the front-end "Edit Form" + * pill, so the click can be attributed without any front-end JavaScript. + * + * @since x.x.x + */ + public const EDIT_FORM_BUTTON_SOURCE_ARG = 'srfm_edit_src'; + /** * Current block attributes for the form being rendered. * Used by child blocks (like inline button) to access parent form's embed styling. @@ -856,6 +864,17 @@ public static function get_form_markup( $id, $show_title_current_page = true, $s } $submit_token = Submit_Token::generate( (int) $id ); + // Admin-only shortcut into the form editor. Emitted here, immediately + // above the , so it occupies its own row in normal flow and can + // never overlap a field. Gated on the same condition as the + // `.srfm-form-container` open above, so a zero-block form (no + // container) never emits an orphaned pill. Works for every embed + // method (block, shortcode, widget) because they all render through + // this function. + if ( '' !== $id && 0 !== $block_count ) { + self::render_edit_form_button( (int) $id ); + } + ?> false`, so `edit_post` collapses to a blanket @@ -1390,7 +1406,15 @@ public static function render_edit_form_button( $form_id ) { // Page-builder editor canvases render the form directly (not over REST), // where their own element-edit handles would collide with the pill. - if ( class_exists( '\Elementor\Plugin' ) && \Elementor\Plugin::$instance->editor->is_edit_mode() ) { + // `$instance` is checked as well as the class name: Elementor declares + // `public static $instance = null` and only populates it on boot, so the + // class can exist while the singleton is still null. Dereferencing it then + // is a fatal Error, not a warning, and guarding only on class_exists() left + // that reachable — test-generate-form-markup.php hit it. The bundled stub + // types $instance as non-nullable, which is why PHPStan reads the isset() + // as redundant and has to be told otherwise. + // @phpstan-ignore-next-line -- Stub disagrees with runtime; see above. + if ( class_exists( '\Elementor\Plugin' ) && isset( \Elementor\Plugin::$instance ) && \Elementor\Plugin::$instance->editor->is_edit_mode() ) { return; } if ( function_exists( 'bricks_is_builder' ) && bricks_is_builder() ) { @@ -1415,6 +1439,12 @@ public static function render_edit_form_button( $form_id ) { return; } + // Attribution marker read back by Admin::maybe_track_edit_form_button_click() + // when the editor loads. Added before the filter below so an integration that + // replaces the link wholesale drops the marker with it, rather than having our + // query arg appended to a third-party URL. + $edit_link = add_query_arg( self::EDIT_FORM_BUTTON_SOURCE_ARG, 'embed', $edit_link ); + /** * Filter the target of the admin "Edit Form" shortcut. * @@ -1439,34 +1469,38 @@ public static function render_edit_form_button( $form_id ) { } wp_enqueue_style( $style_handle ); ?> - - - - - +
+ + + + + +
assertSame( $before + 1, $after, 'Usage tracking must increment the ai_dashboard_widget_uses counter.' ); } + + /** + * The front-end "Edit Form" pill is attributed by a marker query arg rather + * than a click handler, so the counter must only move when the editor is + * genuinely opened from that pill, by someone allowed to edit that form. + * + * Every rejection path is asserted, because each one is a way the counter + * could be inflated by a crafted URL: no marker, a marker with the wrong + * value, a marker pointed at a post that is not a SureForms form, and a user + * without the capability. The counter is read back from the stored option, so + * a guard that silently stops incrementing also fails here. + */ + public function test_maybe_track_edit_form_button_click() { + if ( ! defined( 'SRFM_FORMS_POST_TYPE' ) ) { + $this->markTestSkipped( 'SRFM_FORMS_POST_TYPE not defined' ); + } + + $admin = Admin::get_instance(); + $arg = \SRFM\Inc\Generate_Form_Markup::EDIT_FORM_BUTTON_SOURCE_ARG; + + $form_id = wp_insert_post( + [ + 'post_title' => 'Pill Analytics Form', + 'post_type' => SRFM_FORMS_POST_TYPE, + 'post_status' => 'publish', + ] + ); + $page_id = wp_insert_post( + [ + 'post_title' => 'Not A Form', + 'post_type' => 'post', + 'post_status' => 'publish', + ] + ); + + $administrator = wp_insert_user( + [ + 'user_login' => 'srfm_pill_admin_' . wp_rand(), + 'user_pass' => 'password', + 'user_email' => 'srfm_pill_admin_' . wp_rand() . '@example.com', + 'role' => 'administrator', + ] + ); + $subscriber = wp_insert_user( + [ + 'user_login' => 'srfm_pill_sub_' . wp_rand(), + 'user_pass' => 'password', + 'user_email' => 'srfm_pill_sub_' . wp_rand() . '@example.com', + 'role' => 'subscriber', + ] + ); + + $count = static function () { + return Helper::get_integer_value( Helper::get_srfm_option( 'edit_form_button_clicks', 0 ) ); + }; + + Helper::update_srfm_option( 'edit_form_button_clicks', 0 ); + wp_set_current_user( is_wp_error( $administrator ) ? 0 : (int) $administrator ); + + // No marker at all — an ordinary editor visit must not be counted. + $_GET = [ 'post' => $form_id ]; + $admin->maybe_track_edit_form_button_click(); + $this->assertSame( 0, $count(), 'An editor visit without the marker must not be counted.' ); + + // Marker present but not the value we emit. + $_GET = [ 'post' => $form_id, $arg => 'somewhere-else' ]; + $admin->maybe_track_edit_form_button_click(); + $this->assertSame( 0, $count(), 'An unrecognised marker value must not be counted.' ); + + // Marker reused against a post that is not a SureForms form. + $_GET = [ 'post' => $page_id, $arg => 'embed' ]; + $admin->maybe_track_edit_form_button_click(); + $this->assertSame( 0, $count(), 'A non-form post must not be counted.' ); + + // Marker with no post at all. + $_GET = [ $arg => 'embed' ]; + $admin->maybe_track_edit_form_button_click(); + $this->assertSame( 0, $count(), 'A missing post ID must not be counted.' ); + + // The real path: administrator opening this form from the pill. + $_GET = [ 'post' => $form_id, $arg => 'embed' ]; + $admin->maybe_track_edit_form_button_click(); + $this->assertSame( 1, $count(), 'A genuine pill click should be counted.' ); + + // Cumulative, not one-time — a second visit counts again. + $admin->maybe_track_edit_form_button_click(); + $this->assertSame( 2, $count(), 'The counter should be cumulative.' ); + + // A user who cannot edit the form gets nothing, marker or not. + wp_set_current_user( is_wp_error( $subscriber ) ? 0 : (int) $subscriber ); + $admin->maybe_track_edit_form_button_click(); + $this->assertSame( 2, $count(), 'A user without edit_post must not be counted.' ); + + $_GET = []; + wp_set_current_user( 0 ); + Helper::update_srfm_option( 'edit_form_button_clicks', 0 ); + wp_delete_post( $form_id, true ); + wp_delete_post( $page_id, true ); + if ( ! is_wp_error( $administrator ) ) { + wp_delete_user( (int) $administrator ); + } + if ( ! is_wp_error( $subscriber ) ) { + wp_delete_user( (int) $subscriber ); + } + } } /** @@ -1580,4 +1685,5 @@ public function test_enqueue_thankyou_notice_tracking() { wp_dequeue_script( 'srfm-thankyou-notice-track' ); wp_deregister_script( 'srfm-thankyou-notice-track' ); } + } diff --git a/tests/unit/inc/test-generate-form-markup.php b/tests/unit/inc/test-generate-form-markup.php index 37f2080f3..ed58c8cbf 100644 --- a/tests/unit/inc/test-generate-form-markup.php +++ b/tests/unit/inc/test-generate-form-markup.php @@ -90,14 +90,30 @@ public function test_render_edit_form_button() { $admin = $this->set_current_user_with_role( 'administrator' ); $markup = Generate_Form_Markup::get_form_markup( $form_id ); $this->assertStringContainsString( 'class="srfm-edit-form-btn"', $markup ); - $this->assertStringContainsString( 'href="' . esc_url( get_edit_post_link( $form_id ) ) . '"', $markup ); + $this->assertStringContainsString( 'post=' . $form_id, $markup ); $this->assertTrue( wp_style_is( 'srfm-edit-form-btn', 'enqueued' ), 'The Edit Form stylesheet should be enqueued.' ); + // The pill sits in normal flow ABOVE the form, which is what stops it + // overlapping a field (#3062). DOM order is the guarantee — an overlay only + // clears the fields when the container happens to have enough top padding. + $pill_pos = strpos( $markup, 'srfm-edit-form-btn-wrap' ); + $form_pos = strpos( $markup, 'assertNotFalse( $pill_pos, 'The pill should be wrapped in its flow-level row.' ); + $this->assertNotFalse( $form_pos, 'The form element should be present.' ); + $this->assertLessThan( $form_pos, $pill_pos, 'The pill must render before the , not overlaid on it.' ); + + // ...and the stylesheet must not put it back into an overlay. + $css = $this->call_private_static( Generate_Form_Markup::class, 'get_edit_form_button_css' ); + $this->assertStringNotContainsString( 'position: absolute', $css, 'The pill must not be absolutely positioned over the form.' ); + + // The link carries the attribution marker that Admin reads back on load-post.php. + $this->assertStringContainsString( Generate_Form_Markup::EDIT_FORM_BUTTON_SOURCE_ARG . '=embed', html_entity_decode( $markup ), 'The edit link should carry the analytics source marker.' ); + // A second form links to its OWN editor, not the first — the multiple-forms // acceptance criterion, and a guard against passing the wrong post ID. $markup2 = Generate_Form_Markup::get_form_markup( $form_id2 ); - $this->assertStringContainsString( 'href="' . esc_url( get_edit_post_link( $form_id2 ) ) . '"', $markup2 ); - $this->assertStringNotContainsString( 'href="' . esc_url( get_edit_post_link( $form_id ) ) . '"', $markup2 ); + $this->assertStringContainsString( 'post=' . $form_id2, $markup2 ); + $this->assertStringNotContainsString( 'post=' . $form_id . '&', html_entity_decode( $markup2 ) ); // A suppression filter removes it even for an administrator. add_filter( 'srfm_show_edit_form_button', '__return_false' ); @@ -276,6 +292,19 @@ public function test_register_custom_endpoint() { $this->assertTrue( $found, 'The generate-form-markup endpoint should be registered' ); } + /** + * Invoke a private static method. + * + * @param string $class_name Fully-qualified class name. + * @param string $method_name Method to invoke. + * @return mixed + */ + private function call_private_static( $class_name, $method_name ) { + $method = new ReflectionMethod( $class_name, $method_name ); + $method->setAccessible( true ); + return $method->invoke( null ); + } + /** * Create a user with the given role and make it the current user. * From d4febaff672a385602b0074725bb89bb59067872 Mon Sep 17 00:00:00 2001 From: Vansh Kapoor Date: Mon, 24 Aug 2026 08:40:46 +0530 Subject: [PATCH 09/50] test: pin the Edit Form pill's flow-layout CSS contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit get_edit_form_button_css() changed body without a matching test, which check-test-coverage flags. It also deserves one on its own terms: DOM order alone does not stop the overlap if the CSS lifts the pill back out of flow. Asserts no absolute positioning, no leftover container positioning context, the flow wrapper, and logical spacing. Revert-tested — restoring `position: absolute`, restoring the container rule, and swapping margin-block-end for margin-bottom each fail their own assertion. --- tests/unit/inc/test-generate-form-markup.php | 28 +++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/tests/unit/inc/test-generate-form-markup.php b/tests/unit/inc/test-generate-form-markup.php index ed58c8cbf..01f6b9f75 100644 --- a/tests/unit/inc/test-generate-form-markup.php +++ b/tests/unit/inc/test-generate-form-markup.php @@ -102,10 +102,6 @@ public function test_render_edit_form_button() { $this->assertNotFalse( $form_pos, 'The form element should be present.' ); $this->assertLessThan( $form_pos, $pill_pos, 'The pill must render before the , not overlaid on it.' ); - // ...and the stylesheet must not put it back into an overlay. - $css = $this->call_private_static( Generate_Form_Markup::class, 'get_edit_form_button_css' ); - $this->assertStringNotContainsString( 'position: absolute', $css, 'The pill must not be absolutely positioned over the form.' ); - // The link carries the attribution marker that Admin reads back on load-post.php. $this->assertStringContainsString( Generate_Form_Markup::EDIT_FORM_BUTTON_SOURCE_ARG . '=embed', html_entity_decode( $markup ), 'The edit link should carry the analytics source marker.' ); @@ -125,6 +121,30 @@ public function test_render_edit_form_button() { wp_delete_post( $form_id2, true ); } + /** + * The pill's stylesheet must keep it in normal flow. + * + * This is the half of the overlap fix (#3062) that DOM order alone cannot + * guarantee: rendering above the form is pointless if the CSS lifts the pill + * back out of flow and drops it onto the first row of fields. The container's + * `position: relative` rule goes with it — it existed only to anchor the old + * overlay, so leaving it behind would be dead CSS shipped to every page with + * an embedded form. + */ + public function test_get_edit_form_button_css() { + $css = $this->call_private_static( Generate_Form_Markup::class, 'get_edit_form_button_css' ); + + $this->assertStringNotContainsString( 'position: absolute', $css, 'The pill must not be absolutely positioned over the form.' ); + $this->assertStringNotContainsString( 'position: relative', $css, 'The container no longer needs a positioning context.' ); + $this->assertStringContainsString( '.srfm-edit-form-btn-wrap', $css, 'The flow-level wrapper must be styled.' ); + $this->assertStringContainsString( 'justify-content: flex-end', $css, 'The pill should sit at the inline end of its own row.' ); + + // Logical, not physical — the row has to read correctly in RTL without a + // second rule, which is why the old build used inset-inline-end. + $this->assertStringNotContainsString( 'margin-bottom', $css, 'Spacing should use the logical margin-block-end.' ); + $this->assertStringContainsString( 'margin-block-end', $css ); + } + public function test_add_entries_admin_bar_node() { if ( ! defined( 'SRFM_FORMS_POST_TYPE' ) || ! defined( 'SRFM_ENTRIES' ) ) { $this->markTestSkipped( 'SureForms constants not defined' ); From 05b4265822d118ceabea4a8399447a7441ffd6c5 Mon Sep 17 00:00:00 2001 From: Vansh Kapoor Date: Mon, 24 Aug 2026 08:57:49 +0530 Subject: [PATCH 10/50] feat: report form-views usage to analytics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three signals, so the warehouse can tell whether the feature is actually being used rather than just shipped: - boolean_values.form_views_columns_enabled — whether the Forms list shows the columns. Delegated to Form_Views::is_tracking_enabled() rather than read from the option array, because the setting defaults to ON when the key has never been saved: `! empty()` on the raw array would report every pre-upgrade install as opted out. - numeric_values.total_form_views — views summed across published forms. Drafts and trashed forms are excluded so the numerator cannot outrun a denominator that no longer counts them. - numeric_values.forms_with_views — published forms viewed at least once, which separates "the beacon is firing" from "forms exist but are never embedded". A total alone cannot: one busy form looks like many quiet ones. The sum is a direct query (no core alternative for SUM over postmeta), cached for an hour under the existing sureforms cache group and following the annotation style of most_used_anti_spam(). Revert-tested: including drafts in the sum, relaxing the views filter to >= 0, and reading the toggle raw instead of delegating each fail their own assertion. --- admin/analytics.php | 78 ++++++++++++++++++ tests/unit/admin/test-analytics.php | 119 ++++++++++++++++++++++++++++ 2 files changed, 197 insertions(+) diff --git a/admin/analytics.php b/admin/analytics.php index 20c78ab4b..2aaac3c10 100644 --- a/admin/analytics.php +++ b/admin/analytics.php @@ -8,6 +8,7 @@ namespace SRFM\Admin; use SRFM\Inc\Database\Tables\Entries; +use SRFM\Inc\Form_Views; use SRFM\Inc\Helper; use SRFM\Inc\Learn; use SRFM\Inc\Traits\Get_Instance; @@ -148,6 +149,8 @@ public function add_srfm_analytics_data( $stats_data ) { 'embed_styling_gb_default' => self::embed_styling_gutenberg_count( 'default' ), 'embed_styling_el_default' => self::embed_styling_elementor_count( 'default' ), 'embed_styling_br_default' => self::embed_styling_bricks_count( 'default' ), + 'total_form_views' => $this->total_form_views(), + 'forms_with_views' => $this->forms_with_views(), ]; $stats_data['plugin_data']['sureforms'] = array_merge_recursive( $stats_data['plugin_data']['sureforms'], $this->global_settings_data() ); @@ -488,9 +491,84 @@ public function global_settings_data() { // Payment analytics - check if any payment method is enabled. $global_data['boolean_values']['stripe_enabled'] = $this->is_stripe_enabled(); + // Delegated rather than read from $general_settings directly: the setting + // defaults to ON when the key has never been saved, so `! empty()` on the + // raw array would report every pre-upgrade install as opted out. + $global_data['boolean_values']['form_views_columns_enabled'] = Form_Views::get_instance()->is_tracking_enabled(); + return $global_data; } + /** + * Total page views counted across all published forms. + * + * Paired with `forms_with_views` and the existing `total_entries`, this is what + * makes the conversion figure interpretable in the warehouse: a site with views + * but no entries reads very differently from one with neither. + * + * Counts only published forms, so views left behind by a trashed or draft form + * do not inflate the total against a denominator that no longer includes them. + * + * @since x.x.x + * @return int + */ + public function total_form_views() { + global $wpdb; + + $cache_key = 'srfm_total_form_views'; + $cached_result = wp_cache_get( $cache_key, 'sureforms' ); + + if ( false !== $cached_result ) { + return Helper::get_integer_value( $cached_result ); + } + + // PHPCS: Ignore direct database query warning, as there is no built-in alternative. + // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery + $total = $wpdb->get_var( + $wpdb->prepare( + " + SELECT SUM( pm.meta_value ) + FROM {$wpdb->postmeta} pm + INNER JOIN {$wpdb->posts} p ON p.ID = pm.post_id + WHERE pm.meta_key = %s + AND p.post_type = %s + AND p.post_status = 'publish' + ", + Form_Views::META_KEY, + SRFM_FORMS_POST_TYPE + ) + ); + + $total = Helper::get_integer_value( $total ); + + wp_cache_set( $cache_key, $total, 'sureforms', HOUR_IN_SECONDS ); + + return $total; + } + + /** + * Number of published forms that have been viewed at least once. + * + * Distinguishes "the feature is on but nothing is embedded anywhere" from "the + * beacon is firing" — a total alone cannot, because one busy form looks the + * same as many quiet ones. + * + * @since x.x.x + * @return int + */ + public function forms_with_views() { + return $this->custom_wp_query_total_posts( + [ + [ + 'key' => Form_Views::META_KEY, + 'value' => 0, + 'compare' => '>', + 'type' => 'NUMERIC', + ], + ] + ); + } + /** * Returns user status. * diff --git a/tests/unit/admin/test-analytics.php b/tests/unit/admin/test-analytics.php index 2439d5cf9..ce59cc21f 100644 --- a/tests/unit/admin/test-analytics.php +++ b/tests/unit/admin/test-analytics.php @@ -887,8 +887,127 @@ public function test_forms_using_custom_css_excludes_empty_css() { $this->assertSame( 0, $analytics->forms_using_custom_css() ); } + // ─── Form views & conversion analytics ─────────────────────── + + /** + * `total_form_views` sums the view meta across published forms only. + * + * Trashed and draft forms are excluded deliberately: their views would inflate + * the numerator while `total_entries` and `forms_with_views` no longer count + * them, which is how a warehouse ends up with conversion rates above 100%. + */ + public function test_total_form_views() { + $analytics = Analytics::get_instance(); + + wp_cache_delete( 'srfm_total_form_views', 'sureforms' ); + $this->assertSame( 0, $analytics->total_form_views(), 'No view meta should sum to zero, not null.' ); + + $form_a = $this->create_form_with_views( 7 ); + $form_b = $this->create_form_with_views( 5 ); + $draft = $this->create_form_with_views( 100, 'draft' ); + + wp_cache_delete( 'srfm_total_form_views', 'sureforms' ); + $this->assertSame( 12, $analytics->total_form_views(), 'Published forms should be summed; the draft must not count.' ); + + // The result is cached for an hour, so a later change must not be observed + // until the cache is cleared — pinning that this is a cached read. + update_post_meta( $form_a, \SRFM\Inc\Form_Views::META_KEY, 1000 ); + $this->assertSame( 12, $analytics->total_form_views(), 'The hourly cache should serve the previous total.' ); + + wp_cache_delete( 'srfm_total_form_views', 'sureforms' ); + $this->assertSame( 1005, $analytics->total_form_views(), 'After the cache clears the new total should be read.' ); + + wp_cache_delete( 'srfm_total_form_views', 'sureforms' ); + wp_delete_post( $form_a, true ); + wp_delete_post( $form_b, true ); + wp_delete_post( $draft, true ); + } + + /** + * `forms_with_views` counts forms viewed at least once, not forms that exist. + * + * A form sitting at zero views must not count, otherwise the metric cannot + * distinguish "the beacon is firing" from "forms exist but are never embedded", + * which is the whole reason it sits alongside the raw total. + */ + public function test_forms_with_views() { + $analytics = Analytics::get_instance(); + + $this->assertSame( 0, $analytics->forms_with_views() ); + + $viewed = $this->create_form_with_views( 3 ); + $zero = $this->create_form_with_views( 0 ); + $draft = $this->create_form_with_views( 9, 'draft' ); + + $this->assertSame( 1, $analytics->forms_with_views(), 'Only the published form with a non-zero count should be included.' ); + + wp_delete_post( $viewed, true ); + wp_delete_post( $zero, true ); + wp_delete_post( $draft, true ); + } + + /** + * The columns toggle reported to analytics must match what the Forms list does. + * + * It defaults to ON when the key has never been saved, so reading the raw + * option with `! empty()` would report every install that predates the setting + * as opted out. The value is delegated to Form_Views for exactly that reason, + * and this asserts the absent-key case specifically. + */ + public function test_global_settings_data() { + $analytics = Analytics::get_instance(); + $original = get_option( 'srfm_general_settings_options' ); + + // Never saved → reported as enabled, matching is_tracking_enabled(). + delete_option( 'srfm_general_settings_options' ); + $data = $analytics->global_settings_data(); + $this->assertTrue( $data['boolean_values']['form_views_columns_enabled'], 'An absent setting must report as enabled.' ); + $this->assertSame( + \SRFM\Inc\Form_Views::get_instance()->is_tracking_enabled(), + $data['boolean_values']['form_views_columns_enabled'], + 'Analytics must not diverge from the Forms list.' + ); + + // Explicitly off. + update_option( 'srfm_general_settings_options', [ 'srfm_form_views_tracking' => false ] ); + $data = $analytics->global_settings_data(); + $this->assertFalse( $data['boolean_values']['form_views_columns_enabled'] ); + + // Explicitly on. + update_option( 'srfm_general_settings_options', [ 'srfm_form_views_tracking' => true ] ); + $data = $analytics->global_settings_data(); + $this->assertTrue( $data['boolean_values']['form_views_columns_enabled'] ); + + if ( false === $original ) { + delete_option( 'srfm_general_settings_options' ); + } else { + update_option( 'srfm_general_settings_options', $original ); + } + } + // ─── Helpers ───────────────────────────────────────────────── + /** + * Create a published form carrying the given view count. + * + * @param int $views View count to store. + * @param string $post_status Post status. Default 'publish'. + * @return int Post ID. + */ + private function create_form_with_views( $views, $post_status = 'publish' ) { + $form_id = wp_insert_post( + [ + 'post_title' => 'Views Analytics Form', + 'post_type' => SRFM_FORMS_POST_TYPE, + 'post_status' => $post_status, + ] + ); + + update_post_meta( $form_id, \SRFM\Inc\Form_Views::META_KEY, $views ); + + return (int) $form_id; + } + /** * Create a test post with the given content. * From 45b3f0b82e9a44d4bec33ccbc56d39cad134d77b Mon Sep 17 00:00:00 2001 From: Vansh Kapoor Date: Mon, 24 Aug 2026 09:40:20 +0530 Subject: [PATCH 11/50] feat: make view tracking opt-in, stamped once on first enable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The feature now ships off. Counting begins the first time an administrator switches it on, and from then on the toggle only shows or hides the columns. - is_tracking_enabled() defaults to false. Deny is the fallthrough: absent option, absent key and a corrupted non-array value all return false. The other six default sites (global settings, the abilities getter, the React settings store, the column gate) flip with it. - The stamp is written by maybe_start_tracking(), hooked to the general settings option write rather than to a single handler, so it fires for the settings screen, the abilities endpoint and WP-CLI alike. add_option() keeps it write-once, so an off/on cycle cannot move an open window. Both update_option_* and add_option_* are hooked, because the former does not fire when the row does not exist yet — the fresh-install case this exists for. The new value is read from the second parameter, which is where both hooks put it despite their first parameters differing. - get_tracking_started_at() is now a pure read returning 0 when never enabled. The old lazy write opened a window on any read, including from a site that never enabled the feature. - should_track() gates on the stamp, not the toggle: nothing is counted before the first opt-in, and counting continues once the columns are hidden again. - forms-data.php additionally guards on a non-zero stamp, because the toggle and the stamp are written by different paths and gmdate() on a zero timestamp would widen the window to 1970 and count every entry ever. Dropped the clamp to post_date_gmt: a form cannot have entries from before it existed, so it never changed a result. Revert-tested: defaulting back to ON, dropping the stamp gate in should_track(), re-stamping with update_option(), stamping while the toggle is off, and reading the first hook argument instead of the second each fail their own assertion. The write-once assertion is backdated deliberately — comparing against a stamp written moments earlier passed even with update_option(), since both writes land in the same second. --- .../settings/get-global-settings.php | 4 +- inc/form-views.php | 94 ++++++++++++------ inc/forms-data.php | 17 ++-- inc/global-settings/global-settings.php | 6 +- src/admin/forms/components/FormsTable.js | 4 +- src/admin/settings/Component.js | 4 +- src/admin/settings/pages/General.js | 2 +- tests/unit/admin/test-analytics.php | 12 +-- tests/unit/inc/test-form-views.php | 95 ++++++++++++++----- 9 files changed, 165 insertions(+), 73 deletions(-) diff --git a/inc/abilities/settings/get-global-settings.php b/inc/abilities/settings/get-global-settings.php index 49477fdc8..5ff6706b7 100644 --- a/inc/abilities/settings/get-global-settings.php +++ b/inc/abilities/settings/get-global-settings.php @@ -148,7 +148,7 @@ private function get_general_settings() { 'srfm_ip_log' => false, 'srfm_form_analytics' => false, 'srfm_admin_notification' => true, - 'srfm_form_views_tracking' => true, + 'srfm_form_views_tracking' => false, ]; } @@ -157,7 +157,7 @@ private function get_general_settings() { } if ( ! isset( $settings['srfm_form_views_tracking'] ) ) { - $settings['srfm_form_views_tracking'] = true; + $settings['srfm_form_views_tracking'] = false; } $settings['srfm_bsf_analytics'] = 'yes' === get_option( 'sureforms_usage_optin', false ); diff --git a/inc/form-views.php b/inc/form-views.php index 9d7b79dce..f8e58a2ff 100644 --- a/inc/form-views.php +++ b/inc/form-views.php @@ -79,42 +79,66 @@ public function __construct() { // do_blocks() pass enqueue srfm-form-submit AFTER wp_enqueue_scripts has run, and // those would otherwise never receive the beacon flag. add_action( 'wp_footer', [ $this, 'localize_beacon' ], 5 ); + + // Both hooks: update_option_* does not fire when the option row does not exist + // yet, which is exactly the state of a fresh install saving settings for the + // first time — the case this stamp exists for. + add_action( 'update_option_srfm_general_settings_options', [ $this, 'maybe_start_tracking' ], 10, 2 ); + add_action( 'add_option_srfm_general_settings_options', [ $this, 'maybe_start_tracking' ], 10, 2 ); } /** - * Unix time from which views have been counted. - * - * Written once, on first use, and never rewritten. Tracking is on by default, so - * there is no toggle event to hang this on for the majority of installs — the - * window simply opens the first time anything asks for it, which on an upgrade is - * the moment the feature becomes live. + * Unix time from which views have been counted, or 0 if counting never started. * - * Never re-stamped. Counting runs regardless of the display toggle, so the stamp - * always matches the period the stored view counts cover; moving it forward would - * measure those views against a shorter entry window. + * This is a pure read. The stamp is written by maybe_start_tracking() the first + * time an administrator switches the feature on, and never rewritten — so it is + * also the answer to "has counting ever started", which is what should_track() + * uses. A lazy write here would open the window on any read, including one from + * a site that never enabled the feature. * - * add_option() rather than update_option() so a concurrent request cannot move a - * window that is already open. + * Never re-stamped on a later toggle. Counting does not stop when the columns are + * hidden, so the stamp always matches the period the stored view counts cover; + * moving it forward would measure those views against a shorter entry window. * * @since x.x.x - * @return int Unix timestamp. + * @return int Unix timestamp, or 0 when tracking has never been enabled. */ public function get_tracking_started_at() { - $started = Helper::get_integer_value( get_option( self::TRACKING_STARTED_OPTION, 0 ) ); - - if ( $started > 0 ) { - return $started; - } + return Helper::get_integer_value( get_option( self::TRACKING_STARTED_OPTION, 0 ) ); + } - $started = time(); + /** + * Open the counting window the first time the feature is switched on. + * + * Hooked to the General-settings option write rather than to the REST handler, so + * it fires for every route that flips the setting — the settings screen, the + * abilities/MCP update endpoint, or a direct update_option() from WP-CLI. + * + * add_option() rather than update_option(): it only creates the row when absent, + * so two concurrent saves cannot move a window that is already open, and a later + * off/on cycle leaves the original stamp intact. Not autoloaded — it is read on + * the Forms list screen and by the beacon, not on every request. + * + * The new value is read from the SECOND parameter because that is where both + * hooks put it, despite their first parameters differing: + * `do_action( "update_option_{$option}", $old_value, $value, $option )` and + * `do_action( "add_option_{$option}", $option, $value )`. Taking the first + * argument would read the pre-save value on one hook and the option name on + * the other. + * + * @param mixed $unused Previous value on update, option name on add. Unused. + * @param mixed $value The general settings array being saved. + * @since x.x.x + * @return void + */ + public function maybe_start_tracking( $unused, $value ) { + unset( $unused ); - // Only creates the option when it does not already exist, so the first writer - // wins and later calls read that value back. - if ( ! add_option( self::TRACKING_STARTED_OPTION, $started, '', false ) ) { - $started = Helper::get_integer_value( get_option( self::TRACKING_STARTED_OPTION, $started ) ); + if ( ! is_array( $value ) || empty( $value[ self::SETTING_KEY ] ) ) { + return; } - return $started; + add_option( self::TRACKING_STARTED_OPTION, time(), '', false ); } /** @@ -234,8 +258,9 @@ public function get_views( $form_id ) { * regardless, so switching the columns back on reveals the period they were * hidden for rather than a gap. See should_track(). * - * Defaults to true when the setting has never been saved, so existing installs - * get the columns without an explicit opt-in. + * Off until switched on. Deny is the fallthrough: a missing option, a corrupted + * non-array value, and an absent key all return false, so the columns only ever + * appear after a deliberate opt-in. * * @since x.x.x * @return bool @@ -244,7 +269,7 @@ public function is_tracking_enabled() { $general = get_option( 'srfm_general_settings_options', [] ); if ( ! is_array( $general ) || ! isset( $general[ self::SETTING_KEY ] ) ) { - return true; + return false; } return (bool) $general[ self::SETTING_KEY ]; @@ -256,15 +281,24 @@ public function is_tracking_enabled() { * Excludes logged-in users who can edit content (author/editor/admin) and * form-builder / Instant Form live previews. * - * Deliberately does not consult the General-settings toggle: that setting governs - * whether the Views and Conversion Rate columns are shown, not whether counting - * happens. Counting continues in the background so switching the columns back on - * reveals the period they were hidden for, rather than a gap. + * Gated on the tracking-started stamp, not on the display toggle. The stamp is + * written once, when the feature is first switched on, so: + * + * - a site that has never enabled it counts nothing, which is what "off by + * default" has to mean for a counter — a hidden column that was silently + * accumulating data was never really off; + * - once opened, the window stays open. Hiding the columns again only hides + * them, so switching back on reveals the period rather than a gap, and the + * stored counts always cover exactly the period the stamp claims. * * @since x.x.x * @return bool */ private function should_track() { + if ( $this->get_tracking_started_at() <= 0 ) { + return false; + } + if ( is_user_logged_in() && current_user_can( 'edit_posts' ) ) { return false; } diff --git a/inc/forms-data.php b/inc/forms-data.php index b66d53470..ca8391aaf 100644 --- a/inc/forms-data.php +++ b/inc/forms-data.php @@ -351,7 +351,15 @@ private function prepare_form_for_listing( $post ) { // claim a real measurement of zero. $conversion_rate = null; - if ( Form_Views::get_instance()->is_tracking_enabled() ) { + $window_start = Form_Views::get_instance()->get_tracking_started_at(); + + // A zero stamp means counting never started, so there is nothing to divide by + // and no window to measure against. Guarded as well as the display toggle + // because the two are written by different paths: the toggle could be forced + // on by a direct option write that never ran maybe_start_tracking(), and + // gmdate() on a zero timestamp would silently widen the window to 1970 and + // count every entry the form has ever had. + if ( Form_Views::get_instance()->is_tracking_enabled() && $window_start > 0 ) { $views = Form_Views::get_instance()->get_views( $form_id ); // Compare like with like. The Entries column is all-time, but views only @@ -359,11 +367,8 @@ private function prepare_form_for_listing( $post ) { // same moment — otherwise a form that existed beforehand divides years of // entries by days of views and reports a rate that is pure noise. // - // A form created after the window opened measures from its own creation - // instead, so its first days are not diluted by a window it did not exist for. - $window_start = Form_Views::get_instance()->get_tracking_started_at(); - $form_created = strtotime( (string) $post->post_date_gmt ); - $window_start = $form_created && $form_created > $window_start ? $form_created : $window_start; + // No clamp to the form's own creation date: a form cannot have entries + // from before it existed, so `created_at >= window` already excludes them. $entries_since = Helper::get_integer_value( Entries::get_total_entries_by_status( 'all', diff --git a/inc/global-settings/global-settings.php b/inc/global-settings/global-settings.php index 0e39c4480..2f44172fc 100644 --- a/inc/global-settings/global-settings.php +++ b/inc/global-settings/global-settings.php @@ -152,7 +152,7 @@ public static function srfm_save_general_settings( $setting_options ) { $srfm_form_analytics = $setting_options['srfm_form_analytics'] ?? false; $srfm_bsf_analytics = $setting_options['srfm_bsf_analytics'] ?? false; $srfm_admin_notification = isset( $setting_options['srfm_admin_notification'] ) ? (bool) $setting_options['srfm_admin_notification'] : true; - $srfm_form_views_tracking = isset( $setting_options['srfm_form_views_tracking'] ) ? (bool) $setting_options['srfm_form_views_tracking'] : true; + $srfm_form_views_tracking = isset( $setting_options['srfm_form_views_tracking'] ) ? (bool) $setting_options['srfm_form_views_tracking'] : false; $settings = [ 'srfm_ip_log' => $srfm_ip_log, @@ -665,7 +665,7 @@ public static function srfm_get_general_settings( $request ) { 'srfm_ip_log' => false, 'srfm_form_analytics' => false, 'srfm_admin_notification' => true, - 'srfm_form_views_tracking' => true, + 'srfm_form_views_tracking' => false, ]; } @@ -674,7 +674,7 @@ public static function srfm_get_general_settings( $request ) { } if ( ! isset( $global_setting_options['srfm_general_settings_options']['srfm_form_views_tracking'] ) ) { - $global_setting_options['srfm_general_settings_options']['srfm_form_views_tracking'] = true; + $global_setting_options['srfm_general_settings_options']['srfm_form_views_tracking'] = false; } /** diff --git a/src/admin/forms/components/FormsTable.js b/src/admin/forms/components/FormsTable.js index b7313d469..4a5ba08fc 100644 --- a/src/admin/forms/components/FormsTable.js +++ b/src/admin/forms/components/FormsTable.js @@ -186,7 +186,9 @@ const FormsTable = ( { // (string) — a PHP boolean arrives here as '1' or '', never true/false. PHP // sends '1'/'0' explicitly; a missing flag falls back to '1' so the columns // stay visible on an install whose PHP predates this setting. - ...( '0' === String( window.srfm_admin?.form_views_tracking ?? '1' ) + // Default '0': the feature is opt-in, so a missing flag hides the columns + // rather than showing empty ones. + ...( '1' !== String( window.srfm_admin?.form_views_tracking ?? '0' ) ? [] : [ { diff --git a/src/admin/settings/Component.js b/src/admin/settings/Component.js index a8b06461f..0baa2c6a3 100644 --- a/src/admin/settings/Component.js +++ b/src/admin/settings/Component.js @@ -40,7 +40,7 @@ const Component = ( { path, subpage } ) => { srfm_form_analytics: false, srfm_bsf_analytics: false, srfm_admin_notification: true, - srfm_form_views_tracking: true, + srfm_form_views_tracking: false, } ); const [ emailTabOptions, setEmailTabOptions ] = useState( { srfm_email_summary: false, @@ -270,7 +270,7 @@ const Component = ( { path, subpage } ) => { srfm_bsf_analytics, srfm_admin_notification, // Default ON for installs saved before this toggle existed. - srfm_form_views_tracking = true, + srfm_form_views_tracking = false, } = data.srfm_general_settings_options; const snap = { srfm_ip_log, diff --git a/src/admin/settings/pages/General.js b/src/admin/settings/pages/General.js index d8e22bb83..45a19b26c 100644 --- a/src/admin/settings/pages/General.js +++ b/src/admin/settings/pages/General.js @@ -234,7 +234,7 @@ const FormViewsTrackingContent = ( { 'sureforms' ), description: __( - 'Adds the Views and Conversion Rate columns to the Forms list. A view is counted once per page visit when the form appears on screen, and the conversion rate is the share of those views that ended in a submission. Counting begins when this feature first runs on your site, so submissions received before then are not counted towards the rate. Turning this off hides the columns but keeps counting, so the figures are up to date if you switch it back on.', + 'Adds the Views and Conversion Rate columns to the Forms list. A view is counted once per page visit when the form appears on screen, and the conversion rate is the share of those views that ended in a submission. Counting starts the first time you turn this on, so submissions received before then are not counted towards the rate. Turning it off afterwards only hides the columns — counting continues, so the figures are up to date if you switch it back on.', 'sureforms' ), } } diff --git a/tests/unit/admin/test-analytics.php b/tests/unit/admin/test-analytics.php index ce59cc21f..ac3c5701b 100644 --- a/tests/unit/admin/test-analytics.php +++ b/tests/unit/admin/test-analytics.php @@ -949,19 +949,19 @@ public function test_forms_with_views() { /** * The columns toggle reported to analytics must match what the Forms list does. * - * It defaults to ON when the key has never been saved, so reading the raw - * option with `! empty()` would report every install that predates the setting - * as opted out. The value is delegated to Form_Views for exactly that reason, - * and this asserts the absent-key case specifically. + * The value is delegated to Form_Views rather than read from the option array, + * so the two can never drift — which matters precisely because the default has + * changed once already. This asserts the delegation, not a hardcoded default: + * the absent-key case is compared against `is_tracking_enabled()` itself. */ public function test_global_settings_data() { $analytics = Analytics::get_instance(); $original = get_option( 'srfm_general_settings_options' ); - // Never saved → reported as enabled, matching is_tracking_enabled(). + // Never saved → opt-in, so reported as disabled, matching is_tracking_enabled(). delete_option( 'srfm_general_settings_options' ); $data = $analytics->global_settings_data(); - $this->assertTrue( $data['boolean_values']['form_views_columns_enabled'], 'An absent setting must report as enabled.' ); + $this->assertFalse( $data['boolean_values']['form_views_columns_enabled'], 'An absent setting must report as disabled.' ); $this->assertSame( \SRFM\Inc\Form_Views::get_instance()->is_tracking_enabled(), $data['boolean_values']['form_views_columns_enabled'], diff --git a/tests/unit/inc/test-form-views.php b/tests/unit/inc/test-form-views.php index efe4ce855..209c72f98 100644 --- a/tests/unit/inc/test-form-views.php +++ b/tests/unit/inc/test-form-views.php @@ -20,14 +20,21 @@ protected function setUp(): void { $this->markTestSkipped( 'Form_Views class not available.' ); } - $this->views = Form_Views::get_instance(); - $_SERVER['REMOTE_ADDR'] = '127.0.0.1'; + $this->views = Form_Views::get_instance(); + $_SERVER['REMOTE_ADDR'] = '127.0.0.1'; unset( $_GET['live_mode'] ); wp_set_current_user( 0 ); + + // Counting is opt-in and gated on this stamp, so an open window is the + // precondition for every test about counting. The tests that assert the + // never-enabled state delete it themselves first. + update_option( Form_Views::TRACKING_STARTED_OPTION, time() - HOUR_IN_SECONDS ); } protected function tearDown(): void { wp_set_current_user( 0 ); + delete_option( Form_Views::TRACKING_STARTED_OPTION ); + delete_option( 'srfm_general_settings_options' ); parent::tearDown(); } @@ -104,23 +111,53 @@ public function test_should_track_excludes_privileged_user() { // ────────────────────────────────────────────── public function test_is_tracking_enabled() { - // Default ON when the option/key has never been saved. + // Opt-in: absent option, absent key and a corrupted non-array value all deny. delete_option( 'srfm_general_settings_options' ); - $this->assertTrue( $this->views->is_tracking_enabled(), 'Absent option should default to enabled.' ); + $this->assertFalse( $this->views->is_tracking_enabled(), 'Absent option must default to disabled.' ); + + update_option( 'srfm_general_settings_options', [ 'srfm_ip_log' => true ] ); + $this->assertFalse( $this->views->is_tracking_enabled(), 'Absent key must default to disabled.' ); + + update_option( 'srfm_general_settings_options', 'not-an-array' ); + $this->assertFalse( $this->views->is_tracking_enabled(), 'A corrupted value must deny, not fatal.' ); - // Key present but false → disabled. update_option( 'srfm_general_settings_options', [ 'srfm_form_views_tracking' => false ] ); $this->assertFalse( $this->views->is_tracking_enabled(), 'Explicit false should hide the columns.' ); - // The toggle governs display only — counting must continue while the columns are hidden, - // so switching them back on reveals the period rather than a gap. + + update_option( 'srfm_general_settings_options', [ 'srfm_form_views_tracking' => true ] ); + $this->assertTrue( $this->views->is_tracking_enabled(), 'Explicit true should show the columns.' ); + + delete_option( 'srfm_general_settings_options' ); + } + + /** + * Counting is gated on the tracking-started stamp, not on the display toggle. + * + * Before the feature has ever been switched on, nothing is counted — a hidden + * column that was quietly accumulating data was never really "off by default". + * Once the window is open, hiding the columns again only hides them, so + * switching back on reveals the period rather than a gap. + */ + public function test_should_track_follows_the_tracking_window() { wp_set_current_user( 0 ); - $this->assertTrue( $this->call_private_method( $this->views, 'should_track' ), 'should_track must ignore the display toggle and keep counting.' ); + delete_option( 'srfm_general_settings_options' ); - // Key present and true → enabled. + // Never enabled → no stamp → nothing counted. + delete_option( Form_Views::TRACKING_STARTED_OPTION ); + $this->assertFalse( $this->call_private_method( $this->views, 'should_track' ), 'No stamp means counting has not started.' ); + + // Enabling writes the stamp, and counting begins. update_option( 'srfm_general_settings_options', [ 'srfm_form_views_tracking' => true ] ); - $this->assertTrue( $this->views->is_tracking_enabled(), 'Explicit true should enable tracking.' ); + $this->assertGreaterThan( 0, $this->views->get_tracking_started_at(), 'Enabling should open the window.' ); + $this->assertTrue( $this->call_private_method( $this->views, 'should_track' ), 'Counting should run once the window is open.' ); + + // Disabling hides the columns but must NOT stop counting. + update_option( 'srfm_general_settings_options', [ 'srfm_form_views_tracking' => false ] ); + $this->assertFalse( $this->views->is_tracking_enabled(), 'The columns should be hidden.' ); + $this->assertTrue( $this->call_private_method( $this->views, 'should_track' ), 'Counting must continue while the columns are hidden.' ); delete_option( 'srfm_general_settings_options' ); + delete_option( Form_Views::TRACKING_STARTED_OPTION ); } // ────────────────────────────────────────────── @@ -128,21 +165,34 @@ public function test_is_tracking_enabled() { // ────────────────────────────────────────────── public function test_get_tracking_started_at() { + delete_option( 'srfm_general_settings_options' ); delete_option( Form_Views::TRACKING_STARTED_OPTION ); - $before = time(); - $started = $this->views->get_tracking_started_at(); + // A pure read: asking must never open the window, or a site that never + // enabled the feature would start a window just by rendering the list. + $this->assertSame( 0, $this->views->get_tracking_started_at(), 'Never enabled must read as 0.' ); + $this->assertFalse( get_option( Form_Views::TRACKING_STARTED_OPTION ), 'Reading must not write the stamp.' ); + + $before = time(); + update_option( 'srfm_general_settings_options', [ 'srfm_form_views_tracking' => true ] ); $after = time(); + $started = $this->views->get_tracking_started_at(); - $this->assertGreaterThanOrEqual( $before, $started, 'First call should stamp the current time.' ); - $this->assertLessThanOrEqual( $after, $started, 'First call should stamp the current time.' ); + $this->assertGreaterThanOrEqual( $before, $started, 'Enabling should stamp the current time.' ); + $this->assertLessThanOrEqual( $after, $started, 'Enabling should stamp the current time.' ); - // Write-once: the stamp must not drift on later calls, or the conversion-rate - // window would keep sliding forward and hide entries it already counted. - $this->assertSame( $started, $this->views->get_tracking_started_at(), 'Second call must return the stored stamp.' ); - $this->assertSame( $started, (int) get_option( Form_Views::TRACKING_STARTED_OPTION ), 'Stamp must be persisted.' ); + // Write-once. An off/on cycle must not move the window, or the stored view + // counts would be measured against an entry window shorter than they cover. + // + // Backdated deliberately: comparing against the stamp written a moment ago + // would pass even with update_option(), because both writes land in the same + // second. A known-old value is what actually distinguishes the two. + update_option( Form_Views::TRACKING_STARTED_OPTION, 1000000000 ); + update_option( 'srfm_general_settings_options', [ 'srfm_form_views_tracking' => false ] ); + update_option( 'srfm_general_settings_options', [ 'srfm_form_views_tracking' => true ] ); + $this->assertSame( 1000000000, $this->views->get_tracking_started_at(), 'Re-enabling must not re-stamp an open window.' ); - // Not autoloaded — this is read only on the Forms list screen. + // Not autoloaded — this is read on the Forms list screen and by the beacon. global $wpdb; $autoload = $wpdb->get_var( $wpdb->prepare( @@ -153,11 +203,12 @@ public function test_get_tracking_started_at() { // WP 6.6+ stores 'off' for an explicit false; older cores store 'no'. $this->assertContains( $autoload, [ 'no', 'off' ], 'Stamp must not be autoloaded on every request.' ); - // An existing stamp is returned verbatim, never re-stamped. + // Saving with the toggle off must never open a window. delete_option( Form_Views::TRACKING_STARTED_OPTION ); - add_option( Form_Views::TRACKING_STARTED_OPTION, 1000000000, '', false ); - $this->assertSame( 1000000000, $this->views->get_tracking_started_at(), 'Existing stamp must be preserved.' ); + update_option( 'srfm_general_settings_options', [ 'srfm_form_views_tracking' => false ] ); + $this->assertSame( 0, $this->views->get_tracking_started_at(), 'Saving while off must not open the window.' ); + delete_option( 'srfm_general_settings_options' ); delete_option( Form_Views::TRACKING_STARTED_OPTION ); } From dbce2173627a8bc1c078c54536b8f3cbc97ce1eb Mon Sep 17 00:00:00 2001 From: Vansh Kapoor Date: Mon, 24 Aug 2026 09:42:54 +0530 Subject: [PATCH 12/50] test: cover maybe_start_tracking() directly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the check-test-coverage gap on the new stamp writer. The other tests reach it through update_option(), which only exercises one of the two hook shapes; this calls it directly with both. Pins the contract that makes both hooks work — the new value is the second parameter in each, despite their first parameters differing — plus write-once and the deny-by-default cases (toggle off, key absent, empty array, non-array, null). Revert-tested: reading the first argument, swapping add_option for update_option, and stamping regardless of the toggle each fail their own assertion. --- tests/unit/inc/test-form-views.php | 43 ++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/unit/inc/test-form-views.php b/tests/unit/inc/test-form-views.php index 209c72f98..b6e137ab8 100644 --- a/tests/unit/inc/test-form-views.php +++ b/tests/unit/inc/test-form-views.php @@ -212,6 +212,49 @@ public function test_get_tracking_started_at() { delete_option( Form_Views::TRACKING_STARTED_OPTION ); } + /** + * The stamp writer, called directly with each hook's argument shape. + * + * The other tests drive this through update_option(), which only ever exercises + * the `update_option_*` shape. This pins the contract that makes both hooks + * work: the new value is the SECOND parameter in both, even though their first + * parameters differ — + * `do_action( "update_option_{$option}", $old_value, $value, $option )` versus + * `do_action( "add_option_{$option}", $option, $value )`. An implementation that + * read the first argument would stamp off the previous value on one hook and off + * the option name on the other, and the add_option case is the fresh install + * this whole mechanism exists for. + */ + public function test_maybe_start_tracking() { + $enabled = [ 'srfm_form_views_tracking' => true ]; + $disabled = [ 'srfm_form_views_tracking' => false ]; + + // update_option_* shape: ( $old_value, $value ). + delete_option( Form_Views::TRACKING_STARTED_OPTION ); + $this->views->maybe_start_tracking( $disabled, $enabled ); + $this->assertGreaterThan( 0, $this->views->get_tracking_started_at(), 'The update_option_* shape should stamp.' ); + + // add_option_* shape: ( $option_name, $value ) — a string first argument. + delete_option( Form_Views::TRACKING_STARTED_OPTION ); + $this->views->maybe_start_tracking( 'srfm_general_settings_options', $enabled ); + $this->assertGreaterThan( 0, $this->views->get_tracking_started_at(), 'The add_option_* shape should stamp.' ); + + // Write-once: an already-open window is never moved. + update_option( Form_Views::TRACKING_STARTED_OPTION, 1000000000 ); + $this->views->maybe_start_tracking( $disabled, $enabled ); + $this->assertSame( 1000000000, $this->views->get_tracking_started_at(), 'An open window must not be moved.' ); + + // Everything else denies without an explicit branch: toggle off, key absent, + // empty array, and a corrupted non-array value. + foreach ( [ $disabled, [ 'srfm_ip_log' => true ], [], 'not-an-array', null ] as $value ) { + delete_option( Form_Views::TRACKING_STARTED_OPTION ); + $this->views->maybe_start_tracking( null, $value ); + $this->assertSame( 0, $this->views->get_tracking_started_at(), 'Only an enabled toggle may open the window.' ); + } + + delete_option( Form_Views::TRACKING_STARTED_OPTION ); + } + // ────────────────────────────────────────────── // permissions_check (HMAC token) // ────────────────────────────────────────────── From d517a055c4f90e3198d9bda973b922d2b68469fd Mon Sep 17 00:00:00 2001 From: Vansh Kapoor Date: Mon, 24 Aug 2026 10:45:30 +0530 Subject: [PATCH 13/50] fix: address the review findings on the views and conversion feature MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sort/display mismatch (the worst of them): the Conversion Rate column sorted on all-time entries while it displayed entries from the tracking window, so a form rendering a dash sorted as several hundred percent and landed at the top of a descending sort. Both paths now go through one calculate_form_metrics(), with unmeasurable rates sorting last instead of tying with a real 0%. Beacon hardening: - The rate limiter was a read-modify-write over a transient, so concurrent requests all read the same value and the limit only ever bound sequential traffic — on the only control between an anonymous caller and an unbounded write loop. Now an atomic wp_cache_add/incr where a persistent cache exists. - Buckets key on the network (/24, /64), not the exact address, and a per-form site-wide ceiling is checked first. Without both, an attacker with an IPv6 /64 got a fresh allowance per address and could mint unbounded wp_options rows, since the first hit on a new bucket is always free. - The beacon signs its own 'srfm_view' namespace instead of reusing the submission token, so a token scraped from public markup cannot be replayed against the submit endpoint and this route is not an oracle for whether a submit token is still in an accepted window. NAMESPACE_SUBMIT is the historical literal, so tokens in cached HTML keep verifying. - Views no longer accrue for draft or trashed forms during the token window. increment_views(): wp_postmeta has no unique index, so add_post_meta()'s $unique is a SELECT then an INSERT and two concurrent first-views could create two rows that both increment forever — the comment claiming otherwise was simply wrong. Re-runs the UPDATE when the insert loses. Also CASTs meta_value so a non-numeric value does not error under STRICT_TRANS_TABLES and stop that form counting permanently, and distinguishes a failed query from a missing row. Correctness and UX: - Duplicating a form no longer clones its view count; the copy started life with impressions it never had and a permanent 0% rate. - is_tracking_enabled() self-heals the stamp, so a settings import or restore cannot leave the columns visible while nothing ever counts. - The beacon flag is localized as '1'/'0', matching the convention admin.php sets. A raw bool worked only by accident of (string) false === ''. - The column gate reads a flag returned with the rows instead of a page-load snapshot, so toggling in another tab cannot render columns over empty data. A stale ?orderby=views resets rather than silently date-sorting. - The metric sort is bounded and announces the skip; the listing primes the post cache and skips a redundant entry COUNT; the window boundary is resolved against MySQL's clock once per request rather than per row. - Dropped the analytics cache, which no caller could ever read twice. Swept six comments that still described the pre-opt-in default, including two that shipped to wordpress.org stating the opposite of the code. Tests: calculate_form_metrics, prepare_form_for_listing, window_boundary_sql, network_bucket, hit_counter, remaining_window, the token namespace split, and the duplicate-form skip. The duplicate-form test initially passed for the wrong reason — the helper returns a response array, not an ID — and only the revert-test exposed it. --- admin/analytics.php | 26 +-- assets/js/unminified/form-submit.js | 16 +- inc/duplicate-form.php | 8 + inc/form-views.php | 189 ++++++++++++++++--- inc/forms-data.php | 231 ++++++++++++++++++----- inc/generate-form-markup.php | 7 +- inc/submit-token.php | 50 +++-- src/admin/forms/FormsListingPage.js | 23 ++- src/admin/forms/components/FormsTable.js | 53 +++--- src/admin/settings/Component.js | 2 +- tests/unit/admin/test-analytics.php | 16 +- tests/unit/inc/test-duplicate-form.php | 69 +++++++ tests/unit/inc/test-form-views.php | 96 +++++++++- tests/unit/inc/test-forms-data.php | 119 ++++++++++++ tests/unit/inc/test-submit-token.php | 31 +++ 15 files changed, 785 insertions(+), 151 deletions(-) diff --git a/admin/analytics.php b/admin/analytics.php index 2aaac3c10..3a62d4bd1 100644 --- a/admin/analytics.php +++ b/admin/analytics.php @@ -491,9 +491,10 @@ public function global_settings_data() { // Payment analytics - check if any payment method is enabled. $global_data['boolean_values']['stripe_enabled'] = $this->is_stripe_enabled(); - // Delegated rather than read from $general_settings directly: the setting - // defaults to ON when the key has never been saved, so `! empty()` on the - // raw array would report every pre-upgrade install as opted out. + // Delegated rather than read from $general_settings directly, so the telemetry + // can never disagree with what the Forms list actually shows. That matters + // because the default has already changed once: reimplementing the check here + // means a future change has two places to be made and one to be forgotten. $global_data['boolean_values']['form_views_columns_enabled'] = Form_Views::get_instance()->is_tracking_enabled(); return $global_data; @@ -515,15 +516,12 @@ public function global_settings_data() { public function total_form_views() { global $wpdb; - $cache_key = 'srfm_total_form_views'; - $cached_result = wp_cache_get( $cache_key, 'sureforms' ); - - if ( false !== $cached_result ) { - return Helper::get_integer_value( $cached_result ); - } - + // Deliberately uncached: the only caller is the analytics payload builder, + // which runs once per cron pass. A cache would never see a second read inside + // a request, and any TTL would always have expired between passes — while + // still going stale if a second caller ever appeared. // PHPCS: Ignore direct database query warning, as there is no built-in alternative. - // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery + // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching $total = $wpdb->get_var( $wpdb->prepare( " @@ -539,11 +537,7 @@ public function total_form_views() { ) ); - $total = Helper::get_integer_value( $total ); - - wp_cache_set( $cache_key, $total, 'sureforms', HOUR_IN_SECONDS ); - - return $total; + return Helper::get_integer_value( $total ); } /** diff --git a/assets/js/unminified/form-submit.js b/assets/js/unminified/form-submit.js index ec31637ce..3ef08a124 100644 --- a/assets/js/unminified/form-submit.js +++ b/assets/js/unminified/form-submit.js @@ -232,7 +232,7 @@ function initializeFormHandlers() { */ function trackFormViews( forms ) { // Respect the server-side exclusion flag (admins/editors, previews). - if ( ! window.srfm_view_beacon || ! window.srfm_view_beacon.enabled ) { + if ( '1' !== String( window.srfm_view_beacon?.enabled ?? '0' ) ) { return; } @@ -247,9 +247,13 @@ function trackFormViews( forms ) { } const formId = form.getAttribute( 'form-id' ); - const submitToken = form.getAttribute( 'data-submit-token' ); + const viewToken = form.getAttribute( 'data-view-token' ); - if ( ! formId || ! submitToken ) { + // Reject a malformed form-id up front. Marking the element tracked and + // disconnecting the observer before discovering the id is unusable would + // silently lose the view to a guaranteed 400. + const numericFormId = parseInt( formId, 10 ); + if ( ! Number.isInteger( numericFormId ) || numericFormId < 1 || ! viewToken ) { continue; } @@ -272,12 +276,12 @@ function trackFormViews( forms ) { form.setAttribute( 'data-srfm-view-tracked', '1' ); obs.disconnect(); - wp.apiFetch( { + window.wp.apiFetch( { path: 'sureforms/v1/forms/track-view', method: 'POST', - data: { form_id: parseInt( formId, 10 ) }, + data: { form_id: numericFormId }, headers: { - 'X-WP-Submit-Token': submitToken, + 'X-WP-Submit-Token': viewToken, }, } ).catch( () => { // Beacon is best-effort; never disrupt the page on failure. diff --git a/inc/duplicate-form.php b/inc/duplicate-form.php index f2349c2bb..ef183914a 100644 --- a/inc/duplicate-form.php +++ b/inc/duplicate-form.php @@ -123,6 +123,14 @@ public function duplicate_form( $form_id, $title_suffix = ' (Copy)' ) { continue; } + // The view counter belongs to the form that earned it. Cloning it + // would give the copy an impression count with no submissions behind + // it — a permanent 0% conversion rate — and double-count those views + // in the site-wide analytics total. + if ( \SRFM\Inc\Form_Views::META_KEY === $meta_key ) { + continue; + } + // Handle unserialized metas (these are already arrays/objects). // Note: get_post_meta() returns unslashed data, but add_post_meta() // internally runs wp_unslash() on the value. Without re-slashing, diff --git a/inc/form-views.php b/inc/form-views.php index f8e58a2ff..e0c770c96 100644 --- a/inc/form-views.php +++ b/inc/form-views.php @@ -41,9 +41,9 @@ class Form_Views { public const META_KEY = '_srfm_form_views'; /** - * General-settings key that toggles view + conversion-rate tracking. Lives in - * the `srfm_general_settings_options` option; defaults ON when the key is absent - * (installs that predate this setting). + * General-settings key that shows or hides the Views and Conversion Rate + * columns. Lives in the `srfm_general_settings_options` option and is absent + * until switched on, which reads as off — the feature is opt-in. */ public const SETTING_KEY = 'srfm_form_views_tracking'; @@ -63,10 +63,26 @@ class Form_Views { public const TRACKING_STARTED_OPTION = 'srfm_form_views_tracking_started_at'; /** - * Max beacon hits accepted per visitor IP + form within the rate-limit window. + * Max beacon hits accepted per visitor network + form within the rate-limit window. */ private const RATE_LIMIT_MAX = 20; + /** + * Max beacon hits accepted for one form from all sources within the window. + * + * The per-network bucket alone cannot bound anything: an attacker on an IPv6 /64 + * — standard on any VPS — gets a fresh bucket per address, and the first request + * to each new bucket is always allowed. This ceiling is what actually caps a + * distributed flood, both for the counter's accuracy and for the number of + * buckets that can be minted. + */ + private const RATE_LIMIT_FORM_MAX = 200; + + /** + * Cache group for the beacon's rate-limit counters. + */ + private const RATE_LIMIT_GROUP = 'srfm_form_views'; + /** * Constructor. * @@ -180,7 +196,7 @@ public function permissions_check( $request ) { $token = Helper::get_string_value( $request->get_header( 'X-WP-Submit-Token' ) ); $form_id = absint( Helper::get_integer_value( $request->get_param( 'form_id' ) ) ); - if ( ! Submit_Token::verify( $token, $form_id ) ) { + if ( ! Submit_Token::verify( $token, $form_id, Submit_Token::NAMESPACE_VIEW ) ) { return new WP_Error( 'srfm_view_token_invalid', __( 'Security verification failed.', 'sureforms' ), @@ -204,7 +220,12 @@ public function permissions_check( $request ) { public function track_view( $request ) { $form_id = absint( Helper::get_integer_value( $request->get_param( 'form_id' ) ) ); - if ( ! $form_id || SRFM_FORMS_POST_TYPE !== get_post_type( $form_id ) ) { + // Publicly viewable, not merely the right post type: get_post_type() answers + // the same for a draft or trashed form, and a token minted while the form was + // live stays valid for up to 48 hours afterwards. Without this, views keep + // accruing against content nobody can reach — and the analytics denominator + // counts published forms only, so those views skew the site-wide figures. + if ( ! $form_id || SRFM_FORMS_POST_TYPE !== get_post_type( $form_id ) || ! is_post_publicly_viewable( $form_id ) ) { return new WP_REST_Response( [ 'counted' => false ], 200 ); } @@ -236,7 +257,12 @@ public function localize_beacon() { wp_localize_script( 'srfm-form-submit', 'srfm_view_beacon', - [ 'enabled' => $this->should_track() ] + // '1'/'0' strings, not a raw bool: WP_Scripts::localize() casts every + // scalar with (string), so false arrives as '' and true as '1'. That + // happens to work with a truthiness check today, but anyone later + // 'tidying' this to send '0' would invert the gate, because '0' is a + // truthy JS string. Same contract as admin.php's form_views_tracking. + [ 'enabled' => $this->should_track() ? '1' : '0' ] ); } @@ -254,10 +280,11 @@ public function get_views( $form_id ) { /** * Whether the Views and Conversion Rate columns are shown on the Forms list. * - * Despite the setting key's name this governs display only — counting runs - * regardless, so switching the columns back on reveals the period they were - * hidden for rather than a gap. See should_track(). - * + * Despite the setting key's name this governs display, not counting. Counting is + * gated on the tracking-started stamp instead: nothing is counted until the + * feature is first switched on, and once the window is open, hiding the columns + * again only hides them — so switching back on reveals the period rather than a + * gap. See should_track(). * Off until switched on. Deny is the fallthrough: a missing option, a corrupted * non-array value, and an absent key all return false, so the columns only ever * appear after a deliberate opt-in. @@ -272,7 +299,20 @@ public function is_tracking_enabled() { return false; } - return (bool) $general[ self::SETTING_KEY ]; + $enabled = (bool) $general[ self::SETTING_KEY ]; + + // Self-heal the window. maybe_start_tracking() covers every route that goes + // through update_option(), but the setting can also arrive by a path that + // fires no hook — a settings import, a partial database restore, a direct + // $wpdb write. Left alone, that state shows the columns while nothing ever + // counts, and re-saving the identical array would not repair it because + // update_option() short-circuits on an unchanged value. Writing only when the + // toggle is already on keeps a read from a never-enabled site harmless. + if ( $enabled && $this->get_tracking_started_at() <= 0 ) { + add_option( self::TRACKING_STARTED_OPTION, time(), '', false ); + } + + return $enabled; } /** @@ -320,19 +360,38 @@ private function should_track() { private function increment_views( $form_id ) { global $wpdb; - // Atomic increment so concurrent beacons don't clobber each other. + // CAST rather than a bare + 1: under STRICT_TRANS_TABLES a non-numeric + // meta_value (left by an import or a mistaken update_post_meta) makes the + // arithmetic an error, the query returns false, and this form would stop + // counting permanently. CAST yields 0 for garbage and the count recovers. // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching $updated = $wpdb->query( $wpdb->prepare( - "UPDATE {$wpdb->postmeta} SET meta_value = meta_value + 1 WHERE post_id = %d AND meta_key = %s", + "UPDATE {$wpdb->postmeta} SET meta_value = CAST( meta_value AS UNSIGNED ) + 1 WHERE post_id = %d AND meta_key = %s", $form_id, self::META_KEY ) ); - if ( empty( $updated ) ) { - // Row didn't exist yet — create it (unique so a racing insert can't duplicate). - add_post_meta( $form_id, self::META_KEY, 1, true ); + // Distinguish 0 (no such row) from false (query failed): treating a failure as + // a missing row sends it down the insert path, where it fails again silently. + if ( 0 === $updated ) { + // First view for this form. add_post_meta()'s $unique flag is a SELECT + // followed by an INSERT and wp_postmeta carries no unique index, so two + // concurrent first-views can both insert — after which the UPDATE above + // would increment both rows forever while get_post_meta() reads only one. + // Re-run the UPDATE afterwards: if a racing request already created the + // row, that call increments it and this one adds nothing. + if ( ! add_post_meta( $form_id, self::META_KEY, 1, true ) ) { + // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching + $wpdb->query( + $wpdb->prepare( + "UPDATE {$wpdb->postmeta} SET meta_value = CAST( meta_value AS UNSIGNED ) + 1 WHERE post_id = %d AND meta_key = %s", + $form_id, + self::META_KEY + ) + ); + } } // Keep the post-meta cache consistent after the direct write. @@ -357,21 +416,97 @@ private function is_rate_limited( $form_id ) { return true; // Fail closed if IP cannot be determined. } - $transient_key = 'srfm_view_' . md5( $ip . '_' . $form_id ); - $attempts = get_transient( $transient_key ); + // Bucket on the network, not the exact address. A single attacker routinely + // controls every address in an IPv6 /64, and a per-address bucket would hand + // them a fresh allowance — plus a fresh wp_options row — for each one. + $bucket = self::network_bucket( $ip ); - if ( false === $attempts ) { - set_transient( $transient_key, 1, MINUTE_IN_SECONDS ); - return false; + // Per-form ceiling first, and it is the one that actually bounds a + // distributed flood: the per-network counter can always be escaped by + // rotating networks, and the first hit on a new bucket is free. + if ( self::hit_counter( 'form_' . $form_id ) > self::RATE_LIMIT_FORM_MAX ) { + return true; } - $attempts_count = Helper::get_integer_value( $attempts ); + return self::hit_counter( $bucket . '_' . $form_id ) > self::RATE_LIMIT_MAX; + } - if ( $attempts_count >= self::RATE_LIMIT_MAX ) { - return true; + /** + * Collapse an IP to the network an attacker would have to rotate out of. + * + * /24 for IPv4 and /64 for IPv6 — the smallest blocks normally allocated to a + * single subscriber, so this bounds one actor without pooling unrelated visitors + * any harder than a shared NAT already does. + * + * @param string $ip Validated IP address. + * @since x.x.x + * @return string Opaque bucket key. + */ + private static function network_bucket( $ip ) { + if ( filter_var( $ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6 ) ) { + $packed = inet_pton( $ip ); + // First 8 bytes = the /64 prefix. + $prefix = false === $packed ? $ip : substr( $packed, 0, 8 ); + } else { + $parts = explode( '.', $ip ); + $prefix = count( $parts ) === 4 ? $parts[0] . '.' . $parts[1] . '.' . $parts[2] : $ip; + } + + return md5( (string) $prefix ); + } + + /** + * Increment a rate-limit counter and return its new value. + * + * Atomic where it matters. The previous implementation read a transient, + * compared, then wrote it back — so N concurrent requests all read the same + * value and all wrote value+1, and the limit only ever constrained sequential + * traffic. That mattered here because this counter is the only thing standing + * between an anonymous caller and an unbounded write loop. + * + * With a persistent object cache, wp_cache_add() + wp_cache_incr() is atomic. + * Without one, wp_cache_* is request-local, so the value is carried in a + * transient instead; that path is still not atomic under concurrency, but it + * keeps the per-form ceiling meaningful across sequential requests and avoids + * pretending to a guarantee the storage cannot make. + * + * @param string $key Counter key, unique per bucket and form. + * @since x.x.x + * @return int The counter value after this hit. + */ + private static function hit_counter( $key ) { + if ( wp_using_ext_object_cache() ) { + // add() only succeeds for the first caller, so exactly one request seeds + // the window and every other one increments atomically. + wp_cache_add( $key, 0, self::RATE_LIMIT_GROUP, MINUTE_IN_SECONDS ); + return Helper::get_integer_value( wp_cache_incr( $key, 1, self::RATE_LIMIT_GROUP ) ); + } + + $transient_key = 'srfm_view_' . md5( $key ); + $count = Helper::get_integer_value( get_transient( $transient_key ) ) + 1; + + // Only extend the window when opening it, so a steady stream just under the + // limit cannot hold a bucket open indefinitely by refreshing its own TTL. + if ( 1 === $count ) { + set_transient( $transient_key, $count, MINUTE_IN_SECONDS ); + } else { + set_transient( $transient_key, $count, self::remaining_window( $transient_key ) ); } - set_transient( $transient_key, $attempts_count + 1, MINUTE_IN_SECONDS ); - return false; + return $count; + } + + /** + * Seconds left on an open rate-limit window, floored at one second. + * + * @param string $transient_key Transient holding the counter. + * @since x.x.x + * @return int + */ + private static function remaining_window( $transient_key ) { + $timeout = Helper::get_integer_value( get_option( '_transient_timeout_' . $transient_key, 0 ) ); + $remaining = $timeout - time(); + + return $remaining > 0 ? $remaining : 1; } } diff --git a/inc/forms-data.php b/inc/forms-data.php index ca8391aaf..78d308666 100644 --- a/inc/forms-data.php +++ b/inc/forms-data.php @@ -202,15 +202,26 @@ public function get_forms_list( $request ) { // Execute query — use try/finally to guarantee filter cleanup. try { - // Only take the derived-metric path while tracking is on. With the feature - // off the columns are hidden, so a stored or hand-crafted request to sort by - // them would run the expensive compute-sort-paginate pass to order rows - // nobody can see; fall through to the default ordering instead. - if ( in_array( $orderby, [ 'views', 'conversion_rate' ], true ) && Form_Views::get_instance()->is_tracking_enabled() ) { - // Derived metrics can't be sorted by WP_Query; compute across all - // matching forms, sort, then paginate. + // Derived metrics can't be sorted by WP_Query, so they take a + // compute-sort-paginate pass instead. Only while tracking is on: with the + // feature off the columns are hidden, and a stored or hand-crafted request + // would run that expensive pass to order rows nobody can see. It also + // returns null when the site has more forms than the pass will scan. + $response_data = null; + $is_metric_sort = in_array( $orderby, [ 'views', 'conversion_rate' ], true ); + + if ( $is_metric_sort && Form_Views::get_instance()->is_tracking_enabled() ) { $response_data = $this->get_forms_sorted_by_metric( $args, $orderby, $order, $page, Helper::get_integer_value( $per_page ) ); - } else { + } + + if ( null === $response_data ) { + if ( $is_metric_sort ) { + // WP_Query would silently discard 'views'/'conversion_rate' and fall + // back to post_date anyway. Say so explicitly so the behaviour is in + // the code rather than in core's tolerance for unknown keys. + $args['orderby'] = 'date'; + } + $query = new \WP_Query( $args ); $forms = []; @@ -236,6 +247,12 @@ public function get_forms_list( $request ) { } } + // Travels with the rows so the table gates its columns on the same evaluation + // that produced them. The localized `srfm_admin` flag is only a page-load + // snapshot: toggle the setting in another tab and the open list would keep + // rendering columns while every row came back empty, which reads as data loss. + $response_data['views_enabled'] = Form_Views::get_instance()->is_tracking_enabled(); + return new WP_REST_Response( $response_data, 200 ); } @@ -253,7 +270,9 @@ public function get_forms_list( $request ) { * @param int $page Current page (1-based). * @param int $per_page Items per page. * @since x.x.x - * @return array Response payload matching get_forms_list(). + * @return array|null Response payload, or null when the site has more + * forms than this pass will scan and the caller + * should fall back to ordinary ordering. */ private function get_forms_sorted_by_metric( $args, $orderby, $order, $page, $per_page ) { $id_args = $args; @@ -265,6 +284,37 @@ private function get_forms_sorted_by_metric( $args, $orderby, $order, $page, $pe $id_query = new \WP_Query( $id_args ); + /** + * Largest number of forms this path will sort before giving up. + * + * The pass is one entry COUNT per form, so cost grows linearly with the form + * count while only one page is ever displayed. Past this ceiling the request + * returns null so the caller falls back to ordinary date ordering rather than + * firing thousands of queries to render ten rows. + * + * @param int $limit Maximum forms to sort in PHP. Default 500. + * @since x.x.x + */ + $limit = Helper::get_integer_value( apply_filters( 'srfm_forms_metric_sort_limit', 500 ) ); + + if ( $limit > 0 && count( $id_query->posts ) > $limit ) { + /** + * Fires when the metric sort is skipped because the site has too many forms. + * + * Announced rather than skipped silently: a list that quietly ignores the + * column the user clicked reads as a broken sort, not as a deliberate + * ceiling. Gives a site owner something to hook if they hit it. + * + * @param string $orderby Requested metric, 'views' or 'conversion_rate'. + * @param int $count Number of forms that would have been sorted. + * @param int $limit The ceiling in force. + * @since x.x.x + */ + do_action( 'srfm_forms_metric_sort_skipped', $orderby, count( $id_query->posts ), $limit ); + + return null; + } + // `fields => ids` skips the post-meta cache priming that a normal WP_Query does, // so prime it once for all matched forms — otherwise each get_views() below is a // separate get_post_meta() query (N+1). Entry counts are still one COUNT per form; @@ -277,14 +327,24 @@ static function ( $post ) { $id_query->posts ); update_meta_cache( 'post', $prime_ids ); + + // `fields => ids` skips the post cache, so the get_post() in the pagination + // loop below would be one query per displayed row. Prime it here instead. + // Terms and meta are handled separately, hence both flags false. + _prime_post_caches( $prime_ids, false, false ); } $rows = []; foreach ( $id_query->posts as $post_id ) { $form_id = Helper::get_integer_value( $post_id ); - $views = Form_Views::get_instance()->get_views( $form_id ); - $entries = Helper::get_integer_value( Entries::get_total_entries_by_status( 'all', $form_id ) ); - $metric = 'views' === $orderby ? (float) $views : ( $views > 0 ? $entries / $views * 100 : 0.0 ); + $metrics = $this->calculate_form_metrics( $form_id ); + + // Same helper the column renders from, so the order always matches the + // numbers on screen. An unmeasurable rate sorts as -1 rather than 0, so + // the dash rows group below a genuine 0% instead of tying with it. + $metric = 'views' === $orderby + ? (float) $metrics['views'] + : ( null === $metrics['conversion_rate'] ? -1.0 : (float) $metrics['conversion_rate'] ); $rows[] = [ 'id' => $form_id, @@ -327,29 +387,65 @@ static function ( $a, $b ) use ( $direction ) { } /** - * Prepare a single form for the listing response. + * The tracking-window boundary as a datetime string comparable to `created_at`. * - * @param \WP_Post $post Post object. - * @return array Prepared form data for listing. - * @since 2.0.0 + * `created_at` is written by MySQL (`DEFAULT CURRENT_TIMESTAMP`) and compared in + * the session time zone, not UTC, so a bare `gmdate()` of a PHP timestamp is off + * by the MySQL/PHP clock offset — permanently mis-counting entries near the + * boundary. Entries::get_entries_count_after() solves this by reading + * `SELECT NOW()`, but it does so on every call, which would be one extra query + * per row on a listing page. The offset cannot change within a request, so it is + * resolved once and reused. + * + * @param int $window_start Unix timestamp. + * @return string Datetime string in MySQL's frame of reference. + * @since x.x.x */ - private function prepare_form_for_listing( $post ) { - $form_id = $post->ID; + private static function window_boundary_sql( $window_start ) { + static $offset_seconds = null; - // Get entries count. - $entries_count = Helper::get_integer_value( Entries::get_total_entries_by_status( 'all', $form_id ) ); + if ( null === $offset_seconds ) { + global $wpdb; + + // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching -- Reading the server clock; a cached value would defeat the purpose. + $mysql_now = $wpdb->get_var( 'SELECT NOW()' ); + $mysql_ts = $mysql_now ? strtotime( (string) $mysql_now ) : false; - // Views (impressions) and derived conversion rate. Clamp to 100%: entries are - // all-time while views only accrue from when this feature shipped, so early on a - // form can have more entries than counted views (which would read >100%). - // - // Skipped entirely when tracking is off — the columns are hidden then, so this - // would be a meta read per form on every listing request for values nothing - // renders. - $views = 0; - // null means "not computable yet", which the table renders as a dash. 0.0 would - // claim a real measurement of zero. - $conversion_rate = null; + // Fall back to no adjustment rather than a wild offset if NOW() is + // unreadable or unparseable — a UTC-configured server needs none anyway. + $offset_seconds = false === $mysql_ts ? 0 : $mysql_ts - time(); + } + + return gmdate( 'Y-m-d H:i:s', $window_start + $offset_seconds ); + } + + /** + * Views and conversion rate for one form. + * + * The single source of truth for both the rendered value and the sorted metric. + * They were computed separately at first, and drifted: the sort used all-time + * entries while the column used entries from the tracking window, so a form + * rendering a dash sorted as though its rate were several hundred percent. + * Anything needing these numbers must come through here. + * + * Returns `null` for the rate rather than a number whenever it cannot be + * measured — tracking off, window never opened, no views yet, or more entries + * than views. That last case is not possible in reality (every entry needs a + * view first), so it means the view count is incomplete and any percentage + * would be invented; the table renders the dash instead. `0.0` is reserved for + * a real measurement of zero. + * + * @param int $form_id Form post ID. + * @param string $post_date_gmt Form creation date, GMT. Used to skip a redundant count. + * @param int $entries_all_time All-time entry count, when the caller already has it. + * @return array{views:int,conversion_rate:float|null} + * @since x.x.x + */ + private function calculate_form_metrics( $form_id, $post_date_gmt = '', $entries_all_time = null ) { + $none = [ + 'views' => 0, + 'conversion_rate' => null, + ]; $window_start = Form_Views::get_instance()->get_tracking_started_at(); @@ -359,16 +455,28 @@ private function prepare_form_for_listing( $post ) { // on by a direct option write that never ran maybe_start_tracking(), and // gmdate() on a zero timestamp would silently widen the window to 1970 and // count every entry the form has ever had. - if ( Form_Views::get_instance()->is_tracking_enabled() && $window_start > 0 ) { - $views = Form_Views::get_instance()->get_views( $form_id ); - - // Compare like with like. The Entries column is all-time, but views only - // start accruing when tracking opens, so the rate counts entries from that - // same moment — otherwise a form that existed beforehand divides years of - // entries by days of views and reports a rate that is pure noise. - // - // No clamp to the form's own creation date: a form cannot have entries - // from before it existed, so `created_at >= window` already excludes them. + if ( ! Form_Views::get_instance()->is_tracking_enabled() || $window_start <= 0 ) { + return $none; + } + + $views = Form_Views::get_instance()->get_views( $form_id ); + + if ( $views <= 0 ) { + return $none; + } + + // Compare like with like. The Entries column is all-time, but views only start + // accruing when tracking opens, so the rate counts entries from that same + // moment — otherwise a form that existed beforehand divides years of entries by + // days of views and reports a rate that is pure noise. + $form_created = strtotime( (string) $post_date_gmt ); + + if ( null !== $entries_all_time && $form_created && $form_created >= $window_start ) { + // The form is younger than the window, so every entry it has is already + // inside the window and the caller's all-time count is the same number. + // Skips a second COUNT per row on the listing. + $entries_since = Helper::get_integer_value( $entries_all_time ); + } else { $entries_since = Helper::get_integer_value( Entries::get_total_entries_by_status( 'all', @@ -378,23 +486,46 @@ private function prepare_form_for_listing( $post ) { [ 'key' => 'created_at', 'compare' => '>=', - 'value' => gmdate( 'Y-m-d H:i:s', $window_start ), + 'value' => self::window_boundary_sql( $window_start ), ], ], ] ) ); + } - // More entries than views is impossible — every entry needs a view first — - // so the count is incomplete and any percentage would be invented. null tells - // the table to show its "no data yet" dash rather than a clamped 100%. - if ( $views > 0 && $entries_since <= $views ) { - $conversion_rate = round( $entries_since / $views * 100, 1 ); - } elseif ( $views > 0 ) { - $conversion_rate = null; - } + if ( $entries_since > $views ) { + return [ + 'views' => $views, + 'conversion_rate' => null, + ]; } + return [ + 'views' => $views, + 'conversion_rate' => round( $entries_since / $views * 100, 1 ), + ]; + } + + /** + * Prepare a single form for the listing response. + * + * @param \WP_Post $post Post object. + * @return array Prepared form data for listing. + * @since 2.0.0 + */ + private function prepare_form_for_listing( $post ) { + $form_id = $post->ID; + + // Get entries count. + $entries_count = Helper::get_integer_value( Entries::get_total_entries_by_status( 'all', $form_id ) ); + + // Views and conversion rate come from the same helper the sort path uses, so the + // column can never order by a different number than it displays. + $metrics = $this->calculate_form_metrics( $form_id, $post->post_date_gmt, $entries_count ); + $views = $metrics['views']; + $conversion_rate = $metrics['conversion_rate']; + return [ 'id' => $form_id, 'title' => $post->post_title, diff --git a/inc/generate-form-markup.php b/inc/generate-form-markup.php index f0f55cc5a..eac8f43de 100644 --- a/inc/generate-form-markup.php +++ b/inc/generate-form-markup.php @@ -855,10 +855,15 @@ public static function get_form_markup( $id, $show_title_current_page = true, $s return ob_get_clean(); } $submit_token = Submit_Token::generate( (int) $id ); + // Separately namespaced from the submission token: this one is only good + // for incrementing a view counter, so scraping it from the page buys an + // attacker nothing beyond what the beacon already does, and it cannot be + // replayed against the submit endpoint. + $view_token = Submit_Token::generate( (int) $id, Submit_Token::NAMESPACE_VIEW ); ?> { resetFilters: resetUrlFilters, } = useFormsFilters(); - const { sortBy, sortOrder, handleSort, getSortDirection } = useFormsSort(); + const { sortBy, sortOrder, handleSort, getSortDirection, resetSort } = + useFormsSort(); const { currentPage, perPage, setCurrentPage, setPerPage } = useFormsPagination(); @@ -92,6 +93,23 @@ const FormsListingPage = () => { // Extract data from API response const forms = formsData?.forms || []; + // Server-evaluated per request, so the columns and the rows always agree. + const viewColumnsEnabled = Boolean( formsData?.views_enabled ); + + // A bookmarked ?orderby=views outlives the feature being switched off. The + // server falls back to date order in that case, but the UI would still claim to + // be sorted by a column that is no longer rendered — and no header would show a + // direction, so there is nothing to click to get back. Reset it once we know. + useEffect( () => { + if ( + ! isLoading && + formsData && + ! viewColumnsEnabled && + [ 'views', 'conversion_rate' ].includes( sortBy ) + ) { + resetSort(); + } + }, [ isLoading, formsData, viewColumnsEnabled, sortBy, resetSort ] ); const paginationData = { total: formsData?.total || 0, totalPages: Math.max( 1, formsData?.total_pages || 0 ), @@ -610,6 +628,9 @@ const FormsListingPage = () => { ) : (