Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2023-11-29] [$500] Inconsistency bug - Assign Task - Task description does not remove empty quotes (works fine in private notes) #28342

Closed
6 tasks done
izarutskaya opened this issue Sep 27, 2023 · 71 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@izarutskaya
Copy link

izarutskaya commented Sep 27, 2023

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


Action Performed:

  1. Open the app
  2. Open task report or assign any user task and open the report
  3. Open description
  4. Enter text with multiple empty quotes eg:
> Hello
>
>
  1. Save the description and observe that description displays only one quote with text on task page (which is correct)
  2. Again open description and observe that extra greater then signs are not removed
  3. Paste similar text in composer, send and edit and observe that extra greater then signs are removed
  4. Similarly try the text in private notes of any user and observe that private notes too removes the extra signs

Expected Result:

App should remove greater then signs with no text after it when description is saved like we do throughout the app

Actual Result:

App does remove greater then signs with no text after it when description is saved in task

Workaround:

Unknown

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: v1.3.74-2

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

mac.chrome.desktop.description.is.not.resolved.1.mov
Screen_Recording_20230928_152004_New.Expensify.mp4
Ios.Safari.Native.Description.Is.Not.Resolved.1.mp4
android.chrome.description.is.not.fixed.for.quotes.mp4
mac.chrome.desktop.description.is.not.resolved.mov
Recording.1663.mp4

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695746974925629

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01cbe475b72eea075d
  • Upwork Job ID: 1707098020774612992
  • Last Price Increase: 2023-09-27
  • Automatic offers:
    • DylanDylann | Contributor | 27099861
    • dhanashree-sawant | Reporter | 27099863
@izarutskaya izarutskaya added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 27, 2023
@melvin-bot melvin-bot bot changed the title Inconsistency bug - Assign Task - Task description does not remove empty quotes (works fine in private notes) [$500] Inconsistency bug - Assign Task - Task description does not remove empty quotes (works fine in private notes) Sep 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

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 melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

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

@saranshbalyan-1234
Copy link
Contributor

Proposal

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

In task description data is not trimmed

What is the root cause of that problem?

While saving task description we are not trimming the description here.

const submit = useCallback(
(values) => {
// Set the description of the report in the store and then call Task.editTaskReport
// to update the description of the report on the server
Task.editTaskAndNavigate(props.report, props.session.accountID, {description: values.description});
},
[props],
);

and
const onSubmit = (values) => {
Task.setDescriptionValue(values.taskDescription);
Navigation.goBack(ROUTES.NEW_TASK);
};

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

We need to trim the data before saving like this in TaskDescriptionPage

  const submit = useCallback(
        (values) => {
            // Set the description of the report in the store and then call Task.editTaskReport
            // to update the description of the report on the server
             const editedDescription = parser.replace( values.description.trim());
            Task.editTaskAndNavigate(props.report, props.session.accountID, {description: editedDescription});
        },
        [props],
    );

and in NewTaskDescriptionPage

   const onSubmit = (values) => {
        const editedDescription = parser.replace( values.description.trim());
        Task.setDescriptionValue(editedDescription);
        Navigation.goBack(ROUTES.NEW_TASK);
    };

What alternative solutions did you explore? (Optional)

N/A

@yh-0218
Copy link
Contributor

yh-0218 commented Sep 28, 2023

Proposal

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

Inconsistency bug - Assign Task - Task description does not remove empty quotes

What is the root cause of that problem?

Because we save origin description (user input).
Ex. > Test \n >
We need to update this to > Test before we save using ExpensiMark

const submit = useCallback(
(values) => {
// Set the description of the report in the store and then call Task.editTaskReport
// to update the description of the report on the server
Task.editTaskAndNavigate(props.report, props.session.accountID, {description: values.description});
},
[props],
);

and
const onSubmit = (values) => {
Task.setDescriptionValue(values.taskDescription);
Navigation.goBack(ROUTES.NEW_TASK);
};

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

For TaskDescriptionPage, we need save updatedDescription instead of values.description.

            const parser = new ExpensiMark();
            const editedDescription = parser.replace(values.description.trim());
            const updatedDescription = parser.htmlToMarkdown(editedDescription);
            Task.editTaskAndNavigate(props.report, props.session.accountID, {description: updatedDescription});

For NewTaskDescriptionPage, we need save updatedDescription instead of values.taskDescription.

            const parser = new ExpensiMark();
            const editedDescription = parser.replace(values.taskDescription.trim());
            const updatedDescription = parser.htmlToMarkdown(editedDescription);
            Task.setDescriptionValue(updatedDescription); 
            Navigation.goBack(ROUTES.NEW_TASK); 

What alternative solutions did you explore? (Optional)

@yurakurets
Copy link

yurakurets commented Sep 28, 2023

Proposal

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

Description text is saving just as plain text, while it can be displayed as markdown.

What is the root cause of that problem?

There are 3 steps where it happens:

  1. When user starts creating task

// On submit, we want to call the assignTask function and wait to validate
// the response
function onSubmit(values) {
Task.setDetailsValue(values.taskTitle, values.taskDescription);
Navigation.navigate(ROUTES.NEW_TASK);
}

  1. On the second step of creating

const submit = useCallback(
(values) => {
// Set the description of the report in the store and then call Task.editTaskReport
// to update the description of the report on the server
Task.editTaskAndNavigate(props.report, props.session.accountID, {description: values.description});
},
[props],
);

  1. When user edits task description

const submit = useCallback(
(values) => {
// Set the description of the report in the store and then call Task.editTaskReport
// to update the description of the report on the server
Task.editTaskAndNavigate(props.report, props.session.accountID, {description: values.description});
},
[props],
);

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

We have to parse description field on each of these steps via ExpensiMark and save parsed version of it:

function onSubmit(values) {
        const parser = new ExpensiMark();
        const markdownDescription = parser.htmlToMarkdown(parser.replace(values.taskDescription.trim()))
        Task.setDetailsValue(values.taskTitle, markdownDescription);
        Navigation.navigate(ROUTES.NEW_TASK);
    }
const onSubmit = (values) => {
        const parser = new ExpensiMark();
        const markdownDescription = parser.htmlToMarkdown(parser.replace(values.taskDescription.trim()))
        Task.setDescriptionValue(markdownDescription);
        Navigation.goBack(ROUTES.NEW_TASK);
    };
    const submit = useCallback(
        (values) => {
            const parser = new ExpensiMark();
            const markdownDescription = parser.htmlToMarkdown(parser.replace(values.description.trim()))
            // Set the description of the report in the store and then call Task.editTaskReport
            // to update the description of the report on the server
            Task.editTaskAndNavigate(props.report, props.session.accountID, {description: markdownDescription});
        },
        [props],
    );

The last thing to take into account is the state of NewTaskDetailsPage.js const [taskDescription, setTaskDescription] = useState(props.task.description || '');, because there is an option to go back to this screen and then user will be able to see old state with empty quotes. In order to fix it we should update state on focus, currently it's syncing in useEffect

useEffect(() => {
setTaskTitle(props.task.title);
setTaskDescription(props.task.description || '');
}, [props.task]);

    useEffect(() => {
        setTaskTitle(props.task.title);
    }, [props.task]);

    useFocusEffect(useCallback(() => {
        setTaskDescription(props.task.description || '');
    }, [props.task.description]))
Screen.Recording.2023-09-28.at.14.51.57.mov

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@melvin-bot
Copy link

melvin-bot bot commented Sep 28, 2023

📣 @yurakurets! 📣
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:

  1. Make sure you've read and understood the contributing guidelines.
  2. 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.
  3. 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.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@yurakurets
Copy link

Contributor details
Your Expensify account email: yura.kurets1995@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~013589ef6319817fdf

@melvin-bot
Copy link

melvin-bot bot commented Sep 28, 2023

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@DylanDylann
Copy link
Contributor

Proposal

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

  • Inconsistency bug: Task description does not remove empty quotes (works fine in private notes)

What is the root cause of that problem?

  • Currently, with the description field, we are saving the input text directly:

    const onSubmit = (values) => {
    Task.setDescriptionValue(values.taskDescription);
    Navigation.goBack(ROUTES.NEW_TASK);
    };

  • But for other logic like private notes, we will convert the user`s input to HTML and then save:

    const savePrivateNote = () => {
    const editedNote = parser.replace(privateNote.trim());
    Report.updatePrivateNotes(report.reportID, route.params.accountID, editedNote);
    Keyboard.dismiss();
    // Take user back to the PrivateNotesView page
    Navigation.goBack(ROUTES.HOME);
    };

  • There is no issue when we display these to content (description or private notes).

  • But when editing, with the description, it uses the description that has been saved before as a default value. So if the user types:

> Hello
>
>

when creating a task, it still displays this text in the editing input, which lead to the bug.

  • As I mentioned above, private notes save the HTML content, and when editing, it will convert this HTML content to raw text content, so it will set the default value as "> Hello", the remaining sections are removed.
    const [privateNote, setPrivateNote] = useState(parser.htmlToMarkdown(lodashGet(report, ['privateNotes', route.params.accountID, 'note'], '')).trim());

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

  • We can handle the logic that saves the description as we did with private note: Convert the user input to html and then saves it. When editing, convert the saved html to raw text html and set as the default value:
  • Firstly, update the onSubmit function:
    const onSubmit = (values) => {
    Task.setDescriptionValue(values.taskDescription);
    Navigation.goBack(ROUTES.NEW_TASK);
    };

    to
    const onSubmit = (values) => {
        const parser = new ExpensiMark();
        const parsedTaskDescription = parser.replace(values.taskDescription);
        Task.setDescriptionValue(parsedTaskDescription);
        Navigation.goBack(ROUTES.NEW_TASK);
    };
  • Secondly, add to
    <MenuItemWithTopDescription
    description={props.translate('task.description')}
    title={description}
    onPress={() => Navigation.navigate(ROUTES.NEW_TASK_DESCRIPTION)}
    shouldShowRightIcon
    shouldParseTitle
    numberOfLinesTitle={2}
    titleStyle={styles.flex1}
    />
    to display description properly
  • Then update
    defaultValue={props.task.description}

    to defaultValue={parser.htmlToMarkdown(props.task.description)}

What alternative solutions did you explore? (Optional)

  • NA

Result

Screencast.from.28-09-2023.20.04.02.webm

@melvin-bot melvin-bot bot added the Overdue label Oct 2, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2023

@garrettmknight, @robertKozik Huh... This is 4 days overdue. Who can take care of this?

@garrettmknight
Copy link
Contributor

Reproduced. @robertKozik can you take a look at these proposals?

@melvin-bot melvin-bot bot removed the Overdue label Oct 3, 2023
@robertKozik
Copy link
Contributor

Hi all! Thank you for your proposals! I think we should go with the @DylanDylann proposal as it represents the most complete solution to unify the behaviour with the private notes approach. Other proposals are trying to parse both ways the description, which is in my opinion redundant and resource-consuming operation while saving the value.

Selected Proposal: #28342 (comment)

🎀 👀 🎀 C+ reviewed

@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2023

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

@iwiznia
Copy link
Contributor

iwiznia commented Oct 4, 2023

I don't understand the bug, to me the current behavior is correct. Yes, it is not the same as in other places, but that's not a bug on itself. Are the users prevented from doing an action? Is something actually broken?
I think we should close this. @garrettmknight what are your thoughts here for accepting this as a bug?

@DylanDylann
Copy link
Contributor

@iwiznia I think this logic should be the same throughout the app so users are not confused. Additionally, if the logic is the same, we can create the util function to display, save, ... this type of input content and reuse it in the future

@garrettmknight
Copy link
Contributor

I'd agree that this is a clear inconsistency in how the two text areas work and without any reasoning for one to work differently (i.e. why is the text area in a task not removing erroneous pieces when the text composer?). It's a question of standardizing where the behavior should be the same, not whether the app is broken outright.

@DylanDylann
Copy link
Contributor

DylanDylann commented Oct 5, 2023

@garrettmknight Currently, these are a few components that will remove all the redundant ">":

  1. Message composer
  2. Private notes
  • Message composer is the component that is most used in our app so I think we should follow it

@melvin-bot melvin-bot bot added the Overdue label Oct 9, 2023
@DylanDylann
Copy link
Contributor

DylanDylann commented Nov 8, 2023

@iwiznia Did you check this #28342 (comment)?

@iwiznia
Copy link
Contributor

iwiznia commented Nov 10, 2023

That links to my own comment.
We need to figure out a way of deploying this without breaking it while one PR is deployed but not the other...

@DylanDylann
Copy link
Contributor

@garrettmknight do you have any suggestion for this one? #28342 (comment)

@garrettmknight
Copy link
Contributor

@iwiznia I think he was referring to this part of your comment:

ok, I think I found why it is being removed... will need to do some tests on monday.

@garrettmknight
Copy link
Contributor

Starting a thread about how we can manage the deploy in slack.

@DylanDylann
Copy link
Contributor

DylanDylann commented Nov 14, 2023

@iwiznia Based on the discussion in slack channel, should we consider using my suggestion before? #29144 (comment)

In this proposal #28342 (comment), I let BE store the HTML, not mark down like the private notes logic.
But when implementing this PR, I found out that if we let BE store the HTML, for example,
Hello
123
, it just stores as "Hello123". So when submitting the description to BE, I keep using the original logic (let BE save the mark down string) rather than convert mark down to HTML.

@iwiznia
Copy link
Contributor

iwiznia commented Nov 14, 2023

Huh? If we deploy your change before the backend change is deployed, things will also be broken, no?

@DylanDylann
Copy link
Contributor

No. I mean that we do not need the BE change

@iwiznia
Copy link
Contributor

iwiznia commented Nov 14, 2023

I am so lost... would that solve the issue, make the behavior consistent with private notes and not require a backend change?

@DylanDylann
Copy link
Contributor

@iwiznia Yeah, C+ has reviewed and approved this solution before

@iwiznia
Copy link
Contributor

iwiznia commented Nov 15, 2023

Ah ok, if it does all that, then let's do it

@DylanDylann
Copy link
Contributor

@iwiznia yeah. I will update the PR and ping you review it

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 22, 2023
@melvin-bot melvin-bot bot changed the title [$500] Inconsistency bug - Assign Task - Task description does not remove empty quotes (works fine in private notes) [HOLD for payment 2023-11-29] [$500] Inconsistency bug - Assign Task - Task description does not remove empty quotes (works fine in private notes) Nov 22, 2023
Copy link

melvin-bot bot commented Nov 22, 2023

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

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Nov 22, 2023
Copy link

melvin-bot bot commented Nov 22, 2023

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

If no regressions arise, payment will be issued on 2023-11-29. 🎊

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

Copy link

melvin-bot bot commented Nov 22, 2023

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:

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 28, 2023
@garrettmknight
Copy link
Contributor

Summary of payment:

All paid up.

@garrettmknight
Copy link
Contributor

@robertKozik can you complete the checklist when you get a chance?

@garrettmknight garrettmknight added Weekly KSv2 and removed Daily KSv2 labels Nov 30, 2023
@robertKozik
Copy link
Contributor

  • [@robertKozik] The PR that introduced the bug has been identified. Link to the PR: N/A It was rather inconsistency between behaviour of different inputs
  • [@robertKozik] 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
  • [@robertKozik] 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
  • [@robertKozik] Determine if we should create a regression test for this bug. I think we don't need regression test for it
  • [@robertKozik] 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. N/A

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants