Add updated metrics consent prompt #632
Conversation
src/store/desktopSettings.ts
Outdated
| @@ -27,4 +27,6 @@ export type DesktopSettings = { | |||
| * - `default`: Impersonal, static, plain - default window title bar | |||
| */ | |||
| windowStyle?: 'custom' | 'default'; | |||
| /** Whether the user has consented to updated metrics collection policy (1/16/24) */ | |||
| updatedMetricsConsent?: boolean; | |||
There was a problem hiding this comment.
Maybe we just leave this as a version counter, so next time if we update our policy, we just needs to increment the counter. WDYT?
There was a problem hiding this comment.
I think it's a good idea. However, there may be situations in which a user is behind multiple versions from previous policy update, which would require effort to handle.
There was a problem hiding this comment.
I see. The frontend is now explaining the change in policy right now. Maybe we can design such a system to compare the diff of policy later when we actuall have more than 2 versions?
FYI: I think date might be more suitable here? Reduces some mental effort to map from integer to date & change.
There was a problem hiding this comment.
Changing to Date
src/install/installationManager.ts
Outdated
| @@ -98,6 +98,7 @@ export class InstallationManager { | |||
|
|
|||
| const installWizard = new InstallWizard(installOptions, this.telemetry); | |||
| useDesktopConfig().set('basePath', installWizard.basePath); | |||
| useDesktopConfig().set('metricsConsentDate', new Date()); | |||
There was a problem hiding this comment.
Sorry I didn't make the date argument clear enough. The date when the consent is made does no necessary indicate what they consent. A user can download an old version of the desktop app and get a current date consent date stored in the current setup.
If we want to track which version of the consent they signed, maybe desktop's version number is more nature to track than date?
There was a problem hiding this comment.
Ah, I understand now.
There was a problem hiding this comment.
It seems I will need to add a new build variable
There was a problem hiding this comment.
+1 for desktop version number! Will make any troubleshooting much clearer.
This PR adds a function which checks the status of the user's allowMetrics/SendStatistics setting. If the user previously opted-in, they will be prompted to confirm their choice given the new metrics policy, with the option to disable.
Any new user installing will have the flag
updatedMetricsConsentset during install, bypassing this process.The UI component is in this frontend PR: Comfy-Org/ComfyUI_frontend#2268
┆Issue is synchronized with this Notion page by Unito