Rewriting skips model, providing support

Dhruv Kapadia
1 parent 18da412d
Showing 9 changed files with 124 additions and 31 deletions Show diff stats
app/controllers/prompts_controller.rb
app/models/user.rb
app/models/visitor.rb
db/migrate/20100428190803_rename_skips_table.rb
db/migrate/20100428190907_create_skips_model.rb
db/migrate/20100428211749_add_skip_reason_to_skips.rb
spec/controllers/prompts_controller_spec.rb
spec/models/user_spec.rb
spec/models/visitor_spec.rb
@@ -53,9 +53,7 @@ class PromptsController &lt; InheritedResources::Base
     @prompt = @question.prompts.find(params[:id])
     time_viewed = params['params']['time_viewed']
-
     visitor_identifier = params['params']['auto']
-    
     appearance_lookup = params['params']['appearance_lookup']
     case direction
@@ -71,6 +69,7 @@ class PromptsController &lt; InheritedResources::Base
     respond_to do |format|
       if successful
 	#catchup_marketplace_ids = [120, 117, 1, 116]
+	#TODO refactor into question model
 	if @question.uses_catchup?
 	      logger.info("Question #{@question.id} is using catchup algorithm!")
 	      next_prompt = @question.pop_prompt_queue
@@ -125,12 +124,45 @@ class PromptsController &lt; InheritedResources::Base
     authenticate
     logger.info "#{current_user.inspect} is skipping."
     @question = Question.find(params[:question_id])
-    @prompt = @question.prompts.find(params[:id], :include => [{ :left_choice => :item }, { :right_choice => :item }])
+    @prompt = @question.prompts.find(params[:id]) #, :include => [{ :left_choice => :item }, { :right_choice => :item }])
+
+    time_viewed = params['params']['time_viewed']
+    raise "time_viewed cannot be nil" if time_viewed.nil?
+
+    visitor_identifier = params['params']['auto']
+    raise "visitor identifier cannot be nil" if visitor_identifier.nil?
+    appearance_lookup = params['params']['appearance_lookup']
+    raise "appearance_lookup cannot be nil" if appearance_lookup.nil?
+
+    skip_reason = params['params']['skip_reason'] # optional parameter
     respond_to do |format|
-      if @skip = current_user.record_skip(params['params']['auto'], @prompt)
-        format.xml { render :xml =>  @question.picked_prompt.to_xml(:methods => [:left_choice_text, :right_choice_text]), :status => :ok }
+      if @skip = current_user.record_skip(visitor_identifier, appearance_lookup, @prompt, time_viewed, :skip_reason => skip_reason)
+
+	if @question.uses_catchup?
+	      logger.info("Question #{@question.id} is using catchup algorithm!")
+	      @next_prompt = @question.pop_prompt_queue
+	      if @next_prompt.nil?
+		      logger.info("Catchup prompt cache miss! Nothing in prompt_queue")
+		      @next_prompt = @question.catchup_choose_prompt
+	      end
+	      @question.send_later :add_prompt_to_queue
+	else
+		@next_prompt = @question.picked_prompt
+	end
+
+
+        visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier)
+        @a = current_user.record_appearance(visitor, @next_prompt)
+         
+	appearance_id = Proc.new { |options| options[:builder].tag!('appearance_id', @a.lookup) }
+      
+	visitor_votes = Proc.new { |options| options[:builder].tag!('visitor_votes', visitor.votes.count(:conditions => {:question_id => @question.id})) }
+        visitor_ideas = Proc.new { |options| options[:builder].tag!('visitor_ideas', visitor.items.count) }
+
+
+        format.xml { render :xml =>  @question.picked_prompt.to_xml(:procs => [appearance_id, visitor_votes, visitor_ideas],:methods => [:left_choice_text, :right_choice_text]), :status => :ok }
         format.json { render :json => @question.picked_prompt.to_json, :status => :ok }
       else
         format.xml { render :xml => @skip, :status => :unprocessable_entity }
@@ -45,10 +45,9 @@ class User &lt; ActiveRecord::Base
   end
-  def record_skip(visitor_identifier, prompt)
+  def record_skip(visitor_identifier, appearance_lookup, prompt, time_viewed, options = {})
     visitor = visitors.find_or_create_by_identifier(visitor_identifier)
-    question = prompt.question
-    visitor.skip!(prompt)
+    visitor.skip!(appearance_lookup, prompt, time_viewed, options)
   end
   def activate_question(question_id, options)
@@ -18,6 +18,7 @@ class Visitor &lt; ActiveRecord::Base
   def vote_for!(appearance_lookup, prompt, ordinality, time_viewed)
     @a = Appearance.find_by_lookup(appearance_lookup)
+    #make votefor fail if we cant find the appearance
     choices = prompt.choices
     choice = choices[ordinality] #we need to guarantee that the choices are in the right order (by position)
     other_choices = choices - [choice]
@@ -37,7 +38,12 @@ class Visitor &lt; ActiveRecord::Base
   end
-  def skip!(prompt)
-    prompt_skip = skips.create!(:prompt => prompt)
+  def skip!(appearance_lookup, prompt, time_viewed, options = {})
+    @a = Appearance.find_by_lookup(appearance_lookup)
+    
+    skip_create_options  = { :question_id => prompt.question_id, :prompt_id => prompt.id, :skipper_id=> self.id, :time_viewed => time_viewed, :appearance_id => @a.id} 
+
+    prompt_skip = skips.create!(skip_create_options.merge(options))
+
   end
 end
@@ -0,0 +1,9 @@
+class RenameSkipsTable < ActiveRecord::Migration
+  def self.up
+	  rename_table :skips, :oldskips
+  end
+
+  def self.down
+	  rename_table :oldskips, :skips
+  end
+end
@@ -0,0 +1,19 @@
+class CreateSkipsModel < ActiveRecord::Migration
+  def self.up
+    create_table :skips do |table|
+      table.text :tracking
+      table.integer :site_id
+      table.integer :skipper_id
+      table.integer :question_id
+      table.integer :prompt_id
+      table.integer :appearance_id
+      table.integer :time_viewed #msecs
+      table.timestamps
+    end
+
+  end
+
+  def self.down
+    drop_table :skips
+  end
+end
@@ -0,0 +1,9 @@
+class AddSkipReasonToSkips < ActiveRecord::Migration
+  def self.up
+	  add_column :skips, :skip_reason, :string
+  end
+
+  def self.down
+	  drop_column :skips, :skip_reason
+  end
+end
 require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')
 describe PromptsController do
+   def sign_in_as(user)
+     @controller.current_user = user
+     return user
+   end
+  #   
+   before(:each) do
+    @aoi_clone = Factory.create(:user)
+    sign_in_as(@user = Factory(:email_confirmed_user))
+    @johndoe = Factory.create(:visitor, :identifier => 'johndoe', :site => @aoi_clone)
+    @question = Factory.create(:question, :name => 'which do you like better?', :site => @aoi_clone, :creator => @aoi_clone.default_visitor)
+    @lc = Factory.create(:choice, :question => @question, :creator => @johndoe, :data => 'hello gorgeous')
+    @rc = Factory.create(:choice, :question => @question, :creator => @johndoe, :data => 'goodbye gorgeous')
+    @prompt = Factory.create(:prompt, :question => @question, :tracking => 'sample', :left_choice => @lc, :right_choice => @rc)
+    
+    @visitor = @aoi_clone.visitors.find_or_create_by_identifier("test_visitor_identifier")
+    @appearance = @aoi_clone.record_appearance(@visitor, @prompt)
+   end
+  # 
-  def mock_prompt(stubs={})
-    @mock_prompt ||= mock_model(Prompt, stubs)
-  end
-
-  before(:each) do 
-          @aoi_clone = Factory.create(:user)
-          @question = Factory.create(:question, :site => @aoi_clone, :creator => @aoi_clone.default_visitor)
-  end
-
-  describe "GET index" do
-    it "assigns all prompts as @prompts" do
+#  describe "GET index" do
+#    it "assigns all prompts as @prompts" do
 #      Question.stub!(:find).with(:all).and_return(@question)
 #      Question.stub!(:prompts).with(:all).and_return([mock_prompt])
-      get :index, :question_id => @question.id
-      assigns[:prompts].should == [mock_prompt]
-    end
-  end
+#      get :index, :question_id => @question.id
+#      assigns[:prompts].should == [@prompt]
+#    end
+#  end
   describe "GET show" do
     it "assigns the requested prompt as @prompt" do
-#      Question.stub!(:find).with(:all).and_return(@question)
-#      Prompt.stub!(:find).with("37").and_return(mock_prompt)
-      get :show, :id => "37", :question_id => @question.id
-      assigns[:prompt].should equal(mock_prompt)
+      get :show, :id => @prompt.id, :question_id => @question.id
+      assigns[:prompt].should == @prompt
     end
   end
+  
+  describe "POST skip" do
+    it "records a skip, responds with next prompt" do
+      post :skip, :id => @prompt.id, :question_id => @question.id, :params => {:auto => @visitor, :time_viewed => 30, :appearance_lookup => @appearance.lookup}
+      assigns[:next_prompt].should_not == @prompt
+      assigns[:next_prompt].should_not be_nil
+      assigns[:a].should_not be_nil
+      assigns[:a].should_not == @appearance
+      assigns[:skip].should_not be_nil
+    end
+  end
+
 end
@@ -47,7 +47,7 @@ describe User do
   end
   it "should be able to record a visitor's skip" do
-    s = @aoi_clone.record_skip("johnnydoe", @prompt)
+    s = @aoi_clone.record_skip("johnnydoe", @appearance.lookup, @prompt, 340)
   end
 end
@@ -47,7 +47,7 @@ describe Visitor do
   it "should be able to skip a prompt" do
     #@prompt = @question.prompts.first
     @prompt.should_not be_nil
-    v = @v.skip! @prompt
+    v = @v.skip! @appearance.lookup, @prompt, 304
   end
   it "should accurately update score counts after vote" do
	@@ -53,9 +53,7 @@ class PromptsController < InheritedResources::Base		@@ -53,9 +53,7 @@ class PromptsController < InheritedResources::Base
53	@prompt = @question.prompts.find(params[:id])	53	@prompt = @question.prompts.find(params[:id])
54		54
55	time_viewed = params['params']['time_viewed']	55	time_viewed = params['params']['time_viewed']
56	-
57	visitor_identifier = params['params']['auto']	56	visitor_identifier = params['params']['auto']
58	-
59	appearance_lookup = params['params']['appearance_lookup']	57	appearance_lookup = params['params']['appearance_lookup']
60		58
61	case direction	59	case direction
	@@ -71,6 +69,7 @@ class PromptsController < InheritedResources::Base		@@ -71,6 +69,7 @@ class PromptsController < InheritedResources::Base
71	respond_to do \|format\|	69	respond_to do \|format\|
72	if successful	70	if successful
73	#catchup_marketplace_ids = [120, 117, 1, 116]	71	#catchup_marketplace_ids = [120, 117, 1, 116]
		72	+ #TODO refactor into question model
74	if @question.uses_catchup?	73	if @question.uses_catchup?
75	logger.info("Question #{@question.id} is using catchup algorithm!")	74	logger.info("Question #{@question.id} is using catchup algorithm!")
76	next_prompt = @question.pop_prompt_queue	75	next_prompt = @question.pop_prompt_queue
	@@ -125,12 +124,45 @@ class PromptsController < InheritedResources::Base		@@ -125,12 +124,45 @@ class PromptsController < InheritedResources::Base
125	authenticate	124	authenticate
126	logger.info "#{current_user.inspect} is skipping."	125	logger.info "#{current_user.inspect} is skipping."
127	@question = Question.find(params[:question_id])	126	@question = Question.find(params[:question_id])
128	- @prompt = @question.prompts.find(params[:id], :include => [{ :left_choice => :item }, { :right_choice => :item }])	127	+ @prompt = @question.prompts.find(params[:id]) #, :include => [{ :left_choice => :item }, { :right_choice => :item }])
		128	+
		129	+ time_viewed = params['params']['time_viewed']
		130	+ raise "time_viewed cannot be nil" if time_viewed.nil?
		131	+
		132	+ visitor_identifier = params['params']['auto']
		133	+ raise "visitor identifier cannot be nil" if visitor_identifier.nil?
		134	+ appearance_lookup = params['params']['appearance_lookup']
		135	+ raise "appearance_lookup cannot be nil" if appearance_lookup.nil?
		136	+
		137	+ skip_reason = params['params']['skip_reason'] # optional parameter
129		138
130		139
131	respond_to do \|format\|	140	respond_to do \|format\|
132	- if @skip = current_user.record_skip(params['params']['auto'], @prompt)
133	- format.xml { render :xml => @question.picked_prompt.to_xml(:methods => [:left_choice_text, :right_choice_text]), :status => :ok }	141	+ if @skip = current_user.record_skip(visitor_identifier, appearance_lookup, @prompt, time_viewed, :skip_reason => skip_reason)
		142	+
		143	+ if @question.uses_catchup?
		144	+ logger.info("Question #{@question.id} is using catchup algorithm!")
		145	+ @next_prompt = @question.pop_prompt_queue
		146	+ if @next_prompt.nil?
		147	+ logger.info("Catchup prompt cache miss! Nothing in prompt_queue")
		148	+ @next_prompt = @question.catchup_choose_prompt
		149	+ end
		150	+ @question.send_later :add_prompt_to_queue
		151	+ else
		152	+ @next_prompt = @question.picked_prompt
		153	+ end
		154	+
		155	+
		156	+ visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier)
		157	+ @a = current_user.record_appearance(visitor, @next_prompt)
		158	+
		159	+ appearance_id = Proc.new { \|options\| options[:builder].tag!('appearance_id', @a.lookup) }
		160	+
		161	+ visitor_votes = Proc.new { \|options\| options[:builder].tag!('visitor_votes', visitor.votes.count(:conditions => {:question_id => @question.id})) }
		162	+ visitor_ideas = Proc.new { \|options\| options[:builder].tag!('visitor_ideas', visitor.items.count) }
		163	+
		164	+
		165	+ format.xml { render :xml => @question.picked_prompt.to_xml(:procs => [appearance_id, visitor_votes, visitor_ideas],:methods => [:left_choice_text, :right_choice_text]), :status => :ok }
134	format.json { render :json => @question.picked_prompt.to_json, :status => :ok }	166	format.json { render :json => @question.picked_prompt.to_json, :status => :ok }
135	else	167	else
136	format.xml { render :xml => @skip, :status => :unprocessable_entity }	168	format.xml { render :xml => @skip, :status => :unprocessable_entity }
@@ -0,0 +1,9 @@		@@ -0,0 +1,9 @@
	1	+class RenameSkipsTable < ActiveRecord::Migration
	2	+ def self.up
	3	+ rename_table :skips, :oldskips
	4	+ end
	5	+
	6	+ def self.down
	7	+ rename_table :oldskips, :skips
	8	+ end
	9	+end
@@ -0,0 +1,19 @@		@@ -0,0 +1,19 @@
	1	+class CreateSkipsModel < ActiveRecord::Migration
	2	+ def self.up
	3	+ create_table :skips do \|table\|
	4	+ table.text :tracking
	5	+ table.integer :site_id
	6	+ table.integer :skipper_id
	7	+ table.integer :question_id
	8	+ table.integer :prompt_id
	9	+ table.integer :appearance_id
	10	+ table.integer :time_viewed #msecs
	11	+ table.timestamps
	12	+ end
	13	+
	14	+ end
	15	+
	16	+ def self.down
	17	+ drop_table :skips
	18	+ end
	19	+end
@@ -0,0 +1,9 @@		@@ -0,0 +1,9 @@
	1	+class AddSkipReasonToSkips < ActiveRecord::Migration
	2	+ def self.up
	3	+ add_column :skips, :skip_reason, :string
	4	+ end
	5	+
	6	+ def self.down
	7	+ drop_column :skips, :skip_reason
	8	+ end
	9	+end
1	require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')	1	require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')
2		2
3	describe PromptsController do	3	describe PromptsController do
		4	+ def sign_in_as(user)
		5	+ @controller.current_user = user
		6	+ return user
		7	+ end
		8	+ #
		9	+ before(:each) do
		10	+ @aoi_clone = Factory.create(:user)
		11	+ sign_in_as(@user = Factory(:email_confirmed_user))
		12	+ @johndoe = Factory.create(:visitor, :identifier => 'johndoe', :site => @aoi_clone)
		13	+ @question = Factory.create(:question, :name => 'which do you like better?', :site => @aoi_clone, :creator => @aoi_clone.default_visitor)
		14	+ @lc = Factory.create(:choice, :question => @question, :creator => @johndoe, :data => 'hello gorgeous')
		15	+ @rc = Factory.create(:choice, :question => @question, :creator => @johndoe, :data => 'goodbye gorgeous')
		16	+ @prompt = Factory.create(:prompt, :question => @question, :tracking => 'sample', :left_choice => @lc, :right_choice => @rc)
		17	+
		18	+ @visitor = @aoi_clone.visitors.find_or_create_by_identifier("test_visitor_identifier")
		19	+ @appearance = @aoi_clone.record_appearance(@visitor, @prompt)
		20	+ end
		21	+ #
4		22
5	- def mock_prompt(stubs={})
6	- @mock_prompt \|\|= mock_model(Prompt, stubs)
7	- end
8	-
9	- before(:each) do
10	- @aoi_clone = Factory.create(:user)
11	- @question = Factory.create(:question, :site => @aoi_clone, :creator => @aoi_clone.default_visitor)
12	- end
13	-
14	- describe "GET index" do
15	- it "assigns all prompts as @prompts" do	23	+# describe "GET index" do
		24	+# it "assigns all prompts as @prompts" do
16	# Question.stub!(:find).with(:all).and_return(@question)	25	# Question.stub!(:find).with(:all).and_return(@question)
17	# Question.stub!(:prompts).with(:all).and_return([mock_prompt])	26	# Question.stub!(:prompts).with(:all).and_return([mock_prompt])
18	- get :index, :question_id => @question.id
19	- assigns[:prompts].should == [mock_prompt]
20	- end
21	- end	27	+# get :index, :question_id => @question.id
		28	+# assigns[:prompts].should == [@prompt]
		29	+# end
		30	+# end
22		31
23	describe "GET show" do	32	describe "GET show" do
24	it "assigns the requested prompt as @prompt" do	33	it "assigns the requested prompt as @prompt" do
25	-# Question.stub!(:find).with(:all).and_return(@question)
26	-# Prompt.stub!(:find).with("37").and_return(mock_prompt)
27	- get :show, :id => "37", :question_id => @question.id
28	- assigns[:prompt].should equal(mock_prompt)	34	+ get :show, :id => @prompt.id, :question_id => @question.id
		35	+ assigns[:prompt].should == @prompt
29	end	36	end
30	end	37	end
		38	+
		39	+ describe "POST skip" do
		40	+ it "records a skip, responds with next prompt" do
		41	+ post :skip, :id => @prompt.id, :question_id => @question.id, :params => {:auto => @visitor, :time_viewed => 30, :appearance_lookup => @appearance.lookup}
		42	+ assigns[:next_prompt].should_not == @prompt
		43	+ assigns[:next_prompt].should_not be_nil
		44	+ assigns[:a].should_not be_nil
		45	+ assigns[:a].should_not == @appearance
		46	+ assigns[:skip].should_not be_nil
		47	+ end
		48	+ end
		49	+
31	end	50	end
	@@ -47,7 +47,7 @@ describe User do		@@ -47,7 +47,7 @@ describe User do
47	end	47	end
48		48
49	it "should be able to record a visitor's skip" do	49	it "should be able to record a visitor's skip" do
50	- s = @aoi_clone.record_skip("johnnydoe", @prompt)	50	+ s = @aoi_clone.record_skip("johnnydoe", @appearance.lookup, @prompt, 340)
51	end	51	end
52		52
53	end	53	end
	@@ -47,7 +47,7 @@ describe Visitor do		@@ -47,7 +47,7 @@ describe Visitor do
47	it "should be able to skip a prompt" do	47	it "should be able to skip a prompt" do
48	#@prompt = @question.prompts.first	48	#@prompt = @question.prompts.first
49	@prompt.should_not be_nil	49	@prompt.should_not be_nil
50	- v = @v.skip! @prompt	50	+ v = @v.skip! @appearance.lookup, @prompt, 304
51	end	51	end
52		52
53	it "should accurately update score counts after vote" do	53	it "should accurately update score counts after vote" do