feature: offer users a HaTS survey on the design page #8193

beyackle · 2021-06-24T21:33:51Z

Description

This is the completed feature branch for the HaTS survey feature. Please note that there is code in here that made it into the feature branch without being reviewed, as well as some late-entering bug fixes (I hadn't noticed that the card's own close box triggers the same Recoil dispatcher as I'd been calling manually, but also needed to insert a telemetry item there, so there are a few changes in that area), so this could still use a full review before it gets merged.

Task Item

closes #8029

Screenshots

(not shown in the screenshot is that the "Take the Survey" link routes to the correct URL with the query fields populated with OS, Composer version, machine ID, and Source)

* add stuff to localStorage * add stuff to localStorage * move logic and add more checks * Update constants.tsx

…tFramework-Composer into beyackle/hatsSurveyLink

* add custom style * Update Publish.tsx * display card on Home page * Update constants.tsx * Update shell.ts * Update Home.tsx * move survey into separate component * get more info automatically * move URL into constant * move more props around * Update constants.tsx * turn non-rendering component into hook * move card to design page * start adding ipcRenderer stuff * update card design * get machineID and stash in Recoil * read OS and place in URL * add spacer to notification card * better notification styling * fix constants and typercheck error

This reverts commit 5b9f164.

* add custom style * Update Publish.tsx * display card on Home page * Update constants.tsx * Update shell.ts * Update Home.tsx * move survey into separate component * get more info automatically * move URL into constant * move more props around * Update constants.tsx * turn non-rendering component into hook * move card to design page * start adding ipcRenderer stuff * update card design * get machineID and stash in Recoil * read OS and place in URL * add spacer to notification card * better notification styling * fix constants and typercheck error * fixes from PR * fix card alignment and make aka.ms URL * remove redundant prop * use query-string package to generate link * move MAC truncation to electron server * add fixes from CR and machineID truncation * fix typechecking * Update ToolbarButtonMenu.test.tsx * Update App.tsx

Composer/packages/client/src/components/Notifications/NotificationCard.tsx

tdurnford · 2021-06-24T21:41:26Z

Composer/packages/client/src/components/Notifications/NotificationCard.tsx

+        <Stack horizontal horizontalAlign="space-between">
+          <Stack {...stackProps}>
+            {leftLinkList.map(
+              (link) => link != null && <Stack.Item key={link.label}>{makeLinkLabel(link)}</Stack.Item>


When will link be null?

I'm not sure either, but it wouldn't typecheck without this and it's a harmless thing to check anyway.

The type checker was probably complaining because you previously added null and undefined to the link properties in the Notification type in Composer/packages/types/src/shell.ts. If you removed the check and restart your Typescript Server, the type warning would probably resolve.

But yes, you are correct, the check is harmless.

tdurnford · 2021-06-24T21:44:56Z

Composer/packages/client/src/recoilModel/dispatchers/application.ts

+      // ... and have either never taken the survey or it's been long enough since we did
+      (lastTaken == null || Date.now() - lastTaken > SURVEY_PARAMETERS.timeUntilNextSurvey)
+    ) {
+      // do we hit the probability of the survey appearing?


Leave a better comment explaining what is happening here. It's not clear, and the comment is only vaguely helpful.

cypress · 2021-06-24T21:50:38Z

Test summary

16 • 0 • 1 • 0 • 0

Run details


Project	Composer
Status	Passed
Commit	`cc2808c`
Started	Jun 28, 2021 5:44 PM
Ended	Jun 28, 2021 5:50 PM
Duration	06:23 💡
OS	Linux Ubuntu - 20.04
Browser	Electron 89

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

beyackle · 2021-06-24T21:43:22Z

Composer/packages/client/src/components/Notifications/NotificationCard.tsx

  return (
    <div css={cardContent}>
      {type === 'error' && <Icon css={errorType} iconName="ErrorBadge" />}
      {type === 'success' && <Icon css={successType} iconName="Completed" />}
      {type === 'warning' && <Icon css={warningType} iconName="Warning" />}
+      {type === 'question' && <Icon css={questionType} iconName="UnknownSolid" />}
+      {type === 'congratulation' && <Icon css={congratulationType} iconName="Trophy2Solid" />}


A note here: this isn't used anywhere yet, so I'm all right leaving it out, but it makes a good indicator type for messages like "Congratulations, you've published your first bot to Azure!" or such.

beyackle · 2021-06-24T21:45:00Z

Composer/packages/client/src/components/Notifications/NotificationCard.tsx

@@ -196,7 +259,10 @@ export const NotificationCard = React.memo((props: NotificationProps) => {
        ariaLabel={formatMessage('Close')}
        css={cancelButton}
        iconProps={{ iconName: 'Cancel', styles: { root: { fontSize: '12px' } } }}
-        onClick={() => onDismiss(id)}
+        onClick={() => {
+          cardProps?.onDismiss?.(id);


This is what lets us track dismissals using the close box (or any other custom action we want to perform when the user closes a notification, relayed via card props).

You should leave that as a comment in the code

beyackle · 2021-06-24T21:45:30Z

Composer/packages/client/src/components/Notifications/useSurveyNotification.ts

+  const version = process.env.COMPOSER_VERSION;
+
+  const parameters = {
+    Source: 'Composer',


The capital S is how this is in the spec and how it comes out in the query parameters.

beyackle · 2021-06-24T21:46:55Z

Composer/packages/client/src/pages/design/DesignPage.tsx

@@ -33,6 +34,8 @@ const DesignPage: React.FC<RouteComponentProps<{ dialogId: string; projectId: st

  const activeBot = skillId ?? projectId;

+  useSurveyNotification();


If we ever decide the notification should be triggered elsewhere, this hook is all that will need to be moved there.

beyackle · 2021-06-24T21:59:16Z

Composer/packages/client/src/components/Notifications/NotificationCard.tsx

+        <Stack horizontal horizontalAlign="space-between">
+          <Stack {...stackProps}>
+            {leftLinkList.map(
+              (link) => link != null && <Stack.Item key={link.label}>{makeLinkLabel(link)}</Stack.Item>


I'm not sure either, but it wouldn't typecheck without this and it's a harmless thing to check anyway.

coveralls · 2021-06-24T22:26:36Z

Coverage decreased (-0.03%) to 54.628% when pulling cc2808c on feature/hats-survey into 9986da5 on main.

Composer/packages/electron-server/src/utility/machineId.ts

srinaath · 2021-06-28T16:28:38Z

@beyackle Apart from the minor comments this PR looks good. Lets get it merged today.
CC @garypretty

beyackle added 14 commits June 10, 2021 10:27

feature: add HaTS stuff to localStorage (#8040)

fce1dda

* add stuff to localStorage * add stuff to localStorage * move logic and add more checks * Update constants.tsx

create prototype card

5df7ea6

Merge branch 'main' into feature/hats-survey

f82213b

Merge branch 'feature/hats-survey' of https://github.com/microsoft/Bo…

ade7f27

…tFramework-Composer into beyackle/hatsSurveyLink

add congratulation notification

fb1432a

Merge branch 'main' into feature/hats-survey

7904388

Merge branch 'main' into feature/hats-survey

b9ddfc4

Revert "feature: show survey card on Home page (#8078)" (#8149)

852daf3

This reverts commit 5b9f164.

Merge branch 'main' into feature/hats-survey

2fe5b61

Merge branch 'main' into feature/hats-survey

7fe0126

add custom onDismiss to cards

0eae9a0

restore delete-on-click for links

127a7a7

beyackle requested review from a-b-r-o-w-n, benbrown, boydc2014, cwhitten, GeoffCoxMSFT, hatpick, pavolum, srinaath, tdurnford and tonyanziano as code owners June 24, 2021 21:33

tdurnford reviewed Jun 24, 2021

View reviewed changes

Composer/packages/client/src/components/Notifications/NotificationCard.tsx Outdated Show resolved Hide resolved

github-actions bot assigned tdurnford Jun 24, 2021

tdurnford reviewed Jun 24, 2021

View reviewed changes

beyackle commented Jun 24, 2021

View reviewed changes

Update NotificationCard.tsx

eb12e09

beyackle added 3 commits June 24, 2021 15:41

add comments from PR

54d7a2e

revert ToolbarButtonMenu test

cc5a4d4

Update NotificationCard.tsx

1f1e30e

srinaath reviewed Jun 28, 2021

View reviewed changes

Composer/packages/electron-server/src/utility/machineId.ts Show resolved Hide resolved

github-actions bot assigned srinaath Jun 28, 2021

add comment about truncation

cc2808c

srinaath approved these changes Jun 29, 2021

View reviewed changes

beyackle merged commit 2e4b716 into main Jun 29, 2021

beyackle deleted the feature/hats-survey branch June 29, 2021 23:19

beyackle mentioned this pull request Jul 9, 2021

chore: add unit tests to HaTS survey logic #8308

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: offer users a HaTS survey on the design page #8193

feature: offer users a HaTS survey on the design page #8193

beyackle commented Jun 24, 2021

tdurnford Jun 24, 2021

beyackle Jun 24, 2021

tdurnford Jun 24, 2021 •

edited

tdurnford Jun 24, 2021

cypress bot commented Jun 24, 2021 •

edited

beyackle Jun 24, 2021

beyackle Jun 24, 2021

tdurnford Jun 24, 2021

beyackle Jun 24, 2021

beyackle Jun 24, 2021

beyackle Jun 24, 2021

coveralls commented Jun 24, 2021 •

edited

srinaath commented Jun 28, 2021

		@@ -33,6 +34,8 @@ const DesignPage: React.FC<RouteComponentProps<{ dialogId: string; projectId: st

		const activeBot = skillId ?? projectId;

		useSurveyNotification();

feature: offer users a HaTS survey on the design page #8193

feature: offer users a HaTS survey on the design page #8193

Conversation

beyackle commented Jun 24, 2021

Description

Task Item

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdurnford Jun 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Jun 24, 2021 • edited

Test summary

Run details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 24, 2021 • edited

srinaath commented Jun 28, 2021

tdurnford Jun 24, 2021 •

edited

cypress bot commented Jun 24, 2021 •

edited

coveralls commented Jun 24, 2021 •

edited