Honor legacy opt out status #80
Conversation
| // 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.
Unless I'm reading this package's code wrong, it looks like this will be platform.environment['UserProfile'] on windows (https://github.com/dart-lang/tools/blob/main/pkgs/unified_analytics/lib/src/utils.dart#L78), when the tool will be writing to platform.environment['AppData'] on Windows https://github.com/dart-lang/usage/blob/master/lib/src/usage_impl_io.dart#L89
There was a problem hiding this comment.
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 UserProfile?
There was a problem hiding this comment.
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:
- for the purposes of this PR, I think we need to check for legacy opt out from where package:usage is writing it, which is the AppData env var if present (not sure if it's ever not present, or what package:usage does in that case).
- for the purposes of fulfilling the PDD, I'm not sure what we should do.
There was a problem hiding this comment.
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.
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.
I think we would be okay if we changed the env variable for home on windows to point to APPDATA then? That way we can continue what was done in the past and address @christopherfujino 's item 1 above?
| 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); | ||
| } |
There was a problem hiding this comment.
This has been added to address issue #82 to change the consent message's command based on which tool is using it
|
I'm going to defer to your other reviewers. Looks like they've done a better job than I likely would anyway. |
| // | ||
| // 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.
I'm thinking we should delete the file if we couldn't parse it.
There was a problem hiding this comment.
I'm thinking we should delete the file if we couldn't parse it.
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.
Eventually, once legacy analytics is removed, we can add something to delete legacy analytics files if we see them.
Bumps [dart-lang/setup-dart](https://github.com/dart-lang/setup-dart) from 1.3 to 1.4. - [Release notes](https://github.com/dart-lang/setup-dart/releases) - [Changelog](https://github.com/dart-lang/setup-dart/blob/main/CHANGELOG.md) - [Commits](dart-lang/setup-dart@6a218f2...a57a6c0) --- updated-dependencies: - dependency-name: dart-lang/setup-dart dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Fixes #74
Also addresses the consent message edit: #82