Instrument connection, sync, settings, media, cache, features, and deactivation analytics events - #1226
Instrument connection, sync, settings, media, cache, features, and deactivation analytics events#1226gabrielcld2 wants to merge 19 commits into
Conversation
Develop to uat
Release process GH Action
…activation analytics events Wires the remaining 7 event categories from the analytics tracking spec on top of the existing WPP-1210 custom-events framework: connection management, asset sync, settings & navigation, media & asset actions, non-media cache, extensions & gallery, and deactivation. All call sites reuse Analytics::track() / Analytics.track() and were live-verified against wp-env via WP-CLI/REST dispatch. Adds a permanent e2e analytics-capture mu-plugin and two Playwright specs covering connection and deactivation events. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous commit only included src/js/* source changes. This repo ships compiled js/* output directly, so the extension_toggled, special_offer_clicked, deactivation_modal_viewed, and deactivation_skipped tracking calls weren't actually live until this rebuild. Live-verified via the full Playwright e2e suite against wp-env with real Cloudinary credentials (13/13 passing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nts-tracking # Conflicts: # php/class-admin.php # php/class-deactivation.php
develop bumped phpstan to level 5 after this branch was cut. The Analytics component (added in WPP-1210) was missing from get_component()'s @return union, so every ->track() call site read as "method.notFound" once merged. Re-verified clean at level 5 and against the full Playwright e2e suite.
…e load Image_Preview::preview() set the sample <img>'s src to a bare '#', which resolves to the current document's own URL — every load of Image Settings, Video Settings, or the Responsive/Breakpoints preview (Video_Preview and Breakpoints_Preview both extend Image_Preview) fired a second real HTTP request back at the same admin page before global-transformations.js replaced it with a real preview URL. Found while investigating why settings_page_viewed fired twice per page load: confirmed via wp-env access logs that the second request carried an image/* Accept header and a self-referencing Referer, matching this exact placeholder-image bug. Fixed with an inert 1x1 data-URI placeholder instead. Re-verified: single event per load, single request in the access log, full Playwright suite still green (13/13).
…nts-tracking # Conflicts: # js/cloudinary.js # js/syntax-highlight.js # src/js/main.js
The merge commit resolved js/cloudinary.js and js/syntax-highlight.js conflicts by taking develop's compiled output, dropping the SpecialOffer wiring and the deactivate.js Prettier reformat. Rebuilding restores both on top of develop's dependency upgrades and ESLint flat-config migration. Re-verified: phpcs/phpstan clean, full Playwright suite green (13/13).
Covers bulk_sync_started (via a real authenticated REST call, matching what the "Start bulk sync" button does), sync_completed + asset_sync_failed (via direct invocation of Sync_Queue's run-tracking state machine — a full real-upload sync run is slow/flaky in CI, and this exercises the exact same code path already verified manually), and sync_settings_changed (real UI: toggling the auto-sync radio on the Connect page). Needed test isolation: a real bulk_sync_started call kicks off a genuine background sync thread via a non-blocking loopback request, which would otherwise keep running and race later tests.
Covers asset_edited + transformation_applied (scope: asset) via a real authenticated REST call to save_asset, and transformation_applied (scope: global) via a real settings save on the Image Settings page. transformation_count reliably comes back 0 for the asset-scope case since the synthetic (fileless) test attachment has no real resource type to derive it from — asserted the event shape instead of the count value.
Covers cache_items_viewed, cache_items_toggled, asset_cache_purged, all_cache_purged, and cache_uploaded — all via real authenticated REST calls / direct action dispatch against the live Rest_Assets endpoints. Found along the way: rest_purge_all() resolves its `parent` param via Assets::get_param(), which is only ever populated by the in-memory activate_parent() call inside Assets::activate_parents() for settings- configured paths — a different (and non-persisted-across-requests) lookup than get_asset_parent() (DB-backed, used by show_cache/etc). A custom test-created cache point can't satisfy that lookup without also registering a real settings path, so the purge test targets one of the plugin's own default non-media paths instead, which is already active for exactly this reason.
Covers extension_toggled (real UI: a native checkbox click, bypassing Playwright's visibility check for the collapsed Extensions sidebar panel) and gallery_configured (via Admin::save_settings() directly with a synthetic gallery_config payload, matching what the React gallery panel serializes — avoids driving the full React UI for one settings save). Two timing/state gotchas fixed along the way: extensions.js debounces its change handler by 1000ms before firing, and gallery_config payloads need a value guaranteed to differ from whatever's already saved, since save_settings() silently no-ops on an unchanged value.
… plugin_uninstalled Rounds out connection management and deactivation analytics coverage. Also fixes fakeCloudinaryConnected() to upsert cloudinary_connect instead of a raw UPDATE, which silently no-ops once a real plugin_uninstalled run deletes the option row.
…fix asset_cache_purged flake The e2e capture mu-plugin only logged outgoing analytics-api.cloudinary.com requests and let them proceed, so every local/CI test run was leaking synthetic events and deactivation-reason submissions into the real production collector. It now preempts with a synthetic 200 instead. Also fixes cache-analytics.spec.js's asset_cache_purged test: none of the plugin's default non-media cache paths are active on a fresh install (they default off), so the test now explicitly enables the uploads path via a real settings save before purging, rather than assuming it's already active.
…-visit side effect Enabling the cache path was already explicit via a real settings save, but materializing the underlying asset-parent post depended on a subsequent admin page load's Assets::update_asset_paths() side effect — a timing-sensitive path that flaked once under CI load (passed on retry). Create the post directly instead, removing that dependency entirely.
utkarshcloudinary
left a comment
utkarshcloudinary
left a comment
There was a problem hiding this comment.
Looks good! @gabrielcld2
I reviewed with help of AI [which I reviewed] - left some comments.
| if ( $analytics ) { | ||
| $code = $status->get_error_code(); | ||
| $analytics->track( | ||
| 'connectivity_check_failed', |
There was a problem hiding this comment.
connectivity_check_failed (ping) is not cron-only — it fires on every failed interactive connection test.
check_status() is also called from test_connection() (php/class-connect.php:526), which runs on every rest_test_connection (wizard live-typing check) and on verify_connection() saves. A user typing a bad URL in the wizard emits both connection_test_result (error) and this event with check_type: 'ping'. The PR body/QA notes describe this event as cron-only.
Either gate the track on the cron context (e.g. wp_doing_cron() or only when invoked via the cloudinary_status hook), or accept and document the overlap — as-is it will inflate connectivity-failure metrics with interactive typos.
| protected function shutdown_queue( $type = 'queue' ) { | ||
| if ( 'queue' === $type ) { | ||
| delete_option( self::$queue_enabled ); | ||
| $this->track_run_completed(); |
There was a problem hiding this comment.
Stale run state corrupts the next sync_completed.
This covers the queue-drained path, but the manual-stop path — rest_start_sync() with the queue disabled goes straight to stop_queue() (line ~718), not shutdown_queue() — never clears RUN_STARTED_KEY/RUN_TALLY_KEY and never emits.
The next run's mark_run_started() then no-ops (option already exists), so its sync_completed reports the old start time and accumulated tallies. Suggest clearing run state in stop_queue( 'queue' ) alongside delete_option( self::$queue_key ), or explicitly in the manual-stop branch.
| $state['success'] = $this->queue->start_queue( $type ); | ||
|
|
||
| if ( $state['success'] ) { | ||
| $this->queue->mark_run_started( $type ); |
There was a problem hiding this comment.
Ordering: mark_run_started() runs after start_queue() has already spawned threads.
start_queue() immediately kicks off background threads whose log_sync_result() → tally_run_result() calls no-op until RUN_STARTED_KEY exists — early results are silently dropped from the tally.
Set the run state before starting the queue (and clean it up on the ! $started failure path so a failed start doesn't leave a phantom run behind).
| * | ||
| * @return void | ||
| */ | ||
| public function tally_run_result( $success ) { |
There was a problem hiding this comment.
Tally read-modify-write races across sync threads.
The queue runs multiple concurrent threads (separate requests); get_option → increment → update_option will lose updates under concurrency, undercounting total_synced/total_errors.
Either use an atomic $wpdb increment, or document the counts as approximate — acceptable as a known limitation if noted.
| * | ||
| * @return mixed | ||
| */ | ||
| public function track_setting_changed( $new_value, $current_value, $setting ) { |
There was a problem hiding this comment.
sync_settings_changed fires on no-op saves.
This hooks cloudinary_settings_save_setting, which fires from Settings::set_pending() — and set_pending() is called unconditionally by Connect::rest_save_wizard() (php/class-connect.php:210-211 sets auto_sync every time) and by every Setting::save_value(). Unlike Admin::save_settings(), those callers don't diff first, and this tracker never compares $new_value to $current_value.
Every wizard completion will emit sync_settings_changed for auto_sync even when nothing changed. Add a $new_value !== $current_value guard (noting $current_value may be null when the caller doesn't pass it).
|
|
||
| $analytics = get_plugin_instance()->get_component( 'analytics' ); | ||
| if ( $analytics ) { | ||
| $analytics->track( 'cache_items_viewed', 'cache', null, array( 'cache_point' => (string) $url ) ); |
There was a problem hiding this comment.
cache_items_viewed overcounts. This fires on every rest_get_caches() call, including each pagination step and each search keystroke-triggered request. If the spec intends "user viewed a cache point", consider tracking only page 1 / no-search requests.
|
|
||
| $analytics = $this->plugin->get_component( 'analytics' ); | ||
| if ( $analytics ) { | ||
| $analytics->track( 'notice_dismissed', 'settings', null, array( 'notice_id' => (string) $token ) ); |
There was a problem hiding this comment.
Minor: $token is unsanitized user input going into the event (and into set_transient() — pre-existing). Cheap win: sanitize_key( $token ) for notice_id.
| } | ||
| // Give the fire-and-forget analytics request a moment to | ||
| // leave the browser before the page navigates away. | ||
| setTimeout( function () { |
There was a problem hiding this comment.
Minor: the 150 ms setTimeout is a race with navigation — slow admin-ajax/REST setup can still lose the event. navigator.sendBeacon() or fetch( …, { keepalive: true } ) is the reliable pattern for exit events. Acceptable as fail-silent best-effort, but flaky by design.
| // The deactivation script tracks analytics events, which requires the | ||
| // main `cloudinary` handle (and its `cldData` inline script) to be | ||
| // enqueued — it otherwise has no reason to load on the Plugins screen. | ||
| $dependencies[] = 'cloudinary'; |
There was a problem hiding this comment.
Note: this pulls the full cloudinary bundle (plus jquery/wp-util) onto plugins.php for every site with the plugin installed, just to get cldData + the analytics bridge. Works, but a slim analytics-only handle would be lighter on a core screen. Fine to ship as-is if the size is acceptable.
| * | ||
| * @var array | ||
| */ | ||
| const GLOBAL_TRANSFORMATION_SLUGS = array( |
There was a problem hiding this comment.
Nit: class constant declared mid-class between methods — move it up with the other constants at the top of the class.
3 participants
Approach
Wires the remaining 7 event categories from the Analytics Event Tracking Spec POC's custom-events framework:
connection management, asset sync, settings & navigation, media & asset actions, non-media cache, extensions & gallery, and deactivation (30 events total).
No changes to the transport layer (
Analytics::track()/Analytics.track()) — this is call-site wiring only.Design decisions worth a second look
bulk_sync_startedonly covers the manual REST trigger (Push_Sync::rest_start_sync()). Theauto_restartcase insideSync_Queue::stop_maybe()is treated as internal plumbing and isn't instrumented.asset_sync_failedhooks the broad, already-genericSync::log_sync_result()rather than the narrowerUpload_Sync::upload_asset(), trading a smallasset_typelookup cost for covering every sync type in one place.sync_completedneeded new state (Sync_Queue::mark_run_started()/tally_run_result()/track_run_completed()) since noexisting signal survives the queue's internal restart cycles.
account_switchedis new comparison logic inConnect::verify_connection()—Connect::switch_account()turned out to be dead code (no callers since 2021), not something to hook into.php/assets/class-rest-assets.php, notphp/class-cache.php— the latter is never instantiated (dead code), confirmed via exploration before wiring anything.gallery_configured'slayout/media_countare parsed out of thegallery_configfield's serialized JSON blob rather than being real submitted fields.Unrelated fix included: duplicate page-view bug
While QA'ing
settings_page_viewed, we found it firing twice per single page load on Image Settings / Video Settings. Root cause:Image_Preview::preview()set the sample<img>'ssrc="#", which resolves to the current page's own URL — so the browser fired a second real request back at the same admin page (as an image fetch) beforeglobal-transformations.jsreplaced it with a real preview URL.Video_PreviewandBreakpoints_Previewboth extendImage_Previewand inherited the same bug.This is a genuine, pre-existing perf bug unrelated to the analytics work, but it directly corrupted the accuracy of the new
settings_page_viewedevent on exactly the pages it affects, so it's fixed here rather than filed separately:src="#"→ an inert 1x1 data-URI placeholder. Confirmed via raw Apache access logs (down to one request) and the capture log (down to one event) that the duplicate is gone; full Playwright suite still green afterward.Unrelated fix included:
attributes:classnever applied on thelinkUI componentspecial_offer_clickedneeded the special-offer<a>to render with a stable class, set viaattributes:classon thelinkcomponent's config. It never actually applied:Params_Trait::get_param()splits its path on., so a colon-delimited key never matched the nestedattributesarray. Fixed inphp/ui/component/class-link.phpto readattributesdirectly. Confirmed no other caller relied on the broken fallback.QA notes
Automation
28 of the 30 events in this PR are covered by automated e2e specs — run
npm run test:e2e(one spec file per category:tests/e2e/{connection,sync,settings,media,cache,features,deactivation}-analytics.spec.js) and confirm it's green. That's the QA gate for everything except the two events below, which have no automatable trigger:connectivity_check_failed— cron-only (daily ping / REST-API self-check), no UI path exists. To check manually: connect with a valid Cloudinary URL, then break it (wp option update cloudinary_connect '{"cloudinary_url":"cloudinary://invalid"}' --format=json), runwp cron event run cloudinary_status --due-now, and confirm the event in the capture log (.wp-env/mu-plugins/analytics-capture.php,wp cloudinary analytics-events).account_switched— needs two real Cloudinary cloud names. Connect with account A's credentials via the wizard or Connect settings page, then submit account B's credentials on the same page; confirm the event carriesprevious_cloud_name/new_cloud_name.Regression check: confirm no PHP notices/fatals in
wp-content/debug.logand no new JS console errors while the suite runs — none of this should be visible to a site admin who isn't looking for it.Manual Steps
Setup (one-time per session)
npm run env:startNote: The
.wp-env/mu-plugins/analytics-capture.phpmu-plugin auto-loads and intercepts every outgoing call toanalytics-api.cloudinary.com, logging the payload instead of sending it (and blocking the real send).Per-event loop
event_name,event_category, and the event-specific params.Example: Image Settings save
settings_saved—event_category: "settings",page: "image",changed_keys: ["image_format"](or whatever you changed).transformation_applied—scope: "global"— this fires additionally when the changed key is one of the transformation-related fields (format/quality/etc.), per the diff check inAdmin::save_settings().Same pattern works for any event: swap step 4 for the relevant UI action (or a direct REST call via the browser console using
window.cldData.analytics.nonce/.endpointif the UI path is awkward — see any of the*-analytics.spec.jsfiles for exact REST routes perevent).