Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Domain-To-Plan Credit: Add plan notice for domain credits #98792

Merged
merged 12 commits into from
Feb 5, 2025

Conversation

oswian
Copy link
Contributor

@oswian oswian commented Jan 23, 2025

Related to https://github.com/Automattic/martech/issues/3658

Proposed Changes

Requires: 170944-ghe-Automattic/wpcom

Displays a credit notice on the plans page for users who have unused credit after purchasing a domain and no plan.

Implementation notes:

  • I've renamed client/my-sites/plans-features-main/components/plan-notice-credit-update.tsx to plan-notice-credit-upgrade.tsx ('update' --> 'upgrade') as this appears to be an oversight, as the component itself is named PlanNoticeCreditUpgrade.
  • Although the structure of this notice is very similar to plan-notice-credit-upgrade.tsx, we are using a different hook internally to determine the applicability of the credits (useDomainToPlanCreditsApplicable), and we are presenting a different user message. So while we could update plan-notice-credit-upgrade.tsx to handle both notices and avoid the duplication, I've instead opted to keep it simple and create a new component.

Why are these changes being made?

To encourage folks who purchased a domain and only have a free plan to upgrade to a paid plan. Further context here.

Testing Instructions

  1. Create a domain-only site
  2. Navigate to /plans/:site?flags=domain-to-plan-credit
  3. Verify that the domain-to-plan credit notice is shown
  4. Verify that the domain-to-plan credit notice is not shown for:
    • Site on any paid plan
    • Site on the free plan that has not purchased a domain

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)?

@oswian oswian self-assigned this Jan 23, 2025
@matticbot
Copy link
Contributor

matticbot commented Jan 23, 2025

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

Sections (~742 bytes added 📈 [gzipped])

name                     parsed_size           gzip_size
update-design-flow           +2356 B  (+0.1%)     +641 B  (+0.1%)
link-in-bio-tld-flow         +2356 B  (+0.1%)     +642 B  (+0.1%)
jetpack-app                  +2356 B  (+0.5%)     +666 B  (+0.4%)
plugins                      +2141 B  (+0.1%)     +605 B  (+0.1%)
plans                        +2141 B  (+0.1%)     +605 B  (+0.1%)
import-hosted-site-flow       +215 B  (+0.0%)      +29 B  (+0.0%)

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 (~677 bytes added 📈 [gzipped])

name                                             parsed_size           gzip_size
async-load-signup-steps-plans-theme-preselected      +2356 B  (+0.5%)     +641 B  (+0.4%)
async-load-signup-steps-plans                        +2356 B  (+0.5%)     +641 B  (+0.4%)

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 Jan 24, 2025

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

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

  • notifications
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug add/plan-notice-for-domain-to-plan-credit on your sandbox.

@oswian oswian force-pushed the add/plan-notice-for-domain-to-plan-credit branch 2 times, most recently from b40c883 to 3679872 Compare January 30, 2025 02:31
@oswian oswian marked this pull request as ready for review January 31, 2025 04:41
@oswian oswian requested a review from jeyip January 31, 2025 04:41
@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 Jan 31, 2025
@jeyip
Copy link
Contributor

jeyip commented Feb 3, 2025

Testing

Requirements

Flows:

  • Verify that the domain-to-plan credit notice is shown
  • Verify that the domain-to-plan credit notice is not shown for:
    • Site on any paid plan
    • Site on the free plan that has not purchased a domain

Browsers

  • Chrome

@jeyip
Copy link
Contributor

jeyip commented Feb 3, 2025

I've renamed client/my-sites/plans-features-main/components/plan-notice-credit-update.tsx to plan-notice-credit-upgrade.tsx ('update' --> 'upgrade') as this appears to be an oversight, as the component itself is named PlanNoticeCreditUpgrade.

Since we're introducing a file named plan-notice-domain-to-plan-credit.tsx, what do you think about changing plan-notice-credit-upgrade.tsx a bit more to plan-notice-plan-to-higher-plan-credit.tsx? It's verbose, but I think it makes the shared relationship between the two much clearer.

expect( result.current ).toEqual( 1000 );
} );

test( 'Returns credit when credit value is 0', () => {
Copy link
Contributor

@jeyip jeyip Feb 3, 2025

Choose a reason for hiding this comment

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

Would you mind elaborating on the significance of this spec a bit more? It sounds like its important to differentiate between a $0 credit and no credit in this case 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! The test verifies that the hook maintains a distinction between two different states: a site that is eligible but has no credit (hook returns 0) versus a site that is not eligible (hook returns null). While the distinction isn't utilised in the current UI, I think supporting the distinction in the hook makes it a bit more flexible, and the test helps avoid an accidental change that might conflate the two states.

I hope that makes sense?

I'll update the test description and comments to clarify.

Copy link
Contributor

@jeyip jeyip Feb 5, 2025

Choose a reason for hiding this comment

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

Yep makes sense thanks for clarifying :)

Alongside your inline comments, what do you think about adding a note clarifying that the distinction isn't currently utilized in the UI ( but may be in the future ? )

Copy link
Contributor

@jeyip jeyip left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this! Left a few comments but they're all non-blocking 🙂

Before you merge into production, would you mind hiding things behind a feature flag?

@jeyip jeyip removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 3, 2025
@oswian
Copy link
Contributor Author

oswian commented Feb 4, 2025

Since we're introducing a file named plan-notice-domain-to-plan-credit.tsx, what do you think about changing plan-notice-credit-upgrade.tsx a bit more to plan-notice-plan-to-higher-plan-credit.tsx? It's verbose, but I think it makes the shared relationship between the two much clearer.

Good idea. I've applied the rename you suggested.

@oswian oswian force-pushed the add/plan-notice-for-domain-to-plan-credit branch from aa356b3 to 39338b9 Compare February 4, 2025 10:36
@oswian
Copy link
Contributor Author

oswian commented Feb 4, 2025

Before you merge into production, would you mind hiding things behind a feature flag?

👍 Feature flag check has been added.

@oswian oswian force-pushed the add/plan-notice-for-domain-to-plan-credit branch 2 times, most recently from 6f51468 to 9141a3a Compare February 4, 2025 12:07
- Update `useDomainToPlanCreditsApplicable` to use `useMaxPlanUpgradeCredits`
- Remove deprecated `useDomainToPlanCredits` hook
- Modify test mocks to reflect new hook implementation
- Add support for visible plans parameter in credits hook
- Remove test for null credit value
- Update mock to use `useMaxPlanUpgradeCredits` instead of `useDomainToPlanCredits`
- Adjust test expectations to match new hook implementation
- Add specific support URL for upgrade credits
- Reorder domain-to-plan credit notice type in type definition
- Update test description for domain-to-plan credit notice
- Rename test description for domain-to-plan credits hook
To align with the naming used for the domain-to-plan credit notice
@oswian oswian force-pushed the add/plan-notice-for-domain-to-plan-credit branch from 9141a3a to 808e2bf Compare February 5, 2025 00:26
@oswian
Copy link
Contributor Author

oswian commented Feb 5, 2025

Rebased, proceeding with deploy 🚢

@oswian oswian merged commit 3474e76 into trunk Feb 5, 2025
14 checks passed
@oswian oswian deleted the add/plan-notice-for-domain-to-plan-credit branch February 5, 2025 00:47
@a8ci18n
Copy link

a8ci18n commented Feb 5, 2025

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/17229341

Thank you @oswian for including a screenshot in the description! This is really helpful for our translators.

@a8ci18n
Copy link

a8ci18n commented Feb 5, 2025

Translation for this Pull Request has now been finished.

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.

4 participants