persistent deviceId #910
persistent deviceId #910
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a persistent deviceId that remains constant across app updates and reinstalls. Key changes include updates to test cases (using Self instead of class name), a non-optional deviceId in LocalStorage and its protocol, and modifications to keychain and constants to support persistent deviceId management.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit-tests/LocalStorageTests.swift | Refactored to use Self and updated tests for deviceId and keychain behavior |
| tests/common/MockLocalStorage.swift | Changed deviceId from an optional to a fixed non-optional string for consistent testing |
| swift-sdk/Internal/Utilities/LocalStorageProtocol.swift | Updated deviceId to a non-optional String with a getter only |
| swift-sdk/Internal/Utilities/LocalStorage.swift | Revised deviceId getter to incorporate persistent retrieval logic with state updates |
| swift-sdk/Internal/Utilities/Keychain/IterableKeychain.swift | No functional changes; maintains deviceId storage in the keychain |
| swift-sdk/Internal/InternalIterableAPI.swift | Simplified deviceId logic reflecting the new persistent approach |
| swift-sdk/Core/Constants.swift | Added a constant entry for deviceId |
| let foundDeviceId = [ | ||
| iterableUserDefaults.deviceId, | ||
| keychain.deviceId, | ||
| UIDevice.current.identifierForVendor?.uuidString | ||
| ].compactMap { $0 }.first | ||
| let deviceId = foundDeviceId ?? IterableUtil.generateUUID() | ||
|
|
||
| if iterableUserDefaults.deviceId == nil { | ||
| iterableUserDefaults.deviceId = deviceId | ||
| } | ||
| if keychain.deviceId == nil { | ||
| keychain.deviceId = deviceId | ||
| } | ||
| return deviceId |
There was a problem hiding this comment.
The getter for 'deviceId' modifies state by writing to user defaults and keychain, which may lead to unexpected side effects. Consider refactoring this logic so that state mutations occur in an explicit initialization or setter method rather than within a property getter.
| let foundDeviceId = [ | |
| iterableUserDefaults.deviceId, | |
| keychain.deviceId, | |
| UIDevice.current.identifierForVendor?.uuidString | |
| ].compactMap { $0 }.first | |
| let deviceId = foundDeviceId ?? IterableUtil.generateUUID() | |
| if iterableUserDefaults.deviceId == nil { | |
| iterableUserDefaults.deviceId = deviceId | |
| } | |
| if keychain.deviceId == nil { | |
| keychain.deviceId = deviceId | |
| } | |
| return deviceId | |
| return iterableUserDefaults.deviceId ?? keychain.deviceId ?? UIDevice.current.identifierForVendor?.uuidString ?? "" |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
No, provided code's logic is correct, we want to filter out first non nil from array of values, but if it fails generateUUID will be called, also you deleted persistency to UserDefaults and/or Keychain
|
Are those building errors related to 404 https://www.travis-ci.com/Iterable/swift-sdk ? |
✏️ Description
Constant value across the app update and reinstall process.