diff options
| -rw-r--r-- | app/controllers/request_controller.rb | 10 | ||||
| -rw-r--r-- | app/models/info_request.rb | 13 | ||||
| -rw-r--r-- | app/models/info_request_event.rb | 10 | ||||
| -rw-r--r-- | app/views/admin_request/show.rhtml | 2 | ||||
| -rw-r--r-- | app/views/layouts/default.rhtml | 1 | ||||
| -rw-r--r-- | app/views/request/_correspondence.rhtml | 68 | ||||
| -rw-r--r-- | app/views/request/_followup.rhtml | 5 | ||||
| -rw-r--r-- | app/views/request/describe_state.rhtml | 9 | ||||
| -rw-r--r-- | app/views/request/show.rhtml | 4 | ||||
| -rw-r--r-- | app/views/request/show_response.rhtml | 4 | ||||
| -rw-r--r-- | db/migrate/022_create_info_request_events.rb | 38 | ||||
| -rw-r--r-- | db/migrate/028_give_incoming_messages_events.rb | 19 | ||||
| -rw-r--r-- | db/schema.rb | 2 | ||||
| -rw-r--r-- | public/stylesheets/main.css | 1 | ||||
| -rw-r--r-- | spec/controllers/request_controller_spec.rb | 12 | ||||
| -rw-r--r-- | spec/controllers/user_controller_spec.rb | 2 | ||||
| -rw-r--r-- | todo.txt | 25 |
17 files changed, 120 insertions, 105 deletions
diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb index 9e06a027b..f732cf687 100644 --- a/app/controllers/request_controller.rb +++ b/app/controllers/request_controller.rb @@ -4,14 +4,14 @@ # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved. # Email: francis@mysociety.org; WWW: http://www.mysociety.org/ # -# $Id: request_controller.rb,v 1.42 2008-01-29 03:05:46 francis Exp $ +# $Id: request_controller.rb,v 1.43 2008-02-01 15:27:48 francis Exp $ class RequestController < ApplicationController def show @info_request = InfoRequest.find(params[:id]) - @correspondences = @info_request.incoming_messages + @info_request.info_request_events - @correspondences.sort! { |a,b| a.sent_at <=> b.sent_at } + @info_request_events = @info_request.info_request_events + @info_request_events.sort! { |a,b| a.created_at <=> b.created_at } @status = @info_request.calculate_status @date_response_required_by = @info_request.date_response_required_by @collapse_quotes = params[:unfold] ? false : true @@ -164,8 +164,8 @@ class RequestController < ApplicationController if !@outgoing_message.valid? render :action => 'show_response' elsif authenticated_as_user?(@info_request.user, - :web => "To send a follow up message about your FOI request", - :email => "Then you can send a follow up message to " + @info_request.public_body.name + ".", + :web => "To send your follow up message about your FOI request", + :email => "Then your follow up message to " + @info_request.public_body.name + " will be sent.", :email_subject => "Confirm your FOI follow up message to " + @info_request.public_body.name ) # Send a follow up message diff --git a/app/models/info_request.rb b/app/models/info_request.rb index 5ff0b3a1c..8743a8572 100644 --- a/app/models/info_request.rb +++ b/app/models/info_request.rb @@ -20,7 +20,7 @@ # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved. # Email: francis@mysociety.org; WWW: http://www.mysociety.org/ # -# $Id: info_request.rb,v 1.32 2008-01-30 09:53:47 francis Exp $ +# $Id: info_request.rb,v 1.33 2008-02-01 15:27:49 francis Exp $ require 'digest/sha1' @@ -102,6 +102,7 @@ public incoming_message.save! self.awaiting_description = true + self.log_event("response", { :incoming_message_id => incoming_message.id }) self.save! end @@ -155,7 +156,7 @@ public # Possibly just show 20 working days since the *last* message? Hmmm. earliest_sent = self.outgoing_messages.map { |om| om.last_sent_at }.min if earliest_sent.nil? - raise "internal error, minimum last_sent_at for outgoing_messages is nil for request " + self.id.to_s + raise "internal error, minimum last_sent_at for outgoing_messages is nil for request " + self.id.to_s + " outgoing messages count " + self.outgoing_messages.size.to_s end days_passed = 0 @@ -227,12 +228,12 @@ public # Returns all the messages which the user hasn't described yet def incoming_messages_needing_description if self.described_last_incoming_message_id.nil? - correspondences = self.incoming_messages.find(:all) + incoming_messages = self.incoming_messages.find(:all) else - correspondences = self.incoming_messages.find(:all, :conditions => "id > " + self.described_last_incoming_message_id.to_s) + incoming_messages = self.incoming_messages.find(:all, :conditions => "id > " + self.described_last_incoming_message_id.to_s) end - correspondences.sort! { |a,b| a.sent_at <=> b.sent_at } - return correspondences + incoming_messages.sort! { |a,b| a.sent_at <=> b.sent_at } + return incoming_messages end protected diff --git a/app/models/info_request_event.rb b/app/models/info_request_event.rb index 0b5ac64f5..f291daca5 100644 --- a/app/models/info_request_event.rb +++ b/app/models/info_request_event.rb @@ -15,7 +15,7 @@ # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved. # Email: francis@mysociety.org; WWW: http://www.mysociety.org/ # -# $Id: info_request_event.rb,v 1.8 2008-01-29 01:26:21 francis Exp $ +# $Id: info_request_event.rb,v 1.9 2008-02-01 15:27:49 francis Exp $ class InfoRequestEvent < ActiveRecord::Base belongs_to :info_request @@ -28,7 +28,8 @@ class InfoRequestEvent < ActiveRecord::Base 'followup_sent', 'followup_resent', 'edit_outgoing', # outgoing message edited in admin interface - 'manual' # you did something in the db by hand + 'manual', # you did something in the db by hand + 'response' ] # We store YAML version of parameters in the database @@ -39,11 +40,6 @@ class InfoRequestEvent < ActiveRecord::Base YAML.load(self.params_yaml) end - # Used for sorting with the incoming/outgoing messages - def sent_at - created_at - end - end diff --git a/app/views/admin_request/show.rhtml b/app/views/admin_request/show.rhtml index 1dd82dc6e..6e4c432d6 100644 --- a/app/views/admin_request/show.rhtml +++ b/app/views/admin_request/show.rhtml @@ -73,7 +73,7 @@ <% end %> </tr> -<% for info_request_event in @info_request.info_request_events %> +<% for info_request_event in @info_request.info_request_events.find(:all, :order => "created_at, id") %> <tr class="<%= cycle('odd', 'even') %>"> <% for column in InfoRequestEvent.content_columns.map { |c| c.name } %> <td><%=h info_request_event.send(column) %></td> diff --git a/app/views/layouts/default.rhtml b/app/views/layouts/default.rhtml index 02398dc01..f3437b8dd 100644 --- a/app/views/layouts/default.rhtml +++ b/app/views/layouts/default.rhtml @@ -21,7 +21,6 @@ <% else %> <% end %> <li><%= link_to "About", about_url %></li> - <!-- <li><a href="/about">About</a></li> --> </ul> <% if MySociety::Config.getbool("STAGING_SITE", 1) %> diff --git a/app/views/request/_correspondence.rhtml b/app/views/request/_correspondence.rhtml index 6758bb189..1ff42c5e1 100644 --- a/app/views/request/_correspondence.rhtml +++ b/app/views/request/_correspondence.rhtml @@ -1,28 +1,31 @@ <div id="correspondence"> -<% @last_email = nil - - if (correspondence.class.to_s == 'IncomingMessage') - incoming_message = correspondence%> - <%= render :partial => 'bubble', :locals => { :incoming_message => incoming_message, :body => incoming_message.get_body_for_html_display(@collapse_quotes), :attachments => incoming_message.get_attachments_for_display } %> +<% + @last_email = nil + if !info_request_event.nil? && info_request_event.event_type == 'response' + incoming_message = IncomingMessage.find(info_request_event.params[:incoming_message_id]) + end + + if not incoming_message.nil? + %> + <%= render :partial => 'bubble', :locals => { :incoming_message => incoming_message, :body => incoming_message.get_body_for_html_display(@collapse_quotes), :attachments => incoming_message.get_attachments_for_display } %> - <p class="event_bubble"> - <% if !incoming_message.safe_mail_from.nil? %> - <%= incoming_message.safe_mail_from %> of - <% end %> - <%= public_body_link(@info_request.public_body) %> - <% if incoming_message.is_bounce %> - replied automatically - <% else %> - replied - <% end %> - on <strong><%= simple_date(incoming_message.sent_at) %></strong> - (<%= link_to "link to this", show_response_url(:id => incoming_message.info_request.id, :incoming_message_id => incoming_message.id) %>, - <%= link_to "reply", show_response_url(:id => incoming_message.info_request.id, :incoming_message_id => incoming_message.id) + "#show_response_followup" %>) - </p> -<% elsif (correspondence.class.to_s == 'InfoRequestEvent') - info_request_event = correspondence - if info_request_event.event_type == 'sent' || info_request_event.event_type == 'followup_sent' + <p class="event_bubble"> + <% if !incoming_message.safe_mail_from.nil? %> + <%= incoming_message.safe_mail_from %> of + <% end %> + <%= public_body_link(@info_request.public_body) %> + <% if incoming_message.is_bounce %> + replied automatically + <% else %> + replied + <% end %> + on <strong><%= simple_date(incoming_message.sent_at) %></strong> + (<%= link_to "link to this", show_response_url(:id => incoming_message.info_request.id, :incoming_message_id => incoming_message.id) %>, + <%= link_to "reply", show_response_url(:id => incoming_message.info_request.id, :incoming_message_id => incoming_message.id) + "#show_response_followup" %>) + </p> + <% + elsif info_request_event.event_type == 'sent' || info_request_event.event_type == 'followup_sent' outgoing_message = OutgoingMessage.find(info_request_event.params[:outgoing_message_id]) %> <%= render :partial => 'bubble', :locals => { :body => outgoing_message.get_body_for_html_display(), :attachments => nil } %> @@ -33,7 +36,7 @@ <% if outgoing_message.status == 'sent' %> sent the initial request to <%= public_body_link(@info_request.public_body) %> - on <strong><%= simple_date(info_request_event.sent_at) %></strong> + on <strong><%= simple_date(info_request_event.created_at) %></strong> <% elsif outgoing_message.status == 'ready' %> wrote the initial request, but it has <strong>not yet been sent</strong> <% else raise "unknown outgoing_message.status" %> @@ -45,7 +48,7 @@ <%= outgoing_message.incoming_message_followup.safe_mail_from %> of <% end %> <%= public_body_link(@info_request.public_body) %> - on <strong><%= simple_date(info_request_event.sent_at) %></strong> + on <strong><%= simple_date(info_request_event.created_at) %></strong> <% elsif outgoing_message.status == 'ready' %> wrote a follow up message, but it has <strong>not yet been sent</strong> <% else raise "unknown outgoing_message.status" %> @@ -55,19 +58,18 @@ </p> <% elsif info_request_event.event_type == 'resent' %> <p class="event_plain"> - Sent to <%= public_body_link(@info_request.public_body) %> - again (perhaps to a new contact address) <!-- XXX actually say if it is a new one or not --> - on <strong><%= simple_date(info_request_event.sent_at) %></strong> + Sent to <%= public_body_link(@info_request.public_body) %> + again (perhaps to a new contact address) <!-- XXX actually say if it is a new one or not --> + on <strong><%= simple_date(info_request_event.created_at) %></strong> </p> <% end - if ['sent', 'resent'].include?(info_request_event.event_type) - @last_email = info_request_event.params[:email] + if !info_request_event.nil? + if ['sent', 'resent'].include?(info_request_event.event_type) + @last_email = info_request_event.params[:email] + end end - %> -<% else %> - <% raise "Unknown correspondence type " + correspondence.class.to_s %> -<% end %> +%> </div> diff --git a/app/views/request/_followup.rhtml b/app/views/request/_followup.rhtml index f228ca71d..8266661c9 100644 --- a/app/views/request/_followup.rhtml +++ b/app/views/request/_followup.rhtml @@ -1,8 +1,5 @@ <div id="followup"> -<% if (correspondence.class.to_s == 'IncomingMessage') - incoming_message = correspondence%> - <h2>Send a follow up message <% if !incoming_message.safe_mail_from.nil? %> to <%= incoming_message.safe_mail_from %> @@ -32,7 +29,5 @@ <% end %> </p> -<% end %> - </div> diff --git a/app/views/request/describe_state.rhtml b/app/views/request/describe_state.rhtml index a73bd1611..9f8b4afe8 100644 --- a/app/views/request/describe_state.rhtml +++ b/app/views/request/describe_state.rhtml @@ -11,15 +11,12 @@ <h2><%=MySociety::Format.fancy_pluralize(@needing_description.size, 'New response', 'new responses') %> to your request '<%= request_link @info_request %>'</h2> - <% for correspondence in @needing_description %> - <%= render :partial => 'correspondence', :locals => { :correspondence => correspondence } %> + <% for incoming_message in @needing_description %> + <%= render :partial => 'correspondence', :locals => { :info_request_event => nil, :incoming_message => incoming_message } %> <% end %> - - <div id="show_response_followup"> - <%= render :partial => 'followup', :locals => { :correspondence => @incoming_message } %> - </div> </div> <div id="describe_state_form"> <%= render :partial => 'describe_state' %> </div> + diff --git a/app/views/request/show.rhtml b/app/views/request/show.rhtml index dc47de333..681e64f4a 100644 --- a/app/views/request/show.rhtml +++ b/app/views/request/show.rhtml @@ -48,8 +48,8 @@ <% end %> </p> - <% for correspondence in @correspondences %> - <%= render :partial => 'correspondence', :locals => { :correspondence => correspondence } %> + <% for info_request_event in @info_request_events %> + <%= render :partial => 'correspondence', :locals => { :info_request_event => info_request_event, :incoming_message => nil } %> <% end %> </div> diff --git a/app/views/request/show_response.rhtml b/app/views/request/show_response.rhtml index 670dfb08f..8428b7ff7 100644 --- a/app/views/request/show_response.rhtml +++ b/app/views/request/show_response.rhtml @@ -13,10 +13,10 @@ <h2>Response to your request '<%= request_link @info_request %>'</h2> <% end %> - <%= render :partial => 'correspondence', :locals => { :correspondence => @incoming_message } %> + <%= render :partial => 'correspondence', :locals => { :info_request_event => nil, :incoming_message => @incoming_message } %> <div id="show_response_followup"> - <%= render :partial => 'followup', :locals => { :correspondence => @incoming_message } %> + <%= render :partial => 'followup', :locals => { :incoming_message => @incoming_message } %> </div> </div> diff --git a/db/migrate/022_create_info_request_events.rb b/db/migrate/022_create_info_request_events.rb index fe7450773..858444298 100644 --- a/db/migrate/022_create_info_request_events.rb +++ b/db/migrate/022_create_info_request_events.rb @@ -1,24 +1,24 @@ class CreateInfoRequestEvents < ActiveRecord::Migration - def self.up - create_table :info_request_events do |t| - t.column "info_request_id", :integer - t.column :event_type, :text - t.column :params_yaml, :text - t.column :created_at, :datetime - end + def self.up + create_table :info_request_events do |t| + t.column "info_request_id", :integer + t.column :event_type, :text + t.column :params_yaml, :text + t.column :created_at, :datetime + end - # Create the missing events for requests already sent - InfoRequest.find(:all).each do |info_request| - info_request_event = InfoRequestEvent.new - info_request_event.event_type = 'sent' - info_request_event.params = { :email => info_request.recipient_email, :outgoing_message_id => info_request.outgoing_messages[0].id } - info_request_event.info_request = info_request - info_request_event.created_at = info_request.outgoing_messages[0].sent_at - info_request_event.save! + # Create the missing events for requests already sent + InfoRequest.find(:all).each do |info_request| + info_request_event = InfoRequestEvent.new + info_request_event.event_type = 'sent' + info_request_event.params = { :email => info_request.recipient_email, :outgoing_message_id => info_request.outgoing_messages[0].id } + info_request_event.info_request = info_request + info_request_event.created_at = info_request.outgoing_messages[0].sent_at + info_request_event.save! + end end - end - def self.down - drop_table :info_request_events - end + def self.down + drop_table :info_request_events + end end diff --git a/db/migrate/028_give_incoming_messages_events.rb b/db/migrate/028_give_incoming_messages_events.rb new file mode 100644 index 000000000..270c532a0 --- /dev/null +++ b/db/migrate/028_give_incoming_messages_events.rb @@ -0,0 +1,19 @@ +class GiveIncomingMessagesEvents < ActiveRecord::Migration + def self.up + ActiveRecord::Base.transaction do + IncomingMessage.find(:all).each do |incoming_message| + info_request_event = InfoRequestEvent.new + info_request_event.event_type = 'response' + info_request_event.params = { :incoming_message_id => incoming_message.id } + info_request_event.info_request = incoming_message.info_request + info_request_event.created_at = incoming_message.created_at + info_request_event.save! + end + end + end + + def self.down + InfoRequestEvent.delete_all "event_type = 'response'" + end +end + diff --git a/db/schema.rb b/db/schema.rb index 057a239b9..79fdc7e86 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -9,7 +9,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 27) do +ActiveRecord::Schema.define(:version => 28) do create_table "incoming_messages", :force => true do |t| t.integer "info_request_id", :null => false diff --git a/public/stylesheets/main.css b/public/stylesheets/main.css index f17f19354..0d41bafa7 100644 --- a/public/stylesheets/main.css +++ b/public/stylesheets/main.css @@ -317,6 +317,7 @@ table#list_requests .odd { #show_response_view { float: left; width: 70%; + margin-bottom: 2em; } #show_response_followup { margin-top: 3em; diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb index 426224e37..0aa873d08 100644 --- a/spec/controllers/request_controller_spec.rb +++ b/spec/controllers/request_controller_spec.rb @@ -50,7 +50,7 @@ end describe RequestController, "when showing one request" do integrate_views - fixtures :info_requests, :public_bodies, :users, :incoming_messages, :outgoing_messages # all needed as integrating views + fixtures :info_requests, :info_request_events, :public_bodies, :users, :incoming_messages, :outgoing_messages # all needed as integrating views it "should be successful" do get :show, :id => 101 @@ -69,13 +69,13 @@ describe RequestController, "when showing one request" do it "should show incoming messages" do get :show, :id => 101 - size_before = assigns[:correspondences].size + size_before = assigns[:info_request_events].size ir = info_requests(:fancy_dog_request) receive_incoming_mail('incoming-request-plain.email', ir.incoming_email) get :show, :id => 101 - (assigns[:correspondences].size - size_before).should == 1 + (assigns[:info_request_events].size - size_before).should == 1 end end @@ -153,7 +153,7 @@ end describe RequestController, "when viewing an individual response" do integrate_views - fixtures :info_requests, :public_bodies, :users, :incoming_messages, :outgoing_messages # all needed as integrating views + fixtures :info_requests, :info_request_events, :public_bodies, :users, :incoming_messages, :outgoing_messages # all needed as integrating views it "should show the response" do get :show_response, :id => info_requests(:fancy_dog_request).id, :incoming_message_id => incoming_messages(:useless_incoming_message) @@ -163,7 +163,7 @@ end describe RequestController, "when classifying an individual response" do integrate_views - fixtures :info_requests, :public_bodies, :users, :incoming_messages, :outgoing_messages # all needed as integrating views + fixtures :info_requests, :info_request_events, :public_bodies, :users, :incoming_messages, :outgoing_messages # all needed as integrating views it "should require login" do post :describe_state, :incoming_message => { :described_state => "rejected" }, :id => info_requests(:fancy_dog_request).id, :incoming_message_id => incoming_messages(:useless_incoming_message), :submitted_describe_state => 1 @@ -192,7 +192,7 @@ end describe RequestController, "when sending a followup message" do integrate_views - fixtures :info_requests, :public_bodies, :users, :incoming_messages, :outgoing_messages # all needed as integrating views + fixtures :info_requests, :info_request_events, :public_bodies, :users, :incoming_messages, :outgoing_messages # all needed as integrating views it "should require login" do post :show_response, :outgoing_message => { :body => "What a useless response! You suck." }, :id => info_requests(:fancy_dog_request).id, :incoming_message_id => incoming_messages(:useless_incoming_message), :submitted_followup => 1 diff --git a/spec/controllers/user_controller_spec.rb b/spec/controllers/user_controller_spec.rb index 2fa44eb24..d21b6947f 100644 --- a/spec/controllers/user_controller_spec.rb +++ b/spec/controllers/user_controller_spec.rb @@ -2,7 +2,7 @@ require File.dirname(__FILE__) + '/../spec_helper' describe UserController, "when showing a user" do integrate_views - fixtures :users + fixtures :users, :outgoing_messages it "should be successful" do get :show, :simple_name => "bob-smith" @@ -1,16 +1,7 @@ -refactor so incoming messages are in events - hack storing better info so know when state changed for stats purpose, and for RSS ordering how do we change the state when a follow up is sent? -describe_state post should store and check include message was up to? - -Adam's woes: -http://foi.mysociety.org/request/18/response/31 -- Renders badly -- No doubt leaks email address via download.bin :( -- We don't have "this is an evil techy weird bounce" as option for state of message FOI requests to use to test it ============================== @@ -58,14 +49,28 @@ sort orders we need) Make sure this page is OK http://foi.mysociety.org/request/14 +Adam's woes: +http://foi.mysociety.org/request/18/response/31 +- Renders badly +- No doubt leaks email address via download.bin :( +- We don't have "this is an evil techy weird bounce" as option for state of message +And Tom's + http://foi.mysociety.org/request/8 +And me + http://foi.mysociety.org/request/17 + - why duplicate? + - add reply-to header to emails +Consider removing login links from notifications of new responses + Tidying ======= +describe_state post should store and check incoming message was up to? + Work out how to get it to tell you code coverage of .rhtml files Make it validate the HTML maybe with http://www.anodyne.ca/wp-content/uploads/2007/09/be_valid_xhtml.rb Test that it is actually sending the request outgoing mail, by using deliveries -Add fixtures for info_request_event Test sending a message to bounce/envelope-from address Link internally between different bits of admin interface |