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(installments): preselect value on brand change #2117

Merged
merged 1 commit into from
Apr 21, 2023

Conversation

longyulongyu
Copy link
Contributor

@longyulongyu longyulongyu commented Apr 17, 2023

Summary

  • Fix the issue that the preselect value is not working for the Installments component
  • Add tests to test brand change effect

Explanation for the root cause:

  • Let's say we have the following configuration:
                installmentOptions: {
                    mc: {
                        values: [1, 2, 3],
                        preselectedValue: 2
                    }         
                },
  • During the initial rendering, the brand is card, as we are setting the initial installmentAmount value to installmentOptions?.preselectedValue || installmentOptions?.values[0] in the useState hook, it sets the initial installmentAmount value to undefined.
  • Then the useEffect is called, newAmount is still undefined
  • Change the card number to a master card. As useState is stateful, it would NOT get called, hence the installmentAmount stays undefined at this stage.
  • However the useEffect would be called because the brand changed. As the current installmentAmount is undefined, the installmentOptions?.values?.includes(installmentAmount) is false. newAmount would set to installmentOptions?.values[0], which is 1.
  • That's why the preselectedValue is not working as it was not used in the useEffect hook.

Fix:

  • Based on the analysis, we need to change the useEffect to
setInstallmentAmount(installmentOptions?.preselectedValue ?? installmentOptions?.values[0]);

so that the preselectedValue is picked up.

  • To keep the same intention as the previous code, when the new branch installmentOptions?.values?.includes(installmentAmount), we skip setting up the new value for installmentAmount

Tested scenarios

  • Go to the playground http://localhost:3020/cards
  • Go to the CARD section
  • Fill in a test master card number, the preselected value 2x $129.50 for the installments is selected in the Installments payment field.
  • Switching to a visa card, the preselected value would depend on if the visa installment configuration supports that value. If so, the Installments payment field would remain unchanged. Otherwise, it would switch to the visa installmentOptions (either to the preselectedValue or values[0] )

@longyulongyu longyulongyu changed the title Fix(installments): preselect value on brand change [WIP]Fix(installments): preselect value on brand change Apr 17, 2023
@sonarcloud
Copy link

sonarcloud bot commented Apr 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link
Contributor

Size Change: +25 B (0%)

Total Size: 1.02 MB

Filename Size Change
packages/lib/dist/adyen.js 274 kB +8 B (0%)
packages/lib/dist/cjs/index.js 233 kB +5 B (0%)
packages/lib/dist/es.modern/index.js 113 kB +1 B (0%)
packages/lib/dist/es/index.js 130 kB +11 B (0%)
ℹ️ View Unchanged
Filename Size
packages/lib/dist/es.modern/ar.js 6.17 kB
packages/lib/dist/es.modern/cs-CZ.js 5.59 kB
packages/lib/dist/es.modern/da-DK.js 5.06 kB
packages/lib/dist/es.modern/de-DE.js 5.47 kB
packages/lib/dist/es.modern/el-GR.js 7 kB
packages/lib/dist/es.modern/es-ES.js 5.15 kB
packages/lib/dist/es.modern/fi-FI.js 5.16 kB
packages/lib/dist/es.modern/fr-FR.js 5.26 kB
packages/lib/dist/es.modern/hr-HR.js 5.29 kB
packages/lib/dist/es.modern/hu-HU.js 5.55 kB
packages/lib/dist/es.modern/it-IT.js 5.14 kB
packages/lib/dist/es.modern/ja-JP.js 5.96 kB
packages/lib/dist/es.modern/ko-KR.js 5.59 kB
packages/lib/dist/es.modern/nl-NL.js 5.19 kB
packages/lib/dist/es.modern/no-NO.js 5.07 kB
packages/lib/dist/es.modern/pl-PL.js 5.55 kB
packages/lib/dist/es.modern/pt-BR.js 5.13 kB
packages/lib/dist/es.modern/pt-PT.js 5.25 kB
packages/lib/dist/es.modern/ro-RO.js 5.34 kB
packages/lib/dist/es.modern/ru-RU.js 6.5 kB
packages/lib/dist/es.modern/sk-SK.js 5.65 kB
packages/lib/dist/es.modern/sl-SI.js 5.22 kB
packages/lib/dist/es.modern/sv-SE.js 5.03 kB
packages/lib/dist/es.modern/zh-CN.js 5.41 kB
packages/lib/dist/es.modern/zh-TW.js 5.53 kB
packages/lib/dist/es/ar.js 6.17 kB
packages/lib/dist/es/cs-CZ.js 5.59 kB
packages/lib/dist/es/da-DK.js 5.06 kB
packages/lib/dist/es/de-DE.js 5.47 kB
packages/lib/dist/es/el-GR.js 7 kB
packages/lib/dist/es/es-ES.js 5.15 kB
packages/lib/dist/es/fi-FI.js 5.16 kB
packages/lib/dist/es/fr-FR.js 5.26 kB
packages/lib/dist/es/hr-HR.js 5.29 kB
packages/lib/dist/es/hu-HU.js 5.55 kB
packages/lib/dist/es/it-IT.js 5.14 kB
packages/lib/dist/es/ja-JP.js 5.96 kB
packages/lib/dist/es/ko-KR.js 5.59 kB
packages/lib/dist/es/nl-NL.js 5.19 kB
packages/lib/dist/es/no-NO.js 5.07 kB
packages/lib/dist/es/pl-PL.js 5.55 kB
packages/lib/dist/es/pt-BR.js 5.13 kB
packages/lib/dist/es/pt-PT.js 5.25 kB
packages/lib/dist/es/ro-RO.js 5.34 kB
packages/lib/dist/es/ru-RU.js 6.5 kB
packages/lib/dist/es/sk-SK.js 5.65 kB
packages/lib/dist/es/sl-SI.js 5.22 kB
packages/lib/dist/es/sv-SE.js 5.03 kB
packages/lib/dist/es/zh-CN.js 5.41 kB
packages/lib/dist/es/zh-TW.js 5.53 kB

compressed-size-action

@longyulongyu longyulongyu marked this pull request as ready for review April 18, 2023 08:36
@longyulongyu longyulongyu self-assigned this Apr 18, 2023
@longyulongyu longyulongyu changed the title [WIP]Fix(installments): preselect value on brand change Fix(installments): preselect value on brand change Apr 18, 2023
@longyulongyu longyulongyu merged commit 0fa5e79 into master Apr 21, 2023
@longyulongyu longyulongyu deleted the fix/preselect_value_installments branch April 21, 2023 14:28
@sponglord sponglord mentioned this pull request Apr 24, 2023
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.

2 participants