[$1000] Web - Wrong error messages on debit card page for different cases

[kbecciv]

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. Go to settings > payments > debit card
2. Leave a field empty and try to submit

Note: This behavior seems to persist across the following pages: connect bank > step 1 (bank account) connect bank > step 2 (company info) connect bank > step 3 (personal info) paypal.me page close account page date of birth page

Expected Result:

For empty fields, error should say "this field is required",

Actual Result:

For empty fields too, it basically says wrong input

Workaround:

Unknown

Platforms:

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

• [ ] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [x] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: 1.3.47.3 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/19068b20-03ef-4b1b-abb3-e9d61b8f51b2

Expensify/Expensify Issue URL: Issue reported by: @chiragxarora Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690561860353119

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e4105fa01a84e1da
• Upwork Job ID: 1688661504121946112
• Last Price Increase: 2023-08-14
• Automatic offers:
• Ollyws | Reviewer | 26140675
• chiragxarora | Contributor | 26140678
• chiragxarora | Reporter | 26140679

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [x] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [x] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[kbecciv]

Proposal by: @chiragxarora Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690561860353119

Proposal

Please re-state the problem that we are trying to solve in this issue.

wrong error messages on debit card page for different cases

What is the root cause of that problem?

Root cause of the problem is the wrong logic in validation function here

https://github.com/Expensify/App/blob/1e84793f1cab08f39bc708e26bb2cadd2b13752e/src/pages/settings/Payments/AddDebitCardPage.js#L62-L98

Notice each if block is having an OR condition with common error, here if the field is empty or its wrong, in both the cases we are displaying the same error for wrong field, this is the RCA

What changes do you think we should make in order to solve the problem?

We need to handle empty case and wrong case separately and display separate error for a particular field

something like this

if (!values.nameOnCard || !ValidationUtils.isValidLegalName(values.nameOnCard)) {
    if (!values.nameOnCard) {
        errors.nameOnCard = 'common.error.fieldRequired'
    }
    else errors.nameOnCard = 'addDebitCardPage.error.invalidName';
}

or

if (_.isEmpty(values.nameOnCard)) {
   errors.nameOnCard = 'common.error.fieldRequired';
} else if (!ValidationUtils.isValidLegalName(values.nameOnCard)) {
   errors.nameOnCard = 'addDebitCardPage.error.invalidName';
}

code can be beautifed but this is the main idea, displaying different error for empty state and wrong state

Results

What alternative solutions did you explore? (Optional)

None

[chiragxarora]

Will find and update other similar inputs later!

[jfquevedol2198]

Updated Proposal

Please re-state the problem that we are trying to solve in this issue.

wrong error messages on debit card page for different cases

What is the root cause of that problem?

There is no check for required fields on Form component as well as all pages that use Form component

What changes do you think we should make in order to solve the problem?

We can add validation to check required fields on all pages or Form component, but I think its better to add on Form component only and add required props in components with required field. Update Form component

1. Add requiredFieldRefs in Form component like inputRefs

   const requiredRefs = useRef({});

2. Update this field

   ref: (node) => {
   inputRefs.current[inputID] = node;
   requiredRefs.current[inputID] = child.props.required;
   const {ref} = child;
   if (_.isFunction(ref)) {
       ref(node);
   }
   },

3. Update onValidate function

   if ((!inputValue || !_.isString(inputValue) || inputValue.search(CONST.VALIDATE_FOR_HTML_TAG_REGEX) === -1)) {
   if (requiredRefs.current[inputID]) {
       validationErrors[inputID] = 'common.error.fieldRequired';
   }
   return;
   }

Finally, we will need only to add required props in required fields for all pages. Nothing changes are needed in every pages.

Changes on AddDebitCard page

<TextInput
    inputID="nameOnCard"
    ...
    required
/>
<TextInput
    inputID="cardNumber"
    ...
    required
/>
...
        <TextInput
            inputID="expirationDate"
            ...
            required
        />
...
        <TextInput
            inputID="securityCode"
            ...
            required
        />
...
    <AddressSearch
        inputID="addressStreet"
        ...
        required
    />
...
<TextInput
    inputID="addressZipCode"
    ...
    required
/>
...
    <StatePicker inputID="addressState" required/>
...
<CheckboxWithLabel
    inputID="acceptTerms"
    ...
    required
/>

Update other pages (Add Paypal Me page, Close account page, Add Bank detail page, Date of Birth page) like this.

Result

Add Debit Card Page

Add Paypal Me Page

Date of Birth Page

Close Account Page

What alternative solutions did you explore? (Optional)

N/A

[johncschuster]

Let's hold on proposals until @chiragxarora updates the issue to include other inputs.

[chiragxarora]

Proposal

Second proposal

Please re-state the problem that we are trying to solve in this issue.

wrong error messages on debit card page for different cases

What is the root cause of that problem?

Root cause of the problem is the wrong logic in validation function here

https://github.com/Expensify/App/blob/1e84793f1cab08f39bc708e26bb2cadd2b13752e/src/pages/settings/Payments/AddDebitCardPage.js#L62-L98

Notice each if block is having an OR condition with common error, here if the field is empty or its wrong, in both the cases we are displaying the same error for wrong field, this is the RCA

What changes do you think we should make in order to solve the problem?

There's another way to handle this, we can update the validate function in the debit card page to pick all the required fields using the inputID and run the isRequiredFulfilled() method on them to check if they are blank or not. This method comes handy since its written for the very same purpose

const requiredFields = ['nameOnCard', 'cardNumber', 'expirationDate', 'securityCode']; // can add more fields

_.each(requiredFields, (fieldKey) => {
    if (ValidationUtils.isRequiredFulfilled(values[fieldKey])) {
        return;
    }
    errors[fieldKey] = 'common.error.fieldRequired';
});

// the above will handle required field error

// for the wrong validation error, we need to check if the field contains some value but it is wrong

if (values.nameOnCard && !ValidationUtils.isValidLegalName(values.nameOnCard)) { // removed the NOT ! operator
    errors.nameOnCard = 'addDebitCardPage.error.invalidName';
}
..........

To make the codebase DRY friendly, we could create a function like this for setting required field errors and use that utility method

function getRequiredErrors(values, requiredFields) {
    const errors = {};
    _.each(requiredFields, (fieldKey) => {
        if (ValidationUtils.isRequiredFulfilled(values[fieldKey])) {
            return;
        }
        errors[fieldKey] = 'common.error.fieldRequired';
    });
    return errors;
}

const validate = (values) => {
        const requiredFields = ['nameOnCard', 'cardNumber', 'expirationDate', 'securityCode', 'addressStreet', 'addressZipCode'];

        // setting required field errors

        const errors = ErrorUtils.getRequiredErrors(values, requiredFields);

        // setting custom errors for fields with custom validators

        if (values.nameOnCard && !ValidationUtils.isValidLegalName(values.nameOnCard)) {
            errors.nameOnCard = 'addDebitCardPage.error.invalidName';
        }
        ............
        return errors;

NOTE: similar logic is used on all other pages too like where we have multiple error messages

Results

What alternative solutions did you explore? (Optional)

None

[chiragxarora]

@johncschuster Here are the additional pages that I could find:

• connect bank > step 1 (bank account)
• connect bank > step 2 (company info)
• connect bank > step 3 (personal info)
• paypal.me page
• close account page
• date of birth page

[johncschuster]

Thanks, @chiragxarora! I've updated the OP to include that as a note.

@jfquevedol2198, please take another look and update your proposal to account for these other pages.

[jfquevedol2198]

@johncschuster Here is my Updated Proposal There are some other pages else these pages that uses Form component. Here is the list of those pages:

• EditRequestDescriptionPage
• ReportWelcomeMessagePage
• AdditionalDetailsStep
• MoneyRequestDescriptionPage
• ACHContractStep
• CompanyStep
• RequestorStep
• ValidationStep
• DisplayNamePage
• NewContactMethodPage
• AddressPage
• NewTaskDetailsPage
• NewTaskTitlePage
• TaskTitlePage
• WorkspaceInviteMessagePage
• WorkspaceNewRoomPage
• WorkspaceSettingsPage
• WorkspaceRateAndUnitPage

I think we can simply fix those pages by my solution. Thanks

[melvin-bot[bot]]

@johncschuster Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Current assignee @johncschuster is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

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

[johncschuster]

Not overdue yet, Melvin.

[Ollyws]

Ok so I compiled a list below of form validation behaviours.

Around half have some kind of field required message but alot of them are non-standard.

A general solution like @jfquevedol2198's would be ideal if the field required behaviour was uniform between Form pages, but implementing a general solution to only be used in a minority of pages kind of defeats the object.

On the other hand we can implement these checks for every page where they're missing as @chiragxarora suggested, but this isn't very DRY, assuming we're using the same common.error.fieldRequired in all of them.

I will have to think this over, any further input/suggestions welcome.

AdditionalDetailsStep - Checks if _.isEmpty, no field required message. BankAccountManualStep - Checks if value exists, no field required message. CompanyStep - Checks if value exists, no field required message. AddDebitCardPage - Checks if value exists, no field required message. RequestorStep - Checks using isRequiredFulfilled, no field required message. ValidationStep - Checks using isRequiredFulfilled, no field required message. AddPayPalMePage - isValidPaypalUsername, no fieldRequired message. ACHContractStep - Required fields are defined in an array, no field required message. AddressPage - Required fields are defined in an array, uses common.error.fieldRequired. DateOfBirthPage - Checks if empty, uses common.error.fieldRequired. LegalNamePage - Checks if _.isEmpty, uses common.error.fieldRequired. NewContactMethodPage - Checks if _.isEmpty, custom contactMethodRequired message. CloseAccountPage - Checks if _.isEmpty, custom enterYourDefaultContactMethod message. TaskTitlePage - Checks if _.isEmpty, custom pleaseEnterTaskName message. RoomNamePage - Checks if value exists, custom pleaseEnterRoomName message. NewTaskDetailsPage - Checks if value exists, custom pleaseEnterTaskName message. NewTaskTitlePage - Checks if value exists, custom pleaseEnterTaskName message. WorkspaceNewRoomPage - Checks if value exists, custom pleaseEnterRoomName message. WorkspaceSettingsPage - Checks if value exists, custom nameIsRequiredError message. WorkspaceRateAndUnitPage - Checks using regex, only invalidRateError message.

[chiragxarora]

@Ollyws did you see my second proposal https://github.com/Expensify/App/issues/23954#issuecomment-1663612884 ? It is what we are using in the app as common practice

I think this kind of validate function is something we can generalize on since we already have the methods related to this functionality for the same use

const validate = (values) => {
        const errors = {};
        const requiredFields = ['field1', 'field2',.....]; // can add more fields

        // setting field Required errors
        _.each(requiredFields, (fieldKey) => {
            if (ValidationUtils.isRequiredFulfilled(values[fieldKey])) {
                return;
            }
            errors[fieldKey] = 'common.error.fieldRequired';
        });

        // setting custom validation errors for fields which have custom validators

        if (values.field1 || !ValidationUtils.isValidField(values.field1)) {
            errors.nameOnCard = 'addDebitCardPage.error.invalidName';
        }
        return errors;
}

creating a required prop won't work well as suggested in above proposal for the same reasons you mentioned, for some forms we have different error messages and for many forms we already have required errors set via 2 approaches:

• if/else
• the one in my second proposal So basically we'll have to rewrite validate() for all the forms in app and selectively use that prop even for the fields which are required which makes the solution case based rather than general

[jfquevedol2198]

Yes @Ollyws we can only add required field for the actual required fields on those pages. It won't be affected to pages that doesn't need required field.

[Ollyws]

@chiragxarora Yes I did see that, and agree we do use that approach in a few of the form pages. My only concern is code re-use, perhaps we could create a function to keep things DRY.

[chiragxarora]

yes, we could create a utility and pass required fields to that method I suppose

[chiragxarora]

I've updated my proposal https://github.com/Expensify/App/issues/23954#issuecomment-1663612884 @Ollyws

[jfquevedol2198]

Updated Proposal

Please re-state the problem that we are trying to solve in this issue.

Web - Wrong error messages on debit card page for different cases

What is the root cause of that problem?

Some pages are not checking the required fields

What changes do you think we should make in order to solve the problem?

We can create a utility function for general checking of required fields by passing inputIDs of required fields Add this new function in ValidationUtils:

    const checkRequiredFields = (values, requiredFields) => {
        let errors = {};
        _.each(requiredFields, ({inputID: fieldKey, validFunc}) => {
            if (!ValidationUtils.isRequiredFulfilled(values[fieldKey])) {
                errors[fieldKey] = 'common.error.fieldRequired';
            }
            if (validFunc && !validFunc(values[fieldKey])) {
                errors[fieldKey] = 'common.error.fieldRequired';
            }
        });
        return errors;
    }

Example of usage: Example 1 (AddressPage): https://github.com/Expensify/App/blob/c70a61954b0e5a7be1ea1fcd5e625a6108fb86cb/src/pages/settings/Profile/PersonalDetails/AddressPage.js#L84-L98 Update this:

        const requiredFields = [
            {inputID: 'addressLine1'},
            {inputID: 'city'}, 
            {inputID: 'country'},
            {inputID: 'state'}
        ];
        const errors = ValidationUtils.checkRequiredFields(values, requiredFields);

We can keep complex validation lines like this and like this and remove other lines

Example 2 (DateOfBirthPage): https://github.com/Expensify/App/blob/c70a61954b0e5a7be1ea1fcd5e625a6108fb86cb/src/pages/settings/Profile/PersonalDetails/DateOfBirthPage.js#L47-L55 Update this:

        const requiredFields = [{inputID: 'dob', validFunc: ValidationUtils.isValidDate}];
        const errors = ValidationUtils.checkRequiredFields(values, requiredFields);

We can pass extra validation like ValidationUtils.isValidDate(values.dob)) in validFunc

...

What alternative solutions did you explore? (Optional)

[Ollyws]

@chiragxarora Your updated proposal looks good to me, but let's decide exactly which pages need a fieldRequired error message. As far as I can see:

AdditionalDetailsStep, AddDebitCardPage, BankAccountManualStep, CompanyStep, RequestorStep and AddPayPalMePage

Let me know if you see any others.

@jfquevedol2198 Thanks for the update but your function seems unnecessarily complex for this use case, better to keep things simple here.

[chiragxarora]

https://github.com/Expensify/App/issues/23954#issuecomment-1664601536

These pages need the required error, date of birth page is setting the required error but its getting overwritten, need to adjust the if/else statements there

@Ollyws

[jfquevedol2198]

@Ollyws @johncschuster I think my previous proposal is better to simplify other pages which have required fields. If there is no required field, we can keep it without any change, and if there are some required fields, we can validate by adding required prop simply without adding more codes.

[chiragxarora]

I disagree with this, problems with that proposal:

• adds the required prop but fails to use it correctly (not all required fields have same error message so we can't use this prop on all required fields of the app) It will basically be a workaround solution applied selectively to a small subset of forms in app and not globally
• adding required to form component basically tells that it is to be used on all required input fields
• breaks the code design: value checks are to be performed in the page's validate() method itself and not in the form component. This is why we have ValidationUtils which we use in page's validate method, and we have isRequiredFulfilled() method defined in it for the same reason.
• Now Form component's validate() is a post validation which runs after page's validate() just for data sanitization and scanning for HTML code inside input fields for backend feasibility, so checking for blank values for required error inside it will break the code flow and it shows workaround practice since that logic doesn't belong there (reason why we don't have validation util used anywhere in the form component).
• we'll need to update all the validate methods for every form in app which already have required errors set correctly (basically every form except the few which we are handling here), since those will add second duplicate error key in the error object.
• very prone to regressions

cc @Ollyws

[Ollyws]

@chiragxarora I think your comment may be missing one or two I mentioned. Also not sure if close account page is required as the Please enter your default contact method to close your account. is already very explicit, but we can refine these details in the PR.

So let's go with @chiragxarora's proposal, validating which fields show the fieldRequired error message in the validate function, on a per-page basis and creating a util function to keep things DRY.

🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

Triggered auto assignment to @bondydaa, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[chiragxarora]

Notes for PR:

• error message for state picker? (debit card page)
• error message for state picker? (company step page)
• close account page??

[chiragxarora]

@Ollyws where is AdditionalDetailsStep page present and how do I get to that in the app? I tried but no success! Pls help

[Ollyws]

@chiragxarora https://staging.new.expensify.com/enable-payments

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[chiragxarora]

Totally forgot to ping @bondydaa for assignment 😅 PR is close to ready, but still in draft state since I'm not yet assigned

[melvin-bot[bot]]

📣 @Ollyws 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @chiragxarora 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot[bot]]

📣 @chiragxarora 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[chiragxarora]

Thankyou @johncschuster for assigning

If you think your compensation should be increased for a specific job, you can request a reevaluation by commenting in the Github issue where the Upwork job was posted.

Also, I had a request for this to be re-evaluated for compensation because it was quite time-consuming and involved a lot of checks & changes. Let me know your thoughts @bondydaa @johncschuster

Totally fine if it doesn't qualify for that, its just a request for re-evaluation 😅

[bondydaa]

whoop sorry about missing this

[melvin-bot[bot]]

🎯 ⚡️ Woah @Ollyws / @chiragxarora, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

• when @chiragxarora got assigned: 2023-08-15 20:23:03 Z
• when the PR got merged: 2023-08-18 17:42:00 UTC

On to the next one 🚀

[chiragxarora]

Thankyou @johncschuster for assigning

If you think your compensation should be increased for a specific job, you can request a reevaluation by commenting in the Github issue where the Upwork job was posted.

Also, I had a request for this to be re-evaluated for compensation because it was quite time-consuming and involved a lot of checks & changes. Let me know your thoughts @bondydaa @johncschuster

Totally fine if it doesn't qualify for that, its just a request for re-evaluation 😅

@johncschuster @bondydaa

[Ollyws]

For some reason Melvin hasn't added the [HOLD for payment] title, but the PR was deployed to production on the 24th and by my calculations should be due for payment on the 31st.

[chiragxarora]

Bump @johncschuster

[johncschuster]

Hey everyone! Sorry I missed the bumps here! Let me catch up and get this sorted.

[chiragxarora]

Thankyou

[johncschuster]

Thanks for your patience, everyone! The payment amount was not increased, but given the urgency to complete the issue, I have issued the bonus.

[chiragxarora]

@johncschuster any reason why the request was rejected? It was a pretty long PR involving tons of testing at like a dozen of pages