Download as
Browse Code »

Commit 8139c736285a557e836679af39012327f8c3ae7a

Authored by Dhruv Kapadia
1 parent 87857f3f
Exists in master and in 1 other branch production-vendorized

Refactoring question show code

Showing 4 changed files with 116 additions and 150 deletions   Show diff stats
app/controllers/questions_controller.rb
  Diff comments   View file @8139c73
1 1 require 'fastercsv'
2 2  
3 3 class QuestionsController < InheritedResources::Base
  4 + actions :all, :except => [ :show ]
4 5 before_filter :authenticate
5 6 respond_to :xml, :json
6 7 respond_to :csv, :only => :export #leave the option for xml export here
... ... @@ -55,61 +56,27 @@ class QuestionsController &lt; InheritedResources::Base
55 56  
56 57 def show
57 58 @question = Question.find(params[:id])
58   - visitor_identifier = params[:visitor_identifier]
59   - unless params[:barebones]
60   -
61   - @p = @question.choose_prompt(:algorithm => params[:algorithm])
62   -
63   - if @p.nil?
64   - # could not find a prompt to choose
65   - # I don't know the best http code for the api to respond, but 409 seems the closes
66   - respond_to do |format|
67   - format.xml { render :xml => @question, :status => :conflict and return}
68   - format.json { render :json => @question, :status => :conflict and return}
69   - end
70 59  
71   - end
72   - # @question.create_new_appearance - returns appearance, which we can then get the prompt from.
73   - # At the very least add 'if create_appearance is true,
74   - # we sometimes request a question when no prompt is displayed
75   - # TODO It would be a good idea to find these places and treat them like barebones
76   - if !visitor_identifier.blank?
77   - visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier)
78   - @a = current_user.record_appearance(visitor, @p)
79   - logger.info("creating appearance!")
80   - else
81   - @a = nil
82   - end
83   -
84   - left_choice_text = Proc.new { |options| options[:builder].tag!('left_choice_text', @p.left_choice.item.data) }
85   - right_choice_text = Proc.new { |options| options[:builder].tag!('right_choice_text', @p.right_choice.item.data) }
86   - picked_prompt_id = Proc.new { |options| options[:builder].tag!('picked_prompt_id', @p.id) }
87   - appearance_id = Proc.new { |options| options[:builder].tag!('appearance_id', @a.lookup) }
88   -
89   - visitor_votes = Proc.new { |options| options[:builder].tag!('visitor_votes', visitor.votes.count(:conditions => {:question_id => @question.id})) }
90   - visitor_ideas = Proc.new { |options| options[:builder].tag!('visitor_ideas', visitor.items.count) }
91   -
92   - the_procs = [left_choice_text, right_choice_text, picked_prompt_id]
93   -
94   - if @a
95   - the_procs << appearance_id
96   - the_procs << visitor_votes
97   - the_procs << visitor_ideas
98   - end
  60 + begin
  61 + @question_optional_information = @question.get_optional_information(params)
  62 + rescue RuntimeError
  63 + respond_to do |format|
  64 + format.xml { render :xml => @question.to_xml, :status => :conflict and return}
  65 + end
  66 + end
99 67  
100   - show! do |format|
101   - session['prompts_ids'] ||= []
102   - format.xml {
103   - render :xml => @question.to_xml(:methods => [:item_count], :procs => the_procs)
104   - }
105   - end
106   - else
107   - show! do |format|
108   - session['prompts_ids'] ||= []
109   - format.xml {
110   - render :xml => @question.to_xml(:methods => [:item_count])
111   - }
112   - end
  68 + optional_information = []
  69 + @question_optional_information.each do |key, value|
  70 + optional_information << Proc.new { |options| options[:builder].tag!(key, value)}
  71 + end
  72 +
  73 + respond_to do |format|
  74 + format.xml {
  75 + render :xml => @question.to_xml(:methods => [:item_count], :procs => optional_information)
  76 + }
  77 + format.js{
  78 + render :json => @question.to_json(:methods => [:item_count], :procs => optional_information)
  79 + }
113 80 end
114 81 end
115 82  
... ...
app/models/question.rb
  Diff comments   View file @8139c73
... ... @@ -33,7 +33,7 @@ class Question &lt; ActiveRecord::Base
33 33  
34 34 # if there is one or fewer active choices, we won't be able to find a prompt
35 35 if self.choices.size - self.inactive_choices_count <= 1
36   - return nil
  36 + raise RuntimeError, "More than one choice needs to be active"
37 37 end
38 38  
39 39 if self.uses_catchup? || options[:algorithm] == "catchup"
... ... @@ -110,6 +110,37 @@ class Question &lt; ActiveRecord::Base
110 110 weights
111 111 end
112 112  
  113 + def get_optional_information(params)
  114 +
  115 + return {} if params.nil?
  116 +
  117 + result = {}
  118 + visitor_identifier = params[:visitor_identifier]
  119 + current_user = self.site
  120 +
  121 + if params[:with_prompt]
  122 + @prompt = choose_prompt(:algorithm => params[:algorithm])
  123 + result.merge!({:picked_prompt_id => @prompt.id})
  124 +
  125 + if params[:with_appearance] && visitor_identifier.present?
  126 + visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier)
  127 + @appearance = current_user.record_appearance(visitor, @prompt)
  128 + result.merge!({:appearance_id => @appearance.lookup})
  129 + else
  130 + # throw some error
  131 + end
  132 + end
  133 +
  134 + if params[:with_visitor_stats]
  135 + visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier)
  136 + result.merge!(:visitor_votes => visitor.votes.count(:conditions => {:question_id => self.id}))
  137 + result.merge!(:visitor_ideas => visitor.items.count)
  138 + end
  139 +
  140 + return result
  141 +
  142 + end
  143 +
113 144 def normalize!(weighted)
114 145 if weighted.instance_of?(Hash)
115 146 sum = weighted.inject(0) do |sum, item_and_weight|
... ...
spec/controllers/questions_controller_spec.rb
  Diff comments   View file @8139c73
... ... @@ -10,108 +10,29 @@ describe QuestionsController do
10 10 end
11 11 #
12 12 before(:each) do
13   - sign_in_as(@user = Factory(:email_confirmed_user))
  13 + @user = Factory.create(:user, :email => "pius@alum.mit.edu", :password => "password", :password_confirmation => "password", :id => 8)
  14 + sign_in_as(@user = Factory(:email_confirmed_user))
  15 + @question = @user.create_question("foobarbaz", {:name => 'foo'})
14 16 end
15   - #
16   - def mock_question(stubs={})
17   - @mock_question ||= mock_model(Question, stubs)
18   - end
19   -
20   - def mock_prompt(stubs={})
21   - @mock_prompt ||= mock_model(Prompt, stubs)
22   - end
23   -
24   - def mock_appearance(stubs={})
25   - @mock_appearance||= mock_model(Appearance, stubs)
26   - end
27   -
28   - def mock_visitor(stubs={})
29   - @mock_visitor||= mock_model(Visitor, stubs)
30   - end
31   - #
32   - # describe "GET index" do
33   - # it "assigns all questions as @questions" do
34   - # Question.stub!(:find).with(:all).and_return([mock_question])
35   - # get :index
36   - # assigns[:questions].should == [mock_question]
37   - # end
38   - # end
39   - #
40   - describe "GET show normal" do
41   - before(:each) do
42   - Question.stub!(:find).with("37").and_return(mock_question)
43   - end
44   -
45   -
46   - it "assigns the requested question as @question" do
47   - Question.stub!(:find).with("37").and_return(mock_question)
48   - mock_question.should_receive(:choose_prompt).and_return(mock_prompt)
49   - #TODO it shouldn't call this unless we are generating an appearance, right?
50   -
51   - get :show, :id => "37"
52   - assigns[:question].should equal(mock_question)
53   - assigns[:p].should equal(mock_prompt)
54   - assigns[:a].should be_nil
55   - end
  17 + it "responds with basic question information" do
  18 + get :show, :id => @question.id, :format => "xml"
56 19  
57   -
58   - it "does not create an appearance when the 'barebones' param is set" do
59   - get :show, :id => "37", :barebones => true
60   - assigns[:question].should equal(mock_question)
61   - assigns[:p].should be_nil
62   - assigns[:a].should be_nil
63   - end
64   -
65   - describe "creates an appearance" do
66   - before(:each) do
67   - @visitor_identifier = "somelongunique32charstring"
68   - #stub broken
69   - visitor_list = [mock_visitor]
70   - @user.stub!(:visitors).and_return(visitor_list)
71   - visitor_list.stub!(:find_or_create_by_identifier).and_return(mock_visitor)
72   - @user.stub!(:record_appearance).with(mock_visitor, mock_prompt).and_return(mock_appearance)
73   - end
74   -
75   - #TODO this is not a particularly intutive param to pass in order to create an appearance
76   - it "creates an appearance when a visitor identifier is a param" do
77   - mock_question.should_receive(:choose_prompt).and_return(mock_prompt)
78   - get :show, :id => "37", :visitor_identifier => @visitor_identifier
79   - assigns[:question].should equal(mock_question)
80   - assigns[:p].should equal(mock_prompt)
81   - assigns[:a].should equal(mock_appearance)
82   -
83   - end
84   -
85   - it "does not create an appearance when the 'barebones' param is set, even when a visitor id is sent" do
86   - get :show, :id => "37", :barebones => true, :visitor_identifier => @visitor_identifier
87   - assigns[:question].should equal(mock_question)
88   - assigns[:p].should be_nil
89   - assigns[:a].should be_nil
90   - end
91   -
92   - describe "calls catchup algorithm" do
  20 + assigns[:question].should == @question
  21 + @response.body.should have_tag("question")
  22 + end
93 23  
94   - #TODO Refactor out to use uses_catchup?
95   - it "should pop prompt from cached queue using the catchup algorithm if params dictate" do
96   - mock_question.should_receive(:choose_prompt).with(:algorithm => "catchup").and_return(mock_prompt)
97 24  
98   - get :show, :id => "37", :visitor_identifier => @visitor_identifier, :algorithm => "catchup"
99   - assigns[:question].should equal(mock_question)
100   - assigns[:p].should equal(mock_prompt)
101   - assigns[:a].should equal(mock_appearance)
102   - end
  25 + it "responds with question with prompt and appearance and visitor information" do
  26 + get :show, :id => @question.id, :format => "xml", :with_appearance => true, :with_prompt => true, :with_visitor_stats => true, :visitor_identifier => "jim"
103 27  
104   - it "should handle cache misses gracefully" do
105   - mock_question.should_receive(:choose_prompt).with(:algorithm => "catchup").and_return(mock_prompt)
  28 + assigns[:question].should == @question
  29 + #@response.body.should be_nil
  30 + @response.body.should have_tag("question")
  31 + @response.body.should have_tag("picked_prompt_id")
  32 + @response.body.should have_tag("appearance_id")
  33 + @response.body.should have_tag("visitor_votes")
  34 + @response.body.should have_tag("visitor_ideas")
106 35  
107   - get :show, :id => "37", :visitor_identifier => @visitor_identifier, :algorithm => "catchup"
108   - assigns[:question].should equal(mock_question)
109   - assigns[:p].should equal(mock_prompt)
110   - assigns[:a].should equal(mock_appearance)
111   - end
112   - end
113   - end
114 36 end
115   -
116 37  
117 38 end
... ...
spec/models/question_spec.rb
  Diff comments   View file @8139c73
... ... @@ -19,6 +19,8 @@ describe Question do
19 19 :creator => @aoi_clone.default_visitor
20 20  
21 21 }
  22 +
  23 + @question = @aoi_clone.create_question("foobarbaz", {:name => 'foo'})
22 24  
23 25 end
24 26  
... ... @@ -60,7 +62,52 @@ describe Question do
60 62  
61 63 end
62 64  
  65 + it "should return nil if optional parameters are empty" do
  66 + @question_optional_information = @question.get_optional_information(nil)
  67 + @question_optional_information.should be_empty
  68 + end
  69 +
  70 + it "should return nil if optional parameters are nil" do
  71 + params = {"id" => '37'}
  72 + @question_optional_information = @question.get_optional_information(params)
  73 + @question_optional_information.should be_empty
  74 + end
  75 +
  76 + it "should return a hash with an prompt id when optional parameters contains 'with_prompt'" do
  77 + params = {:id => 124, :with_prompt => true}
  78 + @question_optional_information = @question.get_optional_information(params)
  79 + @question_optional_information.should include(:picked_prompt_id)
  80 + @question_optional_information[:picked_prompt_id].should be_an_instance_of(Fixnum)
  81 + end
63 82  
  83 + it "should return a hash with an appearance hash when optional parameters contains 'with_appearance'" do
  84 + params = {:id => 124, :with_prompt => true, :with_appearance=> true, :visitor_identifier => 'jim'}
  85 + @question_optional_information = @question.get_optional_information(params)
  86 + @question_optional_information.should include(:appearance_id)
  87 + @question_optional_information[:appearance_id].should be_an_instance_of(String)
  88 + end
  89 +
  90 + it "should return a hash with two visitor stats when optional parameters contains 'with_visitor_stats'" do
  91 + params = {:id => 124, :with_visitor_stats=> true, :visitor_identifier => "jim"}
  92 + @question_optional_information = @question.get_optional_information(params)
  93 + @question_optional_information.should include(:visitor_votes)
  94 + @question_optional_information.should include(:visitor_ideas)
  95 + @question_optional_information[:visitor_votes].should be_an_instance_of(Fixnum)
  96 + @question_optional_information[:visitor_ideas].should be_an_instance_of(Fixnum)
  97 + end
  98 +
  99 + it "should return a hash when optional parameters have more than one optional param " do
  100 + params = {:id => 124, :with_visitor_stats=> true, :visitor_identifier => "jim", :with_prompt => true, :with_appearance => true}
  101 + @question_optional_information = @question.get_optional_information(params)
  102 + @question_optional_information.should include(:visitor_votes)
  103 + @question_optional_information.should include(:visitor_ideas)
  104 + @question_optional_information[:visitor_votes].should be_an_instance_of(Fixnum)
  105 + @question_optional_information[:visitor_ideas].should be_an_instance_of(Fixnum)
  106 + @question_optional_information.should include(:picked_prompt_id)
  107 + @question_optional_information[:picked_prompt_id].should be_an_instance_of(Fixnum)
  108 + @question_optional_information.should include(:appearance_id)
  109 + @question_optional_information[:appearance_id].should be_an_instance_of(String)
  110 + end
64 111  
65 112 context "catchup algorithm" do
66 113 before(:all) do
... ...