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

Recurring Payments Block: Minimum transaction amounts #14802

Merged
merged 7 commits into from
Mar 13, 2020

Conversation

beaucollins
Copy link
Contributor

@beaucollins beaucollins commented Feb 25, 2020

Uses Stripe's minimum transaction amounts per currency.

Changes proposed in this Pull Request:

  • Uses Stripe's minimum transaction amounts per currency.
  • Sets the input minimum value to 0 to allow for better stepper experience
  • Prevents using less than minimum amount during component state change
  • Shows the minimum price for the selected currency below the Price input field.

The placeholder text uses formatCurrency which is nice, however it somewhat misleads the user into thinking they can type the currency symbols into the field while we actually only allow that which can be parsed by parseFloat which means values like $0.50 or CHF 0,50 are not considered valid. Should we allow this kind of input?

Testing instructions:

This assumes the "Recurring Payments Block" is available to your Jetpack plan for the site you are testing.

Setup
  • Go to "Posts → Add New"
  • Add a block and type /recurr select "Recurring Payments button"
    image
  • If a plan already exists press the "Add a plan" button to display the form:
    image
Test
  • Clear out the "price" value, the placeholder should show the minimum price
    image

  • Change the currency, if the price is now too low for the given currency, it should show the error (e.g. start with USD and 0.5 and change to MXN)
    image

Proposed changelog entry for your changes:

  • Recurring Payments block minimum price is updated and is dependent on selected currency.

@jetpackbot
Copy link
Collaborator

jetpackbot commented Feb 25, 2020

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 3ce8d81


/**
* Returns the minimum transaction amount for the given currency. If currency is not one of the
* known types it returns ...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@artpi Should we default to a value if the currency is not one of the known options? This shouldn't happen in practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

The currency list gets populated from these constants above. So that should not be happening and we don't want want fallbacks - we want errors in this case :)

@beaucollins beaucollins requested a review from a team February 25, 2020 21:35
@beaucollins beaucollins force-pushed the recurring-payments/minimum-transaction branch from 8d324ef to d943f12 Compare February 25, 2020 21:45
@jeherve jeherve added this to the 8.4 milestone Feb 25, 2020
@jeherve jeherve added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Feb 25, 2020
@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Feb 25, 2020
@beaucollins beaucollins added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Feb 25, 2020
@beaucollins beaucollins requested a review from jeherve February 25, 2020 23:01
artpi
artpi previously approved these changes Feb 26, 2020
Copy link
Contributor

@artpi artpi left a comment

Choose a reason for hiding this comment

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

My comments are not huge blockers


/**
* Returns the minimum transaction amount for the given currency. If currency is not one of the
* known types it returns ...
Copy link
Contributor

Choose a reason for hiding this comment

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

The currency list gets populated from these constants above. So that should not be happening and we don't want want fallbacks - we want errors in this case :)

@beaucollins beaucollins force-pushed the recurring-payments/minimum-transaction branch from 346cb8f to e91674e Compare February 26, 2020 20:10
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello beaucollins! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D39465-code before merging this PR. Thank you!

Recurring Payments Block: Minimum transaction amounts

Uses Stripe's minimum transaction amounts per currency.

- Sets the input minimum value to 0 to allow for better stepper experience
- Prevents using less than minimum amount during component state change
@beaucollins beaucollins force-pushed the recurring-payments/minimum-transaction branch from e91674e to cd7f368 Compare February 26, 2020 20:13
@matticbot
Copy link
Contributor

beaucollins, Your synced wpcom patch D39465-code has been updated.

Copy link
Contributor

@artpi artpi left a comment

Choose a reason for hiding this comment

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

In saveProduct function there is still this.state.editedProductPrice < 5 which does not allow to actually save these products:
Zrzut ekranu 2020-02-28 o 10 28 17

@matticbot
Copy link
Contributor

beaucollins, Your synced wpcom patch D39465-code has been updated.

@beaucollins
Copy link
Contributor Author

In saveProduct function there is still this.state.editedProductPrice < 5 which does not allow to actually save these products:

Good catch @artpi, updated in fc53ad2.

@kraftbj kraftbj added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Mar 5, 2020
@beaucollins
Copy link
Contributor Author

@artpi can you chime in about the question:

For example, if on 8.3 before this PR, I add a plan for MX$5 (where the minimum is MX$10), after upgrade, there is seemingly no notification that I have an invalid plan active on my site or ability to update it.

@artpi
Copy link
Contributor

artpi commented Mar 5, 2020

I'm curious about the migration plan for existing invalid recurring payment blocks.
For example, if on 8.3 before this PR, I add a plan for MX$5 (where the minimum is MX$10), after upgrade, there is seemingly no notification that I have an invalid plan active on my site or ability to update it.

Oooh, nice catch @kraftbj

There is no migration path because they are plainly not working. If you add a MX$5 , the transaction will fail during checkout, with an error that is confusing to the user.
This whole PR is intended to fix this problem.

There are 2 users with plans in MXN<10 and 4 users with plans in JPY that have this issue.

@beaucollins To make sure everything is proper in the block, let's filter out the invalid subscriptions (make them impossible to select)

We will not deal with those 6 users. Their subscriptions are not working anyway, by definition these plans have no subscribers, so they can retry and create a new plan.

@kraftbj
Copy link
Contributor

kraftbj commented Mar 5, 2020

Making invalid plans impossible to select and not providing a fix for those six is reasonable to me.

If you could Slack me those users or how to pull that list, I'm happy to reach out to them proactively to let them know they need to amend their subscriptions. That'd be a nice priority support outreach.

@matticbot
Copy link
Contributor

beaucollins, Your synced wpcom patch D39465-code has been updated.

@matticbot
Copy link
Contributor

beaucollins, Your synced wpcom patch D39465-code has been updated.

@beaucollins
Copy link
Contributor Author

@kraftbj @artpi membership options that do not meet the minimum threshold are no longer available to be selected.

Currently they are filtered out as opposed to "shown and disabled".

@beaucollins beaucollins requested a review from a team March 6, 2020 20:05
@beaucollins beaucollins added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 6, 2020
Copy link
Contributor

@kraftbj kraftbj 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 the updates! Let's do this.

@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 10, 2020
@artpi
Copy link
Contributor

artpi commented Mar 11, 2020

Hey @beaucollins I merged something and suspect you will have to:

  • rebase this
  • wait for D39465-code to pick up new changes (otherwise it will have conflicts )
  • get new approval from this from Brandon
  • merge D39465-code
  • Merge this

@matticbot
Copy link
Contributor

beaucollins, Your synced wpcom patch D39465-code has been updated.

1 similar comment
@matticbot
Copy link
Contributor

beaucollins, Your synced wpcom patch D39465-code has been updated.

@beaucollins
Copy link
Contributor Author

D39465 is deployed.

@beaucollins beaucollins merged commit 771f691 into master Mar 13, 2020
@beaucollins beaucollins deleted the recurring-payments/minimum-transaction branch March 13, 2020 18:53
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 13, 2020
jeherve added a commit that referenced this pull request Mar 20, 2020
jeherve added a commit that referenced this pull request Mar 31, 2020
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Payment Button aka Recurring Payments Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants