Commit ee975a4eb434c06b8fe208acd097e91a3fdde8c5
1 parent
1d4ab5ab
Exists in
master
and in
1 other branch
Refactoring part of question_optional_information to support back button
Showing
2 changed files
with
28 additions
and
15 deletions
Show diff stats
app/models/question.rb
| @@ -140,34 +140,32 @@ class Question < ActiveRecord::Base | @@ -140,34 +140,32 @@ class Question < ActiveRecord::Base | ||
| 140 | current_user = self.site | 140 | current_user = self.site |
| 141 | 141 | ||
| 142 | if params[:with_prompt] | 142 | if params[:with_prompt] |
| 143 | - @prompt = choose_prompt(:algorithm => params[:algorithm]) | ||
| 144 | - result.merge!({:picked_prompt_id => @prompt.id}) | ||
| 145 | - | ||
| 146 | - if params[:with_appearance] && visitor_identifier.present? | 143 | + |
| 144 | + if params[:with_appearance] && visitor_identifier.present? | ||
| 147 | visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier) | 145 | visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier) |
| 148 | 146 | ||
| 149 | last_appearance = visitor.appearances.find(:first, :conditions => {:question_id => self.id}, | 147 | last_appearance = visitor.appearances.find(:first, :conditions => {:question_id => self.id}, |
| 150 | :order => 'created_at DESC', | 148 | :order => 'created_at DESC', |
| 151 | :limit => 1) | 149 | :limit => 1) |
| 152 | - if last_appearance.nil?|| last_appearance.answered? | 150 | + if last_appearance.nil?|| last_appearance.answered? || !last_appearance.prompt.active? |
| 151 | + @prompt = choose_prompt(:algorithm => params[:algorithm]) | ||
| 153 | @appearance = current_user.record_appearance(visitor, @prompt) | 152 | @appearance = current_user.record_appearance(visitor, @prompt) |
| 154 | else | 153 | else |
| 155 | #only display a new prompt and new appearance if the old prompt has not been voted on | 154 | #only display a new prompt and new appearance if the old prompt has not been voted on |
| 156 | @appearance = last_appearance | 155 | @appearance = last_appearance |
| 157 | - possible_prompt = @appearance.prompt | ||
| 158 | - | ||
| 159 | - #edge case, it's possible that the previous prompt has become deactivated in the elapsed time | ||
| 160 | - if possible_prompt.active? | ||
| 161 | - result.merge!({:picked_prompt_id => possible_prompt.id}) | ||
| 162 | - else | ||
| 163 | - @appearance = current_user.record_appearance(visitor, @prompt) | ||
| 164 | - end | ||
| 165 | - end | 156 | + @prompt= @appearance.prompt |
| 157 | + end | ||
| 158 | + | ||
| 166 | result.merge!({:appearance_id => @appearance.lookup}) | 159 | result.merge!({:appearance_id => @appearance.lookup}) |
| 167 | else | 160 | else |
| 168 | # throw some error | 161 | # throw some error |
| 162 | + end | ||
| 163 | + | ||
| 164 | + if !@prompt | ||
| 165 | + @prompt = choose_prompt(:algorithm => params[:algorithm]) | ||
| 169 | end | 166 | end |
| 170 | - end | 167 | + result.merge!({:picked_prompt_id => @prompt.id}) |
| 168 | + end | ||
| 171 | 169 | ||
| 172 | if params[:with_visitor_stats] | 170 | if params[:with_visitor_stats] |
| 173 | visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier) | 171 | visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier) |
| @@ -435,6 +433,11 @@ class Question < ActiveRecord::Base | @@ -435,6 +433,11 @@ class Question < ActiveRecord::Base | ||
| 435 | $redis.get(self.pq_key + "_" + date.to_s + "_"+ "hits") | 433 | $redis.get(self.pq_key + "_" + date.to_s + "_"+ "hits") |
| 436 | end | 434 | end |
| 437 | 435 | ||
| 436 | + def reset_cache_tracking_keys(date) | ||
| 437 | + $redis.del(self.pq_key + "_" + date.to_s + "_"+ "misses") | ||
| 438 | + $redis.del(self.pq_key + "_" + date.to_s + "_"+ "hits") | ||
| 439 | + end | ||
| 440 | + | ||
| 438 | 441 | ||
| 439 | def expire_prompt_cache_tracking_keys(date, expire_time = 24*60*60 * 3) # default expires in three days | 442 | def expire_prompt_cache_tracking_keys(date, expire_time = 24*60*60 * 3) # default expires in three days |
| 440 | $redis.expire(self.pq_key + "_" + date.to_s + "_"+ "hits", expire_time) | 443 | $redis.expire(self.pq_key + "_" + date.to_s + "_"+ "hits", expire_time) |
spec/models/question_spec.rb
| @@ -112,6 +112,16 @@ describe Question do | @@ -112,6 +112,16 @@ describe Question do | ||
| 112 | @question_optional_information[:picked_prompt_id].should == saved_prompt_id | 112 | @question_optional_information[:picked_prompt_id].should == saved_prompt_id |
| 113 | end | 113 | end |
| 114 | 114 | ||
| 115 | + it "should properly handle tracking the prompt cache hit rate when returning the same appearance when a visitor requests two prompts without voting" do | ||
| 116 | + params = {:id => 124, :with_visitor_stats=> true, :visitor_identifier => "jim", :with_prompt => true, :with_appearance => true} | ||
| 117 | + @question.clear_prompt_queue | ||
| 118 | + @question.reset_cache_tracking_keys(Date.today) | ||
| 119 | + @question.get_optional_information(params) | ||
| 120 | + @question.get_prompt_cache_misses(Date.today).should == "1" | ||
| 121 | + @question.get_optional_information(params) | ||
| 122 | + @question.get_prompt_cache_misses(Date.today).should == "1" | ||
| 123 | + end | ||
| 124 | + | ||
| 115 | it "should auto create ideas when 'ideas' attribute is set" do | 125 | it "should auto create ideas when 'ideas' attribute is set" do |
| 116 | @question = Factory.build(:question) | 126 | @question = Factory.build(:question) |
| 117 | @question.ideas = %w(one two three) | 127 | @question.ideas = %w(one two three) |