clean up error report error handling

test plan:
 * try to abuse the javascript record_error endpoint (send garbage
   data for the id, try to do other stuff to make it fail)
 * it should record ever so slightly more useful information

Change-Id: Ibe83fdab3a77175ef1ab73b6c5210ac7265644ae
Reviewed-on: https://gerrit.instructure.com/19041
Reviewed-by: Jacob Fugal <jacob@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Brian Palmer <brianp@instructure.com>
QA-Review: Clare Hetherington <clare@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>

Author: ccutrer

app/controllers/info_controller.rb

@@ -39,9 +39,9 @@ def record_error
     error[:user_agent] = request.headers['User-Agent']
     begin
       report_id = error.delete(:id)
-      @report = ErrorReport.find_by_id(report_id) if report_id.present?
+      @report = ErrorReport.find_by_id(report_id.to_i) if report_id.present? && report_id.to_i != 0
       @report ||= ErrorReport.find_by_id(session.delete(:last_error_id)) if session[:last_error_id].present?
-      @report ||= ErrorReport.create()
+      @report ||= ErrorReport.new
       error.delete(:category) if @report.category.present?
       @report.user = @current_user
       @report.account ||= @domain_root_account
@@ -59,12 +59,13 @@ def record_error
       ErrorReport.log_exception(:default, e,
         :message => "Error Report Creation failed",
         :user_email => (error[:email] rescue ''),
-        :user_id => (error[:user].id rescue ''))
+        :user_id => @current_user.try(:id)
+      )
     end
     respond_to do |format|
       flash[:notice] = t('notices.error_reported', "Thanks for your help!  We'll get right on this")
       format.html { redirect_to root_url }
-      format.json { render :json => {:logged => true, :id => @report.id}.to_json }
+      format.json { render :json => {:logged => true, :id => @report.try(:id) }.to_json }
     end
   end
 

spec/controllers/info_controller_spec.rb

@@ -29,6 +29,31 @@
       assert_recorded_error
     end
 
+    it "should not choke on non-integer ids" do
+      post 'record_error', :error => {:id => 'garbage'}
+      assert_recorded_error
+      ErrorReport.last.message.should_not == "Error Report Creation failed"
+    end
+
+    it "should not return nil.id if report creation failed" do
+      ErrorReport.expects(:find_by_id).once.raises("failed!")
+      post 'record_error', :format => 'json', :error => {:id => 1}
+      JSON.parse(response.body).should == { 'logged' => true, 'id' => nil }
+    end
+
+    it "should not record the user as nil.id if report creation failed" do
+      ErrorReport.expects(:find_by_id).once.raises("failed!")
+      post 'record_error', :error => {:id => 1}
+      ErrorReport.last.user_id.should be_nil
+    end
+
+    it "should record the user if report creation failed" do
+      user = User.create!
+      user_session(user)
+      ErrorReport.expects(:find_by_id).once.raises("failed!")
+      post 'record_error', :error => {:id => 1}
+      ErrorReport.last.user_id.should == user.id
+    end
   end
 
   def assert_recorded_error(msg = "Thanks for your help!  We'll get right on this")