[HOLD for payment 2023-10-02] [$500] IOS - Push notification is sent to the device even though the app is logged out.

[kbecciv]

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. Log in to Account A.
2. Turn OFF the network connection to enter OFFLINE mode.
3. LOG OUT
4. While the app is still in OFFLINE mode, send a message from another account to Account A (IMPORTANT STEP).
5. Turn ON the network connection.
6. Observer: Push notification appears.

Expected Result:

Push notification should not be sent if app is logged out

Actual Result:

Push notification is sent to the device even though the app is logged out.

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: 1.3.58.0 (tested on 1.3.62-3)
Reproducible in staging?: Yes
Reproducible in production?: n/a
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

Push-notification-issue.1.MP4

Expensify/Expensify Issue URL:
Issue reported by: @tranvantoan-qn
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692691382974179

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0134d9932ee5c7e512
• Upwork Job ID: 1698485880458186752
• Last Price Increase: 2023-09-03
• Automatic offers:
• hungvu193 | Contributor | 26547257
• tranvantoan-qn | Reporter | 26547259

[melvin-bot]

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

[melvin-bot]

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

[tranvantoan-qn]

My Test/Reproduction note, hope it helps:

• Make sure that we allowed the app to recieve Notification by turning Notification permission ON (permission popup will be displayed after installing and openning app first time)
• The step 4: send a message from another account to Account A while the app is in OFFLINE mode - is a crucial step
• To make sure Push Notification will be sent properly via Firebase, please check Production build

[hungvu193]

Proposal

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

IOS - Push notification is sent to the device even though the app is logged out.

What is the root cause of that problem?

Let take a look at the signOut function:

App/src/libs/actions/Session/index.js

Lines 41 to 56 in 6e17b64

	function signOut() {
	Log.info('Flushing logs before signing out', true, {}, true);
	API.write('LogOut', {
	// Send current authToken because we will immediately clear it once triggering this command
	authToken: NetworkStore.getAuthToken(),
	partnerUserID: lodashGet(credentials, 'autoGeneratedLogin', ''),
	partnerName: CONFIG.EXPENSIFY.PARTNER_NAME,
	partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD,
	shouldRetry: false,
	});
	clearCache().then(() => {
	Log.info('Cleared all chache data', true, {}, true);
	});
	Timing.clearData();
	}

When user signed out offline, the API request will be pushed to the queue and wait for internet connection, after online again this api will be trigged and logged user out (including the token for push notification), in the mean time while signOut was calling if another user sent a message and user still can receive it because user was still logging out. This is the RCA. That why if you turned on the internet connection for a while and sent another message, you wouldn't receive the notification.

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

I think the most simple way to resolve this issue is adding more condition to our shouldShowPushNotification function.

// call this when signout
shouldShowPushNotification {
 if (!isAuthenticated) {return false}
}

What alternative solutions did you explore? (Optional)

Add the following line to our PushNotification.deregister (index.native.js) function:

     Airship.push.setUserNotificationsEnabled(false);
    Airship.push.iOS.setForegroundPresentationOptionsCallback(() => Promise.resolve([]));
    Airship.push.android.setForegroundDisplayPredicate((pushPayload) => Promise.resolve(() => false));

[sophiepintoraetz]

Will test on Monday!

[melvin-bot]

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

[melvin-bot]

Current assignee @sophiepintoraetz is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[sophiepintoraetz]

I'm able to reproduce this:

RPReplay_Final1693785333.MOV

[thesahindia]

@hungvu193's proposal looks good to me!

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @thesahindia Please request via NewDot manual requests for the Reviewer role ($500)

[melvin-bot]

📣 @hungvu193 🎉 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]

📣 @tranvantoan-qn 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[hungvu193]

Waiting for final review from @roryabraham

[roryabraham]

I'm actually really confused by the RCA here... it seems like we're missing something:

• When the user signs out, we actually call Session.signOutAndRedirectToSignIn.
• redirectToSignIn clears persisted requests. So when does the LogOut API call factually happen? Does it actually happen at all?
• When we clear storage and redirect to sign-in (still offline), we should be triggering PushNotification.deregister – in fact that's the function you're leveraging in your PR. Why is Airship.removeAllListeners insufficient to prevent the notifications from being received?

[hungvu193]

I'm actually really confused by the RCA here... it seems like we're missing something:

• When we clear storage and redirect to sign-in (still offline), we should be triggering PushNotification.deregister – in fact that's the function you're leveraging in your PR. Why is Airship.removeAllListeners insufficient to prevent the notifications from being received?

I realized that PushNotification.deregister didn't clear all the EventType.PushReceived, when we first opened app or reload it, we called init 2 times. Here's the log I saw on Flipper.

So we will still receive the notification in the meantime api was calling.

• When the user signs out, we actually call Session.signOutAndRedirectToSignIn.
• redirectToSignIn clears persisted requests. So when does the LogOut API call factually happen? Does it actually happen at all?

When I checked the logs, I saw that we will call Logout after we reconnected to the network, that's also little weird to me because we already clear all the persisted request.

Overall, the current solution is just to disable the notification when user is logged out. Another solution can be creating a variable inside PushNotification/index.native.js to check if we already init the notification, once it's init we shouldn't call init again. How do you think?

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @hungvu193 got assigned: 2023-09-07 15:06:35 Z
• when the PR got merged: 2023-09-20 13:24:38 UTC
• days elapsed: 8

On to the next one 🚀

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

• Fix: prevent receiving notification when logging out #26973

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

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:

• @hungvu193 requires payment offer (Contributor)
• @thesahindia does not require payment (Eligible for Manual Requests)
• @tranvantoan-qn requires payment offer (Reporter)

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]

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:

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

[sophiepintoraetz]

@thesahindia, please request $500 once you have completed the checklist.

[thesahindia]

It was an improvement. Not a regression. We just need to add a test case.

Steps:-

1. Log in to account A.
2. Turn Off the network connection to enter offline mode.
3. Log out
4. While the app is still in offline mode, send a message from another account to Account A
5. Turn on the network connection.
6. Verify that notification doesn't appear

[JmillsExpensify]

$500 payment approved for @thesahindia based on this comment.