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

🚧🧠🌴 Fix Braintree 3DS in Dev portal (and much more) #3079

Conversation

josemigallas
Copy link
Contributor

@josemigallas josemigallas commented Oct 19, 2022

Related:

What this PR does / why we need it:

THREESCALE-8967: Adding Braintree SCA card fails

We load braintree-web SDK from node_modules AND as a cdn. Besides, the whole braintree flow is implemented in a convoluted way and it's hard to maintain.

First, this PR cleans up the pack used in developer portal that renders a React component which uses braintree-web imported from node_modules.

Second, it removes the dependency from braintre-web CDN by moving the script from admin portal's view into a pack and importing the SDK from node_nodules.

TODO:

  • 3D secure flow not working at the moment

verification

Testing credit cards:

with 3DS disabled 4111111111111111
3DS successful with no challenge 4000000000001000
3DS successful with challenge 4000000000001091
3DS unsuccessful with challenge 4000000000001109

NOTE: both forms: payment details (admin portal) and credit card details (dev portal), are basically the same. They share the same logic and have the same fields. However, one is renders via formtastic in a slim template (edit.html.slim) whereas the other is a React component (BraintreeForm.tsx). The only problem is mainly the differences between class names. Example:

Admin portal (PF4 style)

<li class="string input required stringish" id="customer_first_name_input">
  <label for="customer_first_name" class="label">First name<abbr title="required">*</abbr></label>
  <input id="customer_first_name" type="text" value="Pepe" name="customer[first_name]">
</li>

Dev portal (dev portal style)

<li class="string optional form-group" id="customer_first_name_input">
  <label class="col-md-4 control-label" for="customer_first_name">First name *</label>
  <input required="" class="col-md-6 form-control" id="customer_first_name" name="customer[first_name]" value="Pepe">
</li>

This to me looks like repetition and we should be able to use React in both (or none..) but it goes out of scope of this PR.

@josemigallas josemigallas self-assigned this Oct 19, 2022
@josemigallas josemigallas changed the title 🚧 Refactor Braintree form Refactor Braintree form Oct 28, 2022
@josemigallas josemigallas marked this pull request as ready for review October 28, 2022 17:03
@akostadinov
Copy link
Contributor

Can't really review but looks much better than before.

fields: {
number: {
selector: '#customer_credit_card_number',
placeholder: '4111 1111 1111 1111'
Copy link
Contributor

Choose a reason for hiding this comment

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

does this come from automatically now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it comes from https://github.com/3scale/porta/pull/3079/files#diff-e5280eb9a5ca2994755960970425ea7ff092eef504a1a954682533ea1d1cc75c and it's the same as in the Dev portal form.
This was copy-pasted from a Braintree docs example.

jlledom
jlledom previously approved these changes Nov 8, 2022
@josemigallas josemigallas changed the title Refactor Braintree form 🚧 Refactor Braintree form Jan 19, 2023
@josemigallas josemigallas changed the title 🚧 Refactor Braintree form 🚧 Clean up Braintree forms and remove duplicated dependency from braintree-web Jan 19, 2023
Comment on lines 89 to 96
/**
* Use when mounting a component with promises or side effects, such as BraintreeForm#onSubmit
*/
async function waitForPromises (): Promise<void> {
await act(async () => {
await new Promise((resolve) => setTimeout(resolve))
})
}

Copy link
Contributor

Choose a reason for hiding this comment

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

May you explain this? I don't understand it. AIUI this won't return a Promise b/c the only call in the function is preceded by an await. Besides, there's no return keyword. What does act() do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

async / await is syntactic sugar for wrapping something with a promise. Not returning means the method returns undefined. But if method awaits something inside, then it has to be marked as async and it will wrap this undefined inside a Promise, therefore it will return Promise<void>. In this scenario, a return statement is redundant.

Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but still don't understand the code. What does act() do? and why calling setTimeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anything that triggers a change in the component has to be wrapped around act.

setTimeout is used to add a delay to the promise so that it doesn't resolve immediately.

@josemigallas josemigallas changed the title 🚧 Clean up Braintree forms and remove duplicated dependency from braintree-web 🚧 🧠 🌴 Clean up Braintree forms and remove duplicated dependency from braintree-web Jan 23, 2023
@josemigallas josemigallas changed the title 🚧 🧠 🌴 Clean up Braintree forms and remove duplicated dependency from braintree-web 🧠 🌴 Clean up Braintree forms and remove duplicated dependency from braintree-web Jan 31, 2023
@josemigallas josemigallas requested review from akostadinov, jlledom and a team January 31, 2023 17:08
jlledom
jlledom previously approved these changes Feb 1, 2023
Copy link
Contributor

@jlledom jlledom left a comment

Choose a reason for hiding this comment

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

I don't feel able to review all the TS code, so I cloned the branch and tried to make it work locally. Everything seems to work fine for me, so approved.

@josemigallas josemigallas changed the title 🧠 🌴 Clean up Braintree forms and remove duplicated dependency from braintree-web 🧠🌴 Clean up Braintree forms and remove duplicated dependency from braintree-web Feb 9, 2023
@josemigallas josemigallas changed the title 🧠🌴 Clean up Braintree forms and remove duplicated dependency from braintree-web 🧠🌴 Fix Braintree 3DS in Dev portal (and much more) Feb 14, 2023
@josemigallas josemigallas changed the title 🧠🌴 Fix Braintree 3DS in Dev portal (and much more) 🚧🧠🌴 Fix Braintree 3DS in Dev portal (and much more) Feb 15, 2023
@josemigallas josemigallas changed the base branch from master to braintree_provider_form February 15, 2023 12:56
@josemigallas josemigallas deleted the refactor_braintree_form branch February 15, 2023 15:02
@josemigallas josemigallas restored the refactor_braintree_form branch February 15, 2023 15:05
@josemigallas josemigallas reopened this Feb 15, 2023
@josemigallas josemigallas deleted the branch 3scale:braintree_provider_form February 15, 2023 16:47
@josemigallas josemigallas deleted the refactor_braintree_form branch February 16, 2023 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants