diff options
| -rw-r--r-- | app/controllers/request_controller.rb | 4 | ||||
| -rw-r--r-- | app/models/info_request.rb | 6 | ||||
| -rw-r--r-- | app/models/outgoing_mailer.rb | 90 | ||||
| -rw-r--r-- | app/models/outgoing_message.rb | 10 | ||||
| -rw-r--r-- | app/models/request_mailer.rb | 75 | ||||
| -rw-r--r-- | app/views/layouts/outgoing_mailer.rhtml | 1 | ||||
| -rw-r--r-- | app/views/layouts/request_mailer.rhtml | 2 | ||||
| -rw-r--r-- | app/views/outgoing_mailer/followup.rhtml (renamed from app/views/request_mailer/followup.rhtml) | 0 | ||||
| -rw-r--r-- | app/views/outgoing_mailer/initial_request.rhtml (renamed from app/views/request_mailer/initial_request.rhtml) | 0 | ||||
| -rw-r--r-- | app/views/request/_after_actions.rhtml | 4 | ||||
| -rw-r--r-- | app/views/request/_followup.rhtml | 6 | ||||
| -rw-r--r-- | app/views/request/followup_bad.rhtml | 4 | ||||
| -rw-r--r-- | app/views/request/followup_preview.rhtml | 4 | ||||
| -rw-r--r-- | spec/models/request_mailer_spec.rb | 66 |
14 files changed, 113 insertions, 159 deletions
diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb index fa132001b..0f6bb4010 100644 --- a/app/controllers/request_controller.rb +++ b/app/controllers/request_controller.rb @@ -4,7 +4,7 @@ # 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.189 2009-10-03 01:42:01 francis Exp $ +# $Id: request_controller.rb,v 1.190 2009-10-04 21:53:53 francis Exp $ class RequestController < ApplicationController @@ -444,7 +444,7 @@ class RequestController < ApplicationController raise sprintf("Incoming message %d does not belong to request %d", @incoming_message.info_request_id, @info_request.id) end - if !RequestMailer.is_followupable?(@info_request, @incoming_message) + if !OutgoingMailer.is_followupable?(@info_request, @incoming_message) raise "unexpected followupable inconsistency" if @info_request.public_body.is_requestable? @reason = @info_request.public_body.not_requestable_reason render :action => 'followup_bad' diff --git a/app/models/info_request.rb b/app/models/info_request.rb index 6cc76da4f..71722b454 100644 --- a/app/models/info_request.rb +++ b/app/models/info_request.rb @@ -24,7 +24,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.212 2009-10-03 10:32:32 francis Exp $ +# $Id: info_request.rb,v 1.213 2009-10-04 21:53:54 francis Exp $ require 'digest/sha1' require File.join(File.dirname(__FILE__),'../../vendor/plugins/acts_as_xapian/lib/acts_as_xapian') @@ -812,8 +812,8 @@ public for incoming_message in self.incoming_messages.reverse incoming_message.safe_mail_from - email = RequestMailer.email_for_followup(self, incoming_message) - name = RequestMailer.name_for_followup(self, incoming_message) + email = OutgoingMailer.email_for_followup(self, incoming_message) + name = OutgoingMailer.name_for_followup(self, incoming_message) if !done.include?(email.downcase) ret = ret + [[name, email, incoming_message.id]] diff --git a/app/models/outgoing_mailer.rb b/app/models/outgoing_mailer.rb new file mode 100644 index 000000000..ae7e86f4e --- /dev/null +++ b/app/models/outgoing_mailer.rb @@ -0,0 +1,90 @@ +# models/outgoing_mailer.rb: +# Emails which go to public bodies on behalf of users. +# +# Copyright (c) 2009 UK Citizens Online Democracy. All rights reserved. +# Email: francis@mysociety.org; WWW: http://www.mysociety.org/ +# +# $Id: outgoing_mailer.rb,v 1.1 2009-10-04 21:53:54 francis Exp $ + +# Note: The layout for this wraps messages by lines rather than (blank line +# separated) paragraphs, as is the convention for all the other mailers. This +# turned out to fit better with user exepectations when formatting messages. +# +# XXX The other mail templates are written to use blank line separated +# paragraphs. They could be rewritten, and the wrapping method made uniform +# throughout the application. + +class OutgoingMailer < ApplicationMailer + + # Email to public body requesting info + def initial_request(info_request, outgoing_message) + @wrap_lines_as_paragraphs = true + @from = info_request.incoming_name_and_email + @recipients = info_request.recipient_name_and_email + @subject = info_request.email_subject_request + @body = {:info_request => info_request, :outgoing_message => outgoing_message, + :contact_email => MySociety::Config.get("CONTACT_EMAIL", 'contact@localhost') } + end + + # Later message to public body regarding existing request + def followup(info_request, outgoing_message, incoming_message_followup) + @wrap_lines_as_paragraphs = true + @from = info_request.incoming_name_and_email + @recipients = OutgoingMailer.name_and_email_for_followup(info_request, incoming_message_followup) + @subject = OutgoingMailer.subject_for_followup(info_request, outgoing_message) + @body = {:info_request => info_request, :outgoing_message => outgoing_message, + :incoming_message_followup => incoming_message_followup, + :contact_email => MySociety::Config.get("CONTACT_EMAIL", 'contact@localhost') } + end + + # Separate function, so can be called from controller for logging + # XXX the condition checking valid_to_reply_to? also appears in views/request/_followup.rhtml, + # it shouldn't really, should call something here. + # XXX also OutgoingMessage.get_salutation + # XXX these look like they should be members of IncomingMessage, but logically they + # need to work even when IncomingMessage is nil + def OutgoingMailer.name_and_email_for_followup(info_request, incoming_message_followup) + if incoming_message_followup.nil? || !incoming_message_followup.valid_to_reply_to? + return info_request.recipient_name_and_email + else + # calling safe_mail_from from so censor rules are run + return TMail::Address.address_from_name_and_email(incoming_message_followup.safe_mail_from, incoming_message_followup.mail.from_addrs[0].spec).to_s + end + end + # Used in the preview of followup + def OutgoingMailer.name_for_followup(info_request, incoming_message_followup) + if incoming_message_followup.nil? || !incoming_message_followup.valid_to_reply_to? + return info_request.public_body.name + else + # calling safe_mail_from from so censor rules are run + return incoming_message_followup.safe_mail_from || info_request.public_body.name + end + end + # Used when making list of followup places to remove duplicates + def OutgoingMailer.email_for_followup(info_request, incoming_message_followup) + if incoming_message_followup.nil? || !incoming_message_followup.valid_to_reply_to? + return info_request.recipient_email + else + return incoming_message_followup.mail.from_addrs[0].spec + end + end + # Subject to use for followup + def OutgoingMailer.subject_for_followup(info_request, outgoing_message) + if outgoing_message.what_doing == 'internal_review' + return "Internal review of " + info_request.email_subject_request + else + return info_request.email_subject_followup + end + end + # Whether we have a valid email address for a followup + def OutgoingMailer.is_followupable?(info_request, incoming_message_followup) + if incoming_message_followup.nil? || !incoming_message_followup.valid_to_reply_to? + return info_request.recipient_email_valid_for_followup? + else + # email has been checked in incoming_message_followup.valid_to_reply_to? above + return true + end + end + +end + diff --git a/app/models/outgoing_message.rb b/app/models/outgoing_message.rb index 7de70fc17..36190ce2f 100644 --- a/app/models/outgoing_message.rb +++ b/app/models/outgoing_message.rb @@ -22,7 +22,7 @@ # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved. # Email: francis@mysociety.org; WWW: http://www.mysociety.org/ # -# $Id: outgoing_message.rb,v 1.94 2009-10-04 21:42:07 francis Exp $ +# $Id: outgoing_message.rb,v 1.95 2009-10-04 21:53:54 francis Exp $ class OutgoingMessage < ActiveRecord::Base strip_attributes! @@ -43,7 +43,7 @@ class OutgoingMessage < ActiveRecord::Base def get_salutation ret = "Dear " if self.message_type == 'followup' && !self.incoming_message_followup.nil? && !self.incoming_message_followup.safe_mail_from.nil? && self.incoming_message_followup.valid_to_reply_to? - ret = ret + RequestMailer.name_for_followup(self.info_request, self.incoming_message_followup) + ret = ret + OutgoingMailer.name_for_followup(self.info_request, self.incoming_message_followup) else ret = ret + "Sir or Madam" end @@ -145,18 +145,18 @@ class OutgoingMessage < ActiveRecord::Base def send_message(log_event_type = 'sent') if self.status == 'ready' if self.message_type == 'initial_request' - RequestMailer.deliver_initial_request(self.info_request, self) + OutgoingMailer.deliver_initial_request(self.info_request, self) self.last_sent_at = Time.now self.status = 'sent' self.save! self.info_request.log_event(log_event_type, { :email => self.info_request.recipient_name_and_email, :outgoing_message_id => self.id }) self.info_request.set_described_state('waiting_response') elsif self.message_type == 'followup' - RequestMailer.deliver_followup(self.info_request, self, self.incoming_message_followup) + OutgoingMailer.deliver_followup(self.info_request, self, self.incoming_message_followup) self.last_sent_at = Time.now self.status = 'sent' self.save! - self.info_request.log_event('followup_' + log_event_type, { :email => RequestMailer.name_and_email_for_followup(self.info_request, self.incoming_message_followup), :outgoing_message_id => self.id }) + self.info_request.log_event('followup_' + log_event_type, { :email => OutgoingMailer.name_and_email_for_followup(self.info_request, self.incoming_message_followup), :outgoing_message_id => self.id }) if self.info_request.described_state == 'waiting_clarification' self.info_request.set_described_state('waiting_response') end diff --git a/app/models/request_mailer.rb b/app/models/request_mailer.rb index d2e25f268..3aff5ea63 100644 --- a/app/models/request_mailer.rb +++ b/app/models/request_mailer.rb @@ -1,84 +1,13 @@ # models/request_mailer.rb: -# Emails which go to public bodies on behalf of users. +# Alerts relating to requests. # # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved. # Email: francis@mysociety.org; WWW: http://www.mysociety.org/ # -# $Id: request_mailer.rb,v 1.88 2009-10-04 21:42:07 francis Exp $ +# $Id: request_mailer.rb,v 1.89 2009-10-04 21:53:54 francis Exp $ class RequestMailer < ApplicationMailer - # Email to public body requesting info - def initial_request(info_request, outgoing_message) - @wrap_lines_as_paragraphs = true - @from = info_request.incoming_name_and_email - @recipients = info_request.recipient_name_and_email - @subject = info_request.email_subject_request - @body = {:info_request => info_request, :outgoing_message => outgoing_message, - :contact_email => MySociety::Config.get("CONTACT_EMAIL", 'contact@localhost') } - end - - # Later message to public body regarding existing request - def followup(info_request, outgoing_message, incoming_message_followup) - @wrap_lines_as_paragraphs = true - @from = info_request.incoming_name_and_email - @recipients = RequestMailer.name_and_email_for_followup(info_request, incoming_message_followup) - @subject = RequestMailer.subject_for_followup(info_request, outgoing_message) - @body = {:info_request => info_request, :outgoing_message => outgoing_message, - :incoming_message_followup => incoming_message_followup, - :contact_email => MySociety::Config.get("CONTACT_EMAIL", 'contact@localhost') } - end - - # Separate function, so can be called from controller for logging - # XXX the condition checking valid_to_reply_to? also appears in views/request/_followup.rhtml, - # it shouldn't really, should call something here. - # XXX also OutgoingMessage.get_salutation - # XXX these look like they should be members of IncomingMessage, but logically they - # need to work even when IncomingMessage is nil - def RequestMailer.name_and_email_for_followup(info_request, incoming_message_followup) - if incoming_message_followup.nil? || !incoming_message_followup.valid_to_reply_to? - return info_request.recipient_name_and_email - else - # calling safe_mail_from from so censor rules are run - return TMail::Address.address_from_name_and_email(incoming_message_followup.safe_mail_from, incoming_message_followup.mail.from_addrs[0].spec).to_s - end - end - # Used in the preview of followup - def RequestMailer.name_for_followup(info_request, incoming_message_followup) - if incoming_message_followup.nil? || !incoming_message_followup.valid_to_reply_to? - return info_request.public_body.name - else - # calling safe_mail_from from so censor rules are run - return incoming_message_followup.safe_mail_from || info_request.public_body.name - end - end - # Used when making list of followup places to remove duplicates - def RequestMailer.email_for_followup(info_request, incoming_message_followup) - if incoming_message_followup.nil? || !incoming_message_followup.valid_to_reply_to? - return info_request.recipient_email - else - return incoming_message_followup.mail.from_addrs[0].spec - end - end - # Subject to use for followup - def RequestMailer.subject_for_followup(info_request, outgoing_message) - if outgoing_message.what_doing == 'internal_review' - return "Internal review of " + info_request.email_subject_request - else - return info_request.email_subject_followup - end - end - # Whether we have a valid email address for a followup - def RequestMailer.is_followupable?(info_request, incoming_message_followup) - if incoming_message_followup.nil? || !incoming_message_followup.valid_to_reply_to? - return info_request.recipient_email_valid_for_followup? - else - # email has been checked in incoming_message_followup.valid_to_reply_to? above - return true - end - end - - # Used when an FOI officer uploads a response from their web browser - this is # the "fake" email used to store in the same format in the database as if they # had emailed it. diff --git a/app/views/layouts/outgoing_mailer.rhtml b/app/views/layouts/outgoing_mailer.rhtml new file mode 100644 index 000000000..dbb18483f --- /dev/null +++ b/app/views/layouts/outgoing_mailer.rhtml @@ -0,0 +1 @@ +<%= MySociety::Format.wrap_email_body_by_lines(yield) %> diff --git a/app/views/layouts/request_mailer.rhtml b/app/views/layouts/request_mailer.rhtml index c45d5467e..5b8b44402 100644 --- a/app/views/layouts/request_mailer.rhtml +++ b/app/views/layouts/request_mailer.rhtml @@ -1 +1 @@ -<% if @wrap_lines_as_paragraphs %><%= MySociety::Format.wrap_email_body_by_lines(yield) %><% else %><%= MySociety::Format.wrap_email_body_by_paragraphs(yield) %><% end %> +<%= MySociety::Format.wrap_email_body_by_paragraphs(yield) %> diff --git a/app/views/request_mailer/followup.rhtml b/app/views/outgoing_mailer/followup.rhtml index eacf24617..eacf24617 100644 --- a/app/views/request_mailer/followup.rhtml +++ b/app/views/outgoing_mailer/followup.rhtml diff --git a/app/views/request_mailer/initial_request.rhtml b/app/views/outgoing_mailer/initial_request.rhtml index 7c16dfd90..7c16dfd90 100644 --- a/app/views/request_mailer/initial_request.rhtml +++ b/app/views/outgoing_mailer/initial_request.rhtml diff --git a/app/views/request/_after_actions.rhtml b/app/views/request/_after_actions.rhtml index 88cbdc9c7..d51c6bfb3 100644 --- a/app/views/request/_after_actions.rhtml +++ b/app/views/request/_after_actions.rhtml @@ -22,10 +22,10 @@ <ul> <li> <% if @last_response.nil? %> - <%= link_to "Send follow up to " + RequestMailer.name_for_followup(@info_request, @last_response), show_response_no_followup_url(:id => @info_request.id, :incoming_message_id => nil) + "#followup" %> + <%= link_to "Send follow up to " + OutgoingMailer.name_for_followup(@info_request, @last_response), show_response_no_followup_url(:id => @info_request.id, :incoming_message_id => nil) + "#followup" %> <% else %> <% cache(:controller => "request", :action => "show_response", :id => @info_request.id, :incoming_message_id => @last_response.id, :only_path => true, :template => "_after_actions", :section => "reply_to_link") do %> - <%= link_to "Reply to " + RequestMailer.name_for_followup(@info_request, @last_response), show_response_url(:id => @info_request.id, :incoming_message_id => @last_response.id) + "#followup" %> + <%= link_to "Reply to " + OutgoingMailer.name_for_followup(@info_request, @last_response), show_response_url(:id => @info_request.id, :incoming_message_id => @last_response.id) + "#followup" %> <% end %> <% end %> </li> diff --git a/app/views/request/_followup.rhtml b/app/views/request/_followup.rhtml index 4e78ce8f7..2429c63b2 100644 --- a/app/views/request/_followup.rhtml +++ b/app/views/request/_followup.rhtml @@ -2,15 +2,15 @@ <% if @internal_review %> <h1>Request an internal review - from <%=h RequestMailer.name_for_followup(@info_request, nil) %> + from <%=h OutgoingMailer.name_for_followup(@info_request, nil) %> </h1> <% elsif incoming_message.nil? || !incoming_message.valid_to_reply_to? %> <h2>Send a public follow up message - to <%=h RequestMailer.name_for_followup(@info_request, nil) %> + to <%=h OutgoingMailer.name_for_followup(@info_request, nil) %> </h2> <% else %> <h2>Send a public reply to - <%=h RequestMailer.name_for_followup(@info_request, incoming_message) %> + <%=h OutgoingMailer.name_for_followup(@info_request, incoming_message) %> </h2> <% end %> diff --git a/app/views/request/followup_bad.rhtml b/app/views/request/followup_bad.rhtml index ca314afcb..f3e9f07d7 100644 --- a/app/views/request/followup_bad.rhtml +++ b/app/views/request/followup_bad.rhtml @@ -1,7 +1,7 @@ <% if @incoming_message.nil? || !@incoming_message.valid_to_reply_to? %> - <% @title = "Unable to send follow up message to " + RequestMailer.name_for_followup(@info_request, nil) %> + <% @title = "Unable to send follow up message to " + OutgoingMailer.name_for_followup(@info_request, nil) %> <% else %> - <% @title = "Unable to send a reply to " + RequestMailer.name_for_followup(@info_request, @incoming_message) %> + <% @title = "Unable to send a reply to " + OutgoingMailer.name_for_followup(@info_request, @incoming_message) %> <% end %> <h1><%=@title%></h1> diff --git a/app/views/request/followup_preview.rhtml b/app/views/request/followup_preview.rhtml index e8fd1af36..f711575a8 100644 --- a/app/views/request/followup_preview.rhtml +++ b/app/views/request/followup_preview.rhtml @@ -19,8 +19,8 @@ <div class="correspondence" id="outgoing-0"> <p class="preview_subject"> - <strong>To:</strong> <%=h RequestMailer.name_for_followup(@info_request, @incoming_message) %> - <br><strong>Subject:</strong> <%=h RequestMailer.subject_for_followup(@info_request, @outgoing_message) %> + <strong>To:</strong> <%=h OutgoingMailer.name_for_followup(@info_request, @incoming_message) %> + <br><strong>Subject:</strong> <%=h OutgoingMailer.subject_for_followup(@info_request, @outgoing_message) %> </p> <div class="correspondence_text"> diff --git a/spec/models/request_mailer_spec.rb b/spec/models/request_mailer_spec.rb index d097e0ec5..904f41934 100644 --- a/spec/models/request_mailer_spec.rb +++ b/spec/models/request_mailer_spec.rb @@ -151,72 +151,6 @@ And a paragraph afterwards." end -describe RequestMailer, " when working out follow up addresses" do - # This is done with fixtures as the code is a bit tangled with the way it - # calls TMail. XXX untangle it and make these tests spread out and using - # mocks. Put parts of the tests in spec/lib/tmail_extensions.rb - fixtures :info_requests, :incoming_messages, :raw_emails, :public_bodies - - it "should parse them right" do - ir = info_requests(:fancy_dog_request) - im = ir.incoming_messages[0] - - # check the basic entry in the fixture is fine - RequestMailer.name_and_email_for_followup(ir, im).should == "FOI Person <foiperson@localhost>" - RequestMailer.name_for_followup(ir, im).should == "FOI Person" - RequestMailer.email_for_followup(ir, im).should == "foiperson@localhost" - end - - it "should work when there is only an email address" do - ir = info_requests(:fancy_dog_request) - im = ir.incoming_messages[0] - - im.raw_email.data.sub!("\"FOI Person\" <foiperson@localhost>", "foiperson@localhost") - - # check the basic entry in the fixture is fine - RequestMailer.name_and_email_for_followup(ir, im).should == "foiperson@localhost" - RequestMailer.name_for_followup(ir, im).should == "The Geraldine Quango" - RequestMailer.email_for_followup(ir, im).should == "foiperson@localhost" - end - - it "should quote funny characters" do - ir = info_requests(:fancy_dog_request) - im = ir.incoming_messages[0] - - im.raw_email.data.sub!("FOI Person", "FOI [ Person") - - # check the basic entry in the fixture is fine - RequestMailer.name_and_email_for_followup(ir, im).should == "\"FOI [ Person\" <foiperson@localhost>" - RequestMailer.name_for_followup(ir, im).should == "FOI [ Person" - RequestMailer.email_for_followup(ir, im).should == "foiperson@localhost" - end - - it "should quote quotes" do - ir = info_requests(:fancy_dog_request) - im = ir.incoming_messages[0] - - im.raw_email.data.sub!("FOI Person", "FOI \\\" Person") - - # check the basic entry in the fixture is fine - RequestMailer.name_and_email_for_followup(ir, im).should == "\"FOI \\\" Person\" <foiperson@localhost>" - RequestMailer.name_for_followup(ir, im).should == "FOI \" Person" - RequestMailer.email_for_followup(ir, im).should == "foiperson@localhost" - end - - it "should quote @ signs" do - ir = info_requests(:fancy_dog_request) - im = ir.incoming_messages[0] - - im.raw_email.data.sub!("FOI Person", "FOI @ Person") - - # check the basic entry in the fixture is fine - RequestMailer.name_and_email_for_followup(ir, im).should == "\"FOI @ Person\" <foiperson@localhost>" - RequestMailer.name_for_followup(ir, im).should == "FOI @ Person" - RequestMailer.email_for_followup(ir, im).should == "foiperson@localhost" - end - -end - describe RequestMailer, "when sending reminders to requesters to classify a response to their request" do before do |