Commit 0266868ba6e2c382a6a676ed72dcb87f1a678b9d
Exists in
master
and in
1 other branch
Merge branch 'addFlag'
Showing
13 changed files
with
214 additions
and
46 deletions
Show diff stats
app/controllers/choices_controller.rb
@@ -3,6 +3,8 @@ class ChoicesController < InheritedResources::Base | @@ -3,6 +3,8 @@ class ChoicesController < InheritedResources::Base | ||
3 | actions :show, :index, :create, :update | 3 | actions :show, :index, :create, :update |
4 | belongs_to :question | 4 | belongs_to :question |
5 | has_scope :active, :boolean => true, :only => :index | 5 | has_scope :active, :boolean => true, :only => :index |
6 | + | ||
7 | + before_filter :authenticate, :only => [:flag] | ||
6 | #caches_page :index | 8 | #caches_page :index |
7 | 9 | ||
8 | def index | 10 | def index |
@@ -78,7 +80,7 @@ class ChoicesController < InheritedResources::Base | @@ -78,7 +80,7 @@ class ChoicesController < InheritedResources::Base | ||
78 | visitor_ideas = Proc.new { |options| options[:builder].tag!('visitor_ideas', visitor.items.count) } | 80 | visitor_ideas = Proc.new { |options| options[:builder].tag!('visitor_ideas', visitor.items.count) } |
79 | 81 | ||
80 | format.xml { render :xml => @choice.to_xml(:procs => [saved_choice_id, choice_status, visitor_votes, visitor_ideas]), :status => :ok } | 82 | format.xml { render :xml => @choice.to_xml(:procs => [saved_choice_id, choice_status, visitor_votes, visitor_ideas]), :status => :ok } |
81 | - # format.xml { render :xml => @question.picked_prompt.to_xml(:methods => [:left_choice_text, :right_choice_text], :procs => [saved_choice_id, choice_status]), :status => :ok } | 83 | + # TODO: Why are we rendering a question here? Is the prompt being used later on? |
82 | format.json { render :json => @question.to_json(:procs => [saved_choice_id, choice_status]), :status => :ok } | 84 | format.json { render :json => @question.to_json(:procs => [saved_choice_id, choice_status]), :status => :ok } |
83 | else | 85 | else |
84 | format.xml { render :xml => @choice.errors, :status => :unprocessable_entity } | 86 | format.xml { render :xml => @choice.errors, :status => :unprocessable_entity } |
@@ -96,7 +98,6 @@ class ChoicesController < InheritedResources::Base | @@ -96,7 +98,6 @@ class ChoicesController < InheritedResources::Base | ||
96 | respond_to do |format| | 98 | respond_to do |format| |
97 | if @choice.activate! | 99 | if @choice.activate! |
98 | logger.info "successfully activated choice #{@choice.inspect}" | 100 | logger.info "successfully activated choice #{@choice.inspect}" |
99 | - Question.update_counters(@question.id, :inactive_choices_count => -1) | ||
100 | format.xml { render :xml => true } | 101 | format.xml { render :xml => true } |
101 | format.json { render :json => true } | 102 | format.json { render :json => true } |
102 | else | 103 | else |
@@ -119,7 +120,6 @@ class ChoicesController < InheritedResources::Base | @@ -119,7 +120,6 @@ class ChoicesController < InheritedResources::Base | ||
119 | format.json { render :json => false } | 120 | format.json { render :json => false } |
120 | elsif @choice.deactivate! | 121 | elsif @choice.deactivate! |
121 | logger.info "successfully deactivated choice #{@choice.inspect}" | 122 | logger.info "successfully deactivated choice #{@choice.inspect}" |
122 | - Question.update_counters(@question.id, :inactive_choices_count => 1 ) | ||
123 | format.xml { render :xml => true } | 123 | format.xml { render :xml => true } |
124 | format.json { render :json => true } | 124 | format.json { render :json => true } |
125 | else | 125 | else |
@@ -177,4 +177,35 @@ class ChoicesController < InheritedResources::Base | @@ -177,4 +177,35 @@ class ChoicesController < InheritedResources::Base | ||
177 | end | 177 | end |
178 | end | 178 | end |
179 | end | 179 | end |
180 | + | ||
181 | + def flag | ||
182 | + @question = current_user.questions.find(params[:question_id]) | ||
183 | + @choice = @question.choices.find(params[:id]) | ||
184 | + | ||
185 | + flag_params = {:choice_id => params[:id].to_i, :question_id => params[:question_id].to_i, :site_id => current_user.id} | ||
186 | + | ||
187 | + if explanation = params[:explanation] | ||
188 | + flag_params.merge!({:explanation => explanation}) | ||
189 | + | ||
190 | + end | ||
191 | + if visitor_identifier = params[:visitor_identifier] | ||
192 | + visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier) | ||
193 | + flag_params.merge!({:visitor_id => visitor.id}) | ||
194 | + end | ||
195 | + respond_to do |format| | ||
196 | + if @choice.deactivate! | ||
197 | + flag = Flag.create!(flag_params) | ||
198 | + format.xml { render :xml => @choice.to_xml, :status => :created } | ||
199 | + format.json { render :json => @choice.to_json, :status => :created } | ||
200 | + else | ||
201 | + format.xml { render :xml => @choice.errors, :status => :unprocessable_entity } | ||
202 | + format.json { render :json => @choice.to_json } | ||
203 | + end | ||
204 | + end | ||
205 | + | ||
206 | + end | ||
207 | + | ||
208 | + | ||
209 | + | ||
180 | end | 210 | end |
211 | + |
app/controllers/prompts_controller.rb
@@ -67,9 +67,7 @@ class PromptsController < InheritedResources::Base | @@ -67,9 +67,7 @@ class PromptsController < InheritedResources::Base | ||
67 | 67 | ||
68 | #@prompt.choices.each(&:compute_score!) | 68 | #@prompt.choices.each(&:compute_score!) |
69 | respond_to do |format| | 69 | respond_to do |format| |
70 | - if successful | ||
71 | - | ||
72 | - next_prompt = @question.choose_prompt | 70 | + if successful && next_prompt = @question.choose_prompt |
73 | 71 | ||
74 | visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier) | 72 | visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier) |
75 | @a = current_user.record_appearance(visitor, next_prompt) | 73 | @a = current_user.record_appearance(visitor, next_prompt) |
@@ -111,6 +109,7 @@ class PromptsController < InheritedResources::Base | @@ -111,6 +109,7 @@ class PromptsController < InheritedResources::Base | ||
111 | authenticate | 109 | authenticate |
112 | logger.info "#{current_user.inspect} is skipping." | 110 | logger.info "#{current_user.inspect} is skipping." |
113 | @question = Question.find(params[:question_id]) | 111 | @question = Question.find(params[:question_id]) |
112 | + | ||
114 | @prompt = @question.prompts.find(params[:id]) #, :include => [{ :left_choice => :item }, { :right_choice => :item }]) | 113 | @prompt = @question.prompts.find(params[:id]) #, :include => [{ :left_choice => :item }, { :right_choice => :item }]) |
115 | 114 | ||
116 | time_viewed = params['params']['time_viewed'] | 115 | time_viewed = params['params']['time_viewed'] |
@@ -125,23 +124,7 @@ class PromptsController < InheritedResources::Base | @@ -125,23 +124,7 @@ class PromptsController < InheritedResources::Base | ||
125 | 124 | ||
126 | 125 | ||
127 | respond_to do |format| | 126 | respond_to do |format| |
128 | - if @skip = current_user.record_skip(visitor_identifier, appearance_lookup, @prompt, time_viewed, :skip_reason => skip_reason) | ||
129 | - | ||
130 | - if @question.uses_catchup? | ||
131 | - logger.info("Question #{@question.id} is using catchup algorithm!") | ||
132 | - @next_prompt = @question.pop_prompt_queue | ||
133 | - if @next_prompt.nil? | ||
134 | - @question.record_prompt_cache_miss | ||
135 | - logger.info("Catchup prompt cache miss! Nothing in prompt_queue") | ||
136 | - @next_prompt = @question.catchup_choose_prompt | ||
137 | - else | ||
138 | - @question.record_prompt_cache_hit | ||
139 | - end | ||
140 | - @question.send_later :add_prompt_to_queue | ||
141 | - else | ||
142 | - @next_prompt = @question.picked_prompt | ||
143 | - end | ||
144 | - | 127 | + if @skip = current_user.record_skip(visitor_identifier, appearance_lookup, @prompt, time_viewed, :skip_reason => skip_reason) && @next_prompt = @question.choose_prompt |
145 | 128 | ||
146 | visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier) | 129 | visitor = current_user.visitors.find_or_create_by_identifier(visitor_identifier) |
147 | @a = current_user.record_appearance(visitor, @next_prompt) | 130 | @a = current_user.record_appearance(visitor, @next_prompt) |
@@ -152,11 +135,11 @@ class PromptsController < InheritedResources::Base | @@ -152,11 +135,11 @@ class PromptsController < InheritedResources::Base | ||
152 | visitor_ideas = Proc.new { |options| options[:builder].tag!('visitor_ideas', visitor.items.count) } | 135 | visitor_ideas = Proc.new { |options| options[:builder].tag!('visitor_ideas', visitor.items.count) } |
153 | 136 | ||
154 | 137 | ||
155 | - 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 } | ||
156 | - format.json { render :json => @question.picked_prompt.to_json, :status => :ok } | 138 | + format.xml { render :xml => @next_prompt.to_xml(:procs => [appearance_id, visitor_votes, visitor_ideas],:methods => [:left_choice_text, :right_choice_text]), :status => :ok } |
139 | + format.json { render :json => @next_prompt.to_json, :status => :ok } | ||
157 | else | 140 | else |
158 | - format.xml { render :xml => @skip, :status => :unprocessable_entity } | ||
159 | - format.json { render :json => @skip, :status => :unprocessable_entity } | 141 | + format.xml { render :xml => @prompt.to_xml, :status => :conflict} |
142 | + format.json { render :json => @prompt.to_xml, :status => :conflict} | ||
160 | end | 143 | end |
161 | end | 144 | end |
162 | end | 145 | end |
app/controllers/questions_controller.rb
@@ -60,6 +60,17 @@ class QuestionsController < InheritedResources::Base | @@ -60,6 +60,17 @@ class QuestionsController < InheritedResources::Base | ||
60 | 60 | ||
61 | @p = @question.choose_prompt(:algorithm => params[:algorithm]) | 61 | @p = @question.choose_prompt(:algorithm => params[:algorithm]) |
62 | 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 | + | ||
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, | ||
63 | # we sometimes request a question when no prompt is displayed | 74 | # we sometimes request a question when no prompt is displayed |
64 | # TODO It would be a good idea to find these places and treat them like barebones | 75 | # TODO It would be a good idea to find these places and treat them like barebones |
65 | if !visitor_identifier.blank? | 76 | if !visitor_identifier.blank? |
@@ -96,7 +107,7 @@ class QuestionsController < InheritedResources::Base | @@ -96,7 +107,7 @@ class QuestionsController < InheritedResources::Base | ||
96 | show! do |format| | 107 | show! do |format| |
97 | session['prompts_ids'] ||= [] | 108 | session['prompts_ids'] ||= [] |
98 | format.xml { | 109 | format.xml { |
99 | - render :xml => @question.to_xml(:methods => [:item_count, :votes_count]) | 110 | + render :xml => @question.to_xml(:methods => [:item_count]) |
100 | } | 111 | } |
101 | end | 112 | end |
102 | end | 113 | end |
app/models/choice.rb
1 | class Choice < ActiveRecord::Base | 1 | class Choice < ActiveRecord::Base |
2 | - include Activation | ||
3 | 2 | ||
4 | belongs_to :question, :counter_cache => true | 3 | belongs_to :question, :counter_cache => true |
5 | belongs_to :item | 4 | belongs_to :item |
@@ -10,6 +9,7 @@ class Choice < ActiveRecord::Base | @@ -10,6 +9,7 @@ class Choice < ActiveRecord::Base | ||
10 | #validates_length_of :item, :maximum => 140 | 9 | #validates_length_of :item, :maximum => 140 |
11 | 10 | ||
12 | has_many :votes | 11 | has_many :votes |
12 | + has_many :flags | ||
13 | has_many :prompts_on_the_left, :class_name => "Prompt", :foreign_key => "left_choice_id" | 13 | has_many :prompts_on_the_left, :class_name => "Prompt", :foreign_key => "left_choice_id" |
14 | has_many :prompts_on_the_right, :class_name => "Prompt", :foreign_key => "right_choice_id" | 14 | has_many :prompts_on_the_right, :class_name => "Prompt", :foreign_key => "right_choice_id" |
15 | named_scope :active, :conditions => { :active => true } | 15 | named_scope :active, :conditions => { :active => true } |
@@ -102,6 +102,22 @@ class Choice < ActiveRecord::Base | @@ -102,6 +102,22 @@ class Choice < ActiveRecord::Base | ||
102 | 102 | ||
103 | end | 103 | end |
104 | 104 | ||
105 | + def activate! | ||
106 | + (self.active = true) | ||
107 | + self.save! | ||
108 | + Question.update_counters(self.question_id, :inactive_choices_count => -1) | ||
109 | + end | ||
110 | + | ||
111 | + def suspend! | ||
112 | + (self.active = false) | ||
113 | + self.save! | ||
114 | + end | ||
115 | + | ||
116 | + def deactivate! | ||
117 | + (self.active = false) | ||
118 | + self.save! | ||
119 | + Question.update_counters(self.question_id, :inactive_choices_count => 1) | ||
120 | + end | ||
105 | 121 | ||
106 | protected | 122 | protected |
107 | 123 |
app/models/question.rb
@@ -31,6 +31,11 @@ class Question < ActiveRecord::Base | @@ -31,6 +31,11 @@ class Question < ActiveRecord::Base | ||
31 | 31 | ||
32 | def choose_prompt(options = {}) | 32 | def choose_prompt(options = {}) |
33 | 33 | ||
34 | + # if there is one or fewer active choices, we won't be able to find a prompt | ||
35 | + if self.choices_count - self.inactive_choices_count <= 1 | ||
36 | + return nil | ||
37 | + end | ||
38 | + | ||
34 | if self.uses_catchup? || options[:algorithm] == "catchup" | 39 | if self.uses_catchup? || options[:algorithm] == "catchup" |
35 | logger.info("Question #{self.id} is using catchup algorithm!") | 40 | logger.info("Question #{self.id} is using catchup algorithm!") |
36 | next_prompt = self.pop_prompt_queue | 41 | next_prompt = self.pop_prompt_queue |
config/routes.rb
@@ -15,7 +15,7 @@ ActionController::Routing::Routes.draw do |map| | @@ -15,7 +15,7 @@ ActionController::Routing::Routes.draw do |map| | ||
15 | question.resources :items | 15 | question.resources :items |
16 | question.resources :prompts, :member => {:vote_left => :post, :vote_right => :post, :skip => :post, :vote => :post}, | 16 | question.resources :prompts, :member => {:vote_left => :post, :vote_right => :post, :skip => :post, :vote => :post}, |
17 | :collection => {:single => :get, :index => :get} | 17 | :collection => {:single => :get, :index => :get} |
18 | - question.resources :choices, :member => { :activate => :put, :suspend => :put, :update_from_abroad => :put, :deactivate_from_abroad => :put }, :collection => {:create_from_abroad => :post} | 18 | + question.resources :choices, :member => { :activate => :put, :suspend => :put, :update_from_abroad => :put, :deactivate_from_abroad => :put, :flag => :put}, :collection => {:create_from_abroad => :post} |
19 | end | 19 | end |
20 | map.resources :algorithms | 20 | map.resources :algorithms |
21 | map.connect "/questions/:question_id/prompts/:id/vote/:index", :controller => 'prompts', :action => 'vote' | 21 | map.connect "/questions/:question_id/prompts/:id/vote/:index", :controller => 'prompts', :action => 'vote' |
@@ -0,0 +1,17 @@ | @@ -0,0 +1,17 @@ | ||
1 | +class CreateFlags < ActiveRecord::Migration | ||
2 | + def self.up | ||
3 | + create_table :flags do |table| | ||
4 | + table.string :explanation, :default => "" | ||
5 | + table.integer :visitor_id | ||
6 | + table.integer :choice_id | ||
7 | + table.integer :question_id | ||
8 | + table.integer :site_id | ||
9 | + table.timestamps | ||
10 | + end | ||
11 | + | ||
12 | + end | ||
13 | + | ||
14 | + def self.down | ||
15 | + drop_table :flags | ||
16 | + end | ||
17 | +end |
lib/activation.rb
@@ -1,16 +0,0 @@ | @@ -1,16 +0,0 @@ | ||
1 | -module Activation | ||
2 | - def activate! | ||
3 | - (self.active = true) | ||
4 | - self.save! | ||
5 | - end | ||
6 | - | ||
7 | - def suspend! | ||
8 | - (self.active = false) | ||
9 | - self.save! | ||
10 | - end | ||
11 | - | ||
12 | - def deactivate! | ||
13 | - (self.active = false) | ||
14 | - self.save! | ||
15 | - end | ||
16 | -end | ||
17 | \ No newline at end of file | 0 | \ No newline at end of file |
@@ -0,0 +1,80 @@ | @@ -0,0 +1,80 @@ | ||
1 | +require File.expand_path(File.dirname(__FILE__) + '/../spec_helper') | ||
2 | + | ||
3 | +describe ChoicesController do | ||
4 | + | ||
5 | + def sign_in_as(user) | ||
6 | + @controller.current_user = user | ||
7 | + return user | ||
8 | + end | ||
9 | + # | ||
10 | + before(:each) do | ||
11 | + sign_in_as(@user = Factory(:email_confirmed_user)) | ||
12 | + end | ||
13 | + # | ||
14 | + def mock_question(stubs={}) | ||
15 | + @mock_question ||= mock_model(Question, stubs) | ||
16 | + end | ||
17 | + | ||
18 | + def mock_prompt(stubs={}) | ||
19 | + @mock_prompt ||= mock_model(Prompt, stubs) | ||
20 | + end | ||
21 | + | ||
22 | + def mock_appearance(stubs={}) | ||
23 | + @mock_appearance||= mock_model(Appearance, stubs) | ||
24 | + end | ||
25 | + | ||
26 | + def mock_visitor(stubs={}) | ||
27 | + @mock_visitor||= mock_model(Visitor, stubs) | ||
28 | + end | ||
29 | + | ||
30 | + def mock_choice(stubs={}) | ||
31 | + @mock_choice||= mock_model(Choice, stubs) | ||
32 | + end | ||
33 | + | ||
34 | + def mock_flag(stubs={}) | ||
35 | + @mock_flag ||= mock_model(Flag, stubs) | ||
36 | + end | ||
37 | + | ||
38 | + describe "PUT flag" do | ||
39 | + before(:each) do | ||
40 | + question_list = [mock_question] | ||
41 | + @user.stub!(:questions).and_return(question_list) | ||
42 | + question_list.stub!(:find).with("37").and_return(mock_question) | ||
43 | + | ||
44 | + choice_list = [mock_choice] | ||
45 | + mock_question.stub!(:choices).and_return(choice_list) | ||
46 | + choice_list.stub!(:find).with("123").and_return(mock_choice) | ||
47 | + mock_choice.should_receive(:deactivate!).and_return(true) | ||
48 | + | ||
49 | + | ||
50 | + end | ||
51 | + | ||
52 | + it "deactives a choice when a flag request is sent" do | ||
53 | + Flag.should_receive(:create!).with({:choice_id => 123, :question_id => 37, :site_id => @user.id}) | ||
54 | + put :flag, :id => 123, :question_id => 37 | ||
55 | + | ||
56 | + assigns[:choice].should == mock_choice | ||
57 | + end | ||
58 | + | ||
59 | + it "adds explanation params to flag if sent" do | ||
60 | + Flag.should_receive(:create!).with({:choice_id => 123, :question_id => 37, :site_id => @user.id, :explanation => "This is offensive"}) | ||
61 | + put :flag, :id => 123, :question_id => 37 , :explanation => "This is offensive" | ||
62 | + | ||
63 | + assigns[:choice].should == mock_choice | ||
64 | + end | ||
65 | + | ||
66 | + it "adds visitor_id params to flag if sent" do | ||
67 | + @visitor_identifier = "somelongunique32charstring" | ||
68 | + visitor_list = [mock_visitor] | ||
69 | + @user.stub!(:visitors).and_return(visitor_list) | ||
70 | + visitor_list.should_receive(:find_or_create_by_identifier).with(@visitor_identifier).and_return(mock_visitor) | ||
71 | + | ||
72 | + Flag.should_receive(:create!).with({:choice_id => 123, :question_id => 37, :site_id => @user.id, :explanation => "This is offensive", :visitor_id => mock_visitor.id}) | ||
73 | + | ||
74 | + put :flag, :id => 123, :question_id => 37 , :explanation => "This is offensive", :visitor_identifier => @visitor_identifier | ||
75 | + | ||
76 | + assigns[:choice].should == mock_choice | ||
77 | + end | ||
78 | + end | ||
79 | + | ||
80 | +end |
spec/models/choice_spec.rb
@@ -4,6 +4,7 @@ describe Choice do | @@ -4,6 +4,7 @@ describe Choice do | ||
4 | 4 | ||
5 | it {should belong_to :question} | 5 | it {should belong_to :question} |
6 | it {should belong_to :item} | 6 | it {should belong_to :item} |
7 | + it {should have_many :flags} | ||
7 | it {should validate_presence_of :question} | 8 | it {should validate_presence_of :question} |
8 | 9 | ||
9 | before(:each) do | 10 | before(:each) do |
@@ -27,4 +28,10 @@ describe Choice do | @@ -27,4 +28,10 @@ describe Choice do | ||
27 | choice1 = Choice.create!(@valid_attributes.merge(:data => '1234')) | 28 | choice1 = Choice.create!(@valid_attributes.merge(:data => '1234')) |
28 | @question.prompts.should_not be_empty | 29 | @question.prompts.should_not be_empty |
29 | end | 30 | end |
31 | + | ||
32 | + it "should deactivate a choice" do | ||
33 | + choice1 = Choice.create!(@valid_attributes.merge(:data => '1234')) | ||
34 | + choice1.deactivate! | ||
35 | + choice1.should_not be_active | ||
36 | + end | ||
30 | end | 37 | end |
@@ -0,0 +1,24 @@ | @@ -0,0 +1,24 @@ | ||
1 | +require 'spec_helper' | ||
2 | + | ||
3 | +describe Flag do | ||
4 | + it {should belong_to :question} | ||
5 | + it {should belong_to :choice} | ||
6 | + it {should belong_to :site} | ||
7 | + it {should belong_to :visitor} | ||
8 | + it {should validate_presence_of :choice_id} | ||
9 | + it {should validate_presence_of :question_id} | ||
10 | + | ||
11 | + before(:each) do | ||
12 | + @valid_attributes = { | ||
13 | + :explanation => "value for explanation", | ||
14 | + :visitor_id => 1, | ||
15 | + :choice_id => 1, | ||
16 | + :question_id => 1, | ||
17 | + :site_id => 1 | ||
18 | + } | ||
19 | + end | ||
20 | + | ||
21 | + it "should create a new instance given valid attributes" do | ||
22 | + Flag.create!(@valid_attributes) | ||
23 | + end | ||
24 | +end |