chore: remove session manager completion handlers - WPB-23401#4301
Conversation
# Conflicts: # wire-ios-sync-engine/Source/SessionManager/SessionManager+APIVersionResolver.swift
c6db555 to
58b7054
Compare
|
Test Results2 979 tests 2 952 ✅ 3m 27s ⏱️ Results for commit 58b7054. Summary: workflow run #22055786101 |
| } | ||
|
|
||
| confirmSwitchingAccount { [weak self] isConfirmed in | ||
| let isConfirmed = await confirmSwitchingAccount() |
There was a problem hiding this comment.
thought: not really part of the scope here but I'm curious to see what you think. Calling this method will ask a ui delegate to present an alert and request user confirmation. Do you think this kind of back and forth communication makes sense? Or would it be better to have a single direction: ui tries to select an account, if confirmation is needed, throw error, ui handles, gets confirmation, tries again passing in a confirmation flag. Something liKe:
public func select(_ account: Account, authoritative: Bool) async throws -> ZMUserSession? {
if confirmationNeeded && !authoritative {
throw Error.confirmationNeeded
}
// continue...
}| return nil | ||
| } | ||
|
|
||
| return await withCheckedContinuation { continuation in |
There was a problem hiding this comment.
suggestion: if you follow the path delegate.sessionManagerWillOpenAccount you'll see that it does nothing but immediately invoke the userSessionCanBeTornDown completion handler. I think you can simplify this by removing the delegate method as well as the AppState.loading case.
| select(secondAccount, tearDownCompletion: { [weak self] in | ||
| self?.tearDownSessionAndDelete(account: account, eraseData: eraseData) | ||
| }) | ||
| Task { |
There was a problem hiding this comment.
question: why not make delete(account:reason:eraseData) async?



Issue
This PRs converts a number of UserSession methods with completion handlers to async methods. Necessarily this requires small changes in behavior that are hard to understand due to for example the addition of
Taskin various places which may slightly change the order of execution. I've tried to look at where these have been added and think it is safe (often it is in a kind of fire and forget context) but these changes should be well tested.Testing
These are not specific test steps but more scenarios
stagingfor the obsolete backend, app restarts, error is shown - select switch accounts to go to production.Legacy authentication:
Checklist
[WPB-XXX].