Skip to content

Conversation

@smithjw1
Copy link
Contributor

@smithjw1 smithjw1 commented Nov 6, 2025

Description

We want to restrict the inclusion of our telemetry JavaScript agent to customers using specific integrations that use front-end tracking.

This change adds that restriction to the existing logic.

Changelog Description

Changed

  • Added additional checks before the inclusion of an admin-facing telemetry library.

Pre-review checklist

Please make sure the items below have been covered before requesting a review:

  • [N/A] This change works and has been tested locally or in Codespaces (or has an appropriate fallback).
  • [N/A] This change works and has been tested on a sandbox.
  • [N/A] This change has relevant unit tests (if applicable).
  • [N/A] This change uses a rollout method to ease with deployment (if applicable - especially for large scale actions that require writes).
  • [N/A] This change has relevant documentation additions / updates (if applicable).
  • I've created a changelog description that aligns with the provided examples.

Pre-deploy checklist

  • VIP staff: Ensure any alerts added/updated conform to internal standards (see internal documentation).

Steps to Test

We disable telemetry tracking on all non-production instances and sandboxes, so testing locally is complicated.

  1. Check out PR.
  2. Go to wp-admin
  3. Verify nothing is broken

@smithjw1 smithjw1 requested a review from chriszarate November 6, 2025 17:39
@smithjw1 smithjw1 requested a review from a team as a code owner November 6, 2025 17:39
Copilot AI review requested due to automatic review settings November 6, 2025 17:39
@smithjw1 smithjw1 self-assigned this Nov 6, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds integration-based gating to the Pendo JavaScript library loading mechanism. The library will now only load when at least one of the specified integrations (Parse.ly, Remote Data Blocks, or Real-time Collaboration) is enabled on the site.

Key Changes

  • Introduces a new $required_integrations property listing integrations that must be present
  • Adds defensive check for wpvip_get_enabled_integrations function availability
  • Implements logic to verify at least one required integration is enabled before loading Pendo

*
* @param array $required_integrations Array of integration slugs that must be enabled.
*/
$required_integrations = apply_filters( 'vip_pendo_required_integrations', self::$required_integrations );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they are required, how come we are allowing it to be filtered out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal isnt' to allow them to be filtered out (though you could do that). The goal is to allow the next plugin we want to track to be able to opt-in on their own, without an MU plugins change by using this filter.

I'm thinking this might not be the best approach, since for an integration to exist, it means an MU plugins change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not sure if there is any way around MU plugins change, but I'm not sure how I feel about having a hardcoded list as an approach. I wonder if it's worth adding a method at the integration level?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or metadata? We would only want it for our integrations.

I don't love a hardcoded list, but I was basing this off our other hardcoded list of screens to track in WordPress.

I'm happy to explore another approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think metadata seems like overkill.. Maybe a property at the abstract integration level itself to opt in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've a new PR, more changes overall, but the long-term flexibility is nice.

My only question is about third-party integrations. We don't really want them enabling Pendo tracking; it isn't going to start sending data to anyone but us, but we do want to only opt in our integrations. My thought is that it's a remote possibility, and probably not something to worry about, but I'm interested in what others think.

@sonarqubecloud
Copy link

@codecov
Copy link

codecov bot commented Dec 19, 2025

Codecov Report

❌ Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 34.91%. Comparing base (be0eaa2) to head (16a37eb).
⚠️ Report is 19 commits behind head on develop.

Files with missing lines Patch % Lines
telemetry/pendo/class-pendo-javascript-library.php 90.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #6607      +/-   ##
=============================================
+ Coverage      34.84%   34.91%   +0.07%     
- Complexity      5029     5039      +10     
=============================================
  Files            295      295              
  Lines          20746    20761      +15     
=============================================
+ Hits            7228     7249      +21     
+ Misses         13518    13512       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants