allow user to switch between accounts when the account number is ente…

[mojganii]

This PR introduces a user-friendly dialogue box designed to assist users who mistakenly input their account number instead of a voucher code during the redeeming voucher within the account flow. This feature empowers users to seamlessly log out from their current account and effortlessly transition to an alternative account.

---

This change is 

[linear]

IOS-207 Add error message dialog to "New account" flow for voucher

In the New account user flow, some users think that the Redeem voucher text field is where they should insert a Mullvad account number that they, for example, created on the website.

To mitigate this, the app should recognise that what was entered was a Mullvad account number and present a modal that asks the user if they instead intended to log in to a Mullvad account with a button that says "Log out" . The button leads them to the log in view where the account number they entered instead of the voucher code is already entered in the log in text field.

For more information, see Confluence: https://mullvad.atlassian.net/wiki/spaces/UXUI/pages/2363818026/New+account+user+flow+update#Entering-account-number-instead-of-a-voucher-code

Mockups can be found in Zeplin: https://zpl.io/B1lXR7e

[rablador]

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mojganii)

---

ios/MullvadVPN/Coordinators/CreateAccountVoucherCoordinator.swift line 20 at r1 (raw file):

    private var accountNumber: String?
    private lazy var accountNumberInputErrorView = {
        VerifiedAccountView {

It feels a little weird to me to provide views from coordinator to viewcontroller this way. I understand that it has been set up this way to let us reuse and patch existing RedeemVoucherViewController with account verification functionality, but I wonder if we could do it some other way. Maybe @buggmagnet or @pronebird has suggestions?

---

ios/MullvadVPN/Coordinators/WelcomeCoordinator.swift line 167 at r1 (raw file):

        coordinator.didLogout = { [weak self] voucherCoordinator in
            voucherCoordinator.removeFromParent()
            guard let self else { return }

Nit: Move the guard to top of closure?

---

ios/MullvadVPN/View controllers/RedeemVoucher/VerifiedAccountView.swift line 31 at r1 (raw file):

            tableName: "CreateAccountRedeemingVoucher",
            value: """
            It looks like you have entered a Mullvad account number instead of a voucher code.\

Space needed before the backslash.

---

ios/MullvadVPN/View controllers/RedeemVoucher/VerifiedAccountView.swift line 73 at r1 (raw file):

    }

    private func configureUI() {

We could use stackviews here to handle detailed constraints. Also, simply adding the containerview without dynamic constraints gives us a smooth fade-in animation.

Code snippet (i):

private func configureUI() {
    let contentStack = UIStackView(arrangedSubviews: [messageLabel, logoutButton])
    contentStack.axis = .vertical
    contentStack.spacing = UIMetrics.padding16
    contentStack.isLayoutMarginsRelativeArrangement = true
    contentStack.layoutMargins = containerView.layoutMargins

    containerView.addConstrainedSubviews([contentStack]) {
        contentStack.pinEdgesToSuperview(.all())
    }

    addConstrainedSubviews([containerView]) {
        containerView.pinEdgesToSuperview(.all())
    }

    fadeOut()
}

Code snippet (ii):

func fadeIn() {
    animateWith(animations: {
        self.containerView.alpha = 1.0
    }, duration: 0.3, delay: 0.2)
}

func fadeOut() {
    animateWith(animations: {
        self.containerView.alpha = 0.0
    }, duration: 0.0, delay: 0.0)
}

---

ios/MullvadVPN/View controllers/RedeemVoucher/VerifiedAccountView.swift line 124 at r1 (raw file):

    }

    private func animateWith(

Perhaps this could be turned into an extension on UIView for use elsewhere too?

[pronebird]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadVPN/Coordinators/CreateAccountVoucherCoordinator.swift line 20 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

It feels a little weird to me to provide views from coordinator to viewcontroller this way. I understand that it has been set up this way to let us reuse and patch existing RedeemVoucherViewController with account verification functionality, but I wonder if we could do it some other way. Maybe @buggmagnet or @pronebird has suggestions?

I concur. I think view controller should be managing the views, coordinator should mostly be responsible for bigger picture.

Personally I'd either configure the view controller directly or via delegate to change its behavior in regards of when voucher should be matched against account number or tweak interactor and make it configurable, i.e:

struct CreateAccountVoucherInteractor {

    enum RedeemResult {
        case invalidVoucher
        case voucherIsAccountNumber
        case redeemed
    }

    var shouldMatchVoucherWithAccountNumber = false

    @MainActor
    func redeemVoucher(_ voucher: String) throws async -> RedeemResult  {
        do {
            try await submitVoucher(voucher)

            return .redeemed
        } catch let error as? REST.Error where error.compareErrorCode(.invalidVoucher) {
            guard shouldMatchVoucherWithAccountNumber else { throw error }

            do {
                _ = try await getAccountData(voucher)

                return .voucherIsAccountNumber
            }  catch let error as? REST.Error where error.compareErrorCode(.invalidAccount) {
                return .invalidVoucher
            }
        }
    }

}

[pronebird]

Reviewed 1 of 16 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @mojganii)

[mojganii]

Reviewable status: 2 of 19 files reviewed, 3 unresolved discussions (waiting on @pronebird and @rablador)

---

ios/MullvadVPN/Coordinators/CreateAccountVoucherCoordinator.swift line 20 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

I concur. I think view controller should be managing the views, coordinator should mostly be responsible for bigger picture.

Personally I'd either configure the view controller directly or via delegate to change its behavior in regards of when voucher should be matched against account number or tweak interactor and make it configurable, i.e:

struct CreateAccountVoucherInteractor {

    enum RedeemResult {
        case invalidVoucher
        case voucherIsAccountNumber
        case redeemed
    }

    var shouldMatchVoucherWithAccountNumber = false

    @MainActor
    func redeemVoucher(_ voucher: String) throws async -> RedeemResult  {
        do {
            try await submitVoucher(voucher)

            return .redeemed
        } catch let error as? REST.Error where error.compareErrorCode(.invalidVoucher) {
            guard shouldMatchVoucherWithAccountNumber else { throw error }

            do {
                _ = try await getAccountData(voucher)

                return .voucherIsAccountNumber
            }  catch let error as? REST.Error where error.compareErrorCode(.invalidAccount) {
                return .invalidVoucher
            }
        }
    }

}

Done...

---

ios/MullvadVPN/Coordinators/WelcomeCoordinator.swift line 167 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: Move the guard to top of closure?

Done

---

ios/MullvadVPN/View controllers/RedeemVoucher/VerifiedAccountView.swift line 31 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Space needed before the backslash.

Done.

---

ios/MullvadVPN/View controllers/RedeemVoucher/VerifiedAccountView.swift line 73 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

We could use stackviews here to handle detailed constraints. Also, simply adding the containerview without dynamic constraints gives us a smooth fade-in animation.

It brokes constraints when the view is faded in and out.

---

ios/MullvadVPN/View controllers/RedeemVoucher/VerifiedAccountView.swift line 124 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Perhaps this could be turned into an extension on UIView for use elsewhere too?

The animation depends on layout constraints, and if these constraints are integrated into an extension, there's a strong likelihood of redundancy that ends in breaking UI

[buggmagnet]

Reviewed 6 of 17 files at r1, 4 of 17 files at r2, all commit messages.
Reviewable status: 6 of 19 files reviewed, 6 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/MullvadVPN/View controllers/Login/LoginContentView.swift line 177 at r3 (raw file):

    }

    private func addObservers() {

A view should not be registering to notifications to get its model data, this breaks encapsulation.
Instead, the view controller that holds this view (LoginContentView in this case) should be made aware via the LoginInteractor that there should be a "preferred account number" to fill.

---

ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherInteractor.swift line 68 at r3 (raw file):

    private func verifyVoucherAsAccount(code: String) {
        let executer = accountsProxy.getAccountData(accountNumber: code)
        tasks.append(executer.execute(retryStrategy: .noRetry) { [weak self] result in

You can replace this with tasks.append(executer.execute { [weak self] result in
The default execute method already specifies .noRetry

Code quote:

tasks.append(executer.execute(retryStrategy: .noRetry) { [weak self] result in

---

ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherInteractor.swift line 81 at r3 (raw file):

     Name of notification posted when current account number changes.
     */
    static let didChangePreferredAccountNumber = Notification

This shouldn't be broadcasting notifications about the "preferred account" being changed, instead it should bubble back this information to the Welcome Interactor/Coordinator (whichever has the view model that will be applied to the login view)

[mojganii]

Reviewable status: 6 of 19 files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/View controllers/Login/LoginContentView.swift line 177 at r3 (raw file):

Previously, buggmagnet wrote…

A view should not be registering to notifications to get its model data, this breaks encapsulation.
Instead, the view controller that holds this view (LoginContentView in this case) should be made aware via the LoginInteractor that there should be a "preferred account number" to fill.

Done.

---

ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherInteractor.swift line 68 at r3 (raw file):

Previously, buggmagnet wrote…

You can replace this with tasks.append(executer.execute { [weak self] result in
The default execute method already specifies .noRetry

Done.

---

ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherInteractor.swift line 81 at r3 (raw file):

Previously, buggmagnet wrote…

This shouldn't be broadcasting notifications about the "preferred account" being changed, instead it should bubble back this information to the Welcome Interactor/Coordinator (whichever has the view model that will be applied to the login view)

Done.

[rablador]

Reviewed 14 of 17 files at r2, 2 of 2 files at r3, 8 of 8 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherContentView.swift line 83 at r3 (raw file):

    }()

    private lazy var logoutViewForAccountNumberIsEntered: VerifiedAccountView = {

Nit: Could this be renamed to something more consise? Perhaps "accountVerificationView" or "logoutView"?

---

ios/MullvadVPN/View controllers/RedeemVoucher/VerifiedAccountView.swift line 73 at r1 (raw file):

Previously, mojganii wrote…

It brokes constraints when the view is faded in and out.

Ah, yes, the stack views are finnicky sometimes...

Not pushing for a change, but if you want to it could be remedied by changing to:

Code snippet:

containerView.addConstrainedSubviews([contentStack]) {
    contentStack.pinEdgesToSuperview(PinnableEdges([.leading(0), .top(0)]))
    contentStack.bottomAnchor.constraint(equalTo: containerView.bottomAnchor).withPriority(.defaultHigh)
    contentStack.trailingAnchor.constraint(equalTo: containerView.trailingAnchor).withPriority(.defaultHigh)
}

---

ios/MullvadVPN/View controllers/RedeemVoucher/VerifiedAccountView.swift line 124 at r1 (raw file):

Previously, mojganii wrote…

The animation depends on layout constraints, and if these constraints are integrated into an extension, there's a strong likelihood of redundancy that ends in breaking UI

Yeah, sorry, I didn't look at it properly. Was thinking of some generalisation, but nevermind that!

---

ios/MullvadVPN/View controllers/Login/LoginContentView.swift line 177 at r3 (raw file):

Previously, mojganii wrote…

Done.

+1

[mojganii]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherContentView.swift line 83 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: Could this be renamed to something more consise? Perhaps "accountVerificationView" or "logoutView"?

Sure

[rablador]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @mojganii)

[buggmagnet]

Reviewed 6 of 17 files at r2, 8 of 8 files at r4, 2 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @mojganii)

---

ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift line 48 at r5 (raw file):

    /// Posts `preferredAccountNumber` notification when user inputs the account number instead of voucher code
    private let preferredAccountNumberSubject = PassthroughSubject<String?, Never>()

The String type doesn't need to be optional here

---

ios/MullvadVPN/Coordinators/WelcomeCoordinator.swift line 25 at r5 (raw file):

    var didFinish: ((WelcomeCoordinator) -> Void)?
    var didLogout: ((WelcomeCoordinator, String) -> Void)?

We never use the first parameter passed here in those callbacks, we should remove them.

---

ios/MullvadVPN/View controllers/Login/LoginInteractor.swift line 17 at r5 (raw file):

    private var tunnelObserver: TunnelObserver?
    var didCreateAccount: (() -> Void)?
    var doSuggestPreferredAccountNumber: ((String) -> Void)?

We can rename this suggestPreferredAccountNumber

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 1390 at r5 (raw file):

		F07BF2612A26279100042943 /* RedeemVoucherOperation.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = RedeemVoucherOperation.swift; sourceTree = "<group>"; };
		F07CFF1F29F2720E008C0343 /* RegisteredDeviceInAppNotificationProvider.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RegisteredDeviceInAppNotificationProvider.swift; sourceTree = "<group>"; };
		F09A29782A9F8A9B00EA3B6F /* LogoutDialogueView.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = LogoutDialogueView.swift; sourceTree = "<group>"; };

Please move LogoutDialogueView above the RedeemVoucher*.swift files

[mojganii]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift line 48 at r5 (raw file):

Previously, buggmagnet wrote…

The String type doesn't need to be optional here

Done.

---

ios/MullvadVPN/Coordinators/WelcomeCoordinator.swift line 25 at r5 (raw file):

Previously, buggmagnet wrote…

We never use the first parameter passed here in those callbacks, we should remove them.

Done.

---

ios/MullvadVPN/View controllers/Login/LoginInteractor.swift line 17 at r5 (raw file):

Previously, buggmagnet wrote…

We can rename this suggestPreferredAccountNumber

Done.

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 1390 at r5 (raw file):

Previously, buggmagnet wrote…

Please move LogoutDialogueView above the RedeemVoucher*.swift files

Done.

[buggmagnet]

very well done !

Reviewed 6 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions

[buggmagnet]

I said approve, not request changes

Reviewable status: all files reviewed, 2 unresolved discussions