Better completion callback for activate (#5646)

paulb777 · web-flow · commit 2affb24d4a3a · 2020-05-26T18:01:04.000-07:00
diff --git a/FirebaseRemoteConfig/CHANGELOG.md b/FirebaseRemoteConfig/CHANGELOG.md
@@ -1,7 +1,11 @@
-# Unreleased
+# v4.5.0
 - [changed] Updated `fetchAndActivateWithCompletionHandler:` implementation to activate asynchronously. (#5617)
 - [fixed] Remove undefined class via removing unused proto generated source files. (#4334)
 - [added] Add an URLSession Partial Mock to enable testing without a backend. (#5633)
+- [added] Added activate API that returns a callback with an additional `bool` parameter indicating
+if the config has changed or not. The new API does not error if the console is unchanged. The old
+activate API with only an error parameter is deprecated and will be removed at the next major
+release. (#3586)
 
 # v4.4.11
 - [fixed] Fixed a bug where settings updates weren't applied before fetches. (#4740)
diff --git a/FirebaseRemoteConfig/Sources/FIRRemoteConfig.m b/FirebaseRemoteConfig/Sources/FIRRemoteConfig.m
@@ -278,18 +278,31 @@ - (BOOL)activateFetched {
   return didActivate;
 }
 
+typedef void (^FIRRemoteConfigActivateChangeCompletion)(BOOL changed, NSError *_Nullable error);
+
+- (void)activateWithCompletion:(FIRRemoteConfigActivateChangeCompletion)completion {
+  [self activateWithEitherHandler:completion deprecatedHandler:nil];
+}
+
 - (void)activateWithCompletionHandler:(FIRRemoteConfigActivateCompletion)completionHandler {
+  [self activateWithEitherHandler:nil deprecatedHandler:completionHandler];
+}
+
+- (void)activateWithEitherHandler:(FIRRemoteConfigActivateChangeCompletion)completion
+                deprecatedHandler:(FIRRemoteConfigActivateCompletion)deprecatedHandler {
   __weak FIRRemoteConfig *weakSelf = self;
   void (^applyBlock)(void) = ^(void) {
     FIRRemoteConfig *strongSelf = weakSelf;
     if (!strongSelf) {
       NSError *error = [NSError errorWithDomain:FIRRemoteConfigErrorDomain
                                            code:FIRRemoteConfigErrorInternalError
                                        userInfo:@{@"ActivationFailureReason" : @"Internal Error."}];
-      if (completionHandler) {
+      if (completion) {
         dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
-          completionHandler(error);
+          completion(NO, error);
         });
+      } else if (deprecatedHandler) {
+        deprecatedHandler(error);
       }
       FIRLogError(kFIRLoggerRemoteConfig, @"I-RCN000068", @"Internal error activating config.");
       return;
@@ -298,17 +311,21 @@ - (void)activateWithCompletionHandler:(FIRRemoteConfigActivateCompletion)complet
     // ignored.
     if (strongSelf->_settings.lastETagUpdateTime == 0 ||
         strongSelf->_settings.lastETagUpdateTime <= strongSelf->_settings.lastApplyTimeInterval) {
-      FIRLogWarning(kFIRLoggerRemoteConfig, @"I-RCN000069",
-                    @"Most recently fetched config is already activated.");
-      NSError *error = [NSError
-          errorWithDomain:FIRRemoteConfigErrorDomain
-                     code:FIRRemoteConfigErrorInternalError
-                 userInfo:@{
-                   @"ActivationFailureReason" : @"Most recently fetched config is already activated"
-                 }];
-      if (completionHandler) {
+      FIRLogDebug(kFIRLoggerRemoteConfig, @"I-RCN000069",
+                  @"Most recently fetched config is already activated.");
+      if (completion) {
         dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
-          completionHandler(error);
+          completion(NO, nil);
+        });
+      } else if (deprecatedHandler) {
+        NSError *error = [NSError
+            errorWithDomain:FIRRemoteConfigErrorDomain
+                       code:FIRRemoteConfigErrorInternalError
+                   userInfo:@{
+                     @"ActivationFailureReason" : @"Most recently fetched config already activated"
+                   }];
+        dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+          deprecatedHandler(error);
         });
       }
       return;
@@ -319,9 +336,13 @@ - (void)activateWithCompletionHandler:(FIRRemoteConfigActivateCompletion)complet
     [strongSelf updateExperiments];
     strongSelf->_settings.lastApplyTimeInterval = [[NSDate date] timeIntervalSince1970];
     FIRLogDebug(kFIRLoggerRemoteConfig, @"I-RCN000069", @"Config activated.");
-    if (completionHandler) {
+    if (completion) {
+      dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+        completion(YES, nil);
+      });
+    } else if (deprecatedHandler) {
       dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
-        completionHandler(nil);
+        deprecatedHandler(nil);
       });
     }
   };
diff --git a/FirebaseRemoteConfig/Sources/Public/FIRRemoteConfig.h b/FirebaseRemoteConfig/Sources/Public/FIRRemoteConfig.h
@@ -232,18 +232,24 @@ NS_SWIFT_NAME(RemoteConfig)
 
 #pragma mark - Apply
 
+/// Applies Fetched Config data to the Active Config, causing updates to the behavior and appearance
+/// of the app to take effect (depending on how config data is used in the app).
+/// @param completion Activate operation callback with changed and error parameters.
+- (void)activateWithCompletion:(void (^_Nullable)(BOOL changed,
+                                                  NSError *_Nullable error))completion;
+
 /// Applies Fetched Config data to the Active Config, causing updates to the behavior and appearance
 /// of the app to take effect (depending on how config data is used in the app).
 /// @param completionHandler Activate operation callback.
-- (void)activateWithCompletionHandler:(nullable FIRRemoteConfigActivateCompletion)completionHandler;
+- (void)activateWithCompletionHandler:(nullable FIRRemoteConfigActivateCompletion)completionHandler
+    DEPRECATED_MSG_ATTRIBUTE("Use -[FIRRemoteConfig activateWithCompletion:] instead.");
 
 /// This method is deprecated. Please use -[FIRRemoteConfig activateWithCompletionHandler:] instead.
 /// Applies Fetched Config data to the Active Config, causing updates to the behavior and appearance
 /// of the app to take effect (depending on how config data is used in the app).
 /// Returns true if there was a Fetched Config, and it was activated.
 /// Returns false if no Fetched Config was found, or the Fetched Config was already activated.
-- (BOOL)activateFetched DEPRECATED_MSG_ATTRIBUTE("Use -[FIRRemoteConfig activate] "
-                                                 "instead.");
+- (BOOL)activateFetched DEPRECATED_MSG_ATTRIBUTE("Use -[FIRRemoteConfig activate] instead.");
 
 #pragma mark - Get Config
 /// Enables access to configuration values by using object subscripting syntax.
@@ -265,8 +271,7 @@ NS_SWIFT_NAME(RemoteConfig)
 /// @param aNamespace       Config results under a given namespace.
 - (nonnull FIRRemoteConfigValue *)configValueForKey:(nullable NSString *)key
                                           namespace:(nullable NSString *)aNamespace
-    DEPRECATED_MSG_ATTRIBUTE("Use -[FIRRemoteConfig configValueForKey:] "
-                             "instead.");
+    DEPRECATED_MSG_ATTRIBUTE("Use -[FIRRemoteConfig configValueForKey:] instead.");
 
 /// Gets the config value of a given namespace and a given source.
 /// @param key              Config key.
@@ -281,8 +286,7 @@ NS_SWIFT_NAME(RemoteConfig)
 - (nonnull FIRRemoteConfigValue *)configValueForKey:(nullable NSString *)key
                                           namespace:(nullable NSString *)aNamespace
                                              source:(FIRRemoteConfigSource)source
-    DEPRECATED_MSG_ATTRIBUTE("Use -[FIRRemoteConfig configValueForKey:source:] "
-                             "instead.");
+    DEPRECATED_MSG_ATTRIBUTE("Use -[FIRRemoteConfig configValueForKey:source:] instead.");
 
 /// Gets all the parameter keys from a given source and a given namespace.
 ///
diff --git a/FirebaseRemoteConfig/Tests/FakeConsole/FakeConsoleTests.swift b/FirebaseRemoteConfig/Tests/FakeConsole/FakeConsoleTests.swift
@@ -23,6 +23,7 @@ class FakeConsoleTests: APITestBase {
     fakeConsole.config = ["Key1": "Value1"]
   }
 
+  // Test old API.
   // Contrast with testUnchangedActivateWillError in APITests.swift.
   func testChangedActivateWillNotError() {
     let expectation = self.expectation(description: #function)
@@ -59,6 +60,43 @@ class FakeConsoleTests: APITestBase {
     waitForExpectations()
   }
 
+  // Test New API.
+  // Contrast with testUnchangedActivateWillFlag in APITests.swift.
+  func testChangedActivateWillNotFlag() {
+    let expectation = self.expectation(description: #function)
+    config.fetch { status, error in
+      if let error = error {
+        XCTFail("Fetch Error \(error)")
+      }
+      XCTAssertEqual(status, RemoteConfigFetchStatus.success)
+      self.config.activate { changed, error in
+        XCTAssertNil(error)
+        XCTAssertTrue(changed)
+        XCTAssertEqual(self.config["Key1"].stringValue, "Value1")
+        expectation.fulfill()
+      }
+    }
+    waitForExpectations()
+
+    // Simulate updating console.
+    fakeConsole.config = ["Key1": "Value2"]
+
+    let expectation2 = self.expectation(description: #function + "2")
+    config.fetch { status, error in
+      if let error = error {
+        XCTFail("Fetch Error \(error)")
+      }
+      XCTAssertEqual(status, RemoteConfigFetchStatus.success)
+      self.config.activate { changed, error in
+        XCTAssertNil(error)
+        XCTAssert(changed)
+        XCTAssertEqual(self.config["Key1"].stringValue, "Value2")
+        expectation2.fulfill()
+      }
+    }
+    waitForExpectations()
+  }
+
   private func waitForExpectations() {
     let kFIRStorageIntegrationTestTimeout = 10.0
     waitForExpectations(timeout: kFIRStorageIntegrationTestTimeout,
diff --git a/FirebaseRemoteConfig/Tests/SwiftAPI/APITests.swift b/FirebaseRemoteConfig/Tests/SwiftAPI/APITests.swift
@@ -32,11 +32,8 @@ class APITests: APITestBase {
         XCTFail("Fetch Error \(error)")
       }
       XCTAssertEqual(status, RemoteConfigFetchStatus.success)
-      self.config.activate { error in
-        if let error = error {
-          // This API returns an error if the config was unchanged.
-          print("Activate Error \(error)")
-        }
+      self.config.activate { _, error in
+        XCTAssertNil(error)
         XCTAssertEqual(self.config["Key1"].stringValue, "Value1")
         expectation.fulfill()
       }
@@ -51,11 +48,8 @@ class APITests: APITestBase {
         XCTFail("Fetch Error \(error)")
       }
       XCTAssertEqual(status, RemoteConfigFetchStatus.success)
-      self.config.activate { error in
-        if let error = error {
-          // This API returns an error if the config was unchanged.
-          print("Activate Error \(error)")
-        }
+      self.config.activate { _, error in
+        XCTAssertNil(error)
         XCTAssertEqual(self.config["Key1"].stringValue, "Value1")
         expectation.fulfill()
       }
@@ -75,6 +69,7 @@ class APITests: APITestBase {
     waitForExpectations()
   }
 
+  // Test old API.
   // Contrast with testChangedActivateWillNotError in FakeConsole.swift.
   func testUnchangedActivateWillError() {
     let expectation = self.expectation(description: #function)
@@ -110,6 +105,39 @@ class APITests: APITestBase {
     waitForExpectations()
   }
 
+  // Test New API.
+  // Contrast with testChangedActivateWillNotFlag in FakeConsole.swift.
+  func testUnchangedActivateWillFlag() {
+    let expectation = self.expectation(description: #function)
+    config.fetch { status, error in
+      if let error = error {
+        XCTFail("Fetch Error \(error)")
+      }
+      XCTAssertEqual(status, RemoteConfigFetchStatus.success)
+      self.config.activate { changed, error in
+        XCTAssertTrue(!APITests.useFakeConfig || changed)
+        XCTAssertNil(error)
+        XCTAssertEqual(self.config["Key1"].stringValue, "Value1")
+        expectation.fulfill()
+      }
+    }
+    waitForExpectations()
+    let expectation2 = self.expectation(description: #function + "2")
+    config.fetch { status, error in
+      if let error = error {
+        XCTFail("Fetch Error \(error)")
+      }
+      XCTAssertEqual(status, RemoteConfigFetchStatus.success)
+      self.config.activate { changed, error in
+        XCTAssertFalse(changed)
+        XCTAssertNil(error)
+        XCTAssertEqual(self.config["Key1"].stringValue, "Value1")
+        expectation2.fulfill()
+      }
+    }
+    waitForExpectations()
+  }
+
   private func waitForExpectations() {
     let kFIRStorageIntegrationTestTimeout = 10.0
     waitForExpectations(timeout: kFIRStorageIntegrationTestTimeout,
