-
Notifications
You must be signed in to change notification settings - Fork 15
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
Notification Tracking #27
Notification Tracking #27
Conversation
e43ef70
to
1369d49
Compare
1369d49
to
f5a57cd
Compare
b09b34b
to
cf882b3
Compare
Podfile
Outdated
@@ -12,6 +12,14 @@ target 'AEPTarget' do | |||
pod 'AEPIdentity' | |||
end | |||
|
|||
target 'AEPTargetDemoApp' do | |||
pod 'AEPCore' | |||
pod 'AEPRulesEngine' |
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.
AEPCore has a dependency on AEPRulesEngine so this will be pulled in. We can omit it here unless we are targeting some specific version.
AEPTargetDemoApp/SceneDelegate.swift
Outdated
@@ -0,0 +1,64 @@ | |||
/* | |||
Copyright 2020 Adobe. All rights reserved. |
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.
Copyright 2021, check this for all other files you added
AEPTarget/Sources/Event+Target.swift
Outdated
|
||
/// Returns true if the event is location display request event | ||
var isLocationDisplayedEvent: Bool { | ||
return data?[TargetConstants.EventDataKeys.IS_LOCATION_DISPLAYED] != nil ? true : false |
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.
Optional: You can rewrite this as return data?[TargetConstants.EventDataKeys.IS_LOCATION_DISPLAYED] ?? false
|
||
MobileCore.setLogLevel(.trace) | ||
MobileCore.registerExtensions([Lifecycle.self, Identity.self, Target.self, Signal.self]) | ||
MobileCore.configureWith(appId: "launch-ENc28aaf2fb6934cff830c8d3ddc5465b1-development") |
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.
Can you please specify the name of the launch property/ Org as well here?
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.
Note: Let's integrate Assurance as well (in a future PR).
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.
For Later: You might want to implement Lifecycle in the app as we will need to check we are sending all lifecycle data to Target correctly.
AEPTarget/Sources/TargetState.swift
Outdated
private(set) var thirdPartyId: String? | ||
private(set) var thirdPartyId: String? { | ||
didSet { | ||
dataStore.set(key: TargetConstants.DataStoreKeys.THIRD_PARTY_ID, value: tntId) |
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.
value should be thirdPartyId and not tntId here, let's fix it.
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.
This is deleted as we dont store 3rd party in db
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.
This is needed as we discussed and here is the separate task logged for it - #21
AEPTarget/Sources/Target.swift
Outdated
// if not, Log and bail out | ||
|
||
guard let mboxJson: [String: Any?] = targetState.prefetchedMboxJsonDicts[mBoxName] ?? targetState.loadedMboxJsonDicts[mBoxName] else { | ||
Log.warning(label: Target.LOG_TAG, "\(TargetError.ERROR_CLICK_NOTIFICATION_SEND_FAILED) \(TargetError.ERROR_NO_CACHED_MBOX_FOUND) \(mBoxName) For more details refer to https://aep-sdks.gitbook.io/docs/using-mobile-extensions/adobe-target/target-api-reference#send-an-mbox-click-notification") |
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.
Let's remove doc reference from errors. It adds to clutter plus the doc location is subject to change.
AEPTarget/Sources/Target.swift
Outdated
|
||
// Get the configuration shared state | ||
guard let configuration = getSharedState(extensionName: TargetConstants.Configuration.EXTENSION_NAME, event: event)?.value else { | ||
Log.error(label: Target.LOG_TAG, "Location displayed unsuccessful, configuration is null") |
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.
Fix log message. It says "Location displayed..."
AEPTarget/Sources/Target.swift
Outdated
|
||
// bail out if the target configuration is not available or if the privacy is opted-out | ||
if let error = prepareForTargetRequest(configData: configuration) { | ||
Log.error(label: Target.LOG_TAG, TargetError.ERROR_DISPLAY_NOTIFICATION_SEND_FAILED + error) |
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.
Fix error - ERROR_DISPLAY_NOTIFICATION_SEND_FAILED. It should be for click notification
Log.trace(label: Target.LOG_TAG, "Handling Locations Displayed - event \(event.name) type: \(event.type) source: \(event.source) ") | ||
|
||
// Get the configuration shared state | ||
guard let configuration = getSharedState(extensionName: TargetConstants.Configuration.EXTENSION_NAME, event: event)?.value else { |
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.
Can you please add a TODO for getting the latest configuration wherever needed
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 might not need this 👆 because Configuration will resolve with forward/ backward resolve in case of pending. Nil will be returned only when there is no shared state.
AEPTarget/Sources/Target.swift
Outdated
let lifecycleSharedState = getSharedState(extensionName: TargetConstants.Lifecycle.EXTENSION_NAME, event: event)?.value | ||
let identitySharedState = getSharedState(extensionName: TargetConstants.Identity.EXTENSION_NAME, event: event)?.value | ||
|
||
for mBoxName: String in mBoxNames { |
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.
Optional: type is inferred, not needed.
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.
Feel free to address the comments in the same PR or next as convenient.
@@ -0,0 +1,25 @@ | |||
{ |
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 can remove this as we are integrating AEPCore via CocoaPods. It's needed when using SPM to resolve package dependencies.
prefetch() | ||
}.padding(10) | ||
|
||
Button("Location displayed") { |
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.
Minor: "Locations displayed"
|
||
MobileCore.setLogLevel(.trace) | ||
MobileCore.registerExtensions([Lifecycle.self, Identity.self, Target.self, Signal.self]) | ||
MobileCore.configureWith(appId: "launch-ENc28aaf2fb6934cff830c8d3ddc5465b1-development") |
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.
For Later: You might want to implement Lifecycle in the app as we will need to check we are sending all lifecycle data to Target correctly.
@@ -1,5 +1,5 @@ | |||
/* |
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.
rename DeliveryResponse -> TargetDeliveryResponse
@@ -0,0 +1,216 @@ | |||
/* |
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.
Rename DeliveryRequestBuilder -> TargetDeliveryRequestBuilder
var tokens: [String] = [] | ||
if let optionsArray = cachedMboxJson?[TargetConstants.TargetJson.OPTIONS] as? [[String: Any?]?] { | ||
for option in optionsArray { | ||
guard let optionEventToken: String = option?[TargetConstants.TargetJson.Metric.EVENT_TOKEN] as? String else { |
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.
guard let optionEventToken = ...
return notification | ||
} | ||
|
||
static func getClickedNotification(cachedMboxJson: [String: Any?], parameters: TargetParameters?, timestamp: Int64, lifecycleContextData: [String: String]?) -> Notification? { |
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.
Minor: rename parameters->targetParameters
let id = UUID().uuidString | ||
|
||
// Set parameters: getMboxParameters | ||
let mBoxparameters: [String: String] = getMboxParameters(mboxParameters: parameters?.parameters, lifecycleContextData: lifecycleContextData) |
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.
let mboxparameters = ...
|
||
let mboxName = cachedMboxJson[TargetConstants.TargetJson.Mbox.NAME] as? String ?? "" | ||
|
||
let mBox = Mbox(name: mboxName) |
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.
mBox->mbox
let mBox = Mbox(name: mboxName) | ||
|
||
guard let metrics = cachedMboxJson[TargetConstants.TargetJson.METRICS] as? [Any?] else { | ||
return Notification(id: id, timestamp: timestamp, type: TargetConstants.TargetJson.MetricType.CLICK, mbox: mBox, parameters: mBoxparameters, profileParameters: parameters?.profileParameters, order: parameters?.order?.toInternalOrder(), product: parameters?.product?.toInternalProduct()) |
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.
Can you confirm we send click notifications even without tokens?
Description
Pending
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: