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

Add UI for blocked state #5239

Merged
merged 1 commit into from
Oct 9, 2023
Merged

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Oct 5, 2023


This change is Reviewable

@linear
Copy link

linear bot commented Oct 5, 2023

IOS-322 Add UI for blocked state

With the upcoming changes to the packet tunnel, we should update the UI to support this new connection state. Consider whether all error types require a shortcut for sending a problem report, and have a discussion in Slack. Do try and see how this works on Desktop/Android if you lack inspiration. Until IOS-223 is merged, work off of Andrei's branch instead of main.

https://app.zeplin.io/project/5f928a32fdc9962af9018d70/screen/63c021ed2a0f36bd629f91f1

Link to all current in-app banner messages in the app on all platforms:
https://mullvad.atlassian.net/wiki/spaces/UXUI/pages/1571749912/Guide+to+writing+app+alerts

@rablador rablador changed the base branch from main to tunnel-async October 5, 2023 13:52
@rablador rablador force-pushed the add-ui-for-blocked-state-ios-322 branch from 1421c73 to 1b53b99 Compare October 9, 2023 09:40
@rablador rablador force-pushed the add-ui-for-blocked-state-ios-322 branch from 1b53b99 to 79dd686 Compare October 9, 2023 09:56
Base automatically changed from tunnel-async to main October 9, 2023 10:10
@rablador rablador force-pushed the add-ui-for-blocked-state-ios-322 branch 3 times, most recently from 2d49198 to 2595778 Compare October 9, 2023 10:17
@rablador rablador added the iOS Issues related to iOS label Oct 9, 2023
@rablador rablador marked this pull request as ready for review October 9, 2023 10:17
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 4 files reviewed, 1 unresolved discussion


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

        switch error {
        case .outdatedSchema:
            errorString = "Unable to start tunnel connection after update. Please send a problem report."

Although texts have been discussed with Matilda, slightly unsure about this one.

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.

:lgtm:

Nice job !

Reviewed 2 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @rablador)


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

Previously, rablador (Jon Petersson) wrote…

Although texts have been discussed with Matilda, slightly unsure about this one.

Eh looks good enough IMHO. It's a pretty rare situation, so it's good to encourage problem reports in this case.


ios/MullvadVPN/TunnelManager/TunnelManager.swift line 1103 at r2 (raw file):

            handleBlockedState(reason: .deviceRevoked)
        } else if restError.compareErrorCode(.invalidAccount) {
            handleBlockedState(reason: .invalidAccount)

I wonder how to test that .invalidAccount case... 🤔

@buggmagnet buggmagnet force-pushed the add-ui-for-blocked-state-ios-322 branch from 2595778 to 845d258 Compare October 9, 2023 12:55
@buggmagnet buggmagnet merged commit 2031cb2 into main Oct 9, 2023
3 of 4 checks passed
@buggmagnet buggmagnet deleted the add-ui-for-blocked-state-ios-322 branch October 9, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants