-
Notifications
You must be signed in to change notification settings - Fork 449
Combine DAITA and Multihop to one pill when smart routing is active #8270
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
Combine DAITA and Multihop to one pill when smart routing is active #8270
Conversation
|
🚨 End to end tests failed. Please check the failed workflow run. |
|
🚨 End to end tests failed. Please check the failed workflow run. |
8c68559 to
bdd0d88
Compare
rablador
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR fixes some general swiftlint errors I found as well.
Reviewable status: 0 of 18 files reviewed, all discussions resolved
991807e to
b41986c
Compare
b41986c to
d95ee1a
Compare
buggmagnet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, 13 of 13 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion
ios/MullvadVPN/View controllers/Tunnel/ConnectionView/ChipView/ChipFeature.swift line 34 at r2 (raw file):
state.isMultihop && !settings.tunnelMultihopState.isEnabled ? NSLocalizedString( "FEATURE_INDICATORS_CHIP_DAITA",
We want two different localisation keys for this
How about
FEATURE_INDICATORS_CHIP_DAITA_MULTIHOP ?
d95ee1a to
1e87b7d
Compare
rablador
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)
ios/MullvadVPN/View controllers/Tunnel/ConnectionView/ChipView/ChipFeature.swift line 34 at r2 (raw file):
Previously, buggmagnet wrote…
We want two different localisation keys for this
How about
FEATURE_INDICATORS_CHIP_DAITA_MULTIHOP?
Done.
rablador
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)
ios/MullvadVPN/View controllers/Tunnel/ConnectionView/ChipView/ChipFeature.swift line 34 at r2 (raw file):
Previously, buggmagnet wrote…
We want two different localisation keys for this
How about
FEATURE_INDICATORS_CHIP_DAITA_MULTIHOP?
Done.
buggmagnet
left a comment
There was a problem hiding this 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:complete! all files reviewed, all discussions resolved
acb-mv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
1e87b7d to
049c09f
Compare
SteffenErn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, 13 of 13 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion
ios/MullvadVPN/Coordinators/Settings/DAITA/SettingsDAITAView.swift line 19 at r3 (raw file):
VStack(alignment: .leading, spacing: 8) { if isAutomaticRoutingActive { DAITAMultihopNotice()
28e14ec to
519ea39
Compare
rablador
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 15 of 17 files reviewed, 1 unresolved discussion (waiting on @SteffenErn)
ios/MullvadVPN/Coordinators/Settings/DAITA/SettingsDAITAView.swift line 19 at r3 (raw file):
Done.
SteffenErn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 14 of 17 files reviewed, 3 unresolved discussions (waiting on @buggmagnet)
ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 204 at r4 (raw file):
/// Verify that the app attempts to connect over Multihop. @discardableResult func verifyNotConnectingOverMultihop() -> Self { XCTAssertFalse(app.staticTexts["Multihop"].exists)
When rebasing on main this will fail since the indicators are now buttons. Should be changed to app.buttons["Multihop"].exists
ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 216 at r4 (raw file):
/// Verify that the app attempts to connect using DAITA. @discardableResult func verifyConnectingUsingDAITAThroughMultihop() -> Self { XCTAssertTrue(app.staticTexts["DAITA: Multihop"].exists)
app.buttons["DAITA: Multihop"].exists see comment above
519ea39 to
7cb800b
Compare
rablador
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 10 of 17 files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @SteffenErn)
ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 204 at r4 (raw file):
Previously, SteffenErn (Steffen) wrote…
When rebasing on main this will fail since the indicators are now buttons. Should be changed to
app.buttons["Multihop"].exists
Done.
ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 216 at r4 (raw file):
Previously, SteffenErn (Steffen) wrote…
app.buttons["DAITA: Multihop"].existssee comment above
Done.
7cb800b to
bbe742b
Compare
|
Switching to Direct only doesn't put me in blocked state or disables the automatic multihop. I am somehow able to reconnect to the same configuration of relays even though I have no selection made in select location. |
rablador
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phone or simulator?
Reviewable status: 10 of 17 files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @SteffenErn)
rablador
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, you're sure the relay doesn't support DAITA?
Reviewable status: 10 of 17 files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @SteffenErn)
SteffenErn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
|
This is working fine on the phone, but the sim is borked. Sim doesn't go into blocked state unless you manually disconnect and try to connect again when activating Direct Only in a automatic multihop configuration (for example, Spain via Sweden). |
buggmagnet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@waahlnaden Please do not rely on the simulator to perform tests that would put you in the block state, the simulator cannot run the network extension and as such cannot reliably convey blocked state or not.
We made some improvements lately on it being able to simulate some form of blocked state by looking at the relay selector, but that does not match the full app.
Reviewable status:
complete! all files reviewed, all discussions resolved
|
Yes I know. But, isn't there value in keeping the sim somewhat close to real usage? This is the furthest divergence I've encountered. Just wouldn't want our sim to end up in a state where we eventually cannot trust it at all if we keep adding things that makes it diverge further. |
buggmagnet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some value indeed, but the blocked state is entirely implemented in the Packet Tunnel (which the simulator cannot run) the effort required to make this happen in the simulator is way too big for the result. Especially because we run all our tests on a real device to being with.
Reviewable status:
complete! all files reviewed, all discussions resolved
|
Case closed, looks good to me! |
bbe742b to
e08db1c
Compare
|
🚨 End to end tests failed. Please check the failed workflow run. |


Why this needs to be done
We want to clarify to users that multihop is being used because of "smart routing" with DAITA.
What needs to be done
Combine DAITA and Multihop to one pill when "smart routing" is active. It should say "DAITA: Multihop".
Add info to the DAITA subview by using the navigation header subtitle space: "(i) Multihop is being used to enable DAITA for your selected location."
Acceptance criteria
Tests
UI tests.
This change is