[HOLD for payment 2023-10-13] [$500] mWeb-Conversation-User A in offline typing message, User B is shown "user A is typing" #28063

izarutskaya · 2023-09-23T11:47:26Z

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:

Go to https://staging.new.expensify.com/
As user A, tap profile- Preferences
Toggle on forced offline
Open 1:1 with user B
In other device, as user B, open 1:1 with user A
As user A, in offline type messages to user B and note user B conversation page

Expected Result:

User A in offline typing message, User B must not be shown "user A is typing" in conversation page.

Actual Result:

User A in offline typing message, User B is shown "user A is typing" in conversation page.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

Version Number: 1.3.73-0

Reproducible in staging?: Y

Reproducible in production?: No "Force offline" mode

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

Bug6211463_2user_shown_user_a_typing.mp4

Bug6211463_offline_android_user_a_typing_shown_to_userb.mp4

Bug6211463_1offline_mweb.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause-Internal Team

Slack conversation: @

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01650cee09674fc251
Upwork Job ID: 1707005436379205632
Last Price Increase: 2023-09-27

melvin-bot · 2023-09-23T11:47:31Z

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

melvin-bot · 2023-09-23T11:47:33Z

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

OSBotify · 2023-09-23T11:49:34Z

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

melvin-bot · 2023-09-23T11:49:37Z

Triggered auto assignment to @joelbettner (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

akinwale · 2023-09-23T12:20:21Z

Proposal

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

The "user is typing" event is broadcasted by a user's client even if the client has force offline mode enabled.

What is the root cause of that problem?

Pusher sends socket events even if force offline mode is enabled.

App/src/libs/Pusher/pusher.js

Lines 315 to 324 in 5aa7f6d

    
           function sendEvent(channelName, eventName, payload) { 
        
               // Check to see if we are subscribed to this channel before sending the event. Sending client events over channels 
        
               // we are not subscribed too will throw errors and cause reconnection attempts. Subscriptions are not instant and 
        
               // can happen later than we expect. 
        
               if (!isSubscribed(channelName)) { 
        
                   return; 
        
               } 
        
               socket.send_event(eventName, payload, channelName); 
        
           }

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

Solution 1
Similar to what we have in the bindEventToChannel method, modify the sendEvent method to perform an early return if shouldForceOffline is true.

function sendEvent(channelName, eventName, payload) {
    // Check to see if we are subscribed to this channel before sending the event. Sending client events over channels
    // we are not subscribed too will throw errors and cause reconnection attempts. Subscriptions are not instant and
    // can happen later than we expect.
    if (!isSubscribed(channelName)) {
        return;
    }

+    if (shouldForceOffline) {
+        Log.info('[Pusher] Ignoring a send event because shouldForceOffline = true');
+        return;
+    }

   // ... rest of the method
}

Solution 2
In ReportActionsView, add a useEffect hook to unsubscribe to the pusher channel based on the network.isOffline prop. This, however does not cover all possible pusher events, so Solution 1 is preferable.

const subscribeToReportTypingEvents = () => {
    const didCreateReportSuccessfully = !props.report.pendingFields || (!props.report.pendingFields.addWorkspaceRoom && !props.report.pendingFields.createChat);
    if (!didSubscribeToReportTypingEvents.current && didCreateReportSuccessfully) {
        Report.subscribeToReportTypingEvents(reportID);
        didSubscribeToReportTypingEvents.current = true;
    }
}

useEffect(() => {
    if (network.isOffline) {
        Report.unsubscribeFromReportChannel(reportID);
        didSubscribeToReportTypingEvents.current = true;
        return;
    }
    
    subscribeToReportTypingEvents();
}, [network.isOffline, reportID]);

What alternative solutions did you explore? (Optional)

None.

joelbettner · 2023-09-25T03:28:09Z

I don't think this is an hourly, since I believe this will only ever happen on staging (I THINK we always hide the TestToolMenu in production...I could be wrong). I don't think this was caused by the recent deploy. I think this has been happening ever since we introduced the Force Offline mode. @izarutskaya is this something that you just came across, or is this test something that happens at every deploy?

@neil-marcellini @roryabraham it looks like you are the two fellas who have the most background in the Force Offline mode. Care to weigh in here?

mountiny · 2023-09-25T08:01:24Z

Correct, I agree this is not a deploy blocker, also I think @neil-marcellini knows about this problem from before, I believe I remember some slack thread about this specifically.

izarutskaya · 2023-09-27T11:43:12Z

No, we do not this test at every deploy. This was discovered during another test. Thank you

melvin-bot · 2023-09-27T12:13:07Z

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

melvin-bot · 2023-09-27T12:13:13Z

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

NicMendonca · 2023-09-27T12:13:49Z

^ setting as external

JawadSadiq01 · 2023-09-27T16:08:23Z

Proposal

Please re-state the problem that we are trying to solve in this issue.
Conversation-User A in offline typing message, User B is shown "user A is typing".

What is the root cause of that problem?
When "User A" types a message, there is no code that checks either he had activated the Force Offline. After Force Offline activation, when "User A" types message, then on each character typing, an event "debouncedBroadcastUserIsTyping" triggers. After this event triggering, the "USER B" sees that "USER A" is typing.

What changes do you think we should make in order to solve the problem?
You just need to prevent the call of "debouncedBroadcastUserIsTyping", when he set Force Offline.
Just add the below code in ComposerWithSuggestions.js file.

Please see the attachments below, for knowing where to add the below code in ComposerWithSuggestions.js file.

let isForcedOffline = false;

Onyx.connect({
    key: ONYXKEYS.NETWORK,
    callback: (val) => {
        isForcedOffline = val && val.isOffline;
    },
});

On ComposerWithSuggestions.js#216, update the if condition like below

if (newComment && !isForcedOffline) {
   debouncedBroadcastUserIsTyping(reportID);
}

Video Recording
https://drive.google.com/file/d/1G6wSTFZfK1qvSyK2CcB4L4DEIZ_e6WWK/view?usp=drivesdk

melvin-bot · 2023-10-03T18:57:51Z

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

melvin-bot · 2023-10-03T18:57:53Z

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

akinwale · 2023-10-04T04:29:47Z

@ntdiary My PR is ready for review. Thanks.

melvin-bot · 2023-10-06T07:43:22Z

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

melvin-bot · 2023-10-06T07:43:26Z

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

fix: "user is typing" event sent when force offline mode is enabled #28786

If no regressions arise, payment will be issued on 2023-10-13. 🎊

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:

@akinwale requires payment
@ntdiary requires payment

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2023-10-06T07:43:28Z

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:

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

NicMendonca · 2023-10-13T14:08:13Z

@akinwale @ntdiary bump on BZ check list ^^

Also can you both apply to the job please? https://www.upwork.com/jobs/~01650cee09674fc251

akinwale · 2023-10-13T14:23:10Z

@akinwale @ntdiary bump on BZ check list ^^

Also can you both apply to the job please? https://www.upwork.com/jobs/~01650cee09674fc251

Applied. Thanks!

I believe it's the C+ that's supposed to complete the BZ check list. Weird that my name is in there. 😅

ntdiary · 2023-10-13T14:26:59Z

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:

[@akinwale / @ntdiary] The PR that introduced the bug has been identified. Link to the PR: Add a switch on dev and staging to force offline mode #12135

[@akinwale / @ntdiary] 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: https://github.com/Expensify/App/pull/12135/files#r1358338867

[@akinwale / @ntdiary] 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: No need

[@akinwale / @ntdiary] Determine if we should create a regression test for this bug. No need

[@akinwale / @ntdiary] 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. No need

[@NicMendonca] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

I believe it's the C+ that's supposed to complete the BZ check list. Weird that my name is in there. 😅

I guess it's because you are already in C+ as well. 😄

NicMendonca · 2023-10-13T19:05:29Z

Payment summary:

Reporter: N/A
Contributor: @akinwale - $750 (paid via upwork)
Contributor +: @ntdiary - $750 (paid via upwork)

NicMendonca · 2023-10-13T19:06:06Z

Sent you both an offer!

akinwale · 2023-10-13T19:29:26Z

Sent you both an offer!

Accepted. Thanks!

NicMendonca · 2023-10-16T13:26:28Z

both paid, thank you!

izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 23, 2023

melvin-bot bot assigned NicMendonca Sep 23, 2023

OSBotify mentioned this issue Sep 23, 2023

Deploy Checklist: New Expensify 2023-09-22 #28013

Closed

96 tasks

github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Sep 23, 2023

melvin-bot bot assigned joelbettner Sep 23, 2023

mountiny added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Sep 25, 2023

melvin-bot bot added the Overdue label Sep 27, 2023

NicMendonca added the External Added to denote the issue can be worked on by a contributor label Sep 27, 2023

melvin-bot bot changed the title ~~mWeb-Conversation-User A in offline typing message, User B is shown "user A is typing"~~ [$500] mWeb-Conversation-User A in offline typing message, User B is shown "user A is typing" Sep 27, 2023

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 bot assigned ntdiary Sep 27, 2023

melvin-bot bot removed the Overdue label Sep 27, 2023

neil-marcellini assigned akinwale Oct 3, 2023

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 3, 2023

akinwale mentioned this issue Oct 4, 2023

fix: "user is typing" event sent when force offline mode is enabled #28786

Merged

59 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Oct 4, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 6, 2023

melvin-bot bot changed the title ~~[$500] mWeb-Conversation-User A in offline typing message, User B is shown "user A is typing"~~ [HOLD for payment 2023-10-13] [$500] mWeb-Conversation-User A in offline typing message, User B is shown "user A is typing" Oct 6, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Oct 6, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 12, 2023

ntdiary mentioned this issue Oct 13, 2023

Add a switch on dev and staging to force offline mode #12135

Merged

95 tasks

melvin-bot bot added the Overdue label Oct 16, 2023

NicMendonca closed this as completed Oct 16, 2023

melvin-bot bot removed the Overdue label Oct 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2023-10-13] [$500] mWeb-Conversation-User A in offline typing message, User B is shown "user A is typing" #28063

[HOLD for payment 2023-10-13] [$500] mWeb-Conversation-User A in offline typing message, User B is shown "user A is typing" #28063

izarutskaya commented Sep 23, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Sep 23, 2023

melvin-bot bot commented Sep 23, 2023

OSBotify commented Sep 23, 2023

melvin-bot bot commented Sep 23, 2023

akinwale commented Sep 23, 2023 •

edited

Loading

joelbettner commented Sep 25, 2023

mountiny commented Sep 25, 2023

izarutskaya commented Sep 27, 2023

melvin-bot bot commented Sep 27, 2023

melvin-bot bot commented Sep 27, 2023

NicMendonca commented Sep 27, 2023

JawadSadiq01 commented Sep 27, 2023 •

edited

Loading

melvin-bot bot commented Oct 3, 2023

melvin-bot bot commented Oct 3, 2023

akinwale commented Oct 4, 2023 •

edited

Loading

melvin-bot bot commented Oct 6, 2023

melvin-bot bot commented Oct 6, 2023

melvin-bot bot commented Oct 6, 2023

NicMendonca commented Oct 13, 2023 •

edited

Loading

akinwale commented Oct 13, 2023

ntdiary commented Oct 13, 2023

NicMendonca commented Oct 13, 2023

NicMendonca commented Oct 13, 2023

akinwale commented Oct 13, 2023

NicMendonca commented Oct 16, 2023

[HOLD for payment 2023-10-13] [$500] mWeb-Conversation-User A in offline typing message, User B is shown "user A is typing" #28063

[HOLD for payment 2023-10-13] [$500] mWeb-Conversation-User A in offline typing message, User B is shown "user A is typing" #28063

Comments

izarutskaya commented Sep 23, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Sep 23, 2023

melvin-bot bot commented Sep 23, 2023

Bug0 Triage Checklist (Main S/O)

OSBotify commented Sep 23, 2023

melvin-bot bot commented Sep 23, 2023

akinwale commented Sep 23, 2023 • 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)

joelbettner commented Sep 25, 2023

mountiny commented Sep 25, 2023

izarutskaya commented Sep 27, 2023

melvin-bot bot commented Sep 27, 2023

melvin-bot bot commented Sep 27, 2023

NicMendonca commented Sep 27, 2023

JawadSadiq01 commented Sep 27, 2023 • edited Loading

Proposal

melvin-bot bot commented Oct 3, 2023

melvin-bot bot commented Oct 3, 2023

akinwale commented Oct 4, 2023 • edited Loading

melvin-bot bot commented Oct 6, 2023

melvin-bot bot commented Oct 6, 2023

melvin-bot bot commented Oct 6, 2023

NicMendonca commented Oct 13, 2023 • edited Loading

akinwale commented Oct 13, 2023

ntdiary commented Oct 13, 2023

NicMendonca commented Oct 13, 2023

NicMendonca commented Oct 13, 2023

akinwale commented Oct 13, 2023

NicMendonca commented Oct 16, 2023

izarutskaya commented Sep 23, 2023 •

edited by melvin-bot bot

Loading

akinwale commented Sep 23, 2023 •

edited

Loading

JawadSadiq01 commented Sep 27, 2023 •

edited

Loading

akinwale commented Oct 4, 2023 •

edited

Loading

NicMendonca commented Oct 13, 2023 •

edited

Loading