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

Dismissing and persisting surveys #127

Conversation

eliasyishak
Copy link
Contributor

@eliasyishak eliasyishak commented Jul 19, 2023

This feature will add the ability to dismiss a survey and have its unique id be persisted locally on disk so that it doesn't get prompted again to the user. A dismissal occurs if the user chooses to open the survey or dismiss it without completing it

Address umbrella issue:

Example of usage

// This will grab any available surveys from the remote host and apply
// the sampling rate (if applicable for that survey) and check that the
// the surveys available have not been dismissed or snoozed
final surveyList = analytics.fetchAvailableSurveys();

if (surveyList.length > 0) {
  // Recommended to take the first item in the list since
  // there could be multiple surveys available
  final survey = surveyList.first;

  // pseudo-code to simulate "showing" the survey
  display(survey);

  // Should call the below method to snooze the survey
  analytics.surveyShown(survey);

  // How to handle a permanent dismissal of the survey
  // (ie. the user clicked "No, skip survey"
  if (status == 'dismissed') {
     analytics.dismissSurvey(
       survey: survey,
       acceptedAccepted: status == 'dismissed' ? false : true,
     )
  }
}

  • 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.

Removing NoOp session instance since that was only being used before `2.0.0`
Copy link
Member

@bwilkerson bwilkerson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with these changes, but the description makes it sound like there might be other changes that you intended to include. No problem if that's not the case; just wanted to let you know in case you'd forgotten to git add a file.

@eliasyishak
Copy link
Contributor Author

I'm fine with these changes, but the description makes it sound like there might be other changes that you intended to include. No problem if that's not the case; just wanted to let you know in case you'd forgotten to git add a file.

Ah thank you for for taking the initiative to review this already, but yes, there are going to be some more changes coming that I was going to run by you. Definitely will need another run through

@eliasyishak eliasyishak marked this pull request as ready for review July 24, 2023 21:29
@eliasyishak
Copy link
Contributor Author

@bwilkerson sorry for the noise you must have got from this PR, but I am ready for a review. The first review was before most of the functionality was added

@eliasyishak
Copy link
Contributor Author

@DanTup does this workflow outlined in the PR description seem feasible? No need to review the entire PR if you don't want to by the way, just wanted to confirm the analytics.fetchAvailableSurveys() and analytics.dismissSurvey(...) APIs

@DanTup
Copy link
Contributor

DanTup commented Jul 25, 2023

  // How to handle when the survey gets snoozed
  // (ie. the user did not take action on the survey)
  //
  // This will prevent the survey from showing up again for minutes
  // in Survey.dismissForMinutes
  if (status == 'snoozed') {

I think this action needs to occur as soon as a survey is shown (because we need to suppress duplicates starting immediately), so it should probably be more of a "surveyShown" call (and just implied rather than an explicit request by the client).

If it doesn't occur immediately as the notification is being shown, it may result in duplicates if the user opens another IDE window, for example.

Eg.:

  • Client asks package for surveys (and surveys in the snooze period are not returned)
  • Client picks the first one to show
  • Client shows the notification + tells the package it's showing it
  • Package records that this survey is snoozed for a period
  • If user clicks to complete or dismiss this survey, client tells package to permanently dismiss this survey

@eliasyishak
Copy link
Contributor Author

Ah that's a great point, how does this sound. Instead of having analytics.dismissSurvey(...) which can either snooze or permanently dismiss the survey... we can instead have analytics.dismissSurvey(...) which will only be responsible for permanently dismissing a survey and introduce a new method analytics.snoozeSurvey(...) which should always be called after displaying a survey.

That way, the first window to go and retrieve a survey will also snooze it so it doesn't show up again if the user opens a new window within 2-3 seconds (or whatever the delay is for a popup in VS Code)?

// Recommended to take the first item in the list since
// there could be multiple surveys available
final survey = surveyList.first;

// pseudo-code to simulate "showing" the survey
display(survey);

// Immediately call the method to snooze the survey
analytics.snoozeSurvey(survey);  // <-- new method to be added

// pseudo-code to simulate the user interacting with it
final status = 'dismissed';

// How to handle a permanent dismissal of the survey
// (ie. the user clicked "No, skip survey"
//
// Ideally the user clicking dismiss would call a function/method
// to handle the below
if (status == 'dismissed') {
   analytics.dismissSurvey(
     survey: survey,
     permanently: true,
   )
}

// And if the user didn't interact with the survey, it will automatically
// observe its snooze period before getting called again

@DanTup
Copy link
Contributor

DanTup commented Jul 25, 2023

and introduce a new method analytics.snoozeSurvey(...) which should always be called after displaying a survey.

This sounds good to me, although if you're collecting stats when surveys are shown within the package, perhaps calling it surveyShown() or similar would allow it to record that the survey was shown and snooze it in one step?

Perhaps the same is true for dismissSurvey - if it was two different methods (surveyAccepted and surveyDismissed), it could handle analytics for those cases?

(it's possible you're already handling these, I'm not sure whose responsibility collecting these stats would be)

@eliasyishak
Copy link
Contributor Author

All great ideas, I'll implement this and push a new update. Thanks @DanTup !

@eliasyishak
Copy link
Contributor Author

How does this look, the implementation of the two survey apis will send events for when they were snoozed/shown and when the user has interacted with them indicated by the status variable

final Survey survey = ...;

analytics.surveyShown(survey); // <-- this will automatically snooze the survey

.
.
.

// Some process returns the status indicating if the user accepted or rejected the survey
final bool status = true; // <-- this indicates they opened the survey

// The code below will fire when the status is returned
analytics.dismissSurvey(survey: survey, surveyAccepted: status);

@DanTup
Copy link
Contributor

DanTup commented Jul 25, 2023

Sounds good to me :-)

@eliasyishak eliasyishak merged commit 2be6f2d into dart-lang:survey-handler-feature Jul 26, 2023
2 checks passed
@eliasyishak eliasyishak deleted the persist-dismissed-surveys branch July 26, 2023 15:24
eliasyishak added a commit that referenced this pull request Aug 3, 2023
* Survey handler functionality to fetch available surveys (#91)

* 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

* No surveys returned for opted out users

* Revert "No surveys returned for opted out users"

This reverts commit f6d9f8e.

* No surveys for opted out users (#99)

* Check `okToSend` before fetching surveys

* Added test

* dart format fix

* Update CHANGELOG.md

* Mark as dev

* Change version suffix

* `dart fix --apply --code=combinators_ordering`

* Fix `survey_handler.dart` with new lints

* Add'l fixes to survey_handler

* Remove left hand types from `analytics.dart`

* Fix `survey_handler_test.dart` with new lints

* Fix tests with survey_handler class from lint fixes

* `dart format` fix

* Sampling rate functionality added (#122)

* 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

* Fix survey handler tests to use new .send method

* Fix tests to use new maps for `LogFileStats`

* Dismissing and persisting surveys (#127)

* 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

* Sample rate example added (#130)

* Added example file

* Including example's output in code

* Update sample_rate.dart

* Fix nits

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

* 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

* Refactor for buttons array with `SurveyButton` class (#134)

* 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

* Format fix

* Allow surveys with no conditions to be passed

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

* Update version

* Simplify utility functions for sample rate + check date

* `const` constructor for `Survey` unnamed constructor

* Fix test to unit test sampling function

* Fix dartdocs + check for null outside loop + breaks removed

* Add documentation to example files

* `dart format`

* Catch `TypeError` when parsing json survey

* Adding tests for the sampling rate with margin of error
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.

3 participants