Bug in cached score computation, test case

Dhruv Kapadia
1 parent f9071a75
Showing 4 changed files with 64 additions and 1 deletions Show diff stats
app/models/visitor.rb
config/environments/test.rb
db/migrate/20100326153823_update_incorrect_scores.rb
spec/models/visitor_spec.rb
@@ -25,6 +25,10 @@ class Visitor &lt; ActiveRecord::Base
  
     loser_choice = other_choices.first
     votes.create!(:question_id => prompt.question_id, :prompt_id => prompt.id, :voter_id=> self.id, :choice_id => choice.id, :loser_choice_id => loser_choice.id)
+    # Votes count is a cached value, creating the vote above will increment it in the db, but to get the proper score, we need to increment it in the current object
+    # The updated votes_count object is not saved to the db, so we don't need to worry about double counting
+    # Alternatively, we could just do choice.reload, but that results in another db read
+    choice.votes_count +=1
     choice.compute_score! #update score after win
   end
  
@@ -49,3 +49,7 @@ PAYPAL_API_LOGIN              = &#39;&#39;
 PAYPAL_API_PASSWORD           = ''
 PAYPAL_API_SIGNATURE          = ''
  
+begin
+  require 'factory_girl'
+rescue LoadError
+end
@@ -0,0 +1,20 @@
+class UpdateIncorrectScores < ActiveRecord::Migration
+  def self.up
+
+	  # some scores were computed incorrectly as a result of a bug
+	  # This only affected the cached value of the score, so we only need to recalculate the correct score
+	  # for those choices voted on in the last few days
+	  startDate = 2.days.ago
+	  votes = Vote.find(:all, :conditions => ['created_at > ?', startDate])
+	  toupdate = votes.inject(Set.new){|updatethese, v| updatethese << v.choice_id}
+	  choices = Choice.find(:all, :conditions => {'id' => toupdate.to_a})
+
+	  choices.each do |c|
+		  c.compute_score!
+	  end
+
+  end
+
+  def self.down
+  end
+end
@@ -6,6 +6,7 @@ describe Visitor do
   it {should have_many :questions}
   it {should have_many :votes}
   it {should have_many :skips}
+  it {should have_many :clicks}
  
   before(:each) do
     @aoi_clone = Factory.create(:user, :email => "pius@alum.mit.edu", :password => "password", :password_confirmation => "password", :id => 8)
@@ -29,6 +30,9 @@ describe Visitor do
  
   it "should be able to determine ownership of a question" do
     @v.owns?(Question.new).should be_false
+
+    ownedquestion = Factory.create(:question, :site => @aoi_clone, :creator=> @johndoe)
+    @johndoe.owns?(ownedquestion).should be_true
   end
  
   it "should be able to vote for a prompt" do
@@ -37,10 +41,41 @@ describe Visitor do
     v = @v.vote_for! @prompt, 0
   end
  
-  it "should be able to vote for a prompt" do
+  it "should be able to skip a prompt" do
     #@prompt = @question.prompts.first
     @prompt.should_not be_nil
     v = @v.skip! @prompt
   end
+
+  it "should accurately update score counts after vote" do
+    prev_winner_score = @lc.score
+    prev_loser_score = @rc.score
+    
+    vote = @v.vote_for! @prompt, 0
+    
+    @lc.reload
+    @rc.reload
+
+    @lc.score.should > prev_winner_score
+    @rc.score.should < prev_loser_score
+  end
+  
+  it "should accurately update win and loss totals after vote" do
+    prev_winner_wins = @lc.wins
+    prev_winner_losses = @lc.losses
+    prev_loser_losses = @rc.losses
+    prev_loser_wins = @rc.wins
+    
+    vote = @v.vote_for! @prompt, 0
+    
+    @lc.reload
+    @rc.reload
+
+    @lc.wins.should == prev_winner_wins + 1
+    @lc.losses.should == prev_winner_losses
+    @rc.losses.should ==  prev_loser_losses + 1
+    @rc.wins.should ==  prev_winner_wins
+  end
+
  
 end
...	...	@@ -25,6 +25,10 @@ class Visitor < ActiveRecord::Base
25	25
26	26	loser_choice = other_choices.first
27	27	votes.create!(:question_id => prompt.question_id, :prompt_id => prompt.id, :voter_id=> self.id, :choice_id => choice.id, :loser_choice_id => loser_choice.id)
	28	+ # Votes count is a cached value, creating the vote above will increment it in the db, but to get the proper score, we need to increment it in the current object
	29	+ # The updated votes_count object is not saved to the db, so we don't need to worry about double counting
	30	+ # Alternatively, we could just do choice.reload, but that results in another db read
	31	+ choice.votes_count +=1
28	32	choice.compute_score! #update score after win
29	33	end
30	34
...	...
...	...	@@ -49,3 +49,7 @@ PAYPAL_API_LOGIN = ''
49	49	PAYPAL_API_PASSWORD = ''
50	50	PAYPAL_API_SIGNATURE = ''
51	51
	52	+begin
	53	+ require 'factory_girl'
	54	+rescue LoadError
	55	+end
...	...
...	...	@@ -0,0 +1,20 @@
	1	+class UpdateIncorrectScores < ActiveRecord::Migration
	2	+ def self.up
	3	+
	4	+ # some scores were computed incorrectly as a result of a bug
	5	+ # This only affected the cached value of the score, so we only need to recalculate the correct score
	6	+ # for those choices voted on in the last few days
	7	+ startDate = 2.days.ago
	8	+ votes = Vote.find(:all, :conditions => ['created_at > ?', startDate])
	9	+ toupdate = votes.inject(Set.new){\|updatethese, v\| updatethese << v.choice_id}
	10	+ choices = Choice.find(:all, :conditions => {'id' => toupdate.to_a})
	11	+
	12	+ choices.each do \|c\|
	13	+ c.compute_score!
	14	+ end
	15	+
	16	+ end
	17	+
	18	+ def self.down
	19	+ end
	20	+end
...	...
...	...	@@ -6,6 +6,7 @@ describe Visitor do
6	6	it {should have_many :questions}
7	7	it {should have_many :votes}
8	8	it {should have_many :skips}
	9	+ it {should have_many :clicks}
9	10
10	11	before(:each) do
11	12	@aoi_clone = Factory.create(:user, :email => "pius@alum.mit.edu", :password => "password", :password_confirmation => "password", :id => 8)
...	...	@@ -29,6 +30,9 @@ describe Visitor do
29	30
30	31	it "should be able to determine ownership of a question" do
31	32	@v.owns?(Question.new).should be_false
	33	+
	34	+ ownedquestion = Factory.create(:question, :site => @aoi_clone, :creator=> @johndoe)
	35	+ @johndoe.owns?(ownedquestion).should be_true
32	36	end
33	37
34	38	it "should be able to vote for a prompt" do
...	...	@@ -37,10 +41,41 @@ describe Visitor do
37	41	v = @v.vote_for! @prompt, 0
38	42	end
39	43
40		- it "should be able to vote for a prompt" do
	44	+ it "should be able to skip a prompt" do
41	45	#@prompt = @question.prompts.first
42	46	@prompt.should_not be_nil
43	47	v = @v.skip! @prompt
44	48	end
	49	+
	50	+ it "should accurately update score counts after vote" do
	51	+ prev_winner_score = @lc.score
	52	+ prev_loser_score = @rc.score
	53	+
	54	+ vote = @v.vote_for! @prompt, 0
	55	+
	56	+ @lc.reload
	57	+ @rc.reload
	58	+
	59	+ @lc.score.should > prev_winner_score
	60	+ @rc.score.should < prev_loser_score
	61	+ end
	62	+
	63	+ it "should accurately update win and loss totals after vote" do
	64	+ prev_winner_wins = @lc.wins
	65	+ prev_winner_losses = @lc.losses
	66	+ prev_loser_losses = @rc.losses
	67	+ prev_loser_wins = @rc.wins
	68	+
	69	+ vote = @v.vote_for! @prompt, 0
	70	+
	71	+ @lc.reload
	72	+ @rc.reload
	73	+
	74	+ @lc.wins.should == prev_winner_wins + 1
	75	+ @lc.losses.should == prev_winner_losses
	76	+ @rc.losses.should == prev_loser_losses + 1
	77	+ @rc.wins.should == prev_winner_wins
	78	+ end
	79	+
45	80
46	81	end
...	...