[HOLD for payment 2023-05-03] [$1000] White spaces accepted as Description

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. go to any chat
2. click on + icon and choose sent money or request money
3. enter amount and click on next button
4. click on description
5. enter white space as description and press save button

Expected results:

should not show whitespaces

Actual results:

White space accept as description

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.0
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

Screen.Recording.2023-04-15.at.4.40.55.PM.mov

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

View all open jobs on GitHub

Recording.249.mp4

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0114b38bf7df129f99
• Upwork Job ID: 1649199751054008320
• Last Price Increase: 2023-04-20

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

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

[hasebsiddiqui]

Are you accepting proposals for this?

[allroundexperts]

Proposal

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

White spaces accepted as description text for the IOU action.

What is the root cause of that problem?

The root cause of this issue is that we're not trimming the comment value here.

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

We can trim the value here.

We can also trim the value inside the setMoneyRequestDescription function.

What alternative solutions did you explore? (Optional)

None

[MelvinBot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[akinwale]

Proposal

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

When a user enters only spaces as the description when requesting or sending money, the input is not validated.

What is the root cause of that problem?

Currently, validation is not being performed on the description field. Checking the relevant line of code in MoneyRequestDescriptionPage, we can see that the validate handler is empty. This is based on the assumption that it is supposed to be an optional field. Since there is no validation being performed, any input will be accepted by this field.

App/src/pages/iou/MoneyRequestDescriptionPage.js

Line 72 in f54a8bd

validate={() => ({})}

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

A validate handler needs to be implemented. However, this will not follow the structure of typical validate handlers similar to other classes, because form values which are strings are trimmed by default, so it's impossible to test that it is a string of only whitespaces using the established method.

The MoneyRequestDescriptionPage class needs to be changed as follows:

1. Add a state variable called comment, which will be used to store the input value. this.state = {comment: undefined}
2. Add an onChangeText handler for the TextInput, changeCommentText which will update the state: this.setState({comment: value})
3. Add a validate method.

validate() {
    const errorFields = {};
    if (!_.isUndefined(this.state.comment) && /^\s+$/g.test(this.state.comment)) {
        errorFields.moneyRequestComment = this.props.translate('moneyRequestConfirmationList.error.moneyRequestComment`);
    }

    return errorFields;
}

4. Add the necessary strings for the error message in the translation files.

What alternative solutions did you explore? (Optional)

values.moneyRequestComment will always be a trimmed string when you implement validate using the values parameter, instead of the original string input. This makes it impossible to perform an actual whitespaces-only check on the original string input using this approach.

[johncschuster]

Bug0 Checklist complete. Triaging to Engineering.

[MelvinBot]

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

[alex-mechler]

I'm not sure this is an issue based on our forms guidelines. Started a discussion in slack https://expensify.slack.com/archives/C049HHMV9SM/p1681941815754119?thread_ts=1681557252.708949&cid=C049HHMV9SM

[alex-mechler]

Marking as external https://expensify.slack.com/archives/C049HHMV9SM/p1682034814229019?thread_ts=1681557252.708949&cid=C049HHMV9SM

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~0114b38bf7df129f99

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[ahmedGaber93]

Proposal

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

User can write White spaces as Description.

What is the root cause of that problem?

After I debug the request api?command=RequestMoney, I found the comment value is trimmed in request payload and response, so the value is sent and received from backend correctly.

But we have conflict issue in UX, we use description as optional value but because the form only have this input, we need to prevent user to submit "add description" form if he types empty string or leave it empty and the only option should be available to him is go back.

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

We need to add validation for description value

validate(values) {
    const errors = {};
    if (!_.isString(values.moneyRequestComment) || _.isEmpty(values.moneyRequestComment.trim())) {
        errors.moneyRequestComment = 'we will add custom error here';
    }
    return errors;
}

<Form
    ....
     validate={this.validate} />

What alternative solutions did you explore? (Optional)

keep the value optional and without validation and use confirmation modal to confirm the user after click save is he want to leave without comment?

[hoangzinh]

Proposal

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

White spaces accepted as description

What is the root cause of that problem?

Currently, when user submits description form, we store whatever user input into the Onyx in this line

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

We can trim the spaces from user input before store in Onyx, in this line

        IOU.setMoneyRequestDescription(value.moneyRequestComment.trim());

[hungvu193]

@Santhosh-Sellavel Similar issue was External before this issue here
#17663

[Santhosh-Sellavel]

Yeah, I see we go by the earlier reported issue first. I see your proposal got accepted there. But it is pretty much the dupe of @allroundexperts's proposal, So I think this should be awarded to @allroundexperts anyways wherever we handle this one. Since it got closed we already have a proposal from @allroundexperts here we are good to proceed here, thanks!

@alex-mechler we are good to go with @allroundexperts here, thanks!

C+ Reviewed
🎀 👀 🎀

[gadhiyamanan]

@johncschuster accepted, Thanks!

[Santhosh-Sellavel]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@Santhosh-Sellavel] The PR that introduced the bug has been identified. Link to the PR: Add Push-to-Page support for IOUConfirmationList #16919
• [@Santhosh-Sellavel] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: Add Push-to-Page support for IOUConfirmationList #16919 (comment)
• [@Santhosh-Sellavel] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A nothing to update
• [@Santhosh-Sellavel] Determine if we should create a regression test for this bug.
• [@Santhosh-Sellavel] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again. [HOLD for payment 2023-05-03] [$1000] White spaces accepted as Description #17485 (comment)
• [@johncschuster] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/283356

@alex-mechler If you differ in any of the above let me know, thanks!

[johncschuster]

@gadhiyamanan I've just paid out the reporting bonus! Thanks for the report!

@Santhosh-Sellavel, can you apply when you get a moment? I don't see your application in Upwork.

[johncschuster]

Oh geez, I was holding it wrong. Thanks for pointing that out, @Santhosh-Sellavel! I've sent you the offer!

[Santhosh-Sellavel]

Regression Test Steps

Steps from the PR Look good to me.

1. Go to any chat
2. Click on the + icon and choose Send Money or Request Money
3. Enter the amount and click on the Next button
4. Click on the description
5. enter white space as the description and press the save button
6. Open the description field again and verify that whitespaces do not appear in the saved description

👍 or 👎

cc: @alex-mechler @johncschuster

[alex-mechler]

Maybe we should have the description have both leading and trailing spaces, and verify they are trimmed? Other than that 👍

[alex-mechler]

@johncschuster Bump on completing your step in the BZ checklist / making sure this gets paid out.

[johncschuster]

Done!

[Santhosh-Sellavel]

@johncschuster bump this is not paid yet

[johncschuster]

I’m going to be flying to EC3 today and part of tomorrow. Reassigning so the payment can get taken care of.

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

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

[isabelastisser]

The payments were made in Upwork now; we are all set!

[johncschuster]

Just an FYI, the QA steps were not added, as this behavior was considered an edge case.

Please feel free to reopen the linked issue above if you disagree!