-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix - legal name - Legal name fields don't allow hyphen so composed names can't be properly introduced #76842
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 - legal name - Legal name fields don't allow hyphen so composed names can't be properly introduced #76842
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
@hungvu193 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
joekaufmanexpensify
left a comment
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.
Reasonable change
|
We still need to wait for the translation confirmation right? @FitseTLT |
yes |
src/CONST/index.ts
Outdated
| SPECIAL_CHARS_WITHOUT_NEWLINE: /((?!\n)[()-\s\t])/g, | ||
| DIGITS_AND_PLUS: /^\+?[0-9]*$/, | ||
| ALPHABETIC_AND_LATIN_CHARS: /^[\p{Script=Latin} ]*$/u, | ||
| ALPHABETIC_AND_LATIN_CHARS_WITH_HYPHEN: /^[\p{Script=Latin} -]*$/u, |
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.
I thought that we should only allow hyphen if it's followed by another character?
Otherwise, we can see something like this, it looks pretty weird and I don't think that's a valid name:
Screen.Recording.2025-12-07.at.22.23.11.mov
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.
That was exactly what I was asking. @JmillsExpensify @trjExpensify WDYT about the above
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.
Waiting on this @JmillsExpensify @trjExpensify
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.
Mhm.. so let's take a step back on the history here.
- We had an issue back in March for the same problem. CC: @dylanexpensify @stitesExpensify @allgandalf.
- We had a conversation about it then (retain channel).
TL;DR of the outcome from @iwiznia.
Anything that's not empty is a valid name
- A PR was merged to fix that here.
- There was a request on the PR to make the necessary BE update(s) to go along with it before merging, but I can't see @stitesExpensify's linked PR(s) for that. Did that not happen?
- It also seems like we didn't create a regression test issue to update the tests so Applause don't report it as a bug after it was changed. (CC: @dylanexpensify).
- That seems to have led to an issue being created by Applause about bugs in the validation logic for the field, which @shubham1206agra commented on the original PR here as being the root cause of.
- ... and now we're here.
So, has anything changed between our last outcome and now? If not, then we're still expecting anything but an empty value is a valid legal name. This time, can we please make sure:
- Any BE changes that need to be made is done first before the FE changes.
- We create a regression test and engage Applause to avoid coming back to square one.
Thanks!
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.
I'm up if we decide to allow any name other than empty string on the BE, lemme know 🙇
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.
In this issue, we have implemented stricter rules for our legal name validation. However, the current validation allows hyphen character that's not followed by another Latin character, please see my comment.
I'd suggest that we should only hyphen if it's followed by another Latin character. For example:
hans-vu is valid name while hans-, -hans aren't.
Let me know your thoughts 😄
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.
In this issue, we have implemented stricter rules for our legal name validation
What issue?
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.
@hungvu193 would you be able to raise a discussion in slack, that way we all can communicate faster :)
|
Updated @hungvu193 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid.movAndroid: mWeb ChromemChrome.moviOS: HybridAppIOS.moviOS: mWeb SafarimSafari.movMacOS: Chrome / SafariChrome.mov |
hungvu193
left a comment
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.
Discussion in here: https://expensify.slack.com/archives/C01GTK53T8Q/p1765461123418859
The prettier is on main. I'll go ahead and approve.
LGTM.
|
NAB: @FitseTLT can you please update |
|
Prettier is failing |
|
Updated Explanation of change and tests passed. @MonilBhavsar |
|
thanks! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.2.78-0 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.2.78-8 🚀
|
Explanation of Change
This PR fixes a regression to remove a constraint of only allowing latin characters for legal names. We want to allow any type of characters for legal names. here is a discussion for more context.
Fixed Issues
$ #74490
PROPOSAL: #74490 (comment)
Tests
Offline tests
Same as above
QA Steps
Same as above
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
2025-12-06.00-54-38.mp4
Android: mWeb Chrome
2025-12-05.22-17-30.mp4
iOS: Native
2025-12-05.21-36-24.mp4
iOS: mWeb Safari
2025-12-05.22-16-58.mp4
MacOS: Chrome / Safari
2025-12-05.22-16-30.mp4