[HOLD for payment 2024-08-19] [$250] Onboarding tasks - User can write and send a message if fast even if composer hides

[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: 9.0.16
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): bezawitgebremichael+112233@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

1. Navigate to staging.new.expensify.com
2. Sign in with a new account
3. On the onboarding modal select "Manage my team's expenses"
4. Finish the onboarding flow by filling out business name and first name
5. Open expensify chat
6. Click on a task and type something fast and send it before the composer hides

Expected Result:

If the task thread is intended to be a read only thread then user should not be able to write and send anything in the composer regardless of the speed of typing

Actual Result:

If user opens the read only task and writes something in the composer and sends it fast then user can send a comment before the composer hides

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

Bug6554855_1722100580801.4_5848162808948069277.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a5540041e5f900a2
• Upwork Job ID: 1820322192329234850
• Last Price Increase: 2024-08-05
• Automatic offers:
• dukenv0307 | Reviewer | 103406855
• nkdengineer | Contributor | 103406857

Issue Owner

Current Issue Owner: @kadiealexander

[melvin-bot]

Triggered auto assignment to @kadiealexander (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]

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

[kpadmanabhan]

Click on a task and type something fast and send it before the composer hides

@lanitochka17 : for me the composer never hides. it stays active always. is it supposed to be not brought up at all for "Meet your setup specialist" task or for any task? When is the composer expected to be active like a usual chat as per requirement?

below screenshot is 5 min after i reproduced your test steps in staging now.

[kpadmanabhan]

Edited by proposal-police: This proposal was edited at 2024-08-04 00:18:56 UTC.

Proposal

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

Able to send a message from composer, if really quick in action, before composer is hidden.

What is the root cause of that problem?

We display the composer optimistically until the response from OpenReport API call is processed.
If OpenReport API response contains write action inside permissions json array, then composer is visible.
In case of onboarding tasks OpenReport API contains only read action in permissions array.

               "permissions": [
                    "read"
                ],

Hence the composer is not displayed as defined in
src/pages/home/report/ReportFooter.tsx

    const shouldHideComposer = (!ReportUtils.canUserPerformWriteAction(report) && !shouldShowComposerOptimistically) || isBlockedFromChat;

return (
        <>
            {shouldHideComposer && (
                <View
                    style={[
                        styles.chatFooter,
            . . .

The code that defines this logic is present in
src/libs/ReportUtils.ts

function canUserPerformWriteAction(report: OnyxEntry<Report>) {
    . . . 
    return !isArchivedRoom(report, reportNameValuePairs) && isEmptyObject(reportErrors) && report && isAllowedToComment(report) && !isAnonymousUser && canWriteInReport(report);
}

function canWriteInReport(report: OnyxEntry<Report>): boolean {
    if (Array.isArray(report?.permissions) && report?.permissions.length > 0) {
        return report?.permissions?.includes(CONST.REPORT.PERMISSIONS.WRITE);
    }

    return true;
}

This delay is processing of the OpenReport API causes the composer to appear and disappear.

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

On average it takes between 450ms to 600ms at my side for OpenReport API to respond. Then processing the response and checking the permissions takes another second, which in total adds upto 2 sec.

We can choose to not optimistically display the composer as an option. Or display the composer in the ReportFooter like now, but disable editing in the composer. I would choose the latter as that would give a graceful look and feel to the UI. Once the OpenReport response is processed and if PERMISSIONS contain WRITE option, we can enable editing. If not, hide the composer completely.

What alternative solutions did you explore? (Optional)

Reproduced the behaviour locally with main/HEAD.
Strangely in staging OpenReport API always returns response with

"permissions": [
                    "read",
                    "write",
                    "share"
                ],

Someone from backend can please let me know why OpenReport API is providing Write status when we are in tasks report action.

[github-actions]

@kpadmanabhan Your proposal will be dismissed because you did not follow the proposal template.

[kpadmanabhan]

@kpadmanabhan Your proposal will be dismissed because you did not follow the proposal template.

Nicely configured github-actions. I typed in most of the items with proposal template itself and saved as draft so that i do not loose it by mistake.

@kadiealexander : i hope the proposal wont be dismissed for this reason.

[melvin-bot]

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

[melvin-bot]

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

[kadiealexander]

@dukenv0307 please review the proposal above, thanks!

[nkdengineer]

Proposal

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

If user opens the read only task and writes something in the composer and sends it fast then user can send a comment before the composer hides

What is the root cause of that problem?

With the accountID being an odd number, the targetChatReport of the onboarding task is the Expensify system chat. This report has permission as ['read'] but we don't update the permission of the task report in optimistic data. Then composer of the task report is shown until OpenReport API is complete.

App/src/libs/actions/Report.ts

Lines 3231 to 3235 in b8539ab

	const isAccountIDOdd = AccountUtils.isAccountIDOddNumber(currentUserAccountID ?? 0);
	const targetEmail = isAccountIDOdd ? CONST.EMAIL.NOTIFICATIONS : CONST.EMAIL.CONCIERGE;
	const actorAccountID = PersonalDetailsUtils.getAccountIDsByLogins([targetEmail])[0];
	const targetChatReport = ReportUtils.getChatByParticipants([actorAccountID, currentUserAccountID]);

Another problem, targetChatReport is undefined when it's the system chat because we skip it here

App/src/libs/ReportUtils.ts

Line 5782 in b8539ab

if (!shouldIncludeGroupChats && !isOneOnOneChat(report)) {

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

1. We should add an extra parameter like shouldIncludeAllReport with the default value as false. And if it's true we will not skip any report. Then pass it as true here

// // Skip if it's not a 1:1 chat
if (!shouldIncludeGroupChats && !isOneOnOneChat(report) && !shouldIncludeAllReport) {
    return false;
}

// If we are looking for a group chat, then skip non-group chat report
if (shouldIncludeGroupChats && !isGroupChat(report) && !shouldIncludeAllReport) {
    return false;
}

App/src/libs/ReportUtils.ts

Line 5782 in b8539ab

if (!shouldIncludeGroupChats && !isOneOnOneChat(report)) {

2. Add permissions as targetChatReport?.permissions in the optimistic data of the task here report so the composer will not show briefly when we access to

permissions: targetChatReport?.permissions

App/src/libs/actions/Report.ts

Line 3348 in b8539ab

managerID: currentUserAccountID,

What alternative solutions did you explore? (Optional)

[dukenv0307]

Thank you for your proposals

@kpadmanabhan Your solution seems like a workaround to me
@nkdengineer I don't really understand your RCA. You said the targetChatReport of the onboarding task is the Expensify system chat so why targetChatReport is undefined when it's the system chat because we skip it here ?

Is the Expensify system chat undefined?

[nkdengineer]

I don't really understand your RCA. You said the targetChatReport of the onboarding task is the Expensify system chat so why targetChatReport is undefined when it's the system chat because we skip it here ?

@dukenv0307 I mean the system chat is undefined because we get the targetChatReport via getChatByParticipants function and this function returns false with the system chat report. The reason is getChatByParticipants function only checks the oneOnOne chat and group chat shouldIncludeGroupChats is true.

Is the Expensify system chat undefined?

This report exists in Onyx.

[kpadmanabhan]

@kpadmanabhan Your solution seems like a workaround to me

@dukenv0307 We have to not enable / show composer while reports are being loaded. Other alternative is to have backend expose a light-weight API explicitly defining the type of report clicked. This way we can decide whether to show or hide composer without having to parse all contents of OpenReport API response.

I tried doing the check for shouldHideComposer as the first action while processing the response. Still the time lag is enough for real quick hands to type a text in composer when it is optimistically displayed.

We have 2 factors that cause this behaviour here.

1. Network latency (due to a slow or poor connection)
2. Processing time for OpenReport API response.

Even if I short circuit point 2 as I mentioned above, point 1 will still lead to this behaviour.

In my opinion to display optimistically an enabled composer for all reports is not the wise approach, as it leads to issues like this.

UX may also choose to show an indication of composer loading until OpenReport is processed.

[dukenv0307]

@kpadmanabhan

Network latency (due to a slow or poor connection)

We can update the data optimistically. So it can work even in offline mode.

Proposal from @nkdengineer looks good to me.

We should update permission of task reports optimistically in

App/src/libs/actions/Report.ts

Line 3348 in b8539ab

managerID: currentUserAccountID,

One minor suggestion is to add !isSystemChat condition to getChatByParticipants instead of introducing the new params.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @nkdengineer 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

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

• fix: User can write and send a message onboarding tasks #46851

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

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

• @dukenv0307 requires payment automatic offer (Reviewer)
• @nkdengineer requires payment automatic offer (Contributor)

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

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

[kadiealexander]

Payouts due:

• Contributor: $250 @nkdengineer automatic offer
• Contributor+: $250 @dukenv0307 automatic offer

Upwork job is here.

[kadiealexander]

@dukenv0307 please complete the checklist!

[dukenv0307]

BugZero Checklist:

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

Regression tests:

1. Sign in with a new account
2. On the onboarding modal select "Manage my team's expenses"
3. Finish the onboarding flow by filling out business name and first name
4. Open expensify chat
5. Click on a task and type something fast and send it before the composer hides
6. Verify that: If the task thread is intended to be a read only thread then user should not be able to write and send anything in the composer regardless of the speed of typing

Do we 👍 or 👎