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

Log video hint clicks to firehose #19854

Merged
merged 6 commits into from
Jan 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/src/StudioApp.js
Original file line number Diff line number Diff line change
Expand Up @@ -2922,6 +2922,7 @@ StudioApp.prototype.setPageConstants = function (config, appSpecificConstants) {
inputOutputTable: config.level.inputOutputTable,
is13Plus: config.is13Plus,
isSignedIn: config.isSignedIn,
userId: config.userId,
textToSpeechEnabled: config.textToSpeechEnabled,
isK1: config.level.isK1,
appType: config.app,
Expand Down
3 changes: 2 additions & 1 deletion apps/src/code-studio/progress.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ progress.renderStageProgress = function (scriptData, stageData, progressData,
name,
stages: [stageData],
disablePostMilestone,
age_13_required
age_13_required,
id: stageData.script_id,
}, currentLevelId, false, saveAnswersBeforeNavigation);

store.dispatch(mergeProgress(_.mapValues(progressData.levels,
Expand Down
34 changes: 27 additions & 7 deletions apps/src/lib/util/firehose.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import AWS from 'aws-sdk';
import {createUuid, trySetLocalStorage, tryGetLocalStorage} from '@cdo/apps/utils';
import {getStore} from '@cdo/apps/redux';

/**
* A barebones client for posting data to an AWS Firehose stream.
Expand Down Expand Up @@ -142,14 +143,30 @@ class FirehoseClient {
/**
* Merge various key-value pairs into data.
* @param {hash} data The data to add the key-value pairs to.
* @option {boolean} includeUserId Include userId in records, if signed in
* @return {hash} The data, including the newly added key-value pairs.
*/
addCommonValues(data) {
addCommonValues(data, includeUserId) {
data['created_at'] = new Date().toISOString();
data['environment'] = this.getEnvironment();
data['uuid'] = this.getAnalyticsUuid();
data['device'] = JSON.stringify(this.getDeviceInfo());

const state = getStore().getState();
if (state) {
if (includeUserId) {
const constants = state.pageConstants;
if (constants) {
data['user_id'] = constants.userId;
}
}
const progress = state.progress;
if (progress) {
data['script_id'] = progress.scriptId;
data['level_id'] = parseInt(progress.currentLevelId);
}
}

return data;
}

Expand All @@ -159,10 +176,12 @@ class FirehoseClient {
* @param {hash} data The data to push.
* @param {hash} options Additional (optional) options.
* (default {alwaysPut: false})
* @option options [String] alwaysPut Forces the record to be sent.
* @option options [boolean] alwaysPut Forces the record to be sent.
* @option options [boolean] includeUserId Include userId in records, if signed in
* @option options [function(err, data)] callback Invoked upon completion with error or data
*/
putRecord(deliveryStreamName, data, options = {alwaysPut: false, callback: null}) {
data = this.addCommonValues(data);
putRecord(deliveryStreamName, data, options = {alwaysPut: false, includeUserId: false, callback: null}) {
data = this.addCommonValues(data, options.includeUserId);
if (!this.shouldPutRecord(options['alwaysPut'])) {
console.groupCollapsed("Skipped sending record to " + deliveryStreamName);
console.log(data);
Expand Down Expand Up @@ -194,10 +213,11 @@ class FirehoseClient {
* @param {array[hash]} data The data to push.
* @param {hash} options Additional (optional) options.
* (default {alwaysPut: false})
* @option options [String] alwaysPut Forces the record to be sent.
* @option options [boolean] alwaysPut Forces the record to be sent.
* @option options [boolean] includeUserId Include userId in records, if signed in
*/
putRecordBatch(deliveryStreamName, data, options = {alwaysPut: false}) {
data.map(function (record) { return this.AddCommonValues(record); });
putRecordBatch(deliveryStreamName, data, options = {alwaysPut: false, includeUserId: false}) {
data.map(function (record) { return this.AddCommonValues(record, options.includeUserId); });

if (!this.shouldPutRecord(options['alwaysPut'])) {
console.groupCollapsed("Skipped sending record batch to " + deliveryStreamName);
Expand Down
1 change: 1 addition & 0 deletions apps/src/redux/pageConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ var ALLOWED_KEYS = new Set([
'nonResponsiveVisualizationColumnWidth',
'is13Plus',
'isSignedIn',
'userId',
'isK1',
'textToSpeechEnabled',
'documentationUrl',
Expand Down
4 changes: 3 additions & 1 deletion apps/src/templates/VideoThumbnail.jsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {showVideoDialog} from "@cdo/apps/code-studio/videos";
import React, {PropTypes, Component} from 'react';
import React, {Component, PropTypes} from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

😄

import {videoDataShape} from './types';
import firehoseClient from '@cdo/apps/lib/util/firehose';

Expand All @@ -24,6 +24,7 @@ export default class VideoThumbnail extends Component {
static propTypes = {
logText: PropTypes.string,
video: videoDataShape,
onClick: PropTypes.func,
};

render() {
Expand All @@ -32,6 +33,7 @@ export default class VideoThumbnail extends Component {
<a
style={styles.videoLink}
onClick={() => {
this.props.onClick && this.props.onClick();
showVideoDialog({
src: video.src,
name: video.name,
Expand Down
24 changes: 21 additions & 3 deletions apps/src/templates/instructions/InlineHint.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import ReactDOM from 'react-dom';
import Radium from 'radium';
import ReadOnlyBlockSpace from '../ReadOnlyBlockSpace';
import ChatBubble from './ChatBubble';
import firehoseClient from '@cdo/apps/lib/util/firehose';
import { connect } from 'react-redux';
import { convertXmlToBlockly } from './utils';
import VideoThumbnail from '../VideoThumbnail';
Expand All @@ -18,7 +19,7 @@ const InlineHint = React.createClass({
video: videoDataShape,
ttsUrl: PropTypes.string,
ttsMessage: PropTypes.string,
isBlockly: PropTypes.bool
isBlockly: PropTypes.bool,
},

componentDidMount() {
Expand All @@ -27,14 +28,31 @@ const InlineHint = React.createClass({
}
},

onVideoClick() {
firehoseClient.putRecord(
'analysis-events',
{
study: 'hint-videos',
event: 'click',
data_string: this.props.video.key,
},
{ includeUserId: true }
);
},

render() {
return (
<ChatBubble borderColor={this.props.borderColor} ttsUrl={this.props.ttsUrl} ttsMessage={this.props.ttsMessage}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also want to track that the user asked to see the hint? I think this data is already in the DB, but maybe easier to correlate with hint video clicks if we log it to Firehose as well. @bencodeorg any preference?

<div
dangerouslySetInnerHTML={{ __html: this.props.content }}
/>
{this.props.block && <ReadOnlyBlockSpace block={this.props.block} />}
{this.props.video && <VideoThumbnail video={this.props.video} />}
{this.props.video &&
<VideoThumbnail
onClick={this.onVideoClick}
video={this.props.video}
/>
}
</ChatBubble>
);
}
Expand All @@ -43,5 +61,5 @@ const InlineHint = React.createClass({

export const StatelessInlineHint = Radium(InlineHint);
export default connect(state => ({
isBlockly: state.pageConstants.isBlockly
isBlockly: state.pageConstants.isBlockly,
}))(Radium(InlineHint));
2 changes: 2 additions & 0 deletions dashboard/app/helpers/levels_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ def app_options
# Always pass user age limit
view_options(is_13_plus: current_user && !current_user.under_13?)

view_options(user_id: current_user.id) if current_user

view_options(server_level_id: @level.id)
if @script_level
view_options(
Expand Down
1 change: 1 addition & 0 deletions dashboard/app/helpers/view_options_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ module ViewOptionsHelper
:level_position,
:public_caching,
:is_13_plus,
:user_id,
:has_contained_levels,
:next_level_url,
:responsive_content
Expand Down