-
Notifications
You must be signed in to change notification settings - Fork 58
Honor legacy opt out status #80
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
Changes from all commits
b9eb98a
80152c8
fe81441
22d1753
7a5c7d5
68fa790
1caefe7
b3b5a86
4f3a3cb
16870f6
2c618f8
18eeb66
3dbc207
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -376,8 +376,15 @@ class AnalyticsImpl implements Analytics { | |
} | ||
|
||
@override | ||
String get getConsentMessage => | ||
kToolsMessage.replaceAll('[tool name]', tool.description); | ||
String get getConsentMessage { | ||
// The command to swap in the consent message | ||
final String commandString = | ||
tool == DashTool.flutterTool ? 'flutter' : 'dart'; | ||
|
||
return kToolsMessage | ||
.replaceAll('[tool name]', tool.description) | ||
.replaceAll('[dart|flutter]', commandString); | ||
} | ||
Comment on lines
+379
to
+387
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has been added to address issue #82 to change the consent message's command based on which tool is using it |
||
|
||
/// Checking the [telemetryEnabled] boolean reflects what the | ||
/// config file reflects | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,7 +60,16 @@ class Initializer { | |
required int toolsMessageVersion, | ||
}) { | ||
configFile.createSync(recursive: true); | ||
configFile.writeAsStringSync(kConfigString); | ||
|
||
// If the user was previously opted out, then we will | ||
// replace the line that assumes automatic opt in with | ||
// an opt out from the start | ||
if (legacyOptOut(fs: fs, home: homeDirectory)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless I'm reading this package's code wrong, it looks like this will be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh that's a very good catch. In the PDD, it says all files will be written to the user's home directory, am I correct to assume the home directory on windows is stored in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my understanding is that "home directory" is an imprecise term, and especially on Windows there is not one "correct" answer for where a user's home directory is across different Windows versions and also application usages. In other words:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The home directory as the value of the UserProfile environment variable seems correct to me and fulfills the PDD. The most common definitions of "home directory" under Windows align with this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably should adjust the PDD to allow writing the files in a more Windows-friendly location like AppData. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we would be okay if we changed the env variable for home on windows to point to |
||
configFile.writeAsStringSync( | ||
kConfigString.replaceAll('reporting=1', 'reporting=0')); | ||
} else { | ||
configFile.writeAsStringSync(kConfigString); | ||
} | ||
} | ||
|
||
/// Creates that log file that will store the record formatted | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,10 +2,12 @@ | |
// for details. All rights reserved. Use of this source code is governed by a | ||
// BSD-style license that can be found in the LICENSE file. | ||
|
||
import 'dart:convert'; | ||
import 'dart:io' as io; | ||
import 'dart:math' show Random; | ||
|
||
import 'package:file/file.dart'; | ||
import 'package:path/path.dart' as p; | ||
|
||
import 'enums.dart'; | ||
import 'user_property.dart'; | ||
|
@@ -75,12 +77,88 @@ Directory getHomeDirectory(FileSystem fs) { | |
} else if (io.Platform.isLinux) { | ||
home = envVars['HOME']; | ||
} else if (io.Platform.isWindows) { | ||
home = envVars['UserProfile']; | ||
home = envVars['AppData']; | ||
} | ||
|
||
return fs.directory(home!); | ||
} | ||
|
||
/// Returns `true` if user has opted out of legacy analytics in Dart or Flutter | ||
/// | ||
/// Checks legacy opt-out status for the Flutter | ||
/// and Dart in the following locations | ||
/// | ||
/// Dart: `$HOME/.dart/dartdev.json` | ||
/// | ||
/// Flutter: `$HOME/.flutter` | ||
bool legacyOptOut({ | ||
required FileSystem fs, | ||
required Directory home, | ||
}) { | ||
final File dartLegacyConfigFile = | ||
fs.file(p.join(home.path, '.dart', 'dartdev.json')); | ||
final File flutterLegacyConfigFile = fs.file(p.join(home.path, '.flutter')); | ||
|
||
// Example of what the file looks like for dart | ||
// | ||
// { | ||
// "firstRun": false, | ||
// "enabled": false, <-- THIS USER HAS OPTED OUT | ||
// "disclosureShown": true, | ||
// "clientId": "52710e60-7c70-4335-b3a4-9d922630f12a" | ||
// } | ||
if (dartLegacyConfigFile.existsSync()) { | ||
try { | ||
// Read in the json object into a Map and check for | ||
// the enabled key being set to false; this means the user | ||
// has opted out of analytics for dart | ||
final Map<String, Object?> dartObj = | ||
jsonDecode(dartLegacyConfigFile.readAsStringSync()); | ||
if (dartObj.containsKey('enabled') && dartObj['enabled'] == false) { | ||
return true; | ||
} | ||
} on FormatException { | ||
eliasyishak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// In the case of an error when parsing the json file, return true | ||
// which will result in the user being opted out of unified_analytics | ||
// | ||
// A corrupted file could mean they opted out previously but for some | ||
// reason, the file was written incorrectly | ||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking we should delete the file if we couldn't parse it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I am against such drastic action; if there is a bug in how we are parsing the file we could interfere with the legacy analytics consent system. Better to just leave it behind, as this is after all an optional feature. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Eventually, once legacy analytics is removed, we can add something to delete legacy analytics files if we see them. |
||
} on FileSystemException { | ||
return true; | ||
} | ||
} | ||
|
||
// Example of what the file looks like for flutter | ||
// | ||
// { | ||
// "firstRun": false, | ||
// "clientId": "4c3a3d1e-e545-47e7-b4f8-10129f6ab169", | ||
// "enabled": false <-- THIS USER HAS OPTED OUT | ||
// } | ||
if (flutterLegacyConfigFile.existsSync()) { | ||
try { | ||
// Same process as above for dart | ||
final Map<String, Object?> flutterObj = | ||
jsonDecode(dartLegacyConfigFile.readAsStringSync()); | ||
if (flutterObj.containsKey('enabled') && flutterObj['enabled'] == false) { | ||
return true; | ||
} | ||
} on FormatException { | ||
// In the case of an error when parsing the json file, return true | ||
// which will result in the user being opted out of unified_analytics | ||
// | ||
// A corrupted file could mean they opted out previously but for some | ||
// reason, the file was written incorrectly | ||
return true; | ||
} on FileSystemException { | ||
return true; | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
|
||
/// A UUID generator. | ||
/// | ||
/// This will generate unique IDs in the format: | ||
|
Uh oh!
There was an error while loading. Please reload this page.