Ab test rammeverk #353
Ab test rammeverk #353
Conversation
| @@ -13,7 +13,7 @@ | |||
| "lint-es": "eslint --cache --cache-location '.eslintcache/' --ext .js --ext .jsx --max-warnings=0 src e2e", | |||
| "format": "node prettier.js write", | |||
| "format-check": "node prettier.js lint", | |||
| "start": "razzle start --inspect --inspect-port 9230", | |||
| "start": "cross-env GOOGLE_APPLICATION_CREDENTIALS=./secrets.json razzle start --inspect --inspect-port 9230", | |||
There was a problem hiding this comment.
Denne env variabelen bør sikkert settes på en annen måte
There was a problem hiding this comment.
Denne brukes til å sette client og server secrets for autentisering mot Google analytics management API. Optimalt bør disse verdiene settes som env variabler i de forskjellige miljøene slik at vi slipper secrets.json
src/config.js
Outdated
| case 'dev': | ||
| // test IDs | ||
| return { | ||
| GOOGLE_ACCOUNT_ID: "74405776", |
There was a problem hiding this comment.
Dette er en test konto for å verifisere under utvikling. Dette må konfigureres opp for de forskjellige miljøene.
| experiments={experiments} | ||
| onVariantMount={({expId, expVar, isActiveExperiment}) => { | ||
| if(isActiveExperiment) { | ||
| window.ga('set', { expId, expVar }); // Perhaps get GA from ndla/tracker? |
There was a problem hiding this comment.
Tracking via GA er kanskje skilt ut i en pakke allerede?
There was a problem hiding this comment.
Ja, og abtest-pakken burde vel egentlig vært en del av ndla/tracker og ikke en egen pakke? Så burde man vel kanskje gjøre window.ga-funksjonene i Experiment / Variant komponenten?
src/server/helpers/Document.js
Outdated
| (i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o), | ||
| m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;m.parentNode.insertBefore(a,m) | ||
| })(window,document,'script','https://www.google-analytics.com/analytics.js','ga'); | ||
| window.ga('create', 'UA-74405776-4', 'auto');` |
There was a problem hiding this comment.
Oppretting av tracker er muligens håndtert annerledes i dag?
| @@ -157,6 +158,11 @@ app.get('/lti', ndlaMiddleware, async (req, res) => { | |||
| handleRequest(req, res, ltiRoute); | |||
| }); | |||
|
|
|||
| app.get('/experiments', async (req, res) => { | |||
There was a problem hiding this comment.
Denne bør caches. Den vil hente experiments fra google management API. Har dere noen innspill?
There was a problem hiding this comment.
Vi bør vel legge den til i backenden et sted tipper jeg
| @@ -116,6 +117,7 @@ | |||
| "sass-loader": "^7.1.0", | |||
| "serialize-javascript": "^1.5.0", | |||
| "source-map-support": "^0.5.9", | |||
| "universal-analytics": "^0.4.20", | |||
There was a problem hiding this comment.
Ser ikke at denne er brukt noe sted?
There was a problem hiding this comment.
Den skal jeg fjerne. Gammelt rusk som vi glemte å ta ut.
| experiments={experiments} | ||
| onVariantMount={({expId, expVar, isActiveExperiment}) => { | ||
| if(isActiveExperiment) { | ||
| window.ga('set', { expId, expVar }); // Perhaps get GA from ndla/tracker? |
There was a problem hiding this comment.
Ja, og abtest-pakken burde vel egentlig vært en del av ndla/tracker og ikke en egen pakke? Så burde man vel kanskje gjøre window.ga-funksjonene i Experiment / Variant komponenten?
| @@ -69,5 +70,12 @@ export const routes = [ | |||
| ]; | |||
|
|
|||
| export default function(initialProps = {}, locale) { | |||
| return <App initialProps={initialProps} locale={locale} />; | |||
| return ( | |||
| <ExperimentsContext.Provider | |||
There was a problem hiding this comment.
Bruken av ExperimentsContext er litt forvirrende. Nå brukes samme Context til to foskjellige providers, det virker ikke helt riktig. Ville definert en egen context her og importert den i stedet
| const experimentId = 'OtejUgVLRHmGTAGv7jbFyA'; | ||
|
|
||
| const MastheadMenuModal = ({ t, children }) => ( | ||
| <ExperimentsContext.Consumer> |
| @@ -27,8 +27,17 @@ const Document = ({ helmet, assets, data }) => { | |||
| rel="stylesheet" | |||
| href="https://fonts.googleapis.com/css?family=Source+Sans+Pro:300,400,400i,600,700|Source+Serif+Pro:400,700" | |||
| /> | |||
| {config.gaTrackingId && ( | |||
| <script async src="https://www.google-analytics.com/analytics.js" /> | |||
| {!config.gaTrackingId && ( | |||
No description provided.