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

Donations: Fall back to default products if no products are already defined. #43665

Merged
merged 5 commits into from
Jun 29, 2020

Conversation

kwight
Copy link
Contributor

@kwight kwight commented Jun 24, 2020

The Donations block will rely on membership products existing to give the site visitor options for donating. If the user inserts a Donations block but does not already have some USD subscription products set up, the block will call the new POST /memberships/products endpoint (Automattic/jetpack#16242) to generate some default products.

Part of #42856 .

Screen Shot 2020-06-24 at 4 28 35 PM

Testing Instructions

  • cd apps/full-site-editing; yarn dev --sync;
  • Sandbox the API and the store. More info in PCYsg-lW4-p2 #sandbox-method
  • Sandbox a test site with a paid plan, and ensure the site has the fse-donations-block blog sticker applied (if not, it can be done from the site's Blog RC).
  • At /earn/payments-plans/:site, delete any existing test products.
  • With devtools open, add a Donations block in a new post with the block editor.
  • Connect to Stripe in development mode if necessary.
  • Verify a successful call was made to /memberships/products?type=donation&currency=USD.

@matticbot
Copy link
Contributor

@kwight kwight requested a review from a team June 24, 2020 23:08
@kwight kwight self-assigned this Jun 24, 2020
@kwight kwight added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Block] Donations Donation block labels Jun 24, 2020
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@matticbot
Copy link
Contributor

Caution: This PR affects files in the FSE Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D45454-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing in the FSE Plugin for more info: PCYsg-ly5-p2


setIsLoading( false );
};
if ( result.products.length ) {
Copy link
Member

Choose a reason for hiding this comment

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

We'd need to check there are products for the current currency as well. Right now if you remove all the donation-type plans (AFAIK can be done only with wp shell) and create a few for a currency other than USD, no default plans for USD will be created.

I also think we need to check that returned products contains all intervals and run fetchDefaultProducts if any is missing (the API will create only the missing ones, duplicates prevention is handled in D45327-code).

Copy link
Contributor Author

@kwight kwight Jun 25, 2020

Choose a reason for hiding this comment

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

AFAIK can be done only with wp shell

Ah, found this today: WP.com API v1.1 POST /sites/:site-id/memberships/product/:product-id/delete (got it from the Earn UI when deleting from there). And it has to be the site ID – the domain doesn't work, for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a dev flag for managing this in the Calypso UI? (eg simply return all products in the earn management section) It might be helpful for general debugging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f613589 should take care of the additional checks. I did notice though, that all three defaults were generated even with existing valid intervals, so I'm not sure if that's a bug with what I've done, or the generation.

Copy link
Member

Choose a reason for hiding this comment

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

that all three defaults were generated even with existing valid intervals

Note that the defaults endpoint will return any valid existing plan. It'll create only missing plans. That means the endpoint will return always the 3 plans, but it doesn't mean all of them were generated during the request.

Copy link
Member

Choose a reason for hiding this comment

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

Note that the defaults endpoint will return any valid existing plan. It'll create only missing plans. That means the endpoint will return always the 3 plans, but it doesn't mean all of them were generated during the request.

I confirmed this by deleting a valid plan using the delete endpoint mentioned above and inserting the block. Only one new plan was created and the endpoint keeps returning the existing ones.

@mmtr mmtr force-pushed the add/donations-block-add-defaults branch from 4420b95 to 6434ec2 Compare June 29, 2020 12:44
Copy link
Member

@mmtr mmtr left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@kwight kwight merged commit 569c50b into master Jun 29, 2020
@kwight kwight deleted the add/donations-block-add-defaults branch June 29, 2020 17:05
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Donations Donation block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants