-
Notifications
You must be signed in to change notification settings - Fork 165
Fix: regionalize postal code labels in partial billing address mode #3711
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
base: main
Are you sure you want to change the base?
Fix: regionalize postal code labels in partial billing address mode #3711
Conversation
- Reuse existing ADDRESS_SPECIFICATIONS labels for each country in PARTIAL_ADDRESS_SCHEMA - Ensures consistency between full and partial billing address modes - US shows "Zip Code", other countries show "Postcode" or default "Postal Code" - Add comprehensive tests for partial address mode specifications This fixes an issue where partial billing address mode always showed "Zip Code" regardless of country, while full mode correctly regionalized the label.
👷 Deploy request for adyen-web pending review.Visit the deploys page to approve it
|
|
Summary of ChangesHello @bimsonz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an inconsistency in the billing address forms where the partial address mode failed to regionalize postal code labels, always defaulting to "Zip Code". By integrating the existing country-specific address specifications into the partial address schema, this change ensures that postal code labels are correctly displayed according to the selected country, aligning its behavior with the full billing address mode. This enhancement improves user experience by providing accurate and localized terminology. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly regionalizes the postal code labels for the partial billing address mode by reusing the main address specifications, fixing an inconsistency where 'Zip Code' was always shown. The added tests are comprehensive and validate the fix. I've provided a few suggestions to improve the maintainability of both the configuration and the new tests by reducing code duplication. Overall, this is a solid improvement.
| export const PARTIAL_ADDRESS_SCHEMA: AddressSpecifications = { | ||
| AU: { | ||
| labels: ADDRESS_SPECIFICATIONS.AU.labels, | ||
| schema: [POSTAL_CODE] | ||
| }, | ||
| BR: { | ||
| labels: ADDRESS_SPECIFICATIONS.BR.labels, | ||
| schema: [POSTAL_CODE] | ||
| }, | ||
| CA: { | ||
| labels: ADDRESS_SPECIFICATIONS.CA.labels, | ||
| schema: [POSTAL_CODE] | ||
| }, | ||
| GB: { | ||
| labels: ADDRESS_SPECIFICATIONS.GB.labels, | ||
| schema: [POSTAL_CODE] | ||
| }, | ||
| US: { | ||
| labels: ADDRESS_SPECIFICATIONS.US.labels, | ||
| schema: [POSTAL_CODE] | ||
| }, | ||
| default: { | ||
| labels: { | ||
| [POSTAL_CODE]: 'zipCode' | ||
| }, | ||
| labels: ADDRESS_SPECIFICATIONS.default.labels, | ||
| schema: [POSTAL_CODE] | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The definition of PARTIAL_ADDRESS_SCHEMA is quite repetitive. To improve maintainability and reduce code duplication, you could generate this object dynamically from ADDRESS_SPECIFICATIONS. This would ensure that any future additions to ADDRESS_SPECIFICATIONS are automatically reflected in the partial schema.
export const PARTIAL_ADDRESS_SCHEMA: AddressSpecifications = Object.keys(ADDRESS_SPECIFICATIONS).reduce((acc, countryCode) => {
acc[countryCode] = {
labels: ADDRESS_SPECIFICATIONS[countryCode].labels,
schema: [POSTAL_CODE]
};
return acc;
}, {});| test('should use default postalCode label for GB in partial mode', () => { | ||
| expect(partialSpecifications.getKeyForField('postalCode', 'GB')).toBe('postalCode'); | ||
| }); | ||
|
|
||
| test('should use default postalCode label for CA in partial mode', () => { | ||
| expect(partialSpecifications.getKeyForField('postalCode', 'CA')).toBe('postalCode'); | ||
| }); | ||
|
|
||
| test('should use default postalCode label for AU in partial mode', () => { | ||
| expect(partialSpecifications.getKeyForField('postalCode', 'AU')).toBe('postalCode'); | ||
| }); | ||
|
|
||
| test('should use default postalCode label for BR in partial mode', () => { | ||
| expect(partialSpecifications.getKeyForField('postalCode', 'BR')).toBe('postalCode'); | ||
| }); | ||
|
|
||
| test('should use default postalCode label for unspecified countries in partial mode', () => { | ||
| expect(partialSpecifications.getKeyForField('postalCode', 'FR')).toBe('postalCode'); | ||
| expect(partialSpecifications.getKeyForField('postalCode', 'DE')).toBe('postalCode'); | ||
| expect(partialSpecifications.getKeyForField('postalCode', 'NL')).toBe('postalCode'); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests all check for the same outcome ('postalCode') for different countries. You can combine them into a single parameterized test using test.each to reduce repetition and improve readability.
| test('should use default postalCode label for GB in partial mode', () => { | |
| expect(partialSpecifications.getKeyForField('postalCode', 'GB')).toBe('postalCode'); | |
| }); | |
| test('should use default postalCode label for CA in partial mode', () => { | |
| expect(partialSpecifications.getKeyForField('postalCode', 'CA')).toBe('postalCode'); | |
| }); | |
| test('should use default postalCode label for AU in partial mode', () => { | |
| expect(partialSpecifications.getKeyForField('postalCode', 'AU')).toBe('postalCode'); | |
| }); | |
| test('should use default postalCode label for BR in partial mode', () => { | |
| expect(partialSpecifications.getKeyForField('postalCode', 'BR')).toBe('postalCode'); | |
| }); | |
| test('should use default postalCode label for unspecified countries in partial mode', () => { | |
| expect(partialSpecifications.getKeyForField('postalCode', 'FR')).toBe('postalCode'); | |
| expect(partialSpecifications.getKeyForField('postalCode', 'DE')).toBe('postalCode'); | |
| expect(partialSpecifications.getKeyForField('postalCode', 'NL')).toBe('postalCode'); | |
| }); | |
| test.each(['GB', 'CA', 'AU', 'BR', 'FR', 'DE', 'NL'])('should use default postalCode label for %s in partial mode', countryCode => { | |
| expect(partialSpecifications.getKeyForField('postalCode', countryCode)).toBe('postalCode'); | |
| }); |
| test('partial schema should only contain postalCode field', () => { | ||
| expect(partialSpecifications.getAddressSchemaForCountryFlat('US')).toStrictEqual(['postalCode']); | ||
| expect(partialSpecifications.getAddressSchemaForCountryFlat('GB')).toStrictEqual(['postalCode']); | ||
| expect(partialSpecifications.getAddressSchemaForCountryFlat('FR')).toStrictEqual(['postalCode']); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test can also be parameterized using test.each to avoid repeating assertions for different countries, making the test suite more concise.
test.each(['US', 'GB', 'FR'])('partial schema for %s should only contain postalCode field', countryCode => {
expect(partialSpecifications.getAddressSchemaForCountryFlat(countryCode)).toStrictEqual(['postalCode']);
});- Dynamically generate PARTIAL_ADDRESS_SCHEMA from ADDRESS_SPECIFICATIONS - Use test.each for parameterized tests to reduce duplication - Ensures future countries added to ADDRESS_SPECIFICATIONS are automatically included
|
Great job on the improvements, @bimsonz ! To finalize the PR, could you please beef up the description? We need to add:
|
- Store merchant country in useRef, pass to FieldContainer for labels - Include country in onChange output so parent maintains the value In partial mode the form schema is [postalCode] only - adding country would render an unwanted dropdown. Labels need country for regionalization but it was not being passed through since it is not in form state.
|
Hey @ScottiBR i found an additional fix that was required to persist the country in partial mode when i was testing it in the playground. I've updated the description and added manual testing steps along with screenshots. Let me know if you need anything else. |
This fixes an issue where partial billing address mode always showed "Zip Code" regardless of country, while full mode correctly regionalized the label.
📋 Pull Request Checklist
📝 Summary
This PR fixes postal code label regionalization in partial billing address
mode. Previously, labels were not correctly regionalized because the country
wasn't being passed to the label lookup.
Changes
Commit 1 (6fd9c70): Reuse ADDRESS_SPECIFICATIONS labels in
PARTIAL_ADDRESS_SCHEMA
Commit 2 (8c24be9): Refactor to dynamically generate schema + add tests
Commit 3 (a91c208): Pass merchant country to FieldContainer for label lookup
Changelog note: Fixed partial billing address mode to display regionalized
postal code labels (US shows "Zip code", others show "Postal code")
🧪 Tested scenarios
Manual Test Scenarios
To test, modify
packages/playground/src/pages/Cards/Cards.jsaround line228:
Test 1: US Country
Expected: Label shows "Zip code"
Test 2: Non-US Country (e.g., GB)
Expected: Label shows "Postal code"
🔗 Related GitHub Issue / Internal Ticket number
Closes: