Survey handler feature #109

eliasyishak · 2023-06-01T14:47:13Z

Reference issue tracker:

☂️ Create a survey handler class to enable package to share surveys #81

I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

See our contributor guide for general expectations for PRs.
Larger or significant changes should be discussed in an issue before creating a PR.
Contributions to our repos should follow the Dart style guide and use dart format.
Most changes should add an entry to the changelog and may need to rev the pubspec package version.
Changes to packages require corresponding tests.

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

* Add constant for endpoint that stores metadata json file * Development began on survey handler class with fetch * Update survey_handler.dart * Parsing functionality added in `survey_handler` * Condition class `operator` relabeled to `operatorString` * `Analytics` test and default constructors to use `SurveyHandler` * Refactor + cleanup + error handling * `dart format` fix * Evaluating functionality added to `Analytics` * Format fix * `!=` operator added to `Condition` class * Refactor for fake survey handler to use list of surveys or string * Initial test cases added * Tests added to use json in `FakeSurveyHandler` * Fix nit * Early exit if on null `logFileStats` * Test to check each field in `Survey` and `Condition` * Documentation update

This reverts commit f6d9f8e.

* Check `okToSend` before fetching surveys * Added test * dart format fix * Update CHANGELOG.md

* Sampling rate functionality added * Update tests to have 100% sampling rate * Tests added to test sampling rate * Update survey_handler_test.dart * Fix type for `jsonDecode` * New utility function to convert string into integer * Fix tests with new outputs for sample rate * Use uniqueId for survey instead of description * Add hyphen to lookup * Fix documentation

* Add constant for new file name + clean up session handler Removing NoOp session instance since that was only being used before `2.0.0` * Updating survey handler to create file to persist ids * Revert changes to session handler * Update constant to be a json file * Initialize dismiss surveys file with empty json * Initializer for dismissed file made static * Functionality added to check if survey snoozed or dismissed * Dismiss functionality added * `dismissForDays` -> `dismissForMinutes` * Update survey_handler_test.dart * Clean up external functions to be class methods * Tests added for snoozing and dismissing permanently * Test added for malformed json * Check sample rate before using LogFileStats * Add `surveyShown` API to snooze surveys * Use new URL for survey metadata * Error handling for missing json file

* Added example file * Including example's output in code * Update sample_rate.dart * Fix nits

* Added enum and event constructor survey actions * Fix format errors * Using two events for survey shown and survey action * Created mock class to confirm events are sent * Clean up constructors * Fix nits

* Added newe `SurveyButton` class * Fix tests * Add documentation for enums * Update sample_rate.dart * Update tests to check for `SurveyButton` classes * Remove enum for status of action * Use `snoozeForMinutes` instead of dismiss * Expose `SurveyButton` * Fixing documentation for event class * Order members in survey handler * Refactor to pass button to `surveyInteracted(..)` * `surveyButtonList` --> `buttonList` renaming * Adding example file for how to use survey handler feature * Adding conditional check for url to display

Only checking if `logFileStats` is null if there is a condition in the condition array in the json

eliasyishak · 2023-08-02T19:30:47Z

@christopherfujino the new example files should provide a general workflow for this new feature if you want to start there. And thank you! 😁

github-actions · 2023-08-02T19:57:41Z

Package publishing

Package	Version	Status	Publish tag (post-merge)
package:unified_analytics	3.1.0	ready to publish	`unified_analytics-v3.1.0`
package:extension_discovery	1.0.0	already published at pub.dev
package:cli_config	0.1.1	already published at pub.dev
package:graphs	2.3.2-wip	pre-release version (no publish necessary)

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

christopherfujino · 2023-08-02T19:37:19Z

pkgs/unified_analytics/example/sample_rate.dart

+  //
+  // Test with your own seed and alter other parameters
+  // as needed
+  final uuidGenerator = Uuid(123);


I actually think we should ONLY be providing hard-coded seeds for tests that we expect to be deterministic.

This is actually not for the tests, it's meant to be repeatable because I include the stdout when running the script to demonstrate that the sampling rate is working as expected

Example of the run below

// Output from run above with set parameters // // ''' // Test sample rate = 0.3 // Number of iterations = 10000 // --- // Count of iterations sampled (successes) = 3046 // Actual sample rate = 0.3046 // --- // Runtime = 8ms // '''

I have added documentation to the top of the example file that should make it clearer

I would assume example code would be copy pasted, and best practices.

christopherfujino · 2023-08-02T19:55:43Z

pkgs/unified_analytics/lib/src/analytics.dart

@@ -204,6 +214,15 @@ abstract class Analytics {
  /// that need to be sent off
  void close();

+  /// Method to fetch surveys from the specified endpoint [kContextualSurveyUrl]


Here and throughout, dartdocs should be complete sentences with periods at the end: https://dart.dev/effective-dart/documentation

I have this captured in this issue

[nit] Fix dartdocs to have sentences end in periods #126

that will be a large diff so didn't want to add noise here

Why are you adding more dartdocs that you'll need to update later though?

pkgs/unified_analytics/lib/src/analytics.dart

christopherfujino · 2023-08-02T20:00:32Z

pkgs/unified_analytics/lib/src/analytics.dart

+      // [Survey.conditionList]
+      var conditionsMet = 0;
+      for (final condition in survey.conditionList) {
+        if (logFileStats == null) continue;


can't we check for this outside this for loop, and if so skip the entire loop?

That's a good idea, i'll wrap the loop in the loop instead

pkgs/unified_analytics/lib/src/analytics.dart

christopherfujino · 2023-08-02T20:07:00Z

pkgs/unified_analytics/example/serving_surveys.dart

+  await withClock(Clock.fixed(DateTime(2023, 3, 3, 12, 0)), () async {
+    // Use a memory file system to repeatedly run this example
+    // file with the test instance
+    fs = MemoryFileSystem.test(style: FileSystemStyle.posix);


are these examples to demonstrate how to use the package, or are they test code? I'm guessing we wouldn't want someone to copy paste this to their production code.

They are meant to demonstrate how to use the package.. and that is correct we don't want anyone to copy and paste this into production code.

I will add more documentation at the top to indicate that this file is meant to be used for guidance

pkgs/unified_analytics/lib/src/survey_handler.dart

christopherfujino · 2023-08-02T20:22:57Z

pkgs/unified_analytics/lib/src/survey_handler.dart

+        // that fail to parse
+        try {
+          return Survey.fromJson(element as Map<String, dynamic>);
+          // ignore: avoid_catches_without_on_clauses


rather than ignoring this, can you catch a more specific exception you're expecting? I'm guessing TypeError?

When I try to do the following

try { element as Map<String, dynamic>; element.remove('startDate'); return Survey.fromJson(element); } on TypeError { return null; }

I get the following lint message

The type 'TypeError' should not be caught because it is a subclass of 'Error'. (avoid_catching_errors) (https://dart.dev/lints/avoid_catching_errors)

What's the best approach for catching type errors? I can add the // ignore: avoid_catching_errors line to prevent this

Yeah, I think it's better to ignore that lint than to catch everything.

pkgs/unified_analytics/lib/src/survey_handler.dart

pkgs/unified_analytics/lib/src/utils.dart

christopherfujino

LGTM provided you catch TypeError and // ignore: avoid_catching_errors

eliasyishak added 4 commits May 18, 2023 10:53

No surveys returned for opted out users

f6d9f8e

Revert "No surveys returned for opted out users"

7ea266a

This reverts commit f6d9f8e.

No surveys for opted out users (#99)

80a1e02

* Check `okToSend` before fetching surveys * Added test * dart format fix * Update CHANGELOG.md

github-actions bot added the package:unified_analytics label Jun 1, 2023

eliasyishak closed this Jun 1, 2023

eliasyishak reopened this Jun 1, 2023

eliasyishak added 3 commits June 1, 2023 11:01

Merge branch 'main' into survey-handler-feature

52d9504

Mark as dev

9acd1c0

Change version suffix

4a02da8

eliasyishak closed this Jun 1, 2023

Merge remote-tracking branch 'upstream/main' into survey-handler-feature

a92dc7e

eliasyishak reopened this Jun 1, 2023

eliasyishak added 17 commits June 1, 2023 13:46

dart fix --apply --code=combinators_ordering

d9a6cc1

Fix survey_handler.dart with new lints

8d8ecf7

Add'l fixes to survey_handler

7ae06ce

Remove left hand types from analytics.dart

2ae6a33

Fix survey_handler_test.dart with new lints

03570ec

Fix tests with survey_handler class from lint fixes

a74ab58

dart format fix

164668c

Merge remote-tracking branch 'upstream/main' into survey-handler-feature

ccdd218

Fix survey handler tests to use new .send method

7c1757f

Fix tests to use new maps for LogFileStats

6dea39e

Sample rate example added (#130)

1b75f97

* Added example file * Including example's output in code * Update sample_rate.dart * Fix nits

Send event for surveys shown and surveys dismissed (#133)

ae5cc41

* Added enum and event constructor survey actions * Fix format errors * Using two events for survey shown and survey action * Created mock class to confirm events are sent * Clean up constructors * Fix nits

Merge remote-tracking branch 'upstream/main' into survey-handler-feature

04ef3f1

Format fix

98c2c74

eliasyishak marked this pull request as ready for review August 2, 2023 14:10

eliasyishak mentioned this pull request Aug 2, 2023

☂️ Create a survey handler class to enable package to share surveys #81

Closed

9 tasks

eliasyishak changed the title ~~[wip] Survey handler feature~~ Survey handler feature Aug 2, 2023

Allow surveys with no conditions to be passed

39b995e

Only checking if `logFileStats` is null if there is a condition in the condition array in the json

eliasyishak requested a review from christopherfujino August 2, 2023 19:29

Update version

cf4a496

christopherfujino reviewed Aug 2, 2023

View reviewed changes

pkgs/unified_analytics/lib/src/survey_handler.dart Outdated Show resolved Hide resolved

christopherfujino reviewed Aug 2, 2023

View reviewed changes

pkgs/unified_analytics/lib/src/survey_handler.dart Outdated Show resolved Hide resolved

christopherfujino reviewed Aug 2, 2023

View reviewed changes

pkgs/unified_analytics/lib/src/utils.dart Outdated Show resolved Hide resolved

eliasyishak added 6 commits August 3, 2023 11:03

Simplify utility functions for sample rate + check date

0826f2f

const constructor for Survey unnamed constructor

76f3655

Fix test to unit test sampling function

0ecffcc

Fix dartdocs + check for null outside loop + breaks removed

0d615ea

Add documentation to example files

e975a69

dart format

73f63ce

christopherfujino approved these changes Aug 3, 2023

View reviewed changes

eliasyishak added 2 commits August 3, 2023 14:07

Catch TypeError when parsing json survey

9e8e22d

Adding tests for the sampling rate with margin of error

cca9f31

eliasyishak merged commit 921611a into main Aug 3, 2023

eliasyishak deleted the survey-handler-feature branch August 3, 2023 19:03

eliasyishak mentioned this pull request Aug 7, 2023

Clean up on dart docs across all dart files under lib #142

Merged

1 task

cbracken mentioned this pull request May 30, 2016

Use service library from Observatory #541

Closed

Survey handler feature #109

Survey handler feature #109

Uh oh!

Conversation

eliasyishak commented Jun 1, 2023

Uh oh!

eliasyishak commented Aug 2, 2023

Uh oh!

github-actions bot commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Package publishing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Aug 2, 2023 •

edited

Loading