From 6b126514191f4b47fb89bb70858b3e332bb827a0 Mon Sep 17 00:00:00 2001 From: Andrew Kolos Date: Thu, 12 Dec 2024 11:59:50 -0800 Subject: [PATCH] Fix analytics enabled/disabled event not being sent when the user enables/disables analytics (#160060) Fixes https://github.com/flutter/flutter/issues/160058. In addition, only send an event when the enabled-state of analytics actually gets flipped.
Pre-launch checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
[Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --- packages/flutter_tools/lib/runner.dart | 51 +++++-------- .../general.shard/runner/runner_test.dart | 72 +++++++++++++++++-- 2 files changed, 83 insertions(+), 40 deletions(-) diff --git a/packages/flutter_tools/lib/runner.dart b/packages/flutter_tools/lib/runner.dart index f1a46f9a9a..f9a4362387 100644 --- a/packages/flutter_tools/lib/runner.dart +++ b/packages/flutter_tools/lib/runner.dart @@ -73,47 +73,28 @@ Future run( exitCode: 1); } - // Disable analytics if user passes in the `--disable-analytics` option - // "flutter --disable-analytics" - // - // Same functionality as "flutter config --no-analytics" for disabling - // except with the `value` hard coded as false if (args.contains('--disable-analytics')) { - // The tool sends the analytics event *before* toggling the flag - // intentionally to be sure that opt-out events are sent correctly. - AnalyticsConfigEvent(enabled: false).send(); - - // Normally, the tool waits for the analytics to all send before the - // tool exits, but only when analytics are enabled. When reporting that - // analytics have been disable, the wait must be done here instead. - await globals.flutterUsage.ensureAnalyticsSent(); - - globals.flutterUsage.enabled = false; - globals.printStatus('Analytics reporting disabled.'); - - // TODO(eliasyishak): Set the telemetry for the unified_analytics - // package as well, the above will be removed once we have - // fully transitioned to using the new package, https://github.com/flutter/flutter/issues/128251 + if (globals.analytics.telemetryEnabled) { + globals.analytics.send( + Event.analyticsCollectionEnabled(status: false), + ); + // Before disablig analytics, we need to close the client to make + // sure the above collection event is sent. + await globals.analytics.close(); + } await globals.analytics.setTelemetry(false); + globals.printStatus('Analytics reporting disabled.'); } - // Enable analytics if user passes in the `--enable-analytics` option - // `flutter --enable-analytics` - // - // Same functionality as `flutter config --analytics` for enabling - // except with the `value` hard coded as true if (args.contains('--enable-analytics')) { - // The tool sends the analytics event *before* toggling the flag - // intentionally to be sure that opt-out events are sent correctly. - AnalyticsConfigEvent(enabled: true).send(); - - globals.flutterUsage.enabled = true; - globals.printStatus('Analytics reporting enabled.'); - - // TODO(eliasyishak): Set the telemetry for the unified_analytics - // package as well, the above will be removed once we have - // fully transitioned to using the new package, https://github.com/flutter/flutter/issues/128251 + final bool alreadyEnabled = globals.analytics.telemetryEnabled; await globals.analytics.setTelemetry(true); + if (!alreadyEnabled) { + globals.analytics.send( + Event.analyticsCollectionEnabled(status: true), + ); + } + globals.printStatus('Analytics reporting enabled.'); } // Send an event to GA3 for any users that are opted into GA3 diff --git a/packages/flutter_tools/test/general.shard/runner/runner_test.dart b/packages/flutter_tools/test/general.shard/runner/runner_test.dart index d67899a7a0..619b1547f2 100644 --- a/packages/flutter_tools/test/general.shard/runner/runner_test.dart +++ b/packages/flutter_tools/test/general.shard/runner/runner_test.dart @@ -541,7 +541,7 @@ void main() { ); testUsingContext( - 'runner enabling analytics with flag', + '--enable-analytics and --disable-analytics enables/disables telemetry', () async { io.setExitFunctionForTests((int exitCode) {}); @@ -550,8 +550,6 @@ void main() { await runner.run( ['--disable-analytics'], () => [], - // This flutterVersion disables crash reporting. - flutterVersion: '[user-branch]/', shutdownHooks: ShutdownHooks(), ); @@ -560,8 +558,6 @@ void main() { await runner.run( ['--enable-analytics'], () => [], - // This flutterVersion disables crash reporting. - flutterVersion: '[user-branch]/', shutdownHooks: ShutdownHooks(), ); @@ -574,6 +570,72 @@ void main() { }, ); + testUsingContext( + '--enable-analytics and --disable-analytics send an event when telemetry is enabled/disabled', + () async { + io.setExitFunctionForTests((int exitCode) {}); + await globals.analytics.setTelemetry(true); + + await runner.run( + ['--disable-analytics'], + () => [], + shutdownHooks: ShutdownHooks(), + ); + + expect( + (globals.analytics as FakeAnalytics).sentEvents, + [Event.analyticsCollectionEnabled(status: false)], + ); + + (globals.analytics as FakeAnalytics).sentEvents.clear(); + expect(globals.analytics.telemetryEnabled, false); + await runner.run( + ['--enable-analytics'], + () => [], + shutdownHooks: ShutdownHooks(), + ); + + expect((globals.analytics as FakeAnalytics).sentEvents, [ + Event.analyticsCollectionEnabled(status: true), + ]); + }, + overrides: { + Analytics: () => fakeAnalytics, + FileSystem: () => MemoryFileSystem.test(), + ProcessManager: () => FakeProcessManager.any(), + }, + ); + + testUsingContext( + '--enable-analytics and --disable-analytics do not send an event when telemetry is already enabled/disabled', + () async { + io.setExitFunctionForTests((int exitCode) {}); + + await globals.analytics.setTelemetry(false); + await runner.run( + ['--disable-analytics'], + () => [], + shutdownHooks: ShutdownHooks(), + ); + + expect((globals.analytics as FakeAnalytics).sentEvents, isEmpty); + + await globals.analytics.setTelemetry(true); + await runner.run( + ['--enable-analytics'], + () => [], + shutdownHooks: ShutdownHooks(), + ); + + expect((globals.analytics as FakeAnalytics).sentEvents, isEmpty); + }, + overrides: { + Analytics: () => fakeAnalytics, + FileSystem: () => MemoryFileSystem.test(), + ProcessManager: () => FakeProcessManager.any(), + }, + ); + testUsingContext( 'throw error when both flags passed', () async {