[$500] Chat - The line breaks won't clear/appear on the chat #35417

kbecciv · 2024-01-30T17:19:46Z

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.33.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:

Issue found when executing PR #35300

Action Performed:

Open a chat
Insert a few line breaks into the composer
Send a photo attachment

Expected Result:

Line breaks should clear/appear on the chat.

Actual Result:

If you send an attachment, the line breaks won't clear/appear on the chat.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6361104_1706633504689.XLII8792.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01c9ba0e8e79576abf
Upwork Job ID: 1752381112662769664
Last Price Increase: 2024-02-13

melvin-bot · 2024-01-30T17:19:51Z

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

melvin-bot · 2024-01-30T17:19:51Z

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

melvin-bot · 2024-01-30T17:19:56Z

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

kbecciv · 2024-01-30T17:20:24Z

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

FitseTLT · 2024-01-30T17:34:31Z

Proposal

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

The line breaks won't clear/appear on the chat

What is the root cause of that problem?

We are early returning if length is zero after trimming the comment

App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js

Lines 336 to 342 in a44e804

    
           const trimmedComment = commentRef.current.trim(); 
        
           const commentLength = ReportUtils.getCommentLength(trimmedComment); 
        
           // Don't submit empty comments or comments that exceed the character limit 
        
           if (!commentLength || commentLength > CONST.MAX_COMMENT_LENGTH) { 
        
               return ''; 
        
           }

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

We should take the clearing code before the early return

App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js

Lines 339 to 342 in a44e804

    
           // Don't submit empty comments or comments that exceed the character limit 
        
           if (!commentLength || commentLength > CONST.MAX_COMMENT_LENGTH) { 
        
               return ''; 
        
           }

        debouncedSaveReportComment.cancel();
        updateComment('');
        setTextInputShouldClear(true);
        if (isComposerFullSize) {
            Report.setIsComposerFullSize(reportID, false);
        }
        setIsFullComposerAvailable(false);

What alternative solutions did you explore? (Optional)

dukenv0307 · 2024-01-30T20:29:17Z

Proposal

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

Chat - The line breaks won't clear/appear on the chat

What is the root cause of that problem?

When clicking on "send" button in attachment modal, the validation logic will be called that leads to the rest is not called

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

Currently, there are 2 methods which we can send a message: 1st is pressing on "Enter" key or clicking on "Send" icon on the right side of the composer. 2nd is clicking on "Send" button in attachment modal
The 1st works well. In this method, the handleCreateTask is called, then the validation (note that, the validation logic that I mentioned in the RCA section is also contained in here) is called, finally, it calls the send message and clean up logic
The 2nd just call the send message and clean up logic (That leads to a lot of bug: For example, user who is blocked by concierge still can send attachment. Or the create task feature does not work when sending attachment)
Based on the above analytic, the general solution is that: apply the 1st in case of clicking on "Send" button in the attachment modal for consistency. Then, just need to remove the validation logic. Below are steps:

Update the onSubmitComment so it can be used in send attachment case:

    const onSubmitComment = useCallback(
        (text, file) => {
            ...
            if (file) {
                Report.addAttachment(getReportID(route), file, text);
            } else {
                Report.addComment(getReportID(route), text);
            }
        },
        [route, handleCreateTask],
    );

Then, we need to update addAttachment to use handleSendMessage
Remove the validation logic that leads to our bug

What alternative solutions did you explore? (Optional)

Once we type anything in composer, upload an attachment, then click on "Send" button, just the attachment is sent, anything we type in composer still persists. In other words, we will consider the sending attachment and sending text to be separate. So it will be easier to handle the case: When user types more than 10.000 characters, they still can send the attachment

dukenv0307 · 2024-01-30T20:39:31Z

@slafortune @thesahindia Should the behavior be "Once we type anything in composer, upload an attachment, then click on "Send" button, just the attachment is sent, anything we type in composer still persists. In other words, we will consider the sending attachment and sending text to be separately"

slafortune · 2024-01-31T20:27:31Z

I think that the "attachment" should always be attached to the message - with one send. If someone want to send something separate, they would simply add only an attachement.

dukenv0307 · 2024-02-01T11:56:11Z

@slafortune I think it can make users confuse. Currently, clicking on "Send" button in the attachment modal (see below image) will send the attached with a message that the user has typed, so it can make user confuse that: why I just click on "Send" attachment, but the text message is also sent?

FitseTLT · 2024-02-01T12:34:51Z

@slafortune I think it can make users confuse. Currently, clicking on "Send" button in the attachment modal (see below image) will send the attached with a message that the user has typed, so it can make user confuse that: why I just click on "Send" attachment, but the text message is also sent?

Although I can't give the link to the discussion (difficult to find it), I remember I have come up with a discussion that this sending message with attachement is indeed the expected behaviour. NOw because the default behaviour is send the message reset it when attachement is sent and in this case the message is all but a bunch of new lines it is not considered as a message b/c we trim it and it will end up with an empty string. So what we can do is reset the message although nothing is to be sent expect the attachement as I am recommending in my propsal otherwise we leave it as it is and close this issue.

thesahindia · 2024-02-04T23:53:53Z

Although I can't give the link to the discussion (difficult to find it), I remember I have come up with a discussion that this sending message with attachement is indeed the expected behaviour.

Yeah there was a discussion in the past.

thesahindia · 2024-02-04T23:55:51Z

@FitseTLT, I think your solution will cause a regression. If we reach the character limit and send an attachment the composer will get clear.

dukenv0307 · 2024-02-05T06:36:22Z

@thesahindia Do you have any feedback about my proposal here? Note that you can ignore my alternative solution based on this comment

FitseTLT · 2024-02-05T09:11:44Z

@FitseTLT, I think your solution will cause a regression. If we reach the character limit and send an attachment the composer will get clear.

@thesahindia If we don't want to clear it in this case, we can separate the condition and clear only when the trimmed comment is empty to deal with this current issue

App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js

Lines 339 to 342 in a44e804

    
           // Don't submit empty comments or comments that exceed the character limit 
        
           if (!commentLength || commentLength > CONST.MAX_COMMENT_LENGTH) { 
        
               return ''; 
        
           }

if (commentLength > CONST.MAX_COMMENT_LENGTH) {
            return '';
        }

        debouncedSaveReportComment.cancel();

        updateComment('');
        setTextInputShouldClear(true);
        if (isComposerFullSize) {
            Report.setIsComposerFullSize(reportID, false);
        }
        setIsFullComposerAvailable(false);
        // Don't submit empty comments or comments that exceed the character limit
        if (!commentLength) {
            return '';
        }

melvin-bot · 2024-02-06T16:00:10Z

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

slafortune · 2024-02-07T16:22:24Z

I think we should close this for more pressing issues, as the functionality is working as discussed. Thoughts on this @thesahindia ?

thesahindia · 2024-02-07T18:34:18Z

@thesahindia Do you have any feedback about my proposal here? Note that you can ignore my alternative solution based on this comment

Your solution looks good to me but can you test it thoroughly and verify that there won't be any regressions?

thesahindia · 2024-02-07T18:38:25Z

I think we should close this for more pressing issues, as the functionality is working as discussed. Thoughts on this @thesahindia ?

I am fine with closing this since it's an edge case and it will be just a small improvement.

melvin-bot · 2024-02-13T15:04:51Z

@slafortune @thesahindia 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 · 2024-02-13T15:17:58Z

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

melvin-bot · 2024-02-13T16:00:30Z

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

kbecciv 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 Jan 30, 2024

melvin-bot bot assigned slafortune Jan 30, 2024

melvin-bot bot changed the title ~~Chat - The line breaks won't clear/appear on the chat~~ [$500] Chat - The line breaks won't clear/appear on the chat Jan 30, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 30, 2024

melvin-bot bot assigned thesahindia Jan 30, 2024

lanitochka17 mentioned this issue Jan 30, 2024

[$500] iOS - Chat - The line breaks won't clear/appear on the chat #35418

Closed

6 tasks

melvin-bot bot added the Overdue label Feb 7, 2024

melvin-bot bot removed the Overdue label Feb 7, 2024

melvin-bot bot added the Overdue label Feb 12, 2024

slafortune closed this as completed Feb 13, 2024

melvin-bot bot removed the Overdue label Feb 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[$500] Chat - The line breaks won't clear/appear on the chat #35417

[$500] Chat - The line breaks won't clear/appear on the chat #35417

kbecciv commented Jan 30, 2024 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Jan 30, 2024

melvin-bot bot commented Jan 30, 2024

melvin-bot bot commented Jan 30, 2024

kbecciv commented Jan 30, 2024

FitseTLT commented Jan 30, 2024 •

edited

Loading

dukenv0307 commented Jan 30, 2024 •

edited

Loading

dukenv0307 commented Jan 30, 2024 •

edited

Loading

slafortune commented Jan 31, 2024

dukenv0307 commented Feb 1, 2024

FitseTLT commented Feb 1, 2024

thesahindia commented Feb 4, 2024

thesahindia commented Feb 4, 2024

dukenv0307 commented Feb 5, 2024 •

edited

Loading

FitseTLT commented Feb 5, 2024 •

edited

Loading

melvin-bot bot commented Feb 6, 2024

slafortune commented Feb 7, 2024

thesahindia commented Feb 7, 2024 •

edited

Loading

thesahindia commented Feb 7, 2024

melvin-bot bot commented Feb 13, 2024

melvin-bot bot commented Feb 13, 2024

melvin-bot bot commented Feb 13, 2024

[$500] Chat - The line breaks won't clear/appear on the chat #35417

[$500] Chat - The line breaks won't clear/appear on the chat #35417

Comments

kbecciv commented Jan 30, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Jan 30, 2024

melvin-bot bot commented Jan 30, 2024

melvin-bot bot commented Jan 30, 2024

kbecciv commented Jan 30, 2024

FitseTLT commented Jan 30, 2024 • edited Loading

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)

dukenv0307 commented Jan 30, 2024 • edited Loading

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)

dukenv0307 commented Jan 30, 2024 • edited Loading

slafortune commented Jan 31, 2024

dukenv0307 commented Feb 1, 2024

FitseTLT commented Feb 1, 2024

thesahindia commented Feb 4, 2024

thesahindia commented Feb 4, 2024

dukenv0307 commented Feb 5, 2024 • edited Loading

FitseTLT commented Feb 5, 2024 • edited Loading

melvin-bot bot commented Feb 6, 2024

slafortune commented Feb 7, 2024

thesahindia commented Feb 7, 2024 • edited Loading

thesahindia commented Feb 7, 2024

melvin-bot bot commented Feb 13, 2024

melvin-bot bot commented Feb 13, 2024

melvin-bot bot commented Feb 13, 2024

kbecciv commented Jan 30, 2024 •

edited by melvin-bot bot

Loading

FitseTLT commented Jan 30, 2024 •

edited

Loading

dukenv0307 commented Jan 30, 2024 •

edited

Loading

dukenv0307 commented Jan 30, 2024 •

edited

Loading

dukenv0307 commented Feb 5, 2024 •

edited

Loading

FitseTLT commented Feb 5, 2024 •

edited

Loading

thesahindia commented Feb 7, 2024 •

edited

Loading