[HOLD for payment 2024-08-07] [$250] Desktop - Task - Error occurs when update long content for task description for second time

[lanitochka17]

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.76-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: https://expensify.testrail.io/index.php?/runs/view/21450&group_by=cases:section_id&group_order=asc&group_id=291935
Email or phone of affected tester (no customers): crowdtest.expensify+admin@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

1. Open the app
2. Start group chat with another user
3. Create a task for this user without a description
4. Update description with long content + emoji (Can use in attachment)
5. Observe the error message of the app show
6. Update this content to description one more time

Expected Result:

The error of the app shows, warning the user because the content too long

Actual Result:

The error occurs when updating long content for the task description for a second time, after that user can't update the description or task name

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6493142_1716830309579.Screen_Recording_2024-05-27_at_23.57.54.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011028270f108dc08b
• Upwork Job ID: 1795195550390841344
• Last Price Increase: 2024-06-03
• Automatic offers:
• abdulrahuman5196 | Reviewer | 102597496
• samilabud | Contributor | 102597500

Issue Owner

Current Issue Owner: @carlosmiceli / @adelekennedy

[melvin-bot]

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@bfitzexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #vip-vsp

[neonbhai]

Proposal

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

Task - Error occurs when update long content for task description for second time

What is the root cause of that problem?

We don't remove the optimistic message if an error occurs, this interfers with get older report action calls since the current report action does not exist in server.

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

We should update failureData here to set the optimistic message's reportActionID to null

App/src/libs/actions/Task.ts

Line 489 in c22945a

value: {[editTaskReportAction.reportActionID]: {pendingAction: null}},

value: {[editTaskReportAction.reportActionID]: null},

Result

Before

Screen.Recording.2024-05-28.at.2.02.36.AM.mov

After

Screen.Recording.2024-05-28.at.1.44.43.AM.mov

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~011028270f108dc08b

[melvin-bot]

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

[samilabud]

Proposal

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

Edited Task - Error occurs when description content exceed the 500 characters after parsed as html.

What is the root cause of that problem?

This error occurs when description content exceed the 500 characters after parsed as html in getParsedComment(), when we parse the description to HTML format using getParsedComment(). This happens because we validate the limit of 500 characters when it is single text strings, but we send it to the server after parsed, we notice the error when we are near the limit of 500 and the getParsedComment() returns converted the lines breaks to <br />, because this increase the text 5 characters for every line break.

Technical explanation

In other words we validate the text has a max length of 500 characters, ie, the next text has 493 characters:

Lorem ipsum dolor sit amet, consectetur adipiscing elit. Praesent placerat consectetur enim sit amet sodales. Etiam elementum, lorem id sollicitudin dignissim, quam nibh sodales arcu, imperdiet consequat risus mi sit amet lorem. Donec iaculis at tortor eget vulputate. Duis dignissim non diam id mattis. Suspendisse sit amet consequat elit, feugiat mattis libero. In hendrerit dui ac ante hendrerit condimentum nec

ddddDonec tellus odio, tristique non hendrerit quis, scelerisque placerat odio.

But this is converted to (505 characters):

Lorem ipsum dolor sit amet, consectetur adipiscing elit. Praesent placerat consectetur enim sit amet sodales. Etiam elementum, lorem id sollicitudin dignissim, quam nibh sodales arcu, imperdiet consequat risus mi sit amet lorem. Donec iaculis at tortor eget vulputate. Duis dignissim non diam id mattis. Suspendisse sit amet consequat elit, feugiat mattis libero. In hendrerit dui ac ante hendrerit condimentum nec

ddddDonec tellus odio, tristique non hendrerit quis, scelerisque placerat odio.

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

We can validate the parsed description max length instead of the plain text here:

App/src/pages/tasks/TaskDescriptionPage.tsx

Lines 41 to 43 in 63a20f4

	if (values?.description && values.description?.length > CONST.DESCRIPTION_LIMIT) {
	ErrorUtils.addErrorMessage(errors, 'description', ['common.error.characterLimitExceedCounter', {length: values.description.length, limit: CONST.DESCRIPTION_LIMIT}]);
	}

Like:

const validate = useCallback((values: FormOnyxValues<typeof ONYXKEYS.FORMS.EDIT_TASK_FORM>): FormInputErrors<typeof ONYXKEYS.FORMS.EDIT_TASK_FORM> => {
        const errors = {};
        const parsedDescription = ReportUtils.getParsedComment(values?.description);

        if (parsedDescription && parsedDescription?.length > CONST.DESCRIPTION_LIMIT) {
            ErrorUtils.addErrorMessage(errors, 'description', ['common.error.characterLimitExceedCounter', {length: parsedDescription.length, limit: CONST.DESCRIPTION_LIMIT}]);
        }

        return errors;
    }, []);

Test:

Max.lenght.parsed.description.mp4

What alternative solutions did you explore?

We can remove the ReportUtils.getParsedComment(description) and let the description as a plain text.

[tienifr]

Proposal

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

The error occurs when updating long content for the task description for a second time, after that user can't update the description or task name

What is the root cause of that problem?

• Initially, task report has reportActions is [{actionName: "Created", reportActionID: A}]

• When adding the long description, we add an optimistic message in here. Now the reportActions is [{actionName: "Created", reportActionID: A}, {actionName: "TASKEDITED", reportActionID: B}]

• Then, if the API is failed, we do not remove [{actionName: "TASKEDITED", reportActionID: B}] in here

• Now reportActions is still [{actionName: "Created", reportActionID: A}, {actionName: "TASKEDITED", reportActionID: B}]

• When we edit the description in second time, we add an optimistic message as well. Now the reportActions is [{actionName: "Created", reportActionID: A}, {actionName: "TASKEDITED", reportActionID: B}, {actionName: "TASKEDITED", reportActionID: C, previousReportActionID: A}].

• If we put the above reportActions to this function, the result is only [{actionName: "TASKEDITED", reportActionID: C, previousReportActionID: A}]. This result is the reportActions variable in here. And because it contains only {actionName: "TASKEDITED", reportActionID: C, previousReportActionID: A}, so hasCreatedAction is false, so the logic getOlderActions in here is called infinitely.

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

• First, we need to remove the {actionName: "TASKEDITED", reportActionID: B} action in the above example when the API is failed. Do it by updating this to:

           value: {[editTaskReportAction.reportActionID]: null},

• Then, the description's length validation need to be synchronized between BE and FE. Here, here and here need to be the same as how we validate the length in BE. To do it, we can update this to:

        if (values?.description && ReportUtils.getCommentLength(values.description) > CONST.DESCRIPTION_LIMIT) {
            ErrorUtils.addErrorMessage(errors, 'description', ['common.error.characterLimitExceedCounter', {length: ReportUtils.getCommentLength(values.description), limit: CONST.DESCRIPTION_LIMIT}]);
        }

Note: We already had the same logic in WorkspaceNewRoomPage.

What alternative solutions did you explore? (Optional)

NA

[melvin-bot]

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[bfitzexpensify]

Adding a second BZ team member to keep an eye on this while I'm ooo until June 11th.

Current status: proposal review.

[melvin-bot]

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

[adelekennedy]

@abdulrahuman5196 there are three proposals to review - will you be able to check them out today?

[abdulrahuman5196]

hi will check on this today

[abdulrahuman5196]

@samilabud 's proposal addresses the root cause of html text length mismatch since we use length of the plain text for comparison.

@neonbhai 's proposal should remove the new optimistic task action, but I think the solution should be fixing the html text length mismatch which would fix the issue at root. This could be considered as a extended fix but not the core fix.

@tienifr's proposal seems to be a improved combination of the both, so I don't think I can value it as a separate proposal.

[abdulrahuman5196]

@samilabud 's proposal here #42660 (comment) looks good and works well.

🎀 👀 🎀
C+ Reviewed

[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 9.0.14-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fixed task description when edit second time 42660 #43084

If no regressions arise, payment will be issued on 2024-08-07. 🎊

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

• @samilabud requires payment automatic offer (Contributor)
• @abdulrahuman5196 requires payment automatic offer (Reviewer)

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

• [@abdulrahuman5196] The PR that introduced the bug has been identified. Link to the PR:
• [@abdulrahuman5196] 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:
• [@abdulrahuman5196] 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:
• [@abdulrahuman5196] Determine if we should create a regression test for this bug.
• [@abdulrahuman5196] 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.
• [@bfitzexpensify / @adelekennedy] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[carlosmiceli]

@bondydaa What do you mean by updating the test steps?

[bondydaa]

regression test steps

[carlosmiceli]

These test steps here? Update them so that they make sure to maintain the description <500 chars? Sorry for the annoying questions but wanna make sure I do it right, first time logging into TestRail 😅

[mallenexpensify]

@carlosmiceli , @abdulrahuman5196, the C+, is responsible for proposing regression test steps, then the QA team will decide where to create them, if needed

[carlosmiceli]

@mallenexpensify great, thanks for letting us know! Will cross this off my list of pending things then.

[adelekennedy]

Payments have been made -we're just waiting for the regression test steps @abdulrahuman5196

[abdulrahuman5196]

The PR that introduced the bug has been identified. Link to the PR:
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:
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:

Not a regression IMO

Determine if we should create a regression test for this bug.

Yes.

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.

1. Start group chat with another user
2. Create a task for this user without a description
3. Update description with long content + emoji
4. Observe the error message of the app show
5. Update this content to description one more time

@adelekennedy I haven't received payment for this. And I am getting paid in newDot. Thank you.

[adelekennedy]

@abdulrahuman5196 that's my bad - I see that you ended the contract vs us closing and paying out

Contributor: @abdulrahuman5196 paid $250 via Upwork ( Matt A did on 8/30 as a bonus payment)

[adelekennedy]

https://github.com/Expensify/Expensify/issues/418967

[abdulrahuman5196]

Payment is still pending on this issue.

[abdulrahuman5196]

@adelekennedy I closed the contract before because I am getting paid via newDot now 😅 as called out.
And to answer question, only after my previous comment, it was remitted.
If possible kindly request a refund, so that I can get paid via newDot.

[adelekennedy]

🤦‍♀️

Payouts due:

• Contributor+: $250 @abdulrahuman5196 NewDot

[mallenexpensify]

@abdulrahuman5196 , can you please cancel your NewDot payment request! I've paid you via Upwork. You were assigned to this issue before your eligibility date. Thx

[abdulrahuman5196]

Deleted the newDot expense. Thank you for payment.

[abdulrahuman5196]

hi not overdue, We can close this issue @adelekennedy