eduvpn / apple

app for iOS and macOS
Other
58 stars 18 forks source link

Change when the session expiry notification is shown #443

Closed roop closed 2 years ago

roop commented 2 years ago

Continuing discussion from #442:

Currently, the session expiry notification is shown at the latest of these time instances:

In case the latest of the above is after expiry_time (this can happen in test servers with session validity less than 30 mins), the session expiry notification is shown immediately after expiry at half the time left to session expiry (changed in PR #451).

We want the session expiry notification to be shown at the latest of these three time instances:

As happens now, in case the latest of the above is after expiry_time (this can happen in test servers with session validity less than 30 mins), the session expiry notification shall be shown immediately after expiry.

Is that correct? @efef @fkooman

miquels commented 2 years ago

It would be great if it did not show the session expiry notification if it is already being shown. My organization has the requirement that sessions expire after 24 hours (between 3-5 at night). When I left my laptop on by accident for a few days, then came back I had to click away lots and lots of expiry notifications, all stacked on top of one another.

roop commented 2 years ago

@miquels Thanks for reporting that. I would expect 2 notifications, one saying "Your VPN session is expiring", and another one saying "Your VPN session has expired". Did you see more than 2 that you had to dismiss one after the other?

Also, did you see the notifications as alerts in the app, or as banners at the top right of your screen?

roop commented 2 years ago

@miquels I forgot about the alert we show on device wakeup. I realize that if you suspend and wakeup the mac while the app was running with the session expired, it can indeed have a stack of many alerts.

I'll fix this as part of issue #445.

roop commented 2 years ago

@efef: In the Settings / Preferences, the checkbox for enabling session expiry notifications currently says:

[ x ] Notify when the session is about to expire If enabled, a notification is triggered 30 mins before session expiry.

With #451, it has been changed to:

[ x ] Notify when the session is about to expire If enabled, a notification is triggered 30 mins before session expiry, and when the session expires.

With the change proposed here, how should it read. Will this be ok?:

[ x ] Notify when the session is about to expire If enabled, a notification is shown before and on session expiry.

efef commented 2 years ago

Based on Francois his description, it is just fine, only please change 30 minutes before expiry to 60 minutes

roop commented 2 years ago

The 30 mins to 60 mins change was done as part of #451 (commit 054e9bf)