-
Notifications
You must be signed in to change notification settings - Fork 119
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
make push notifs configurable, i18n, & only for users missing labels #969
Conversation
Make the text for push notifications configurable, translatable, and filterable to only include users with at least one recent trip that has no user input. The new config field 'push_notifications' has 'title' and 'message' (the text, per language) and 'recent_user_input_threshold' (the number of days to consider for determining 'recent user input'). Note the caveat about partially-labeled trips.
I believe this works, but I don't know a good way to test it end-to-end. I thought I could load a dump (of |
|
I can generate a new dump on staging but that still won't be enough, because you don't have the API key that allows you to send the notifications. The only option would be for me to pull it and run it directly against staging. Trying that now... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
wrt
I think they should be moved somewhere else, I just don't know the best place to put them
This would be emission/core/common.py
https://github.com/e-mission/e-mission-server/blob/master/emission/core/common.py
I will merge this as soon as I test on staging; should be out by the end of the weekend.
import json | ||
import logging | ||
import os | ||
import requests | ||
import sys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops! I guess it was still exiting if the config wasn't available, but with a different error?!
import emission.net.ext_service.push.notify_usage as pnu | ||
|
||
STUDY_CONFIG = os.getenv('STUDY_CONFIG', "stage-program") | ||
|
||
|
||
def users_without_recent_user_input(uuid_list, recent_user_input_threshold=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
future fix: it should be possible to write unit tests for both these function, particularly if we want to reuse them later.
return filtered_uuids | ||
|
||
|
||
def bin_users_by_lang(uuid_list, langs, lang_key='phone_lang'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
To test this, I modified I then tested as below:
|
Make the text for push notifications configurable, translatable, and filterable to only include users with at least one recent trip that has no user input. The new config field 'push_notifications' has 'title' and 'message' (the text, per language) and 'recent_user_input_threshold' (the number of days to consider for determining 'recent user input'). Note the caveat about partially-labeled trips.