Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.55k stars 2.9k forks source link

[HOLD for regression test] [$4000] Bug: Entering 0 as a name causes problems #13779

Closed kavimuru closed 1 year ago

kavimuru commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Problem

Entering 0 as a name can cause strange errors because on the backend we check if the name is empty and "0" is considered empty in PHP. There are a few different manifestations of this problem listed below. Whoever is assigned to work on this issue should first try to find all occurrences of this problem in the app so we can fix them all at once.

Solution

Use front end validation to prevent a user from entering "0" as a name anywhere in the app. To do so, create a validation helper in the ValidationUtils that will return false if "0" is provided for a name. Next use that validation helper where relevant and in accordance with our form guidelines.

On the backend make sure that "0" can not be saved as a valid name anywhere in the app.

Additionally, let's match the front and back end validation exactly so that the user has timely feedback when they enter an invalid name.

Backend validation

Known bugs

1. Workspace name

Action Performed:

  1. Go to settings > workspace >General settings
  2. edit workspace name to 0
  3. click save

Expected Result:

0 should be considered an invalid workspace name and an error should be shown before clicking save

Actual Result:

0 is considered a valid workspace name until after you click save, then an error is shown that it's an invalid name.

Notes/Photos/Videos:

invalid workspace

https://user-images.githubusercontent.com/43996225/209018852-109b8ad8-69ef-41e3-badf-0a8052bdcd82.mov

2. Profile name

Action Performed:

  1. Click on the profile icon
  2. Click on Profile
  3. Click display name
  4. Change the first name to 0
  5. Make sure the last name is empty
  6. Click Save

Expected Result:

0 should be considered an invalid profile name and an error should be shown before clicking save

Actual Result:

There is no error and on the profile page the display name appears as the user's email address

Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/212992836-3c742a03-eb3b-400d-a1c0-a7258285fd17.mov

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on? All

Version Number: 1.2.42-2 Reproducible in staging?: y Reproducible in production?: y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @gadhiyamanan Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1671635151324279

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bc5fba9186abc46a
  • Upwork Job ID: 1616174276234260480
  • Last Price Increase: 2023-02-02
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @lschurr (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

neil-marcellini commented 1 year ago

I'm going to make this weekly while it's on hold.

lschurr commented 1 year ago

Believe this is still on hold.

neil-marcellini commented 1 year ago

WAQ is under control for now so I'm taking this off hold. I think we can make this external because we can show an error from the frontend if "0" is entered as a name.

Alternatively we could allow "0" as a valid name and fix it on the backend, but I doubt that we want to do that. I'll ask for some more opinions since I'm not sure.

neil-marcellini commented 1 year ago

Posted in Slack here.

neil-marcellini commented 1 year ago

In Slack we decided that "0" should not be a valid name and that it should be validated on the front end and the backend. I can take care of the backend and I'll mark the issue external so that a contributor can take care of the front end.

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01bc5fba9186abc46a

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External)

melvin-bot[bot] commented 1 year ago

Current assignee @neil-marcellini is eligible for the External assigner, not assigning anyone new.

sanjayharan commented 1 year ago

@neil-marcellini Hey Can you please specify the accepted value format of InpuTextField for the workspace name?

Puneet-here commented 1 year ago

Proposal

We need to use another this.assignError here like the one below. We have to check if values.firstName is equal to 0 (values.firstName && Number(values.firstName) === 0)in a variable and pass that as 3rd argument (or use directly) and pass 0 for error message argument ( like this {invalidCharacter: 0})

https://github.com/Expensify/App/blob/b50364bda83f2c6e621ccf1b3b27ffdb2c598249/src/pages/settings/Profile/DisplayNamePage.js#L65-L71

Edit:- It will look like this -

 this.assignError( 
     errors, 
     'firstName', 
     isValidFirstName, 
     Localize.translateLocal( 
         'personalDetails.error.hasInvalidCharacter', 
         {invalidCharacter: 0},

isValidFirstName will be the new variable (not sure about the name though )

Didn't see the workspace case. We will need to add validation there too https://github.com/Expensify/App/blob/b50364bda83f2c6e621ccf1b3b27ffdb2c598249/src/pages/workspace/WorkspaceSettingsPage.js#L64

 if (Number(values.name) === 0) {
            errors.name = this.props.translate('personalDetails.error.hasInvalidCharacter',
                {invalidCharacter: 0});
        }
sanjayharan commented 1 year ago

Proposal: I think solution proposed by @kavimuru is good. So that we should create a validation helper in the ValidationUtils that will return false if "0" is provided for a name and as a enhancement i think we should create regex expression for validating workspace name.

Puneet-here commented 1 year ago

I think we should check other pages to see if the same problem exists there. I will check other places and update my proposal if I see more instances.

sanjayharan commented 1 year ago

Proposal:
(1) regex validation in ValidationUtils file (2) Update constant file (3) Update function validate() in WorkSpaceSettingsPage

             We can tweak Regular Expression as per our needs, for instance i have created regex as discussed in slack.

             Same thing can be done for first name and last name input field in DisplayNamePage file. But for that we required clarity on restrictions needs to be added on the fields.   
Screenshot 2023-01-20 at 2 11 05 PM Screenshot 2023-01-20 at 2 11 23 PM Screenshot 2023-01-20 at 2 11 38 PM
mollfpr commented 1 year ago

@neil-marcellini For this issue, we only focus on fixing the workspace name and profile display name?

neil-marcellini commented 1 year ago

Thanks for the proposals and discussion. So far I like @Puneet-here's proposal best. It needs to be updated with a bit more validation. I will edit the issue description again to describe the exact front end validation that we want.

I think we should check other pages to see if the same problem exists there. I will check other places and update my proposal if I see more instances.

Yes, please let me know if you find other instances.

sanjayharan commented 1 year ago

Thanks for the update @neil-marcellini But i doubt if punit's proposal will handle multiple 0 only string.

neil-marcellini commented 1 year ago

I have updated the description to include the backend validation requirements. It looks like we already do a lot of this on the profile page.

Here's my review of @Puneet-here's proposal.

We don't need to convert the name strings to numbers. We can do values.firstName === '0'. For the DisplayNamePage I like the idea of using assignErrors again to specifically check if the name is not '0'. We need to do the same for the last name. We also need to make sure the first name doesn't contain concierge or expensify. I don't like how we currently return early with a subset of the possible errors here. cc @sketchydroide I don't think this is ideal and I thought you would want to know, or maybe you had a specific reason. https://github.com/Expensify/App/blob/88ccc55778774533a3de738f516e34532656b9c9/src/pages/settings/Profile/DisplayNamePage.js#L83-L85

The form guidelines state that multiple errors should be displayed at once if multiple validations fail. https://github.com/Expensify/App/blob/39a49927fd7172d9a44580d720133b245c7835e1/contributingGuides/FORMS.md#L132-L134


For the WorkspaceSettingsPage we need to add more validation.

Puneet-here commented 1 year ago

Proposal

Add the code below for showing error if it's 0 in firstName

        this.assignError(
            errors,
            'firstName',
            values.firstName === '0',
            Localize.translateLocal(
                'personalDetails.error.hasInvalidCharacter',
                {invalidCharacter: 0},
            ),
        );

For lastName we can do the same as firstName

https://github.com/Expensify/App/blob/b50364bda83f2c6e621ccf1b3b27ffdb2c598249/src/pages/settings/Profile/DisplayNamePage.js#L65-L71

For handling 0 case at workspace -

https://github.com/Expensify/App/blob/b50364bda83f2c6e621ccf1b3b27ffdb2c598249/src/pages/workspace/WorkspaceSettingsPage.js#L64

        if (values.name === '0') {
            errors.name = this.props.translate('personalDetails.error.hasInvalidCharacter',
                {invalidCharacter: 0});
        }

For restricting names like Expensify, concierge we can add individual checks or we can make an array for such names and use _.contains(restrictedNames, values.firstName.toLowerCase()) to show a generic error. I think we should do the same for workspace name because a user can use Expensify as their workspace name and invite other people to the rooms where the user will see expensify as the workspace name

For length check at workspace use

if (values.name.trim().length > 80) {
            errors.name = 'error message needs to be added ';
        }

for the profile we already have char limit https://github.com/Expensify/App/blob/28aab0882f2b96797da0a7db40f227aac2de79f2/src/pages/settings/Profile/DisplayNamePage.js#L89

sanjayharan commented 1 year ago

@Puneet-here @neil-marcellini

Giver the multiple conditions and constraints there will be multiple if blocks will require to validate each one. example: sap-rate for spaces and special characters and maximum length.

and regex can validate it in much more easier way

neil-marcellini commented 1 year ago

Given the multiple conditions and constraints there will be multiple if blocks will require to validate each one.

That's fine. I think it's easier to read and it also allows us to have a specific error message for each case.

neil-marcellini commented 1 year ago

values.firstName is a string that's why we will need to change it to a number

Why does it have to be a number? What's wrong with values.firstName === '0'?

sanjayharan commented 1 year ago

@neil-marcellini Right for specific error message that will work fine. Should i propose for multiple condition with specific error message?

` validate(input_string) { const errors = {};

if (!re.match(regex, input_string)) {
  if (re.match('^0$', input_string)) {
    //input must not be only 0.
    errors.name = this.props.translate('');
  } else if (re.match('^s', input_string)) {
    //input must not contain whitespaces.
    errors.name = this.props.translate('');
  } else if (input_string.length < 1 || input_string.length > 80) {
    //input must be between 1 and 80 characters long.
    errors.name = this.props.translate('');
  } else {
    //input must only contain letters, numbers, and dashes.
    errors.name = this.props.translate('');
  }
  return errors;
}

}`

we can do something like this

Puneet-here commented 1 year ago

Why does it have to be a number? What's wrong with values.firstName === '0'?

Not sure what I was thinking. We can definitely do that.

Puneet-here commented 1 year ago

For restricting names like Expensify, concierge we can add individual checks or we can make an array for such names and use _.contains(restrictedNames, values.firstName.toLowerCase()) to show a generic error. I think we should do the same for workspace name because a user can use Expensify as their workspace name and invite other people to the rooms where the user will see expensify as the workspace name

@neil-marcellini, any thoughts on this?

Puneet-here commented 1 year ago

',' and ';' will be removed

The current Form implementation doesn't allow this but we can make some change in Form and use a new prop to modify the input but I am not sure if we should do that because I saw discussions about always not modifying the user input on expensify

mollfpr commented 1 year ago

@Puneet-here You can take a look on the RoomNameInput. It’s has text modification and used in Form.js.

fedirjh commented 1 year ago

Neither name can be "0"

@neil-marcellini Profile display names can be empty ? or it should be Neither name can be "0" or empty

Puneet-here commented 1 year ago

Will wait for confirmation from @neil-marcellini before updating the proposal for modification

Puneet-here commented 1 year ago

or it should be Neither name can be "0" or empty

@fedirjh we allow user to have no name set so it can be empty

fedirjh commented 1 year ago

I think this still missing some informations , there are many scenarios here

Expected Result:

0 should be considered an invalid profile name and an error should be shown before clicking save

Actual Result:

There is no error and on the profile page the display name appears as the user's email address

Empty name will lead to the same actual results here

lschurr commented 1 year ago

Are we still reviewing proposals on this one @mollfpr?

mollfpr commented 1 year ago

@lschurr Yes! But I see that everyone will update their proposal once @neil-marcellini it's confirmed their concern. I'll hold the review a little bit more.

neil-marcellini commented 1 year ago

@Puneet-here

For restricting names like Expensify, concierge we can add individual checks or we can make an array for such names and use _.contains(restrictedNames, values.firstName.toLowerCase()) to show a generic error.

I would use a regex to match concierge|expensify within the first name and set the result to a restrictedWork variable. If there is a match, use a param in the translate function to output an error message that is something like this ${restrictedWord} is not allowed in your first name.

I think we should do the same for workspace name because a user can use Expensify as their workspace name and invite other people to the rooms where the user will see expensify as the workspace name

You can raise that concern in Slack if you want, but I think we're pretty happy with our workspace naming rules as they are. I will work on getting a copy check for the new error messages we need.

neil-marcellini commented 1 year ago

',' and ';' will be removed

The current Form implementation doesn't allow this but we can make some change in Form and use a new prop to modify the input but I am not sure if we should do that because I saw discussions about always not modifying the user input on expensify

@Puneet-here Yes I suppose I should have clarified. Although the backend removes these characters, you are right that we don't want to modify the user input on the front end. The DisplayNamePage already uses ValidationUtils.findInvalidSymbols to show an error message if a comma or semicolon is included so we are all set for that validation. https://github.com/Expensify/App/blob/f0846a3a8ff138dfdc8f41da4f83a60fce6c683b/src/libs/ValidationUtils.js#L351-L357

neil-marcellini commented 1 year ago

@Puneet-here let's set a max length on these inputs as per the form guidelines and then I think we can remove any error messages related to that. https://github.com/Expensify/App/blob/39a49927fd7172d9a44580d720133b245c7835e1/contributingGuides/FORMS.md#L22-L24

May I please have a copy check on the following new error messages?

Workspace name

When the user inputs 0 as their workspace name => "Invalid workspace name". When the user inputs a workspace name containing spaces" => "Workspace names can not contain spaces".

Display names

When the user inputs 0 as their first name => "Invalid first name". When the user inputs 0 as their last name => "Invalid last name". When 'concierge' or 'expensify' is part of the first name => "concierge is not allowed in your first name". (same for expensify).

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @rosegrech (Waiting for copy), see https://stackoverflow.com/c/expensify/questions/7025/ for more details.

rosegrech commented 1 year ago

for workspace name

If they only enter 0 then the error message suggested should work. However, if it can never contain a 0 or any numbers in the name (which I don't think is the case), we should be more clear.

For the space issue per Grammarly it should be:

Workspace names cannot contain spaces.

rosegrech commented 1 year ago

For Display names for the last ones, just be to to cap C in Concierge and same for Expensify of course :)

neil-marcellini commented 1 year ago

Ok, thanks @rosegrech. Here's the updated copy for the error messages.

Workspace name

When the user inputs 0 as their workspace name => "Invalid workspace name". When the user inputs a workspace name containing spaces" => "Workspace names cannot contain spaces".

Display names

When the user inputs 0 as their first name => "Invalid first name". When the user inputs 0 as their last name => "Invalid last name". When 'concierge' or 'expensify' is part of the first name => "Concierge is not allowed in your first name". (same for Expensify).

Puneet-here commented 1 year ago

If we want to use regex then we can do something like this - Add the code given below to validate method https://github.com/Expensify/App/blob/a10949decb7fd1822a902eb24dfda2f204172894/src/pages/settings/Profile/DisplayNamePage.js#L58

const firstNameRestrictedWord = values.firstName.match(/Expensify|Concierge/gi);
   this.assignError(
            errors,
            'firstName',
            firstNameRestrictedWord,
            Localize.translateLocal(
                'personalDetails.error.hasRestrictedWord',
                {restrictedWord: firstNameRestrictedWord},
            ),
        );

We need to add new key below - https://github.com/Expensify/App/blob/a10949decb7fd1822a902eb24dfda2f204172894/src/languages/en.js#L556

            hasRestrictedWord: ({restrictedWord}) => `${restrictedWord} is not allowed in your first name`,

Remove the code below and the error message copy https://github.com/Expensify/App/blob/a10949decb7fd1822a902eb24dfda2f204172894/src/pages/settings/Profile/DisplayNamePage.js#L87-L94

Use maxLength={CONST.NAME_MAX_LENGTH} at https://github.com/Expensify/App/blob/a10949decb7fd1822a902eb24dfda2f204172894/src/pages/settings/Profile/DisplayNamePage.js#L137

https://github.com/Expensify/App/blob/a10949decb7fd1822a902eb24dfda2f204172894/src/pages/settings/Profile/DisplayNamePage.js#L146

Use maxLength={CONST.WORKSPACE_NAME_MAX_LENGTH} at https://github.com/Expensify/App/blob/a10949decb7fd1822a902eb24dfda2f204172894/src/pages/workspace/WorkspaceSettingsPage.js#L116

We will need to add WORKSPACE_NAME_MAX_LENGTH, NAME_MAX_LENGTH to CONST.js

neil-marcellini commented 1 year ago

Thanks @Puneet-here that looks great. I think you have a solid proposal with the combination of these comments 1, 2.

const firstNameRestrictedWord = values.firstName.match(/Expensify|Concierge/gi);

I have a few minor notes on this. First of all, firstNameRestrictedWord is an array and I don't think we want to pass that directly into the translation method. Originally I was thinking that we would only use the first matched string, but I think it would actually be better to match expensify or concierge, and return both those words if they both exist in the name.

So if I entered this as my name Expensifysdfsdfsconciergesdfsdexpensify then I would see two error messages at the same time, which helps the user fix both mistakes at once. Expensify is not allowed in your first name Concierge is not allowed in your first name

~We can use this regex to match expensify or concierge in the name without duplicates/(Expensify|Concierge)(?!.*\1)/gi. Here is an explanation of the regex. Then we can loop through the matches and assign errors.~

Actually, scratch that haha. That's overly complicated. Let's go back to something close to what you had originally. Check if the lowercase version of the first name contains concierge, and if so assign the error message. We will do another check for expensify. That's much more simple!

If you could please post a combined proposal in another comment that would be helpful for @mollfpr's review and make it easy to link to from a PR. All you @mollfpr, 👍 on his proposal from me.

neil-marcellini commented 1 year ago

@neil-marcellini Profile display names can be empty ? or it should be Neither name can be "0" or empty

@neil-marcellini it looks like you missed my comment . Any update ?

@fedirjh if I didn't specify something as not allowed in the validation instructions, then it is allowed. Yes profile display names can be empty and in that case we default to the user's primary login (email or phone). We don't want that to happen when they enter '0', and more specifically we don't want to allow '0' as a name.

fedirjh commented 1 year ago

Proposal

Full code here #14522

Updated :

  1. Add isValidWorkspaceName function in ValidationUtils
  2. Add assignError to ValidationUtils so we can use it in WorkspaceSettingsPage , we can use it elsewhere
  3. Add isValidDisplayName function in ValidationUtils to validate first an last name
  4. Add hasRestrictedWord function in ValidationUtils to validate restricted words
  5. Add maxLength to Workspace Name input
  6. In CompanyStep add validation for 0 for companyName

In CompanyStep when companyName is set to 0 , we can submit the form and we get this error Auth SetupWithdrawalAccount returned an error

Screenshot 2023-01-25 at 12 01 05 AM Screenshot 2023-01-25 at 12 01 37 AM

isValidWorkspaceName :

We validate each input for all cases we have then we return an array of validation results that will be utilized within assignError function

function isValidWorkspaceName(workspaceName) {
    // Case Empty
    if (!workspaceName || !workspaceName.trim().length) {
        return ['workspace.editor.nameIsRequiredError'];
    }

    // Case Equals to Zero
    if (workspaceName.trim() === '0') {
        return ['workspace.editor.nameInvalidError'];
    }

    // Case contain Spaces
    if (/\s/.test(workspaceName.trim())) {
        return ['workspace.editor.nameContainSpaceError'];
    }
    return [];
}

WorkspaceSettingsPage :

The function isValidWorkspaceName will return an array of validation results. If the array is empty, it indicates that the input is valid. If not, we pass the validation array to the assignError function, which will assign translated errors to each input.

validate(values) {
        const errors = {};

        const workSpaceNameValidation = ValidationUtils.isValidWorkspaceName(values.name);

        ValidationUtils.assignError(
            errors,
            'name',
            !_.isEmpty(workSpaceNameValidation),
            workSpaceNameValidation,
        );

        return errors;
    }

isValidDisplayName :

We validate each input for all cases we have then we return an array of validation results that will be utilized within assignError function

function isValidDisplayName(name, valuesToBeValidated, withRestrictedWords = false) {
    if (!valuesToBeValidated || !valuesToBeValidated.trim().length) {
        // we can display an error if empty
        return [];
    }
    const value = valuesToBeValidated.trim();
    const invalidCharacter = findInvalidSymbols(valuesToBeValidated);
    const restrictedWords = ['Expensify', 'Concierge'];

    // check if name equals to 0
    if (value === '0') {
        return ['personalDetails.error.nameInvalidError', {name}];
    }

    // Check for invalid characters in name
    if (!_.isEmpty(invalidCharacter)) {
        return ['personalDetails.error.hasInvalidCharacter', {invalidCharacter}];
    }

    // we should validate restricted words if withRestrictedWord is set to true
    if (!withRestrictedWords) {
        return [];
    }

    // check if name equals to Expensify or Concierge
    const restrictedWordsErrors = _.compact(
        _.map(
            restrictedWords,
            word => (hasRestrictedWord(value, word) ? ['personalDetails.error.nameRestrictedError', {restrictedWord: word, name}] : null),
        ),
    );

    return _.flatten(restrictedWordsErrors);
}

DisplayNamePage :

The function isValidDisplayName will return an array of validation results. If the array is empty, it indicates that the input is valid. If not, we pass the validation array to the assignError function, which will assign translated errors to each input.

    validate(values) {
        const errors = {};

        const firstNameValidation = ValidationUtils.isValidDisplayName('first name', values.firstName, true);
        const lastNameValidation = ValidationUtils.isValidDisplayName('last name', values.lastName);

        ValidationUtils.assignError(
            errors,
            'firstName',
            !_.isEmpty(firstNameValidation),
            firstNameValidation,
        );

        ValidationUtils.assignError(
            errors,
            'lastName',
            !_.isEmpty(lastNameValidation),
            lastNameValidation,
        );

hasRestrictedWord

Checks if given name includes given restricted Word

function hasRestrictedWord(name, restrictedWord) {
    return new RegExp(restrictedWord, 'i').test(name);
}

assignError :

I have revised the function to incorporate the use of Localize.translateLocal within the function itself, rather than using it in the function call

function assignError(errors, errorKey, hasError, errorCopy) {
    const validateErrors = errors;
    if (hasError) {
        const [phrase, variables] = errorCopy;
        validateErrors[errorKey] = Localize.translateLocal(phrase, variables);
    }
    return validateErrors;
}
mollfpr commented 1 year ago

Thanks @neil-marcellini! I'll start reviewing.

mollfpr commented 1 year ago

@Puneet-here Could you update your proposal and add a diff for the changes? Thanks!

Puneet-here commented 1 year ago

Sorry for the delay I was OOO. I think we can use something like this -

 const firstNameRestrictedWord = values.firstName.match(/Expensify|Concierge/gi);
        this.assignError(
            errors,
            'firstName',
            firstNameRestrictedWord,
            Localize.translateLocal(
                'personalDetails.error.hasRestrictedWord',
                {restrictedWord: _.first(firstNameRestrictedWord)},
            ),
        );

First we will check for Expensify and Concierge in first name if we have them we will get an array back now we will use _.first(firstNameRestrictedWord) to show the first match if the first name contains both restricted words they will see the error for first restricted word and when they remove it they will see the error for the second restricted word. The regex will match both lower and upperclass characters

https://user-images.githubusercontent.com/101883770/214887011-a38334a9-443d-4817-ab85-df41e948f169.mov

I will share the code diff in few mins.

Puneet-here commented 1 year ago

Updated Proposal

Make the suggested changes at DisplayNamePage

--- a/src/pages/settings/Profile/DisplayNamePage.js
+++ b/src/pages/settings/Profile/DisplayNamePage.js
@@ -58,6 +58,8 @@ class DisplayNamePage extends Component {
     validate(values) {
         const errors = {};

+        const firstNameRestrictedWord = values.firstName.match(/Expensify|Concierge/gi);
+
         // Check for invalid characters in first and last name
         const [firstNameInvalidCharacter, lastNameInvalidCharacter] = ValidationUtils.findInvalidSymbols(
             [values.firstName, values.lastName],
@@ -80,19 +83,33 @@ class DisplayNamePage extends Component {
                 {invalidCharacter: lastNameInvalidCharacter},
             ),
         );
-        if (!_.isEmpty(errors)) {
-            return errors;
-        }
-
-        // Check the character limit for first and last name
-        const characterLimitError = Localize.translateLocal('personalDetails.error.characterLimit', {limit: CONST.FORM_CHARACTER_LIMIT});
-        const [hasFirstNameError, hasLastNameError] = ValidationUtils.doesFailCharacterLimitAfterTrim(
-            CONST.FORM_CHARACTER_LIMIT,
-            [values.firstName, values.lastName],
+        this.assignError(
+            errors,
+            'firstName',
+            values.firstName === '0',
+            Localize.translateLocal('personalDetails.error.invalidLastName')
+            ),
+        );
+        this.assignError(
+            errors,
+            'lastName',
+            values.lastName === '0',
+            Localize.translateLocal('personalDetails.error.invalidFirstName'),
+        );
+        this.assignError(
+            errors,
+            'firstName',
+            firstNameRestrictedWord,
+            Localize.translateLocal(
+                'personalDetails.error.hasRestrictedWord',
+                {restrictedWord: _.first(firstNameRestrictedWord)},
+            ),
         );
-        this.assignError(errors, 'firstName', hasFirstNameError, characterLimitError);
-        this.assignError(errors, 'lastName', hasLastNameError, characterLimitError);
-
         return errors;
     }

@@ -137,6 +154,7 @@ class DisplayNamePage extends Component {
                         <TextInput
                             inputID="firstName"
                             name="fname"
+                            maxLength={CONST.NAME_MAX_LENGTH}
                             label={this.props.translate('common.firstName')}
                             defaultValue={lodashGet(currentUserDetails, 'firstName', '')}
                             placeholder={this.props.translate('displayNamePage.john')}
@@ -146,6 +164,7 @@ class DisplayNamePage extends Component {
                         <TextInput
                             inputID="lastName"
                             name="lname"
+                            maxLength={CONST.NAME_MAX_LENGTH}
                             label={this.props.translate('common.lastName')}
                             defaultValue={lodashGet(currentUserDetails, 'lastName', '')}

Make the suggested changes at WorkspaceSettingsPage

--- a/src/pages/workspace/WorkspaceSettingsPage.js
+++ b/src/pages/workspace/WorkspaceSettingsPage.js
@@ -66,6 +66,9 @@ class WorkspaceSettingsPage extends React.Component {
         if (!values.name || !values.name.trim().length) {
             errors.name = this.props.translate('workspace.editor.nameIsRequiredError');
         }
+        if (values.name === '0') {
+            errors.name = this.props.translate('personalDetails.error.invalidName);
+        }
         return errors;
     }

@@ -116,6 +119,7 @@ class WorkspaceSettingsPage extends React.Component {
                         >
                             <TextInput
                                 inputID="name"
+                                maxLength={CONST.WORKSPACE_NAME_MAX_LENGTH}
                                 label={this.props.translate('workspace.editor.nameInputLabel')}
                                 containerStyles={[styles.mt4]}

At en.js (same changes will be applied to es.js) -

--- a/src/languages/en.js
+++ b/src/languages/en.js
@@ -550,10 +550,8 @@ export default {
     },
     personalDetails: {
         error: {
-            firstNameLength: 'First name shouldn\'t be longer than 50 characters',
-            lastNameLength: 'Last name shouldn\'t be longer than 50 characters',
-            characterLimit: ({limit}) => `Exceeds the max length of ${limit} characters`,
             hasInvalidCharacter: ({invalidCharacter}) => `Please remove the ${invalidCharacter} from the name field.`,
+            hasRestrictedWord: ({restrictedWord}) => `${restrictedWord} is not allowed in your first name`,
+            invalidFirstName: 'Invalid first name',
+            invalidLastName: 'Invalid last name',

     resendValidationForm: {
@@ -945,6 +942,7 @@ export default {
             nameInputLabel: 'Name',
             nameInputHelpText: 'This is the name you will see on your workspace.',
             nameIsRequiredError: 'You need to define a name for your workspace',
+            invalidName: 'invalid workspace name',

At CONST.js add -

--- a/src/CONST.js
+++ b/src/CONST.js
@@ -36,6 +36,8 @@ const CONST = {
     // Maximum width and height size in px for a selected image
     AVATAR_MAX_WIDTH_PX: 4096,
     AVATAR_MAX_HEIGHT_PX: 4096,
+    NAME_MAX_LENGTH: 50,
+    WORKSPACE_NAME_MAX_LENGTH: 80,
neil-marcellini commented 1 year ago

First we will check for Expensify and Concierge in first name if we have them we will get an array back now we will use _.first(firstNameRestrictedWord) to show the first match

@Puneet-here I would like to make sure that we show an error message for Concierge and Expensify at the same time if the name contains both. That's why I suggested this https://github.com/Expensify/App/issues/13779#issuecomment-1402370025. Anyways that's a small detail.

Let's go back to something close to what you had originally. Check if the lowercase version of the first name contains concierge, and if so assign the error message. We will do another check for expensify. That's much more simple!


Display names

When the user inputs 0 as their first name => "Invalid first name". When the user inputs 0 as their last name => "Invalid last name".

It looks like the error messages aren't quite right in your proposal. Please update it so that we get the proper error message when a name is 0.