Merge pull request #254 from concordia-publishing-house/simplify-err

Suggestion: Err should validate 'problem_id' and 'fingerprint'

Merge pull request #254 from concordia-publishing-house/simplify-err
Suggestion: Err should validate 'problem_id' and 'fingerprint'
Cyril Mougel
2 parents 7de482ac 559d808d
Showing 11 changed files with 51 additions and 40 deletions Show diff stats
app/interactors/problem_updater_cache.rb
app/models/app.rb
app/models/err.rb
app/models/error_report.rb
app/models/problem.rb
spec/fabricators/err_fabricator.rb
spec/fabricators/problem_fabricator.rb
spec/interactors/problem_updater_cache_spec.rb
spec/models/app_spec.rb
spec/models/err_spec.rb
spec/models/problem_spec.rb
@@ -43,8 +43,6 @@ class ProblemUpdaterCache
     attrs[:last_notice_at] = last_notice.created_at if last_notice
     attrs.merge!(
       :message     => notice.message,
-      :environment => notice.environment_name,
-      :error_class => notice.error_class,
       :where       => notice.where,
       :messages    => attribute_count(:message, messages),
       :hosts       => attribute_count(:host, hosts),
@@ -42,11 +42,17 @@ class App
   accepts_nested_attributes_for :notification_service, :allow_destroy => true,
     :reject_if => proc { |attrs| !NotificationService.subclasses.map(&:to_s).include?(attrs[:type].to_s) }
+  # Acceps a hash with the following attributes:
+  #
+  # * <tt>:error_class</tt> - the class of error (required to create a new Problem)
+  # * <tt>:environment</tt> - the environment the source app was running in (required to create a new Problem)
+  # * <tt>:fingerprint</tt> - a unique value identifying the notice
+  #
   def find_or_create_err!(attrs)
     Err.where(
       :fingerprint => attrs[:fingerprint]
     ).first ||
-      problems.create!.errs.create!(attrs)
+      problems.create!(attrs.slice(:error_class, :environment)).errs.create!(attrs)
   end
   # Mongoid Bug: find(id) on association proxies returns an Enumerator
@@ -6,20 +6,16 @@ class Err
   include Mongoid::Document
   include Mongoid::Timestamps
-  field :error_class, :default => "UnknownError"
-  field :component
-  field :action
-  field :environment, :default => "unknown"
   field :fingerprint
-  belongs_to :problem
   index :problem_id
-  index :error_class
   index :fingerprint
+  belongs_to :problem
   has_many :notices, :inverse_of => :err, :dependent => :destroy
+  validates_presence_of :problem_id, :fingerprint
+
   delegate :app, :resolved?, :to => :problem
 end
-
@@ -23,15 +23,9 @@ class ErrorReport
   end
   def rails_env
-    server_environment['environment-name'] || 'development'
-  end
-
-  def component
-    request['component'] || 'unknown'
-  end
-
-  def action
-    request['action']
+    rails_env = server_environment['environment-name']
+    rails_env = 'development' if rails_env.blank?
+    rails_env
   end
   def app
@@ -69,8 +63,6 @@ class ErrorReport
   def error
     @error ||= app.find_or_create_err!(
       :error_class => error_class,
-      :component => component,
-      :action => action,
       :environment => rails_env,
       :fingerprint => fingerprint
     )
@@ -39,6 +39,8 @@ class Problem
   has_many :errs, :inverse_of => :problem, :dependent => :destroy
   has_many :comments, :inverse_of => :err, :dependent => :destroy
+  validates_presence_of :error_class, :environment
+
   before_create :cache_app_attributes
   scope :resolved, where(:resolved => true)
@@ -91,11 +93,12 @@ class Problem
   end
   def unmerge!
+    attrs = {:error_class => error_class, :environment => environment}
     problem_errs = errs.to_a
     problem_errs.shift
     [self] + problem_errs.map(&:id).map do |err_id|
       err = Err.find(err_id)
-      app.problems.create.tap do |new_problem|
+      app.problems.create(attrs).tap do |new_problem|
         err.update_attribute(:problem_id, new_problem.id)
         new_problem.reset_cached_attributes
       end
 Fabricator :err do
   problem!
-  error_class!         { 'FooError' }
-  component     'foo'
-  action        'bar'
-  environment   'production'
+  fingerprint 'some-finger-print'
 end
 Fabricator :notice do
 Fabricator(:problem) do
   app! { Fabricate(:app) }
   comments { [] }
+  error_class 'FooError'
+  environment 'production'
 end
 Fabricator(:problem_with_comments, :from => :problem) do
@@ -27,8 +27,6 @@ describe ProblemUpdaterCache do
         it 'update information about this notice' do
           expect(problem.message).to eq notice.message
-          expect(problem.environment).to eq notice.environment_name
-          expect(problem.error_class).to eq notice.error_class
           expect(problem.where).to eq notice.where
         end
@@ -68,8 +66,6 @@ describe ProblemUpdaterCache do
         end
         it 'update information about this notice' do
           expect(problem.message).to eq notice.message
-          expect(problem.environment).to eq notice.environment_name
-          expect(problem.error_class).to eq notice.error_class
           expect(problem.where).to eq notice.where
         end
@@ -118,8 +114,6 @@ describe ProblemUpdaterCache do
       it 'update information about this notice' do
         expect(problem.message).to eq notice.message
-        expect(problem.environment).to eq notice.environment_name
-        expect(problem.error_class).to eq notice.error_class
         expect(problem.where).to eq notice.where
       end
@@ -162,9 +162,8 @@ describe App do
       @app = Fabricate(:app)
       @conditions = {
         :error_class  => 'Whoops',
-        :component    => 'Foo',
-        :action       => 'bar',
-        :environment  => 'production'
+        :environment  => 'production',
+        :fingerprint  => 'some-finger-print'
       }
     end
 require 'spec_helper'
 describe Err do
-  it 'sets a default error_class and environment' do
-    err = Err.new
-    err.error_class.should == "UnknownError"
-    err.environment.should == "unknown"
+
+  context 'validations' do
+    it 'requires a fingerprint' do
+      err = Fabricate.build(:err, :fingerprint => nil)
+      err.should_not be_valid
+      err.errors[:fingerprint].should include("can't be blank")
+    end
+
+    it 'requires a problem' do
+      err = Fabricate.build(:err, :problem_id => nil)
+      err.should_not be_valid
+      err.errors[:problem_id].should include("can't be blank")
+    end
   end
-end
+end
 require 'spec_helper'
 describe Problem do
+  
+  context 'validations' do
+    it 'requires a error_class' do
+      err = Fabricate.build(:problem, :error_class => nil)
+      err.should_not be_valid
+      err.errors[:error_class].should include("can't be blank")
+    end
+
+    it 'requires an environment' do
+      err = Fabricate.build(:problem, :environment => nil)
+      err.should_not be_valid
+      err.errors[:environment].should include("can't be blank")
+    end
+  end
+  
   describe "Fabrication" do
     context "Fabricate(:problem)" do
       it 'should have no comment' do
1	Fabricator(:problem) do	1	Fabricator(:problem) do
2	app! { Fabricate(:app) }	2	app! { Fabricate(:app) }
3	comments { [] }	3	comments { [] }
		4	+ error_class 'FooError'
		5	+ environment 'production'
4	end	6	end
5		7
6	Fabricator(:problem_with_comments, :from => :problem) do	8	Fabricator(:problem_with_comments, :from => :problem) do
1	require 'spec_helper'	1	require 'spec_helper'
2		2
3	describe Err do	3	describe Err do
4	- it 'sets a default error_class and environment' do
5	- err = Err.new
6	- err.error_class.should == "UnknownError"
7	- err.environment.should == "unknown"	4	+
		5	+ context 'validations' do
		6	+ it 'requires a fingerprint' do
		7	+ err = Fabricate.build(:err, :fingerprint => nil)
		8	+ err.should_not be_valid
		9	+ err.errors[:fingerprint].should include("can't be blank")
		10	+ end
		11	+
		12	+ it 'requires a problem' do
		13	+ err = Fabricate.build(:err, :problem_id => nil)
		14	+ err.should_not be_valid
		15	+ err.errors[:problem_id].should include("can't be blank")
		16	+ end
8	end	17	end
9	-end
10		18
		19	+end
1	require 'spec_helper'	1	require 'spec_helper'
2		2
3	describe Problem do	3	describe Problem do
		4	+
		5	+ context 'validations' do
		6	+ it 'requires a error_class' do
		7	+ err = Fabricate.build(:problem, :error_class => nil)
		8	+ err.should_not be_valid
		9	+ err.errors[:error_class].should include("can't be blank")
		10	+ end
		11	+
		12	+ it 'requires an environment' do
		13	+ err = Fabricate.build(:problem, :environment => nil)
		14	+ err.should_not be_valid
		15	+ err.errors[:environment].should include("can't be blank")
		16	+ end
		17	+ end
		18	+
4	describe "Fabrication" do	19	describe "Fabrication" do
5	context "Fabricate(:problem)" do	20	context "Fabricate(:problem)" do
6	it 'should have no comment' do	21	it 'should have no comment' do