diff --git a/apps/src/sites/studio/pages/signup.js b/apps/src/sites/studio/pages/signup.js index 905eb93115fce..fcef007a3e8cc 100644 --- a/apps/src/sites/studio/pages/signup.js +++ b/apps/src/sites/studio/pages/signup.js @@ -6,6 +6,7 @@ import CountryAutocompleteDropdown from '@cdo/apps/templates/CountryAutocomplete import { COUNTRIES } from '@cdo/apps/geographyConstants'; import SchoolNotFound from '@cdo/apps/templates/SchoolNotFound'; import i18n from "@cdo/locale"; +import firehoseClient from "../../../lib/util/firehose"; const SCHOOL_TYPES_HAVING_NCES_SEARCH = ['charter', 'private', 'public']; @@ -139,6 +140,7 @@ window.SignupManager = function (options) { $("#user_user_type").change(function () { var value = $(this).val(); + trackUserTypeSelected(value); if (value === "student") { showStudent(); } else if (value === "teacher") { @@ -146,6 +148,16 @@ window.SignupManager = function (options) { } }); + function trackUserTypeSelected(type) { + const eventName = "select-" + type; + firehoseClient.putRecord({ + study: 'account-sign-up', + study_group: 'control', + event: eventName, + data_string: self.options.signUpUID, + }); + } + function setSchoolInfoVisibility(state) { if (state) { $("#schooldropdown-block").fadeIn(); diff --git a/dashboard/app/controllers/application_controller.rb b/dashboard/app/controllers/application_controller.rb index f4453dc56063d..b8018b7c68a34 100644 --- a/dashboard/app/controllers/application_controller.rb +++ b/dashboard/app/controllers/application_controller.rb @@ -24,6 +24,8 @@ class ApplicationController < ActionController::Base before_action :fix_crawlers_with_bad_accept_headers + before_action :clear_sign_up_session_vars + def fix_crawlers_with_bad_accept_headers # append text/html as an acceptable response type for Edmodo and weebly-agent's malformed HTTP_ACCEPT header. if request.formats.include?("image/*") && @@ -271,4 +273,12 @@ def pairing_user_ids # TODO(asher): Determine whether we need to guard against it being nil. session[:pairings].nil? ? [] : session[:pairings] end + + def clear_sign_up_session_vars + if session[:sign_up_uid] || session[:sign_up_type] || session[:sign_up_tracking_expiration] + session.delete(:sign_up_uid) + session.delete(:sign_up_type) + session.delete(:sign_up_tracking_expiration) + end + end end diff --git a/dashboard/app/controllers/home_controller.rb b/dashboard/app/controllers/home_controller.rb index 26bffce9a4dd0..b0f4e0ab17504 100644 --- a/dashboard/app/controllers/home_controller.rb +++ b/dashboard/app/controllers/home_controller.rb @@ -8,6 +8,10 @@ class HomeController < ApplicationController # clicking on a link. skip_before_action :verify_authenticity_token, only: 'set_locale' + # The terms_and_privacy page gets loaded in an iframe on the signup page, so skip + # clearing the sign up tracking variables + skip_before_action :clear_sign_up_session_vars, only: [:terms_and_privacy] + def set_locale set_locale_cookie(params[:locale]) if params[:locale] if params[:i18npath] diff --git a/dashboard/app/controllers/omniauth_callbacks_controller.rb b/dashboard/app/controllers/omniauth_callbacks_controller.rb index 0eaf11ae505c2..8ec8345ce4d8f 100644 --- a/dashboard/app/controllers/omniauth_callbacks_controller.rb +++ b/dashboard/app/controllers/omniauth_callbacks_controller.rb @@ -4,6 +4,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController include UsersHelper + skip_before_action :clear_sign_up_session_vars + # GET /users/auth/:provider/callback def all if should_connect_provider? @@ -59,6 +61,7 @@ def login auth_hash = request.env['omniauth.auth'] auth_params = request.env['omniauth.params'] provider = auth_hash.provider.to_s + session[:sign_up_type] = provider # Fiddle with data if it's a Powerschool request (other OpenID 2.0 providers might need similar treatment if we add any) if provider == 'powerschool' @@ -70,7 +73,7 @@ def login auth_hash = extract_microsoft_data(request.env["omniauth.auth"]) end - @user = User.from_omniauth(auth_hash, auth_params) + @user = User.from_omniauth(auth_hash, auth_params, session) # Set user-account locale only if no cookie is already set. if @user.locale && @@ -117,6 +120,11 @@ def self.get_cache_key(oauth_param, user) def register_new_user(user) move_oauth_params_to_cache(user) session["devise.user_attributes"] = user.attributes + + # For some providers, signups can happen without ever having hit the sign_up page, where + # our tracking data is usually populated, so do it here + SignUpTracking.begin_sign_up_tracking(session) + redirect_to new_user_registration_url end @@ -281,6 +289,10 @@ def silent_takeover(oauth_user, auth_hash) def sign_in_user flash.notice = I18n.t('auth.signed_in') + + # Will only log if the sign_up page session cookie is set, so this is safe to call in all cases + SignUpTracking.log_sign_in(resource, session, request) + sign_in_and_redirect @user end diff --git a/dashboard/app/controllers/registrations_controller.rb b/dashboard/app/controllers/registrations_controller.rb index 9dd020971c0a4..02a414b879816 100644 --- a/dashboard/app/controllers/registrations_controller.rb +++ b/dashboard/app/controllers/registrations_controller.rb @@ -7,10 +7,19 @@ class RegistrationsController < Devise::RegistrationsController :migrate_to_multi_auth, :demigrate_from_multi_auth ] skip_before_action :verify_authenticity_token, only: [:set_age] + skip_before_action :clear_sign_up_session_vars, only: [:new, :create] def new session[:user_return_to] ||= params[:user_return_to] @already_hoc_registered = params[:already_hoc_registered] + + SignUpTracking.begin_sign_up_tracking(session) + FirehoseClient.instance.put_record( + study: 'account-sign-up', + event: 'load-sign-up-page', + data_string: session[:sign_up_uid] + ) + super end @@ -51,6 +60,22 @@ def create storage_id = take_storage_id_ownership_from_cookie(current_user.id) current_user.generate_progress_from_storage_id(storage_id) if storage_id end + + sign_up_type = session[:sign_up_type] + sign_up_type ||= resource.email ? 'email' : 'other' + if session[:sign_up_tracking_expiration]&.future? + result = resource.persisted? ? 'success' : 'error' + tracking_data = { + study: 'account-sign-up', + event: "#{sign_up_type}-sign-up-#{result}", + data_string: session[:sign_up_uid], + data_json: { + detail: resource.to_json, + errors: resource.errors&.full_messages + }.to_json + } + FirehoseClient.instance.put_record(tracking_data) + end end # diff --git a/dashboard/app/models/user.rb b/dashboard/app/models/user.rb index 42dc4a40e9256..5d678144637bd 100644 --- a/dashboard/app/models/user.rb +++ b/dashboard/app/models/user.rb @@ -75,6 +75,7 @@ require 'cdo/race_interstitial_helper' require 'cdo/shared_cache' require 'school_info_interstitial_helper' +require 'sign_up_tracking' class User < ActiveRecord::Base include SerializedProperties @@ -707,10 +708,14 @@ def self.name_from_omniauth(raw_name) end CLEVER_ADMIN_USER_TYPES = ['district_admin', 'school_admin'].freeze - def self.from_omniauth(auth, params) + def self.from_omniauth(auth, params, session = nil) omniauth_user = find_by_credential(type: auth.provider, id: auth.uid) - omniauth_user ||= create do |user| - initialize_new_oauth_user(user, auth, params) + + unless omniauth_user + omniauth_user = create do |user| + initialize_new_oauth_user(user, auth, params) + end + SignUpTracking.log_sign_up_result(omniauth_user, session) end omniauth_user.update_oauth_credential_tokens(auth) diff --git a/dashboard/app/views/devise/registrations/new.html.haml b/dashboard/app/views/devise/registrations/new.html.haml index b8ff5cd0cbdef..b7887fcd2a752 100644 --- a/dashboard/app/views/devise/registrations/new.html.haml +++ b/dashboard/app/views/devise/registrations/new.html.haml @@ -182,6 +182,9 @@ $(document).ready(function() { // Send through some values that the JavaScript will need. window.signupManager = new SignupManager({ + signUpType: "#{session[:sign_up_type]}", + signUpUID: "#{session[:sign_up_uid]}", + signUpTrackingExpiration: "#{session[:sign_up_tracking_expiration]}", isTeacher: "#{resource.teacher?}", returnToUrl: "#{user_return_to_url}", teacherDashboardUrl: "#{teacher_dashboard_url}", diff --git a/dashboard/lib/sign_up_tracking.rb b/dashboard/lib/sign_up_tracking.rb new file mode 100644 index 0000000000000..016f9aa5e2c67 --- /dev/null +++ b/dashboard/lib/sign_up_tracking.rb @@ -0,0 +1,42 @@ +require 'cdo/firehose' + +module SignUpTracking + def self.begin_sign_up_tracking(session) + unless session[:sign_up_tracking_expiration]&.future? + session[:sign_up_uid] = SecureRandom.uuid.to_s + session[:sign_up_tracking_expiration] = 5.minutes.from_now + end + end + + def self.log_sign_in(user, session, request) + return unless user && session && request + provider = request.env['omniauth.auth'].provider.to_s + if session[:sign_up_tracking_expiration]&.future? + tracking_data = { + study: 'account-sign-up', + event: "#{provider}-sign-in", + data_string: session[:sign_up_uid] + } + FirehoseClient.instance.put_record(tracking_data) + end + end + + def self.log_sign_up_result(user, session) + return unless user && session + sign_up_type = session[:sign_up_type] + sign_up_type ||= user.email ? 'email' : 'other' + if session[:sign_up_tracking_expiration]&.future? + result = user.persisted? ? 'success' : 'error' + tracking_data = { + study: 'account-sign-up', + event: "#{sign_up_type}-sign-up-#{result}", + data_string: session[:sign_up_uid], + data_json: { + detail: user.to_json, + errors: user.errors&.messages + }.to_json + } + FirehoseClient.instance.put_record(tracking_data) + end + end +end diff --git a/dashboard/test/controllers/registrations_controller/new_account_tracking_test.rb b/dashboard/test/controllers/registrations_controller/new_account_tracking_test.rb new file mode 100644 index 0000000000000..867a8f5f9783e --- /dev/null +++ b/dashboard/test/controllers/registrations_controller/new_account_tracking_test.rb @@ -0,0 +1,158 @@ +require 'test_helper' + +module RegistrationsControllerTests + class NewAccountTrackingTest < ActionDispatch::IntegrationTest + EMAIL = 'upgraded@code.org' + PASSWORD = '1234567' + DEFAULT_UID = '1111' # sso uid + UUID = 'abcdefg1234567' # tracking uuid + STUDY = 'account-sign-up' + + USER_PARAMS_GOOD = { + name: 'A name', + password: PASSWORD, + password_confirmation: PASSWORD, + email: EMAIL, + hashed_email: User.hash_email(EMAIL), + gender: 'F', + age: '18', + user_type: 'student', + terms_of_service_version: 1 + } + + USER_PARAMS_ERROR = { + name: 'A name', + password: PASSWORD, + password_confirmation: PASSWORD + "oops!", + email: EMAIL, + hashed_email: User.hash_email(EMAIL), + gender: 'F', + age: '18', + user_type: 'student', + terms_of_service_version: 1 + } + + setup do + SecureRandom.stubs(:uuid).returns(UUID) + OmniAuth.config.test_mode = true + OmniAuth.config.mock_auth[:google_oauth2] = generate_auth_user_hash(provider: 'google_oauth2') + end + + test 'loading sign up page sends Firehose page load event' do + FirehoseClient.instance.expects(:put_record).once.with do |data| + data[:study] == STUDY && + data[:event] == 'load-sign-up-page' && + data[:data_string] == UUID + end + get '/users/sign_up' + end + + test 'successful email sign up sends Firehose success event' do + events = %w(load-sign-up-page email-sign-up-success) + FirehoseClient.instance.expects(:put_record).times(2).with do |data| + data[:study] == STUDY && + data[:event] == events.shift && + data[:data_string] == UUID + end + + get '/users/sign_up' + + assert_creates(User) do + post '/users.json', params: { + user: USER_PARAMS_GOOD + } + end + end + + test 'email sign up with wrong password confirmation sends Firehose error event' do + events = %w(load-sign-up-page email-sign-up-error) + FirehoseClient.instance.expects(:put_record).times(2).with do |data| + data[:study] == STUDY && + data[:event] == events.shift && + data[:data_string] == UUID + end + + get '/users/sign_up' + + assert_does_not_create(User) do + post '/users.json', params: { + user: USER_PARAMS_ERROR + } + end + end + + test 'successful oauth sign up after hitting sign up page sends Firehose success events' do + events = %w(load-sign-up-page google_oauth2-sign-up-success) + FirehoseClient.instance.expects(:put_record).times(2).with do |data| + data[:study] == STUDY && + data[:event] == events.shift && + data[:data_string] == UUID + end + + get '/users/sign_up' + + @request.env["devise.mapping"] = Devise.mappings[:user] + assert_creates(User) do + get '/users/auth/google_oauth2' + follow_redirect! + end + end + + test 'login to existing account after hitting sign up page sends Firehose sign in event' do + create :teacher, :unmigrated_google_sso, uid: DEFAULT_UID, email: EMAIL + events = %w(load-sign-up-page google_oauth2-sign-in) + FirehoseClient.instance.expects(:put_record).times(2).with do |data| + data[:study] == STUDY && + data[:event] == events.shift && + data[:data_string] == UUID + end + + get '/users/sign_up' + + assert_does_not_create(User) do + get '/users/auth/google_oauth2' + follow_redirect! + end + end + + test 'tracking cookie is cleared when hitting another random page on the site' do + create :teacher, :unmigrated_google_sso, uid: DEFAULT_UID, email: EMAIL + events = %w(load-sign-up-page) + FirehoseClient.instance.expects(:put_record).once.with do |data| + data[:study] == STUDY && + data[:event] == events.shift && + data[:data_string] == UUID + end + + get '/users/sign_up' + get '/courses' # should clear tracking variable, events should not be sent afterwards + + @request.env["devise.mapping"] = Devise.mappings[:user] + assert_does_not_create(User) do + get '/users/auth/google_oauth2' + follow_redirect! + end + end + + private + + def generate_auth_user_hash(args) + OmniAuth::AuthHash.new( + uid: args[:uid] || DEFAULT_UID, + provider: args[:provider] || 'facebook', + info: { + name: args[:name] || 'someone', + email: args[:email] || EMAIL, + user_type: args[:user_type] || 'teacher', + dob: args[:dob] || Date.today - 20.years, + gender: args[:gender] || 'f' + }, + credentials: { + token: args[:token] || '12345', + expires_at: args[:expires_at] || 'some-future-time', + refresh_token: args[:refresh_token] || nil + } + ) + end + end +end diff --git a/lib/cdo/firehose.rb b/lib/cdo/firehose.rb index 7085da7aa3432..3d4c00d643752 100644 --- a/lib/cdo/firehose.rb +++ b/lib/cdo/firehose.rb @@ -74,8 +74,7 @@ def add_common_values(data) environment: rack_env, device: 'server-side'.to_json ) - data_with_common_values[user_id] = current_user.id if current_user - + data_with_common_values[user_id] ||= current_user.id if current_user data_with_common_values end end