[HOLD for payment 2023-06-02] [$1000] white space accepted as description in Assign task

[kavimuru]

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

---

1. Action Performed:

2. Click on FAB button -> Click on Assign task option
3. Enter description as white space
4. click on Next button

Expected Result:

description should be trimmed

Actual Result:

description not trimmed

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

Simulator.Screen.Recording.-.iPhone.13.-.2023-05-10.at.20.13.42.mp4

Recording.557.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011652c334684162d9
• Upwork Job ID: 1659614646252167168
• Last Price Increase: 2023-05-19

[melvin-bot]

Triggered auto assignment to @strepanier03 (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

[kavimuru]

Proposal

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

leading and trailing space accepted as description and title in Assign task

What is the root cause of that problem?

we're not trimming the title and description

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

We can trim() the title and description at the below pages

App/src/pages/tasks/NewTaskDetailsPage.js

Line 51 in 46390d6

TaskUtils.setDetailsValue(values.taskTitle, values.taskDescription);

App/src/pages/tasks/NewTaskDescriptionPage.js

Line 50 in 46390d6

TaskUtils.setDescriptionValue(values.taskDescription);

App/src/pages/tasks/NewTaskTitlePage.js

Line 58 in 46390d6

TaskUtils.setTitleValue(values.taskTitle);

What alternative solutions did you explore? (Optional)

None

[allroundexperts]

I think it was decided that we shouldn't fiddle with the user input in this issue cc @pecanoro

[gadhiyamanan]

we have similar issues which we have solved
#17485
#17770
#14287
#18006

[pecanoro]

I opened a discussion internally to see what we should do in these cases.

[allroundexperts]

Thanks @pecanoro!

[strepanier03]

I'm following along with the discussion. Also, I don't have the task option in my FAB, so I'll look into that for proper testing on my end.

[strepanier03]

Bumped the discussion in Slack.

[strepanier03]

We haven't reached a decision yet but I bumped the thread so we can move forward.

[strepanier03]

The discussion was opened up to a wider audience today. Post in #expensify-open-source here.

If no pushback by tomorrow I'll move this forward with trimming as the goal.

[strepanier03]

Conclusion of discussion:

We have decided to take an always trim stance moving forward so we are free to move this to external to work on trimming the white space.

Note: We don't want to trim white space between individual words of course.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~011652c334684162d9

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[gadhiyamanan]

Proposal

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

leading and trailing spaces space accepted as description and title in Assign task

What is the root cause of that problem?

we're not trimming the title and description

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

We can trim() the title and description at the below pages

App/src/pages/tasks/NewTaskDetailsPage.js

Line 51 in 46390d6

TaskUtils.setDetailsValue(values.taskTitle, values.taskDescription);

App/src/pages/tasks/NewTaskDescriptionPage.js

Line 52 in 7378626

TaskUtils.setDescriptionValue(values.taskDescription);

App/src/pages/tasks/NewTaskTitlePage.js

Line 60 in 7378626

TaskUtils.setTitleValue(values.taskTitle);

App/src/pages/tasks/TaskTitlePage.js

Line 63 in 7378626

TaskUtils.editTaskAndNavigate(props.task.report, props.session.email, values.title, '', '');

App/src/pages/tasks/TaskDescriptionPage.js

Line 62 in 7378626

TaskUtils.editTaskAndNavigate(props.task.report, props.session.email, '', values.description, '');

What alternative solutions did you explore? (Optional)

None

[BhuvaneshPatil]

Proposal

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

While Assigning a task, in the first section of modal that opens from right. Description value that is saved is saved with trialing and leading spaces.

What is the root cause of that problem?

The root cause of problem is we are not cleaning (trimming) the value before we set that value in state.

App/src/pages/tasks/NewTaskDetailsPage.js

Lines 50 to 53 in 01875f4

	function onSubmit(values) {
	TaskUtils.setDetailsValue(values.taskTitle, values.taskDescription);
	Navigation.navigate(ROUTES.NEW_TASK);
	}

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

The solution to this problem is trimming the value before actually saving it into state (.setDetailsValue) on - To implement this we can use trim() method from javascript.

App/src/pages/tasks/NewTaskDetailsPage.js

Lines 50 to 51 in 01875f4

	function onSubmit(values) {
	TaskUtils.setDetailsValue(values.taskTitle, values.taskDescription);

What alternative solutions did you explore? (Optional)

There are many places where we are taking input for description in Assign Task. We need to change them as well.
Good approaches are

• To create a utility function for such task and use it everywhere.
• Create a function in TaskUtils that will be called whenever we are saving the description.

[gadhiyamanan]

Add defaultValue={props.task.description} to description field

App/src/pages/tasks/NewTaskDetailsPage.js

Lines 86 to 89 in 46390d6

	<TextInput
	inputID="taskDescription"
	label={props.translate('newTaskPage.descriptionOptional')}
	/>

Add also add defaultProps and propTypes

    task: {
        description: '',
    },

    task: PropTypes.shape({
        /** Description of the Task */
        description: PropTypes.string,
    }),

OR

we can add defaultValue="" . we are using defaultValue="" in EmojiPickerMenu because we are trying to trim text in filterEmojis

App/src/components/EmojiPicker/EmojiPickerMenu/index.js

Lines 487 to 496 in d7cbeca

	<TextInput
	label={this.props.translate('common.search')}
	onChangeText={this.filterEmojis}
	defaultValue=""
	ref={(el) => (this.searchInput = el)}
	autoFocus
	selectTextOnFocus={this.state.selectTextOnFocus}
	onSelectionChange={this.onSelectionChange}
	onFocus={() => this.setState({isFocused: true, highlightedIndex: -1, isUsingKeyboardMovement: false})}
	onBlur={() => this.setState({isFocused: false})}

App/src/components/EmojiPicker/EmojiPickerMenu/index.js

Lines 383 to 384 in d7cbeca

	filterEmojis(searchTerm) {
	const normalizedSearchTerm = searchTerm.toLowerCase().trim().replaceAll(':', '');

cc: @sobitneupane

[iwiznia]

We can trim() the title and description at the below pages

@sobitneupane, @gadhiyamanan maybe it would be better to call trim inside all those methods instead of on the caller?

[gadhiyamanan]

maybe it would be better to call trim inside all those methods instead of on the caller?

In that case, We can add trim() to below lines

App/src/libs/actions/Task.js

Line 289 in a891b6c

const reportName = title || report.reportName;

App/src/libs/actions/Task.js

Line 365 in a891b6c

description: description || report.description,

App/src/libs/actions/Task.js

Line 393 in a891b6c

Onyx.merge(ONYXKEYS.TASK, {title, description});

App/src/libs/actions/Task.js

Line 401 in a891b6c

Onyx.merge(ONYXKEYS.TASK, {title});

App/src/libs/actions/Task.js

Line 409 in a891b6c

Onyx.merge(ONYXKEYS.TASK, {description});

[iwiznia]

Cool, sounds good.

[melvin-bot]

📣 @gadhiyamanan You have been assigned to this job by @iwiznia!
Please apply to this job in Upwork 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 📖

[gadhiyamanan]

@sobitneupane PR is ready for review

[melvin-bot]

🎯 ⚡️ Woah @sobitneupane / @gadhiyamanan, great job pushing this forwards! ⚡️

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

• when @gadhiyamanan got assigned: 2023-05-22 16:59:37 Z
• when the PR got merged: 2023-05-24 16:43:38 UTC

On to the next one 🚀

[strepanier03]

PR is merged and awaiting deploy. All good here for now.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.18-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• trim task title and description #19401

If no regressions arise, payment will be issued on 2023-06-02. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter - @gadhiyamanan - $250
• Contributor that fixed the issue - @gadhiyamanan - $1000 + $500 (speed bonus) = $1500
• Contributor+ that helped on the issue and/or PR - @sobitneupane - $1000 + $500 (speed bonus) = $1500

Speed bonus assessment: Contributor hired on May 22 / PR merged on May 24 = 2 days Eligible for bonus

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

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:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR: Create Task FrontEnd Changes #17992
• [@sobitneupane] 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: Create Task FrontEnd Changes #17992 (comment)
• [@sobitneupane] 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: https://expensify.slack.com/archives/C049HHMV9SM/p1685954728540999
• [@sobitneupane] Determine if we should create a regression test for this bug. - I don't think separate regression test will be required. QA should make sure that white space are not accepted in any text input.
• [@sobitneupane] 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.
• [@strepanier03] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[strepanier03]

Prepping for hold removal tomorrow.

---

@gadhiyamanan - I have hired you for the job in Upwork. I will pay the reporting bonus for this job in the one I hired you for so we don't have to make a second job, if that's okay. The total with speed bonus will be $1750.

@sobitneupane - I've hired you as well. Feel free to complete the checklist and I'll update it and create the reg test GH if needed, then do the pay out.

Back tomorrow 👋

[sobitneupane]

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:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#17992

• [@sobitneupane] 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:

#17992 (comment)

• [@sobitneupane] 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:

https://expensify.slack.com/archives/C049HHMV9SM/p1685954728540999

• [@sobitneupane] Determine if we should create a regression test for this bug.

I don't think separate regression test will be required. QA should make sure that white space are not accepted in any text input.

[strepanier03]

Awesome, thank you for that @sobitneupane.

I have paid everyone and finished off the checklist. Closing this out now.

Fantastic job everyone!