Refactor for buttons array with SurveyButton class

[eliasyishak]

Reference issue:

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

With the new update to the json to include a button array, this PR includes a new class that will be generated for each button that is listed with a given survey

Example of the json update

[
    {
        "uniqueId": "eca0100a-505b-4539-96d0-57235f816cef",
        "startDate": "2023-07-01T09:00:00-07:00",
        "endDate": "2023-08-31T09:00:00-07:00",
        "description": "Help improve Flutter's release builds with this 3-question survey!",
        "snoozeForMinutes": "7200",
        "samplingRate": "0.1",
        "conditions": [
            {
                "field": "logFileStats.recordCount",
                "operator": ">=",
                "value": 1000
            }
        ],
        // Newly added key in json that contains button info
        "buttons": [
            {
                "buttonText": "Take Survey",
                "action": "accept",
                "url": "https://google.qualtrics.com/jfe/form/SV_5gsB2EuG5Et5Yy2",
                "promptRemainsVisible": false
            },
            {
                "buttonText": "Dismiss",
                "action": "dismiss",
                "url": null,
                "promptRemainsVisible": false
            },
            {
                "buttonText": "More Info",
                "action": "snooze",
                "url": "https://docs.flutter.dev/reference/crash-reporting",
                "promptRemainsVisible": true
            }
        ]
    }
]

Example for how clients using this package can fetch and work with the SurveyButton class

void main () async {
  final Analytics analytics = Analytics(...);
  
  // The call below will fetch from the remotely hosted survey
  // metadata json file
  //
  // It will automatically apply the sampling rate provided in the json
  // as well as check if the survey has been dismissed or snoozed
  //
  // If the survey has already been snoozed, it will check for the elapsed time
  // being greater than the `dismissForMinutes` field provided for each survey
  final surveyList = await analytics.fetchAvailableSurveys();

  // For this example, we can assume that only one survey is returned
  assert(surveyList.length == 1);

  final Survey survey = surveyList.first;
  
  // Each client will implement this differently but for this example,
  // the below function simulates showing a survey
  displaySurvey(survey);
  
  // Immediately after showing the survey, each client should invoke
  // the below method to log that it was shown to the user and automatically
  // snooze it
  analytics.surveyShown(survey);
  
  // For clients of this package that can be interactive (ie. vs code with pop ups)
  // the `SurveyButton` class can be used, which is stored in a list
  //
  // Assume there is only one button for this simple example
  final SurveyButton surveyButton = survey.surveyButtonList.first;
  
  // Extract data from the class by referencing its fields
  final String buttonText = surveyButton.buttonText;
  final String? url = surveyButton.url; // This is nullable, some buttons may not redirect
  final ButtonAction action = surveyButton.action; // ButtonAction is an enum
  
  // To permanently dismiss a survey, another method will be used
  //
  // This value will need to be assigned through the client-user interactions, but
  // simulating here that the "user accepted the survey"
  final bool accepted = true;
  
  // This will ensure that the survey doesn't show up again
  analytics.dismissSurvey(
    survey: survey,
    surveyAccepted: accepted
  );
}

---

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

[eliasyishak]

@DanTup do you mind taking a look if this looks like something you can work with easily on the vs code side?

The change we made to the json file will allow us to have a configurable number of buttons that each have their own redirect url (if applicable).

Using the dismissSurvey(...) method will get rid of it permanently and surveyShown(...) will ensure that the same pop up won't keep coming up as you mentioned in this comment

[DanTup]

Would this be better called "snoozeForMinutes", since "dismiss" seems to be used elsewhere to mean a permanent dismissal, and "snooze" is used to meant temporary?

[eliasyishak]

Ah good call, would help reduce any confusion

[DanTup]

This looks fine to me - the notifications in VS Code can have multiple buttons and we can do different things on each. In this case the analysis server would check which button was clicked, open the url (if provided) from that button and record the action.

A possible tweak to the API:

       {
            "buttonText": "Take Survey",
            "action": "accept",
            "url": "https://google.qualtrics.com/jfe/form/SV_5gsB2EuG324y2"
        }

   final bool accepted = true;
   
  // This will ensure that the survey doesn't show up again
  analytics.dismissSurvey(
    survey: survey,
    surveyAccepted: accepted
  );

It seems like the specific "action" here doesn't really matter to the client. If the client just passed the raw action instead of interpreting it (as snooze/accept/dismiss), it would be possibly to add additional actions in future without any changes to the "client".

For example one of your examples has a "More Info" button which snoozes, but maybe you don't want to snooze there. You could add a new kind of action and the client doesn't need to interpret it, you just do that in the analytics.dismissSurvey (or whatever it would be called)?

Oh, one thing to note though - is that right now any "More Info" button would dismiss the notification, so the user would get more info, but not actually be able to get back to accept the survey. That's probably not an issue if the More Info page also has a link to the survey, but if you want it to be re-shown on the client (we can't make it persist on the screen in VS Code, but we could immediately re-show it), then we'd need some way to signal that.

[eliasyishak]

It seems like the specific "action" here doesn't really matter to the client. If the client just passed the raw action instead of interpreting it (as snooze/accept/dismiss), it would be possibly to add additional actions in future without any changes to the "client".

So the action field in the SurveyButton class is really a signal for the analytics package indicating if they accepted or dismissed based on a button selected. So in the below snippet

analytics.dismissSurvey(
    survey: survey,
    surveyAccepted: accepted
  );

The accepted field will either be true or false and that will depend on how you assign each button. So if the first button has the value accept, that means clicking that button will set the surveyAccepted parameter in dismissSurvey(...) as true.

The same is done for dismiss except the opposite, we will instead pass false for surveyAccepted.

Oh, one thing to note though - is that right now any "More Info" button would dismiss the notification, so the user would get more info, but not actually be able to get back to accept the survey. That's probably not an issue if the More Info page also has a link to the survey, but if you want it to be re-shown on the client (we can't make it persist on the screen in VS Code, but we could immediately re-show it), then we'd need some way to signal that.

And great catch on this one too, I spoke to @jayoung-lee about this and she seemed to be okay with the pop up coming back up after observing its snooze period. But ideally, the more info redirect should have a link to the survey as well (maybe as a banner or something) so that users are not confused about where to go to fill the survey

[jayoung-lee]

Thanks for looping me in!

Yes, since the popup would reappear later, I think it would be acceptable to snooze it. (In a perfect scenario, we would prefer to keep the popup open until the developers check the "More Info" and return. However, IIUC, the popups currently disappear after a set duration of time...)

I find the idea of adding the surveys link to the "More Info" web page appealing. I'll discuss this with the DevRel team to explore the possibility of automatically updating the page with the JSON file as well!

[eliasyishak]

I find the idea of adding the surveys link to the "More Info" web page appealing. I'll discuss this with the DevRel team to explore the possibility of automatically updating the page with the JSON file as well!

That sounds like a good idea to me. One thing to keep in mind is that we probably want to use some kind of query parameter in the "More Info" url that indicates it's coming from a survey served by package:unified_analytics... otherwise, everyone would have access to the survey (which may be fine?) even though the survey is getting served to a subset of users (indicated by our sample rate)

[eliasyishak]

@DanTup additionally, I agree your approach of just passing the string for the action field. Making the updates now.

The snippet below is the implementation for the dismiss method and it seemed overkill to have the enum

  @override
  void dismissSurvey({required Survey survey, required bool surveyAccepted}) {
    _surveyHandler.dismiss(survey, true);
    final status = surveyAccepted ? 'accepted' : 'dismissed';
    send(Event.surveyAction(surveyId: survey.uniqueId, status: status));
  }

[jayoung-lee]

I believe there's value in transparently listing all the surveys currently running on the website.

But you made a valid point. I'll add a string at the end of the survey URL on the website (like ?Source=website), so we can filter them out later, if needed. We'll have a screening question in the survey in most cases, too.

(Nevertheless, the sampling remains crucial in preventing excessive annoyance!)

[DanTup]

@DanTup additionally, I agree your approach of just passing the string for the action field. Making the updates now.

I don't think I conveyed what I meant very well. My suggestion was to avoid the client needing to do any interpretation of the action field (to know whether it's accept or dismiss), so that in future if you wanted additional actions the clients don't need to be updated (to know whether they were accept/dismiss).

For example:

  final survey = (await analytics.fetchAvailableSurveys()).first;
  // Client shows the survey and gets back the button the user clicked
  final selectedButton = await displaySurvey(survey.title, survey.buttons);

  // If the button has a URL, open it
  if (selectedButton?.url != null) {
    openUrl(selectedButton?.url);
  }

  // Tell the package which button was clicked
  // Package handles reading "action" and recording appropriately
  analytics.surveyInteracted(survey, selectedButton);

This means in future if you wanted to add a new type of button with a new action, the logic for it (whether it snoozes, dismisses, etc.) could be inside the package. I don't know if it's likely more actions will be added, but it seemed a little cleaner to avoid the client having to interpret the string action just to know whether to pass a true or a false back to the package (using a boolean also means there are only every two options, but passing back the whole button means the package can have as many paths as it wants).

In a perfect scenario, we would prefer to keep the popup open until the developers check the "More Info" and return. However, IIUC, the popups currently disappear after a set duration of time...

Yep, we can't necessarily keep the popup on screen for a long time - however if the user clicks "More Info" we could at least re-show the notification so a) it's visible for the next 10-15 sec, and b) it's still in the notification area if they choose to look in there. If we don't re-show it, then after the user clicks More Info they would have to wait another x hours/days to see the prompt again (and we also wouldn't know if they followed the link from the More Info page, so we might show them the prompt again even though they went through and completed it).

So for ex., we could add a field to the buttons to control whether the prompt should "remain visible" after clicking (which for VS code would actually be an explicit re-show):

       {
            "buttonText": "More Info",
            "action": "moreInfo",
            "url": "http://example.org/",
            "promptRemainsVisible": true
        }

(we could also use action == "moreInfo" to infer this, but along the lines above, it might be better to keep control of the behaviour in the package and avoid the clients needing to handle the specific logic of which actions mean what)

[eliasyishak]

This means in future if you wanted to add a new type of button with a new action, the logic for it (whether it snoozes, dismisses, etc.) could be inside the package.

Ah I see now. Passing the SurveyButton itself back to an api on the analytics instance would make it easier for me to process as well; like deciding if it needs to be snoozed or permanently dismissed from showing up again. Would it be easy for you to return the SurveyButton class back to the analytics instance?

  final survey = (await analytics.fetchAvailableSurveys()).first;

  //    vvvvvvvvv   <-- will this class be the same i pass you?
  final SurveyButton selectedButton = await displaySurvey(survey.title, survey.buttons);

If the above is possible, then I can refactor to accept the button class instead of an action string

 {
      "buttonText": "More Info",
      "action": "moreInfo",
      "url": "http://example.org/",
      "promptRemainsVisible": true // may not be necessary if we can lump this functionality into the `moreInfo` action
  }

This also makes sense to me but i'll defer to @jayoung-lee... this seems to be better since we won't have to add more functionality to the website, all we need to do is set aside a button action that is used for keeping the popup up

• accept: means the user has decided to take the survey and we dismiss it for good
• dismiss: means the user has decided not to take the survey and we dismiss it for good
• snooze: if we have a button that says dismiss for a few minutes (won't get rid of it permanently)
• moreInfo: when this is clicked, the IDE will know to open the popup again with the same survey
  • however, by default when this button is clicked the package will "snooze" survey so it doesn't show up again while the IDE brings it back up for an additional 10-15 seconds as @DanTup mentioned

[eliasyishak]

@DanTup would an explicit field on SurveyButton be better for indicating to keep the prompt visible or would it be enough to check for an action string == moreInfo?

[DanTup]

Would it be easy for you to return the SurveyButton class back to the analytics instance?

  final survey = (await analytics.fetchAvailableSurveys()).first;

  //    vvvvvvvvv   <-- will this class be the same i pass you?
  final SurveyButton selectedButton = await displaySurvey(survey.title, survey.buttons);

Yes, I believe so. Although we're round-tripping via JSON to the client here, the code in the server will just be awaiting the response. Although we might need to put the buttons into a Map by their text (or index) to match up the response we get from the JSON, I think it's worth doing that to have a nicer API.

all we need to do is set aside a button action that is used for keeping the popup up
...
would an explicit field on SurveyButton be better for indicating to keep the prompt visible or would it be enough to check for an action string == moreInfo?

My suggestion above for using a separate field was to avoid the client needing to do any interpretation of action. By adding a specific field that controls the behaviour, it frees the survey package to invent new actions in future, but still control the behaviour in the editor (eg. it could create new kinds of buttons/actions that keep the prompt visible without the clients needing to be updated to know about them).

Again, I'm not sure how useful this would actually be, but I feel like the less logic (such as what each action actually means) in the editor, the simpler the integration for each client, and the more flexible it is for survey package changes in the future (and if this all comes from the JSON, the behaviour could even vary per-survey - although for this specific example that's probably unlikely).

[eliasyishak]

Yes, I believe so. Although we're round-tripping via JSON to the client here, the code in the server will just be awaiting the response.

Great so I can start to refactor dismissSurvey(...) to be surveyInteracted(survey, buttonClicked) (which is better since it's more general) and take in a Survey instance, along with the SurveyButton that was clicked on by the user.

What kind of response can you receive from the JSON when a button is clicked? I can setup a Map<String, SurveyButton> where the key will be the text for the button (we'll need to ensure we have a check to ensure no duplicate texts are uploaded)

My suggestion above for using a separate field was to avoid the client needing to do any interpretation of action.

I see, so instead of interacting with the action field to determine if it stays up, we will instead interact with the promptRemainsVisible field (which will need to be added) but at least this will only have one purpose. Does that sound like what you were imagining?

[DanTup]

What kind of response can you receive from the JSON when a button is clicked?

For LSP, the original behaviour was that we just get back the text of the button that was clicked, although LSP now also supports round-tripping arbitrary data with each button, so we can include an index or similar so we could locate the button that way (it's not certain that all clients support that though, so the button text is the most compatible).

I can setup a Map<String, SurveyButton> where the key will be the text for the button (we'll need to ensure we have a check to ensure no duplicate texts are uploaded)

If you provide it we could use it there to save making our own - although it also feels quite LSP-specific, so having the analysis server produce a Map also seems reasonable to me. If you do expose a Map, we'd need to ensure it's sorted (or there's also a List) to ensure the buttons appear in the correct order.

I see, so instead of interacting with the action field to determine if it stays up, we will instead interact with the promptRemainsVisible field (which will need to be added) but at least this will only have one purpose. Does that sound like what you were imagining?

Assuming "we" here means the client of the package (eg. analysis server), yep. The survey package presumably would still use the action internally (to know how to record stats etc.) , but the specific flags for clients means that clients don't need to contain the logic that maps actions on to the individual behaviours (which would probably just be a few if/switch expressions, but having that inside the survey package seems cleaner).

[eliasyishak]

If you provide it we could use it there to save making our own - although it also feels quite LSP-specific, so having the analysis server produce a Map also seems reasonable to me. If you do expose a Map, we'd need to ensure it's sorted (or there's also a List) to ensure the buttons appear in the correct order.

Yep, that just reminded me why I did put it in a list originally now; to preserve the correct order. With that consideration, I will have the analytics package just provide a list and leave it up to the client of this package to determine which button was clicked (since this may be different for each client). But I will still have surveyInteracted(...) take in the Survey and SurveyButton instances

but the specific flags for clients means that clients don't need to contain the logic that maps actions on to the individual behaviours

I agree this is a big value add as well, I'll make the necessary updates to include this new field for each button in the json

 {
      "buttonText": "More Info",
      "action": "moreInfo",
      "url": "http://example.org/",
      "promptRemainsVisible": true
  }

[eliasyishak]

Okay the updates I mentioned last have been committed. The example code below shows the workflow

  // Use the analytics instance to fetch the first available survey
  final Survey survey = (await analytics.fetchAvailableSurveys()).first;

  // Invoke the method to let the analytics package know that the survey
  // has been shown
  //
  // This will automatically "snooze" the first survey so that subsequent windows
  // in VS Code don't show the same survey
  analytics.surveyShown(survey);

  // Client shows the survey and gets back the button the user clicked
  //
  // For this example, I am assuming you are receiving the index of the button
  // that was clicked so i can grab the button from the button list by index
  //
  // Real implementation may use a Map if needed; we just need to be able to identify
  // which button in `survey.buttonList` was selected
  final int selectedButtonIndex = await displaySurvey(survey.description, survey.buttonList);
  final SurveyButton selectedButton = survey.buttonList[selectedButtonIndex];

  // If the button has a URL, open it
  if (selectedButton?.url != null) {
    openUrl(selectedButton?.url);
  }

  // Tell the package which button was clicked
  // Package handles reading "action" and recording appropriately
  analytics.surveyInteracted(survey, selectedButton);

  // If the button clicked requires the pop up to remain up
  if (selectedButton.promptRemainsVisible) {
    // Code to re-prompt the survey and handle the above again
  }

@DanTup if the above looks good to you, then I can go ahead with the merge. This marks the final merge before we can begin implementation

[eliasyishak]

@DanTup this example file should make it easier for us to discuss any changes to the workflow (if any are needed)

[eliasyishak]

@bwilkerson also an fyi if this example file workflow... from the analysis server side, does this workflow make sense to you?

[DanTup]

All sounds good to me!

[DanTup]

If this is useful info for debugging, printing when there's a URL that would be opened might be useful too.