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

Conversation

eliasyishak
Copy link
Contributor

@eliasyishak eliasyishak commented Jan 25, 2024

This cleans up the logic so that we can separate the logic for okToSend and shouldShowMessage

This came up while migrating devtools to package:unified_analytics

Before this PR

final Analytics analytics = Analytics(...);

print(analytics.shouldShowMessage);  // <-- this would return true on first runs

// Invoke the command to let the package know the client has seen a message
analytics.clientShowedMessage();

print(analytics.shouldShowMessage);  // <-- this would still return true; bad

The above shows that invoking analytics.shouldShowMessage after confirming we have shown the message would not change what gets returned.

This was never caught when working with the flutter cli because when we run the check for shouldShowMessage, it only happened once. With devtools, because it is an app that would check shouldShowMessage every time a new window is created in Chrome, it would continue to show the consent message even if the user has already accepted in the first window in Chrome.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

Copy link

github-actions bot commented Jan 25, 2024

Package publishing

Package Version Status Publish tag (post-merge)
package:unified_analytics 5.8.1 ready to publish unified_analytics-v5.8.1
package:cli_config 0.1.2 already published at pub.dev
package:extension_discovery 2.0.1-wip WIP (no publish necessary)
package:graphs 2.3.2-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@christopherfujino christopherfujino requested review from kenzieschmoll and removed request for christopherfujino January 25, 2024 22:21
@christopherfujino
Copy link
Member

I think @kenzieschmoll should probably review this and decide if this is the right semantics.

@eliasyishak
Copy link
Contributor Author

I think @kenzieschmoll should probably review this and decide if this is the right semantics.

sounds good, @kenzieschmoll let me know if you'd like to review this together

@@ -59,6 +59,99 @@ 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

Comment on lines +83 to +84
reason: 'On the first run, we should not be ok '
'to send any events, even if the user accepts');
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).

@eliasyishak eliasyishak merged commit f6e67f2 into dart-lang:main Jan 29, 2024
6 checks passed
@eliasyishak eliasyishak deleted the fix-bug-for-client-showed-message branch January 29, 2024 19:17
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jan 30, 2024
… mime, mockito, test, tools, vector_math

Revisions updated by `dart tools/rev_sdk_deps.dart`.

args (https://github.com/dart-lang/args/compare/46d5033..03386ba):
  03386ba  2024-01-23  Albert Kaiser  Add missing curly braces in README.md (dart-lang/args#261)

csslib (https://github.com/dart-lang/csslib/compare/1ad2d1e..ec86ee5):
  ec86ee5  2024-01-09  Kevin Moore  Require Dart 3.0, update and fix lints (dart-lang/csslib#194)

ecosystem (https://github.com/dart-lang/ecosystem/compare/1e2785d..9ee08a4):
  9ee08a4  2024-01-29  Moritz  Add `ignore` flag to health workflows (dart-lang/ecosystem#218)
  a283d70  2024-01-17  Moritz  Make health testable (dart-lang/ecosystem#224)
  f61a550  2024-01-16  Moritz  Enable experiments for health (dart-lang/ecosystem#226)
  c81f25c  2024-01-16  Moritz  Add submodule support to `publish.yaml` (dart-lang/ecosystem#225)
  b51c356  2024-01-12  Moritz  Add submodules support to `health.yaml` (dart-lang/ecosystem#223)
  d7aaecb  2024-01-10  Moritz  Run `health.yaml` for bots (dart-lang/ecosystem#222)
  971c733  2024-01-10  Moritz  Don't write the failure string when skipping (dart-lang/ecosystem#220)

html (https://github.com/dart-lang/html/compare/06bc148..910f6d7):
  910f6d7  2024-01-26  Kevin Moore  Update lints, require Dart 3.2 (dart-lang/html#236)
  aaf7d1a  2024-01-25  Kevin Moore  blast_repo fixes (dart-lang/html#235)

http_multi_server (https://github.com/dart-lang/http_multi_server/compare/ae48489..491f7c6):
  491f7c6  2024-01-24  Kevin Moore  Update lints, require Dart 3.2 (dart-lang/http_multi_server#63)
  0df95e0  2024-01-24  Kevin Moore  blast_repo fixes (dart-lang/http_multi_server#62)

logging (https://github.com/dart-lang/logging/compare/4d35a4e..e04942d):
  e04942d  2024-01-18  Kevin Moore  update min SDK and deps (dart-lang/logging#155)
  a03a946  2024-01-18  Craig Labenz  Hierarchical logging documentation (dart-lang/logging#146)
  439ec80  2024-01-18  Kevin Moore  blast_repo fixes (dart-lang/logging#154)

mime (https://github.com/dart-lang/mime/compare/ca9f059..99fbdcc):
  99fbdcc  2024-01-24  Kevin Moore  Update to latest lints, require Dart 3.2 (dart-lang/mime#114)

mockito (https://github.com/dart-lang/mockito/compare/e15e000..0422551):
  0422551  2024-01-10  Oleh Prypin  Ignore "must_be_immutable" warning in generated files.

test (https://github.com/dart-lang/test/compare/846d73e..6700049):
  6700049d  2024-01-29  Nate Bosch  Prepare to publish package:checks (dart-lang/test#2178)
  a5c4f010  2024-01-24  Nate Bosch  Use a raw string for console logging with path (dart-lang/test#2177)
  fe3102ee  2024-01-10  dependabot[bot]  Bump js from 0.6.7 to 0.7.0 in /pkgs/test (dart-lang/test#2168)
  c709cde0  2024-01-10  Jacob MacDonald  fix a bug where test html files were not created in precompiled mode (dart-lang/test#2170)
  0eddae47  2024-01-09  Nate Bosch  Document the silent reporter (dart-lang/test#2163)

tools (https://github.com/dart-lang/tools/compare/8ffc077..f6e67f2):
  f6e67f2  2024-01-29  Elias Yishak  Fix logic for first run to fix `shouldShowMessage` behavior (dart-lang/tools#228)
  b97bd5c  2024-01-29  Elias Yishak  Checking for devtools config file for opt out (dart-lang/tools#227)

vector_math (https://github.com/google/vector_math.dart/compare/38a00c3..cb976c7):
  cb976c7  2024-01-28  Andrew Brampton  Update README.md to show how to use vector_math_64. (google/vector_math.dart#312)
  d99c903  2024-01-25  Andrew Brampton  Added a toString, operator == and hashCode to the Quad class. (google/vector_math.dart#311)

Change-Id: Ie42ec078b7b4d408d5167e38f05f1f37b754afb0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/349301
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants