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

Checkout: abstracting Brazil/Ebank form fields #24095

Merged
merged 6 commits into from
Apr 17, 2018

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Apr 12, 2018

This PR extracts extra fields required for Ebanx payments in Brazil from the credit card fields component, in order that we may use them in other payment-related forms such as bank transfer/redirect.

Along with the fields, I'm also moving clientside field validation for checkout/credit card fields into lib/checkout so that we can extend the rule to include the paypal/redirect panels and so on.

There is no new functionality.

For a background see: pxLjZ-4BN-p2

Screenshot

screen shot 2018-04-12 at 4 45 49 pm

Testing

  1. Set your currency to 'BRL'
  2. Add something to your cart and head to checkout
  3. At the payment step, select 'Brazil' as your country
  4. Without entering any data, attempt to submit
  5. Fill in credit card information (use a dummy card no#) and Ebanx fields (a valid tax code is 111.444.777-35)

Expectations

At 3: You should see the Ebanx extra fields
At 4: The fields should show validation messages
At 5: Valid form data should permit payment attempt

@ramonjd ramonjd added [Feature] Checkout The checkout screen and process for purchases made on WordPress.com. Payment Methods labels Apr 12, 2018
@ramonjd ramonjd self-assigned this Apr 12, 2018
@matticbot
Copy link
Contributor

@ramonjd ramonjd requested review from a team April 12, 2018 06:49
@ramonjd ramonjd force-pushed the update/abstract-ebanx-fields branch 2 times, most recently from 29848c9 to 0a14521 Compare April 12, 2018 06:52
@yoavf
Copy link
Contributor

yoavf commented Apr 12, 2018

Works well, but I've noticed a small visual regression with the state/postal code fields.

Before:
screen shot 2018-04-12 at 11 12 35

After:
screen shot 2018-04-12 at 11 12 29

@ramonjd
Copy link
Member Author

ramonjd commented Apr 13, 2018

Thanks for catching that @yoavf

I didn't pull the existing css over to the new component. Now looks like this:

Mobile

screen shot 2018-04-13 at 12 47 51 pm

Desktop

screen shot 2018-04-13 at 12 47 31 pm

@ramonjd
Copy link
Member Author

ramonjd commented Apr 13, 2018

Just realized, after looking how to introduce validation to the redirect payment box, that I need to abstract the ebanx validation rules as well.

More to come on this PR... :)

Copy link
Contributor

@yoavf yoavf left a comment

Choose a reason for hiding this comment

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

This works well, but I think we need to go a little bit deeper and cleanup some of the intermediate code, maybe in a way combine some of shouldRenderAdditionalEbanxFields, isEbanxEnabledForCountry and ebanxFieldRules. Let me know if that doesn't make sense :)

*/
function paymentFieldRules( paymentDetails, paymentType ) {
switch ( paymentType ) {
case 'tef':
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's simplify this for now and remove the 'tef' rule. We can add it to #23830 later.

* Internal dependencies
*/
import { validatePaymentDetails, getCreditCardType } from '../validation';
import { isEbanxEnabledForCountry, isValidCPF } from 'lib/checkout/ebanx';
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that it's in lib/checkout this function should be renamed. The actual meaning right now is "Should ebanx be used for credit-card processing in this country"

Copy link
Member Author

Choose a reason for hiding this comment

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

Bit of a mouthful, but the least prolix I could come up with was: isEbanxCreditCardProcessingEnabledForCountry

@ramonjd ramonjd force-pushed the update/abstract-ebanx-fields branch from 9dce9e8 to 21a9274 Compare April 16, 2018 01:52
@ramonjd
Copy link
Member Author

ramonjd commented Apr 16, 2018

This works well, but I think we need to go a little bit deeper and cleanup some of the intermediate code, maybe in a way combine some of shouldRenderAdditionalEbanxFields, isEbanxEnabledForCountry and ebanxFieldRules.

Thanks for testing. I agree, Ebanx is now peppered about the shop.

By 'combine' do you mean in the same module, or aggregate some of the logic into higher-order functions?

@ramonjd
Copy link
Member Author

ramonjd commented Apr 16, 2018

I've pulled together the common Ebanx methods, and genericized the <EbanxPaymentFields /> in e83f842294bbdbeef04d05487ef89634b8e93e6e.

Let me know if you see any further areas that could be cleaned up.

@yoavf yoavf added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 16, 2018
@yoavf yoavf force-pushed the update/abstract-ebanx-fields branch from d104bb7 to 1aaa846 Compare April 16, 2018 10:23
Returning a fragment to enable testing in enzyme
…o we can use the validation lib for other payment handlers

Updated paths and tests
…bledForCountry` to reflect usage and change in directory structure

Removed switch condition `tef` until #23830
Added extra text for `paymentFieldRules()`
Genericizing <EbanxPaymentFields /> to handle multiple possible ebanx fields
Updated tests and README
@yoavf yoavf force-pushed the update/abstract-ebanx-fields branch from 1aaa846 to e6541c9 Compare April 16, 2018 10:42
@yoavf yoavf added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 16, 2018
Copy link
Contributor

@yoavf yoavf left a comment

Choose a reason for hiding this comment

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

Thanks 👍 Looks great now, and much saner.
Tested manually, and ran locally the relevant e2e tests (which is the reason for all the rebased/forced pushes - sorry about that).

@yoavf yoavf added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 16, 2018
@ramonjd ramonjd merged commit 032cfc6 into master Apr 17, 2018
@ramonjd ramonjd deleted the update/abstract-ebanx-fields branch April 17, 2018 06:34
@matticbot matticbot removed [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Checkout The checkout screen and process for purchases made on WordPress.com. Payment Methods
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants