Don't allow reply to invalid addresses, or to postmaster@ addresses.

Centralise some of the display of names for replies.
Note in comment other parts that need centralising.

5 files changed, 35 insertions, 21 deletions

diff --git a/app/models/incoming_message.rb b/app/models/incoming_message.rb
index a0f26aa40..0b01f18d1 100644
--- a/app/models/incoming_message.rb
+++ b/app/models/incoming_message.rb
@@ -19,7 +19,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: incoming_message.rb,v 1.151 2008-09-24 17:21:00 francis Exp $
+# $Id: incoming_message.rb,v 1.152 2008-10-17 07:02:01 francis Exp $
 
 # TODO
 # Move some of the (e.g. quoting) functions here into rblib, as they feel
@@ -884,6 +884,26 @@ class IncomingMessage < ActiveRecord::Base
     def IncomingMessage.get_all_file_extentions
         return $file_extension_to_mime_type.keys.join(" ")
     end
+
+    # Return false if for some reason this is a message that we should let them reply to
+    def valid_to_reply_to?
+        # check validity of email
+        email = self.mail.from_addrs[0].spec
+        if !MySociety::Validate.is_valid_email(email)
+            return false
+        end
+
+        # reject postmaster - authorities seem to nearly always not respond to
+        # email to postmaster, and it tells to only happen after delivery failure.
+        prefix = email
+        prefix =~ /^(.*)@/
+        prefix = $1
+        if !prefix.nil? && prefix == 'postmaster'
+            return false
+        end
+
+        return true
+    end
 end
 
 
diff --git a/app/models/outgoing_message.rb b/app/models/outgoing_message.rb
index 84fb511d7..437fe07c9 100644
--- a/app/models/outgoing_message.rb
+++ b/app/models/outgoing_message.rb
@@ -21,7 +21,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.66 2008-09-22 22:16:37 francis Exp $
+# $Id: outgoing_message.rb,v 1.67 2008-10-17 07:02:01 francis Exp $
 
 class OutgoingMessage < ActiveRecord::Base
     belongs_to :info_request
@@ -39,8 +39,8 @@ class OutgoingMessage < ActiveRecord::Base
     # How the default letter starts and ends
     def get_salutation
         ret = "Dear "
-        if self.message_type == 'followup' && !self.incoming_message_followup.nil? && !self.incoming_message_followup.safe_mail_from.nil?
-            ret = ret + self.incoming_message_followup.safe_mail_from
+        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)
         else
             ret = ret + "Sir or Madam"
         end
diff --git a/app/models/request_mailer.rb b/app/models/request_mailer.rb
index 374355810..316567e60 100644
--- a/app/models/request_mailer.rb
+++ b/app/models/request_mailer.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_mailer.rb,v 1.61 2008-10-14 12:48:49 francis Exp $
+# $Id: request_mailer.rb,v 1.62 2008-10-17 07:02:01 francis Exp $
 
 class RequestMailer < ApplicationMailer
     
@@ -26,9 +26,13 @@ class RequestMailer < ApplicationMailer
             :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
     def RequestMailer.name_and_email_for_followup(info_request, incoming_message_followup)
-        if incoming_message_followup.nil?
+        if incoming_message_followup.nil? || !incoming_message_followup.valid_to_reply_to?
             return info_request.recipient_name_and_email
         else
             IncomingMessage # get Monkeypatch!
@@ -37,7 +41,7 @@ class RequestMailer < ApplicationMailer
     end
     # Used in the preview of followup
     def RequestMailer.name_for_followup(info_request, incoming_message_followup)
-        if incoming_message_followup.nil?
+        if incoming_message_followup.nil? || !incoming_message_followup.valid_to_reply_to?
             return info_request.public_body.name
         else
             return incoming_message_followup.safe_mail_from || info_request.public_body.name
@@ -45,7 +49,7 @@ class RequestMailer < ApplicationMailer
     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?
+        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
diff --git a/app/views/request/_followup.rhtml b/app/views/request/_followup.rhtml
index 19150e93f..b363e94de 100644
--- a/app/views/request/_followup.rhtml
+++ b/app/views/request/_followup.rhtml
@@ -1,14 +1,11 @@
 <div id="followup">
 
-    <% if incoming_message.nil? %>
+    <% if incoming_message.nil? || !incoming_message.valid_to_reply_to? %>
         <h2>Send a follow up message 
-        to '<%=h @info_request.public_body.name %>'
+        to '<%=h RequestMailer.name_for_followup(@info_request, nil) %>'
         </h2>
     <% else %>
-        <h2>Send a reply
-            <% if !incoming_message.safe_mail_from.nil? %>
-                to <%= incoming_message.safe_mail_from %>
-            <% end %>
+        <h2>Send a reply to <%=h RequestMailer.name_for_followup(@info_request, incoming_message) %>
         </h2>
     <% end %>
 
diff --git a/todo.txt b/todo.txt
index 00865136a..fb1c1b0c9 100644
--- a/todo.txt
+++ b/todo.txt
@@ -1,8 +1,5 @@
 Test data for Tony
 
-XXX remove details from set_described_state 
-requires_admin_deatils
-
 Next
 ====
 
@@ -33,10 +30,6 @@ Remember - internal reviews can be *aborted* if they just send the response :)
 
 Request withdrawn by user status/marker?
 
-Perhaps encourage user to annotate what they learnt after getting information?
-When they say "successful", encourage them to make an annotation?
-"Some of the information" option should give you choice of complaining if you like.
-
 Clear out all the need admin attention requests
 
 Merge workflow into one stream - find information, if you can't find it then request it.