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

Refactor key store #68

Merged
merged 21 commits into from
Nov 14, 2019
Merged

Refactor key store #68

merged 21 commits into from
Nov 14, 2019

Conversation

connected-rkingsmill
Copy link
Contributor

No description provided.

… and keyStore

Add init(with verification) and remove verify and login functions - should be done when creating AccountManager
…eated after app launch and from the LoginVC and LandingVC
Consolidate init functions into two - one that handles the init from appLaunch with the stored credentials and the other that handles the init from the verify call
Remove shared instance
Remove login call - move to be called directly on GuardianAPI from ViewController
Add file for new Account class
Move the following functionality from AccountManager to Account:
-addCurrentDevice
-setUser
-removeDevice
-storing of credentials, token, currentDevice and user
…Account

Update DependencyProviding AccountManaging with these changes
Update everywhere in the app to refer to accountManager.account for the refactored properties and functions
Change user to have only one boolean to indicate if user hasReachedMaxDevices
Remove function that loops through countries to match the city to determine country code
# Conflicts:
#	FirefoxPrivateNetworkVPN.xcodeproj/project.pbxproj
@@ -13,7 +13,7 @@ import Foundation

struct Device: Codable, UserDefaulting {

static let userDefaultsKey = "currentDevice"
static let userDefaultsKey = "device"

Choose a reason for hiding this comment

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

We'll have to let people know that they'll need to log in again if we change this.

@@ -32,7 +32,8 @@ struct VPNCountry: Codable {
code: originalCity.code,
latitude: originalCity.latitude,
longitude: originalCity.longitude,
servers: [server]
servers: [server],
flagCode: self.code

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved: 9bce0f6

@@ -55,6 +56,7 @@ struct VPNCity: UserDefaulting {
let latitude: Float
let longitude: Float
let servers: [VPNServer]
let flagCode: String

Choose a reason for hiding this comment

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

I wonder if this needs to be optional, since we won't have a flagCode value in the VPNCity JSON and it only gets set via VPNCountry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved: 9bce0f6

var user: User? { get }
var token: String? { get }
var currentDevice: Device? { get }
var account: Account? { get}

Choose a reason for hiding this comment

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

{ get } spacing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved: 9bce0f6

var accountManager: AccountManaging {
return AccountManager.sharedManager
}

Choose a reason for hiding this comment

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

Probably can discard this file's changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -39,14 +40,14 @@ class DeviceManagementDataSource: NSObject, UITableViewDataSource {
.subscribe { [weak tableView] event in
guard let deviceKey = event.element else { return }

let accountManager = DependencyFactory.sharedFactory.accountManager
accountManager.removeDevice(with: deviceKey) { result in
guard let account = DependencyFactory.sharedFactory.accountManager.account else { return }

Choose a reason for hiding this comment

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

This feels weird since we have "private let account = DependencyFactory.sharedFactory.accountManager.account" on the class right above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved: 9bce0f6

F4E8FA602374E00700D840D8 /* HyperlinkItem.swift */,
F4E8B2342368C39C001CEC8A /* TunnelAction.swift */,
F4B21E422366514700136316 /* VPNState.swift */,
F49090BD2379D43200C8430A /* SettingsItem.swift */,
F4A7FA23237C5C960034AF6C /* Account.swift */,
F4A7FA25237C5CE00034AF6C /* Credentials.swift */,

Choose a reason for hiding this comment

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

Sort alphabetically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved: 9bce0f6

Copy link

@corbyziesman corbyziesman left a comment

Choose a reason for hiding this comment

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

Some comments, but overall this all looks really good and makes this area of the code a lot more clear and understandable.

… it from DependencyFactory again in removeDeviceEvent

Sort files alphabetically (for Corby)
Make flagCode on VPNCity optional since it doesn't come from JSON
Fix spacing
… user defaults when current device is removed
…ms to String and instead has a description var
-Fixed conflicts in pbcproj file
@connected-rkingsmill connected-rkingsmill merged commit 1bac804 into develop Nov 14, 2019
@connected-rkingsmill connected-rkingsmill deleted the refactor-key-store branch November 14, 2019 21:20
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.

None yet

2 participants