Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.55k stars 2.9k forks source link

[$125] Task-Inconsistency in error display on creating task with long task title #50398

Open IuliiaHerets opened 1 month ago

IuliiaHerets commented 1 month 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: v9.0.46-2 Reproducible in staging?: Y Reproducible in production?: Y Issue reported by: Applause Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Open a chat
  3. Enter [] long task name and send the message
  4. Note there was an error creating this task, please try later message is displayed.
  5. Tap plus icon -- assign task
  6. Enter in title the same ling task name used in step 3
  7. Tap next
  8. Note character limit exceeded error is displayed

Expected Result:

There must not be inconsistency in error display for long task title while creating task using assign task & via []

Actual Result:

Inconsistency in error display for long task title while creating task using assign task & via [] .

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/9137f44f-02f5-4de3-9200-493a162ceb62

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021844741867905614976
  • Upwork Job ID: 1844741867905614976
  • Last Price Increase: 2024-11-08
Issue OwnerCurrent Issue Owner: @shubham1206agra
melvin-bot[bot] commented 1 month ago

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

IuliiaHerets commented 1 month ago

@twisterdotcom 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

bernhardoj commented 1 month ago

Proposal

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

We can create a task with long title using markdown, but will result in error.

What is the root cause of that problem?

I'm guessing we don't want to allow the user to create a long title task when using markdown. The current issue is that, the regex or the logic doesn't have a limit on how much character the title should be. https://github.com/Expensify/App/blob/273e75128dbcb24eab227a8308cc0a9ed32c288f/src/pages/home/report/ReportFooter.tsx#L136

In comparison, creating task from the + button has a validation to limit the task title. https://github.com/Expensify/App/blob/273e75128dbcb24eab227a8308cc0a9ed32c288f/src/pages/tasks/NewTaskDetailsPage.tsx#L60-L61

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

We need to validate the task title length too when creating it from the markdown. https://github.com/Expensify/App/blob/273e75128dbcb24eab227a8308cc0a9ed32c288f/src/pages/home/report/ReportFooter.tsx#L162-L168

if (title.length > CONST.TITLE_CHARACTER_LIMIT) {
    return false;
}

Task.createTaskAndNavigate(report.reportID, title, '', assignee?.login ?? '', assignee?.accountID, assigneeChatReport, report.policyID, true);
return true;
ChavdaSachin commented 1 month ago

Proposal

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

Task-Inconsistency in error display on creating task with long task title

What is the root cause of that problem?

When we create task from create task page, we have validation for title length so we haven't considered a display error message in case task title is too long. But we could not implement the same validation for markdown.

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

Make changes to errors for failure data here

 errors: title.length>CONST.TITLE_CHARACTER_LIMIT ?ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('task.titleTooLong') : ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('task.genericCreateTaskFailureMessage'),

https://github.com/Expensify/App/blob/cb0ccbc9fa25744e1a12dcc4e1bfd191563d8814/src/libs/actions/Task.ts#L269

include new error in en.ts and es.ts files

        titleTooLong: 'Task title is too long',

https://github.com/Expensify/App/blob/cb0ccbc9fa25744e1a12dcc4e1bfd191563d8814/src/languages/en.ts#L4083

        taskTitleTooLong: 'El título de la tarea es demasiado largo',

https://github.com/Expensify/App/blob/cb0ccbc9fa25744e1a12dcc4e1bfd191563d8814/src/languages/es.ts#L4128

This task error message is what I think is most appropriate, but any error message that team agrees on could be implemented here.(BTW this is the same error message BE responds with)

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.

twisterdotcom commented 1 month ago

Very edge case, so downgrading but it is a bug.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

Upwork job price has been updated to $125

shubham1206agra commented 1 month ago

The problem is that we are creating this task inline and do not validate it in the composer. But we should have done it.

ChavdaSachin commented 1 month ago

@shubham1206agra you are right, we do not validate it in composer. But there is a reason why we can't..... These are the two outcomes we could think of...

  1. We don't have MD preview for task like we have for other MD, else we could stop showing preview and user can understand that title is too long.
  2. We could stop making createTask call and let it be a regular comment. which does not look consistent IMO.

Do you have any suggestions how can we inform user about the length if we validate in composer? I can't think of any...

melvin-bot[bot] commented 3 weeks ago

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

shubham1206agra commented 3 weeks ago

@twisterdotcom I don't think we are showing error at correct place. We should show error before submitting the message. Just like maximum limit exceeded for comment. Do you agree with this comment?

melvin-bot[bot] commented 3 weeks ago

@twisterdotcom @shubham1206agra this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 2 weeks ago

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

shubham1206agra commented 2 weeks ago

@twisterdotcom Bump here https://github.com/Expensify/App/issues/50398#issuecomment-2428511240

twisterdotcom commented 2 weeks ago

Man this bug is so niche. Yes, I agree, if we can show the error earlier, ie before they even try to send the message to create the task, that is better and as you say, more in line with the comment maximum error.

melvin-bot[bot] commented 1 week ago

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

wildan-m commented 1 week ago

Proposal

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

Inconsistency in error messages when creating tasks with long titles.

What is the root cause of that problem?

We did not validate task title length when changing the main composer value. This could be considered a new validation feature.

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

I've investigated the behavior of this task markdown

  1. Tasks are only rendered if they come from the main composer, not if edited from a submitted chat.
  2. The task title cannot contain markdown, so the length is counted with all typed symbols.

First we need to extract this regex to CONST so we can re-use them in ReportActionCompose

https://github.com/Expensify/App/blob/bef062b4caa7f665159dc107911e708031e648c4/src/pages/home/report/ReportFooter.tsx#L127-L136

Move to CONST:


        /**
         * Matching task rule by group
         * Group 1: Start task rule with []
         * Group 2: Optional email group between \s+....\s* start rule with @+valid email or short mention
         * Group 3: Title is remaining characters
         */
        // The regex is copied from the expensify-common CONST file, but the domain is optional to accept short mention
        TASK_TITLE_WITH_OPTONAL_SHORT_MENTION: `^\\[\\]\\s+(?:@(?:${EMAIL_WITH_OPTIONAL_DOMAIN}))?\\s*([\\s\\S]*)` 

Then modify:

https://github.com/Expensify/App/blob/bef062b4caa7f665159dc107911e708031e648c4/src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx#L493-L498

To


                                        <ComposerWithSuggestions

                                            onValueChange={(value) => {
                                                if (value.length === 0 && isComposerFullSize) {
                                                    Report.setIsComposerFullSize(reportID, false);
                                                }

                                                debouncedValidate(value, reportID);
                                            }}
                                        />

Where debouncedValidate (we can change name later) will wrap existing validation with task specific validation


    const debouncedValidate = useDebounce((value, reportID) => {
        const match = value.match(CONST.REGEX.TASK_TITLE_WITH_OPTONAL_SHORT_MENTION);
        if (!match) {
            setHasExceededTaskTitleLength(false);
            validateCommentMaxLength(value, { reportID });
            return;
        }

        let title = match[3] ? match[3].trim().replace(/\n/g, ' ') : undefined;
        setHasExceededTaskTitleLength(title ? title.length > CONST.TITLE_CHARACTER_LIMIT : false);
    }, 100);

Since this task specific validation is simpler than validateCommentMaxLength validation, and the validateCommentMaxLength also already debounce by 1500ms I think it's reasonable to add small debounce here e.g. 100ms or no debounce at all

Then use the hasExceededTaskTitleLength side by side with hasExceededMaxCommentLength


hasExceededMaxCommentLength || hasExceededTaskTitleLength

Apply here, here, here, and here.

We need to make the error message more accurate by replacing the max length if it's task title. Modify ExceededCommentLength component to:


type ExceededCommentLengthProps = {
    shouldUseTitleLimit?: boolean;
};

function ExceededCommentLength({shouldUseTitleLimit}: ExceededCommentLengthProps) {
            style={[styles.textMicro, styles.textDanger, styles.chatItemComposeSecondaryRow, styles.mlAuto, styles.pl2]}
            numberOfLines={1}
        >
            {translate('composer.commentExceededMaxLength', { formattedMaxLength: numberFormat(shouldUseTitleLimit ? CONST.TITLE_CHARACTER_LIMIT : CONST.MAX_COMMENT_LENGTH) })}
        </Text>
    );
}

And use it this way


<ExceededCommentLength shouldUseTitleLimit={hasExceededTaskTitleLength} />}

Branch for this solution

What alternative solutions did you explore? (Optional)

N/A

melvin-bot[bot] commented 1 week ago

@twisterdotcom @shubham1206agra this issue is now 4 weeks old, please consider:

Thanks!

shubham1206agra commented 1 week ago

@wildan-m Please provide a test branch so I can test these changes.

wildan-m commented 1 week ago

@shubham1206agra The branch was provided.

image

melvin-bot[bot] commented 6 days ago

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

melvin-bot[bot] commented 4 days ago

@twisterdotcom, @shubham1206agra Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 2 days ago

@twisterdotcom, @shubham1206agra 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

twisterdotcom commented 1 day ago

Can we assign @wildan-m @shubham1206agra?

melvin-bot[bot] commented 6 hours ago

@twisterdotcom, @shubham1206agra Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!