Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix logic for first run to fix shouldShowMessage behavior #228

Merged
Next Next commit
Adding new _firstRun
eliasyishak committed Jan 25, 2024
commit a650cc658d61aadf516ddeb9d6ef4cda3ff2f293
23 changes: 19 additions & 4 deletions pkgs/unified_analytics/lib/src/analytics.dart
Original file line number Diff line number Diff line change
@@ -353,6 +353,9 @@ class AnalyticsImpl implements Analytics {
/// 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 +391,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(
@@ -414,6 +423,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(
@@ -479,7 +493,8 @@ class AnalyticsImpl implements Analytics {
telemetryEnabled &&
!_showMessage &&
_clientShowedMessage &&
!_telemetrySuppressed;
!_telemetrySuppressed &&
!_firstRun;

@override
Map<String, ToolInfo> get parsedTools => _configHandler.parsedTools;
@@ -501,15 +516,15 @@ class AnalyticsImpl implements Analytics {
tool: tool.label,
versionNumber: toolsMessageVersion,
);
_showMessage = true;
_showMessage = false;
}
if (_configHandler.parsedTools[tool.label]!.versionNumber <
toolsMessageVersion) {
_configHandler.incrementToolVersion(
tool: tool.label,
newVersionNumber: toolsMessageVersion,
);
_showMessage = true;
_showMessage = false;
eliasyishak marked this conversation as resolved.
Show resolved Hide resolved
}
_clientShowedMessage = true;
}
9 changes: 7 additions & 2 deletions pkgs/unified_analytics/test/unified_analytics_test.dart
Original file line number Diff line number Diff line change
@@ -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,
25 changes: 25 additions & 0 deletions pkgs/unified_analytics/test/workflow_test.dart
Original file line number Diff line number Diff line change
@@ -59,6 +59,31 @@ void main() {
.childFile(kDismissedSurveyFileName);
});

test('Confirm workflow for first run', () {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are the best example for this change

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');
Comment on lines +83 to +84
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... does this mean if the user accepts analytics collection, and then does a bunch of interactions with our tools that we will not send any events for this session? It seems like we should start sending events as soon as the user accepts, even if it is the first run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was actually a requirement from the PDD, even for users that have opted in, analytic events will only be collected on the second time it is run. It's also listed in the consent message that was approved

The {{ toolDescription }} uses Google Analytics to report usage and diagnostic
data along with package dependencies, and crash reporting to send basic crash
reports. This data is used to help improve the Dart platform, Flutter framework,
and related tools.

Telemetry is not sent on the very first run. To disable reporting of telemetry,
run this terminal command:

    {{ toolName }} --disable-analytics

If you opt out of telemetry, an opt-out event will be sent, and then no further
information will be sent. This data is collected in accordance with the Google
Privacy Policy (https://policies.google.com/privacy).

});

test('Confirm workflow for checking tools into the config file', () {
final firstAnalytics = Analytics.test(
tool: initialTool,