Fix presenting notification settings onboarding

[mojganii]

This PR fixes the issue where the notification onboarding page was presented even though the user had already been prompted to allow notifications.

---

This change is 

[linear]

IOS-1515 Notification onboarding shown whenever app is opened

[github-actions]

🚨 End to end tests failed. Please check the failed workflow run.

[pinkisemils]

When I run your branch, my settings are wiped again.

@pinkisemils made 2 comments.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @mojganii).

---

ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift line 105 at r1 (raw file):

            Task {
                let isAllowed = await UNUserNotificationCenter.isAllowed
                appPreferences.isNotificationPermissionAsked = isAllowed

Should this not be set to true if we did actually ask for permissions?

[pinkisemils]

Given that we're struggling with this a bit, I suggest we add a snapshot test - we extract the json object that was stored in the app preferences in the last test flight and we add a test that attempts to use the newer format to see it work.

@pinkisemils made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @mojganii).

[rablador]

@rablador reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils).

---

ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift line 105 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Should this not be set to true if we did actually ask for permissions?

Yes, we should disregard whether the user has allowed or not, the only thing we need to know is that they've been asked once.

[mojganii]

@mojganii made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils).

---

ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift line 105 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Yes, we should disregard whether the user has allowed or not, the only thing we need to know is that they've been asked once.

If we’re checking !appPreferences.isNotificationPermissionAsked, then isAllowed should decide whether to show the prompt or not. For example, when a user updates the app and runs it for the first time, appPreferences.isNotificationPermissionAsked will be false. In that case, we need to ignore showing the permission prompt if notifications are already enabled.

[rablador]

@rablador made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils).

---

ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift line 105 at r1 (raw file):

Previously, mojganii wrote…

If we’re checking !appPreferences.isNotificationPermissionAsked, then isAllowed should decide whether to show the prompt or not. For example, when a user updates the app and runs it for the first time, appPreferences.isNotificationPermissionAsked will be false. In that case, we need to ignore showing the permission prompt if notifications are already enabled.

We could use .notDetermined state to check if the user has made a decision at all. appPreferences.isNotificationPermissionAsked == false and status == .notDetermined should then yield the popup.

[mojganii]

@mojganii made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils).

---

ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift line 105 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

We could use .notDetermined state to check if the user has made a decision at all. appPreferences.isNotificationPermissionAsked == false and status == .notDetermined should then yield the popup.

if the status of the notification is anything except .authorized then we will give to the user the chance to enable it once. I don't think so the logic if hard to follow. would you like me to add a comment to make it clearer?

[pinkisemils]

Ah, I get a settings wipe if I run it on top of a main install, but when installing over the test flight or the app store version, it works fine. So I retract my statement.

@pinkisemils made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion.

[rablador]

@rablador made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils).

---

ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift line 105 at r1 (raw file):

Previously, mojganii wrote…

if the status of the notification is anything except .authorized then we will give to the user the chance to enable it once. I don't think so the logic if hard to follow. would you like me to add a comment to make it clearer?

I just realised I didn't fully consider the update case. I just talked to @pinkisemils and we both think that we could just simplify things here but not considering if updating user have allowed or denied notifications in the past. Since we're bringing in a new settings page and features relying on it (IAN), we can show the popup once anyway. So we'd only need to check the appPreferences.isNotificationPermissionAsked flag and nothing else. Both new users and updating users will have the flag being false and when they've seen it we just set it to true. That's it. What do you think?

[rablador]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils).

[pinkisemils]

@pinkisemils made 1 comment and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved.

---

ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift line 105 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I just realised I didn't fully consider the update case. I just talked to @pinkisemils and we both think that we could just simplify things here but not considering if updating user have allowed or denied notifications in the past. Since we're bringing in a new settings page and features relying on it (IAN), we can show the popup once anyway. So we'd only need to check the appPreferences.isNotificationPermissionAsked flag and nothing else. Both new users and updating users will have the flag being false and when they've seen it we just set it to true. That's it. What do you think?

I revise my first comment, I think this code is sound - we will not ask for more permissions if the user has already enabled notifications - in all other cases, we will show it at most once anyway.

[pinkisemils]

@pinkisemils reviewed 3 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved.

[mojganii]

isn't it because we had the problem on main?

@mojganii made 1 comment.
Reviewable status: 3 of 13 files reviewed, all discussions resolved.

[pinkisemils]

@pinkisemils reviewed 10 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii).

---

ios/MullvadVPN.xcodeproj/xcshareddata/xcschemes/MullvadVPN.xcscheme line 285 at r2 (raw file):

   </TestAction>
   <LaunchAction
      buildConfiguration = "Staging"

This shouldn't be a part of these changes.

[mojganii]

@mojganii made 1 comment.
Reviewable status: 12 of 13 files reviewed, 1 unresolved discussion (waiting on @pinkisemils).

---

ios/MullvadVPN.xcodeproj/xcshareddata/xcschemes/MullvadVPN.xcscheme line 285 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

This shouldn't be a part of these changes.

good catch!

[rablador]

@rablador reviewed 10 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii and @pinkisemils).

---

ios/MullvadVPNUITests/Base/BaseUITestCase.swift line 324 at r3 (raw file):

        } while successIconShown == false && retryCount < maxRetryCount

        skipNotificationPromptIfShown()

With this check here, do we need to check in the other tests as well?

[pinkisemils]

@pinkisemils reviewed 1 file and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii).

[pinkisemils]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii).

[mojganii]

@mojganii made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador).

---

ios/MullvadVPNUITests/Base/BaseUITestCase.swift line 324 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

With this check here, do we need to check in the other tests as well?

yeah. a few of them are using this function

[pinkisemils]

@pinkisemils made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils).

---

ios/MullvadVPNUITests/Base/BaseUITestCase.swift line 324 at r3 (raw file):

Previously, mojganii wrote…

yeah. a few of them are using this function

It can still be shown in other cases, I guess. But is that really true?

[pinkisemils]

The problem on main was fixed by my small change, was it not?

@pinkisemils made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion.

[pinkisemils]

@pinkisemils reviewed 3 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador).

[rablador]

@rablador reviewed 3 files and all commit messages, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved.