Do not store serialized objects in target_data

Victor Costa
1 parent 65cdb388
Showing 6 changed files with 60 additions and 4 deletions Show diff stats
db/migrate/20160216134912_clean_target_data_column.rb
lib/merit_ext.rb
script/check_merit_actions_vs_points.rb
script/process_merit_rules.rb
test/unit/article_test.rb
test/unit/merit_ext_test.rb
@@ -0,0 +1,27 @@
+class CleanTargetDataColumn < ActiveRecord::Migration
+  
+  def up
+    models = {'article' => 'Article', 'comment' => 'Comment', 'vote' => 'Vote',
+      'friendship' => 'Friendship', 'profile' => 'Profile',
+      'articlefollower' => 'ArticleFollower'}
+
+    Merit::Action.where("target_model NOT IN (?)", models.keys).find_each do |action|
+      next if action.target_data.blank?
+      obj = YAML.load(action.target_data) rescue nil
+      unless obj.nil?
+        action.update_attribute(:target_model, obj.class.respond_to?(:base_class) ? obj.class.base_class.name : obj.class.name)
+      end
+    end
+
+    models.each do |old_name, new_name|
+      Merit::Action.where(target_model: old_name).update_all(target_model: new_name)
+    end
+
+    Merit::Action.update_all(target_data: nil)
+  end
+  
+  def down
+    puts "Warning: cannot restore target_data"
+  end
+  
+end
 require 'merit/badge_ext'
 require 'merit/sash'
 require 'merit/badges_sash'
+require 'merit/action'
 module Merit
@@ -40,6 +41,16 @@ module Merit
     end
   end
+  class Action
+    def target_obj
+      target_model.constantize.find_by_id(target_id)
+    end
+
+    def rules_matcher
+      @rules_matcher ||= ::Merit::RulesMatcher.new(target_model.downcase, action_method)
+    end
+  end
+
   module ClassMethods
     def has_merit_actions(options = {})
@@ -64,9 +75,8 @@ module Merit
       :user_id => user ? user.id : nil,
       :action_method => action,
       :had_errors => self.errors.present?,
-      :target_model => self.class.base_class.name.downcase,
-      :target_id => self.id,
-      :target_data => self.to_yaml
+      :target_model => self.class.base_class.name,
+      :target_id => self.id
     })
     action.check_all_rules
     action
@@ -17,7 +17,9 @@ class ProcessObserver
     merit = changed_data[:merit_object]
     if merit.kind_of?(Merit::Score::Point)
       action = Merit::Action.find(changed_data[:merit_action_id])
-      new_date = YAML.load(action.target_data).created_at
+      model = action.target_obj
+      return if model.nil?
+      new_date = model.created_at
       action.update_attribute(:created_at, new_date)
       merit.update_attribute(:created_at, new_date)
     end
@@ -6,6 +6,8 @@ class ProcessObserver
     merit = changed_data[:merit_object]
     if merit.kind_of?(Merit::Score::Point)
       action = Merit::Action.find(changed_data[:merit_action_id])
+      model = action.target_obj
+      return if model.nil?
       new_date = YAML.load(action.target_data).created_at
       action.update_attribute(:created_at, new_date)
       merit.update_attribute(:created_at, new_date)
@@ -209,4 +209,11 @@ class ArticleTest &lt; ActiveSupport::TestCase
     assert_equal [], person.badges
   end
+  should 'restore article object from action' do
+    create_point_rule_definition('article_author')
+    article = create(TextArticle, :profile_id => person.id, :author => person)
+    assert_equal 1, person.score_points.count
+    assert_equal article, person.score_points.first.action.target_obj
+  end
+
 end
@@ -24,4 +24,12 @@ class MeritExtTest &lt; ActiveSupport::TestCase
     assert !point.undo_rule?
   end
+  should 'return target object associated to the merit action' do
+    article = fast_create(Article)
+    action = Merit::Action.new
+    action.target_model = article.class.base_class.name
+    action.target_id = article.id
+    assert_equal article, action.target_obj
+  end
+
 end
@@ -0,0 +1,27 @@		@@ -0,0 +1,27 @@
	1	+class CleanTargetDataColumn < ActiveRecord::Migration
	2	+
	3	+ def up
	4	+ models = {'article' => 'Article', 'comment' => 'Comment', 'vote' => 'Vote',
	5	+ 'friendship' => 'Friendship', 'profile' => 'Profile',
	6	+ 'articlefollower' => 'ArticleFollower'}
	7	+
	8	+ Merit::Action.where("target_model NOT IN (?)", models.keys).find_each do \|action\|
	9	+ next if action.target_data.blank?
	10	+ obj = YAML.load(action.target_data) rescue nil
	11	+ unless obj.nil?
	12	+ action.update_attribute(:target_model, obj.class.respond_to?(:base_class) ? obj.class.base_class.name : obj.class.name)
	13	+ end
	14	+ end
	15	+
	16	+ models.each do \|old_name, new_name\|
	17	+ Merit::Action.where(target_model: old_name).update_all(target_model: new_name)
	18	+ end
	19	+
	20	+ Merit::Action.update_all(target_data: nil)
	21	+ end
	22	+
	23	+ def down
	24	+ puts "Warning: cannot restore target_data"
	25	+ end
	26	+
	27	+end
	@@ -209,4 +209,11 @@ class ArticleTest < ActiveSupport::TestCase		@@ -209,4 +209,11 @@ class ArticleTest < ActiveSupport::TestCase
209	assert_equal [], person.badges	209	assert_equal [], person.badges
210	end	210	end
211		211
		212	+ should 'restore article object from action' do
		213	+ create_point_rule_definition('article_author')
		214	+ article = create(TextArticle, :profile_id => person.id, :author => person)
		215	+ assert_equal 1, person.score_points.count
		216	+ assert_equal article, person.score_points.first.action.target_obj
		217	+ end
		218	+
212	end	219	end
	@@ -24,4 +24,12 @@ class MeritExtTest < ActiveSupport::TestCase		@@ -24,4 +24,12 @@ class MeritExtTest < ActiveSupport::TestCase
24	assert !point.undo_rule?	24	assert !point.undo_rule?
25	end	25	end
26		26
		27	+ should 'return target object associated to the merit action' do
		28	+ article = fast_create(Article)
		29	+ action = Merit::Action.new
		30	+ action.target_model = article.class.base_class.name
		31	+ action.target_id = article.id
		32	+ assert_equal article, action.target_obj
		33	+ end
		34	+
27	end	35	end