-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Have Dashboard politely ask users occasionally if they'll share telemetry #5827
Conversation
Co-authored-by: fainashalts <[email protected]> Co-authored-by: liang liang <[email protected]>
bd04409
to
cae3adf
Compare
"Telemetry data" => "Telemetry"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Happy to approve tomorrow; just want to give others a chance to take a look first.
Thanks @cliffoo!
Date.now() - analyticsConfig.analyticsMessageDateTime! > | ||
ARBITRARY_ANALYTICS_NEXT_ASK_THRESHOLD; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this kind of math is rife with edge cases around leap years and whatnot. Better if we can do a proper duration check vs. millisecond arithmetic and assuming we know how many milliseconds a year is.
Happy to accept an issue for this instead of fixing it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(OK, if this triggers a day early, it doesn't really matter. But still... good practice not to assume you can just subtract dates like that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://date-fns.org/ may be a viable option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to accept an issue for this instead of fixing it now.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I've changed this to compare Date
objects instead. The logic is as follows:
const askAfter = new Date(lastAskedInMs);
askAfter.setDate(askAfter.getDate() + 365);
if (new Date() > askAfter) // Ask again!
Note that this is affected by leap years. We can fix it by using the pair setFullYear
and getFullYear
instead of setDate
and getDate
, but I figured days are easier to work with if we later want to change this threshold to some other number of days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we wanted to be super accurate, the way to do this would be to do, like, add(date, thresholdDuration)
, where thresholdDuration = { years: 1 }
, then check against that.
But it's okay. If you say "no" on 2023-12-01 and we ask again on 2024-11-30... kinda trivial.
To figure out if the user should be prompted again to opt in for analytics
When asking user to re-consider opting in for analytics
@@ -13,6 +13,7 @@ export interface ContextValue { | |||
lifecycle: ReceivedMessageLifecycle<DashboardProviderMessage> | |||
) => any; | |||
toggleNotice: () => void; | |||
updateAnalyticsConfig: (value: boolean) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for this sorta thing, I'll also note that, in situations like this, you might consider getting into the habit of making separate operations for enable vs. disable. It feels a bit more hygienic: enableAnalytics()
and disableAnalytics()
are more clear and succinct compared to updateAnalyticsConfig(true)
and updateAnalyticsConfig(false)
.
(There are downsides here, of course, like having to pass an extra function around in the context here, but my personal stance is that it's worth it to have operations align with specific user actions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I think the language is very polite and clear! I agree with @gnidan on separating enableAnalytics()
and disableAnalytics()
but that can be done as follow-on work if desired. :)
|
||
type NotificationPropsWithId = NotificationProps & { id: string }; | ||
|
||
export const analyticsNotificationId = "analytics"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, what is this id used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this is so that an id can be assigned to a specific notification, so we can reference that notification later and make changes to it.
Here the notification with id "analytics"
is updated to show hey thanks for enabling telemetry when user clicks enable, and hidden when user click no.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like it
Addresses: #4828
Use dashboard to prompt our users to graciously enable analytics.
Behavior on load:
How?
DashboardServer
's express server has two new endpoints:POST
andGET
at/analytics
. Their corresponding handlers read and write to the user config object, which in turn modifies the config file on disk. Frontend just interacts with/analytics
and manages notifications as needed.Wording
The prompt content (located at
src/utils/notifications/analytics.tsx
) mostly follows that of the previous PR (#5290).Testing instructions
~/.config/truffle-nodejs/config.json
)enableAnalytics
,analyticsSet
,analyticsMessageDateTime
for a clean slate, maybeuniqueId
also. Continue if these fields are not present.Enable
.Dismiss
.analyticsMessageDateTime
to1
(no quotes!).Related: