Review Identity extension

[shalehaha]

part of #268

[shalehaha]

@kevinlind @emdobrin can one of you help us review the Identity extension?

[kevinlind]

@shalehaha Yes I can take a look, however we've already planned out this sprint. What is the timeline for this to be done?

[kevinlind]

Here are my review notes of the Identity extension.

Fix CustomIdentity equality

CustomIdentity objects are currently tested for equality using the identifier however they should be considered equal if they have the same type as is done in v5.
Update the following methods to generate map keyed on CustomIdentity.type instead of identifier:

aepsdk-core-ios/AEPIdentity/Sources/IdentityProperties.swift

Line 84 in 4d939d3

mutating func mergeAndCleanCustomerIds(_ newCustomerIds: [CustomIdentity]) {

aepsdk-core-ios/AEPIdentity/Sources/IdentityProperties.swift

Line 92 in 4d939d3

private func idListToDict(_ ids: [CustomIdentity]?) -> [String?: CustomIdentity] {

Update CustomIdentity to check two objects for equality just from type:

aepsdk-core-ios/AEPIdentity/Sources/CustomIdentity.swift

Line 35 in 4d939d3

extension CustomIdentity: Equatable {

Also, update test cases to validate the above. For example, with the current code I'm able to call setAdvertisingIdentifier twice and the resulting customerIds list will contain two ad id entries.

Remove Opt-out check from handleIdentityRequest:

aepsdk-core-ios/AEPIdentity/Sources/Identity.swift

Line 71 in 4d939d3

if shouldIgnore(event: event) {

Only syncIdentifiers is aborted if privacy is opted out, however that method already checks for the privacy status. The other API calls, appendToUrl, getUrlVariables, and Identifiers request are still valid even if privacy is opted out.

Remove duplicate default server constant

aepsdk-core-ios/AEPIdentity/Sources/IdentityConstants.swift

Line 23 in 4d939d3

static let DEFAULT_SERVER = "dpm.demdex.net"

Rename MID to ECID throughout

Even though the network calls still use mid, in v5 we've been changing the wording, especially in log entries, to use the new ECID name.

aepsdk-core-ios/AEPIdentity/Sources/MID.swift

Line 15 in 4d939d3

struct MID: Equatable, Codable, Hashable, CustomStringConvertible {

aepsdk-core-ios/AEPIdentity/Sources/IdentityProperties.swift

Line 19 in 4d939d3

var mid: MID?

Execute force sync event immediately

In bootup, the forced sync event should happen before other events, however there may have been Identity requests made to the SDK which are queued in the EventHub. Consider processing force sync event immediately, by calling syncIdentifiers directly.

aepsdk-core-ios/AEPIdentity/Sources/IdentityState.swift

Line 54 in 4d939d3

eventDispatcher(Event.forceSyncEvent())

In processPrivacyChange, same as with bootup, there's the possiblity of another event getting queued before this force sync. Also, and more importantly, this delay may cause issues with Target which depends on the ECID when there is a privacy change. We should really consider just calling syncIdentifiers here.

aepsdk-core-ios/AEPIdentity/Sources/IdentityState.swift

Line 232 in 4d939d3

eventDispatcher(event.forceSyncEvent())

Don't use 'next version' when creating a shared state

Setting the shared state to the latest version can cause issues. This new shared state's version will be greater than the event version of any event in the Identity queue. If any of those events in the queue try to create a shared state it will fail. Instead, as was done in v5, need to add an event to the back of the Identity event queue which will create this shared state with the version of that event.

aepsdk-core-ios/AEPIdentity/Sources/IdentityState.swift

Line 330 in 4d939d3

createSharedState(identityProperties.toEventData(), nil)

aepsdk-core-ios/AEPIdentity/Sources/IdentityState.swift

Line 340 in 4d939d3

createSharedState(identityProperties.toEventData(), nil)

Update log entry

Update log message so it's not so dreadful. An aborted sync is common, for example when an app always sets the same Ad ID on launch. Instead try "Ignored an ID sync request because no new IDs to sync after the last request."

aepsdk-core-ios/AEPIdentity/Sources/IdentityState.swift

Line 129 in 4d939d3

Log.debug(label: LOG_TAG, "sync identifiers request failed, shouldSync returned false.")

[nporter-adbe]

Comment to track the feedback from @kevinlind

• Fix CustomIdentity equality (Fix equality for CustomIdentity #308)
• Remove Opt-out check from handleIdentityRequest (Fix equality for CustomIdentity #308)
• Remove duplicate default server constant (Identity feedback (MID -> ECID, log, constant) #307)
• Rename MID to ECID throughout (Identity feedback (MID -> ECID, log, constant) #307)
• Execute force sync event immediately (Block Identity queue until we can force sync on bootup #315)
• Don't use 'next version' when creating a shared state (already addressed in Version nil event shared state at 0 #299)
• Update log entry (Identity feedback (MID -> ECID, log, constant) #307)

[nporter-adbe]

Closing as all feedback has been addressed. Thanks for the thoughtful feedback @kevinlind.