Implement media extension #8

rymorale · 2021-03-04T21:56:49Z

Description

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.

- Handle tracker requests by creating a new session and then map the given tracker id to the returned session id - TODO: Handle track requests to call corresponding tracker APIs - Listen for configuration / privacy changes and update media states

rymorale · 2021-03-04T21:57:40Z

Marking WIP until MediaService is available and further functionality can be added. Opening PR for now to get some early feedback.

codecov · 2021-03-04T22:00:12Z

Codecov Report

Merging #8 (8a60db9) into dev (95dc9b9) will increase coverage by 0.54%.
The diff coverage is 89.32%.

@@            Coverage Diff             @@
##              dev       #8      +/-   ##
==========================================
+ Coverage   93.84%   94.38%   +0.54%     
==========================================
  Files          13       15       +2     
  Lines        1363     1459      +96     
==========================================
+ Hits         1279     1377      +98     
+ Misses         84       82       -2

praveek · 2021-03-04T23:35:06Z

AEPMedia/Sources/Media.swift

+    /// Parameters:
+    ///  - event: The `Event` for which shared state is to be retrieved.
+    ///  - dependencies: An array of names of event's dependencies.
+    private func updateMediaState(forEvent event: Event, dependencies: [String]) {


Let us remove the method from this class. We will let MediaService maintain MediaState updates.

praveek · 2021-03-04T23:35:47Z

AEPMedia/Sources/Media.swift

+    /// - Parameter:
+    ///   - event: The configuration response event
+    private func handleConfigurationResponseEvent(_ event: Event) {
+        updateMediaState(forEvent: event, dependencies: dependencies)


Can we also listen for EventHub SharedState update and pass it to MediaService ?

praveek · 2021-03-04T23:37:00Z

AEPMedia/Sources/Media.swift

+
+        let trackerConfig = trackerRequestData[MediaConstants.Tracker.EVENT_PARAM] as? [String: Any] ?? [:]
+
+        mediaState.setTrackerConfig(with: trackerConfig)


Tracker config is not stored in media state. We pass it to init method when creating a tracker.
It should be something like
trackerMapping[trackerId] = MediaTracker(config: trackerConfig, hitProcessor: mediaService)

praveek · 2021-03-04T23:39:55Z

AEPMedia/Sources/Media.swift

    public var name = MediaConstants.EXTENSION_NAME
    public var friendlyName = MediaConstants.FRIENDLY_NAME
    public static var extensionVersion = MediaConstants.EXTENSION_VERSION
    public var metadata: [String: String]?
+    let dependencies: [String] = [MediaConstants.Configuration.SHARED_STATE_NAME, MediaConstants.Identity.SHARED_STATE_NAME, MediaConstants.Analytics.SHARED_STATE_NAME]
+    #if DEBUG
+        var sessionIdToTrackerIdMapping: [String: String] = [:]


We don't map sessionId to trackerId.
instead
let trackerMapping: [String: MediaEventTracking]

praveek · 2021-03-04T23:40:22Z

AEPMedia/Sources/Media.swift

-        //TOO
+        // TODO: revisit when media service implemented
+
+        updateMediaState(forEvent: event, dependencies: dependencies)


We don't need to do it for every trackRequest.

praveek · 2021-03-04T23:41:50Z

AEPMedia/Sources/Media.swift

+    #if DEBUG
+        var sessionIdToTrackerIdMapping: [String: String] = [:]
+        var mediaState: MediaState
+        var trackerCalled = false


We don't need this variable.

praveek · 2021-03-04T23:42:32Z

AEPMedia/Sources/MediaState.swift


 class MediaState {
    //TODO: implementation of this class.
+    private let LOG_TAG = "MediaState"
+    private(set) var privacyStatus: PrivacyStatus = .unknown
+    private var trackerConfig: [String: Any]?


We can remove this one.

praveek · 2021-03-04T23:44:04Z

AEPMedia/Sources/MediaState.swift

+    private(set) var blob: String?
+    private(set) var visitorCustomerIDs: [[String: Any]]?
+
+    private(set) var dispatchQueue: DispatchQueue = DispatchQueue(label: MediaConstants.FRIENDLY_NAME)


We really don't need a dispatchQueue just for updating variables.

praveek · 2021-03-04T23:44:37Z

AEPMedia/Sources/mediacollection/MediaCollectionEventTracking.swift

+ */
+import Foundation
+
+class MediaCollectionEventTracking: MediaEventTracking {


We don't need this class.

praveek

Changes requested

…implement-media-extension

- add stubs for MediaCoreTracker and MediaService - add FakeMediaCoreTracker for tests - remove references to sessionId

…implement-media-extension

addb · 2021-03-23T23:19:11Z

AEPMedia/Sources/Media.swift

+        let trackerConfig = eventData[MediaConstants.Tracker.EVENT_PARAM] as? [String: Any] ?? [:]
+
+        Log.debug(label: LOG_TAG, "\(#function) - Creating tracker with tracker id: \(trackerId).")
+        trackers[trackerId] = MediaEventTracker(hitProcessor: mediaService!, config: trackerConfig)


we should avoid forced unwrapping mediaService.

AEPMedia/Sources/Media.swift

addb · 2021-03-23T23:48:46Z

AEPMedia/Sources/Media.swift

+        }
+
+        guard let trackerId = eventData[MediaConstants.Tracker.ID] as? String, trackerId.count > 0 else {
+            Log.debug(label: LOG_TAG, "\(#function) - Tracker ID is nil, unable to create tracker.")


nit: "Tracker ID is invalid, unable to create internal tracker."

addb · 2021-03-24T00:01:07Z

AEPMedia/Sources/Media.swift

+        }
+
+        Log.debug(label: LOG_TAG, "\(#function) - tracking media for tracker id: \(trackerId).")
+        _ = trackMedia(trackerId: trackerId, eventData: eventData)


we can simply call trackMedia right?

i'll make trackMedia's result discardable

addb · 2021-03-24T00:03:14Z

AEPMedia/Sources/Media.swift

+    }
+
+    private func trackMedia(trackerId: String, eventData: [String: Any]) -> Bool {
+        if let tracker = trackers[trackerId] {


nit: I think guard let would be cleaner

addb · 2021-03-24T00:04:17Z

AEPMedia/Sources/MediaConstants.swift


 enum MediaConstants {
    static let EXTENSION_NAME                           = "com.adobe.module.media"
    static let FRIENDLY_NAME                            = "Media Analytics"
    static let EXTENSION_VERSION                        = "0.0.1"
    static let DATASTORE_NAME                           = EXTENSION_NAME
+    static let DEFAULT_PRIVACY_STATUS                   = PrivacyStatus.unknown


Should we have this in MediaState, since it is managed and handled there only.

yup good point. removed the constant and declared it within media state.

And we can get rid of core import as well then.

praveek · 2021-03-24T01:29:49Z

AEPMedia/Sources/Media.swift

    public var name = MediaConstants.EXTENSION_NAME
    public var friendlyName = MediaConstants.FRIENDLY_NAME
    public static var extensionVersion = MediaConstants.EXTENSION_VERSION
    public var metadata: [String: String]?
+    #if DEBUG
+        var trackers: [String: MediaEventTracker]


Can we use MediaEventTrackable protocol instead of MediaEventTracker?

praveek · 2021-03-24T01:30:36Z

AEPMedia/Sources/Media.swift

    public var name = MediaConstants.EXTENSION_NAME
    public var friendlyName = MediaConstants.FRIENDLY_NAME
    public static var extensionVersion = MediaConstants.EXTENSION_VERSION
    public var metadata: [String: String]?
+    #if DEBUG
+        var trackers: [String: MediaEventTracker]
+        var mediaState: MediaState


Can we remove mediaState from the extension and let MediaService maintain it?

yes, i'll update

praveek · 2021-03-24T01:31:24Z

AEPMedia/Sources/Media.swift


-    /// Handler for media  tracker creation events
+        if mediaState.getPrivacyStatus() == .optedOut {


We should directly read privacy status from EventData instead of reading it from Media state

praveek · 2021-03-24T01:32:31Z

AEPMedia/Sources/Media.swift

+    #if DEBUG
+        var trackers: [String: MediaEventTracker]
+        var mediaState: MediaState
+        var mediaService: MediaService?


Can we make MediaService non optional?

praveek · 2021-03-24T01:33:21Z

AEPMedia/Sources/Media.swift

+            return
+        }
+
+        guard let trackerId = eventData[MediaConstants.Tracker.ID] as? String, trackerId.count > 0 else {


nit: !trackerId.empty

praveek · 2021-03-24T01:35:01Z

AEPMedia/Sources/Media.swift

+        Log.debug(label: LOG_TAG, "\(#function) - Privacy status is opted-out. Clearing persisted media sessions.")
+        // clear tracked sessions and end all running sessions within the media service
+        trackers.removeAll()
+        mediaService?.abortAllSessions()


MediaService can internally call abortAllSessions in response to updateState.

rymorale added 3 commits March 3, 2021 14:54

Add Media Extension

d29d482

- Handle tracker requests by creating a new session and then map the given tracker id to the returned session id - TODO: Handle track requests to call corresponding tracker APIs - Listen for configuration / privacy changes and update media states

format

7e7212d

add unit tests for media extension

1359721

praveek reviewed Mar 4, 2021

View reviewed changes

rymorale added 2 commits March 11, 2021 12:10

Merge branch 'dev' of https://github.com/adobe/aepsdk-media-ios into …

651c2a3

…implement-media-extension

updates based on review

9a3fa1a

- add stubs for MediaCoreTracker and MediaService - add FakeMediaCoreTracker for tests - remove references to sessionId

rymorale changed the title ~~[WIP] Implement media extension~~ Implement media extension Mar 11, 2021

rymorale added 6 commits March 12, 2021 08:54

Merge branch 'dev' of https://github.com/adobe/aepsdk-media-ios into …

ad21613

…implement-media-extension

add test for sharedStateUpdateEvent

103323c

Merge branch 'dev' of https://github.com/adobe/aepsdk-media-ios into …

ab8043a

…implement-media-extension

fix issues after merging latest dev

6281f79

move mediahit to parent folder

afbbcc7

update MediaState within MediaService

5ba7c96

addb reviewed Mar 23, 2021

View reviewed changes

AEPMedia/Sources/Media.swift Show resolved Hide resolved

addb reviewed Mar 23, 2021

View reviewed changes

addb reviewed Mar 24, 2021

View reviewed changes

praveek reviewed Mar 24, 2021

View reviewed changes

rymorale added 2 commits March 23, 2021 19:06

review updates

9e35c81

additional review updates

8a60db9

praveek approved these changes Mar 24, 2021

View reviewed changes

rymorale merged commit 2f1e2ed into adobe:dev Mar 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement media extension #8

Implement media extension #8

rymorale commented Mar 4, 2021

rymorale commented Mar 4, 2021

codecov bot commented Mar 4, 2021 •

edited

Loading

praveek Mar 4, 2021

praveek Mar 4, 2021 •

edited

Loading

praveek Mar 4, 2021 •

edited

Loading

praveek Mar 4, 2021

praveek Mar 4, 2021

praveek Mar 4, 2021

praveek Mar 4, 2021

praveek Mar 4, 2021

praveek Mar 4, 2021

praveek left a comment

addb Mar 23, 2021

addb Mar 23, 2021

addb Mar 24, 2021

rymorale Mar 24, 2021 •

edited

Loading

addb Mar 24, 2021

addb Mar 24, 2021

rymorale Mar 24, 2021

addb Mar 24, 2021

praveek Mar 24, 2021

praveek Mar 24, 2021

rymorale Mar 24, 2021

praveek Mar 24, 2021

praveek Mar 24, 2021

praveek Mar 24, 2021

praveek Mar 24, 2021


		let trackerConfig = trackerRequestData[MediaConstants.Tracker.EVENT_PARAM] as? [String: Any] ?? [:]

		mediaState.setTrackerConfig(with: trackerConfig)


		/// Handler for media tracker creation events
		if mediaState.getPrivacyStatus() == .optedOut {

Implement media extension #8

Implement media extension #8

Conversation

rymorale commented Mar 4, 2021

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

rymorale commented Mar 4, 2021

codecov bot commented Mar 4, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

praveek Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

praveek Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praveek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymorale Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 4, 2021 •

edited

Loading

praveek Mar 4, 2021 •

edited

Loading

praveek Mar 4, 2021 •

edited

Loading

rymorale Mar 24, 2021 •

edited

Loading