Skip to content

Subscribers: Use new helper library#101857

Merged
allilevine merged 9 commits intotrunkfrom
update/subscribers-to-use-helper-library
Apr 2, 2025
Merged

Subscribers: Use new helper library#101857
allilevine merged 9 commits intotrunkfrom
update/subscribers-to-use-helper-library

Conversation

@allilevine
Copy link
Member

@allilevine allilevine commented Mar 25, 2025

Related to https://github.com/Automattic/loop/issues/567

Proposed Changes

  • Behind the feature flag subscribers-helper-library, add a param to use the new subscribers library: use_new_helper.
  • Also behind the flag, update the subscription ids and dates to match the library responses.
  • Add use_new_helper to the cache key.

Why are these changes being made?

  • To use the more performant helper library.

Testing Instructions

  • Go to /subscribers/{site url}
  • Smoke test:
    • The subscribers load without errors (on the sandbox or in the console).
    • The correct data displays in the table.
    • You can sort by all sortable columns.
    • You can filter by all filters.
    • You can combine filters.
    • Deleted subscribers are filtered out.
    • You can click on a subscriber and see the correct URL and details.
    • Pagination works correctly.
    • Import new subscriber(s) and check there's no lag in updating the list. You'll need to turn on write access and sandbox the job (import_subscribers).
    • Remove new subscriber(s) and check there's no lag in updating the list. You'll need to turn on write access.
    • Export subscribers.
  • Regression test that subscribers still loads correctly without the library (you can turn off the flag in the dev environment or use calypso live). And that you can delete users and load subscriber details.
  • Regression test Jetpack Cloud without the library.

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
    • For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@allilevine allilevine self-assigned this Mar 25, 2025
@github-actions
Copy link

github-actions bot commented Mar 25, 2025

@matticbot
Copy link
Contributor

matticbot commented Mar 25, 2025

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~284 bytes added 📈 [gzipped])

Details
name         parsed_size           gzip_size
subscribers       +440 B  (+0.0%)     +284 B  (+0.1%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~85 bytes added 📈 [gzipped])

Details
name                                                 parsed_size           gzip_size
async-load-calypso-my-sites-stats-pages-subscribers       +159 B  (+0.1%)      +85 B  (+0.1%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@matticbot
Copy link
Contributor

matticbot commented Mar 25, 2025

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • notifications
  • odyssey-stats
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/subscribers-to-use-helper-library on your sandbox.

@allilevine allilevine force-pushed the update/subscribers-to-use-helper-library branch from 3824c1c to c20e8de Compare March 27, 2025 16:58
@allilevine allilevine requested a review from Copilot March 27, 2025 19:17
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 27, 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 pull request adds support for a new subscribers helper library behind a feature flag, updating type definitions, query parameters, cache key construction, and components that display subscriber data.

  • Updated type definitions in index.ts to accommodate responses from both the new helper library and the old format.
  • Updated queries and cache key generation to include the new helper flag.
  • Modified subscriber data view components to conditionally use new helper library fields for subscription IDs and dates.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
client/my-sites/subscribers/types/index.ts Updated Subscriber type definitions to include new helper fields.
client/my-sites/subscribers/queries/use-subscribers-query.tsx Added the new helper flag to query parameters and cache key.
client/my-sites/subscribers/helpers/index.ts Integrated the new helper flag into the cache key generation.
client/my-sites/subscribers/components/subscriber-data-views/subscriber-data-views.tsx Updated subscription ID and date accessors to handle both formats.
Comments suppressed due to low confidence (2)

client/my-sites/subscribers/components/subscriber-data-views/subscriber-data-views.tsx:60

  • [nitpick] The helper flag is declared as 'useNewHelper' here using camelCase, but elsewhere it is referred to as 'use_new_helper' using snake_case. Consider unifying the naming convention for clarity and consistency.
const useNewHelper = config.isEnabled( 'subscribers-helper-library' );

client/my-sites/subscribers/components/subscriber-data-views/subscriber-data-views.tsx:64

  • Using the logical OR operator may inadvertently default to 0 even if a valid subscription ID of 0 exists. Consider using the nullish coalescing operator (??) to properly check for null or undefined values.
return subscriber.wpcom_subscription_id || subscriber.email_subscription_id || 0;

@allilevine allilevine marked this pull request as ready for review March 27, 2025 19:18
@allilevine allilevine requested a review from Copilot March 28, 2025 19:31
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 enables the use of a new subscribers helper library behind the "subscribers-helper-library" feature flag to improve performance. Key changes include:

  • Adding new optional fields (email_subscription_id, wpcom_subscription_id, and corresponding dates) to support the new helper library.
  • Updating queries, mutations, and cache key generation to include the new flag.
  • Modifying subscriber details and data views components to use the new helper library fields when available.

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
client/my-sites/subscribers/types/index.ts Added optional fields for new helper library support in subscription types.
client/my-sites/subscribers/queries/use-subscribers-query.tsx Updated query parameters and cache keys to include the new flag.
client/my-sites/subscribers/mutations/use-subscriber-remove-mutation.ts Adapted removal logic to use helper functions based on the flag.
client/my-sites/subscribers/helpers/index.ts Modified cache key generation to include the use_new_helper flag.
client/my-sites/subscribers/components/subscriber-details/subscriber-details.tsx Introduced a fallback check when converting the subscription date.
client/my-sites/subscribers/components/subscriber-data-views/subscriber-data-views.tsx Implemented helper methods for consistent subscription ID and date handling across components.
Files not reviewed (1)
  • config/development.json: Language not supported
Comments suppressed due to low confidence (1)

client/my-sites/subscribers/queries/use-subscribers-query.tsx:19

  • [nitpick] For consistency across the codebase, consider using camelCase (e.g., useNewHelper) for the flag instead of snake_case.
use_new_helper?: boolean;

import wpcom from 'calypso/lib/wp';
import { getSubscriberDetailsCacheKey, getSubscriberDetailsType } from '../helpers';
import type { Subscriber } from '../types';
import type { SubscriberDetails } from '../types';
Copy link
Member Author

Choose a reason for hiding this comment

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

Type fix: the Subscriber type no longer always has subscriber_id because we're using different ids with the library. So I created a separate type for SubscriberDetails.

@allilevine allilevine force-pushed the update/subscribers-to-use-helper-library branch from bab41e0 to ed36013 Compare April 1, 2025 14:36
Copy link
Contributor

@spsiddarthan spsiddarthan left a comment

Choose a reason for hiding this comment

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

LGTM and tests well

@allilevine allilevine force-pushed the update/subscribers-to-use-helper-library branch from 936f50c to 4811975 Compare April 2, 2025 13:28
@allilevine allilevine merged commit a312627 into trunk Apr 2, 2025
13 checks passed
@allilevine allilevine deleted the update/subscribers-to-use-helper-library branch April 2, 2025 14:00
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 2, 2025
JessBoctor pushed a commit to JessBoctor/wp-calypso that referenced this pull request Apr 15, 2025
…01857)

* Use new helper library.

* Update id and date handling.

* Add feature flag.

* Fix type errors.

* Turn on feature flag in dev env and fix id issues.

* Remove no cache.

* Fix the date_subscribed type error in a better way.

* Assert that we have the date in the query.

* Turn subscribers-helper-library flag on in Jetpack Cloud dev environment, as well.
JessBoctor pushed a commit to JessBoctor/wp-calypso that referenced this pull request Apr 17, 2025
…01857)

* Use new helper library.

* Update id and date handling.

* Add feature flag.

* Fix type errors.

* Turn on feature flag in dev env and fix id issues.

* Remove no cache.

* Fix the date_subscribed type error in a better way.

* Assert that we have the date in the query.

* Turn subscribers-helper-library flag on in Jetpack Cloud dev environment, as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments