kbecciv commented 10 months 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!

Version Number: 1.4.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 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

Open the app
Navigate to any chat
Tap the "+" button > Assign task
Copy text with a line break and paste into the title field
Complete task creation flow

Expected Result:

Task is created and the title is displayed as one line

Actual Result:

The error message "Auth CreateTask returned an error. Unexpected error create task, please try again later" is shown, and the title is displayed with a line break.

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/7151fd61-2d37-42a2-a623-022e27d4fb08

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~015e745855aa3defdd
Upwork Job ID: 1725145741215453184
Last Price Increase: 2023-11-16
Automatic offers:

tienifr | Contributor | 27817765

melvin-bot[bot] commented 10 months ago

Job added to Upwork: https://www.upwork.com/jobs/~015e745855aa3defdd

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

s-alves10 commented 10 months ago

Proposal

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

When copying multiline text in task title, multiline title is displayed in task report and error is shown

What is the root cause of that problem?

We use copy-paste function of native platforms in TextInput component. When pasting multiline text in single line TextInput on Android, it looks like a single line, but its value has a linebreak. For instance, when pasting 'hello\nworld', it looks like 'hello world' but its real value is 'hello\nworld'. When creating task with multiline title, it looks like the backend returns error

This is the root cause

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

We need to remove all the line breaks from the title Update https://github.com/Expensify/App/blob/6ea4539d15ac523a070532a69ab99cbf1dabba0c/src/pages/tasks/NewTaskDetailsPage.js#L108 to

    onValueChange={(value) => setTaskTitle(value.replace(/\n/g, ' '))}

This works as expected

Note: We need to apply this change to NewTaskTitlePage as well

Result

https://github.com/Expensify/App/assets/126839717/66eaf1ef-91c4-4b97-884f-4fea98e34320

What alternative solutions did you explore? (Optional)

ZhenjaHorbach commented 10 months ago

Proposal

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

Task - Error message displayed when creating task with line break in title

What is the root cause of that problem?

The main problem is that when we save a value, we do not format it in any way Although we expect the title not to be several lines

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

Before saving the title, we can customize it and replace all line breaks with spaces (I like this option, since this option will raise fewer questions about why during insertion, line breaks disappear)

https://github.com/Expensify/App/blob/6ea4539d15ac523a070532a69ab99cbf1dabba0c/src/libs/actions/Task.js#L543-L546

As a result

function setDetailsValue(title, description) {
    // This is only needed for creation of a new task and so it should only be stored locally
    Onyx.merge(ONYXKEYS.TASK, {title: title.replace(/\n/g, " ").trim(), description: description.trim()});
}

What alternative solutions did you explore? (Optional)

NA (edited)

alexismailov2 commented 10 months ago

Looks like Upwork task https://www.upwork.com/jobs/500-Android-Task-Error-message-displayed-when-creating-task-with-line-break-title-31440-Expensify_~015e745855aa3defdd/?referrer_url_path=find_work_home Is not relevant anymore?

melvin-bot[bot] commented 10 months ago

📣 @alexismailov2! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

esh-g commented 10 months ago

Proposal

Please re-state the problem we are trying to solve?

The error message "Auth CreateTask returned an error. Unexpected error create task, please try again later" is shown, and the title is displayed with a line break.

What is the root cause of this problem?

The root cause is that while pasting, the TextInput allows pasting line breaks even though they are not visible in the text. This causes backend to return error since task title should not contain line breaks.

What changes should be made to fix this?

Instead of making changes individually on each occurrence of this bug (which could be in many places) we can solve this in the FormProvider component itself by removing the line breaks if the TextInput is not multiline. This can be done in thw following code here: https://github.com/Expensify/App/blob/1ddd581b88fd82d46e1fed6a27874152a79e3b50/src/components/Form/FormProvider.js#L281-L293 Instead of setting the value directly, we can remove the line breaks if the input is multiline

setInputValues((prevState) => {
    let trimmedValue = value;

    if (!(propsToParse.multiline || propsToParse.autoGrowHeight)) {
        trimmedValue = StringUtils.removeLineBreaks(value);
    }

    const newState = {
        ...prevState,
        [inputKey]: trimmedValue,
    };

    if (shouldValidateOnChange) {
        onValidate(newState);
    }
    return newState;
});

The above function StringUtils.removeLineBreaks function can be defined like this:

function removeLineBreaks(value: string): string {
    return value.replace(/[\r\n]+/g, ' '); // removes typical line break as well as carriage return
}

What alternative did you explore?

We can also use the above method StringUtils.removeLineBreaks function individually in each case of the bug that pops up. (NewTaskTitlePage and TaskTitlePage in this case).

tienifr commented 10 months ago

Proposal

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

The error message "Auth CreateTask returned an error. Unexpected error create task, please try again later" is shown, and the title is displayed with a line break.

What is the root cause of that problem?

This problem only for Android because for Android, if we past a text with line breaks to a non-multiline text input, it will not convert the line break to space. While in all other platforms, it does convert. This is native behavior of Android input.

So when we send a task title with line breaks to the back-end, it will throw error.

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

We need to fix in BaseTextInput onChangeText listener to convert line breaks to space, if the input is non-multiline. We can put in only in the native.js (or even android.js) because this fix is only needed for Android.

This will minimize changes to other platforms and will fix this inconsistency for all inputs in the app.

What alternative solutions did you explore? (Optional)

We can consider submitting an upstream fix for this as well but since this is native behavior of Android input, I doubt this will be accepted.

nixonbaguiluck commented 10 months ago

Proposal

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

The error message "Auth CreateTask returned an error. Unexpected error create task, please try again later" is shown, and the title is displayed with a line break.

What is the root cause of that problem?

The difference in behavior between Android, iOS, and web when copying multi-line strings into a component is due to the inherent platform-specific handling of line breaks.

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

We can fix this issue so briefly if we change only one line as following part.

https://github.com/Expensify/App/blob/3f7e66a8e39b8ecfe4921c74f11f168deafd5d21/src/pages/tasks/NewTaskDetailsPage.js#L111-L111

<View style={styles.mb5}>
    <InputWrapper
        ...
        onValueChange={(value) => setTaskTitle(value.replace(/\r?\n|\r/g, ' '))}
    />
</View>

Result

What alternative solutions did you explore? (Optional)

NA

melvin-bot[bot] commented 10 months ago

📣 @nixonbaguiluck! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

nixonbaguiluck commented 10 months ago

Contributor details Your Expensify account email: nixon.bagui.luck@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0105bc70566336d7a1

melvin-bot[bot] commented 10 months ago

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

sakluger commented 10 months ago

Bumped in Slack https://expensify.slack.com/archives/C01GTK53T8Q/p1700588773073479 for review.

sobitneupane commented 10 months ago

Will review it shortly.

sobitneupane commented 10 months ago

Thanks for the proposal everyone.

@tienifr's proposal looks good to me. The issue exists in multiple flows in the app. So, I believe making the change in BaseTextInput index.native.js is the best way to solve the issue.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

📣 @sobitneupane Please request via NewDot manual requests for the Reviewer role ($500)

melvin-bot[bot] commented 10 months ago

📣 @tienifr 🎉 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 📖

Beamanator commented 10 months ago

not overdue, waiting for PR to be ready

tienifr commented 10 months ago

PR ready for review https://github.com/Expensify/App/pull/31904.

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

https://github.com/Expensify/App/pull/31904

If no regressions arise, payment will be issued on 2023-12-08. :confetti_ball:

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
[x] Contributor that fixed the issue
[ ] Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

@sobitneupane does not require payment (Eligible for Manual Requests)
@tienifr requires payment offer (Contributor)

melvin-bot[bot] commented 10 months ago

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:
[ ] [@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:
[ ] [@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:
[ ] [@sobitneupane] Determine if we should create a regression test for this bug.
[ ] [@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.
[ ] [@sakluger] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

sakluger commented 10 months ago

@sobitneupane mind finishing the BZ checklist so that we can approve your payment tomorrow? Thanks!

sobitneupane commented 10 months ago

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:

I don't think we can point to a particular PR for this bug. This bug exists in all the single line inputs in the app. I would say it was present from the very beginning in the android app.

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

N/A

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

N/A

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

Yes.

[ ] [@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.

https://github.com/Expensify/App/issues/31440#issuecomment-1847042685

sobitneupane commented 10 months ago

Regression Test Proposal:

Go to any single line input in the app.( For example: 'Display Name' in Profile or 'Task Title' in 'Assign Task')
Copy multiline texts and paste in input field.
Submit the form.
Verify that the input is accepted and line breaks in the text in replaced by spaces.

Do we agree 👍 or 👎

sakluger commented 10 months ago

Looks good, thanks!

Summarizing payouts for this issue:

Contributor: @tienifr $500 (paid on Upwork) Contributor+: @sobitneupane $500 (payable via Manual Request)

sakluger commented 10 months ago

@sobitneupane please let me know once you've sent the manual request so I can close the issue. Thanks!

sakluger commented 9 months ago

I'm going to close this one out. @sobitneupane please let me know in Slack if it needs to be reopened for any reason.

sobitneupane commented 9 months ago

https://github.com/Expensify/App/issues/31440#issuecomment-1848003899

Requested payment on newDot.

JmillsExpensify commented 9 months ago

$500 payment approved for @sobitneupane based on this comment.

Expensify / App

[HOLD for payment 2023-12-08] [$500] Android - Task - Error message displayed when creating task with line break in title #31440

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Bug0 Triage Checklist (Main S/O)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

Please re-state the problem we are trying to solve?

What is the root cause of this problem?

What changes should be made to fix this?

What alternative did you explore?

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)