diff options
| -rw-r--r-- | app/controllers/request_controller.rb | 12 | ||||
| -rw-r--r-- | app/controllers/request_game_controller.rb | 13 | ||||
| -rw-r--r-- | app/models/info_request.rb | 45 | ||||
| -rw-r--r-- | app/models/info_request_event.rb | 22 | ||||
| -rw-r--r-- | app/models/request_classification.rb | 16 | ||||
| -rw-r--r-- | app/views/request_game/play.rhtml | 12 | ||||
| -rw-r--r-- | db/migrate/20120910153022_create_request_classifications.rb | 14 | ||||
| -rw-r--r-- | lib/tasks/temp.rake | 9 | ||||
| -rw-r--r-- | spec/controllers/request_controller_spec.rb | 14 | ||||
| -rw-r--r-- | spec/models/info_request_spec.rb | 51 | ||||
| -rw-r--r-- | spec/models/request_mailer_spec.rb | 33 |
11 files changed, 169 insertions, 72 deletions
diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb index 6e983a014..268ecc73a 100644 --- a/app/controllers/request_controller.rb +++ b/app/controllers/request_controller.rb @@ -422,19 +422,19 @@ class RequestController < ApplicationController old_described_state = @info_request.described_state @info_request.set_described_state(params[:incoming_message][:described_state]) - # If you're not the *actual* requester owner. e.g. you are playing the + # If you're not the *actual* requester. e.g. you are playing the # classification game, or you're doing this just because you are an # admin user (not because you also own the request). if !@info_request.is_actual_owning_user?(authenticated_user) - # Log what you did, for classification game score purposes. We - # don't log if you were the requester XXX This is presumably so you - # don't score for classifying your own requests. Could instead - # always log and filter at display time. - @info_request.log_event("status_update", + # Log the status change by someone other than the requester + event = @info_request.log_event("status_update", { :user_id => authenticated_user.id, :old_described_state => old_described_state, :described_state => @info_request.described_state, }) + # Create a classification event for league tables + RequestClassification.create!(:user_id => authenticated_user.id, + :info_request_event_id => event.id) # Don't give advice on what to do next, as it isn't their request RequestMailer.deliver_old_unclassified_updated(@info_request) if !@info_request.is_external? diff --git a/app/controllers/request_game_controller.rb b/app/controllers/request_game_controller.rb index 904c44759..f22652dd1 100644 --- a/app/controllers/request_game_controller.rb +++ b/app/controllers/request_game_controller.rb @@ -11,13 +11,12 @@ class RequestGameController < ApplicationController def play session[:request_game] = Time.now - old = InfoRequest.find_old_unclassified(:conditions => ["prominence = 'normal'"]) - @missing = old.size + @missing = InfoRequest.count_old_unclassified(:conditions => ["prominence = 'normal'"]) @total = InfoRequest.count @done = @total - @missing @percentage = (@done.to_f / @total.to_f * 10000).round / 100.0 - @requests = old.sort_by{ rand }.slice(0..2) + @requests = InfoRequest.get_random_old_unclassified(3) if @missing == 0 flash[:notice] = _('<p>All done! Thank you very much for your help.</p><p>There are <a href="{{helpus_url}}">more things you can do</a> to help {{site_name}}.</p>', @@ -25,12 +24,8 @@ class RequestGameController < ApplicationController :site_name => site_name) end - @league_table_28_days = InfoRequestEvent.make_league_table( - [ "event_type = 'status_update' and created_at >= ?", Time.now() - 28.days ] - )[0..10] - @league_table_all_time = InfoRequestEvent.make_league_table( - [ "event_type = 'status_update'"] - )[0..10] + @league_table_28_days = RequestClassification.league_table(10, [ "created_at >= ?", Time.now() - 28.days ]) + @league_table_all_time = RequestClassification.league_table(10) @play_urls = true end diff --git a/app/models/info_request.rb b/app/models/info_request.rb index 6f472c290..f2d8929bc 100644 --- a/app/models/info_request.rb +++ b/app/models/info_request.rb @@ -223,7 +223,7 @@ class InfoRequest < ActiveRecord::Base incoming_message.clear_in_database_caches! end end - + # For debugging def InfoRequest.profile_search(query) t = Time.now.usec @@ -939,17 +939,44 @@ public # Used to find when event last changed def InfoRequest.last_event_time_clause(event_type=nil) event_type_clause = '' - event_type_clause = " and info_request_events.event_type = '#{event_type}'" if event_type - "(select created_at from info_request_events where info_request_events.info_request_id = info_requests.id#{event_type_clause} order by created_at desc limit 1)" + event_type_clause = " AND info_request_events.event_type = '#{event_type}'" if event_type + "(SELECT created_at + FROM info_request_events + WHERE info_request_events.info_request_id = info_requests.id + #{event_type_clause} + ORDER BY created_at desc + LIMIT 1)" end - def InfoRequest.find_old_unclassified(extra_params={}) + def InfoRequest.old_unclassified_params(extra_params, include_last_response_time=false) last_response_created_at = last_event_time_clause('response') age = extra_params[:age_in_days] ? extra_params[:age_in_days].days : OLD_AGE_IN_DAYS - params = {:select => "*, #{last_response_created_at} as last_response_time", - :conditions => ["awaiting_description = ? and #{last_response_created_at} < ? and url_title != 'holding_pen' and user_id is not null", - true, Time.now() - age], - :order => "last_response_time"} + params = { :conditions => ["awaiting_description = ? + AND #{last_response_created_at} < ? + AND url_title != 'holding_pen' + AND user_id IS NOT NULL", + true, Time.now() - age] } + if include_last_response_time + params[:select] = "*, #{last_response_created_at} AS last_response_time" + params[:order] = 'last_response_time' + end + return params + end + + def InfoRequest.count_old_unclassified(extra_params={}) + params = old_unclassified_params(extra_params) + count(:all, params) + end + + def InfoRequest.get_random_old_unclassified(limit) + params = old_unclassified_params({}) + params[:limit] = limit + params[:order] = "random()" + find(:all, params) + end + + def InfoRequest.find_old_unclassified(extra_params={}) + params = old_unclassified_params(extra_params, include_last_response_time=true) params[:limit] = extra_params[:limit] if extra_params[:limit] params[:include] = extra_params[:include] if extra_params[:include] if extra_params[:order] @@ -958,7 +985,7 @@ public end if extra_params[:conditions] condition_string = extra_params[:conditions].shift - params[:conditions][0] += " and #{condition_string}" + params[:conditions][0] += " AND #{condition_string}" params[:conditions] += extra_params[:conditions] end find(:all, params) diff --git a/app/models/info_request_event.rb b/app/models/info_request_event.rb index a827d19a4..54d2f5ef7 100644 --- a/app/models/info_request_event.rb +++ b/app/models/info_request_event.rb @@ -43,7 +43,7 @@ class InfoRequestEvent < ActiveRecord::Base 'resent', 'followup_sent', 'followup_resent', - + 'edit', # title etc. edited (in admin interface) 'edit_outgoing', # outgoing message edited (in admin interface) 'edit_comment', # comment edited (in admin interface) @@ -53,7 +53,7 @@ class InfoRequestEvent < ActiveRecord::Base 'move_request', # changed user or public body (in admin interface) 'hide', # hid a request (in admin interface) 'manual', # you did something in the db by hand - + 'response', 'comment', 'status_update' @@ -389,24 +389,6 @@ class InfoRequestEvent < ActiveRecord::Base return TMail::Address.parse(prev_addr).address == TMail::Address.parse(curr_addr).address end - # Given a find condition clause, creates a league table of users who made those events. - # XXX this isn't very generic yet, it is just used for the categorisation game tables. - def InfoRequestEvent.make_league_table(conditions) - status_update_events = InfoRequestEvent.find(:all, :conditions => conditions) - table = Hash.new { |h,k| h[k] = 0 } - for event in status_update_events - user_id = event.params[:user_id] - table[user_id] += 1 - end - league_table = [] - for user_id, count in table - user = User.find(user_id) - league_table.push([user, count]) - end - league_table.sort! { |a,b| b[1] <=> a[1] } - return league_table - end - def json_for_api(deep, snippet_highlight_proc = nil) ret = { :id => self.id, diff --git a/app/models/request_classification.rb b/app/models/request_classification.rb new file mode 100644 index 000000000..678b6cd16 --- /dev/null +++ b/app/models/request_classification.rb @@ -0,0 +1,16 @@ +class RequestClassification < ActiveRecord::Base + belongs_to :user + + # return classification instances representing the top n + # users, with a 'cnt' attribute representing the number + # of classifications the user has made. + def RequestClassification.league_table(size, conditions=[]) + find(:all, :select => 'user_id, count(*) as cnt', + :conditions => conditions, + :group => 'user_id', + :order => 'cnt desc', + :limit => size, + :include => :user) + end + +end
\ No newline at end of file diff --git a/app/views/request_game/play.rhtml b/app/views/request_game/play.rhtml index acf6cce87..eedf19ca2 100644 --- a/app/views/request_game/play.rhtml +++ b/app/views/request_game/play.rhtml @@ -7,22 +7,22 @@ </p> <h2>Top recent players</h2> <table> - <% c = 0; for user, count in @league_table_28_days %> + <% c = 0; for classifications in @league_table_28_days %> <tr> <td> <%= c += 1 %>. <td> - <td> <%= user_link(user) %> </td> - <td> <%=pluralize(count, 'request').gsub(" ", " ")%> </td> + <td> <%= user_link(classifications.user) %> </td> + <td> <%=pluralize(classifications.cnt, 'request').gsub(" ", " ")%> </td> </tr> <% end %> </table> <h2>All time best players</h2> <table> - <% c = 0; for user, count in @league_table_all_time %> + <% c = 0; for classifications in @league_table_all_time %> <tr> <td> <%= c += 1 %>. <td> - <td> <%= user_link(user) %> </td> - <td> <%=pluralize(count, 'request').gsub(" ", " ")%> </td> + <td> <%= user_link(classifications.user) %> </td> + <td> <%= pluralize(classifications.cnt, 'request').gsub(" ", " ")%> </td> </tr> <% end %> </table> diff --git a/db/migrate/20120910153022_create_request_classifications.rb b/db/migrate/20120910153022_create_request_classifications.rb new file mode 100644 index 000000000..7c6270c9e --- /dev/null +++ b/db/migrate/20120910153022_create_request_classifications.rb @@ -0,0 +1,14 @@ +class CreateRequestClassifications < ActiveRecord::Migration + def self.up + create_table :request_classifications do |t| + t.integer :user_id + t.integer :info_request_event_id + t.timestamps + end + add_index :request_classifications, :user_id + end + + def self.down + drop_table :request_classifications + end +end diff --git a/lib/tasks/temp.rake b/lib/tasks/temp.rake index 9decc13db..e49a84ecb 100644 --- a/lib/tasks/temp.rake +++ b/lib/tasks/temp.rake @@ -1,5 +1,14 @@ namespace :temp do + desc 'Populate the request_classifications table from info_request_events' + task :populate_request_classifications => :environment do + InfoRequestEvent.find_each(:conditions => ["event_type = 'status_update'"]) do |classification| + RequestClassification.create!(:created_at => classification.created_at, + :user_id => classification.params[:user_id], + :info_request_event_id => classification.id) + end + end + desc "Remove plaintext passwords from post_redirect params" task :remove_post_redirect_passwords => :environment do PostRedirect.find_each(:conditions => ['post_params_yaml is not null']) do |post_redirect| diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb index b8425613c..95737a250 100644 --- a/spec/controllers/request_controller_spec.rb +++ b/spec/controllers/request_controller_spec.rb @@ -1275,7 +1275,8 @@ describe RequestController, "when classifying an information request" do expected_params = {:user_id => users(:silly_name_user).id, :old_described_state => 'waiting_response', :described_state => 'rejected'} - @dog_request.should_receive(:log_event).with("status_update", expected_params) + event = mock_model(InfoRequestEvent) + @dog_request.should_receive(:log_event).with("status_update", expected_params).and_return(event) post_status('rejected') end @@ -1314,10 +1315,19 @@ describe RequestController, "when classifying an information request" do end it 'should log a status update event' do + event = mock_model(InfoRequestEvent) expected_params = {:user_id => @admin_user.id, :old_described_state => 'waiting_response', :described_state => 'rejected'} - @dog_request.should_receive(:log_event).with("status_update", expected_params) + @dog_request.should_receive(:log_event).with("status_update", expected_params).and_return(event) + post_status('rejected') + end + + it 'should record a classification' do + event = mock_model(InfoRequestEvent) + @dog_request.stub!(:log_event).with("status_update", anything()).and_return(event) + RequestClassification.should_receive(:create!).with(:user_id => @admin_user.id, + :info_request_event_id => event.id) post_status('rejected') end diff --git a/spec/models/info_request_spec.rb b/spec/models/info_request_spec.rb index c55127992..76be32e0c 100644 --- a/spec/models/info_request_spec.rb +++ b/spec/models/info_request_spec.rb @@ -350,20 +350,49 @@ describe InfoRequest do end it 'should add extra conditions if supplied' do - InfoRequest.should_receive(:find).with(:all, - {:select=> anything, - :order=> anything, - :conditions=>["awaiting_description = ? and (select created_at from info_request_events where info_request_events.info_request_id = info_requests.id and info_request_events.event_type = 'response' order by created_at desc limit 1) < ? and url_title != 'holding_pen' and user_id is not null and prominence != 'backpage'", - true, Time.now - 21.days]}) + expected_conditions = ["awaiting_description = ? + AND (SELECT created_at + FROM info_request_events + WHERE info_request_events.info_request_id = info_requests.id + AND info_request_events.event_type = 'response' + ORDER BY created_at desc LIMIT 1) < ? + AND url_title != 'holding_pen' + AND user_id IS NOT NULL + AND prominence != 'backpage'".split(' ').join(' '), + true, Time.now - 21.days] + # compare conditions ignoring whitespace differences + InfoRequest.should_receive(:find) do |all, query_params| + query_string = query_params[:conditions][0] + query_params[:conditions][0] = query_string.split(' ').join(' ') + query_params[:conditions].should == expected_conditions + end InfoRequest.find_old_unclassified({:conditions => ["prominence != 'backpage'"]}) end - it 'should ask the database for requests that are awaiting description, have a last response older than 21 days old, are not the holding pen and are not backpaged' do - InfoRequest.should_receive(:find).with(:all, - {:select=>"*, (select created_at from info_request_events where info_request_events.info_request_id = info_requests.id and info_request_events.event_type = 'response' order by created_at desc limit 1) as last_response_time", - :order=>"last_response_time", - :conditions=>["awaiting_description = ? and (select created_at from info_request_events where info_request_events.info_request_id = info_requests.id and info_request_events.event_type = 'response' order by created_at desc limit 1) < ? and url_title != 'holding_pen' and user_id is not null", - true, Time.now - 21.days]}) + it 'should ask the database for requests that are awaiting description, have a last response older + than 21 days old, have a user, are not the holding pen and are not backpaged' do + expected_conditions = ["awaiting_description = ? + AND (SELECT created_at + FROM info_request_events + WHERE info_request_events.info_request_id = info_requests.id + AND info_request_events.event_type = 'response' + ORDER BY created_at desc LIMIT 1) < ? + AND url_title != 'holding_pen' + AND user_id IS NOT NULL".split(' ').join(' '), + true, Time.now - 21.days] + expected_select = "*, (SELECT created_at + FROM info_request_events + WHERE info_request_events.info_request_id = info_requests.id + AND info_request_events.event_type = 'response' + ORDER BY created_at desc LIMIT 1) + AS last_response_time".split(' ').join(' ') + InfoRequest.should_receive(:find) do |all, query_params| + query_string = query_params[:conditions][0] + query_params[:conditions][0] = query_string.split(' ').join(' ') + query_params[:conditions].should == expected_conditions + query_params[:select].split(' ').join(' ').should == expected_select + query_params[:order].should == "last_response_time" + end InfoRequest.find_old_unclassified end diff --git a/spec/models/request_mailer_spec.rb b/spec/models/request_mailer_spec.rb index ea75ec765..98681a9e9 100644 --- a/spec/models/request_mailer_spec.rb +++ b/spec/models/request_mailer_spec.rb @@ -29,7 +29,7 @@ describe RequestMailer, " when receiving incoming mail" do InfoRequest.holding_pen_request.incoming_messages.size.should == 1 last_event = InfoRequest.holding_pen_request.incoming_messages[0].info_request.get_last_event last_event.params[:rejected_reason].should == "Could not identify the request from the email address" - + deliveries = ActionMailer::Base.deliveries deliveries.size.should == 1 mail = deliveries[0] @@ -49,7 +49,7 @@ describe RequestMailer, " when receiving incoming mail" do InfoRequest.holding_pen_request.incoming_messages.size.should == 1 last_event = InfoRequest.holding_pen_request.incoming_messages[0].info_request.get_last_event last_event.params[:rejected_reason].should =~ /there is no "From" address/ - + deliveries = ActionMailer::Base.deliveries deliveries.size.should == 1 mail = deliveries[0] @@ -69,7 +69,7 @@ describe RequestMailer, " when receiving incoming mail" do InfoRequest.holding_pen_request.incoming_messages.size.should == 1 last_event = InfoRequest.holding_pen_request.incoming_messages[0].info_request.get_last_event last_event.params[:rejected_reason].should =~ /Only the authority can reply/ - + deliveries = ActionMailer::Base.deliveries deliveries.size.should == 1 mail = deliveries[0] @@ -222,12 +222,27 @@ describe RequestMailer, "when sending reminders to requesters to classify a resp RequestMailer.alert_new_response_reminders_internal(7, 'new_response_reminder_1') end - it 'should ask for all requests that are awaiting description and whose latest response is older than the number of days given and that are not the holding pen' do - expected_params = {:conditions => [ "awaiting_description = ? and (select created_at from info_request_events where info_request_events.info_request_id = info_requests.id and info_request_events.event_type = 'response' order by created_at desc limit 1) < ? and url_title != 'holding_pen' and user_id is not null", - true, Time.now() - 7.days ], - :include => [ :user ], - :order => "info_requests.id"} - InfoRequest.should_receive(:find).with(:all, expected_params).and_return([]) + it 'should ask for all requests that are awaiting description and whose latest response is older + than the number of days given and that are not the holding pen' do + expected_conditions = [ "awaiting_description = ? + AND (SELECT created_at + FROM info_request_events + WHERE info_request_events.info_request_id = info_requests.id + AND info_request_events.event_type = 'response' + ORDER BY created_at desc LIMIT 1) < ? + AND url_title != 'holding_pen' + AND user_id IS NOT NULL".split(' ').join(' '), + true, Time.now() - 7.days ] + + # compare the query string ignoring any spacing differences + InfoRequest.should_receive(:find) do |all, query_params| + query_string = query_params[:conditions][0] + query_params[:conditions][0] = query_string.split(' ').join(' ') + query_params[:conditions].should == expected_conditions + query_params[:include].should == [ :user ] + query_params[:order].should == 'info_requests.id' + end + send_alerts end |