Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not show account revoked banner when on account revoked screen #5531

Merged

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Nov 30, 2023

A banner with a blocked state reason (unable to authenticate account) is shown when on the account revoked screen. This is superfluous information.


This change is Reviewable

Copy link

linear bot commented Nov 30, 2023

IOS-396 When the revoked view is shown and the app fails to reconnect again, the in-app banner is shown

signal-2023-11-21-151952.png

To trigger this bug, revoke your device and toggle the VPN connection from the settings app.

Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/Notifications/Notification Providers/TunnelStatusNotificationProvider.swift line 69 at r1 (raw file):

    }

    private func updateLastTunnelError(_ tunnelState: TunnelState) -> Bool {

the function is doing two responsibilities that makes readability more complex.I suggest export these sort of conditions from your class to make is shorter and more descriptive

Code snippet:

fileprivate extension TunnelState {
    var isWaitingForConnectivity : Bool {
        self == .waitingForConnectivity(.noConnection)
    }
    
    var noNetwork : Bool {
        self == .waitingForConnectivity(.noConnection)
    }
    
    var accountIsInvalid : Bool {
        if case let .error(blockedStateReason) = self,
           blockedStateReason == .accountExpired || blockedStateReason == .deviceRevoked {
            return true
        }
        return false
    }
    
}

ios/MullvadVPN/Notifications/Notification Providers/TunnelStatusNotificationProvider.swift line 229 at r1 (raw file):

    }

    private func localizedReasonForBlockedStateError(_ error: BlockedStateReason) -> String {

I think this part can be common along different type of objects

Code snippet:

extension BlockedStateReason : CustomStringConvertible {
    public var description: String {
        let errorString: String
        
        switch self {
        case .outdatedSchema:
            errorString = "Unable to start tunnel connection after update. Please disconnect and reconnect."
        case .noRelaysSatisfyingConstraints:
            errorString = "No servers match your settings, try changing server or other settings."
        case .invalidAccount:
            errorString = "You are logged in with an invalid account number. Please log out and try another one."
        case .deviceRevoked, .deviceLoggedOut:
            errorString = "Unable to authenticate account. Please log out and log back in."
        default:
            errorString = "Unable to start tunnel connection. Please send a problem report."
        }
        
        return NSLocalizedString(
            "BLOCKED_STATE_ERROR_TITLE",
            tableName: "Main",
            value: errorString,
            comment: ""
        )
    }
}

@rablador rablador force-pushed the when-the-revoked-view-is-shown-and-the-app-fails-to-ios-396 branch from a886703 to 45b552d Compare December 5, 2023 09:09
Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @mojganii)


ios/MullvadVPN/Notifications/Notification Providers/TunnelStatusNotificationProvider.swift line 69 at r1 (raw file):

Previously, mojganii wrote…

the function is doing two responsibilities that makes readability more complex.I suggest export these sort of conditions from your class to make is shorter and more descriptive

As was discussed elsewhere we should probably not add concepts such as account validation to TunnelState, but I extracted some of the code to a new function to lower complexity.


ios/MullvadVPN/Notifications/Notification Providers/TunnelStatusNotificationProvider.swift line 229 at r1 (raw file):

Previously, mojganii wrote…

I think this part can be common along different type of objects

I don't think we need to make these strings generally accessible at this point, since they're only used here anyway. Once we need them elsewhere I agree with your suggestion.

@buggmagnet buggmagnet requested a review from mojganii December 5, 2023 10:37
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @mojganii and @rablador)


ios/MullvadVPN/Notifications/Notification Providers/TunnelStatusNotificationProvider.swift line 120 at r2 (raw file):

    }

    private func tunnelError(from tunnelState: TunnelState) -> BlockedStateReason? {

Without context, it's impossible to figure out why we do this here.
Can you add a comment that explains why we filter only for accountExpired and deviceRevoked errors ?

@rablador rablador force-pushed the when-the-revoked-view-is-shown-and-the-app-fails-to-ios-396 branch from 45b552d to 3e61d29 Compare December 5, 2023 11:49
Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)


ios/MullvadVPN/Notifications/Notification Providers/TunnelStatusNotificationProvider.swift line 120 at r2 (raw file):

Previously, buggmagnet wrote…

Without context, it's impossible to figure out why we do this here.
Can you add a comment that explains why we filter only for accountExpired and deviceRevoked errors ?

Done.

@buggmagnet buggmagnet force-pushed the when-the-revoked-view-is-shown-and-the-app-fails-to-ios-396 branch from 3e61d29 to 6578f24 Compare December 5, 2023 12:14
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

@buggmagnet buggmagnet merged commit f2594c1 into main Dec 5, 2023
4 of 5 checks passed
@buggmagnet buggmagnet deleted the when-the-revoked-view-is-shown-and-the-app-fails-to-ios-396 branch December 5, 2023 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants