Auto add authorities #587

Patrick-Erichsen · 2020-04-20T03:39:57Z

Description

This PR creates a new notification and filtering feature for Healthcare Authorities in a user's location.

Issue

#425

How to test

Unit

npx jest HCAService.spec.js

Integration

Visit the Choose health authority screen and press the Add Trusted Source button. Press the GPS filter switch and confirm that all health authorities are removed (the current live yaml file does not include a bounds field so by default there will be no authorities in your area)

Comments / Concerns

I added a line to the BackgroundTaskService that will send a push notification to the user every 12 hours if there is an authority in their region that they haven't subscribed to.
- Is 12 hours too frequent? I believe we can only have a single BackgroundTaskService (ours runs every 12 hours to check for intersections), so I'm not sure how else we could check at a pre-defined interval.
- What if a user intentionally decides not to subscribe to a particular authority? Currently, they will continue to get a notification every 12 hours to subscribe.
It would be nice if upon receiving a push notification, we could direct users to the Choose health authority screen. However, the only way to do that on iOS as far as I can tell is through the deprecated alertAction param. I believe that we can do this for Android through the action field for local notifications, but I'm not sure if we want that inconsistency between platforms.
I'm not sure of a good way to test the push notification piece of this PR without changing some code around to force the notification and then revert the changes. I was thinking of doing a similar approach that we use for intersections but would like some feedback before implementing. @tstirrat - feedback here would be appreciated.
Currently, the authorities list does not contain bounds info. @penrods - how should we go about updating that file on raw.githack, and in general, is there a way that we're going to force authorities to include bounds? And in the case where a user uploads an authority with the Add authority via URL option, will we just allow them to not include bounds?

Translation Notes

There are several new strings for translation.

Signed-off-by: Patrick Erichsen <[email protected]>

Tweak the text of the toggle UI. Also add a localization string for manual URL entry.

Several light changes to this: * The GPS filter toggle wasn't behaving properly * Made the entire toggle line look different and react to a touch Also: * Added localization string for the text that appears when entering URL manually (it was hard-coded)

penrods · 2020-04-20T09:00:44Z

I've looked at the ChooseProvider.js screen and it works well! Awesome work, @Patrick-Erichsen. I tweaked the UI and behavior a little bit on this PR branch.

I haven't had time to verify the auto-subscription notification yet.

penrods · 2020-04-20T09:16:10Z

Answering some of your questions:

We can require bounding boxes for authorities and validate it when a new authority requests inclusion on that list via a PR. I have already added tooling to the publisher and will added the bounding box for Haiti under the https://raw.githubusercontent.com/tripleblindmarket/safe-places/develop/healthcare-authorities.yaml.
The manual addition of a healthcare authority capability does not require a GPS boundry, correct. It is to allow a user to hook to a safe-paths.json source that for some reason is not in the YAML Global Registry yet.

NOTE: We need to switch to the CDN version of the YAML before we push this code to the app stores.

app/locales/en.json

alpita-masurkar

please fix this: typo in numAuthories in en.json

Signed-off-by: Patrick Erichsen <[email protected]>

Patrick-Erichsen · 2020-04-20T14:05:51Z

please fix this: typo in numAuthories in en.json

Fixed - that same typo was copy/pasted in another file as well. Thanks for catching.

Patrick-Erichsen · 2020-04-20T14:19:50Z

The linter is failing due to an error on no-mocks-import from these two lines:

import * as mockHCA from '../__mocks__/mockHCA';
import {
  mockNullUserLocHistory,
  mockUserLocHistory,
} from '../__mocks__/mockUserLocHistory';

Anyone mind if I just ignore that rule for those lines? I think it's meant to prevent actual module mocks - this is just mock data.

kenpugsley · 2020-04-20T15:11:11Z

The linter is failing due to an error on no-mocks-import from these two lines:
import * as mockHCA from '../__mocks__/mockHCA';
import {
  mockNullUserLocHistory,
  mockUserLocHistory,
} from '../__mocks__/mockUserLocHistory';
Anyone mind if I just ignore that rule for those lines? I think it's meant to prevent actual module mocks - this is just mock data.

@tstirrat - thoughts on this?

kenpugsley · 2020-04-20T15:12:04Z

Added Do Not Merge (yet) as we need to hold off on merging this until the branch for v1 is complete.

tstirrat · 2020-04-20T15:28:35Z

a rule override is totally fine given that you're not double importing a module mock

kenpugsley · 2020-04-20T22:01:54Z

I think there's still some UI work to do on this. Feel free to remove the Do Not Merge (yet) label when the UI is solid and we think we're in a solid place.

Patrick-Erichsen · 2020-04-20T22:48:10Z

Based on the standup call today, I'm going to do a bit of refactoring on this:

Create an "Auto-Subscribe" functionality and add it to the onboarding screens
If a user is auto subscribed, the push notification will notify them of their new authorities they were automatically subscribed to, rather than just notifying them that there are authorities in their area.

Signed-off-by: Patrick Erichsen <[email protected]>

tstirrat · 2020-04-24T21:21:08Z

It would be nice if upon receiving a push notification, we could direct users to the Choose health authority screen. However, the only way to do that on iOS as far as I can tell is ...

Some contextual linking on a platform is better than none. So I'd be in favor of doing as much as possible here (but in another PR)

tstirrat

Nothing major, great stuff on this!

app/constants/storage.js

app/helpers/Intersect.js

tstirrat · 2020-04-24T21:23:55Z

app/locales/en.json

@@ -6,11 +6,19 @@
    "authorities_desc": "Choose trusted healthcare authorities in your area to obtain exposure data. Either select a name from the global registry, or enter the web address provided by an authority which has implemented Safe Paths.",
    "authorities_input_placeholder": "Paste your URL here",
    "authorities_no_sources": "No data source yet",
+    "authorities_new_in_area_title": "New Healthcare Authority In Your Region",


Take screenhots of these new strings so we can upload to lokalise

Where should we be placing these screenshots? This is used in a push notification - so we want a screenshot of that notification to upload to Lokalise?

app/locales/en.json

app/services/HCAService.js

app/views/onboarding/Onboarding5.js

e2e/InsufficientLocationPermissions.spec.js

e2e/NoAuthSubscriptionPermission.spec.js

e2e/helpers/onboarding.js

app/views/ChooseProvider.js

tstirrat · 2020-04-24T23:41:55Z

app/locales/en.json

@@ -51,6 +61,9 @@
    "launch_notif_header": "Notifications will let you know if you cross paths with an infected person.",
    "launch_notif_subheader": "We won't bother you except to share updates on your potential exposure risks.",
    "launch_notification_access": "Allow notifications",
+    "launch_authority_header": "Healthcare Authorities will give us the local data to know if you cross paths with an infected person.",
+    "launch_authority_subheader": "Automatically subscribe to receive the latest updates from Healthcare Authorities in your area.",
+    "launch_authority_access": "Health authority subscription",


This is my main issue, it's ambiguous on the final screen:

I like you're proposed update: Subscribe to nearby authorities. I think that along with the longer subtext in that screen should be enough context for a user. Maybe this is something, in particular, we can ask the testers to give us thoughts on?

Diarmid could probably weigh in ahead of time too

Signed-off-by: Patrick Erichsen <[email protected]>

…ivate-kit into auto-add-authorities

Patrick-Erichsen · 2020-04-26T00:32:14Z

@tstirrat I feel pretty solid at this point. Think it's ready to get in front of some testers ASAP.

Signed-off-by: Patrick Erichsen <[email protected]>

kenpugsley · 2020-04-26T22:29:51Z

I'll be doing a technical and functional review tonight. I think we need to open the discussion on whether or not we've covered this well enough for inclusion in v1.0.1.

Signed-off-by: Patrick Erichsen <[email protected]>

Patrick-Erichsen · 2020-04-27T02:02:12Z

e2e tests are failing right now due to the missing Haitiain language strings. Once this is merged I'll open a new PR to only run the full suite of language tests on a merge to fix this issue.

tstirrat · 2020-04-27T06:01:46Z

@kenpugsley we checked with Diarmid and there were no specific blockers getting this in.

One desired enhancement after this, is to immediately auto sub to any nearby HCAs when choosing the auto option, but this requires a refactor of the location tracking stuff as we've mentioned elsewhere (a non trivial job).

tstirrat · 2020-04-27T06:05:37Z

@Patrick-Erichsen I think the label is too long on the onboard 5 screen:

I fixed it so it wraps if too long, but was not quire sure what to do with the vertical height of the Onboarding5 screen.

if we can keep the checkbox line to a single line, it's not quite ideal but it would probably prevent much of this wrapping:

(galaxy nexus emulator)

tstirrat · 2020-04-27T06:10:10Z

Still a problem on nexus4:

Even if I make it single line :(

I suspect we could merge and fast follow with some kind of text hack that uses less text on small devices.... as long as you pretty pretty pinky swear to look into it straight after I'd still love to get this PR in to prevent as many merge conflicts etc.

Signed-off-by: Patrick Erichsen <[email protected]>

Patrick-Erichsen · 2020-04-27T13:41:32Z

Pushed up a fix to instead make the subheader text width .8 instead of .55. Given how long the text was it felt like .55 was too narrow.

tstirrat · 2020-04-27T16:18:55Z

Pushed up a fix to instead make the subheader text width .8 instead of .55. Given how long the text was it felt like .55 was too narrow.

If you make a visual change provide a screenshot so that I don't have to check out everything and rebuild just to see the text tweak you did :)

Patrick-Erichsen · 2020-04-27T20:57:47Z

@tstirrat

* develop: Auto add authorities (#587) Clean up more eslint issues (#678) Make nav back arrow color theme aware (#686) yarn run-ios now runs pod install (#681) Fix settings icon on iOS 12, replace with new icon (#687) Feature/creole tests (#690)

* develop: Fix "plural" keys, add i18n checks for new PRs (#648) Auto add authorities (#587) Clean up more eslint issues (#678) Make nav back arrow color theme aware (#686) yarn run-ios now runs pod install (#681) Fix settings icon on iOS 12, replace with new icon (#687) Feature/creole tests (#690) Fix formatting EULAs (#671) [i18n] Don't translate terms_of_use_url (#642) Fix background to run to edge of screen on iOS (#682)

Patrick-Erichsen added 5 commits April 18, 2020 21:50

Add HCAService with test suite

478eb47

Signed-off-by: Patrick Erichsen <[email protected]>

Add tests

1ee9ba9

Signed-off-by: Patrick Erichsen <[email protected]>

Prompt for new HCAs regardless of saved count

fd1c794

Signed-off-by: Patrick Erichsen <[email protected]>

Convert to push notification

240ad8a

Signed-off-by: Patrick Erichsen <[email protected]>

Fix merge conflicts

396356f

Signed-off-by: Patrick Erichsen <[email protected]>

penrods self-requested a review April 20, 2020 05:55

Steve Penrod added 2 commits April 20, 2020 03:51

Tweak filter text and add localization

a4a9ad0

Tweak the text of the toggle UI. Also add a localization string for manual URL entry.

Fix behavior and tweak UI

108237e

Several light changes to this: * The GPS filter toggle wasn't behaving properly * Made the entire toggle line look different and react to a touch Also: * Added localization string for the text that appears when entering URL manually (it was hard-coded)

alpita-masurkar reviewed Apr 20, 2020

View reviewed changes

app/locales/en.json Outdated Show resolved Hide resolved

alpita-masurkar requested changes Apr 20, 2020

View reviewed changes

Fix typo

93779ff

Signed-off-by: Patrick Erichsen <[email protected]>

Merge branch 'develop' into auto-add-authorities

9ab37cb

kenpugsley added Do Not Merge (yet) Hold off on merging! This can be used for various reasons, Work in Progress, outside legal review... For next next release labels Apr 20, 2020

Patrick-Erichsen added 4 commits April 20, 2020 21:36

Fix merge conflicts

4be0b35

Signed-off-by: Patrick Erichsen <[email protected]>

Merge branch 'develop' into auto-add-authorities

879f9b8

Fix merge conflicts

661c7b6

Signed-off-by: Patrick Erichsen <[email protected]>

Merge branch 'develop' into auto-add-authorities

f830c0f

tstirrat suggested changes Apr 24, 2020

View reviewed changes

tstirrat reviewed Apr 24, 2020

View reviewed changes

app/views/ChooseProvider.js Outdated Show resolved Hide resolved

tstirrat reviewed Apr 24, 2020

View reviewed changes

Patrick-Erichsen added 5 commits April 25, 2020 18:43

Incorporate PR review feedback

867e3d9

Fix e2e tests

388f943

Signed-off-by: Patrick Erichsen <[email protected]>

Merge branch 'develop' into auto-add-authorities

019c3d4

Language string updates

f67552c

Signed-off-by: Patrick Erichsen <[email protected]>

Merge branch 'auto-add-authorities' of github.com:Patrick-Erichsen/pr…

800fde8

…ivate-kit into auto-add-authorities

Fix unit tests

d9a25d8

Signed-off-by: Patrick Erichsen <[email protected]>

Merge develop

5b7dea0

Signed-off-by: Patrick Erichsen <[email protected]>

tstirrat previously approved these changes Apr 27, 2020

View reviewed changes

tstirrat dismissed their stale review via dfd9a02 April 27, 2020 06:01

Make subheader text wider

6354db8

Signed-off-by: Patrick Erichsen <[email protected]>

Patrick-Erichsen closed this Apr 27, 2020

Patrick-Erichsen reopened this Apr 27, 2020

tstirrat approved these changes Apr 27, 2020

View reviewed changes

tstirrat merged commit 8e68422 into Path-Check:develop Apr 27, 2020

Patrick-Erichsen deleted the auto-add-authorities branch April 27, 2020 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto add authorities #587

Auto add authorities #587

Patrick-Erichsen commented Apr 20, 2020 •

edited by penrods

Loading

penrods commented Apr 20, 2020

penrods commented Apr 20, 2020

alpita-masurkar left a comment

Patrick-Erichsen commented Apr 20, 2020

Patrick-Erichsen commented Apr 20, 2020

kenpugsley commented Apr 20, 2020

kenpugsley commented Apr 20, 2020 •

edited

Loading

tstirrat commented Apr 20, 2020

kenpugsley commented Apr 20, 2020

Patrick-Erichsen commented Apr 20, 2020 •

edited

Loading

tstirrat commented Apr 24, 2020 •

edited

Loading

tstirrat left a comment

tstirrat Apr 24, 2020

Patrick-Erichsen Apr 25, 2020

tstirrat Apr 24, 2020

Patrick-Erichsen Apr 25, 2020

tstirrat Apr 25, 2020

Patrick-Erichsen commented Apr 26, 2020

kenpugsley commented Apr 26, 2020

Patrick-Erichsen commented Apr 27, 2020

tstirrat commented Apr 27, 2020

tstirrat commented Apr 27, 2020

tstirrat commented Apr 27, 2020

Patrick-Erichsen commented Apr 27, 2020

tstirrat commented Apr 27, 2020

Patrick-Erichsen commented Apr 27, 2020

Auto add authorities #587

Auto add authorities #587

Conversation

Patrick-Erichsen commented Apr 20, 2020 • edited by penrods Loading

Description

Issue

How to test

Unit

Integration

Comments / Concerns

Translation Notes

penrods commented Apr 20, 2020

penrods commented Apr 20, 2020

alpita-masurkar left a comment

Choose a reason for hiding this comment

Patrick-Erichsen commented Apr 20, 2020

Patrick-Erichsen commented Apr 20, 2020

kenpugsley commented Apr 20, 2020

kenpugsley commented Apr 20, 2020 • edited Loading

tstirrat commented Apr 20, 2020

kenpugsley commented Apr 20, 2020

Patrick-Erichsen commented Apr 20, 2020 • edited Loading

tstirrat commented Apr 24, 2020 • edited Loading

tstirrat left a comment

Choose a reason for hiding this comment

tstirrat Apr 24, 2020

Choose a reason for hiding this comment

Patrick-Erichsen Apr 25, 2020

Choose a reason for hiding this comment

tstirrat Apr 24, 2020

Choose a reason for hiding this comment

Patrick-Erichsen Apr 25, 2020

Choose a reason for hiding this comment

tstirrat Apr 25, 2020

Choose a reason for hiding this comment

Patrick-Erichsen commented Apr 26, 2020

kenpugsley commented Apr 26, 2020

Patrick-Erichsen commented Apr 27, 2020

tstirrat commented Apr 27, 2020

tstirrat commented Apr 27, 2020

tstirrat commented Apr 27, 2020

Patrick-Erichsen commented Apr 27, 2020

tstirrat commented Apr 27, 2020

Patrick-Erichsen commented Apr 27, 2020

Patrick-Erichsen commented Apr 20, 2020 •

edited by penrods

Loading

kenpugsley commented Apr 20, 2020 •

edited

Loading

Patrick-Erichsen commented Apr 20, 2020 •

edited

Loading

tstirrat commented Apr 24, 2020 •

edited

Loading