Commit 73c400bbb76b8d7e7f02783fc4aa6b3165575008
1 parent
47e920d6
Exists in
master
and in
1 other branch
Refactoring skip and vote to use polymorphism in appearance
Showing
11 changed files
with
39 additions
and
15 deletions
Show diff stats
app/models/appearance.rb
| @@ -3,12 +3,9 @@ class Appearance < ActiveRecord::Base | @@ -3,12 +3,9 @@ class Appearance < ActiveRecord::Base | ||
| 3 | belongs_to :prompt | 3 | belongs_to :prompt |
| 4 | belongs_to :question | 4 | belongs_to :question |
| 5 | 5 | ||
| 6 | - #technically, an appearance should either one vote or one skip, not one of both objects, but these declarations provide some useful helper methods | ||
| 7 | - # we could refactor this to use rails polymorphism, but currently the foreign key is stored in the vote and skip object | ||
| 8 | - has_one :vote | ||
| 9 | - has_one :skip | 6 | + belongs_to :answerable, :polymorphic => true |
| 10 | 7 | ||
| 11 | def answered? | 8 | def answered? |
| 12 | - vote || skip | 9 | + !self.answerable_id.nil? |
| 13 | end | 10 | end |
| 14 | end | 11 | end |
app/models/skip.rb
| @@ -2,5 +2,5 @@ class Skip < ActiveRecord::Base | @@ -2,5 +2,5 @@ class Skip < ActiveRecord::Base | ||
| 2 | belongs_to :skipper, :class_name => "Visitor", :foreign_key => "skipper_id" | 2 | belongs_to :skipper, :class_name => "Visitor", :foreign_key => "skipper_id" |
| 3 | belongs_to :question | 3 | belongs_to :question |
| 4 | belongs_to :prompt | 4 | belongs_to :prompt |
| 5 | - belongs_to :appearance | 5 | + has_one :appearance, :as => :answerable |
| 6 | end | 6 | end |
app/models/user.rb
| @@ -41,8 +41,7 @@ class User < ActiveRecord::Base | @@ -41,8 +41,7 @@ class User < ActiveRecord::Base | ||
| 41 | end | 41 | end |
| 42 | 42 | ||
| 43 | def record_appearance(visitor, prompt) | 43 | def record_appearance(visitor, prompt) |
| 44 | - a = Appearance.create(:voter => visitor, :prompt => prompt, :question_id => prompt.question_id, | ||
| 45 | - :lookup => Digest::MD5.hexdigest(rand(10000000000).to_s + visitor.id.to_s + prompt.id.to_s) ) | 44 | + a = Appearance.create(:voter => visitor, :prompt => prompt, :question_id => prompt.question_id, :site_id => self.id, :lookup => Digest::MD5.hexdigest(rand(10000000000).to_s + visitor.id.to_s + prompt.id.to_s) ) |
| 46 | end | 45 | end |
| 47 | 46 | ||
| 48 | 47 |
app/models/visitor.rb
| @@ -25,7 +25,7 @@ class Visitor < ActiveRecord::Base | @@ -25,7 +25,7 @@ class Visitor < ActiveRecord::Base | ||
| 25 | if options[:appearance_lookup] | 25 | if options[:appearance_lookup] |
| 26 | @appearance = prompt.appearances.find_by_lookup(options.delete(:appearance_lookup)) | 26 | @appearance = prompt.appearances.find_by_lookup(options.delete(:appearance_lookup)) |
| 27 | return nil unless @appearance # don't allow people to fake appearance lookups | 27 | return nil unless @appearance # don't allow people to fake appearance lookups |
| 28 | - options.merge!(:appearance_id => @appearance.id) | 28 | + options.merge!(:appearance => @appearance) |
| 29 | end | 29 | end |
| 30 | 30 | ||
| 31 | choice = prompt.choices[ordinality] #we need to guarantee that the choices are in the right order (by position) | 31 | choice = prompt.choices[ordinality] #we need to guarantee that the choices are in the right order (by position) |
| @@ -45,7 +45,7 @@ class Visitor < ActiveRecord::Base | @@ -45,7 +45,7 @@ class Visitor < ActiveRecord::Base | ||
| 45 | if options[:appearance_lookup] | 45 | if options[:appearance_lookup] |
| 46 | @appearance = prompt.appearances.find_by_lookup(options.delete(:appearance_lookup)) | 46 | @appearance = prompt.appearances.find_by_lookup(options.delete(:appearance_lookup)) |
| 47 | return nil unless @appearance | 47 | return nil unless @appearance |
| 48 | - options.merge!(:appearance_id => @appearance.id) | 48 | + options.merge!(:appearance => @appearance) |
| 49 | end | 49 | end |
| 50 | 50 | ||
| 51 | options.merge!(:question_id => prompt.question_id, :prompt_id => prompt.id, :skipper_id => self.id) | 51 | options.merge!(:question_id => prompt.question_id, :prompt_id => prompt.id, :skipper_id => self.id) |
app/models/vote.rb
| @@ -11,7 +11,7 @@ class Vote < ActiveRecord::Base | @@ -11,7 +11,7 @@ class Vote < ActiveRecord::Base | ||
| 11 | belongs_to :prompt, :counter_cache => true | 11 | belongs_to :prompt, :counter_cache => true |
| 12 | belongs_to :choice, :counter_cache => true | 12 | belongs_to :choice, :counter_cache => true |
| 13 | belongs_to :loser_choice, :class_name => "Choice", :foreign_key => "loser_choice_id" | 13 | belongs_to :loser_choice, :class_name => "Choice", :foreign_key => "loser_choice_id" |
| 14 | - belongs_to :appearance | 14 | + has_one :appearance, :as => :answerable |
| 15 | 15 | ||
| 16 | named_scope :recent, lambda { |*args| {:conditions => ["created_at > ?", (args.first || Date.today.beginning_of_day)]} } | 16 | named_scope :recent, lambda { |*args| {:conditions => ["created_at > ?", (args.first || Date.today.beginning_of_day)]} } |
| 17 | named_scope :with_question, lambda { |*args| {:conditions => {:question_id => args.first }} } | 17 | named_scope :with_question, lambda { |*args| {:conditions => {:question_id => args.first }} } |
db/migrate/20100714160406_add_polymorphic_answerable_to_appearance.rb
0 → 100644
| @@ -0,0 +1,11 @@ | @@ -0,0 +1,11 @@ | ||
| 1 | +class AddPolymorphicAnswerableToAppearance < ActiveRecord::Migration | ||
| 2 | + def self.up | ||
| 3 | + add_column :appearances, :answerable_id, :integer | ||
| 4 | + add_column :appearances, :answerable_type, :string | ||
| 5 | + end | ||
| 6 | + | ||
| 7 | + def self.down | ||
| 8 | + remove_column :appearances, :answerable_id | ||
| 9 | + remove_column :appearances, :answerable_type | ||
| 10 | + end | ||
| 11 | +end |
db/schema.rb
| @@ -9,7 +9,7 @@ | @@ -9,7 +9,7 @@ | ||
| 9 | # | 9 | # |
| 10 | # It's strongly recommended to check this file into your version control system. | 10 | # It's strongly recommended to check this file into your version control system. |
| 11 | 11 | ||
| 12 | -ActiveRecord::Schema.define(:version => 20100621164536) do | 12 | +ActiveRecord::Schema.define(:version => 20100714160406) do |
| 13 | 13 | ||
| 14 | create_table "appearances", :force => true do |t| | 14 | create_table "appearances", :force => true do |t| |
| 15 | t.integer "voter_id" | 15 | t.integer "voter_id" |
| @@ -19,6 +19,8 @@ ActiveRecord::Schema.define(:version => 20100621164536) do | @@ -19,6 +19,8 @@ ActiveRecord::Schema.define(:version => 20100621164536) do | ||
| 19 | t.string "lookup" | 19 | t.string "lookup" |
| 20 | t.datetime "created_at" | 20 | t.datetime "created_at" |
| 21 | t.datetime "updated_at" | 21 | t.datetime "updated_at" |
| 22 | + t.integer "answerable_id" | ||
| 23 | + t.string "answerable_type" | ||
| 22 | end | 24 | end |
| 23 | 25 | ||
| 24 | add_index "appearances", ["lookup"], :name => "index_appearances_on_lookup" | 26 | add_index "appearances", ["lookup"], :name => "index_appearances_on_lookup" |
lib/tasks/prune_db.rake
| @@ -62,5 +62,20 @@ namespace :prune_db do | @@ -62,5 +62,20 @@ namespace :prune_db do | ||
| 62 | end | 62 | end |
| 63 | end | 63 | end |
| 64 | end | 64 | end |
| 65 | + | ||
| 66 | + task(:move_vote_and_skip_ids_to_appearance => :environment) do | ||
| 67 | + Vote.find_each do |v| | ||
| 68 | + @appearance = Appearance.find(v.appearance_id) | ||
| 69 | + @appearance.answerable = v | ||
| 70 | + @appearance.save | ||
| 71 | + | ||
| 72 | + end | ||
| 73 | + Skip.find_each do |s| | ||
| 74 | + @appearance = Appearance.find(s.appearance_id) | ||
| 75 | + @appearance.answerable = s | ||
| 76 | + @appearance.save | ||
| 77 | + end | ||
| 78 | + end | ||
| 79 | + | ||
| 65 | 80 | ||
| 66 | end | 81 | end |
spec/models/appearance_spec.rb
| @@ -7,8 +7,7 @@ describe Appearance do | @@ -7,8 +7,7 @@ describe Appearance do | ||
| 7 | it {should belong_to :voter} | 7 | it {should belong_to :voter} |
| 8 | it {should belong_to :prompt} | 8 | it {should belong_to :prompt} |
| 9 | it {should belong_to :question} | 9 | it {should belong_to :question} |
| 10 | - it {should have_one :vote} | ||
| 11 | - it {should have_one :skip} | 10 | + it {should belong_to :answerable} |
| 12 | 11 | ||
| 13 | it "should create a new instance given valid attributes" do | 12 | it "should create a new instance given valid attributes" do |
| 14 | Appearance.create!(@valid_attributes) | 13 | Appearance.create!(@valid_attributes) |
spec/models/skip_spec.rb
| @@ -3,6 +3,7 @@ require File.expand_path(File.dirname(__FILE__) + '/../spec_helper') | @@ -3,6 +3,7 @@ require File.expand_path(File.dirname(__FILE__) + '/../spec_helper') | ||
| 3 | describe Skip do | 3 | describe Skip do |
| 4 | it {should belong_to :prompt} | 4 | it {should belong_to :prompt} |
| 5 | it {should belong_to :skipper} | 5 | it {should belong_to :skipper} |
| 6 | + it {should have_one :appearance} | ||
| 6 | before(:each) do | 7 | before(:each) do |
| 7 | @valid_attributes = { | 8 | @valid_attributes = { |
| 8 | 9 |
spec/models/vote_spec.rb
| @@ -6,7 +6,7 @@ describe Vote do | @@ -6,7 +6,7 @@ describe Vote do | ||
| 6 | it {should belong_to :prompt} | 6 | it {should belong_to :prompt} |
| 7 | it {should belong_to :choice} | 7 | it {should belong_to :choice} |
| 8 | it {should belong_to :loser_choice} | 8 | it {should belong_to :loser_choice} |
| 9 | - it {should belong_to :appearance} | 9 | + it {should have_one :appearance} |
| 10 | 10 | ||
| 11 | before(:each) do | 11 | before(:each) do |
| 12 | @question = Factory.create(:aoi_question) | 12 | @question = Factory.create(:aoi_question) |