Instrument connection, sync, settings, media, cache, features, and deactivation analytics events - #1226
Draft
gabrielcld2 wants to merge 17 commits into
Draft
Instrument connection, sync, settings, media, cache, features, and deactivation analytics events#1226gabrielcld2 wants to merge 17 commits into
gabrielcld2 wants to merge 17 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
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.