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

Newsletter imports: Handle errors correctly #99162

Merged
merged 11 commits into from
Feb 7, 2025
Merged

Conversation

spsiddarthan
Copy link
Contributor

@spsiddarthan spsiddarthan commented Jan 31, 2025

Manual imports and CSV imports error handling isn't happening currently, the UI always assumes the imports are successful. This PR fixes it by handling the responses from WPCOM. Keep in mind that you have to sandbox public-api against 171979-ghe-Automattic/wpcom to test this PR.

  • This PR adds error handling for imports. Also, to make a request to the WPCOM API, this code relies on wpcomProxyRequest.
  • wpcomProxyRequest unfortunately considers anything that's not 2xx code as an errror, but doesn't parse the response of the object correctly.
  • The WPCOM endpoint has been returning WP_Errors with a 200 status code, which is why we haven't been seeing these errors on the UI.
  • I attempted to fix the WPCOM and make it return a 400s' but wpcomProxyRequest doesn't parse the resposne correctly and doesn't read the 400 JSON response from WPCOM, it converts it into a string.
  • Chaging wpcomProxyRequest's parsing could lead to side effects since other parts of the code use it as well.
  • I tried replacing wpcomProxyRequest with apiFetch but the proxy requests didn't work as expected.
  • Instead, I decided to change the formatting of the response in the WPCOM PR - 171979-ghe-Automattic/wpcom.

Proposed Changes

  • Added the useImportError hook to see if the API call to import subscribers returns an error.
  • Added error code based error formatting.

Why are these changes being made?

  • To handle the errors from the endpoint that handles subscriber imports.

Testing Instructions

Happy Case

  • On a free account with no subscribers, ensure you're able to add a subscriber manually and with a CSV file of 5 emails. You can ask Chatgpt to generate a csv of emails specyfing the format of the email.
  • On a paid account, ensure you're able to add a subscriber manually and with a CSV file of 100+ emails.

Testing this bug

  • Sandbox 171979-ghe-Automattic/wpcom
  • Cancel the subscription on your paid account from payments admin where you have 100+ subscribers, and try adding a subscriber manually. You should the error being shown in a notice.
  • Try importing a CSV file. You will see the error being shown in a notice. Clicking on Upgrade should take you to the upgrade page.
  • Repeat the above step on a free Jetpack site as well.

Before:

CleanShot.2025-02-05.at.17.22.06.mp4

After:

CleanShot.2025-02-05.at.17.27.48.mp4

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

@matticbot
Copy link
Contributor

matticbot commented Jan 31, 2025

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

Sections (~274 bytes added 📈 [gzipped])

name         parsed_size           gzip_size
subscribers       +748 B  (+0.1%)     +256 B  (+0.1%)
people             +51 B  (+0.0%)      +10 B  (+0.0%)
import             +51 B  (+0.0%)       +8 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 (~211 bytes added 📈 [gzipped])

name                                                 parsed_size           gzip_size
async-load-calypso-my-sites-stats-pages-subscribers       +607 B  (+0.4%)     +211 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.

@spsiddarthan spsiddarthan changed the title Newsleterr imports: Handle errors correctly Newsletter imports: Handle errors correctly Jan 31, 2025
@matticbot
Copy link
Contributor

matticbot commented Jan 31, 2025

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

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

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

To test WordPress.com changes, run install-plugin.sh $pluginSlug add/error-handling on your sandbox.

@spsiddarthan spsiddarthan requested a review from a team January 31, 2025 14:47
@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
@allilevine
Copy link
Member

Hey @spsiddarthan do you mind if I merge #98562? There might be some conflicts.

@spsiddarthan
Copy link
Contributor Author

Of course - feel free to merge that PR. Thanks for checking in, @allilevine. And going forward with other PRs as well, feel free to merge, I will happy to rebase my PRs whenever I begin work the next day :)

Copy link
Member

@allilevine allilevine left a comment

Choose a reason for hiding this comment

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

I rebased and tested (thanks for the CSV tip!) and this is working well. Can we also change the message on the back-end (fbhepr%2Skers%2Sjcpbz%2Sjc%2Qpbagrag%2Serfg%2Qncv%2Qcyhtvaf%2Sraqcbvagf%2Sfhofpevoref.cuc%3Se%3Q604q8o54%231072-og)? It has a typo and it's not very informative. Here's the message for the same error in the Substack importer:

Screen Shot 2025-01-31 at 1 57 27 PM

Something like this? "You've reached the 100-email limit on our free plan, so we couldn't import your subscriber list. Upgrade to any paid plan to upload without limits."

Also, what do you think of leaving the notification in place until the user dismisses it? A link to upgrade would also be great, but I'm not sure if we can pass one to a notice. 🤔

@spsiddarthan
Copy link
Contributor Author

spsiddarthan commented Feb 3, 2025

Thanks, @allilevine for the rebase and the testing.

Something like this? "You've reached the 100-email limit on our free plan, so we couldn't import your subscriber list. Upgrade to any paid plan to upload without limits."

I have updated the copy on WPCOM for this. You should see this in a retest when you sandbox 171979-ghe-Automattic/wpcom.

Screenshot 2025-02-03 at 5 07 11 PM

Also, what do you think of leaving the notification in place until the user dismisses it?

Good idea, thanks, I have made the change.

A link to upgrade would also be great, but I'm not sure if we can pass one to a notice.

It is possible I think, and I took a dig at this, but it quickly became a can of worms for different reasons.

  • The 400 response from the import subscribers is actually returned with a http 200 status code. The backend endpoint - fbhepr%2Skers%2Sjcpbz%2Sjc%2Qpbagrag%2Serfg%2Qncv%2Qcyhtvaf%2Sraqcbvagf%2Sfhofpevoref.cuc%3Se%3Q604q8o54%23%23996-og doesn't return a WP_Error as a 400.
Screenshot 2025-02-03 at 4 41 32 PM Screenshot 2025-02-03 at 4 41 27 PM
  • I took a dig at fixing it on WPCOM by using wp_send_json_error, but the calypso code sees it always like this. I spent some time making wpcomProxyRequest, but couldn't quite do it, the promise rejects with a string and I am not able to see the error object we return from WPCOM. Both the backend and the front-end here seem old and the implementation of it seems a little outdated.
Screenshot 2025-02-03 at 4 24 45 PM
  • After we fix the above, we need to provide the right upgrade link based on whether the site is a Jetpack or a WPCOM one.

Do you think we can merge this PR for now and return to it after fixing the above issues? I can open a few CANI issues to refactor.

@Addison-Stavlo
Copy link
Contributor

Addison-Stavlo commented Feb 3, 2025

"You've hit the 100-email limit on our free plan, so we couldn't import your subscriber list. Upgrade to a paid plan for unlimited uploads."

Q: This email limit is for how many subscribers can be added with 1 file import (and not about total subscribers the site can have)? That is, a user seeing this error can still add more subscribers but just not 100+ at a time?

If that is the case, we might consider some rewording of the copy some. It kind of reads to me like the site hit its subscriber limit in general, although still could probably use some tweaks if thats the case too.

Im not great with new copies personally, I usually ping Dave when I have to finalize them.

@allilevine
Copy link
Member

Do you think we can merge this PR for now and return to it after fixing the above issues? I can open a few CANI issues to refactor.

@spsiddarthan That sounds good to me! Thank you for looking into it 🙌

Q: This email limit is for how many subscribers can be added with 1 file import (and not about total subscribers the site can have)? That is, a user seeing this error can still add more subscribers but just not 100+ at a time?

If that is the case, we might consider some rewording of the copy some. It kind of reads to me like the site hit its subscriber limit in general, although still could probably use some tweaks if thats the case too.

@Addison-Stavlo It's a general limit, not an import limit. If you already have 100 or more subscribers, and you try to import even one, you'll see an error. There's documentation here: https://wordpress.com/support/import-subscribers-to-a-newsletter/#errors-during-import

We can definitely tweak that if it's not clear.

Im not great with new copies personally, I usually ping Dave when I have to finalize them.

cc @davemart-in for your input 🙂

@spsiddarthan
Copy link
Contributor Author

Thanks for the questions and thoughts, @Addison-Stavlo. And, thanks for the answers, @allilevine.

I will wait for @davemart-in's input on the copy before merging this PR. Dave, for context, this is the error message we thought we will show when a free user faces the 100 email subscribers limit -
"You've hit the 100-email limit on our free plan, so we couldn't import your subscriber list. Upgrade to a paid plan for unlimited uploads."

@davemart-in
Copy link
Contributor

Let's shorten the message to just:

You've reached the 100-subscriber limit on our free plan. Upgrade for unlimited subscribers.

Could we use the notice component and add an upgrade link? Here's a mockup:

CleanShot 2025-02-04 at 06 11 16@2x

@spsiddarthan
Copy link
Contributor Author

Let's shorten the message to just:

Thank you!

Could we use the notice component and add an upgrade link? Here's a mockup:

@davemart-in, I wanted to add an upgrade link as well, but there's an issue with uniquely identifying this limits error, which I explained in this comment. Since this route returns multiple error codes, we'd need to refactor both the front-end and backend to handle it properly. The backend is returning a 200 instead of the 400 errors, but the front-end isn't reading the individual error codes when I make WPCOM return the 400s. I am happy to take the refactor on, it's probably a day's work, but wanted to be sure it's worth doing now.

@davemart-in
Copy link
Contributor

Let's proceed with the refactor. Thanks.

@arcangelini
Copy link
Contributor

arcangelini commented Feb 4, 2025

@Addison-Stavlo @allilevine I read this differently, the limit is just how many we allow them to put in themselves. It is not a limit to how many subscribers they can have.

I read this in Jetpack - https://jetpack.com/support/newsletter/import-subscribers/#add-up-to-10000-subscribers

No matter what plan you have, there is no limit to number of people who can choose to subscribe to your blog directly via your subscription form. Writing engaging content and promoting it via Jetpack Blaze or to your social media is a great way to have more followers.

I also read it this way in the code comment:

11 	/**
12 	 * The maximum number of subscribers that do not require a manual review.
13 	 */
14 	public const MAX_SUBSCRIBERS = 10000;
15 
16 	/**
17 	 * The maximum number of subscribers that can be added through the subscriber importer
18 	 * for users on a free plan.
19 	 */
20 	public const MAX_FREE_SUBSCRIBERS = 100;

Separately, I don't think we communicate this very well across the documentation. It also reads to me that 10,000 subscribers is all I am allowed to have when I upgrade even though I think that is a per batch limit.

@allilevine
Copy link
Member

allilevine commented Feb 4, 2025

I read this differently, the limit is just how many we allow them to put in themselves. It is not a limit to how many subscribers they can have.

Got it, so there's a 100 quota on imported subscribers for all time, but a 10k limit on how many people subscribe to the site. That explains why we see free sites with hundreds of subscribers that can't import one or more subscribers. Yeah that's not clear from our documentation.

@Addison-Stavlo
Copy link
Contributor

Oh wow, thanks for the extra details! That is a little complicated to get a copy out thats 100% accurate to users 🤣... So their site has not hit its limit on subscribers, but the user has hit their limit on how many they can import themselves.

Maybe something like "You have reached the free plan limit for importing subscribers. Don't worry, (users|people|?) can still subscribe to your site, but you must upgrade to a paid plan to import more yourself". It gets a little wordy once we try to clarify all the context 😅

@spsiddarthan
Copy link
Contributor Author

spsiddarthan commented Feb 5, 2025

How about this?

Screenshot 2025-02-05 at 4 14 51 PM

@spsiddarthan
Copy link
Contributor Author

I've updated the PR description to explain the reasoning behnd the changes a bit more.


dispatch.importCsvSubscribersStartSuccess( siteId, data.upload_id );
if ( data.upload_id ) {
Copy link
Contributor Author

@spsiddarthan spsiddarthan Feb 5, 2025

Choose a reason for hiding this comment

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

This is the bit where we are handling the response from WPCOM. The else part takes care of the WP_Errors that are returned with a 200 error code.

Copy link
Member

@allilevine allilevine left a comment

Choose a reason for hiding this comment

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

This is working well except for an issue with the success notice that I noted on the back-end PR: 171979-ghe-Automattic/wpcom#issuecomment-1043892

@spsiddarthan
Copy link
Contributor Author

Some additional testing that I performed:

  • Free users are able to add subcribers by typing in their emails.
  • Free users see a notice saying their subscribers have been imported when they try to import the same list of users.
  • Free users are able to import from csv within their limits.
  • WPCOM free users see the error notice when they try to upload beyond 100+subscribers and are taken to the right subscriber page and can upgrade.
  • Jetpack free users see the error notice when they try to upload beyond 100+subscribers and are taken to the right subscriber page and can upgrade.
  • Paid users can import 100+ subscribers
  • Paid users see a notice when they reimport the same subscribers

Copy link
Member

@allilevine allilevine left a comment

Choose a reason for hiding this comment

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

I tested (while sandboxed): the happy paths, the bug, and Jetpack. I got the correct notices and upgrade links. ✅

@spsiddarthan spsiddarthan merged commit 921d762 into trunk Feb 7, 2025
14 checks passed
@spsiddarthan spsiddarthan deleted the add/error-handling branch February 7, 2025 05:11
@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 Feb 7, 2025
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.

6 participants