Skip to content

Conversation

@adlius
Copy link
Contributor

@adlius adlius commented Mar 21, 2019

Ticket

https://openscience.atlassian.net/browse/ENG-44

Purpose

To add GA tracking to MFR to track when users click open the Hypothesis panel.

Changes

  • In mfr/extensions/pdf/templates/viewer.mako, we add GA tracking imports.
  • In mfr/server/static/js/mfr.child.hypothesis.js, we set window.hypothesisConfig to use the onLayoutChange() callback provided by hypothesis to watch for opening of annotation panel.
  • Add GA tracking ID to settings and use that in renderer.

Side effects

None

QA Notes

To test this, you'll need access to Google Analytics panel for staging server.
Check to make sure the event is only tracked when users open the panel for the first time on that page. Page refresh counts as a new page. User is counted by browser, not OSF User. So if the same OSFUser opens two browser and opens the panel in both, it counts as two events.

Deployment Notes

Add GOOGLE_ANALYTICS_TRACKING_ID for to staging/prod settings.

@coveralls
Copy link

coveralls commented Mar 21, 2019

Coverage Status

Coverage increased (+0.03%) to 71.145% when pulling 3df7593 on adlius:feature/google-analytics into 1c9b11d on CenterForOpenScience:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 71.145% when pulling a539d45 on adlius:feature/google-analytics into 1c9b11d on CenterForOpenScience:develop.

Copy link
Contributor

@cslzchen cslzchen left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor style suggestion and two general questions @adlius Thanks!

Pass this PR on to @felliott for final review (I haven't tested the code locally.)


var sidePanelOpened = false;
var sendAnalyticsIfExpanded = function (expanded) {
if (expanded && !sidePanelOpened) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a dumb question about hypothesis configuration. What is expanded (or layout.expanded a few lines below more specifically)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

layout.expanded denotes whether the hypothesis side panel is expanded when onLayoutChanged() is called.



SENTRY_DSN = config.get_nullable('SENTRY_DSN', None)
GOOGLE_ANALYTICS_TRACKING_ID = config.get_nullable('GOOGLE_ANALYTICS_TRACKING_ID', None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can GA script handle None tracking ID? I guess so but just want to double check with you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure. It'll probably throw an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cslzchen Confirmed that it would not cause an error when tracking ID is None.

Copy link
Contributor

@cslzchen cslzchen left a comment

Choose a reason for hiding this comment

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

LG and RTM

ga('send', 'event', {
eventCategory: 'Hypothesis',
eventAction: 'Open Hypothesis Panel',
eventLabel: window.DEFAULT_URL.split('/')[5],
Copy link
Member

Choose a reason for hiding this comment

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

What is this value? Could you add a comment saying what this is expected to be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eventLabel should be the preprint guid.

Copy link
Contributor

Choose a reason for hiding this comment

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

A follow-up question, is it guaranteed that the GUID is always at index 5? Can we add some extra check before ga to parse window.DEFAULT_URL.split('/')[5] and handle unexpected URL format. In addition, having a comment on what the expected URL looks like will be very helpful as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no guarantee that window.DEFAULT_URL.split('/')[5] always return the preprint guid. However, if the rendered file is a preprint's primary file, window.DEFAULT_URL should be the WB link to the file on OSFStorage (e.g. https://files.us.staging2.osf.io/v1/resources/twq6d/providers/osfstorage/5c9cd1e3c8f5970017ed07d3?direct=&mode=render), and from which we can extract the guid of the preprint.
And since hypothesis is only enabled on a preprint's primary file, it should be fine.
I'll add some checks for unexpected URL and log to sentry when that happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! Thanks for the explanation. This WB API rarely changes. If it ever changed, we would be informed right away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cslzchen PR updated.

@adlius
Copy link
Contributor Author

adlius commented Mar 28, 2019

@felliott Comment is added.

@adlius adlius force-pushed the feature/google-analytics branch from 56fb047 to f72d3fa Compare March 28, 2019 15:49
More specifically, this GA event tracks when users click open the
Hypothesis side panel for each preprints.
@cslzchen cslzchen force-pushed the feature/google-analytics branch from f72d3fa to 111430d Compare March 29, 2019 12:02
The preprints GUID used by the GA tracking event is retreived from
window.DEFAULT_URL, which is the third segment of the URL path.
@cslzchen cslzchen force-pushed the feature/google-analytics branch from 111430d to 3df7593 Compare March 29, 2019 12:17
var sidePanelOpened = false;
// window.DEFAULT_URL should be the wb link in this format:
// https://<wb-domain>/v1/resources/<preprint-guid>/providers/osfstorage/<file-id>?direct=&mode=render
// TODO: parse and validate the WB URL before retrieving the preprints GUID
Copy link
Contributor

Choose a reason for hiding this comment

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

I added a TODO comment to your code. Although the split you use here is unaware of the HTTP protocol, URL domain and path, I think for now it is good enough. A better way (probably a future improvement) is to fully parse and validate the URL.

@cslzchen cslzchen merged commit 3df7593 into CenterForOpenScience:develop Mar 29, 2019
cslzchen added a commit that referenced this pull request Mar 29, 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.

5 participants