[HOLD for payment 2024-04-09] [$500] Task - A new line is added to code block when saving unchanged description with code block

lanitochka17 commented 7 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: Reproducible in staging?: Reproducible in production?: 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:

Go to staging.new.expensify.com
Create a task without description
Go to task report
Click description
Type a word in code block in one line
Save the description
Click on the description
Save the description
Repeat Step 7 and 8 several times

Expected Result:

No extra line will be added each time saving unchanged description with code block The system message for task descrption will not appear each time saving unchanged description with code block

Actual Result:

An extra line will be added each time saving unchanged description with code block The system message for task descrption appears each time saving unchanged description with code block

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/8aa25040-22c1-4c67-95bf-2b40abe3654d

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01e1f69c2987288ad8
Upwork Job ID: 1767765440789991424
Last Price Increase: 2024-03-13

melvin-bot[bot] commented 7 months ago

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

lanitochka17 commented 7 months ago

@jliexpensify 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 commented 7 months ago

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

allgandalf commented 7 months ago

Proposal

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

A new line is added to code block when saving unchanged description with code block

What is the root cause of that problem?

This is a issue with expensify-common library, we use replace which leads to this bug:

https://github.com/Expensify/App/blob/39bd108377e87a3b8bf678581a1416123a35a9ff/src/pages/tasks/TaskDescriptionPage.tsx#L114

Everytime we use replace it adds a extra line to the start of the markdown

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

We can make use of the filterRules in replace: https://github.com/Expensify/expensify-common/blob/77d0b150ba6bfbe7a64b3c3e30b65592b2e58c4a/lib/ExpensiMark.d.ts#L26-L27

In the taskDescriptionPage, we need to apply the following markdownInBackticks filter option:

defaultValue={parser.htmlToMarkdown((props.report && parser.replace(props.report.description, {
                                filterRules: ["markdownInBackticks"]
                            })) || '')}

Note: I won't recommend fixing this upstream as we use parser at many places and this is related to only markdown, so we can use the filterRules provided upstream to deal with this bug.

Result:

https://github.com/Expensify/App/assets/110545952/0edbe622-4a06-4aa3-8efe-a61a0320b836

bernhardoj commented 7 months ago

Proposal

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

Saving an unchanged codefence markdown will add 1 new line.

What is the root cause of that problem?

When we save a codefence markdown with new line,

hello

EditTask response will return the new line as \r\n. Currently, codefence markdown regex only matches `` with\n` as the opening tag. https://github.com/Expensify/expensify-common/blob/77d0b150ba6bfbe7a64b3c3e30b65592b2e58c4a/lib/ExpensiMark.js#L33-L34

So, when we convert it to markdown, there will be an extra new line by \r\n in the codefence content/body.

It's know that the sever might returns \r\n as the new line as shown by the comment below. https://github.com/Expensify/App/blob/9302ade9988b1cba3e07b48f474bc35a6deea7fb/src/pages/tasks/TaskDescriptionPage.tsx#L50-L51

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

To solve it case by case, we can also apply StringUtils.normalizeCRLF to the task description before converting it to markdown when showing the task description in both TaskDescriptionPage and TaskView.

What alternative solutions did you explore? (Optional)

Update the codefence regex to match either \r\n or \n. https://github.com/Expensify/expensify-common/blob/77d0b150ba6bfbe7a64b3c3e30b65592b2e58c4a/lib/ExpensiMark.js#L33-L34

/(&#x60;&#x60;&#x60;(?:\r\n|\n)?)((?:\s*?(?!(?:\r\n|\n)?&#x60;&#x60;&#x60;(?!&#x60;))[\S])+\s*?)((?=(?:\r\n|\n)?)&#x60;&#x60;&#x60;)/g

// the difference is instead of using \n, we use (?:\r\n|\n)
// ?: to not capture it as a group as it will broke the replacement logic

melvin-bot[bot] commented 7 months ago

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

melvin-bot[bot] commented 7 months ago

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

melvin-bot[bot] commented 7 months ago

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

jliexpensify commented 7 months ago

Sorry, was sick yesterday. Can repro - what a weird bug.

tienifr commented 7 months ago

Proposal

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

An extra line will be added each time saving unchanged description with code block
The system message for task description appears each time saving unchanged description with code block

What is the root cause of that problem?

When we create or edit a task description, the back-end will return the description with \r\n as the new line, this is also acknowledged by us here Then in our parser, we only acknowledge the \n as a possible new line after "```", if it's not there, we'll append new line, this leads to a new line being added each time we edit the task
The 1. is one of the root cause for this problem. After 1. is fixed, we'll be left with this
Although we didn't change anything, there's still another updated the description to message, with spaces between the backticks and the text inside backticks.

This is because when we save

```hello```

as codeblock, we'll save it as is. Meanwhile, when we display it here, we normalize it by adding extra new lines, this is what we do in the main composer as well.

hello

Now when we save, it will be saved with the extra new lines, we detect changes and the updated the description to is added. Although to the users, nothing changes at all.

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

Update the code fence Regex here to acknowledge \r\n as possible new line character as well

/(&#x60;&#x60;&#x60;(?:\r\n|\n)?)((?:\s*?(?!(?:\r\n|\n)?&#x60;&#x60;&#x60;(?!&#x60;))[\S])+\s*?)((?=(?:\r\n|\n)?)&#x60;&#x60;&#x60;)/g

Before saving the description here, and also when creating the task with description, we should normalize the description just like when we display it here. So what we save will be consistent with what we displayed. And if the user sees there's no visual change, there's indeed no change.

What alternative solutions did you explore? (Optional)

An alternate solution is we can call StringUtils.normalizeCRLF on the report description whenever we try to display it to set it as default value of the input.

DylanDylann commented 7 months ago

Thanks for all your proposals

@GandalfGwaihir's proposal is to filter out the markdownInBackticks rule and it's not correct since we need to add the extra line for description when users open edit task page

@bernhardoj's proposal doesn't fix the second point The system message for task descrption will not appear each time saving unchanged description with code block

@tienifr's solution makes sense since it fixes 2 points in the OP

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 7 months ago

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

DylanDylann commented 7 months ago

Waiting internal engineer give a final approval

melvin-bot[bot] commented 7 months ago

❌ There was an error making the offer to @DylanDylann for the Reviewer role. The BZ member will need to manually hire the contributor.

melvin-bot[bot] commented 7 months ago

❌ There was an error making the offer to @tienifr for the Contributor role. The BZ member will need to manually hire the contributor.

melvin-bot[bot] commented 7 months ago

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

jliexpensify commented 7 months ago

Hi @muttmuure - I am headed OOO from 21st to 31st March. Here's a summary of where we are at:

We've got @tienifr working on a PR.
You may need to pay this out

PAYMENT SUMMARY

C: @tienifr $500
C+: @DylanDylann $500

UPWORKS JOB

I'll grab this one if when I'm back, thanks!

DylanDylann commented 7 months ago

@tienifr Is there any update here?

tienifr commented 7 months ago

@DylanDylann https://github.com/Expensify/expensify-common/pull/669, https://github.com/Expensify/App/pull/38908 these PRs are ready for review.

jliexpensify commented 6 months ago

Back, removed Matt!

melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.58-8 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/38908

If no regressions arise, payment will be issued on 2024-04-09. :confetti_ball:

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

@tienifr requires payment (Needs manual offer from BZ)
@DylanDylann requires payment (Needs manual offer from BZ)

melvin-bot[bot] commented 6 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:

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

DylanDylann commented 6 months ago

BugZero Checklist:

[x] The PR that introduced the bug has been identified. Link to the PR: N/A
[x] 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
[x] 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
[x] Determine if we should create a regression test for this bug. Yes
[x] 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.

Regression test:

Create the task without description
Edit a task description with code fence without line break
```
```task description```
```
Save the description
Verify the updated description to system message is correct
Open task description edit page again
Repeat step 2 and 3 several times
Verify that no extra line break is added each time saving the description
Verify that no new updated the description to system message is generated

Do we 👍 or 👎

jliexpensify commented 6 months ago

PAYMENT SUMMARY

C: @tienifr $500
C+: @DylanDylann $500

UPWORKS JOB

melvin-bot[bot] commented 6 months ago

Payment Summary

Upwork Job

ROLE: @tienifr paid $(AMOUNT) via Upwork (LINK)
ROLE: @DylanDylann paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@jliexpensify)

[ ] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
[ ] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1767765440789991424/hired)
[ ] I have paid out the Upwork contracts or cancelled the ones that are incorrect
[ ] I have verified the payment summary above is correct

jliexpensify commented 6 months ago

Paid and job closed

Expensify / App