Recreate new installation after deletion from keychain #112
Conversation
Codecov Report
@@ Coverage Diff @@
## main #112 +/- ##
==========================================
+ Coverage 81.03% 81.04% +0.01%
==========================================
Files 65 65
Lines 5479 5482 +3
==========================================
+ Hits 4440 4443 +3
Misses 1039 1039
Continue to review full report at Codecov.
|
|
@funkenstrahlen can you try out this branch and see if you run into the same issue? The testcases pass and new installationIds are created |
cbaker6
changed the title
| #if !os(Linux) && !os(Android) | ||
| if let installationFromKeychain: CurrentInstallationContainer<BaseParseInstallation> | ||
| = try? KeychainStore.shared.get(valueFor: ParseStorage.Keys.currentInstallation) { | ||
| if installationFromKeychain.installationId == oldInstallationId { |
There was a problem hiding this comment.
What do you think about extending the test case here and also checking if installationFromKeychain.currentInstallation != nil?
There was a problem hiding this comment.
This may not always be the case by the time the test gets here because of the dispatching to main queue. If it is nil when it gets here, it won't be in a couple of seconds
There was a problem hiding this comment.
You're right. I haven't thought about the dispatch. The additional test case is not required. Has just been a thought as it's not checked right now.
There was a problem hiding this comment.
#113 should provide more predictable behavior. Let me know your thoughts on that thread
| DispatchQueue.main.async { | ||
| if let installationFromMemory: CurrentInstallationContainer<BaseParseInstallation> | ||
| = try? ParseStorage.shared.get(valueFor: ParseStorage.Keys.currentInstallation) { | ||
| if installationFromMemory.installationId == oldInstallationId { |
There was a problem hiding this comment.
What do you think about extending the test case here and also checking if installationFromKeychain.currentInstallation != nil?
| } | ||
| DispatchQueue.main.async { | ||
| if let installationFromKeychain = BaseParseInstallation.current { | ||
| if installationFromKeychain.installationId == oldInstallationId { |
There was a problem hiding this comment.
What do you think about extending the test case here and also checking if installationFromKeychain.currentInstallation != nil?
| #if !os(Linux) && !os(Android) | ||
| if let installationFromKeychain: CurrentInstallationContainer<BaseParseInstallation> | ||
| = try? KeychainStore.shared.get(valueFor: ParseStorage.Keys.currentInstallation) { | ||
| if installationFromKeychain.installationId == oldInstallationId { |
There was a problem hiding this comment.
What do you think about extending the test case here and also checking if installationFromKeychain.currentInstallation != nil?
| DispatchQueue.main.async { | ||
| if let installationFromMemory: CurrentInstallationContainer<BaseParseInstallation> | ||
| = try? ParseStorage.shared.get(valueFor: ParseStorage.Keys.currentInstallation) { | ||
| if installationFromMemory.installationId == oldInstallationId { |
There was a problem hiding this comment.
What do you think about extending the test case here and also checking if installationFromKeychain.currentInstallation != nil?
|
Thank you for quick response and the PR! I added some suggestions above to have a more detailed test suite. I added this branch to my project and can confirm it works a lot better now! Thank you! However I still need to manually save the newly created installation to the server after logout because every subsequent func logout() {
PodliveUser.logout { (result) in
switch result {
case .failure(let error):
print("Could not logout: " + error.localizedDescription)
case .success():
print("successfully logged out")
NotificationCenter.default.post(name: Notification.Name.userUpdated, object: nil)
PodliveInstallation.current?.save(completion: { (result) in
switch result {
case .success(_):
self.loginAsAnonymousUser()
case .failure(let error):
print("Could not save installation: " + error.localizedDescription)
}
})
}
}
}Do you think saving the installation to the server should be done by the framework after logout? |
I believe this comment may answer this question: #93 (comment) |
Since you logout/login immediately, I can see the possibility of odd behavior because of the need for Installation to be on the main queue. There can be times your anonymous login occurs before the new installation is created. You can minimize this odd behavior by logging in anonymously after a short time, say a second (dispatch after a second). The changes discussed in #113 should help with this also and you might not need to wait anytime after to log back in, but with the current implementation, you should wait a second or ask the user to press a button to login |
Thank you for linking the related issue thread. Actually I do have custom properties in my Installation object and therefore need to retrieve it manually. However I think you are absolutely correct that fetching the Installation at this point should not be part of the framework. |
That's correct. Great you thought about that! I think implementing #113 is the best approach to create a more predictable behavior. |
Even with the changes, you should probably implement one of my suggestions I mentioned above. You can see that because of threading the new installation may not be ready immediately in the keychain: Parse-Swift/Tests/ParseSwiftTests/ParseUserCombineTests.swift Lines 313 to 336 in b671243 |
Automatically create a new installation after it's deleted from the Keychain. A deletion typically occurs on user logout. Fix #110