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

[#13104] Accounts request form: auto-unify country names #13117

Merged

Conversation

franciscoSavala
Copy link
Contributor

@franciscoSavala franciscoSavala commented May 21, 2024

Part of #13104

Outline of Solution
Added common variations for countries with help of this dataset. I am waiting for a response from the core team to know the common variations in country names. Until then, this is an idea of what the solution could be.

The solution takes the country given by free text of the request account form and then maps it with known countries. Otherwise, it leaves it as is.

@franciscoSavala
Copy link
Contributor Author

#13104 Waiting review...

Copy link
Contributor

@jayasting98 jayasting98 left a comment

Choose a reason for hiding this comment

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

Here are some thoughts on this.

Comment on lines 121 to 125
'United States': 'USA',
US: 'USA',
America: 'USA',
UK: 'United Kingdom',
Deutschland: 'Germany',
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these the only possibilities?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some countries to the list with the change in the issue, and also included the possibility to correct any capitalization typos in the listed countries.

Comment on lines 119 to 126
// Country Mapping
const countryMapping: { [key: string]: string } = {
'United States': 'USA',
US: 'USA',
America: 'USA',
UK: 'United Kingdom',
Deutschland: 'Germany',
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like there is a better place to put this. I am not completely sure where though; perhaps another reviewer has a better idea. However, maybe at the very least, it should be outside the function, like maybe it should be a class constant.

Copy link
Contributor Author

@franciscoSavala franciscoSavala Jun 1, 2024

Choose a reason for hiding this comment

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

I was thinking of creating another class so that the map can only be accessed with a method and cannot be modified, but I'm not sure if it's the correct apporach.

@jayasting98 jayasting98 added c.Feature User-facing feature; can be new feature or enhancement to existing feature s.ToDiscuss The issue/PR is undergoing discussion/review by the core team labels May 29, 2024
@damithc
Copy link
Contributor

damithc commented May 29, 2024

@franciscoSavala I have updated #13104 with a list of common replacements to include

@weiquu weiquu requested a review from jayasting98 June 30, 2024 06:32
@weiquu weiquu added s.ToReview The PR is waiting for review(s) and removed s.ToDiscuss The issue/PR is undergoing discussion/review by the core team labels Jun 30, 2024
@nusoss-bot
Copy link

Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

Copy link
Contributor

@mingyuanc mingyuanc left a comment

Choose a reason for hiding this comment

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

LGTM

@mingyuanc mingyuanc added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Jul 8, 2024
@domoberzin domoberzin self-requested a review July 10, 2024 14:18
Copy link
Contributor

@domoberzin domoberzin left a comment

Choose a reason for hiding this comment

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

LGTM

@domoberzin domoberzin merged commit 3f5f68d into TEAMMATES:master Jul 11, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Feature User-facing feature; can be new feature or enhancement to existing feature s.FinalReview The PR is ready for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants