Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Handle error code 10 on Android #864

Merged
merged 4 commits into from
Jul 24, 2020
Merged

Handle error code 10 on Android #864

merged 4 commits into from
Jul 24, 2020

Conversation

henrytao-me
Copy link
Contributor

@henrytao-me henrytao-me commented Jul 23, 2020 •
edited
Loading

This PR handles error code 10 on Android by checking EN permission when accessing EN methods. It also extracts all exception in the app into a utils for convenience.

Ref #593

How to test?

Test in all cases: EN is OFF, Bluetooth is OFF, EN and Bluetooth are OFF.

Expected log

  • Error: performExposureStatusUpdate {"error": {"error": [Error: API_NOT_ENABLED], "message": "API_NOT_ENABLED"}} is called after performExposureStatusUpdate.
  • finalize is called at the end with: lastChecked.period should be the same before and after updating. last.Checked.timestamp should be different.
[Thu Jul 23 2020 15:26:05.754]  LOG      App.appInit() {}
[Thu Jul 23 2020 15:26:06.883]  LOG      updateExposureStatusInBackground {"exposureStatus": {"type": "monitoring"}}
[Thu Jul 23 2020 15:26:06.895]  LOG      registerPeriodicTask {"result": true}
[Thu Jul 23 2020 15:26:07.170]  LOG      getExposureConfiguration {"exposureConfigurationUrl": "http://192.168.0.24:8001/exposure-configuration/CA.json"}
[Thu Jul 23 2020 15:26:07.169]  LOG      Using downloaded exposureConfiguration. {}
[Thu Jul 23 2020 15:26:07.183]  LOG      Saving exposure configuration to secure storage. {}
[Thu Jul 23 2020 15:26:07.191]  LOG      retrieveDiagnosisKeys {"period": 0, "url": "http://192.168.0.24:8001/retrieve/302/00000/f3068242ab71ef941656bd16e81856eb034819cf52af520899513dad50bb1c72"}
[Thu Jul 23 2020 15:26:07.229]  LOG      performExposureStatusUpdate {"exposureConfiguration": {"attenuationLevelValues": [0, 0, 2, 2, 2, 2, 2, 2], "attenuationWeight": 50, "daysSinceLastExposureLevelValues": [0, 1, 1, 1, 1, 1, 1, 1], "daysSinceLastExposureWeight": 50, "durationLevelValues": [0, 0, 0, 0, 5, 5, 5, 5], "durationWeight": 50, "minimumRiskScore": 1, "transmissionRiskLevelValues": [1, 1, 1, 1, 1, 1, 1, 1], "transmissionRiskWeight": 50}, "keysFileUrls": ["/data/user/0/ca.gc.hcsc.canada.stopcovid/app_c2acf5e3-43d0-4c49-b2d9-7f67dd53f4c2/keys.zip"], "lastCheckedPeriod": 18466}
[Thu Jul 23 2020 15:26:07.354]  LOG      Error: performExposureStatusUpdate {"error": {"error": [Error: API_NOT_ENABLED], "message": "API_NOT_ENABLED"}}
[Thu Jul 23 2020 15:26:07.477]  LOG      finalize {"currentExposureStatus": {"lastChecked": {"period": 0, "timestamp": 1595532368901}, "type": "monitoring"}, "previousExposureStatus": {"type": "monitoring"}}
[Thu Jul 23 2020 15:26:07.484]  LOG      updatedExposureStatusInBackground {"exposureStatus": {"lastChecked": {"period": 0, "timestamp": 1595532368901}, "type": "monitoring"}}

  • If you already passed onboarding screen and enable EN framework:

    • Terminate the app.
    • Go to Settings, disable EN framework.
    • Open the app.
    • Observe js log.

  • If you are still in onboarding screen:

    • In ExposureNotificationServiceProvider, call updateExposureStatusInBackground immediately.
    • Open the app.
    • Observe js log.
    • Go through onboarding flow and DO NOT allow to turn on EN.
    • At home screen, observe js log.
    • Click Enable COVID Alert.
    • Allow to turn on EN.
    • Observe js log goes through as normal without Error: performExposureStatusUpdate. You might see it hangs at performExposureStatusUpdate because you don't have exposed TEKs or you are in exposed state.

@henrytao-me henrytao-me requested a review from a team as a code owner July 23, 2020 19:21
henrytao-me
henrytao-me commented Jul 23, 2020
View reviewed changes
src/services/ExposureNotificationService/ExposureNotificationService.ts
@@ -388,7 +388,7 @@ export class ExposureNotificationService {
}

private async processPendingExposureSummary() {
const summary = await this.exposureNotification.getPendingExposureSummary();
const summary = await this.exposureNotification.getPendingExposureSummary().catch(() => undefined);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unnecessary to try catch PendingExposureSummary. We can return undefined and the method will return early anyway.

@jeberhardt jeberhardt self-requested a review July 23, 2020 20:02
jeberhardt
jeberhardt reviewed Jul 23, 2020
View reviewed changes
android/app/src/main/java/app/covidshield/module/ExposureNotificationModule.kt Outdated
@@ -96,6 +103,10 @@ class ExposureNotificationModule(context: ReactApplicationContext) : ReactContex
@ReactMethod
fun detectExposure(configuration: ReadableMap, diagnosisKeysURLs: ReadableArray, promise: Promise) {
promise.launch(this) {
if (getStatusInternal() != Status.ACTIVE) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the use of getStatusInternal() at this point is overreaching what is required. It could be possible to perform the detectExposure functions with Bluetooth turned off, but with the exposureNotificationClient enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I just tested on Android when EN is ON but Bluetooth is OFF, the framework still can run detectExposure. However, I wouldn't think it is important to separate the condition between EN and Bluetooth. It's clearly that in order for the app to work properly, both EN and Bluetooth have to be ON. I prefer to keep same logic for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeberhardt points out this

Some people may turn off Bluetooth to "save battery", like when they are at home, and only turn it on when they go out. It's possible. I don't think consistency in the logic should be more important than specific functionality.

I will test all changes again with just EN.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a result of this #866 (review), I would prefer to check both EN + Bluetooth. This is a low risk approach, imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested again with just checking EN. It works fine. 👍 cc @jeberhardt @timarney

@henrytao-me henrytao-me force-pushed the issue/593/error-code-10 branch 2 times, most recently from d658729 to 9af19f8 Compare July 24, 2020 13:09
@timarney
Copy link
Member

@henrytao-me

I'm still testing but wondering what comes back from the Framework in this case

@timarney
Copy link
Member

We'll handle #864 (comment) in a separate PR.

@henrytao-me henrytao-me merged commit f3d4fd0 into master Jul 24, 2020
@henrytao-me henrytao-me deleted the issue/593/error-code-10 branch July 24, 2020 14:16
@henrytao-me henrytao-me mentioned this pull request Jul 24, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jeberhardt jeberhardt jeberhardt left review comments

@timarney timarney timarney approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@henrytao-me @timarney @jeberhardt