Commit 35d6ac47a1742ec39241a6accc1c659cb8211a1c
1 parent
c8233782
Exists in
master
and in
1 other branch
Refactored skip to take parameters like vote
Showing
7 changed files
with
97 additions
and
55 deletions
Show diff stats
app/controllers/prompts_controller.rb
| @@ -49,52 +49,39 @@ class PromptsController < InheritedResources::Base | @@ -49,52 +49,39 @@ class PromptsController < InheritedResources::Base | ||
| 49 | 49 | ||
| 50 | def skip | 50 | def skip |
| 51 | logger.info "#{current_user.inspect} is skipping." | 51 | logger.info "#{current_user.inspect} is skipping." |
| 52 | - @question = Question.find(params[:question_id]) | ||
| 53 | - | ||
| 54 | - @prompt = @question.prompts.find(params[:id]) #, :include => [{ :left_choice => :item }, { :right_choice => :item }]) | ||
| 55 | - | ||
| 56 | - time_viewed = params['params']['time_viewed'] | ||
| 57 | - raise "time_viewed cannot be nil" if time_viewed.nil? | ||
| 58 | - | ||
| 59 | - visitor_identifier = params['params']['auto'] | ||
| 60 | - raise "visitor identifier cannot be nil" if visitor_identifier.nil? | ||
| 61 | - appearance_lookup = params['params']['appearance_lookup'] | ||
| 62 | - raise "appearance_lookup cannot be nil" if appearance_lookup.nil? | 52 | + @question = current_user.questions.find(params[:question_id]) |
| 53 | + @prompt = @question.prompts.find(params[:id]) | ||
| 63 | 54 | ||
| 64 | - skip_reason = params['params']['skip_reason'] # optional parameter | ||
| 65 | - | 55 | + skip_options = params[:skip] || {} |
| 56 | + skip_options.merge!(:prompt => @prompt, :question => @question) | ||
| 66 | 57 | ||
| 67 | - respond_to do |format| | ||
| 68 | - if @skip = current_user.record_skip(visitor_identifier, appearance_lookup, @prompt, time_viewed, :skip_reason => skip_reason) | ||
| 69 | - | ||
| 70 | - #This is not hte right way to do this. See def vote for a better example | 58 | + successful = response = current_user.record_skip(skip_options) |
| 59 | + optional_information = [] | ||
| 60 | + if params[:next_prompt] | ||
| 71 | begin | 61 | begin |
| 72 | - @next_prompt = @question.choose_prompt | 62 | + params[:next_prompt].merge!(:with_prompt => true) # We always want to get the next possible prompt |
| 63 | + @question_optional_information = @question.get_optional_information(params[:next_prompt]) | ||
| 73 | rescue RuntimeError | 64 | rescue RuntimeError |
| 74 | 65 | ||
| 75 | respond_to do |format| | 66 | respond_to do |format| |
| 76 | format.xml { render :xml => @prompt.to_xml, :status => :conflict and return} | 67 | format.xml { render :xml => @prompt.to_xml, :status => :conflict and return} |
| 77 | end | 68 | end |
| 78 | end | 69 | end |
| 79 | - | ||
| 80 | - visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier) | ||
| 81 | - @a = current_user.record_appearance(visitor, @next_prompt) | ||
| 82 | - | ||
| 83 | - appearance_id = Proc.new { |options| options[:builder].tag!('appearance_id', @a.lookup) } | ||
| 84 | - | ||
| 85 | - visitor_votes = Proc.new { |options| options[:builder].tag!('visitor_votes', visitor.votes.count(:conditions => {:question_id => @question.id})) } | ||
| 86 | - visitor_ideas = Proc.new { |options| options[:builder].tag!('visitor_ideas', visitor.items.count) } | ||
| 87 | - | ||
| 88 | - | ||
| 89 | - format.xml { render :xml => @next_prompt.to_xml(:procs => [appearance_id, visitor_votes, visitor_ideas],:methods => [:left_choice_text, :right_choice_text]), :status => :ok } | ||
| 90 | - format.json { render :json => @next_prompt.to_json, :status => :ok } | 70 | + response = @question.prompts.find(@question_optional_information.delete(:picked_prompt_id)) |
| 71 | + @question_optional_information.each do |key, value| | ||
| 72 | + optional_information << Proc.new { |options| options[:builder].tag!(key, value)} | ||
| 73 | + end | ||
| 74 | + end | ||
| 75 | + respond_to do |format| | ||
| 76 | + if !successful.nil? | ||
| 77 | + format.xml { render :xml => response.to_xml(:procs => optional_information , :methods => [:left_choice_text, :right_choice_text, :left_choice_id, :right_choice_id]), :status => :ok } | ||
| 78 | + format.json { render :json => response.to_json(:procs => optional_information, :methods => [:left_choice_text, :right_choice_text, :left_choice_id, :right_choice_id]), :status => :ok } | ||
| 91 | else | 79 | else |
| 92 | - format.xml { render :xml => @prompt.to_xml, :status => :conflict} | ||
| 93 | - format.json { render :json => @prompt.to_xml, :status => :conflict} | 80 | + format.xml { render :xml => @prompt.to_xml, :status => :unprocessable_entity } |
| 81 | + format.json { render :json => @prompt.to_xml, :status => :unprocessable_entity } | ||
| 94 | end | 82 | end |
| 95 | end | 83 | end |
| 96 | end | 84 | end |
| 97 | - | ||
| 98 | 85 | ||
| 99 | def show | 86 | def show |
| 100 | @question = current_user.questions.find(params[:question_id]) | 87 | @question = current_user.questions.find(params[:question_id]) |
app/models/user.rb
| @@ -47,9 +47,14 @@ class User < ActiveRecord::Base | @@ -47,9 +47,14 @@ class User < ActiveRecord::Base | ||
| 47 | end | 47 | end |
| 48 | 48 | ||
| 49 | 49 | ||
| 50 | - def record_skip(visitor_identifier, appearance_lookup, prompt, time_viewed, options = {}) | ||
| 51 | - visitor = visitors.find_or_create_by_identifier(visitor_identifier) | ||
| 52 | - visitor.skip!(appearance_lookup, prompt, time_viewed, options) | 50 | + def record_skip(options) |
| 51 | + visitor_identifier = options.delete(:visitor_identifier) | ||
| 52 | + if visitor_identifier.nil? | ||
| 53 | + visitor = default_visitor | ||
| 54 | + else | ||
| 55 | + visitor = visitors.find_or_create_by_identifier(visitor_identifier) | ||
| 56 | + end | ||
| 57 | + visitor.skip!(options) | ||
| 53 | end | 58 | end |
| 54 | 59 | ||
| 55 | def activate_question(question_id, options) | 60 | def activate_question(question_id, options) |
app/models/visitor.rb
| @@ -36,14 +36,19 @@ class Visitor < ActiveRecord::Base | @@ -36,14 +36,19 @@ class Visitor < ActiveRecord::Base | ||
| 36 | 36 | ||
| 37 | v = votes.create!(options) | 37 | v = votes.create!(options) |
| 38 | end | 38 | end |
| 39 | - | ||
| 40 | - def skip!(appearance_lookup, prompt, time_viewed, options = {}) | ||
| 41 | - @a = Appearance.find_by_lookup(appearance_lookup) | ||
| 42 | - | ||
| 43 | - skip_create_options = { :question_id => prompt.question_id, :prompt_id => prompt.id, :skipper_id=> self.id, :time_viewed => time_viewed, :appearance_id => @a.id} | ||
| 44 | 39 | ||
| 45 | - #the most common optional reason is 'skip_reason', probably want to refactor to make time viewed an optional parameter | ||
| 46 | - prompt_skip = skips.create!(skip_create_options.merge(options)) | 40 | + def skip!(options) |
| 41 | + return nil if !options || !options[:prompt] | ||
| 42 | + | ||
| 43 | + prompt = options.delete(:prompt) | ||
| 44 | + | ||
| 45 | + if options[:appearance_lookup] | ||
| 46 | + @appearance = prompt.appearances.find_by_lookup(options.delete(:apperance_lookup)) | ||
| 47 | + return nil unless @appearance | ||
| 48 | + options.merge!(:appearance_id => @appearance.id) | ||
| 49 | + end | ||
| 47 | 50 | ||
| 51 | + options.merge!(:question_id => prompt.question_id, :prompt_id => prompt.id, :skipper_id => self.id) | ||
| 52 | + prompt_skip = skips.create!(options) | ||
| 48 | end | 53 | end |
| 49 | end | 54 | end |
spec/controllers/prompts_controller_spec.rb
| @@ -21,16 +21,41 @@ describe PromptsController do | @@ -21,16 +21,41 @@ describe PromptsController do | ||
| 21 | assigns[:prompt].should == @prompt | 21 | assigns[:prompt].should == @prompt |
| 22 | end | 22 | end |
| 23 | end | 23 | end |
| 24 | - | ||
| 25 | - describe "POST skip" do | ||
| 26 | - it "records a skip, responds with next prompt" do | ||
| 27 | - post :skip, :id => @prompt.id, :question_id => @question.id, :params => {:auto => @visitor, :time_viewed => 30, :appearance_lookup => @appearance.lookup} | ||
| 28 | - assigns[:next_prompt].should_not be_nil | ||
| 29 | - assigns[:a].should_not be_nil | ||
| 30 | - assigns[:a].should_not == @appearance | ||
| 31 | - assigns[:skip].should_not be_nil | 24 | + |
| 25 | + describe "POST skip" do | ||
| 26 | + it "records a skip without any optional params" do | ||
| 27 | + controller.current_user.should_receive(:record_skip).and_return(true) | ||
| 28 | + post(:skip, :question_id => @question.id, :id => @prompt.id, | ||
| 29 | + :format => :xml) | ||
| 32 | end | 30 | end |
| 33 | - end | 31 | + |
| 32 | + it "records a skip with optional params" do | ||
| 33 | + controller.current_user.should_receive(:record_skip).and_return(true) | ||
| 34 | + post(:skip, :question_id => @question.id, :id => @prompt.id, | ||
| 35 | + :skip => { | ||
| 36 | + :visitor_identifier => "jim", | ||
| 37 | + :time_viewed => rand(1000), | ||
| 38 | + :skip_reason => "some reason"}) | ||
| 39 | + end | ||
| 40 | + | ||
| 41 | + it "records a skip, responds with next prompt" do | ||
| 42 | + controller.current_user.should_receive(:record_skip).and_return(true) | ||
| 43 | + post(:skip, :question_id => @question.id, :id => @prompt.id, | ||
| 44 | + :skip => { | ||
| 45 | + :visitor_identifier => "jim", | ||
| 46 | + :time_viewed => rand(1000), | ||
| 47 | + :skip_reason => "some reason"}, | ||
| 48 | + :next_prompt => { | ||
| 49 | + :with_appearance => true, | ||
| 50 | + :with_visitor_stats => true, | ||
| 51 | + :visitor_identifier => "jim"} | ||
| 52 | + ) | ||
| 53 | + assigns[:question_optional_information].should_not be_nil | ||
| 54 | + assigns[:question_optional_information][:appearance_id].should_not be_nil | ||
| 55 | + assigns[:question_optional_information][:visitor_votes].should_not be_nil | ||
| 56 | + assigns[:question_optional_information][:visitor_ideas].should_not be_nil | ||
| 57 | + end | ||
| 58 | + end | ||
| 34 | 59 | ||
| 35 | describe "POST vote" do | 60 | describe "POST vote" do |
| 36 | it "votes on a prompt" do | 61 | it "votes on a prompt" do |
spec/models/question_spec.rb
| @@ -219,12 +219,18 @@ describe Question do | @@ -219,12 +219,18 @@ describe Question do | ||
| 219 | :time_viewed => rand(1000), | 219 | :time_viewed => rand(1000), |
| 220 | :direction => (rand(2) == 0) ? "left" : "right"} | 220 | :direction => (rand(2) == 0) ? "left" : "right"} |
| 221 | 221 | ||
| 222 | + skip_options = {:visitor_identifier => visitor.identifier, | ||
| 223 | + :appearance_lookup => @a.lookup, | ||
| 224 | + :prompt => @p, | ||
| 225 | + :time_viewed => rand(1000), | ||
| 226 | + :skip_reason => "some reason"} | ||
| 227 | + | ||
| 222 | choice = rand(3) | 228 | choice = rand(3) |
| 223 | case choice | 229 | case choice |
| 224 | when 0 | 230 | when 0 |
| 225 | user.record_vote(vote_options) | 231 | user.record_vote(vote_options) |
| 226 | when 1 | 232 | when 1 |
| 227 | - user.record_skip(visitor.identifier, @a.lookup, @p, rand(1000)) | 233 | + user.record_skip(skip_options) |
| 228 | when 2 | 234 | when 2 |
| 229 | #this is an orphaned appearance, so do nothing | 235 | #this is an orphaned appearance, so do nothing |
| 230 | end | 236 | end |
spec/models/user_spec.rb
| @@ -83,7 +83,14 @@ describe User do | @@ -83,7 +83,14 @@ describe User do | ||
| 83 | it "should be able to record a visitor's skip" do | 83 | it "should be able to record a visitor's skip" do |
| 84 | @visitor = @aoi_clone.visitors.find_or_create_by_identifier("test_visitor_identifier") | 84 | @visitor = @aoi_clone.visitors.find_or_create_by_identifier("test_visitor_identifier") |
| 85 | @appearance = @aoi_clone.record_appearance(@visitor, @prompt) | 85 | @appearance = @aoi_clone.record_appearance(@visitor, @prompt) |
| 86 | - s = @aoi_clone.record_skip("johnnydoe", @appearance.lookup, @prompt, 340) | 86 | + optional_skip_params = { |
| 87 | + :visitor_identifier => @visitor.identifier, | ||
| 88 | + :appearance_lookup => @appearance.lookup, | ||
| 89 | + :time_viewed => 340 | ||
| 90 | + } | ||
| 91 | + required_skip_params = {:prompt => @prompt } | ||
| 92 | + params = optional_skip_params.merge(required_skip_params) | ||
| 93 | + s = @aoi_clone.record_skip(params) | ||
| 87 | end | 94 | end |
| 88 | 95 | ||
| 89 | end | 96 | end |
spec/models/visitor_spec.rb
| @@ -18,6 +18,7 @@ describe Visitor do | @@ -18,6 +18,7 @@ describe Visitor do | ||
| 18 | 18 | ||
| 19 | @required_vote_params = {:prompt => @prompt, | 19 | @required_vote_params = {:prompt => @prompt, |
| 20 | :direction => "left"} | 20 | :direction => "left"} |
| 21 | + @required_skip_params = {:prompt => @prompt} | ||
| 21 | end | 22 | end |
| 22 | 23 | ||
| 23 | it "should create a new instance given valid attributes" do | 24 | it "should create a new instance given valid attributes" do |
| @@ -93,7 +94,13 @@ describe Visitor do | @@ -93,7 +94,13 @@ describe Visitor do | ||
| 93 | 94 | ||
| 94 | it "should be able to skip a prompt" do | 95 | it "should be able to skip a prompt" do |
| 95 | @appearance = @aoi_clone.record_appearance(@visitor, @prompt) | 96 | @appearance = @aoi_clone.record_appearance(@visitor, @prompt) |
| 96 | - s = @visitor.skip! @appearance.lookup, @prompt, 304 | 97 | + @optional_skip_params = { |
| 98 | + :appearance_lookup => @appearance.lookup, | ||
| 99 | + :time_viewed => 304, | ||
| 100 | + :skip_reason => "some reason" | ||
| 101 | + } | ||
| 102 | + allparams = @required_skip_params.merge(@optional_skip_params) | ||
| 103 | + s = @visitor.skip!(allparams) | ||
| 97 | end | 104 | end |
| 98 | 105 | ||
| 99 | it "should accurately update score counts after vote" do | 106 | it "should accurately update score counts after vote" do |