VPN-5688: Prevent VPN activation when completing onboarding #8276
Conversation
There was a problem hiding this comment.
After onboarding (testing on iOS), the first time I hit the VPN toggle in the app it doesn't turn on. I believe this is related to this PR.
Also, this records glean metrics for turning off the tunnel, right? Is that something that is okay here? (I'm not sure what the answer is.)
| @@ -200,6 +200,7 @@ public class IOSControllerImpl : NSObject { | |||
| //Used to create a VPN configuration without connecting during onboarding | |||
| if isOnboarding { | |||
| IOSControllerImpl.logger.info(message: "Finishing onboarding... do not turn on the VPN after gaining permission") | |||
| failureCallback() | |||
There was a problem hiding this comment.
Since this is now being intentionally used as a disconnectCallback, what do you think about renaming it? And updating the log line in the callback?
There was a problem hiding this comment.
Agree - updated.
Actually, this exists outside this change, albeit less noticeable as there is the whole "grant system permissions / add configuration" flow in between the user clicking the toggle and the VPN turning on
it does, and I had not considered telemetry impact... I doubt we'd want actual disconnected telemetry |
Do you mean "outside this change" as in "current state in prod" or "current state with new onboarding"? Currently on prod, doesn't it default to being on when you come back from the permission, so if there is something weird here it's not noticeable to users? |
I mean currently in prod, heres a video of the flicker (connecting -> off -> on) RPReplay_Final1697223015.MP4 |
My concern is that this makes the flicker bug much worse, from a user's perspective: Currently:
With this PR:
The current setup arguably seems reasonable (as they come back to our app), but the second setup very much seems buggy (as the VPN just doesn't turn on), and this is the user's first experience with our product. I believe this bug is now being tracked as https://mozilla-hub.atlassian.net/browse/VPN-5681. If we're going to merge this before fixing VPN-5681, I'd want approval from Design and/or Product. Does that seem reasonable? (Also, if we need someone to look into 5681, I'm happy to pick that up.) |
I agree, this bug is exacerbated due to the change in flow (since there is no settings flow to break up the vpn toggle press and then flicker off back on), but in both the current production and in this PR, the VPN DOES turn on after only being tapped once after a second or two of "confusion" (or at least that has been my experience). Can you verify that you are definitely needing to click the vpn toggle twice? attaching a video of what I am seeing with this patch: RPReplay_Final1697489737.MP4
I am not entirely sure how related they are as the bug I believe we are discussing happens in prod 😳 so hopefully not very related. Definitely agree that we should get opinions from prod/design before proceeding with this worsened "flicker" |
|
My bad on two points:
That said, I still think we need a sign off before merging - or to work on that delay. I'm happy to look into it if we decide to block this PR on that fix. |
| @@ -219,7 +219,8 @@ void Telemetry::initialize() { | |||
| connectionManager, &ConnectionManager::controllerDisconnected, this, | |||
| [this, connectionManager]() { | |||
| if (Feature::get(Feature::Feature_superDooperMetrics)->isSupported()) { | |||
| if (connectionManager->state() == ConnectionManager::StateOff) { | |||
| if (connectionManager->state() == ConnectionManager::StateOff && | |||
| SettingsHolder::instance()->onboardingCompleted()) { | |||
There was a problem hiding this comment.
Thank you - I was worried this "don't log telemetry" was going to be waaaay harder than that. Relatedly, I just learned that there is no :sweatsmile: emoji in GitHub.
Description
Reference
VPN-5688: Client is turned ON when the user allows to create a VPN connection