Commit 2e283c5d667f0c9324b06a78454b651eca481e4f

Authored by Cyril Mougel
1 parent a3ed065c
Exists in master and in 1 other branch production

Refactor the problem merge system

* Extract the Problem#merge! in his own class
* Extract the update cache system from problem in his own class
* Welcome back to the backtrace_line_spec not found by a rspec spec
  because no end by _spec.rb
app/interactors/problem_merge.rb 0 → 100644
@@ -0,0 +1,26 @@ @@ -0,0 +1,26 @@
  1 +require 'problem_destroy'
  2 +
  3 +class ProblemMerge
  4 + def initialize(*problems)
  5 + problems = problems.flatten.uniq
  6 + @merged_problem = problems[0]
  7 + @child_problems = problems[1..-1]
  8 + raise ArgumentError.new("need almost 2 uniq different problems") if @child_problems.empty?
  9 + end
  10 + attr_reader :merged_problem, :child_problems
  11 +
  12 + def merge
  13 + child_problems.each do |problem|
  14 + merged_problem.errs.concat Err.where(:problem_id => problem.id)
  15 + ProblemDestroy.execute(problem)
  16 + end
  17 + reset_cached_attributes
  18 + merged_problem
  19 + end
  20 +
  21 + private
  22 +
  23 + def reset_cached_attributes
  24 + ProblemUpdaterCache.new(merged_problem).update
  25 + end
  26 +end
app/interactors/problem_updater_cache.rb 0 → 100644
@@ -0,0 +1,90 @@ @@ -0,0 +1,90 @@
  1 +class ProblemUpdaterCache
  2 + def initialize(problem, notice=nil)
  3 + @problem = problem
  4 + @notice = notice
  5 + end
  6 + attr_reader :problem
  7 +
  8 + ##
  9 + # Update cache information about child associate to this problem
  10 + #
  11 + # update the notices count, and some notice informations
  12 + #
  13 + # @return [ Problem ] the problem with this update
  14 + #
  15 + def update
  16 + update_notices_count
  17 + update_notices_cache
  18 + problem
  19 + end
  20 +
  21 + private
  22 +
  23 + def update_notices_count
  24 + if @notice
  25 + problem.inc(:notices_count, 1)
  26 + else
  27 + problem.update_attribute(
  28 + :notices_count, problem.notices.count
  29 + )
  30 + end
  31 + end
  32 +
  33 + ##
  34 + # Update problem statistique from some notice information
  35 + #
  36 + def update_notices_cache
  37 + first_notice = notices.first
  38 + last_notice = notices.last
  39 + notice ||= @notice || first_notice
  40 +
  41 + attrs = {}
  42 + attrs[:first_notice_at] = first_notice.created_at if first_notice
  43 + attrs[:last_notice_at] = last_notice.created_at if last_notice
  44 + attrs.merge!(
  45 + :message => notice.message,
  46 + :environment => notice.environment_name,
  47 + :error_class => notice.error_class,
  48 + :where => notice.where,
  49 + :messages => attribute_count(:message, messages),
  50 + :hosts => attribute_count(:host, hosts),
  51 + :user_agents => attribute_count(:user_agent_string, user_agents)
  52 + ) if notice
  53 + problem.update_attributes!(attrs)
  54 + end
  55 +
  56 + def notices
  57 + @notices ||= @notice ? [@notice].sort(&:created_at) : problem.notices.order_by([:created_at, :asc])
  58 + end
  59 +
  60 + def messages
  61 + @notice ? problem.messages : {}
  62 + end
  63 +
  64 + def hosts
  65 + @notice ? problem.hosts : {}
  66 + end
  67 +
  68 + def user_agents
  69 + @notice ? problem.user_agents : {}
  70 + end
  71 +
  72 + private
  73 +
  74 + def attribute_count(value, init)
  75 + init.tap do |counts|
  76 + notices.each do |notice|
  77 + counts[attribute_index(notice.send(value))] ||= {
  78 + 'value' => notice.send(value),
  79 + 'count' => 0
  80 + }
  81 + counts[attribute_index(notice.send(value))]['count'] += 1
  82 + end
  83 + end
  84 + end
  85 +
  86 + def attribute_index(value)
  87 + @attributes_index ||= {}
  88 + @attributes_index[value.to_s] ||= Digest::MD5.hexdigest(value.to_s)
  89 + end
  90 +end
app/models/notice.rb
@@ -26,7 +26,7 @@ class Notice @@ -26,7 +26,7 @@ class Notice
26 ] 26 ]
27 ) 27 )
28 28
29 - after_create :increase_counter_cache, :cache_attributes_on_problem, :unresolve_problem 29 + after_create :cache_attributes_on_problem, :unresolve_problem
30 before_save :sanitize 30 before_save :sanitize
31 before_destroy :decrease_counter_cache, :remove_cached_attributes_from_problem 31 before_destroy :decrease_counter_cache, :remove_cached_attributes_from_problem
32 32
@@ -116,10 +116,6 @@ class Notice @@ -116,10 +116,6 @@ class Notice
116 116
117 protected 117 protected
118 118
119 - def increase_counter_cache  
120 - problem.inc(:notices_count, 1)  
121 - end  
122 -  
123 def decrease_counter_cache 119 def decrease_counter_cache
124 problem.inc(:notices_count, -1) if err 120 problem.inc(:notices_count, -1) if err
125 end 121 end
@@ -133,7 +129,7 @@ class Notice @@ -133,7 +129,7 @@ class Notice
133 end 129 end
134 130
135 def cache_attributes_on_problem 131 def cache_attributes_on_problem
136 - problem.cache_notice_attributes(self) 132 + ProblemUpdaterCache.new(problem, self).update
137 end 133 end
138 134
139 def sanitize 135 def sanitize
app/models/problem.rb
@@ -83,15 +83,7 @@ class Problem @@ -83,15 +83,7 @@ class Problem
83 83
84 84
85 def self.merge!(*problems) 85 def self.merge!(*problems)
86 - problems = problems.flatten.uniq  
87 - merged_problem = problems.shift  
88 - problems.each do |problem|  
89 - merged_problem.errs.concat Err.where(:problem_id => problem.id)  
90 - problem.errs(true) # reload problem.errs (should be empty) before problem.destroy  
91 - problem.destroy  
92 - end  
93 - merged_problem.reset_cached_attributes  
94 - merged_problem 86 + ProblemMerge.new(problems).merge
95 end 87 end
96 88
97 def merged? 89 def merged?
@@ -128,9 +120,7 @@ class Problem @@ -128,9 +120,7 @@ class Problem
128 120
129 121
130 def reset_cached_attributes 122 def reset_cached_attributes
131 - update_attribute(:notices_count, notices.count)  
132 - cache_app_attributes  
133 - cache_notice_attributes 123 + ProblemUpdaterCache.new(self).update
134 end 124 end
135 125
136 def cache_app_attributes 126 def cache_app_attributes
@@ -145,26 +135,6 @@ class Problem @@ -145,26 +135,6 @@ class Problem
145 end 135 end
146 end 136 end
147 137
148 - def cache_notice_attributes(notice=nil)  
149 - first_notice = notices.order_by([:created_at, :asc]).first  
150 - last_notice = notices.order_by([:created_at, :asc]).last  
151 - notice ||= first_notice  
152 -  
153 - attrs = {}  
154 - attrs[:first_notice_at] = first_notice.created_at if first_notice  
155 - attrs[:last_notice_at] = last_notice.created_at if last_notice  
156 - attrs.merge!(  
157 - :message => notice.message,  
158 - :environment => notice.environment_name,  
159 - :error_class => notice.error_class,  
160 - :where => notice.where,  
161 - :messages => attribute_count_increase(:messages, notice.message),  
162 - :hosts => attribute_count_increase(:hosts, notice.host),  
163 - :user_agents => attribute_count_increase(:user_agents, notice.user_agent_string)  
164 - ) if notice  
165 - update_attributes!(attrs)  
166 - end  
167 -  
168 def remove_cached_notice_attributes(notice) 138 def remove_cached_notice_attributes(notice)
169 update_attributes!( 139 update_attributes!(
170 :messages => attribute_count_descrease(:messages, notice.message), 140 :messages => attribute_count_descrease(:messages, notice.message),
@@ -184,15 +154,6 @@ class Problem @@ -184,15 +154,6 @@ class Problem
184 end 154 end
185 155
186 private 156 private
187 - def attribute_count_increase(name, value)  
188 - counter, index = send(name), attribute_index(value)  
189 - if counter[index].nil?  
190 - counter[index] = {'value' => value, 'count' => 1}  
191 - else  
192 - counter[index]['count'] += 1  
193 - end  
194 - counter  
195 - end  
196 157
197 def attribute_count_descrease(name, value) 158 def attribute_count_descrease(name, value)
198 counter, index = send(name), attribute_index(value) 159 counter, index = send(name), attribute_index(value)
lib/tasks/errbit/database.rake
@@ -6,7 +6,9 @@ namespace :errbit do @@ -6,7 +6,9 @@ namespace :errbit do
6 desc "Updates cached attributes on Problem" 6 desc "Updates cached attributes on Problem"
7 task :update_problem_attrs => :environment do 7 task :update_problem_attrs => :environment do
8 puts "Updating problems" 8 puts "Updating problems"
9 - Problem.all.each(&:cache_notice_attributes) 9 + Problem.all.each{|problem|
  10 + ProblemUpdaterCache.new(self).update
  11 + }
10 end 12 end
11 13
12 desc "Updates Problem#notices_count" 14 desc "Updates Problem#notices_count"
spec/fabricators/problem_fabricator.rb
@@ -10,3 +10,13 @@ Fabricator(:problem_with_comments, :from => :problem) do @@ -10,3 +10,13 @@ Fabricator(:problem_with_comments, :from => :problem) do
10 end 10 end
11 } 11 }
12 end 12 end
  13 +
  14 +Fabricator(:problem_with_errs, :from => :problem) do
  15 + after_create { |parent|
  16 + 3.times do
  17 + Fabricate(:err, :problem => parent)
  18 + end
  19 + }
  20 +end
  21 +
  22 +
spec/interactors/problem_merge_spec.rb 0 → 100644
@@ -0,0 +1,48 @@ @@ -0,0 +1,48 @@
  1 +require 'spec_helper'
  2 +
  3 +describe ProblemMerge do
  4 + let(:problem) { Fabricate(:problem_with_errs) }
  5 + let(:problem_1) { Fabricate(:problem_with_errs) }
  6 +
  7 + describe "#initialize" do
  8 + it 'failed if less than 2 uniq problem pass in args' do
  9 + expect {
  10 + ProblemMerge.new(problem)
  11 + }.to raise_error(ArgumentError)
  12 + end
  13 +
  14 + it 'extract first problem like merged_problem' do
  15 + problem_merge = ProblemMerge.new(problem, problem, problem_1)
  16 + expect(problem_merge.merged_problem).to eql problem
  17 + end
  18 + it 'extract other problem like child_problems' do
  19 + problem_merge = ProblemMerge.new(problem, problem, problem_1)
  20 + expect(problem_merge.child_problems).to eql [problem_1]
  21 + end
  22 + end
  23 +
  24 + describe "#merge" do
  25 + let!(:problem_merge) {
  26 + ProblemMerge.new(problem, problem_1)
  27 + }
  28 + let(:first_errs) { problem.errs }
  29 + let(:merged_errs) { problem_1.errs }
  30 + let!(:notice) { Fabricate(:notice, :err => first_errs.first) }
  31 + let!(:notice_1) { Fabricate(:notice, :err => merged_errs.first) }
  32 + it 'delete one of problem' do
  33 + expect {
  34 + problem_merge.merge
  35 + }.to change(Problem, :count).by(-1)
  36 + end
  37 +
  38 + it 'move all err in one problem' do
  39 + problem_merge.merge
  40 + problem.reload.errs.should eq (first_errs | merged_errs)
  41 + end
  42 +
  43 + it 'update problem cache' do
  44 + ProblemUpdaterCache.should_receive(:new).with(problem).and_return(mock(:update => true))
  45 + problem_merge.merge
  46 + end
  47 + end
  48 +end
spec/interactors/problem_updater_cache_spec.rb 0 → 100644
@@ -0,0 +1,153 @@ @@ -0,0 +1,153 @@
  1 +require 'spec_helper'
  2 +
  3 +describe ProblemUpdaterCache do
  4 + let(:problem) { Fabricate(:problem_with_errs) }
  5 + let(:first_errs) { problem.errs }
  6 + let!(:notice) { Fabricate(:notice, :err => first_errs.first) }
  7 +
  8 + describe "#update" do
  9 + context "without notice pass args" do
  10 + before do
  11 + problem.update_attribute(:notices_count, 0)
  12 + end
  13 +
  14 + it 'update the notice_count' do
  15 + expect {
  16 + ProblemUpdaterCache.new(problem).update
  17 + }.to change{
  18 + problem.notices_count
  19 + }.from(0).to(1)
  20 + end
  21 +
  22 + context "with only one notice" do
  23 + before do
  24 + problem.update_attributes!(:messages => {})
  25 + ProblemUpdaterCache.new(problem).update
  26 + end
  27 +
  28 + it 'update information about this notice' do
  29 + expect(problem.message).to eq notice.message
  30 + expect(problem.environment).to eq notice.environment_name
  31 + expect(problem.error_class).to eq notice.error_class
  32 + expect(problem.where).to eq notice.where
  33 + end
  34 +
  35 + it 'update first_notice_at' do
  36 + expect(problem.first_notice_at).to eq notice.created_at
  37 + end
  38 +
  39 + it 'update last_notice_at' do
  40 + expect(problem.last_notice_at).to eq notice.created_at
  41 + end
  42 +
  43 + it 'update stats messages' do
  44 + expect(problem.messages).to eq({
  45 + Digest::MD5.hexdigest(notice.message) => {'value' => notice.message, 'count' => 1}
  46 + })
  47 + end
  48 +
  49 + it 'update stats hosts' do
  50 + expect(problem.hosts).to eq({
  51 + Digest::MD5.hexdigest(notice.host) => {'value' => notice.host, 'count' => 1}
  52 + })
  53 + end
  54 +
  55 + it 'update stats user_agents' do
  56 + expect(problem.user_agents).to eq({
  57 + Digest::MD5.hexdigest(notice.user_agent_string) => {'value' => notice.user_agent_string, 'count' => 1}
  58 + })
  59 + end
  60 + end
  61 +
  62 + context "with several notices" do
  63 + let!(:notice_2) { Fabricate(:notice, :err => first_errs.first) }
  64 + let!(:notice_3) { Fabricate(:notice, :err => first_errs.first) }
  65 + before do
  66 + problem.update_attributes!(:messages => {})
  67 + ProblemUpdaterCache.new(problem).update
  68 + end
  69 + it 'update information about this notice' do
  70 + expect(problem.message).to eq notice.message
  71 + expect(problem.environment).to eq notice.environment_name
  72 + expect(problem.error_class).to eq notice.error_class
  73 + expect(problem.where).to eq notice.where
  74 + end
  75 +
  76 + it 'update first_notice_at' do
  77 + expect(problem.first_notice_at.to_time).to eq notice.created_at.to_time
  78 + end
  79 +
  80 + it 'update last_notice_at' do
  81 + expect(problem.last_notice_at.to_i).to be_within(1).of(notice.created_at.to_i)
  82 + end
  83 +
  84 + it 'update stats messages' do
  85 + expect(problem.messages).to eq({
  86 + Digest::MD5.hexdigest(notice.message) => {'value' => notice.message, 'count' => 3}
  87 + })
  88 + end
  89 +
  90 + it 'update stats hosts' do
  91 + expect(problem.hosts).to eq({
  92 + Digest::MD5.hexdigest(notice.host) => {'value' => notice.host, 'count' => 3}
  93 + })
  94 + end
  95 +
  96 + it 'update stats user_agents' do
  97 + expect(problem.user_agents).to eq({
  98 + Digest::MD5.hexdigest(notice.user_agent_string) => {'value' => notice.user_agent_string, 'count' => 3}
  99 + })
  100 + end
  101 +
  102 + end
  103 + end
  104 +
  105 + context "with notice pass in args" do
  106 +
  107 + before do
  108 + ProblemUpdaterCache.new(problem, notice).update
  109 + end
  110 +
  111 + it 'increase notices_count by 1' do
  112 + expect {
  113 + ProblemUpdaterCache.new(problem, notice).update
  114 + }.to change{
  115 + problem.notices_count
  116 + }.by(1)
  117 + end
  118 +
  119 + it 'update information about this notice' do
  120 + expect(problem.message).to eq notice.message
  121 + expect(problem.environment).to eq notice.environment_name
  122 + expect(problem.error_class).to eq notice.error_class
  123 + expect(problem.where).to eq notice.where
  124 + end
  125 +
  126 + it 'update first_notice_at' do
  127 + expect(problem.first_notice_at).to eq notice.created_at
  128 + end
  129 +
  130 + it 'update last_notice_at' do
  131 + expect(problem.last_notice_at).to eq notice.created_at
  132 + end
  133 +
  134 + it 'inc stats messages' do
  135 + expect(problem.messages).to eq({
  136 + Digest::MD5.hexdigest(notice.message) => {'value' => notice.message, 'count' => 2}
  137 + })
  138 + end
  139 +
  140 + it 'inc stats hosts' do
  141 + expect(problem.hosts).to eq({
  142 + Digest::MD5.hexdigest(notice.host) => {'value' => notice.host, 'count' => 2}
  143 + })
  144 + end
  145 +
  146 + it 'inc stats user_agents' do
  147 + expect(problem.user_agents).to eq({
  148 + Digest::MD5.hexdigest(notice.user_agent_string) => {'value' => notice.user_agent_string, 'count' => 2}
  149 + })
  150 + end
  151 + end
  152 + end
  153 +end
spec/models/backtrace_line.rb
@@ -1,12 +0,0 @@ @@ -1,12 +0,0 @@
1 -require 'spec_helper'  
2 -  
3 -describe BacktraceLine do  
4 - subject { described_class.new(raw_line) }  
5 -  
6 - describe "root at the start of decorated filename" do  
7 - let(:raw_line) { { 'number' => rand(999), 'file' => '[PROJECT_ROOT]/app/controllers/pages_controller.rb', 'method' => ActiveSupport.methods.shuffle.first.to_s } }  
8 - it "should leave leading root symbol in filepath" do  
9 - subject.decorated_path.should == '/app/controllers/'  
10 - end  
11 - end  
12 -end  
spec/models/backtrace_line_spec.rb 0 → 100644
@@ -0,0 +1,12 @@ @@ -0,0 +1,12 @@
  1 +require 'spec_helper'
  2 +
  3 +describe BacktraceLine do
  4 + subject { described_class.new(raw_line) }
  5 +
  6 + describe "root at the start of decorated filename" do
  7 + let(:raw_line) { { 'number' => rand(999), 'file' => '[PROJECT_ROOT]/app/controllers/pages_controller.rb', 'method' => ActiveSupport.methods.shuffle.first.to_s } }
  8 + it "should leave leading root symbol in filepath" do
  9 + subject.decorated_path.should == 'app/controllers/'
  10 + end
  11 + end
  12 +end
spec/models/problem_spec.rb
@@ -23,6 +23,17 @@ describe Problem do @@ -23,6 +23,17 @@ describe Problem do
23 end.should change(Comment, :count).by(3) 23 end.should change(Comment, :count).by(3)
24 end 24 end
25 end 25 end
  26 +
  27 + context "Fabricate(:problem_with_errs)" do
  28 + it 'should be valid' do
  29 + Fabricate.build(:problem_with_errs).should be_valid
  30 + end
  31 + it 'should have 3 errs' do
  32 + lambda do
  33 + Fabricate(:problem_with_errs)
  34 + end.should change(Err, :count).by(3)
  35 + end
  36 + end
26 end 37 end
27 38
28 context '#last_notice_at' do 39 context '#last_notice_at' do
@@ -49,7 +60,7 @@ describe Problem do @@ -49,7 +60,7 @@ describe Problem do
49 problem.first_notice_at.should == notice1.created_at 60 problem.first_notice_at.should == notice1.created_at
50 61
51 notice2 = Fabricate(:notice, :err => err) 62 notice2 = Fabricate(:notice, :err => err)
52 - problem.first_notice_at.should == notice1.created_at 63 + problem.first_notice_at.to_time.should eq notice1.created_at
53 end 64 end
54 end 65 end
55 66
@@ -134,22 +145,6 @@ describe Problem do @@ -134,22 +145,6 @@ describe Problem do
134 end 145 end
135 end 146 end
136 147
137 -  
138 - context ".merge!" do  
139 - it "collects the Errs from several problems into one and deletes the other problems" do  
140 - problem1 = Fabricate(:err).problem  
141 - problem2 = Fabricate(:err).problem  
142 - problem1.errs.length.should == 1  
143 - problem2.errs.length.should == 1  
144 -  
145 - lambda {  
146 - merged_problem = Problem.merge!(problem1, problem2)  
147 - merged_problem.reload.errs.length.should == 2  
148 - }.should change(Problem, :count).by(-1)  
149 - end  
150 - end  
151 -  
152 -  
153 context "#unmerge!" do 148 context "#unmerge!" do
154 it "creates a separate problem for each err" do 149 it "creates a separate problem for each err" do
155 problem1 = Fabricate(:notice).problem 150 problem1 = Fabricate(:notice).problem
@@ -188,17 +183,17 @@ describe Problem do @@ -188,17 +183,17 @@ describe Problem do
188 183
189 context "searching" do 184 context "searching" do
190 it 'finds the correct record' do 185 it 'finds the correct record' do
191 - find = Fabricate(:problem, :resolved => false, :error_class => 'theErrorclass::other', 186 + find = Fabricate(:problem, :resolved => false, :error_class => 'theErrorclass::other',
192 :message => "other", :where => 'errorclass', :environment => 'development', :app_name => 'other') 187 :message => "other", :where => 'errorclass', :environment => 'development', :app_name => 'other')
193 - dont_find = Fabricate(:problem, :resolved => false, :error_class => "Batman", 188 + dont_find = Fabricate(:problem, :resolved => false, :error_class => "Batman",
194 :message => 'todo', :where => 'classerror', :environment => 'development', :app_name => 'other') 189 :message => 'todo', :where => 'classerror', :environment => 'development', :app_name => 'other')
195 Problem.search("theErrorClass").unresolved.should include(find) 190 Problem.search("theErrorClass").unresolved.should include(find)
196 Problem.search("theErrorClass").unresolved.should_not include(dont_find) 191 Problem.search("theErrorClass").unresolved.should_not include(dont_find)
197 end 192 end
198 end 193 end
199 end 194 end
200 -  
201 - 195 +
  196 +
202 context "notice counter cache" do 197 context "notice counter cache" do
203 before do 198 before do
204 @app = Fabricate(:app) 199 @app = Fabricate(:app)
@@ -213,15 +208,15 @@ describe Problem do @@ -213,15 +208,15 @@ describe Problem do
213 it "adding a notice increases #notices_count by 1" do 208 it "adding a notice increases #notices_count by 1" do
214 lambda { 209 lambda {
215 Fabricate(:notice, :err => @err, :message => 'ERR 1') 210 Fabricate(:notice, :err => @err, :message => 'ERR 1')
216 - }.should change(@problem, :notices_count).from(0).to(1) 211 + }.should change(@problem.reload, :notices_count).from(0).to(1)
217 end 212 end
218 213
219 it "removing a notice decreases #notices_count by 1" do 214 it "removing a notice decreases #notices_count by 1" do
220 notice1 = Fabricate(:notice, :err => @err, :message => 'ERR 1') 215 notice1 = Fabricate(:notice, :err => @err, :message => 'ERR 1')
221 - lambda { 216 + expect {
222 @err.notices.first.destroy 217 @err.notices.first.destroy
223 @problem.reload 218 @problem.reload
224 - }.should change(@problem, :notices_count).from(1).to(0) 219 + }.to change(@problem, :notices_count).from(1).to(0)
225 end 220 end
226 end 221 end
227 222