Added support for Target session Id setter & getter by swarna04 · Pull Request #115 · adobe/aepsdk-target-ios

swarna04 · 2022-06-30T17:00:59Z

Description

Ref: [MOB-16753], [MOB-16754]

Added Target session Id getter and setter APIs in the SDK to support hybrid implementations.

Added new public APIs getSessionId and setSessionId
Added unit, functional and integration tests to verify the functionality.

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

PravinPK · 2022-07-19T17:43:31Z

AEPTarget/Sources/Target+PublicAPI.swift

+    /// - Parameter id: a string containing the value of the Target session Id to be set in the SDK.
+    static func setSessionId(_ id: String?) {
+        let eventData = [TargetConstants.EventDataKeys.TARGET_SESSION_ID: id ?? ""]
+        let event = Event(name: TargetConstants.EventName.REQUEST_IDENTITY, type: EventType.target, source: EventSource.requestIdentity, data: eventData)


it would be nice to have the eventName be more evident, we can call them
"Set Target Session Id" rather than "TargetRequestIdentity" for all the API's

Griffon debugging would be easier

PravinPK · 2022-07-19T17:44:12Z

AEPTarget/Sources/Target+PublicAPI.swift

+    ///
+    /// - Parameter completion: the callback `closure` invoked with the current session Id or `nil` if there was an error retrieving it.
+    static func getSessionId(_ completion: @escaping (String?, Error?) -> Void) {
+        let event = Event(name: TargetConstants.EventName.REQUEST_IDENTITY, type: EventType.target, source: EventSource.requestIdentity, data: nil)


Similar to above - EventName can be "Get Target Session Identifier" rather than "TargetRequestIdentity"

Yeah, I'd planned to clean up this code later but since you've mentioned I'll take it up here.

PravinPK · 2022-07-19T18:20:35Z

AEPTarget/Sources/Target+PublicAPI.swift

+    /// persisted for a period defined by `target.sessionTimeout` configuration setting.
+    ///
+    /// - Parameter completion: the callback `closure` invoked with the current session Id or `nil` if there was an error retrieving it.
+    static func getSessionId(_ completion: @escaping (String?, Error?) -> Void) {


At some point, we should consider using Swift Result's as opposed to callback with optional String and Error
Docs:
https://www.hackingwithswift.com/articles/161/how-to-use-result-in-swift
https://developer.apple.com/documentation/swift/result

Sure, it's probably a more elegant way to return multiple parameters in asynchronous APIs. I believe it does have some drawbacks in terms of Objective-C compatibility, however we could definitely explore more here.

PravinPK · 2022-07-19T18:34:58Z

AEPTarget/Sources/Target+PublicAPI.swift

+    /// This ID is preserved between app upgrades, is saved and restored during the standard application
+    /// backup process, and is removed at uninstall, upon privacy status update to opt out or when AEPTarget.resetExperience is called.
+    ///
+    /// - Parameter id: a string containing the value of the Target session Id to be set in the SDK.


We can also mention in the docs that an empty or nil sessionId value will reset the target session

PravinPK · 2022-07-19T18:36:53Z

AEPTarget/Sources/Target.swift

-            setThirdPartyId(thirdPartyId: thirdPartyId, event: event)
-        } else {
-            dispatchRequestIdentityResponse(triggerEvent: event)
+        if let eventData = event.data {


A handful of functions in this file are missing docs.. May be we can update them in a later PR or have a JIRA created for them

I'll create a JIRA ticket to update any missing docs for existing methods outside of this work.

PravinPK · 2022-07-19T18:38:52Z

AEPTarget/Sources/Target.swift

+        }
+
+        if sessionId != targetState.storedSessionId {
+            Log.trace(label: Target.LOG_TAG, "setSessionId - Updated Target session Id with the provided value \(sessionId).")


Can this be Log.debug as well ?

PravinPK · 2022-07-19T19:05:10Z

AEPTarget/Sources/TargetState.swift

+    }

-    private(set) var storedSessionId: String
+#if DEBUG


just checking:

Wondering if we can remove the if DEBUG flag and keep setting storedSessionId as still private
Aaand for the Functional Test, we can test setSessionId and GetSessionID together.. so we don't have to mock targetState.sessionId

Yup. I've revisited this code since we need to be able to update the sessionId from Target extension class as well. In summary, exposed another method in state that allows updating (and resetting) the session Id.

codecov · 2022-07-21T20:48:36Z

Codecov Report

Merging #115 (53a58b5) into dev-v3.2.0 (e2b4fda) will decrease coverage by 0.27%.
The diff coverage is 85.07%.

@@              Coverage Diff               @@
##           dev-v3.2.0     #115      +/-   ##
==============================================
- Coverage       90.40%   90.13%   -0.27%     
==============================================
  Files              24       24              
  Lines            1261     1307      +46     
==============================================
+ Hits             1140     1178      +38     
- Misses            121      129       +8

sbenedicadb

looks good - a couple of small changes requested.

can we also update the objc test app with these new apis?

sbenedicadb · 2022-07-22T20:28:33Z

AEPTarget/Sources/Target+PublicAPI.swift

+            guard let responseEvent = responseEvent else {
+                let error = "Request to get Target session Id failed with error, \(TargetError.ERROR_TIMEOUT)"
+                completion(nil, TargetError(message: error))
+                Log.error(label: Target.LOG_TAG, error)


error is too aggressive for these logs - we should scale these back to warnings

sbenedicadb · 2022-07-22T20:35:39Z

AEPTarget/Sources/Target+PublicAPI.swift

+                Log.error(label: Target.LOG_TAG, error)
+                return
+            }
+            guard let sessionId = eventData[TargetConstants.EventDataKeys.TARGET_SESSION_ID] as? String else {


isn't it valid for there to be no session? i'm thinking this case should be handled differently.

There will always be a Target session (and a session Id). What scenario do you have in mind?

prior to using target for the first time the session id is nil right?

also wondered if this getter should take timeout into consideration. i.e. - if there is a session id but it's expired, should calling this getter return nil?

Yes, the session Id is generated upon the first Target request. Also, if the session has expired, the subsequent get call will return the new session Id generated by the SDK. This session Id is then used in all future Target requests and can also be passed to another channel to have the same session across.

sbenedicadb · 2022-07-22T20:47:23Z

AEPTarget/Sources/Target.swift

+    /// - Parameters:
+    ///     - sessionId: new session Id that needs to be set in the SDK
+    private func setSessionId(sessionId: String) {
+        if targetState.privacyStatusIsOptOut {


given this is a block that returns, it should be switched to use the guard/else pattern

sbenedicadb · 2022-07-22T21:07:31Z

AEPTarget/Sources/TargetState.swift

+    private(set) var storedSessionId: String {
+        didSet {
+            if storedSessionId.isEmpty {
+                dataStore.remove(key: TargetConstants.DataStoreKeys.SESSION_ID)
+            } else {
+                dataStore.set(key: TargetConstants.DataStoreKeys.SESSION_ID, value: storedSessionId)
+            }
+        }
+    }


i like this optimization 👍

swarna04 · 2022-07-25T13:32:53Z

can we also update the objc test app with these new apis?

The objC test app is updated for both sessionId and tntId as part of the other pr #116.

* Added support for Target session Id setter & getter (#115) * Added support for Target session Id setter/ getter * code cleanup * Fixed formatting issue * Incorporated feedback * Incorporated feedback * Added support for TntId setter API (#116) * Added support for Target session Id setter/ getter * code cleanup * Fixed formatting issue * Added support for Target tntId setter * code cleanup * fixed formatting * fixed definition conflict test issue * Incorporated feedback * Incorporated feedback * Using distinct Event name * More testapp updates * Release prep v3.2.0 (#117) * release prep v3.2.0 * Feedback updates * Dependency and doc updates * Incorporated feedback * Fixed issue: Persisted edge host value was not used for Target request issued upon app close & relaunch (#119) * Fixed an issue where persisted edge host value was not used for Target request issued upon app close & relaunch * minor doc update * Target doc updates (#120) * Target doc updates * docs cleanup

swarna04 added 3 commits June 30, 2022 09:57

Added support for Target session Id setter/ getter

9c5881b

code cleanup

c4bb52b

Fixed formatting issue

20e0dc8

swarna04 requested a review from PravinPK July 18, 2022 17:39

PravinPK requested changes Jul 19, 2022

View reviewed changes

swarna04 requested a review from sbenedicadb July 20, 2022 15:51

Incorporated feedback

5294c8b

sbenedicadb requested changes Jul 22, 2022

View reviewed changes

Incorporated feedback

53a58b5

sbenedicadb approved these changes Jul 25, 2022

View reviewed changes

swarna04 merged commit b8f5213 into adobe:dev-v3.2.0 Jul 25, 2022

Conversation

swarna04 commented Jun 30, 2022

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sbenedicadb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swarna04 commented Jul 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jul 21, 2022 •

edited

Loading