This repository has been archived by the owner on Jun 24, 2022. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR attempts to fix push notification.
What could when wrong on master?
Initial state is incorrect:
Since we start persisting exposure status, we split
start
intostart
andinit
. Because the initial state is retrieved from local storage, it is async. So, if the app is running in background (through background job), master just callupdateExposureStatusInBackground
without calling init. Based on this assumption,On Android: ✅
We call
init
before callingupdateExposureStatusInBackground
which means Android should receive notification. If there is no notification, it could be different issue.On iOS: ❌
We don't call
init
inbackgroundScheduler.registerPeriodicTask
which means in some race condition, iOS won't have correct initial state.Logic to show notification:
Also the logic for showing exposed notification is quite fragile to break https://github.com/cds-snc/covid-shield-mobile/blob/master/src/services/ExposureNotificationService/ExposureNotificationService.ts#L151. Essentially, we want to send exposure notification to user only once but the way we are doing it is checking the status from
monitoring
toexposed
.This will break when: If user finishes onboarding and minimize the app, the app still runs in but without
updateExposureStatusInBackground
. It means if the status is changed toexposed
, the next timeupdateExposureStatusInBackground
is run, it won't show notification. In this case, user doesn't know that the app is set to exposed already.Ref #570