search

eduvpn / apple

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

Show alert on session expiry #442

Closed roop closed 2 years ago

roop commented 2 years ago

When the connection expires, the app should show an alert, saying "This session has expired". This can be helpful when the user is say on a video call when VPN expires and he/she is wondering what went wrong.

(Requested by @efef.)

roop commented 2 years ago

@efef: Can you please confirm the following:

efef commented 2 years ago

@roop I'd expect either a notification OR alert when the VPN is active would be important. Also on iOS an user should be informed at the moment the certificate was expired and the VPN disconnects.

roop commented 2 years ago

Okay, then let's make it as: If the app is in a state when it can show an alert (macOS: app is runnning / iOS: app is in running and in the foreground), the app shall show an alert. If not, it will show a system notification.

The 30-mins-before-expiry notification shall remain as is: always a system notification.

@efef Does that sound alright?

efef commented 2 years ago

hi @roop yes sounds fine. About the 30-minute-before-expiry I notice Simon had for windows a bit different approach: https://github.com/Amebis/eduVPN/blob/547f93a169b49d531f6e6ad7a2222837127b75f8/eduVPN/ViewModels/VPN/Session.cs#L229

He implemented: there has to pass 30 minutes from authentication (because of browser session expiry) and there has to be only 1 day left to expire. If the session expiry is at 75% than show the notification. Essentially this means that people with a daily expirty of 10 hours they will get a notify 2.5 hours before expiry.

Would be welcome if we can align the macOS/iOS notification function. Is this doable?

roop commented 2 years ago

Would be welcome if we can align the macOS/iOS notification function. Is this doable?

Yes, sure. Filed #443 for that.

roop commented 2 years ago

Okay, then let's make it as: If the app is in a state when it can show an alert (macOS: app is runnning / iOS: app is in running and in the foreground), the app shall show an alert. If not, it will show a system notification.

The alert / system notification shall be shown only if notifications are enabled in Preferences.

Currently, the text next to the checkbox in preferences reads:

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

This will be changed to:

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

@efef Does that look correct?

efef commented 2 years ago

hi @roop

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

Sorry for the late reply.

Is it possible to change it to:

There has to pass 30 minutes from authentication because of browser session expiry. When the original session expiry time is less than 24 hours and when the current actual session expiry is at 75% than show notification. (Essentially this means that people with a daily expiry of 10 hours they will get a notify 2.5 hours before expiry.) For all session times (less than 24 hrs and above), a notification is shown 30 mins before session expiry, and when the session expires.

roop commented 2 years ago

For all session times (less than 24 hrs and above), a notification is shown 30 mins before session expiry

I think that is wrong not what we want, right? We want it at 75% of session completion, I presume.

Otherwise, I understand the change.

The question I have is not about the implementation logic, but about the wording in the Settings / Preferences.

roop commented 2 years ago

@efef:

Ah, I think I understand your point now. I guess I wasn't thinking straight when I wrote the last comment. Sorry about that.

Maybe the wording should read as:

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

roop commented 2 years ago

@efef: I asked a question on this in #443, so got confused. Still, what do you think about this:

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

efef commented 2 years ago

sounds fine to me!