-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[HOLD for payment 2021-11-04] Fake growl about changes being made in profile is displayed when saving a name that's over 50 characters long #5091
Comments
Triggered auto assignment to @Beamanator ( |
Here is my suggestion, Within ProfilePage.js file we have to add firstname lastname max chars validation within updatePersonalDetails() function before line 170 (i.e. before setPersonalDetails call ). If firstname, lastname max chars. validation failed then we will show growl message with Error and return. i.e. We will not proceed to call setPersonalDetails api in this case. So this way it will also save api call in case of firstname or lastname max length more than allowed chars. Below is the Current Code: App/src/pages/settings/Profile/ProfilePage.js Lines 160 to 183 in 6cd518e
Code will look like as under after First name, last name validation added. updatePersonalDetails() {
const {
firstName,
lastName,
pronouns,
selfSelectedPronouns,
selectedTimezone,
isAutomaticTimezone,
} = this.state;
// Below code added to validate firstname, lastname max. length.
if (firstName.trim().length > 50) {
Growl.show(this.props.translate('profilePage.maxCharsAllowFirstname', {maxValue: 50}), CONST.GROWL.ERROR, 3000);
return;
}
if (lastName.trim().length > 50) {
Growl.show(this.props.translate('profilePage.maxCharsAllowLastname', {maxValue: 50}), CONST.GROWL.ERROR, 3000);
return;
}
setPersonalDetails({
firstName: firstName.trim(),
lastName: lastName.trim(),
pronouns: pronouns === this.props.translate('pronouns.selfSelect')
? selfSelectedPronouns
: pronouns,
timezone: {
automatic: isAutomaticTimezone,
selected: selectedTimezone,
},
});
Growl.show(this.props.translate('profilePage.growlMessageOnSave'), CONST.GROWL.SUCCESS, 3000);
} Also we have to add constants within src/languages/en.js file. profilePage: {
...,
maxCharsAllowFirstname: ({maxValue}) => `First name shouldn't be longer than ${maxValue} characters`,
maxCharsAllowLastname: ({maxValue}) => `Last name shouldn't be longer than ${maxValue} characters`,
}, Also We have to add constants within src/languages/es.js file. profilePage: {
...,
maxCharsAllowFirstname: ({maxValue}) => `El nombre no debe tener más de ${maxValue} caracteres`,
maxCharsAllowLastname: ({maxValue}) => `El apellido no debe tener más de ${maxValue} caracteres`,
}, Please also suggest me proper grammar for English and Spanish if need any changes in message. Here is the screen record how it works once code updated as per my suggestion. Screen.Recording.2021-09-04.at.11.02.30.AM.movAt present External label not set with this issue, so just giving suggestion. It might be helpful for someone if handled internally. If this issue consider as External and my solution accepted then kindly invite me on Upwork project. Here is my Upwork profile: https://www.upwork.com/freelancers/~01c718889ed7821f82 Thank you. |
Hi there, the above proposal looks great and working as expected. But just in case if we want a different solution. I'm adding mine. Inside setPersonalDetails function, instead of optimistically updating the profile details, we can wait for the API completion and show success & error growl based on the jsonCode that we get from response. It will look like this:
Then we can remove the additional success Growl on Demo: video-2021-09-04_11.17.24.mp4 |
Hi @thesahindia , your solution also looks good, but some confusion from my side as under:
If api will not return response.message in localised language then in case of error it will show error message in english, I am not sure that server side api will return error in localised language or not. If api will return localised language message then it is ok. Other major problem with changing setPersonalDetails() code within src/libs/actions/PersonalDetails.js is that setPersonalDetails function may be called from multiple places, so once the code changes it will show Growl success or error message either it need to show or not. For example I can see that setPersonalDetails is called from updateTimezone() function within src/libs/DateUtils.js file, and also from src/libs/Navigation/AppNavigator/AuthScreens.js file within Onyx.connect() at line 65, also called from setAvatar() method within src/libs/actions/PersonalDetails.js file itself. I think it is not good ides to change api logic within lib file src/libs/actions/PersonalDetails.js because it may result to regression and affect the behaviour of other places from where setPersonalDetails() function called. Let expensify engineer to decide which one is better way to go. Also thanks for the suggestion. |
Hi @PrashantMangukiya, Thank you for looking into the solution and giving remarks!
To show the translated version of the message, we can either use App/src/libs/actions/BankAccounts.js Lines 794 to 811 in 6cd518e
I looked into this and found that this PR is already dealing with that (by setting a flag called I hope all the questions are answered now. |
Hi, @thesahindia I got it. Thanks for the message. |
Triggered auto assignment to @stephanieelliott ( |
Sorry for the delay y'all, this looks like a nice external issue so I'm going to begin reviewing proposals 👍 |
Thanks for your proposals @thesahindia and @PrashantMangukiya ! I found this conversation in Slack which covers this situation. Since we have a unique error message that needs to be thrown only in this case, I will make a back-end change to throw this new @thesahindia I like your proposal best, so I'll assign this to you but I'm also going to put this issue on hold until the back-end work is done 👍 |
Thanks @Beamanator, I will wait for the back-end changes. |
Internal PR in motion 👍 |
Quick update: Internal PR was merged, but the changes won't be visible in the production environment till probably tomorrow (Tuesday). Here's an overview of the changes:
|
Just tested, the back end work is not live on production quite yet! Should be soon though. |
Hi @thesahindia, I hate to delay this further but I've placed this issue on hold as per this update, as we are prioritising issues related to a feature release scheduled for 9/24. As an apology for the delay, we will add a $100 bonus as a thank you for waiting. |
PR ready to merge, but won't merge till the n6 hold ends 👍 Nice work @thesahindia ! @stephanieelliott have you created a job for this? I didn't see it above, but I may have missed it :D |
Thanks for the bump, @Beamanator! Just now created the job in Upwork, @thesahindia can you please post a quick proposal so we can get payment arranged once the PR is merged? Here is the job: upwork.com/jobs/~01ab1db8fe7689f675 |
Simply put the maxLength ={50} prop in your text input. User will not able to put more than 50 characters. and don't need to put check like greater than or less than as it will effect on app performance. |
@stephanieelliott, Submitted the proposal. |
Adding Exported since we now have an Upwork post for this job :D |
ok coming late to this issue but I think we should show a red error under the input when the input goes beyond 50 chars following other components of the App. If we know already the there is an issue with input data and we are still waiting for the backend to throw up, does not make sense. The user should know immediately that there is an issue. |
Please refer to this post for updated information on the |
@parasharrajat hmm good point, there's a big form / input field redesign push coming up soon, so I think we can make those changes once that comes through. There is currently some internal discussion about how we want to standardize our fields & forms, as far as I know the discussion has not finished |
@Beamanator with that being the case, should we put this on hold until we figure out how we want to standardize? Or do we keep forging ahead to fix this in the interim? |
Here is one similar comment #5531 (comment) from Marc |
@parasharrajat good point, thanks for linking that convo. @stephanieelliott |
@parasharrajat I updated my thoughts above - what do you say we keep the changes in #5525 to show errors if the back-end fails, but I'm also working on a PR for front-end validation so we ideally will never have to see those errors coming from the back end, but they're handled just in case |
Yup. There is no issue merging that. anyway we will end up using some of the code from that PR. |
@thesahindia are you around to respond to my comments on your PR so we can get it merged soon? |
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.10-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2021-11-04. 🎊 |
hmmmm... I might have jumped the gun here. I'm auditing issues to catch up on payments. I see @thesahindia is assigned to this issue and hired in Upwork so I paid them. Should they be paid for this issue? |
Yes! his PR was merged so thanks for paying 👍 |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
User should be informed that there's a maximum of 50 characters in the first and last name inputs.
Actual Result:
There's a growl saying that the changes were "saved". After reload the name is back to previous name.
More info from @vitHoracek
{"code":666,"jsonCode":666,"type":"Expensify\\Libs\\Error\\ExpError","UUID":"05f93e83-41a5-42a6-ad9c-471d43afae2e","message":"Last name shouldn't be longer than 50 characters","title":"","data":[],"htmlMessage":"","requestID":"2P7skf"}
Workaround:
Don't save a name over 50 characters (?)
Platform:
Where is this issue occurring?
Version Number: 1.0.93-0
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Recording.215.mp4
Expensify/Expensify Issue URL:
View all open jobs on GitHub
From @vitHoracek https://expensify.slack.com/archives/C01GTK53T8Q/p1630687779397100
The text was updated successfully, but these errors were encountered: