Refactor analytics (#443)

**Why**: To make it easier to use, and to allow for further simplification down the road. **How**: - Allow the `track_event` method to take a hash as a second argument, which allows adding various properties, rather than having to use the event title as a differentiator. For example, instead of having two event names to track success and failure scenarios, we only have one event name with different attributes. For example: `track_event(:auth, result: 'success')` and `track_event(:auth, result: 'failed')`. - By default, the `track_event` method assumes the user is the current_user or an instance of AnonymousUser. If you need to pass in a specific user, pass in a hash with the key `user_id` and the user's uuid as the value, such as `track_event(:password_reset, user_id: user_from_token.uuid)`. - Remove the `track_anonymous_event` method since it's no longer needed. - Remove the staccato gem and server-side Google Analytics since it doesn't support hash attributes for events. There are other services that can better suit our needs, such as Keen.io, or even storing the data in our own DB.
18F · Sep 2, 2016 · 3a7d2f9 · 3a7d2f9
1 parent ddd1ee0
commit 3a7d2f9
Show file tree

Hide file tree

Showing 14 changed files with 89 additions and 159 deletions.
diff --git a/Gemfile b/Gemfile
@@ -40,7 +40,6 @@ gem 'simple_form'
 gem 'sinatra', require: false
 gem 'slim-rails'
 gem 'split', require: 'split/dashboard'
-gem 'staccato'
 gem 'twilio-ruby'
 # TODO(amos): Unfork this gem: https://github.com/18F/identity-private/issues/708
 gem 'two_factor_authentication', github: 'amoose/two_factor_authentication',

diff --git a/Gemfile.lock b/Gemfile.lock
@@ -528,7 +528,6 @@ GEM
     sshkit (1.11.2)
       net-scp (>= 1.1.2)
       net-ssh (>= 2.8.0)
-    staccato (0.4.7)
     sysexits (1.2.0)
     systemu (2.6.5)
     temple (0.7.7)
@@ -668,7 +667,6 @@ DEPENDENCIES
   spring
   spring-commands-rspec
   spring-watcher-listen
-  staccato
   test_after_commit
   thin
   timecop

diff --git a/app/controllers/users/confirmations_controller.rb b/app/controllers/users/confirmations_controller.rb
@@ -45,15 +45,14 @@ def set_view_variables
     end
 
     def do_confirm
-      analytics.track_event('Password Created and User Confirmed', @confirmable)
-
       @confirmable.confirm
       @confirmable.update(reset_requested_at: nil)
       sign_in_and_redirect_user
+      analytics.track_event('Password Created and User Confirmed')
     end
 
     def process_user_with_password_errors
-      analytics.track_event('Password Creation: invalid', @confirmable)
+      analytics.track_event('Password Creation: invalid', user_id: @confirmable.uuid)
 
       set_view_variables
       render :show
@@ -69,7 +68,9 @@ def process_user_with_confirmation_errors
     end
 
     def process_already_confirmed_user
-      analytics.track_event('Email Confirmation: User Already Confirmed', @confirmable)
+      analytics.track_event(
+        'Email Confirmation: User Already Confirmed', user_id: @confirmable.uuid
+      )
 
       action_text = 'Please sign in.' unless user_signed_in?
       flash[:error] = t('devise.confirmations.already_confirmed', action: action_text)
@@ -88,14 +89,14 @@ def process_unconfirmed_user
     end
 
     def process_expired_confirmation_token
-      analytics.track_event('Email Confirmation: token expired', @confirmable)
+      analytics.track_event('Email Confirmation: token expired', user_id: @confirmable.uuid)
 
       flash[:error] = resource.decorate.confirmation_period_expired_error
       render :new
     end
 
     def process_valid_confirmation_token
-      analytics.track_event('Email Confirmation: valid token', @confirmable)
+      analytics.track_event('Email Confirmation: valid token', user_id: @confirmable.uuid)
 
       flash.now[:notice] = t('devise.confirmations.confirmed_but_must_set_password')
       render :show
@@ -112,7 +113,7 @@ def after_confirmation_path_for(resource)
     end
 
     def process_confirmed_user
-      analytics.track_event('Email changed and confirmed', @confirmable)
+      analytics.track_event('Email changed and confirmed', user_id: @confirmable.uuid)
       create_user_event(:email_changed, @confirmable)
 
       flash[:notice] = t('devise.confirmations.confirmed')
@@ -135,7 +136,7 @@ def sign_in_and_redirect_user
     def track_invalid_confirmation_token(token)
       token ||= 'nil'
 
-      analytics.track_anonymous_event('Invalid Email Confirmation Token', token)
+      analytics.track_event('Invalid Email Confirmation Token', token: token)
     end
   end
 end
diff --git a/app/controllers/users/passwords_controller.rb b/app/controllers/users/passwords_controller.rb
@@ -9,11 +9,10 @@ def create
 
       analytics_user = User.find_by_email(email) || NonexistentUser.new
       analytics.track_event(
-        "Password Reset Requested for #{analytics_user.role} role", analytics_user
+        'Password Reset Request', user_id: analytics_user.uuid, role: analytics_user.role
       )
 
-      flash[:success] = t('notices.password_reset')
-      redirect_to new_user_session_path
+      redirect_to new_user_session_path, notice: t('notices.password_reset')
     end
 
     def edit
@@ -55,19 +54,19 @@ def handle_invalid_token
     end
 
     def handle_no_user_matches_token
-      analytics.track_anonymous_event(
-        'Reset password: invalid token', params[:reset_password_token]
+      analytics.track_event(
+        'Reset password: invalid token', token: params[:reset_password_token]
       )
       flash[:error] = t('devise.passwords.invalid_token')
     end
 
     def handle_expired_token(user)
-      analytics.track_event('Reset password: token expired', user)
+      analytics.track_event('Reset password: token expired', user_id: user.uuid)
       flash[:error] = t('devise.passwords.token_expired')
     end
 
     def handle_successful_password_reset
-      analytics.track_event('Password reset', resource)
+      analytics.track_event('Password reset', user_id: resource.uuid)
 
       flash[:notice] = t('devise.passwords.updated_not_active') if is_flashing_format?
 
@@ -83,7 +82,7 @@ def handle_unsuccessful_password_reset
         return
       end
 
-      analytics.track_event('Reset password: invalid password', resource)
+      analytics.track_event('Reset password: invalid password', user_id: resource.uuid)
       render :edit
     end
 

diff --git a/app/controllers/users/registrations_controller.rb b/app/controllers/users/registrations_controller.rb
@@ -22,8 +22,8 @@ def create
 
         track_registration(@register_user_email_form)
       else
-        analytics.track_anonymous_event(
-          'User Registration: invalid email', @register_user_email_form.email
+        analytics.track_event(
+          'User Registration: invalid email', email: @register_user_email_form.email
         )
         render :new
       end
@@ -45,10 +45,12 @@ def disable_account_creation
     def track_registration(form)
       if form.email_taken?
         existing_user = User.find_by_email(form.email)
-        analytics.track_event('Registration Attempt with existing email', existing_user)
+        analytics.track_event(
+          'Registration Attempt with existing email', user_id: existing_user.uuid
+        )
       else
         user = form.user
-        analytics.track_event('Account Created', user)
+        analytics.track_event('Account Created', user_id: user.uuid)
         create_user_event(:account_created, user)
       end
     end

diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb
@@ -17,7 +17,7 @@ def active
 
     def timeout
       if sign_out
-        analytics.track_anonymous_event('Session Timed Out')
+        analytics.track_event('Session Timed Out')
         flash[:timeout] = t('session_timedout')
       end
       redirect_to root_url
@@ -38,9 +38,11 @@ def alive?
     def track_authentication_attempt(email)
       existing_user = User.find_by_email(email)
 
-      return analytics.track_event('Authentication Attempt', existing_user) if existing_user
+      if existing_user
+        return analytics.track_event('Authentication Attempt', user_id: existing_user.uuid)
+      end
 
-      analytics.track_anonymous_event('Authentication Attempt with nonexistent user')
+      analytics.track_event('Authentication Attempt with nonexistent user')
     end
   end
 end
diff --git a/app/jobs/analytics_event_job.rb b/app/jobs/analytics_event_job.rb
diff --git a/app/services/analytics.rb b/app/services/analytics.rb
@@ -4,38 +4,18 @@ def initialize(user, request)
     @request = request
   end
 
-  def track_event(event, subject = user)
-    uuid = subject.uuid
+  def track_event(event, attributes = { user_id: uuid })
+    attributes[:user_id] = uuid unless attributes.key?(:user_id)
 
-    AnalyticsEventJob.perform_later(
-      google_analytics_options.merge(action: event, user_id: uuid)
-    )
+    Rails.logger.info("#{event}: #{attributes}")
 
-    Rails.logger.info("#{event} by #{uuid}")
-
-    ahoy.track(event, request_attributes.merge(user_id: uuid))
-  end
-
-  def track_anonymous_event(event, attribute = nil)
-    AnalyticsEventJob.perform_later(
-      google_analytics_options.merge(action: event, value: attribute)
-    )
-
-    Rails.logger.info("#{event}: #{attribute}")
-
-    ahoy.track(event, request_attributes.merge(value: attribute))
+    ahoy.track(event, attributes.merge!(request_attributes))
   end
 
   private
 
   attr_reader :user, :request
 
-  def google_analytics_options
-    @google_analytics_options ||= request_attributes.merge(
-      anonymize_ip: true
-    )
-  end
-
   def request_attributes
     {
       user_ip: request.remote_ip,
@@ -46,4 +26,8 @@ def request_attributes
   def ahoy
     @ahoy ||= Rails.env.test? ? FakeAhoyTracker.new : Ahoy::Tracker.new(request: request)
   end
+
+  def uuid
+    user.uuid
+  end
 end
diff --git a/spec/controllers/users/confirmations_controller_spec.rb b/spec/controllers/users/confirmations_controller_spec.rb
@@ -2,50 +2,45 @@
 
 describe Users::ConfirmationsController, devise: true do
   describe 'Invalid email confirmation tokens' do
-    it 'tracks nil email confirmation token' do
+    before do
       stub_analytics
 
-      expect(@analytics).to receive(:track_anonymous_event).
-        with('Invalid Email Confirmation Token', 'nil')
+      expect(@analytics).to receive(:track_event).with('GET request for confirmations#show')
+    end
+
+    it 'tracks nil email confirmation token' do
+      expect(@analytics).to receive(:track_event).
+        with('Invalid Email Confirmation Token', token: 'nil')
 
       get :show, confirmation_token: nil
     end
 
     it 'tracks blank email confirmation token' do
-      stub_analytics
-
-      expect(@analytics).to receive(:track_anonymous_event).
-        with('Invalid Email Confirmation Token', '')
+      expect(@analytics).to receive(:track_event).
+        with('Invalid Email Confirmation Token', token: '')
 
       get :show, confirmation_token: ''
     end
 
     it 'tracks confirmation token as a single-quoted empty string' do
-      stub_analytics
-
-      expect(@analytics).to receive(:track_anonymous_event).
-        with('Invalid Email Confirmation Token', "''")
+      expect(@analytics).to receive(:track_event).
+        with('Invalid Email Confirmation Token', token: "''")
 
       get :show, confirmation_token: "''"
     end
 
     it 'tracks confirmation token as a double-quoted empty string' do
-      stub_analytics
-
-      expect(@analytics).to receive(:track_anonymous_event).
-        with('Invalid Email Confirmation Token', '""')
+      expect(@analytics).to receive(:track_event).
+        with('Invalid Email Confirmation Token', token: '""')
 
       get :show, confirmation_token: '""'
     end
 
     it 'tracks already confirmed token' do
       user = create(:user, confirmation_token: 'foo')
 
-      stub_analytics
-
-      expect(@analytics).to receive(:track_event).with('GET request for confirmations#show')
       expect(@analytics).to receive(:track_event).
-        with('Email Confirmation: User Already Confirmed', user)
+        with('Email Confirmation: User Already Confirmed', user_id: user.uuid)
 
       get :show, confirmation_token: 'foo'
     end
@@ -54,11 +49,8 @@
       user = create(:user, :unconfirmed)
       user.update(confirmation_token: 'foo', confirmation_sent_at: Time.current - 2.days)
 
-      stub_analytics
-
-      expect(@analytics).to receive(:track_event).with('GET request for confirmations#show')
       expect(@analytics).to receive(:track_event).
-        with('Email Confirmation: token expired', user)
+        with('Email Confirmation: token expired', user_id: user.uuid)
 
       get :show, confirmation_token: 'foo'
     end
@@ -73,7 +65,7 @@
 
       expect(@analytics).to receive(:track_event).with('GET request for confirmations#show')
       expect(@analytics).to receive(:track_event).
-        with('Email Confirmation: valid token', user)
+        with('Email Confirmation: valid token', user_id: user.uuid)
 
       get :show, confirmation_token: 'foo'
     end
@@ -87,7 +79,7 @@
       stub_analytics
 
       expect(@analytics).to receive(:track_event).
-        with('Password Created and User Confirmed', user)
+        with('Password Created and User Confirmed')
 
       patch :confirm, password_form: { password: 'NewVal!dPassw0rd' }, confirmation_token: 'foo'
     end
@@ -99,7 +91,7 @@
       stub_analytics
 
       expect(@analytics).to receive(:track_event).
-        with('Password Creation: invalid', user)
+        with('Password Creation: invalid', user_id: user.uuid)
 
       patch :confirm, password_form: { password: 'NewVal' }, confirmation_token: 'foo'
     end
@@ -118,7 +110,7 @@
 
       expect(@analytics).to receive(:track_event).with('GET request for confirmations#show')
       expect(@analytics).to receive(:track_event).
-        with('Email changed and confirmed', user)
+        with('Email changed and confirmed', user_id: user.uuid)
 
       get :show, confirmation_token: 'foo'
     end