Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: webui update metrics to opt-out by default (part of 2074) #2084

Merged
merged 13 commits into from
Jan 27, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion public/locales/en/status.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
},
"customApiConfig": "Custom JSON configuration",
"AskToEnable": {
"label": "Help improve this app by sending anonymous usage data."
"label": "We have recently updated our telemetry policy to switch to opt-out metrics. You previously had telemetry collection disabled. You can disable telemetry collection (again) on the settings page."
},
"tour": {
"step1": {
Expand Down
52 changes: 38 additions & 14 deletions src/bundles/analytics.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,20 @@ import { ACTIONS as EXP } from './experiments'
* @property {'ANALYTICS_ADD_CONSENT'} type
* @property {{name:string}} payload
*
* @typedef {Object} ToggleAskToEnable
* @property {'ANALYTICS_ASK_TO_ENABLE'} type
0xDanomite marked this conversation as resolved.
Show resolved Hide resolved
* @property {{shouldAsk:boolean?}} payload
*
* @typedef {ExperimentsToggle|DesktopSettingToggle} Toggle
* @typedef {MakeDir|Write|AddByPath|Move|Delete|DownloadLink} FilesMessage
* @typedef {AnalyticsEnabled|AnalyticsDisabled|RemoveConsent|AddConsent} AnalyticsMessage
* @typedef {AnalyticsEnabled|AnalyticsDisabled|RemoveConsent|AddConsent|ToggleAskToEnable} AnalyticsMessage
* @typedef {Init|ConfigSave|Toggle|FilesMessage|AnalyticsMessage} Message
*
* @typedef {Object} Model
* @property {number} lastEnabledAt
* @property {number} lastDisabledAt
* @property {string[]} consent
* @property {boolean?} showAskToEnable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

showBanner ?

*
* @typedef {Object} State
* @property {Model} analytics
Expand All @@ -72,7 +77,8 @@ const ACTIONS = Enum.from([
'ANALYTICS_ENABLED',
'ANALYTICS_DISABLED',
'ANALYTICS_ADD_CONSENT',
'ANALYTICS_REMOVE_CONSENT'
'ANALYTICS_REMOVE_CONSENT',
'ANALYTICS_ASK_TO_ENABLE'
0xDanomite marked this conversation as resolved.
Show resolved Hide resolved
])

// Only record specific actions listed here.
Expand Down Expand Up @@ -148,20 +154,19 @@ const selectors = {
* @param {State} state
*/
selectAnalyticsEnabled: (state) => state.analytics.consent.length > 0,
/**
* @param {State} state
*/
selectAnalyticsConsentNeverChosen: (state) => state.analytics.consent.length === 0 && !state.analytics.lastDisabledAt && !state.analytics.lastEnabledAt,
/**
* @param {State} state
*/
selectAnalyticsConsentOptedOut: (state) => state.analytics.consent.length === 0 && state.analytics.lastDisabledAt && !state.analytics.lastEnabledAt,
0xDanomite marked this conversation as resolved.
Show resolved Hide resolved
/**
* Ask the user if we may enable analytics.
* @param {State} state
*/
selectAnalyticsAskToEnable: (state) => {
const { lastEnabledAt, lastDisabledAt, consent } = state.analytics
// user has not explicitly chosen
if (!lastEnabledAt && !lastDisabledAt && consent.length === 0) {
// ask to enable.
return true
}
// user has already made an explicit choice; dont ask again.
return false
},
selectAnalyticsAskToEnable: (state) => state.analytics.showAskToEnable,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
selectAnalyticsAskToEnable: (state) => state.analytics.showAskToEnable,
selectShowAnalyticsBanner: (state) => state.analytics.showAnalyticsBanner,


selectAnalyticsActionsToRecord: createSelector(
'selectIsIpfsDesktop',
Expand Down Expand Up @@ -257,6 +262,13 @@ const actions = {
}
addConsent(name, store)
dispatch({ type: 'ANALYTICS_ADD_CONSENT', payload: { name } })
},
/**
* @param {boolean?} shouldAsk
* @returns {function(Context):void}
*/
doToggleAskToEnable: (shouldAsk) => ({ dispatch }) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
doToggleAskToEnable: (shouldAsk) => ({ dispatch }) => {
doToggleAnalyticsBanner: (shouldAsk) => ({ dispatch }) => {

this name doesn't quite make sense.. but something better than doToggleAskToEnable

dispatch({ type: 'ANALYTICS_ASK_TO_ENABLE', payload: { shouldAsk } })
}
}

Expand All @@ -276,7 +288,8 @@ const createAnalyticsBundle = ({
ACTIONS.ANALYTICS_DISABLED,
ACTIONS.ANALYTICS_DISABLED,
ACTIONS.ANALYTICS_ADD_CONSENT,
ACTIONS.ANALYTICS_REMOVE_CONSENT
ACTIONS.ANALYTICS_REMOVE_CONSENT,
ACTIONS.ANALYTICS_ASK_TO_ENABLE
0xDanomite marked this conversation as resolved.
Show resolved Hide resolved
],

/**
Expand Down Expand Up @@ -310,10 +323,16 @@ const createAnalyticsBundle = ({
// Don't track clicks or links as it can include full url.
// Countly.q.push(['track_clicks'])
// Countly.q.push(['track_links'])
Comment on lines 324 to 326
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a note that we can filter out URLs that have CIDs (for any future readers)


if (store.selectAnalyticsEnabled()) {
const consent = store.selectAnalyticsConsent()
addConsent(consent, store)
SgtPooki marked this conversation as resolved.
Show resolved Hide resolved
} else if (store.selectAnalyticsConsentOptedOut() || store.selectAnalyticsConsentNeverChosen()) {
if (store.selectAnalyticsConsentOptedOut()) {
store.doToggleAskToEnable(true)
}

// add consent/opt in by default
store.doEnableAnalytics()
}

store.subscribeToSelectors(['selectRouteInfo'], ({ routeInfo }) => {
Expand Down Expand Up @@ -371,6 +390,7 @@ const createAnalyticsBundle = ({
state = state || {
lastEnabledAt: 0,
lastDisabledAt: 0,
showAskToEnable: false,
0xDanomite marked this conversation as resolved.
Show resolved Hide resolved
consent: []
}

Expand All @@ -388,6 +408,10 @@ const createAnalyticsBundle = ({
const lastDisabledAt = (consent.length === 0) ? Date.now() : state.lastDisabledAt
return { ...state, lastDisabledAt, consent }
}
case ACTIONS.ANALYTICS_ASK_TO_ENABLE: {
const showAskToEnableBanner = action.payload?.shouldAsk || false
return { ...state, showAskToEnable: showAskToEnableBanner }
0xDanomite marked this conversation as resolved.
Show resolved Hide resolved
}
default: {
// deal with missing consent state from 2.4.0 release.
if (!state.consent) {
Expand Down
49 changes: 30 additions & 19 deletions src/bundles/analytics.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,19 @@ function createStore (analyticsOpts = {}) {
)()
}

it('should disable analytics by default', () => {
it('should enable analytics by default', () => {
const store = createStore()
expect(store.selectAnalyticsEnabled()).toBe(false)
// no events will be sent as no consents have been given
expect(store.selectAnalyticsConsent()).toEqual([])
// user has not explicitly opted in or out yet
expect(global.Countly.opt_in).not.toHaveBeenCalled()
expect(global.Countly.opt_out).not.toHaveBeenCalled()
expect(store.selectAnalyticsEnabled() || store.selectAnalyticsInitialConsent()).toBe(true)
0xDanomite marked this conversation as resolved.
Show resolved Hide resolved
// user has not explicitly opted out yet
expect(global.Countly.opt_in).toHaveBeenCalled()
expect(global.Countly.opt_in.mock.calls.length).toBe(1)
// events will be sent as consents have been given by default
expect(store.selectAnalyticsConsent()).toEqual(['sessions', 'events', 'views', 'location'])
})

it('should enable analytics if user has explicitly enabled it', () => {
const store = createStore()
global.Countly.opt_in.mockClear()
0xDanomite marked this conversation as resolved.
Show resolved Hide resolved
store.doEnableAnalytics()
expect(store.selectAnalyticsEnabled()).toBe(true)
expect(store.selectAnalyticsConsent()).toEqual(['sessions', 'events', 'views', 'location'])
Expand All @@ -52,6 +53,8 @@ it('should enable analytics if user has explicitly enabled it', () => {

it('should disable analytics if user has explicitly disabled it', () => {
const store = createStore()
global.Countly.opt_in.mockClear()
global.Countly.opt_out.mockClear()
store.doDisableAnalytics()
expect(store.selectAnalyticsEnabled()).toBe(false)
expect(store.selectAnalyticsConsent()).toEqual([])
Expand All @@ -60,40 +63,34 @@ it('should disable analytics if user has explicitly disabled it', () => {
expect(global.Countly.opt_out.mock.calls.length).toBe(1)
})

it('should enable selectAnalyticsAskToEnable if user has not explicity enabled or disabled it', () => {
it('should disable selectAnalyticsAskToEnable if user has not explicity enabled or disabled it', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might make sense to rename these tests as user stories:

describe('user explicitly opted out', () => {
 it('selectShouldShowBanner is true', () => {...})
 it('consent is set to defaults', () => {...})
} 

describe('user explicitly opted in', () => {
 it('selectShouldShowBanner is false', () => {...})
 it('consent is what is in the store', () => {...})
} 

describe('user has not accepted nor declined consent', () => {
 it('selectShouldShowBanner is false', () => {...})
 it('consent is set to defaults', () => {...})
} 

const store = createStore()
expect(store.selectAnalyticsAskToEnable()).toBe(true)
expect(store.selectAnalyticsAskToEnable()).toBe(false)
})

it('should disable selectAnalyticsAskToEnable if user has explicity disabled it', () => {
const store = createStore()
global.Countly.opt_out.mockClear()
store.doDisableAnalytics()
expect(store.selectAnalyticsAskToEnable()).toBe(false)
})
0xDanomite marked this conversation as resolved.
Show resolved Hide resolved

it('should disable selectAnalyticsAskToEnable if user has explicity enabled it', () => {
const store = createStore()
global.Countly.opt_out.mockClear()
store.doEnableAnalytics()
expect(store.selectAnalyticsAskToEnable()).toBe(false)
})

it('should disable selectAnalyticsAskToEnable if analytics are enabled', () => {
const store = createStore()
global.Countly.opt_out.mockClear()
store.doEnableAnalytics()
expect(store.selectAnalyticsAskToEnable()).toBe(false)
})

it('should toggle analytics', () => {
const store = createStore()

store.doToggleAnalytics()
expect(store.selectAnalyticsEnabled()).toBe(true)
expect(store.selectAnalyticsConsent()).toEqual(['sessions', 'events', 'views', 'location'])
expect(global.Countly.opt_in).toHaveBeenCalled()
expect(global.Countly.opt_out).not.toHaveBeenCalled()
expect(global.Countly.opt_in.mock.calls.length).toBe(1)

// reset the mocks here to simplify the following assetions
global.Countly.opt_in.mockClear()
global.Countly.opt_out.mockClear()

Expand All @@ -103,11 +100,25 @@ it('should toggle analytics', () => {
expect(global.Countly.opt_in).not.toHaveBeenCalled()
expect(global.Countly.opt_out).toHaveBeenCalled()
expect(global.Countly.opt_out.mock.calls.length).toBe(1)

// reset the mocks here to simplify the following assetions
global.Countly.opt_in.mockClear()
global.Countly.opt_out.mockClear()

store.doToggleAnalytics()
expect(store.selectAnalyticsEnabled()).toBe(true)
expect(store.selectAnalyticsConsent()).toEqual(['sessions', 'events', 'views', 'location'])
expect(global.Countly.opt_in).toHaveBeenCalled()
expect(global.Countly.opt_out).not.toHaveBeenCalled()
expect(global.Countly.opt_in.mock.calls.length).toBe(1)
})

it('should toggle consent', () => {
const store = createStore()

// reset to off from previous
store.doToggleAnalytics()
global.Countly.opt_in.mockClear()
global.Countly.opt_out.mockClear()
store.doToggleConsent('crashes')
expect(store.selectAnalyticsEnabled()).toBe(true)
expect(store.selectAnalyticsConsent()).toEqual(['crashes'])
Expand Down
12 changes: 3 additions & 9 deletions src/components/ask/AskToEnable.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,14 @@
import React from 'react'
import Button from '../button/Button'

const AskToEnable = ({ className, label, yesLabel, noLabel, onYes, onNo, detailsLabel = 'More info...', detailsLink }) => {
const AskToEnable = ({ className, label, yesLabel, onYes }) => {
return (
<div className={`f6 pv3 pv2-ns ph3 tc bg-snow charcoal ${className}`}>
<span className='fw4 lh-copy dib mb2'>
<div className={`f6 pv3 pv2-ns ph3 tc bg-snow charcoal flex ${className}`}>
<span className='fw4 lh-copy dib mb2 tl'>
{label}
{detailsLink && (
<a href={detailsLink} className='dib focus-outline link blue ml2'>
{detailsLabel}
</a>
)}
</span>
<span className='dib'>
<Button className='ml3 mv1 tc' bg={'bg-green'} onClick={onYes}>{yesLabel}</Button>
<Button className='ml3 mv1 tc' color='charcoal' bg={'bg-snow-muted'} onClick={onNo}>{noLabel}</Button>
</span>
</div>
)
Expand Down
10 changes: 4 additions & 6 deletions src/status/StatusPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const StatusPage = ({
analyticsAskToEnable,
doEnableAnalytics,
doDisableAnalytics,
doToggleAskToEnable,
toursEnabled,
handleJoyrideCallback,
nodeBandwidthEnabled
Expand Down Expand Up @@ -56,12 +57,8 @@ const StatusPage = ({
<AskToEnable
className='mt3'
label={t('AskToEnable.label')}
yesLabel={t('app:actions.ok')}
noLabel={t('app:actions.noThanks')}
detailsLabel={t('app:actions.moreInfo')}
0xDanomite marked this conversation as resolved.
Show resolved Hide resolved
detailsLink='#/settings/analytics'
onYes={doEnableAnalytics}
onNo={doDisableAnalytics} />
yesLabel={t('app:actions.close')}
onYes={() => doToggleAskToEnable(false)} />
}
<div style={{ opacity: ipfsConnected ? 1 : 0.4 }}>
{ nodeBandwidthEnabled
Expand Down Expand Up @@ -98,5 +95,6 @@ export default connect(
'selectToursEnabled',
'doEnableAnalytics',
'doDisableAnalytics',
'doToggleAskToEnable',
withTour(withTranslation('status')(StatusPage))
)