fix(decide): change nil to NSNull for decision flag notification (#386)

jaeopt · web-flow · commit f1995d5dbb0f · 2021-02-01T10:54:48.000-08:00
* change nil to NSNull for decision flag notification
* fix decisionEventDispatched false set
diff --git a/DemoSwiftApp/AppDelegate.swift b/DemoSwiftApp/AppDelegate.swift
@@ -181,6 +181,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
             self.user = self.optimizely.createUserContext(userId: self.userId,
                                                           attributes: self.attributes)
             let decision = self.user.decide(key: self.featureKey, options: [.includeReasons])
+            print("DECISION = \(decision)")
             if let variationKey = decision.variationKey {
                 self.openVariationView(variationKey: variationKey)
             } else {
diff --git a/Sources/Implementation/DecisionInfo.swift b/Sources/Implementation/DecisionInfo.swift
@@ -138,8 +138,8 @@ struct DecisionInfo {
             decisionInfo[Constants.DecisionInfoKeys.flagKey] = flagKey
             decisionInfo[Constants.DecisionInfoKeys.enabled] = enabled
             decisionInfo[Constants.DecisionInfoKeys.variables] = variableValues
-            decisionInfo[Constants.DecisionInfoKeys.variationKey] = variation?.key
-            decisionInfo[Constants.DecisionInfoKeys.ruleKey] = ruleKey
+            decisionInfo[Constants.DecisionInfoKeys.variationKey] = variation?.key ?? NSNull()      // keep key in the map even with nil value
+            decisionInfo[Constants.DecisionInfoKeys.ruleKey] = ruleKey ?? NSNull()                  //
             decisionInfo[Constants.DecisionInfoKeys.reasons] = reasons
             decisionInfo[Constants.DecisionInfoKeys.decisionEventDispatched] = decisionEventDispatched
         }
diff --git a/Sources/Implementation/Events/BatchEventBuilder.swift b/Sources/Implementation/Events/BatchEventBuilder.swift
@@ -1,5 +1,5 @@
 /****************************************************************************
- * Copyright 2019-2020, Optimizely, Inc. and contributors                   *
+ * Copyright 2019-2021, Optimizely, Inc. and contributors                   *
  *                                                                          *
  * Licensed under the Apache License, Version 2.0 (the "License");          *
  * you may not use this file except in compliance with the License.         *
@@ -32,10 +32,6 @@ class BatchEventBuilder {
                                       ruleType: String,
                                       enabled: Bool) -> Data? {
         
-        if (ruleType == Constants.DecisionSource.rollout.rawValue || variation == nil) && !config.sendFlagDecisions {
-            return nil
-        }
-        
         let metaData = DecisionMetadata(ruleType: ruleType, ruleKey: experiment?.key ?? "", flagKey: flagKey, variationKey: variation?.key ?? "", enabled: enabled)
         
         let decision = Decision(variationID: variation?.id ?? "",
diff --git a/Sources/Optimizely+Decide/OptimizelyClient+Decide.swift b/Sources/Optimizely+Decide/OptimizelyClient+Decide.swift
@@ -62,14 +62,17 @@ extension OptimizelyClient {
         }
         
         if !allOptions.contains(.disableDecisionEvent) {
-            sendImpressionEvent(experiment: decision?.experiment,
-                                variation: decision?.variation,
-                                userId: userId,
-                                attributes: attributes,
-                                flagKey: feature.key,
-                                ruleType: decision?.source ?? Constants.DecisionSource.rollout.rawValue,
-                                enabled: enabled)
-            decisionEventDispatched = true
+            let ruleType = decision?.source ?? Constants.DecisionSource.rollout.rawValue
+            if shouldSendDecisionEvent(source: ruleType, decision: decision) {
+                sendImpressionEvent(experiment: decision?.experiment,
+                                    variation: decision?.variation,
+                                    userId: userId,
+                                    attributes: attributes,
+                                    flagKey: feature.key,
+                                    ruleType: ruleType,
+                                    enabled: enabled)
+                decisionEventDispatched = true
+            }
         }
         
         var variableMap = [String: Any]()
diff --git a/Sources/Optimizely+Decide/OptimizelyDecision.swift b/Sources/Optimizely+Decide/OptimizelyDecision.swift
@@ -68,3 +68,26 @@ extension OptimizelyDecision: Equatable {
             lhs.reasons == rhs.reasons
     }
 }
+
+extension OptimizelyDecision: CustomStringConvertible {
+    public var description: String {
+        let variationKey = self.variationKey == nil ? "nil" : ("\"" + self.variationKey! + "\"")
+        let ruleKey = self.ruleKey == nil ? "nil" : ("\"" + self.ruleKey! + "\"")
+        
+        return """
+        {
+          variationKey: \(variationKey)
+          enabled: \(enabled)
+          variables: \(variables)
+          ruleKey: \(ruleKey)
+          flagKey: "\(flagKey)"
+          userContext: \(userContext)
+          reasons: [
+        """
+            + (reasons.isEmpty ? ""  : reasons.reduce("\n") {$0 +  "    - \($1)\n"}) +
+        """
+          ]
+        }
+        """
+    }
+}
diff --git a/Sources/Optimizely+Decide/OptimizelyUserContext.swift b/Sources/Optimizely+Decide/OptimizelyUserContext.swift
@@ -139,3 +139,9 @@ extension OptimizelyUserContext: Equatable {
     }
     
 }
+
+extension OptimizelyUserContext: CustomStringConvertible {
+    public var description: String {
+        return "{ userId: \(userId), attributes: \(attributes) }"
+    }
+}
diff --git a/Sources/Optimizely/OptimizelyClient.swift b/Sources/Optimizely/OptimizelyClient.swift
@@ -404,13 +404,15 @@ open class OptimizelyClient: NSObject {
             logger.i(.featureNotEnabledForUser(featureKey, userId))
         }
 
-        sendImpressionEvent(experiment: pair?.experiment,
-                            variation: pair?.variation,
-                            userId: userId,
-                            attributes: attributes,
-                            flagKey: featureKey,
-                            ruleType: source,
-                            enabled: featureEnabled)
+        if shouldSendDecisionEvent(source: source, decision: pair) {
+            sendImpressionEvent(experiment: pair?.experiment,
+                                variation: pair?.variation,
+                                userId: userId,
+                                attributes: attributes,
+                                flagKey: featureKey,
+                                ruleType: source,
+                                enabled: featureEnabled)
+        }
         
         sendDecisionNotification(userId: userId,
                                  attributes: attributes,
@@ -760,6 +762,11 @@ open class OptimizelyClient: NSObject {
 
 extension OptimizelyClient {
     
+    func shouldSendDecisionEvent(source: String, decision: FeatureDecision?) -> Bool {
+        guard let config = self.config else { return false }
+        return (source == Constants.DecisionSource.featureTest.rawValue && decision?.variation != nil) || config.sendFlagDecisions
+    }
+    
     func sendImpressionEvent(experiment: Experiment?,
                              variation: Variation?,
                              userId: String,
diff --git a/Sources/Optimizely/OptimizelyError.swift b/Sources/Optimizely/OptimizelyError.swift
@@ -97,11 +97,13 @@ extension OptimizelyError: CustomStringConvertible, ReasonProtocol {
         switch self {
         case .generic:                                      message = "Unknown reason."
             
+        // DO NOT CHANGE these critical error messages - FSC will validate exact-wordings of these messages.
         case .sdkNotReady:                                  message = "Optimizely SDK not configured properly yet."
         case .featureKeyInvalid(let key):                   message = "No flag was found for key \"\(key)\"."
         case .variableValueInvalid(let key):                message = "Variable value for key \"\(key)\" is invalid or wrong type."
         case .invalidJSONVariable:                          message = "Invalid variables for OptimizelyJSON."
 
+        // These error messages not validated by FSC
         case .experimentKeyInvalid(let key):                message = "Experiment key (\(key)) is not in datafile. It is either invalid, paused, or archived."
         case .experimentIdInvalid(let id):                  message = "Experiment ID (\(id)) is not in datafile."
         case .experimentHasNoTrafficAllocation(let key):    message = "No traffic allocation rules are defined for experiement (\(key))."
diff --git a/Sources/Optimizely/OptimizelyJSON.swift b/Sources/Optimizely/OptimizelyJSON.swift
@@ -199,3 +199,9 @@ extension OptimizelyJSON {
     }
     
 }
+
+extension OptimizelyJSON {
+    public override var description: String {
+        return "\(map)"
+    }
+}
diff --git a/Tests/OptimizelyTests-Common/BatchEventBuilderTests_Events.swift b/Tests/OptimizelyTests-Common/BatchEventBuilderTests_Events.swift
@@ -1,5 +1,5 @@
 /****************************************************************************
- * Copyright 2019-2020, Optimizely, Inc. and contributors                   *
+ * Copyright 2019-2021, Optimizely, Inc. and contributors                   *
  *                                                                          *
  * Licensed under the Apache License, Version 2.0 (the "License");          *
  * you may not use this file except in compliance with the License.         *
@@ -99,42 +99,6 @@ class BatchEventBuilderTests_Events: XCTestCase {
         XCTAssertNil(de["value"])
     }
     
-    func testCreateImpressionEventWithSendFlagDecisions() {
-        let scenarios: [String: Bool] = [
-            "experiment": true,
-            "anything-else": true,
-            Constants.DecisionSource.featureTest.rawValue: true,
-            Constants.DecisionSource.rollout.rawValue: false
-        ]
-        let attributes: [String: Any] = [
-            "s_foo": "foo",
-            "b_true": true,
-            "i_42": 42,
-            "d_4_2": 4.2
-        ]
-        let experiment = optimizely.config?.getExperiment(id: "10390977714")
-        let variation = experiment?.getVariation(id: "10416523162")
-        
-        for scenario in scenarios {
-            let event = BatchEventBuilder.createImpressionEvent(config: optimizely.config!, experiment: experiment!, variation: variation, userId: userId, attributes: attributes, flagKey: experiment!.key, ruleType: scenario.key, enabled: true)
-            scenario.value ? XCTAssertNotNil(event): XCTAssertNil(event)
-        }
-        
-        // nil variation should always return nil
-        for scenario in scenarios {
-            let event = BatchEventBuilder.createImpressionEvent(config: optimizely.config!, experiment: experiment!, variation: nil, userId: userId, attributes: attributes, flagKey: experiment!.key, ruleType: scenario.key, enabled: true)
-            XCTAssertNil(event)
-        }
-        
-        // should always return a event if sendFlagDecisions is set
-        optimizely.config?.project.sendFlagDecisions = true
-        for scenario in scenarios {
-            let event = BatchEventBuilder.createImpressionEvent(config: optimizely.config!, experiment: experiment!, variation: nil, userId: userId, attributes: attributes, flagKey: experiment!.key, ruleType: scenario.key, enabled: true)
-            XCTAssertNotNil(event)
-        }
-        optimizely.config?.project.sendFlagDecisions = nil
-    }
-    
     func testCreateImpressionEventWithoutVariation() {
         let attributes: [String: Any] = [
             "s_foo": "foo",
diff --git a/Tests/OptimizelyTests-Common/DecisionListenerTests.swift b/Tests/OptimizelyTests-Common/DecisionListenerTests.swift
diff --git a/Tests/OptimizelyTests-Common/OptimizelyUserContextTests_Decide.swift b/Tests/OptimizelyTests-Common/OptimizelyUserContextTests_Decide.swift

Original file line number	Diff line number	Diff line change
`@@ -139,3 +139,9 @@ extension OptimizelyUserContext: Equatable {`
`139`	`139`	`}`
`140`	`140`
`141`	`141`	`}`
	`142`	`+`
	`143`	`+extension OptimizelyUserContext: CustomStringConvertible {`
	`144`	`+ public var description: String {`
	`145`	`+ return "{ userId: \(userId), attributes: \(attributes) }"`
	`146`	`+ }`
	`147`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -199,3 +199,9 @@ extension OptimizelyJSON {`
`199`	`199`	`}`
`200`	`200`
`201`	`201`	`}`
	`202`	`+`
	`203`	`+extension OptimizelyJSON {`
	`204`	`+ public override var description: String {`
	`205`	`+ return "\(map)"`
	`206`	`+ }`
	`207`	`+}`