feature: add new survey notification to design page #8155

beyackle · 2021-06-21T21:30:35Z

Description

[This is a redo of #8078, which was closed by mistake]

This adds the logic to show a notification on the home screen prompting the user to take a survey. This also adds some new notification types and a fairly generic way to track a user's usage of various features inside the browser's own local storage, which will be useful for other possible business cases (like extending the conditions surveys pop up in, or cosmetic things like Achievements).

Task Item

refs #8029
[will close when this merges into main]

Screenshots

cypress · 2021-06-21T23:38:28Z

Test summary

16 • 0 • 1 • 0 • 0

Run details


Project	Composer
Status	Passed
Commit	`f062b3f`
Started	Jun 24, 2021 5:12 PM
Ended	Jun 24, 2021 5:17 PM
Duration	05:29 💡
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

srinaath · 2021-06-22T18:06:23Z

Composer/packages/electron-server/src/main.ts

    mainWindow?.webContents.send('session-update', 'session-started');

    if (process.env.COMPOSER_DEV_TOOLS) {
      mainWindow?.webContents.openDevTools();
    }
+
+    log(`Machine ID is ${machineId}`);


Is this log required? Maybe only in dev env.

It's not required, but it's useful - this goes into the debug log which users won't see unless they're running via the console anyway.

Logs are only visible from the terminal, so I think it's fine.

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

srinaath · 2021-06-22T18:21:50Z

Composer/packages/client/src/App.tsx

@@ -42,6 +43,10 @@ export const App: React.FC = () => {
    ipcRenderer?.on('cleanup', (_event) => {
      performAppCleanupOnQuit();
    });
+
+    ipcRenderer?.on('machine-info', (_event, info) => {
+      setMachineInfo(info);


It looks like you are using the machine info only inside the useSurveyNotification hook. If so, why do we need to create a recoil state for it. You can just use the ipcRenderer.on inside the hook. Ex useInitializeLoger.ts

I was trying to make this work last week, but it runs into annoying race conditions where the message handler isn't initialized before the message gets sent and so nothing gets through. Using the Recoil store lets us put the machine info down somewhere whenever we receive it and then count on it being available when we need it.

Makes sense to usethe recoil state unless the hook can be registered in app.tsx

Can you explain the race condition a bit more. I'm also confused why we can't just initialize the survey here when the event is received? If the message handler doesn't get initialized before the message is sent, we would never get the message, right?

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

tdurnford · 2021-06-22T20:15:34Z

Composer/packages/electron-server/src/main.ts

    mainWindow?.webContents.send('session-update', 'session-started');

    if (process.env.COMPOSER_DEV_TOOLS) {
      mainWindow?.webContents.openDevTools();
    }
+
+    log(`Machine ID is ${machineId}`);


Logs are only visible from the terminal, so I think it's fine.

Composer/packages/types/src/shell.ts

tdurnford · 2021-06-22T20:19:01Z

Composer/packages/client/src/App.tsx

@@ -42,6 +43,10 @@ export const App: React.FC = () => {
    ipcRenderer?.on('cleanup', (_event) => {
      performAppCleanupOnQuit();
    });
+
+    ipcRenderer?.on('machine-info', (_event, info) => {
+      setMachineInfo(info);


Can you explain the race condition a bit more. I'm also confused why we can't just initialize the survey here when the event is received? If the message handler doesn't get initialized before the message is sent, we would never get the message, right?

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

tdurnford · 2021-06-24T18:05:45Z

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

@@ -13,17 +13,20 @@ import { readTextFileSync, writeJsonFileSync } from './fs';

 export const persistedFilePath = path.join(app.getPath('userData'), 'persisted.json');

+function truncate(str: string): string {


Arrow function

I don't see what that would get us here. This isn't inside a component that gets re-rendered, this isn't exported anywhere, it's just a simple utility function that belongs to this one file.

The other functions in this file use arrow functions. Best to be consistent. More of a nit.

tdurnford · 2021-06-24T18:08:28Z

Composer/packages/lib/code-editor/src/components/toolbar/__tests__/ToolbarButtonMenu.test.tsx

@@ -35,7 +35,7 @@ const templatePayload: TemplateRefPayload = {
 const propertiesPayload: PropertyRefPayload = {
  kind: 'property',
  data: {


Why are these tests being changed? They're not related to this work at all.

They're being changed because they were failing mysteriously. I feel like the word "this" is too common to be used as a name and search target in tests like this without false positives happening.

* feature: add HaTS stuff to localStorage (#8040) * add stuff to localStorage * add stuff to localStorage * move logic and add more checks * Update constants.tsx * create prototype card * add congratulation notification * feature: show survey card on Home page (#8078) * 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 * Revert "feature: show survey card on Home page (#8078)" (#8149) This reverts commit 5b9f164. * feature: add new survey notification to design page (#8155) * 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 * add custom onDismiss to cards * restore delete-on-click for links * Update NotificationCard.tsx * add comments from PR * revert ToolbarButtonMenu test * Update NotificationCard.tsx * add comment about truncation

beyackle added 24 commits June 14, 2021 16:25

add custom style

a23eebb

Update Publish.tsx

95d2677

display card on Home page

de094b9

Update constants.tsx

611ff7f

Update shell.ts

1bef946

Merge branch 'feature/hats-survey' into beyackle/hatsUsage

21aaba2

Update Home.tsx

5f51dab

move survey into separate component

397a746

get more info automatically

0fe5ebd

move URL into constant

518ed0e

move more props around

150ec25

Update constants.tsx

6a160b2

turn non-rendering component into hook

43beda7

move card to design page

ecf2e2d

start adding ipcRenderer stuff

dadb925

update card design

a9babdd

get machineID and stash in Recoil

7ea55db

read OS and place in URL

7b4d460

add spacer to notification card

27037dc

better notification styling

854cffc

fix constants and typercheck error

7d8f8ae

Merge branch 'feature/hats-survey' into beyackle/hatsUsage

f7c1459

fixes from PR

24d9f06

fix card alignment and make aka.ms URL

80f7464

beyackle requested review from a-b-r-o-w-n, benbrown, boydc2014, cwhitten, GeoffCoxMSFT and hatpick as code owners June 21, 2021 21:30

beyackle requested review from pavolum, srinaath, tdurnford and tonyanziano as code owners June 21, 2021 21:30

beyackle changed the title ~~Beyackle/hats usage~~ feature: add new survey notification to design page Jun 21, 2021

remove redundant prop

e727353

srinaath reviewed Jun 22, 2021

View reviewed changes

github-actions bot assigned srinaath Jun 22, 2021

srinaath reviewed Jun 22, 2021

View reviewed changes

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

srinaath reviewed Jun 22, 2021

View reviewed changes

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

srinaath requested changes Jun 22, 2021

View reviewed changes

use query-string package to generate link

fbeff10

github-actions bot assigned tdurnford Jun 22, 2021

tdurnford requested changes Jun 22, 2021

View reviewed changes

move MAC truncation to electron server

c6416d5

tdurnford reviewed Jun 22, 2021

View reviewed changes

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

tdurnford reviewed Jun 22, 2021

View reviewed changes

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

beyackle added 5 commits June 23, 2021 15:03

add fixes from CR and machineID truncation

ac6853a

fix typechecking

d0cd394

Update ToolbarButtonMenu.test.tsx

7121967

Update App.tsx

0dd4982

Merge branch 'feature/hats-survey' into beyackle/hatsUsage

f062b3f

tdurnford reviewed Jun 24, 2021

View reviewed changes

beyackle merged commit 587700b into feature/hats-survey Jun 24, 2021

beyackle deleted the beyackle/hatsUsage branch June 24, 2021 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add new survey notification to design page #8155

feature: add new survey notification to design page #8155

beyackle commented Jun 21, 2021 •

edited

cypress bot commented Jun 21, 2021 •

edited

srinaath Jun 22, 2021 •

edited

beyackle Jun 22, 2021

tdurnford Jun 22, 2021

srinaath Jun 22, 2021

beyackle Jun 22, 2021

srinaath Jun 22, 2021

tdurnford Jun 22, 2021

tdurnford Jun 22, 2021

tdurnford Jun 22, 2021

tdurnford Jun 24, 2021

beyackle Jun 24, 2021

tdurnford Jun 24, 2021

tdurnford Jun 24, 2021

beyackle Jun 24, 2021

		@@ -13,17 +13,20 @@ import { readTextFileSync, writeJsonFileSync } from './fs';

		export const persistedFilePath = path.join(app.getPath('userData'), 'persisted.json');

		function truncate(str: string): string {

feature: add new survey notification to design page #8155

feature: add new survey notification to design page #8155

Conversation

beyackle commented Jun 21, 2021 • edited

Description

Task Item

Screenshots

cypress bot commented Jun 21, 2021 • edited

Test summary

Run details

srinaath Jun 22, 2021 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beyackle commented Jun 21, 2021 •

edited

cypress bot commented Jun 21, 2021 •

edited

srinaath Jun 22, 2021 •

edited