Merge pull request #24817 from code-dot-org/move-hb-logging-to-redshift

Move failed account takeover logging to redshift, include error
code-dot-org · Sep 17, 2018 · c0173ca · c0173ca
2 parents 632b843 + 2f379b9
commit c0173ca
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 14 deletions.
diff --git a/dashboard/app/helpers/users_helper.rb b/dashboard/app/helpers/users_helper.rb
@@ -19,13 +19,18 @@ def move_sections_and_destroy_source_user(source_user:, destination_user:, takeo
     # No-op if source_user is nil
     return true unless source_user.present?
 
+    firehose_params = {
+      source_user: source_user,
+      destination_user: destination_user,
+      type: takeover_type,
+      provider: destination_user.provider,
+    }
+
     if source_user.has_activity?
-      # We don't want to destroy an account with progress.
-      # In theory this should not happen, so we log a Honeybadger error and return.
-      Honeybadger.notify(
-        error_class: 'Oauth takeover called for user with progress',
-        errors_message: "Attempted takeover for account with progress. Cancelling takeover of account with id #{source_user.id} by id #{destination_user.id}"
-      )
+      # We don't want to destroy an account with progress. Log to Redshift and return false.
+      firehose_params[:type] = "cancelled-#{takeover_type}"
+      firehose_params[:error] = "Attempted takeover for account with progress."
+      log_account_takeover_to_firehose(firehose_params)
       return false
     end
 
@@ -40,12 +45,7 @@ def move_sections_and_destroy_source_user(source_user:, destination_user:, takeo
       source_user.destroy!
     end
 
-    log_account_takeover_to_firehose(
-      source_user: source_user,
-      destination_user: destination_user,
-      type: takeover_type,
-      provider: destination_user.provider
-    )
+    log_account_takeover_to_firehose(firehose_params)
     true
   rescue
     false
@@ -94,7 +94,7 @@ def check_and_apply_oauth_takeover(user)
     end
   end
 
-  def log_account_takeover_to_firehose(source_user:, destination_user:, type:, provider:)
+  def log_account_takeover_to_firehose(source_user:, destination_user:, type:, provider:, error: nil)
     FirehoseClient.instance.put_record(
       study: 'user-soft-delete-audit',
       event: "#{type}-account-takeover", # Silent or OAuth takeover
@@ -103,6 +103,7 @@ def log_account_takeover_to_firehose(source_user:, destination_user:, type:, pro
       data_string: provider, # OAuth provider
       data_json: {
         user_type: destination_user.user_type,
+        error: error,
       }.to_json
     )
   end

diff --git a/dashboard/test/controllers/omniauth_callbacks_controller_test.rb b/dashboard/test/controllers/omniauth_callbacks_controller_test.rb
@@ -449,7 +449,7 @@ class OmniauthCallbacksControllerTest < ActionController::TestCase
 
       assert oauth_student.has_activity?
 
-      Honeybadger.expects(:notify).at_least_once
+      FirehoseClient.any_instance.expects(:put_record).at_least_once
 
       set_oauth_takeover_session_variables(provider, oauth_student)
       check_and_apply_oauth_takeover(student)