[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

[isagoico]

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:

1. Navigate to settings > Profile
2. Change the first or last name to something over 50 characters
3. Save the changes
4. Refresh the page

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

The response of the API call is 666 so we should not handle it as success.

{"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?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

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

When you update your first OR last name with very long name, we present successful growl message and optimistically change the name on front end. After reload the name is back to previous name. Based on the API call response, the limit is 50 characters, but we do not handle the response correctly. Details in

[MelvinBot]

Triggered auto assignment to @Beamanator (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[PrashantMangukiya]

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

	updatePersonalDetails() {
	const {
	firstName,
	lastName,
	pronouns,
	selfSelectedPronouns,
	selectedTimezone,
	isAutomaticTimezone,
	} = this.state;
	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);
	}

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.mov

At 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.

[thesahindia]

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:

/**
 * Sets the personal details object for the current user
 *
 * @param {Object} details
 */
function setPersonalDetails(details) {
    API.PersonalDetails_Update({details: JSON.stringify(details)})
        .then((response) => {
            if (response.jsonCode === 200) {
                Growl.show(translateLocal('profilePage.growlMessageOnSave'), CONST.GROWL.SUCCESS, 3000);
            } else {
                Growl.error(response.message, 3000);
                return;
            }

            if (details.timezone) {
                NameValuePair.set(CONST.NVP.TIMEZONE, details.timezone);
            }
            mergeLocalPersonalDetails(details);
        }).catch((error) => {
            console.debug('Error while setting personal details', error);
        });
}

Then we can remove the additional success Growl on ProfilePage.js

Demo:

video-2021-09-04_11.17.24.mp4

[PrashantMangukiya]

Hi @thesahindia , your solution also looks good, but some confusion from my side as under:

if (response.jsonCode === 200) {
     Growl.show(translateLocal('profilePage.growlMessageOnSave'), CONST.GROWL.SUCCESS, 3000);
} else {
    Growl.error(response.message, 3000);
     return;
 }

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.

[thesahindia]

Hi @PrashantMangukiya, Thank you for looking into the solution and giving remarks!
Both of the points are valid and I'll try to reply to them.

If api will not return response.message in localised language then in case of error it will show error message in english,

To show the translated version of the message, we can either use jsonCode or message of the response and based on that we can set correct message
An example where something similar is implemented:

App/src/libs/actions/BankAccounts.js

Lines 794 to 811 in 6cd518e

	if (response.jsonCode === 666 || response.jsonCode === 404) {
	error = response.message;
	}
	if (response.jsonCode === 402) {
	if (response.message === CONST.BANK_ACCOUNT.ERROR.MISSING_ROUTING_NUMBER
	|| response.message === CONST.BANK_ACCOUNT.ERROR.MAX_ROUTING_NUMBER
	) {
	errors.routingNumber = true;
	achData.subStep = CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL;
	} else if (response.message === CONST.BANK_ACCOUNT.ERROR.MISSING_INCORPORATION_STATE) {
	error = translateLocal('bankAccount.error.incorporationState');
	} else if (response.message === CONST.BANK_ACCOUNT.ERROR.MISSING_INCORPORATION_TYPE) {
	error = translateLocal('bankAccount.error.companyType');
	} else {
	console.error(response.message);
	}
	}

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.

I looked into this and found that this PR is already dealing with that (by setting a flag called shouldGrowl) so no regression might occur. (We can just tweak that a little if we want growls after API completion)

I hope all the questions are answered now.

[PrashantMangukiya]

Hi, @thesahindia I got it. Thanks for the message.

[MelvinBot]

Triggered auto assignment to @stephanieelliott (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[Beamanator]

Sorry for the delay y'all, this looks like a nice external issue so I'm going to begin reviewing proposals 👍

[Beamanator]

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 jsonCode (I'll post here when I know what that code will be) so it can be handled on the front end.

@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 👍

[thesahindia]

Thanks @Beamanator, I will wait for the back-end changes.

[Beamanator]

Internal PR in motion 👍

[Beamanator]

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:

• The 'First name shouldn\'t be longer than 50 characters' error will have a jsonCode of 400
• The 'Last name shouldn\'t be longer than 50 characters' error will have a jsonCode of 401

[stephanieelliott]

Just tested, the back end work is not live on production quite yet! Should be soon though.

[kadiealexander]

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.

[Beamanator]

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

[stephanieelliott]

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

[dalbeer22g]

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.

[thesahindia]

@stephanieelliott, Submitted the proposal.

[Beamanator]

Adding Exported since we now have an Upwork post for this job :D

[parasharrajat]

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.
@Beamanator

[kadiealexander]

Please refer to this post for updated information on the n6 hold, we've raised the bonus to $250 for all issues where a PR is created before the N6 hold is lifted.

[Beamanator]

@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

[stephanieelliott]

@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?

[parasharrajat]

Here is one similar comment #5531 (comment) from Marc

[Beamanator]

@parasharrajat good point, thanks for linking that convo.

@stephanieelliott I think we pay out thesahindia for his work (since I approved the proposal and he did a great job, I would have merged his code if we hadn't been on n6-hold), close this one & his PR, and I'll create a new issue with updated Expected vs Actual result tomorrow - Actually I take this back, I think thesahindia's changes are very useful, but we should also do more front-end changes

[Beamanator]

@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

[parasharrajat]

Yup. There is no issue merging that. anyway we will end up using some of the code from that PR.

[Beamanator]

@thesahindia are you around to respond to my comments on your PR so we can get it merged soon?

[botify]

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:

• show profile errors #5525

If no regressions arise, payment will be issued on 2021-11-04. 🎊

[mallenexpensify]

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?

[Beamanator]

Yes! his PR was merged so thanks for paying 👍