-
Notifications
You must be signed in to change notification settings - Fork 32
[FSSDK-10761] feat: make vuid as opt-in #556
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
Conversation
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.
Looks good. A few suggestions.
Sources/ODP/OdpManager.swift
Outdated
vuid = userId | ||
fsUserId = nil | ||
eventManager.identifyUser(vuid: userId, userId: nil) | ||
} else if let _vuid = vuid { |
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.
We also need the case covered -
vuid: nil and
user_id: valid not vuid
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.
I believe we have unit tests for all these combos. If missed, can we add one?
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.
Yah, test case is failing. Made the vuid optional to identifyUser
.
func identifyUser(vuid: String?, userId: String?) {
var identifiers = [String: String]()
if let _vuid = vuid {
identifiers[Constants.ODP.keyForVuid] = _vuid
}
let logger = OPTLoggerFactory.getLogger() | ||
|
||
// a single vuid should be shared for all SDK instances | ||
static let shared = OdpVuidManager() |
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.
We should keep it a singleton, so can share a single vuid per device.
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.
Declared singleton and add initialize method.
static let shared = VuidManager()
func initialize(enabled: Bool) {
self.enabled = enabled
if enabled {
self._vuid = load()
} else {
self.remove()
}
}
* master: [FSSDK-10771] Implement UPS request batching for decideForKeys (#559) # Conflicts: # OptimizelySwiftSDK.xcodeproj/project.pbxproj
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.
LGTM!
Summary
Logical changes when vuid is not enabled
Test plan
Issues