From 8139c736285a557e836679af39012327f8c3ae7a Mon Sep 17 00:00:00 2001 From: Dhruv Kapadia Date: Mon, 31 May 2010 15:25:09 -0400 Subject: [PATCH] Refactoring question show code --- app/controllers/questions_controller.rb | 73 ++++++++++++++++++++----------------------------------------------------- app/models/question.rb | 33 ++++++++++++++++++++++++++++++++- spec/controllers/questions_controller_spec.rb | 113 +++++++++++++++++------------------------------------------------------------------------------------------------ spec/models/question_spec.rb | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 116 insertions(+), 150 deletions(-) diff --git a/app/controllers/questions_controller.rb b/app/controllers/questions_controller.rb index 705d4e3..16ef7a9 100644 --- a/app/controllers/questions_controller.rb +++ b/app/controllers/questions_controller.rb @@ -1,6 +1,7 @@ require 'fastercsv' class QuestionsController < InheritedResources::Base + actions :all, :except => [ :show ] before_filter :authenticate respond_to :xml, :json respond_to :csv, :only => :export #leave the option for xml export here @@ -55,61 +56,27 @@ class QuestionsController < InheritedResources::Base def show @question = Question.find(params[:id]) - visitor_identifier = params[:visitor_identifier] - unless params[:barebones] - - @p = @question.choose_prompt(:algorithm => params[:algorithm]) - - if @p.nil? - # could not find a prompt to choose - # I don't know the best http code for the api to respond, but 409 seems the closes - respond_to do |format| - format.xml { render :xml => @question, :status => :conflict and return} - format.json { render :json => @question, :status => :conflict and return} - end - end - # @question.create_new_appearance - returns appearance, which we can then get the prompt from. - # At the very least add 'if create_appearance is true, - # we sometimes request a question when no prompt is displayed - # TODO It would be a good idea to find these places and treat them like barebones - if !visitor_identifier.blank? - visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier) - @a = current_user.record_appearance(visitor, @p) - logger.info("creating appearance!") - else - @a = nil - end - - left_choice_text = Proc.new { |options| options[:builder].tag!('left_choice_text', @p.left_choice.item.data) } - right_choice_text = Proc.new { |options| options[:builder].tag!('right_choice_text', @p.right_choice.item.data) } - picked_prompt_id = Proc.new { |options| options[:builder].tag!('picked_prompt_id', @p.id) } - 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) } - - the_procs = [left_choice_text, right_choice_text, picked_prompt_id] - - if @a - the_procs << appearance_id - the_procs << visitor_votes - the_procs << visitor_ideas - end + begin + @question_optional_information = @question.get_optional_information(params) + rescue RuntimeError + respond_to do |format| + format.xml { render :xml => @question.to_xml, :status => :conflict and return} + end + end - show! do |format| - session['prompts_ids'] ||= [] - format.xml { - render :xml => @question.to_xml(:methods => [:item_count], :procs => the_procs) - } - end - else - show! do |format| - session['prompts_ids'] ||= [] - format.xml { - render :xml => @question.to_xml(:methods => [:item_count]) - } - end + optional_information = [] + @question_optional_information.each do |key, value| + optional_information << Proc.new { |options| options[:builder].tag!(key, value)} + end + + respond_to do |format| + format.xml { + render :xml => @question.to_xml(:methods => [:item_count], :procs => optional_information) + } + format.js{ + render :json => @question.to_json(:methods => [:item_count], :procs => optional_information) + } end end diff --git a/app/models/question.rb b/app/models/question.rb index 8ff23a3..a810624 100644 --- a/app/models/question.rb +++ b/app/models/question.rb @@ -33,7 +33,7 @@ class Question < ActiveRecord::Base # if there is one or fewer active choices, we won't be able to find a prompt if self.choices.size - self.inactive_choices_count <= 1 - return nil + raise RuntimeError, "More than one choice needs to be active" end if self.uses_catchup? || options[:algorithm] == "catchup" @@ -110,6 +110,37 @@ class Question < ActiveRecord::Base weights end + def get_optional_information(params) + + return {} if params.nil? + + result = {} + visitor_identifier = params[:visitor_identifier] + current_user = self.site + + if params[:with_prompt] + @prompt = choose_prompt(:algorithm => params[:algorithm]) + result.merge!({:picked_prompt_id => @prompt.id}) + + if params[:with_appearance] && visitor_identifier.present? + visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier) + @appearance = current_user.record_appearance(visitor, @prompt) + result.merge!({:appearance_id => @appearance.lookup}) + else + # throw some error + end + end + + if params[:with_visitor_stats] + visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier) + result.merge!(:visitor_votes => visitor.votes.count(:conditions => {:question_id => self.id})) + result.merge!(:visitor_ideas => visitor.items.count) + end + + return result + + end + def normalize!(weighted) if weighted.instance_of?(Hash) sum = weighted.inject(0) do |sum, item_and_weight| diff --git a/spec/controllers/questions_controller_spec.rb b/spec/controllers/questions_controller_spec.rb index 3e59d05..def0e6c 100644 --- a/spec/controllers/questions_controller_spec.rb +++ b/spec/controllers/questions_controller_spec.rb @@ -10,108 +10,29 @@ describe QuestionsController do end # before(:each) do - sign_in_as(@user = Factory(:email_confirmed_user)) + @user = Factory.create(:user, :email => "pius@alum.mit.edu", :password => "password", :password_confirmation => "password", :id => 8) + sign_in_as(@user = Factory(:email_confirmed_user)) + @question = @user.create_question("foobarbaz", {:name => 'foo'}) end - # - def mock_question(stubs={}) - @mock_question ||= mock_model(Question, stubs) - end - - def mock_prompt(stubs={}) - @mock_prompt ||= mock_model(Prompt, stubs) - end - - def mock_appearance(stubs={}) - @mock_appearance||= mock_model(Appearance, stubs) - end - - def mock_visitor(stubs={}) - @mock_visitor||= mock_model(Visitor, stubs) - end - # - # describe "GET index" do - # it "assigns all questions as @questions" do - # Question.stub!(:find).with(:all).and_return([mock_question]) - # get :index - # assigns[:questions].should == [mock_question] - # end - # end - # - describe "GET show normal" do - before(:each) do - Question.stub!(:find).with("37").and_return(mock_question) - end - - - it "assigns the requested question as @question" do - Question.stub!(:find).with("37").and_return(mock_question) - mock_question.should_receive(:choose_prompt).and_return(mock_prompt) - #TODO it shouldn't call this unless we are generating an appearance, right? - - get :show, :id => "37" - assigns[:question].should equal(mock_question) - assigns[:p].should equal(mock_prompt) - assigns[:a].should be_nil - end + it "responds with basic question information" do + get :show, :id => @question.id, :format => "xml" - - it "does not create an appearance when the 'barebones' param is set" do - get :show, :id => "37", :barebones => true - assigns[:question].should equal(mock_question) - assigns[:p].should be_nil - assigns[:a].should be_nil - end - - describe "creates an appearance" do - before(:each) do - @visitor_identifier = "somelongunique32charstring" - #stub broken - visitor_list = [mock_visitor] - @user.stub!(:visitors).and_return(visitor_list) - visitor_list.stub!(:find_or_create_by_identifier).and_return(mock_visitor) - @user.stub!(:record_appearance).with(mock_visitor, mock_prompt).and_return(mock_appearance) - end - - #TODO this is not a particularly intutive param to pass in order to create an appearance - it "creates an appearance when a visitor identifier is a param" do - mock_question.should_receive(:choose_prompt).and_return(mock_prompt) - get :show, :id => "37", :visitor_identifier => @visitor_identifier - assigns[:question].should equal(mock_question) - assigns[:p].should equal(mock_prompt) - assigns[:a].should equal(mock_appearance) - - end - - it "does not create an appearance when the 'barebones' param is set, even when a visitor id is sent" do - get :show, :id => "37", :barebones => true, :visitor_identifier => @visitor_identifier - assigns[:question].should equal(mock_question) - assigns[:p].should be_nil - assigns[:a].should be_nil - end - - describe "calls catchup algorithm" do + assigns[:question].should == @question + @response.body.should have_tag("question") + end - #TODO Refactor out to use uses_catchup? - it "should pop prompt from cached queue using the catchup algorithm if params dictate" do - mock_question.should_receive(:choose_prompt).with(:algorithm => "catchup").and_return(mock_prompt) - get :show, :id => "37", :visitor_identifier => @visitor_identifier, :algorithm => "catchup" - assigns[:question].should equal(mock_question) - assigns[:p].should equal(mock_prompt) - assigns[:a].should equal(mock_appearance) - end + it "responds with question with prompt and appearance and visitor information" do + get :show, :id => @question.id, :format => "xml", :with_appearance => true, :with_prompt => true, :with_visitor_stats => true, :visitor_identifier => "jim" - it "should handle cache misses gracefully" do - mock_question.should_receive(:choose_prompt).with(:algorithm => "catchup").and_return(mock_prompt) + assigns[:question].should == @question + #@response.body.should be_nil + @response.body.should have_tag("question") + @response.body.should have_tag("picked_prompt_id") + @response.body.should have_tag("appearance_id") + @response.body.should have_tag("visitor_votes") + @response.body.should have_tag("visitor_ideas") - get :show, :id => "37", :visitor_identifier => @visitor_identifier, :algorithm => "catchup" - assigns[:question].should equal(mock_question) - assigns[:p].should equal(mock_prompt) - assigns[:a].should equal(mock_appearance) - end - end - end end - end diff --git a/spec/models/question_spec.rb b/spec/models/question_spec.rb index 360834c..66ddf2f 100644 --- a/spec/models/question_spec.rb +++ b/spec/models/question_spec.rb @@ -19,6 +19,8 @@ describe Question do :creator => @aoi_clone.default_visitor } + + @question = @aoi_clone.create_question("foobarbaz", {:name => 'foo'}) end @@ -60,7 +62,52 @@ describe Question do end + it "should return nil if optional parameters are empty" do + @question_optional_information = @question.get_optional_information(nil) + @question_optional_information.should be_empty + end + + it "should return nil if optional parameters are nil" do + params = {"id" => '37'} + @question_optional_information = @question.get_optional_information(params) + @question_optional_information.should be_empty + end + + it "should return a hash with an prompt id when optional parameters contains 'with_prompt'" do + params = {:id => 124, :with_prompt => true} + @question_optional_information = @question.get_optional_information(params) + @question_optional_information.should include(:picked_prompt_id) + @question_optional_information[:picked_prompt_id].should be_an_instance_of(Fixnum) + end + it "should return a hash with an appearance hash when optional parameters contains 'with_appearance'" do + params = {:id => 124, :with_prompt => true, :with_appearance=> true, :visitor_identifier => 'jim'} + @question_optional_information = @question.get_optional_information(params) + @question_optional_information.should include(:appearance_id) + @question_optional_information[:appearance_id].should be_an_instance_of(String) + end + + it "should return a hash with two visitor stats when optional parameters contains 'with_visitor_stats'" do + params = {:id => 124, :with_visitor_stats=> true, :visitor_identifier => "jim"} + @question_optional_information = @question.get_optional_information(params) + @question_optional_information.should include(:visitor_votes) + @question_optional_information.should include(:visitor_ideas) + @question_optional_information[:visitor_votes].should be_an_instance_of(Fixnum) + @question_optional_information[:visitor_ideas].should be_an_instance_of(Fixnum) + end + + it "should return a hash when optional parameters have more than one optional param " do + params = {:id => 124, :with_visitor_stats=> true, :visitor_identifier => "jim", :with_prompt => true, :with_appearance => true} + @question_optional_information = @question.get_optional_information(params) + @question_optional_information.should include(:visitor_votes) + @question_optional_information.should include(:visitor_ideas) + @question_optional_information[:visitor_votes].should be_an_instance_of(Fixnum) + @question_optional_information[:visitor_ideas].should be_an_instance_of(Fixnum) + @question_optional_information.should include(:picked_prompt_id) + @question_optional_information[:picked_prompt_id].should be_an_instance_of(Fixnum) + @question_optional_information.should include(:appearance_id) + @question_optional_information[:appearance_id].should be_an_instance_of(String) + end context "catchup algorithm" do before(:all) do -- libgit2 0.21.2