Fix logic for first run to fix shouldShowMessage behavior (#228)

* Adding new `_firstRun`

* Remove redundant `_clientShowedMessage` var

* Enhance test to add incrementing + new tool

* Update version

* Clean up dartdoc

* Update workflow_test.dart

* Set private _showMessage in method scope

* Format fix

Author: eliasyishak

pkgs/unified_analytics/CHANGELOG.md

@@ -1,5 +1,6 @@
 ## 5.8.1
 
+- Refactor logic for `okToSend` and `shouldShowMessage`
 - Check devtools config file for legacy opt out status
 
 ## 5.8.0

pkgs/unified_analytics/lib/src/analytics.dart

@@ -333,26 +333,16 @@ class AnalyticsImpl implements Analytics {
   /// message has been updated by the package.
   late bool _showMessage;
 
-  /// This will be switch to true once it has been confirmed by the
-  /// client using this package that they have shown the
-  /// consent message to the developer.
-  ///
-  /// If the tool using this package as already shown the consent message
-  /// and it has been added to the config file, it will be set as true.
-  ///
-  /// It will also be set to true once the tool using this package has
-  /// invoked [clientShowedMessage].
-  ///
-  /// If this is false, all events will be blocked from being sent.
-  bool _clientShowedMessage = false;
-
   /// When set to `true`, various assert statements will be enabled
   /// to ensure usage of this class is within GA4 limitations.
   final bool _enableAsserts;
 
   /// Telemetry suppression flag that is set via [Analytics.suppressTelemetry].
   bool _telemetrySuppressed = false;
 
+  /// Indicates if this is the first run for a given tool.
+  bool _firstRun = false;
+
   /// The list of futures that will contain all of the send events
   /// from the [GAClient].
   final _futures = <Future<Response>>[];
@@ -388,7 +378,13 @@ class AnalyticsImpl implements Analytics {
       toolsMessageVersion: toolsMessageVersion,
     );
     initializer.run();
-    _showMessage = initializer.firstRun;
+    if (initializer.firstRun) {
+      _showMessage = true;
+      _firstRun = true;
+    } else {
+      _showMessage = false;
+      _firstRun = false;
+    }
 
     // Create the config handler that will parse the config file
     _configHandler = ConfigHandler(
@@ -397,13 +393,6 @@ class AnalyticsImpl implements Analytics {
       initializer: initializer,
     );
 
-    // If the tool has already been added to the config file
-    // we can assume that the client has successfully shown
-    // the consent message
-    if (_configHandler.parsedTools.containsKey(tool.label)) {
-      _clientShowedMessage = true;
-    }
-
     // Check if the tool has already been onboarded, and if it
     // has, check if the latest message version is greater to
     // prompt the client to show a message
@@ -414,6 +403,11 @@ class AnalyticsImpl implements Analytics {
         _configHandler.parsedTools[tool.label]?.versionNumber ?? -1;
     if (currentVersion < toolsMessageVersion) {
       _showMessage = true;
+
+      // If the message version has been updated, it will be considered
+      // as if it was a first run and any events attempting to get sent
+      // will be blocked
+      _firstRun = true;
     }
 
     _clientIdFile = fs.file(
@@ -464,22 +458,19 @@ class AnalyticsImpl implements Analytics {
   /// Checking the [telemetryEnabled] boolean reflects what the
   /// config file reflects.
   ///
-  /// Checking the [_showMessage] boolean indicates if this the first
-  /// time the tool is using analytics or if there has been an update
-  /// the messaging found in constants.dart - in both cases, analytics
-  /// will not be sent until the second time the tool is used.
-  ///
-  /// Additionally, if the client has not invoked
-  /// [Analytics.clientShowedMessage], then no events shall be sent.
+  /// Checking the [_showMessage] boolean indicates if the consent
+  /// message has been shown for the user, this boolean is set to `true`
+  /// when the tool using this package invokes the [clientShowedMessage]
+  /// method.
   ///
   /// If the user has suppressed telemetry [_telemetrySuppressed] will
   /// return `true` to prevent events from being sent for current invocation.
+  ///
+  /// Checking if it is the first time a tool is running with this package
+  /// as indicated by [_firstRun].
   @override
   bool get okToSend =>
-      telemetryEnabled &&
-      !_showMessage &&
-      _clientShowedMessage &&
-      !_telemetrySuppressed;
+      telemetryEnabled && !_showMessage && !_telemetrySuppressed && !_firstRun;
 
   @override
   Map<String, ToolInfo> get parsedTools => _configHandler.parsedTools;
@@ -496,22 +487,24 @@ class AnalyticsImpl implements Analytics {
 
   @override
   void clientShowedMessage() {
+    // Check the tool needs to be added to the config file
     if (!_configHandler.parsedTools.containsKey(tool.label)) {
       _configHandler.addTool(
         tool: tool.label,
         versionNumber: toolsMessageVersion,
       );
-      _showMessage = true;
     }
+
+    // When the tool already exists but the consent message version
+    // has been updated
     if (_configHandler.parsedTools[tool.label]!.versionNumber <
         toolsMessageVersion) {
       _configHandler.incrementToolVersion(
         tool: tool.label,
         newVersionNumber: toolsMessageVersion,
       );
-      _showMessage = true;
     }
-    _clientShowedMessage = true;
+    _showMessage = false;
   }
 
   @override

pkgs/unified_analytics/test/unified_analytics_test.dart

@@ -74,7 +74,9 @@ void main() {
       fs: fs,
       platform: platform,
     );
+    expect(initializationAnalytics.shouldShowMessage, true);
     initializationAnalytics.clientShowedMessage();
+    expect(initializationAnalytics.shouldShowMessage, false);
 
     // The main analytics instance, other instances can be spawned within tests
     // to test how to instances running together work
@@ -143,8 +145,6 @@ void main() {
     expect(dartToolDirectory.listSync().length, equals(5),
         reason:
             'There should only be 5 files in the $kDartToolDirectoryName directory');
-    expect(initializationAnalytics.shouldShowMessage, true,
-        reason: 'For the first run, the message should be shown');
     expect(configFile.readAsLinesSync().length,
         kConfigString.split('\n').length + 1,
         reason: 'The number of lines should equal lines in constant value + 1 '
@@ -430,7 +430,12 @@ void main() {
       fs: fs,
       platform: platform,
     );
+    expect(secondAnalytics.shouldShowMessage, true);
+    expect(secondAnalytics.okToSend, false);
     secondAnalytics.clientShowedMessage();
+    expect(secondAnalytics.shouldShowMessage, false);
+    expect(secondAnalytics.okToSend, false,
+        reason: 'New version for the message will be treated as a first run');
 
     expect(secondAnalytics.parsedTools[initialTool.label]?.versionNumber,
         toolsMessageVersion + 1,

pkgs/unified_analytics/test/workflow_test.dart

@@ -59,6 +59,99 @@ void main() {
         .childFile(kDismissedSurveyFileName);
   });
 
+  test('Confirm workflow for first run', () {
+    final firstAnalytics = Analytics.test(
+      tool: initialTool,
+      homeDirectory: home,
+      measurementId: measurementId,
+      apiSecret: apiSecret,
+      flutterChannel: flutterChannel,
+      toolsMessageVersion: toolsMessageVersion,
+      toolsMessage: toolsMessage,
+      flutterVersion: flutterVersion,
+      dartVersion: dartVersion,
+      fs: fs,
+      platform: platform,
+    );
+
+    expect(firstAnalytics.shouldShowMessage, true);
+    expect(firstAnalytics.okToSend, false);
+
+    firstAnalytics.clientShowedMessage();
+    expect(firstAnalytics.shouldShowMessage, false);
+    expect(firstAnalytics.okToSend, false,
+        reason: 'On the first run, we should not be ok '
+            'to send any events, even if the user accepts');
+  });
+
+  test('Confirm workflow for updated tools message version + new tool', () {
+    // Helper function to check the state of the instance
+    void checkAnalyticsInstance(Analytics instance) {
+      expect(instance.shouldShowMessage, true);
+      expect(instance.okToSend, false);
+
+      instance.clientShowedMessage();
+      expect(instance.shouldShowMessage, false);
+      expect(instance.okToSend, false,
+          reason: 'On the first run, we should not be ok '
+              'to send any events, even if the user accepts');
+    }
+
+    final firstAnalytics = Analytics.test(
+      tool: initialTool,
+      homeDirectory: home,
+      measurementId: measurementId,
+      apiSecret: apiSecret,
+      flutterChannel: flutterChannel,
+      toolsMessageVersion: toolsMessageVersion,
+      toolsMessage: toolsMessage,
+      flutterVersion: flutterVersion,
+      dartVersion: dartVersion,
+      fs: fs,
+      platform: platform,
+    );
+
+    checkAnalyticsInstance(firstAnalytics);
+
+    // Instance where we increment the version of the message
+    final secondAnalytics = Analytics.test(
+      tool: initialTool,
+      homeDirectory: home,
+      measurementId: measurementId,
+      apiSecret: apiSecret,
+      flutterChannel: flutterChannel,
+      toolsMessageVersion: toolsMessageVersion + 1, // Incrementing version
+      toolsMessage: toolsMessage,
+      flutterVersion: flutterVersion,
+      dartVersion: dartVersion,
+      fs: fs,
+      platform: platform,
+    );
+
+    // Running the same checks for the second instance, it should
+    // behave the same as if it was a first run
+    checkAnalyticsInstance(secondAnalytics);
+
+    // Instance for a different tool with the incremented version
+    final thirdAnalytics = Analytics.test(
+      tool: secondTool, // Different tool
+      homeDirectory: home,
+      measurementId: measurementId,
+      apiSecret: apiSecret,
+      flutterChannel: flutterChannel,
+      toolsMessageVersion: toolsMessageVersion + 1, // Incrementing version
+      toolsMessage: toolsMessage,
+      flutterVersion: flutterVersion,
+      dartVersion: dartVersion,
+      fs: fs,
+      platform: platform,
+    );
+
+    // The instance with a new tool getting onboarded should be
+    // treated the same as the 2 previous instances
+    checkAnalyticsInstance(thirdAnalytics);
+  });
+
   test('Confirm workflow for checking tools into the config file', () {
     final firstAnalytics = Analytics.test(
       tool: initialTool,