Commit 51e3a6930d27d7f06c32244b197895dff4151b0e
1 parent
d0820545
Exists in
master
and in
1 other branch
Handle updating counter cache values when invalid flag is used
Showing
3 changed files
with
108 additions
and
8 deletions
Show diff stats
app/models/vote.rb
| @@ -23,6 +23,7 @@ class Vote < ActiveRecord::Base | @@ -23,6 +23,7 @@ class Vote < ActiveRecord::Base | ||
| 23 | serialize :tracking | 23 | serialize :tracking |
| 24 | 24 | ||
| 25 | after_create :update_winner_choice, :update_loser_choice | 25 | after_create :update_winner_choice, :update_loser_choice |
| 26 | + after_save :update_counter_caches_based_on_flags | ||
| 26 | 27 | ||
| 27 | def update_winner_choice | 28 | def update_winner_choice |
| 28 | choice.reload # make sure we're using updated counter values | 29 | choice.reload # make sure we're using updated counter values |
| @@ -33,4 +34,22 @@ class Vote < ActiveRecord::Base | @@ -33,4 +34,22 @@ class Vote < ActiveRecord::Base | ||
| 33 | loser_choice.reload | 34 | loser_choice.reload |
| 34 | loser_choice.compute_score! | 35 | loser_choice.compute_score! |
| 35 | end | 36 | end |
| 37 | + | ||
| 38 | + # this is necessary to handle counter cache, at least until the following patch is accepted: | ||
| 39 | + # https://rails.lighthouseapp.com/projects/8994/tickets/3521-patch-add-conditional-counter-cache | ||
| 40 | + def update_counter_caches_based_on_flags | ||
| 41 | + if valid_record_changed? | ||
| 42 | + if valid_record | ||
| 43 | + Question.increment_counter(:votes_count, self.question_id) | ||
| 44 | + Prompt.increment_counter(:votes_count, self.prompt_id) | ||
| 45 | + Choice.increment_counter(:wins, self.choice_id) | ||
| 46 | + Choice.increment_counter(:losses, self.loser_choice_id) | ||
| 47 | + else | ||
| 48 | + Question.decrement_counter(:votes_count, self.question_id) | ||
| 49 | + Prompt.decrement_counter(:votes_count, self.prompt_id) | ||
| 50 | + Choice.decrement_counter(:wins, self.choice_id) | ||
| 51 | + Choice.decrement_counter(:losses, self.loser_choice_id) | ||
| 52 | + end | ||
| 53 | + end | ||
| 54 | + end | ||
| 36 | end | 55 | end |
spec/models/visitor_spec.rb
| @@ -126,6 +126,7 @@ describe Visitor do | @@ -126,6 +126,7 @@ describe Visitor do | ||
| 126 | 126 | ||
| 127 | valid_skip = @visitor.skip!(allparams) | 127 | valid_skip = @visitor.skip!(allparams) |
| 128 | @visitor.skips.count.should == 1 | 128 | @visitor.skips.count.should == 1 |
| 129 | + @visitor.skips.size.should == 1 | ||
| 129 | @appearance.reload.answerable.should == valid_skip | 130 | @appearance.reload.answerable.should == valid_skip |
| 130 | 131 | ||
| 131 | # we need to reset because vote_for deletes keys from the params | 132 | # we need to reset because vote_for deletes keys from the params |
| @@ -137,6 +138,7 @@ describe Visitor do | @@ -137,6 +138,7 @@ describe Visitor do | ||
| 137 | invalid_skip.validity_information.should == "Appearance #{@appearance.id} already answered" | 138 | invalid_skip.validity_information.should == "Appearance #{@appearance.id} already answered" |
| 138 | @appearance.reload.answerable.should == valid_skip | 139 | @appearance.reload.answerable.should == valid_skip |
| 139 | @visitor.reload.skips.count.should == 1 | 140 | @visitor.reload.skips.count.should == 1 |
| 141 | + @visitor.reload.skips.size.should == 1 | ||
| 140 | end | 142 | end |
| 141 | 143 | ||
| 142 | it "should mark a vote as invalid if submitted with an already answered appearance" do | 144 | it "should mark a vote as invalid if submitted with an already answered appearance" do |
| @@ -146,6 +148,7 @@ describe Visitor do | @@ -146,6 +148,7 @@ describe Visitor do | ||
| 146 | 148 | ||
| 147 | valid_vote = @visitor.vote_for!(allparams) | 149 | valid_vote = @visitor.vote_for!(allparams) |
| 148 | @visitor.votes.count.should == 1 | 150 | @visitor.votes.count.should == 1 |
| 151 | + @question.reload.votes.size.should == 1 | ||
| 149 | @appearance.reload.answerable.should == valid_vote | 152 | @appearance.reload.answerable.should == valid_vote |
| 150 | 153 | ||
| 151 | # we need to reset because vote_for deletes keys from the params | 154 | # we need to reset because vote_for deletes keys from the params |
| @@ -157,6 +160,7 @@ describe Visitor do | @@ -157,6 +160,7 @@ describe Visitor do | ||
| 157 | invalid_vote.validity_information.should == "Appearance #{@appearance.id} already answered" | 160 | invalid_vote.validity_information.should == "Appearance #{@appearance.id} already answered" |
| 158 | @appearance.reload.answerable.should == valid_vote | 161 | @appearance.reload.answerable.should == valid_vote |
| 159 | @visitor.reload.votes.count.should == 1 | 162 | @visitor.reload.votes.count.should == 1 |
| 163 | + @question.reload.votes.size.should == 1 #test counter cache works as well | ||
| 160 | end | 164 | end |
| 161 | 165 | ||
| 162 | it "should accurately update score counts after vote" do | 166 | it "should accurately update score counts after vote" do |
spec/models/vote_spec.rb
| @@ -11,6 +11,10 @@ describe Vote do | @@ -11,6 +11,10 @@ describe Vote do | ||
| 11 | before(:each) do | 11 | before(:each) do |
| 12 | @question = Factory.create(:aoi_question) | 12 | @question = Factory.create(:aoi_question) |
| 13 | @prompt = @question.prompts.first | 13 | @prompt = @question.prompts.first |
| 14 | + @required_params = {:question => @question, :prompt => @prompt, | ||
| 15 | + :choice => @prompt.left_choice, | ||
| 16 | + :voter=> @question.site.default_visitor, | ||
| 17 | + :loser_choice => @prompt.right_choice} | ||
| 14 | end | 18 | end |
| 15 | 19 | ||
| 16 | it "should create a new instance with factory girl" do | 20 | it "should create a new instance with factory girl" do |
| @@ -95,16 +99,89 @@ describe Vote do | @@ -95,16 +99,89 @@ describe Vote do | ||
| 95 | end | 99 | end |
| 96 | 100 | ||
| 97 | it "should allow default valid_record behavior to be overriden by default" do | 101 | it "should allow default valid_record behavior to be overriden by default" do |
| 98 | - required_params = {:question => @question, :prompt => @prompt, | ||
| 99 | - :choice => @prompt.left_choice, | ||
| 100 | - :voter=> @question.site.default_visitor, | ||
| 101 | - :loser_choice => @prompt.right_choice} | ||
| 102 | - | ||
| 103 | - vote = Vote.create!(required_params) | 102 | + vote = Vote.create!(@required_params) |
| 104 | vote.valid_record.should be_true | 103 | vote.valid_record.should be_true |
| 105 | 104 | ||
| 106 | - | ||
| 107 | - vote = Vote.create!(required_params.merge!(:valid_record => false)) | 105 | + vote = Vote.create!(@required_params.merge!(:valid_record => false)) |
| 108 | vote.valid_record.should be_false | 106 | vote.valid_record.should be_false |
| 109 | end | 107 | end |
| 108 | + | ||
| 109 | + it "should update counter cache on question when vote is flagged as invalid" do | ||
| 110 | + vote = Factory.create(:vote, :question => @question) | ||
| 111 | + @question.reload | ||
| 112 | + @question.votes.size.should == 1 | ||
| 113 | + @question.votes_count.should == 1 | ||
| 114 | + | ||
| 115 | + vote.valid_record = false; | ||
| 116 | + vote.save | ||
| 117 | + | ||
| 118 | + @question.reload | ||
| 119 | + @question.votes.size.should == 0 | ||
| 120 | + @question.votes_count.should == 0 | ||
| 121 | + | ||
| 122 | + the_vote = Factory.create(:vote, :question => @question) | ||
| 123 | + the_vote.validity_information = "blah blah blah" | ||
| 124 | + the_vote.save | ||
| 125 | + | ||
| 126 | + @question.reload.votes.size.should == 1 | ||
| 127 | + | ||
| 128 | + end | ||
| 129 | + it "should update counter cache on question when vote is created with invalid flag" do | ||
| 130 | + vote = Vote.create!(@required_params) | ||
| 131 | + @question.reload.votes.size.should == 1 | ||
| 132 | + | ||
| 133 | + vote = Vote.create!(@required_params.merge!(:valid_record => false)) | ||
| 134 | + @question.reload.votes.size.should == 1 | ||
| 135 | + end | ||
| 136 | + | ||
| 137 | + it "should update counter cache on choice after changing valid status" do | ||
| 138 | + vote = Factory.create(:vote, :question => @question, :prompt => @prompt, | ||
| 139 | + :choice => @prompt.left_choice) | ||
| 140 | + | ||
| 141 | + @prompt.left_choice.reload | ||
| 142 | + @prompt.left_choice.votes.size.should == 1 | ||
| 143 | + @prompt.left_choice.wins.should == 1 | ||
| 144 | + | ||
| 145 | + vote.valid_record = false | ||
| 146 | + vote.save | ||
| 147 | + @prompt.left_choice.reload | ||
| 148 | + @prompt.left_choice.votes.size.should == 0 | ||
| 149 | + @prompt.left_choice.wins.should == 0 | ||
| 150 | + | ||
| 151 | + end | ||
| 152 | + it "should update counter cache on prompt after changing valid status" do | ||
| 153 | + vote = Factory.create(:vote, :question => @question, :prompt => @prompt) | ||
| 154 | + | ||
| 155 | + @prompt.reload | ||
| 156 | + @prompt.votes.size.should == 1 | ||
| 157 | + @prompt.votes_count.should == 1 | ||
| 158 | + | ||
| 159 | + vote.valid_record = false | ||
| 160 | + vote.save | ||
| 161 | + @prompt.reload | ||
| 162 | + @prompt.votes.size.should == 0 | ||
| 163 | + @prompt.votes_count.should == 0 | ||
| 164 | + | ||
| 165 | + end | ||
| 166 | + | ||
| 167 | + it "should update counter cache on loser_choice after invalid is changed" do | ||
| 168 | + vote = Factory.create(:vote, :question => @question, :prompt => @prompt, | ||
| 169 | + :choice => @prompt.left_choice, | ||
| 170 | + :loser_choice => @prompt.right_choice) | ||
| 171 | + | ||
| 172 | + @prompt.right_choice.reload | ||
| 173 | + @prompt.right_choice.votes.size.should == 0 | ||
| 174 | + @prompt.right_choice.wins.should == 0 | ||
| 175 | + @prompt.right_choice.losses.should == 1 | ||
| 176 | + | ||
| 177 | + vote.valid_record = false | ||
| 178 | + vote.save | ||
| 179 | + | ||
| 180 | + @prompt.right_choice.reload | ||
| 181 | + @prompt.right_choice.votes.size.should == 0 | ||
| 182 | + @prompt.right_choice.wins.should == 0 | ||
| 183 | + @prompt.right_choice.losses.should == 0 | ||
| 184 | + | ||
| 185 | + end | ||
| 186 | + | ||
| 110 | end | 187 | end |