-
Notifications
You must be signed in to change notification settings - Fork 353
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 account deletion in TunnelManager #5520
Refactor account deletion in TunnelManager #5520
Conversation
IOS-310 Refactor account deletion in TunnelManager
Currently
Completion handler executes on main queue however it taps into private We should perhaps look into consolidating the entire "deletion" logic inside of
Note that operation will ensure that completion is dispatched on main queue. Also: I suggest to look at how If you decide to consolidation account deletion + tunnel removal into
Side note on current architecture of tunnel manager
|
36d7a78
to
47e4976
Compare
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.
Nice one !
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
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 @rablador)
ios/MullvadVPN/TunnelManager/SetAccountOperation.swift
line 164 at r1 (raw file):
) { deleteAccount(accountNumber: accountNumber) { [self] result in interactor.setSettings(LatestTunnelSettings(), persist: true)
You should consider erasing the last used account number since it's no longer exists but of course you'd have to check the result
before you can make that assumption. i.e if result.isSuccess { /* remove last used account number */ }
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 6 of 6 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)
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 @pronebird)
ios/MullvadVPN/TunnelManager/SetAccountOperation.swift
line 164 at r1 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
You should consider erasing the last used account number since it's no longer exists but of course you'd have to check the
result
before you can make that assumption. i.eif result.isSuccess { /* remove last used account number */ }
This is currently handled in TunnelManager.deleteAccount
, but I'll move it into startDeleteAccountFlow
instead.
94a925c
to
c1b96c8
Compare
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 r2.
Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @pronebird and @rablador)
ios/MullvadVPN/TunnelManager/TunnelManager.swift
line 444 at r2 (raw file):
completion: ((Error?) -> Void)? = nil ) { setAccount(action: .delete(accountNumber)) { [weak self] result in
Variable 'self' was written to, but never read
c1b96c8
to
7963de8
Compare
Currently
TunnelManager.deleteAccount()
is somewhat fragmented.DeleteAccountOperation
only runs a network request that deletes account from backend, but it leaves the part for removing the tunnel and logging out device on completionHandler.Completion handler executes on main queue however it taps into private
unsetTunnelConfiguration()
which returns control back on private queue. Subsequent call back to UI continues on the same private queue which violates UI threading model (all UI calls must happen on main queue).This PR removes
DeleteAccountOperation
altogether and adds a new.delete
action toSetAccountOperation
, thereby consolidating account creation and deletion into one operation.This change is