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

Add TTL support and custom headers support. #22988

Merged
merged 8 commits into from May 6, 2019
Merged

Add TTL support and custom headers support. #22988

merged 8 commits into from May 6, 2019

Conversation

pszafer
Copy link
Contributor

@pszafer pszafer commented Apr 11, 2019

Related issue: #22249 (comment)
Added TTL support in service and changed default TTL from 0 to 86400.

Creating headers manually (before webpush lib did it for us) to allow us add custom FCM headers like urgency, priority and maybe we find better priority header in the future.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If user exposed functionality or configuration variables are added/changed:

  • Documentation added/updated in home-assistant.io
    I will update docs if you agree with these changes.

@github-actions
Copy link

Hey there @robbiet480, mind taking a look at this pull request as its been labeled with a integration (html5) you are listed as a codeowner for? Thanks!

@codecov
Copy link

codecov bot commented Apr 11, 2019

Codecov Report

Merging #22988 into dev will decrease coverage by 0.19%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev   #22988     +/-   ##
=========================================
- Coverage   94.29%    94.1%   -0.2%     
=========================================
  Files         461      448     -13     
  Lines       37551    36756    -795     
=========================================
- Hits        35410    34590    -820     
- Misses       2141     2166     +25
Impacted Files Coverage Δ
homeassistant/components/html5/notify.py 82.37% <62.5%> (-3.13%) ⬇️
homeassistant/components/mqtt/fan.py 74.33% <0%> (-23.64%) ⬇️
homeassistant/bootstrap.py 58.78% <0%> (-15.62%) ⬇️
homeassistant/components/hassio/ingress.py 67.76% <0%> (-10.29%) ⬇️
homeassistant/components/axis/device.py 93.02% <0%> (-6.98%) ⬇️
homeassistant/components/deconz/config_flow.py 97.77% <0%> (-2.23%) ⬇️
homeassistant/components/hue/bridge.py 73.07% <0%> (-1%) ⬇️
homeassistant/components/mqtt/climate.py 99.49% <0%> (-0.51%) ⬇️
homeassistant/components/hassio/auth.py 90.47% <0%> (-0.44%) ⬇️
homeassistant/components/axis/camera.py 90.47% <0%> (-0.44%) ⬇️
... and 175 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c174b83...63f1395. Read the comment docs.

@robbiet480
Copy link
Member

I don't know how I feel about this. I don't think we should just always set priority levels for every notification, since pushes could be going to platforms other than Android which may handle priority differently. Furthermore and Android specific though, priority messages are limited to app buckets, so sending too many priority notifications can actually cause all notifications for Chrome to become disabled. More info here and here. So we need to be extremely careful with a heavy handed approach that this PR takes.

Can we not just support priority in the data dictionary of a notification so that we can leave this up to users?

@thomasgermain
Copy link
Contributor

I'm adding my two cents here,

if i consider my POV (and only my POV). I'm kind of new user (using HA for 3 months) and I have like 30 automations using push notification. These notifications are only delivered to android devices (my phone and wife's phone).
I guess there are other users that have much more automations using push notification and that are delivered to all kind of devices.

Adding priority in data sounds good, but it will require me some changes (basically, adding the priority to all my automations using push notification).

It would be nice to be able to define both priority in data and directly in component configuration, I guess something like this:

notify:
  - platform: html5
    name: NOTIFIER_NAME
    vapid_pub_key: YOUR_PUBLIC_KEY
    vapid_prv_key: YOUR_PRIVATE_KEY
    vapid_email: YOUR_EMAIL
    default_priority: ...

Default value would be Normal and the only other value High
Of course, priority in data would take precedence over default_priority.

With this, user has the choice to configure HA in a way that requires less changes for him.

This does not seem difficult to do (I know, i'm dev too, easy to say, a bit harder to do :))

@bieniu
Copy link
Member

bieniu commented Apr 12, 2019

I've done recently a lot of notification tests with different priority and TTL settings. My phone is never waking up from sleep regardless of priority. But with TTL == 0 I don't receive any notifications when phone sleeps. TTL != 0 is more important and I think that in this PR we should focus on TTL, not add a permanent high priority to all notifications.

@pszafer
Copy link
Contributor Author

pszafer commented Apr 12, 2019

added two possible priorites: normal and high.
Default is normal and user can decide in single notification if he wants change it to high.

@thomasgermain
Copy link
Contributor

thomasgermain commented Apr 12, 2019

@pszafer
I'm gonna try that this week end, don't see why it wouldn't work.

@bieniu
I think you are true, for android devices TTL seems to be as most important as priority. At least, you want to receive your notification when you are looking at your phone. I guess it could be something configurable like @pszafer just did for priority ?

Anyway, i'm gonna try that TTL/prioriy combination this week end, but I think it's really depending on the phone (manufacturer) since they do some custom battery optimization.

EDIT: sorry, it seems that TTL is already configurable

@bieniu
Copy link
Member

bieniu commented Apr 12, 2019

Priority configured by user is in my opinion the best way. Great work @pszafer

@ghost
Copy link

ghost commented Apr 13, 2019

Has anyone else had issues with the service notify.html5_dismiss no longer working with this patch? I can call the service directly via the HA UI, but neither the notifications on my phone get dismissed, nor push notifications queued in the cloud to get delivered to my Chrome web browser get deleted.

@pszafer
Copy link
Contributor Author

pszafer commented Apr 13, 2019

@eyager1 just checked. Dismiss is working for me, even with high priority.

@thomasgermain
Copy link
Contributor

Hi,

here are my test results.

Here is my config

  • Oneplus 5 (rooted)
  • Chrome beta with battery not optimized option (I only use beta because I cannot de-optimized chrome stable which is a system app)
  • webapk for HA also battery optimized
  • I restared my phone and HA between each test
Doze TTL PriorityHigh PriorityNormal
Active 0 Notification displayed instantly Notification displayed instantly
Active 30 Notification displayed instantly Notification displayed instantly
Active 86400 Notification displayed instantly Notification displayed instantly
Just went to sleep 0 Notification displayed instantly Notification displayed instantly
Just went to sleep 30 Notification displayed instantly Notification displayed instantly
Just went to sleep 86400 Notification displayed instantly Notification displayed instantly
Deep sleep > 10min 0 Not waking up my phone. Notification is displayed after waking up myself my phone Not waking up my phone. Notification not displaying after waking up myself my phone
Deep sleep > 10min 30 Waking up my phone ~30 seconds later - waking phone after TTL -> notification lost // waking phone before TTL -> notification lost
Deep sleep > 10min 86400 Waking up my phone ~30 seconds later Notification pops up after waking up my phone myself

Please note, by "instantly" I mean delay from 0 to 5 sec.

I really thing the good choice is the let user decide (actually, configure) TTL and priority according to his needs and his phone, since i'm pretty sure, tests above won't be the same depending on the phone

@bieniu
Copy link
Member

bieniu commented Apr 14, 2019

@eyager1 For me dismiss works as usually with this fix.

@bieniu
Copy link
Member

bieniu commented Apr 17, 2019

@robbiet480 What do you think about the changes that @pszafer did (priority configured by the user)? There is a chance that this PR would be merged before 0.92 beta?

homeassistant/components/html5/notify.py Outdated Show resolved Hide resolved
homeassistant/components/html5/notify.py Outdated Show resolved Hide resolved
homeassistant/components/html5/notify.py Show resolved Hide resolved
@pszafer
Copy link
Contributor Author

pszafer commented Apr 23, 2019

@robbiet480 what do you think about this PR now?

@andriej
Copy link
Contributor

andriej commented Apr 25, 2019

Looking at #22249 comments it seems to fix issue.

Copy link
Member

@robbiet480 robbiet480 left a comment

Choose a reason for hiding this comment

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

This looks fine but tests need to get fixed so we don't take a coverage hit for such a widely used integration.

@pszafer
Copy link
Contributor Author

pszafer commented Apr 27, 2019

Hope it is enough. 88%. I have troubles to increase it more...

@Konstigt
Copy link

Konstigt commented May 9, 2019

Comment from @jrconlin looks important for this issue. Does not recommend using 86400.

TTL=0 means "If the recipient isn't immediately available, toss the notification". Generally, it's probably not what you want. In most cases, TTL=28800 is probably good enough. It's 8 hours. That should be more than enough for the recipient to get the message, and it still be reasonably relevant. Servers get to set the max amount of time they'll hang onto a message, and will usually tell you if you exceed it.

I'll note that this is NOT the same thing as the exp in the VAPID header. That header identifies you as the sender and the exp says "This info is valid until this time". The max value for EXP is 86400, but due to the fact that clock skew is a thing, you probably want to use a value like 43200. The idea is that if you're sending lots and lots of updates (we get millions from some sources) calculating the same VAPID header over and over is really expensive, so you can cache the header and just reuse it for a few hours.

@pszafer
Copy link
Contributor Author

pszafer commented May 9, 2019

@Konstigt I found one bug in JWT, so I can change default TTL as well. @robbiet480 waiting for your opinion.
About exp header, I stopped using webpush method from Webpusher because we cannot set priority: high in this case. Anyway Webpusher doesn't set vapid_claims['exp'] for us anymore and without this it seems to be working fine.

@balloob balloob mentioned this pull request May 14, 2019
alandtse pushed a commit to alandtse/home-assistant that referenced this pull request Oct 12, 2019
* Add TTL custom support and custom headers support.

* fix pywebpush version

* removed whitespaces surrounding docstrings.

* fixes for tests

* priority option to data

* checking of ATTR_ENDPOINT

* change checking of target to vol.Schema

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

Successfully merging this pull request may close these issues.

None yet

7 participants