Rework product analytics consent #1542
Conversation
OskarDamkjaer
changed the title
There was a problem hiding this comment.
Great!
Is there a way to write more tests on this to make sure it's not broken in the future?
Either E2E tests (should be doable to fake an Aura env?), or maybe component tests on the settings sidebar with a fake store.
The :debug output might be useful in the E2E tests.
src/browser/modules/App/App.tsx
Outdated
| export function App(props: any) { | ||
| const [derivedTheme, setEnvironmentTheme] = useDerivedTheme( | ||
| props.theme, | ||
| LIGHT_THEME | ||
| ) | ||
| // @ts-expect-error ts-migrate(7053) FIXME: No index signature with a parameter of type 'strin... Remove this comment to see the full error message | ||
| const themeData = themes[derivedTheme] || themes[LIGHT_THEME] | ||
| export class App extends React.Component<any, AppState> { |
There was a problem hiding this comment.
What's the reason for moving to a class component here?
There was a problem hiding this comment.
The metrics data event handler is now dependent on the redux state to know if we can send event or not, since it was registered in a useEffect, the handler would need to be unregistered and reregistered on every mount state had changed. At first I wasn't confident that would work well with segment and moved to a class component where the callback reference could be reused, but looking at the final implementation we could have stayed as a function component.
Fix tests Self review adsf
OskarDamkjaer
force-pushed
the
tracking_consent
branch
from
8642fcd
to
31bc550
Compare
OskarDamkjaer
force-pushed
the
tracking_consent
branch
from
54c4727
to
946067b
Compare
| @@ -256,7 +288,7 @@ export function App(props: any) { | |||
| ) | |||
| } | |||
|
|
|||
| const mapStateToProps = (state: any) => { | |||
| const mapStateToProps = (state: GlobalState) => { | |||
| @@ -697,6 +707,7 @@ export const serverConfigEpic = (some$: any, store: any) => | |||
| return Rx.Observable.of(null) | |||
| }) | |||
| }) | |||
| .do(() => store.dispatch(update({ serverConfigDone: true }))) | |||
There was a problem hiding this comment.
Not sure I'm a big fan of using this as a one-time thing - because this epic runs at an interval and the value can change between runs.
Using it like this kind of implies that it only runs once.
A name change might be good enough. serverConfigReads: Int maybe to really emphasize that it's a recurring thing?
I'm going to approve this anyway now, but give it a think and change if you want.
There was a problem hiding this comment.
This new state opens up for browser to be in a pending state for other areas as well (all other configs just above this) rather than having a local default to fallback on.
There was a problem hiding this comment.
I think serverConfigReads is a better implementation, good idea!
Co-authored-by: Oskar Hane <oh@oskarhane.com>
This PR reworks our product analytics consent settings, we now coalesce settings from Aura, Desktop, a new setting in neo4j.conf and a new setting in browser to determine if we can send user data and crash reports. To make it clear what setting takes prevalence and be confident that they are followed, I've created the util function
usedTelemetrySettingSource.The browser setting also shows where the to go to change the current settings (if not in browser) and shows a banner on the first five mounts, informing users of the new setting. Have also reached out to PM to get a review to test from the statistics end.