Skip to content

[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

Merged
merged 24 commits into from
Nov 20, 2024
Merged

Conversation

muzahidul-opti
Copy link
Contributor

@muzahidul-opti muzahidul-opti commented Oct 15, 2024

Summary

Logical changes when vuid is not enabled

  • vuid is not generated/saved.
  • Delete any existing vuid (leftover by previous init).
  • “client-intialized” event not auto-fired.
  • vuid is not included in ODP events as a default attribuite for tracking.
  • createUserContext() will be rejected when userId is not provided.

Test plan

Issues

@muzahidul-opti muzahidul-opti marked this pull request as ready for review October 17, 2024 12:11
@muzahidul-opti muzahidul-opti requested a review from a team as a code owner October 17, 2024 12:11
Copy link
Contributor

@jaeopt jaeopt left a 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.

vuid = userId
fsUserId = nil
eventManager.identifyUser(vuid: userId, userId: nil)
} else if let _vuid = vuid {
Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor Author

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()
Copy link
Contributor

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.

Copy link
Contributor Author

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()
        }
    }
    

@muzahidul-opti muzahidul-opti requested a review from jaeopt October 30, 2024 17:03
* master:
  [FSSDK-10771] Implement UPS request batching for decideForKeys (#559)

# Conflicts:
#	OptimizelySwiftSDK.xcodeproj/project.pbxproj
jaeopt
jaeopt previously approved these changes Nov 15, 2024
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@muzahidul-opti muzahidul-opti merged commit 62ae49b into master Nov 20, 2024
9 of 12 checks passed
@muzahidul-opti muzahidul-opti deleted the muzahid/vuid-optln branch November 20, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants