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 smart routing to daita settings data #6870

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Sep 26, 2024


This change is Reviewable

@rablador rablador added the iOS Issues related to iOS label Sep 26, 2024
@rablador rablador self-assigned this Sep 26, 2024
Copy link

linear bot commented Sep 26, 2024

@rablador rablador force-pushed the add-smart-routing-to-daita-settings-data-ios-838 branch from 3a830d0 to 568584e Compare September 26, 2024 08:07
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.

2024-09-30 10:26:45.003185+0200 MullvadVPN[1536:467494] [LoadTunnelConfigurationOperation] Cannot read settings.
Caused by: Key "daitaState" not found at "data.daita". (domain = NSCocoaErrorDomain, code = 4865)

If I upgrade from an existing AppStore Build, I cannot connect anymore.
Please make sure to:

  • Test upgrading from a public version
  • Add unit tests (ideally automated one but that's up to your discretion) that test this.

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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.

Ah, my bad, I forgot about migration!

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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.

Should we either:

  1. Keep state (rather than renaming to daitaState) and add smartRouting prop?
  2. Deprecate state and introduce daitaStateand smartRoutingState (like current code)?

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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.

Ex of second option:

@available(*, deprecated, renamed: "daitaState")
public let state: DAITAState = .off

[...]

public init(from decoder: any Decoder) throws {
    let container = try decoder.container(keyedBy: CodingKeys.self)

    daitaState = try container.decodeIfPresent(DAITAState.self, forKey: .daitaState)
        ?? container.decodeIfPresent(DAITAState.self, forKey: .state)
        ?? .off

    smartRoutingState = try container.decode(SmartRoutingState.self, forKey: .smartRoutingState)
}

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@rablador rablador force-pushed the add-smart-routing-to-daita-settings-data-ios-838 branch 2 times, most recently from 9af2b2e to f82898e Compare October 1, 2024 06:56
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.

Option 1 is better IMHO, daitaState is redundant since we're already talking about a DaitaSettings object.

Reviewable status: 12 of 13 files reviewed, all discussions resolved

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.

What would the name of the smart routing state? We could call it simply smartRouting, but then we'd end up with state and smartRouting where both are states.

Reviewable status: 12 of 13 files reviewed, all discussions resolved (waiting on @buggmagnet)

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.

Ok I see what you mean, then let's go with your original proposition. We rename state to daitaState and introduce the smart routing var as well.

Reviewable status: 12 of 13 files reviewed, all discussions resolved

@buggmagnet buggmagnet force-pushed the add-smart-routing-to-daita-settings-data-ios-838 branch from f82898e to 11bd868 Compare October 2, 2024 11:02
@rablador rablador force-pushed the add-smart-routing-to-daita-settings-data-ios-838 branch from 11bd868 to 9af680a Compare October 2, 2024 12:52
@pinkisemils pinkisemils merged commit ca45116 into main Oct 2, 2024
8 of 9 checks passed
@pinkisemils pinkisemils deleted the add-smart-routing-to-daita-settings-data-ios-838 branch October 2, 2024 12:55
Copy link

github-actions bot commented Oct 2, 2024

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

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.

3 participants