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

feat: add self-hosted, doNotTrack-aware, analytics #930

Merged
merged 15 commits into from
Feb 25, 2019
Merged

Conversation

olizilla
Copy link
Member

@olizilla olizilla commented Jan 9, 2019

Add analytics via self-hosted countly install.

ipfs-analytics-2

WIP on #774
see also: ipfs/ipfs-gui#72

TODO:

License: MIT
Signed-off-by: Oli Evans oli@tableflip.io

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@ghost ghost assigned olizilla Jan 9, 2019
@ghost ghost added the status/in-progress In progress label Jan 9, 2019
License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@olizilla
Copy link
Member Author

olizilla commented Jan 31, 2019

A good first pass at the copy for "what data is collected?"

screenshot 2019-01-31 at 11 20 19

From the info at https://resources.count.ly/docs/security-faq#section-default-metrics-properties-collected

@olizilla
Copy link
Member Author

Tests for the bundle logic are in place 📋 ✅ ✨

https://github.com/ipfs-shipyard/ipfs-webui/blob/186a5086f15cc9d710638184aa5675a38ed0de3f/src/bundles/analytics.test.js

 PASS  src/bundles/analytics.test.js
  ✓ should normalise the doNotTrack state from the navigator.doNotTrack value (6ms)
  ✓ should enable analytics if doNotTrack is falsey
  ✓ should disable analytics if doNotTrack is true (1ms)
  ✓ should enable analytics if doNotTrack is true but user has explicitly enabled it
  ✓ should disable analytics if doNotTrack is falsey but user has explicitly disabled it (1ms)
  ✓ should enable selectAnalyticsAskToEnable if doNotTrack is true and user has not explicity enabled or disabled it
  ✓ should disable selectAnalyticsAskToEnable if doNotTrack is true and user has explicity disabled it (1ms)
  ✓ should disable selectAnalyticsAskToEnable if doNotTrack is true and user has explicity enabled it
  ✓ should disable selectAnalyticsAskToEnable if analytics are enabled
  ✓ should toggle analytics (14ms)

Test Suites: 1 passed, 1 total
Tests:       10 passed, 10 total
Snapshots:   0 total
Time:        1.53s

@olizilla olizilla changed the title wip: add countly self-hosted, opt-in analytics wip: add countly self-hosted, doNotTrack-aware, analytics Jan 31, 2019
@olizilla olizilla changed the title wip: add countly self-hosted, doNotTrack-aware, analytics wip: add self-hosted, doNotTrack-aware, analytics Jan 31, 2019
@olizilla
Copy link
Member Author

Call to action on the status page if you have doNotTrack enabled and you haven't yet explicitly opted in or out.

screenshot 2019-01-31 at 11 47 14

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@olizilla olizilla changed the title wip: add self-hosted, doNotTrack-aware, analytics feat: add self-hosted, doNotTrack-aware, analytics Jan 31, 2019
Copy link
Contributor

@fsdiogo fsdiogo left a comment

Choose a reason for hiding this comment

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

In general LGTM, but please check my suggestions!

public/locales/en/settings.json Outdated Show resolved Hide resolved
public/locales/en/settings.json Outdated Show resolved Hide resolved
src/bundles/analytics.js Outdated Show resolved Hide resolved
src/bundles/analytics.js Show resolved Hide resolved
src/bundles/analytics.test.js Outdated Show resolved Hide resolved
src/components/analytics-toggle/AnalyticsToggle.js Outdated Show resolved Hide resolved
const items = Array(9).fill(1)
return (
<React.Fragment>
<Checkbox className='dib bg-white pa3' onChange={doToggleAnalytics} checked={analyticsEnabled} label={
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Checkbox className='dib bg-white pa3' onChange={doToggleAnalytics} checked={analyticsEnabled} label={
<Checkbox className='dib pa3' onChange={doToggleAnalytics} checked={analyticsEnabled} label={

I prefer this without the white background to blend with the rest of the Settings, but I'll leave it up to you!

{t('AnalyticsToggle.label')}
</span>
} />
<div className='f6 charcoal lh-copy mw7'>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div className='f6 charcoal lh-copy mw7'>
<div className='f6 ml3 charcoal lh-copy mw7'>

fsdiogo and others added 2 commits February 5, 2019 15:25
Co-Authored-By: olizilla <oli@tableflip.io>
Co-Authored-By: olizilla <oli@tableflip.io>
@olizilla
Copy link
Member Author

News on this. We're setting up a countly instance at countly.ipfs.io should be ready by next week.

@olizilla
Copy link
Member Author

Waiting on DNS for countly.ipfs.io https://github.com/protocol/infra/pull/438

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@olizilla
Copy link
Member Author

I'd like to roll this out today, so please eyeball the code!

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

LGTM, tiny suggestions below

repoStats
repoStats,
createAnalyticsBundle({
countlyAppKey: window.ipfsDesktop ? '47fbb3db3426d2ae32b3b65fe40c564063d8b55d' : '8fa213e6049bff23b08e5f5fbac89e7c27397612',
Copy link
Member

Choose a reason for hiding this comment

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

nit: meaning can be lost/swapped during future refectoring, and it will be awfully hard to detect.
May be safer to move API keys to COUNTLY_APPKEY_DESKTOP / COUNTLY_APPKEY_WEBUI consts

"AnalyticsToggle": {
"label": "Help improve this app by sending anonymous usage data",
"summary": "What data is collected?",
"paragraph1": "IPFS hosts a <1>Countly</1> instance to record anonymous usage data for this app.",
Copy link
Member

@lidel lidel Feb 22, 2019

Choose a reason for hiding this comment

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

Perhaps we should clarify we mean Shipyard||Community||Project, not the protocol itself.

Suggested change
"paragraph1": "IPFS hosts a <1>Countly</1> instance to record anonymous usage data for this app.",
"paragraph1": "IPFS Shipyard hosts a <1>Countly</1> instance to record anonymous usage data for this app.",


function getDoNotTrack () {
if (!root.navigator) return false
const value = root.doNotTrack || root.navigator.doNotTrack || root.navigator.msDoNotTrack
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

LGTM! I subscribe @lidel's and @fsdiogo's comments.

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@olizilla olizilla merged commit b3fe232 into master Feb 25, 2019
@olizilla olizilla deleted the analytics branch February 25, 2019 10:19
@ghost ghost removed the status/in-progress In progress label Feb 25, 2019
@olizilla olizilla mentioned this pull request Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants