Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design configurable notification schedules #900

Closed
JGreenlee opened this issue May 4, 2023 · 27 comments
Closed

Design configurable notification schedules #900

JGreenlee opened this issue May 4, 2023 · 27 comments

Comments

@JGreenlee
Copy link

We want to allow for a more flexible notification system that can be tailored to any of multiple "schemes" and also consider the user's preference for the timing of the notification.

The interval of the notifications (i.e. daily? weekly?), will be determined by the user's assignment to one of many groups.
The time of day at which the notification comes (i.e. noon? 8pm?) will be specified by the user.

The open question is whether the logic for scheduling notifications should be handled server-side or client-side.
The current system uses push notifications to send a reminder at the same time every day. Thus, the notification is created server-side and pushed out.

Server-side / push notification pros:

  • We can see how many trips there are to label
  • More friendly with background restrictions

Client-side / local notification pros:

  • It would be easier to have the "time of day" be a local setting / not stored on server
@JGreenlee
Copy link
Author

JGreenlee commented May 4, 2023

Ideally, with local notifications, we would be able to create notifications according to schedule in JavaScript. However, we don't know that this will always work when the app is in the background.
So using local notifications would probably require interfacing with some native code.

@JGreenlee
Copy link
Author

Given that a mechanism already exists for creating reminder push notifications, I am leaning towards saying we should just expand on it.

The challenge that this approach faces is that the user's preference for 'time of day' must be somehow recorded on the server, so that the push notification logic respects the user's chosen time.

@shankari
Copy link
Contributor

shankari commented May 5, 2023

Couple of high level comments on this:

  • I'm not sure that the background restrictions are in fact a huge deal
    • let's say that the user asks for a notification at 1am
    • but in fact, the user's phone is restricted (low battery/doze mode) at 1am
    • I believe the notification will be delivered when the phone gets out of the restricted state
    • that seems to be fine since there is no point in delivering a notification if the user is not looking at their phone anyway
  • on the other hand, "The challenge that this approach faces is that the user's preference for 'time of day' must be somehow recorded on the server, so that the push notification logic respects the user's chosen time." is easy, we can just store it in the profile using profile/update, similar to
         }).then(function(sync_interval) {
             CommHelper.updateUser({
                device_token: t.token,
                curr_platform: ionic.Platform.platform(),
                curr_sync_interval: sync_interval
             });
             return t;

@shankari
Copy link
Contributor

shankari commented May 5, 2023

To me the bigger questions are:

  • can we even implement this fully with local notifications?
    • we can store the user-specified time on the phone, but will we also store the random group on the phone?
    • if we do, how will we know which group the user is in for the downstream analysis?
  • in terms of maintainability, with a cleaned up version of Add popup to label screen if incorrect permissions e-mission-phone#964, it is not clear if we still even need the local notification plugin
    • we can generate a standard local notification from native code (without any fancy additionalData, and without any javascript callbacks) pretty easily
    • when the notification is created, we launch the app and that checks the settings, so we don't need the redirect parameters
    • the local notification plugin is not very well maintained, so removing it, and lowering the complexity of the app is a Good Thing in general
  • does remote notification code have actions? If not, it seems like it would be hard to implement the adaptive "time of day" notification. How had you planned to support the adaptive notification?

@JGreenlee
Copy link
Author

how will we know which group the user is in for the downstream analysis?

I think we had previously said that if notifications were local, the group assignments could be part of the OPCodes, sorted out beforehand. But with server-side notifications, we wouldn't have to worry about this and it'd be less complex.

the local notification plugin is not very well maintained, so removing it, and lowering the complexity of the app is a Good Thing in general

Agreed. If possible, it'd be nice to maintain only one notification mechanism

does remote notification code have actions? ... How had you planned to support the adaptive notification?

I did plan on having an action button that would shortcut to the profile for the 'time of day' to be changed.
I looked around for a long while trying to find if this is supported. I was surprised, but unfortunately, I don't think this is supported by default and requires native implementation to create notifications with buttons. To me, this looks like more work than it is worth for us right now.

I don't think the action button is strictly necessary though - the 'time of day' setting could still be accessed and updated in the profile tab. We just have to make sure that at some point, people are informed that the setting exists

Thoughts?

@shankari
Copy link
Contributor

shankari commented May 5, 2023

I don't think the action button is strictly necessary though - the 'time of day' setting could still be accessed and updated in the profile tab. We just have to make sure that at some point, people are informed that the setting exists

I guess we could also put it into the onboarding flow, but I am loath to increase the hoops that people have to jump through while installing the app.

Agreed. If possible, it'd be nice to maintain only one notification mechanism

On the other hand, local notifications are more reliable than remote notifications (remote notifications require a data network to be delivered and local notifications can be instantaneous). And once we switch to react native, the plugins are likely to be better maintained.

So let's first start with what would be the ideal functionality and then go down the ease of implementation and maintainability route.

It sounds like the ideal functionality is action buttons? what would we want to happen when the user clicks on the action button? Because the only thing that is broken right now is the ios callback when the user clicks on the local notification, and I think we can fudge that because we do receive it as a remote notification.

@JGreenlee
Copy link
Author

So the ideal functionality, regardless of implementation, looks like this:

Reminders come in according to schedule, considering both 1) the schedule assignment and 2) the user's time preference

When a reminder shows, the primary interaction (clicking the notification) opens the label screen and marks that the session was started via a reminder so that we can track it later for analysis.
A secondary interaction (action button below the notification) opens the 'time of day' configuration in the profile tab.

For the post-migration future, once we are using React Native, I think that the action button will be much easier to implement, regardless of whether the scheduling happened on the server or the phone.

@JGreenlee
Copy link
Author

JGreenlee commented May 5, 2023

the only thing that is broken right now is the ios callback when the user clicks on the local notification

When you say the callback is broken, do you mean it doesn't open the app? What happens instead on click?

@shankari
Copy link
Contributor

shankari commented May 5, 2023

For the post-migration future, once we are using React Native, I think that the action button will be much easier to implement, regardless of whether the scheduling happened on the server or the phone.

Ok, so then I think we should implement the action button, hacking it if necessary. Note that we can implement the action button in two ways:

  • as a local notification directly
  • as a remote notification that is intercepted by the app and converted to a local notification

so we still have some flexibility there

When you say the callback is broken, do you mean it doesn't open the app? What happens instead on click?

The app is launched, but the callback in www/js/splash/localnotify.js is not invoked.
The callback in www/js/splash/remotenotify.js is invoked, which is why I am optimistic that we can hack it, even if I'm not able to poke around the native code.

After I push the point release out to staging, I'll file an issue and put in all the details.
In the interim, you should be able to see what you get in remotenotify.js when you schedule a local notification with actions, and the user clicks on the action.

@shankari
Copy link
Contributor

shankari commented May 9, 2023

New considerations:

  • if config is stored only on the phone, then it will be lost when the user changes phones

    • solution: store the config on the server in a profile or a similar object and then pull from server during onboarding and only use default if no existing config
  • all notifications must be relative to install time

    • should be relative per original install time
  • if we wanted to do this on server

    • there would be a script that ran every hour, looked at the install time and notification pref and determined which users needed to be notified
    • notify the subset of users
  • if we wanted to do this on the phone

    • schedule all notifications when there is an update
    • update times are: onboarding and user time change

Pros for phone:

  • much simpler because we can just schedule the entire notification schedule on update and let the phone OS scheduling mechanism take over
  • supports more fine grained time specification which may be better for some users (8:30 instead of 8pm)

Pros for servers:

  • Number of trips needed to be labeled; put into the notification
  • Skip the notification if there are no trips to be labeled
  • No need to put a bunch of stuff in profile (or in other document)

At least for now, it seems like phone is the clear winner, given the simplicity and the time constraints
We could experiment with server later depending on feedback for phone.
We could even do things where the server pushes out data before the schedule notification and the phone uses that to amend the scheduled notification later.

@shankari
Copy link
Contributor

shankari commented May 9, 2023

To store data on the server:

  • CommHelper.updateUser({ which will put the data into the profile
  • create a new data type, similar to manual/demographic_survey and store it and load it like we work with that data type

After writing this out though, I do think that the profile is better because:

  • we don't need to create a new data type
  • we generally create new data types if we think that the data will be used in travel behavior analysis (and archived in TSDC...) and this information is not necessarily useful for that

@JGreenlee
Copy link
Author

From my three-day "pilot test" of e-mission/e-mission-phone#969 on a test phone, the scheduling is working as expected - reliably and punctual to the minute.

There are one or two minor things I'd like to adjust before a release. I'll open a follow-up PR today or tomorrow. (1- on Android, see if I can get the time picker to automatically open when the "Change Time" redirect is handled. 2- make sure NotificationScheduler is initialized even if user doesn't open to the Profile tab, config-permitting)

@shankari
Copy link
Contributor

shankari commented May 20, 2023

@JGreenlee couple of things:

@shankari
Copy link
Contributor

shankari commented May 20, 2023

@JGreenlee Also, let's think through and document what will happen to existing users when the app is updated.

  • I think this will crash
    • there will not be any reminderSchemes in the config, so it looks like initReminderSchemes will have all kinds of crashes
  • and if we do have this, then we need to turn off the server-side push notifications so people don't get dual notifications
  • will the existing programs be able to change the time of the notification if they don't have any schedules defined?

@shankari
Copy link
Contributor

@JGreenlee while testing this to see if the crash occurs with an old style config, I found that the module is apparently never initialized. I changed the code to:

    $ionicPlatform.ready().then(async () => {
        _config = await DynamicConfig.configReady();
        console.log("NOTIF: dynamic config ready");
        setUpActions();
        console.log("NOTIF: setupActions() done");
        update();
        console.log("NOTIF: update() done");
    });

And launched the final app (not em-dev-app) on android, where I can see console logs even before chrome was connected

I do see messages for CONFIG_READY

Broadcasting event UI_CONFIG_READY
unifiedlogger.js:49 Resolved UI_CONFIG_READY promise in intro.js, filling in templates:undefined

But no NOTIF: messages

I wonder if this is because we initialize the module after the intro is done

@shankari
Copy link
Contributor

Yup, this is only initialized in

www/js//splash/notifScheduler.js:3:angular.module('emission.splash.notifscheduler',
www/js//control/general-settings.js:8:                                        'emission.splash.notifscheduler',

So if the user does not go to the settings, we will not register for the ionicReady callback or the configReady callback and will not set up any notifications

@JGreenlee
Copy link
Author

@shankari Yes, those are both things I want to address in the follow-up commit

NotificationScheduler is currently initiated from the Profile screen, but the user might not necessarily open that after onboarding. So it should be initialized from the label screen, or directly after onboarding

And secondly, it is initialized without regard to config options. It should only initialize when reminderSchemes is present.
I did check before, and it seemed to cause errors but not crash, when not present

@JGreenlee
Copy link
Author

JGreenlee commented May 21, 2023

Regarding i18n, if the study will have users who prefer different languages, we can specify the text like this:

{
  "en": "Please take a moment to label your trips",
  "es": "Por favor, tómese un momento para etiquetar sus viajes"
}

(just like we did for time-use)

@shankari
Copy link
Contributor

shankari commented May 21, 2023

I did check before, and it seemed to cause errors but not crash, when not present

@JGreenlee Right, it won't crash since it is not a native code change, but it won't schedule any notifications.

And secondly, it is initialized without regard to config options. It should only initialize when reminderSchemes is present.

Maybe we should switch to local notifications for all programs so that we can get the benefit of the flexible time of day.
But we can discuss that later. For now, we will have to at least turn off the server side notifications for programs that do have reminderSchemes present.

Regarding i18n, if the study will have users who prefer different languages, we can specify the text like this:

Yeah I think we need to do this

Yes, those are both things I want to address in the follow-up commit

Do you have an ETA for these follow-up changes? If you can't get to them, I can address them this weekend.

@shankari
Copy link
Contributor

I am working on this for now

For now, we will have to at least turn off the server side notifications for programs that do have reminderSchemes present.

@JGreenlee
Copy link
Author

Do you have an ETA for these follow-up changes? If you can't get to them, I can address them this weekend.

Tomorrow, or maybe late tonight (depends on my sinuses)

@shankari
Copy link
Contributor

LMK if your sinuses don't cooperate and I can take over tomorrow
Also, LMK how to run the pilot test so that I can also test, and we can have the Denver folks also test without waiting for multiple weeks.

Can you document how to run the pilot test? That will help me to test as well and to test this in the future if/when we make changes to it.

shankari added a commit to shankari/e-mission-server that referenced this issue May 21, 2023
If the program/study has flexible notifications configured, we do not want to
send push notifications from the server. These are inflexible, and will be
generated every day at the same times, so we will not actually see flexibility
in the notifications.

So we check the config and skip the server side notification if flexible
notifications are configured.

This is an implementation of
e-mission/e-mission-docs#900 (comment)

Testing done:

Without flexible notifications

```
$ ./e-mission-py.bash bin/push/push_remind.py
/Users/kshankar/e-mission/gis_branch_tests/bin/push/push_remind.py:20: SyntaxWarning: "is not" with a literal. Did you mean "!="?
  if r.status_code is not 200:
storage not configured, falling back to sample, default configuration
URL not formatted, defaulting to "Stage_database"
Connecting to database URL localhost
DEBUG:root:STUDY_CONFIG is stage-program
DEBUG:root:About to download config from https://raw.githubusercontent.com/e-mission/nrel-openpath-deploy-configs/main/configs/stage-study.nrel-op.json
DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): raw.githubusercontent.com:443
DEBUG:urllib3.connectionpool:https://raw.githubusercontent.com:443 "GET /e-mission/nrel-openpath-deploy-configs/main/configs/stage-study.nrel-op.json HTTP/1.1" 200 1697
DEBUG:root:Successfully downloaded config with version 1 for Staging environment for testing studies only and data collection URL https://openpath-stage.nrel.gov/api/
DEBUG:root:Getting tokens matching query {'user_id': {'$in': [UUID('53b18320-898d-4c2b-abad-1e9eabe0385e'), UUID('1aa2c106-70f5-4525-b170-5b367adb6323'), UUID('6afb9865-c48b-4d35-a64c-c94f5a9da5cb')]}}
WARNING:root:ignoring entry {'curr_platform': 'ios'} due to None values
WARNING:root:ignoring entry {} due to None values
WARNING:root:ignoring entry {'curr_platform': 'ios'} due to None values
DEBUG:root:user_id_list of length 3 -> token list of length 0
DEBUG:root:module_name = emission.net.ext_service.push.notify_interface_impl.firebase
DEBUG:root:module = <module 'emission.net.ext_service.push.notify_interface_impl.firebase' from '/Users/kshankar/e-mission/gis_branch_tests/emission/net/ext_service/push/notify_interface_impl/firebase.py'>
DEBUG:root:interface_obj_fn = <function get_interface at 0x11271b310>
DEBUG:root:interface_obj = <emission.net.ext_service.push.notify_interface_impl.firebase.FirebasePush object at 0x112767790>
DEBUG:root:interface_obj = <emission.net.ext_service.push.notify_interface_impl.firebase.FirebasePush object at 0x112767790>
DEBUG:root:len(token_list) == 0, skipping fcm token mapping to save API call
after mapping iOS tokens, imported 0 -> processed 0
combo token map has 0 ios entries and 0 android entries
DEBUG:urllib3.util.retry:Converted retries value: 3 -> Retry(total=3, connect=None, read=None, redirect=None, status=None)
DEBUG:root:{'ios': {'multicast_ids': [], 'success': 0, 'failure': 0, 'canonical_ids': 0, 'results': [], 'topic_message_id': None}, 'android': {'multicast_ids': [], 'success': 0, 'failure': 0, 'canonical_ids': 0, 'results': [], 'topic_message_id': None}}
```

With flexible notifications

```
$ STUDY_CONFIG=denver-casr ./e-mission-py.bash bin/push/push_remind.py
/Users/kshankar/e-mission/gis_branch_tests/bin/push/push_remind.py:20: SyntaxWarning: "is not" with a literal. Did you mean "!="?
  if r.status_code is not 200:
storage not configured, falling back to sample, default configuration
URL not formatted, defaulting to "Stage_database"
Connecting to database URL localhost
DEBUG:root:STUDY_CONFIG is denver-casr
DEBUG:root:About to download config from https://raw.githubusercontent.com/e-mission/nrel-openpath-deploy-configs/main/configs/denver-casr.nrel-op.json
DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): raw.githubusercontent.com:443
DEBUG:urllib3.connectionpool:https://raw.githubusercontent.com:443 "GET /e-mission/nrel-openpath-deploy-configs/main/configs/denver-casr.nrel-op.json HTTP/1.1" 200 2221
DEBUG:root:Successfully downloaded config with version 1 for Denver E-bike Rebate Program Data Collection   and data collection URL https://denver-casr-openpath.nrel.gov/api/
DEBUG:root:Found flexible notification configuration, skipping server-side push
```
@JGreenlee
Copy link
Author

The 'pilot test' schedule was not that sophisticated:

  "schedule": [
      {
          "start": 0,
          "end": 1,
          "intervalInDays": 1
      },
      {
          "start": 3,
          "intervalInDays": 2
      }
  ],

I just wanted to make sure notifications came on the correct days:

  • Day 0 - yes
  • Day 1 - yes
  • Day 2 - no
  • Day 3 - yes
  • Day 4 - no
  • Day 5 - yes
  • ...

I also varied the defaultTime between groups so that I could visibly see that random group assignment was working each time I ran a new OPcode.

On Thursday I set it up on a real phone, I observed that notifications came on Thursday and Friday but not Saturday.

  • When a notification came, if I changed the 'Time of Day' to later in the day, a second notification would come at the new time (expected).
  • I repeated this many times on Thursday and Friday and always got notified at the selected time (expected)
  • Never got any notification on Saturday, no matter what time was picked (expected)

The full reminderSchemes I used is as below - although we are about to internationalize the text so this will be irrelevant:

"reminderSchemes": {
    "weekly": {
        "title": "{Please take a moment to label your trips}",
        "text": "{Click here to open the app and view unlabeled trips}",
        "schedule": [
            {
                "start": 0,
                "end": 1,
                "intervalInDays": 1
            },
            {
                "start": 3,
                "intervalInDays": 2
            }
        ],
        "defaultTime": "21:00"
    },
    "week-quarterly": {
        "title": "{Please take a moment to label your trips}",
        "text": "{Click here to open the app and view unlabeled trips}",
        "schedule": [
            {
                "start": 0,
                "end": 1,
                "intervalInDays": 1
            },
            {
                "start": 3,
                "intervalInDays": 2
            }
        ],
        "defaultTime": "22:00"
    },
    "passive": {
        "title": "{Please take a moment to label your trips}",
        "text": "{Click here to open the app and view unlabeled trips}",
        "schedule": [
            {
                "start": 0,
                "end": 1,
                "intervalInDays": 1
            },
            {
                "start": 3,
                "intervalInDays": 2
            }
        ],
        "defaultTime": "23:00"
    }
}

@JGreenlee
Copy link
Author

JGreenlee commented May 21, 2023

Follow-up PR: e-mission/e-mission-phone#976

see if I can get the time picker to automatically open when the "Change Time" redirect is handled

I didn't get this working, but I think it's ok. The redirect works and the Profile Tab opens - it just would have been nice for the time picker to automatically pop up

@shankari
Copy link
Contributor

shankari commented May 21, 2023

I didn't get this working, but I think it's ok. The redirect works and the Profile Tab opens - it just would have been nice for the time picker to automatically pop up

Yeah I would not worry too much about it now; can handle in a later PR or after the rewrite.

The full reminderSchemes I used is as below - although we are about to internationalize the text so this will be irrelevant:

For testing, I propose the following single scheme for all the schemes

        "schedule": [
            {
                "start": 0,
                "end": 1,
                "intervalInDays": 1
            },
            {
                "start": 3,
                "end": 5,
                "intervalInDays": 2
            }
        ],

So if the Denver folks install tomorrow, they should see:

  • Day 0 (Mon) - yes
  • Day 1 (Tue) - yes
  • Day 2 (Wed) - no
  • Day 3 (Thu) - yes
  • Day 4 (Fri) - no
  • Day 5 (Sat) - yes
  • Day 6 (Sun) - no
  • Day 7 (Mon) - no
  • Day 8 (Tue) - no

We can change the config on Monday and then they can send out emails to participants at the end of the month (Wed).
Good idea on different notification times to check the scheme assignment; I will keep that in the config.

@shankari
Copy link
Contributor

shankari commented May 21, 2023

The one other fit and finish change we may want to do for this (can wait until the next point release) is to show users which group they are in. Unless we are worried that it will bias the results somehow to do that.

Filed #913 to track this

@shankari
Copy link
Contributor

We have now largely deployed this. Let us close this giant issue and track improvements/fit-finish issues separately for clarity. I am aware of at least two:

Not sure if @JGreenlee is still going to try to have the time picker automatically pop up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants