Tests for use of different addresses in replies.

Fix bug in quoting of those replies.
Make it remove @ signs from name part of them.

4 files changed, 78 insertions, 33 deletions

diff --git a/app/models/incoming_message.rb b/app/models/incoming_message.rb
index 11e31535b..002914bca 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.196 2009-04-07 16:01:55 francis Exp $
+# $Id: incoming_message.rb,v 1.197 2009-04-08 05:29:35 francis Exp $
 
 # TODO
 # Move some of the (e.g. quoting) functions here into rblib, as they feel
@@ -1052,14 +1052,12 @@ class IncomingMessage < ActiveRecord::Base
         return get_body_for_quoting + "\n\n" + get_attachment_text
     end
 
-    # Returns the name of the person the incoming message is from, or nil if there isn't one
-    # or if there is only an email address.
+    # Returns the name of the person the incoming message is from, or nil if
+    # there isn't one or if there is only an email address. XXX can probably
+    # remove this and from_name_if_present (which is a monkey patch) by just
+    # calling .from_addrs[0].name instead? (as RequestMailer.name_for_followup etc. do)
     def safe_mail_from
-        if self.mail.from && (!self.mail.friendly_from.include?('@'))
-            return self.mail.friendly_from
-        else
-            return nil
-        end
+        self.mail.from_name_if_present
     end
 
     def mail_from_domain
diff --git a/app/models/request_mailer.rb b/app/models/request_mailer.rb
index a3a1e8b74..2e69be363 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.70 2009-04-07 16:01:36 francis Exp $
+# $Id: request_mailer.rb,v 1.71 2009-04-08 05:29:35 francis Exp $
 
 class RequestMailer < ApplicationMailer
     
@@ -37,7 +37,7 @@ class RequestMailer < ApplicationMailer
         if incoming_message_followup.nil? || !incoming_message_followup.valid_to_reply_to?
             return info_request.recipient_name_and_email
         else
-            return incoming_message_followup.mail.from_addrs[0].quoted_full
+            return incoming_message_followup.mail.from_addrs[0].full_quoted_address
         end
     end
     # Used in the preview of followup
@@ -45,7 +45,7 @@ class RequestMailer < ApplicationMailer
         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
+            return incoming_message_followup.mail.from_addrs[0].name || info_request.public_body.name
         end
     end
     # Used when making list of followup places to remove duplicates
@@ -92,7 +92,7 @@ class RequestMailer < ApplicationMailer
     def stopped_responses(info_request, email)
         @from = contact_from_name_and_email
         headers 'Return-Path' => blackhole_email, 'Reply-To' => @from # we don't care about bounces, likely from spammers
-        @recipients = email.from_addrs[0].quoted_full
+        @recipients = email.from_addrs[0].full_quoted_address
         @subject = "Your response to an FOI request was not delivered"
         attachment :content_type => 'message/rfc822', :body => email.body
         @body = { 
diff --git a/lib/tmail_extensions.rb b/lib/tmail_extensions.rb
index 9aabdfeed..a02460f7f 100644
--- a/lib/tmail_extensions.rb
+++ b/lib/tmail_extensions.rb
@@ -4,7 +4,7 @@
 # Copyright (c) 2009 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: tmail_extensions.rb,v 1.1 2009-04-07 16:23:28 francis Exp $
+# $Id: tmail_extensions.rb,v 1.2 2009-04-08 05:29:36 francis Exp $
 
 # Monkeypatch!
 
@@ -13,12 +13,22 @@ module TMail
     class Mail
         # Monkeypatch! (check to see if this becomes a standard function in
         # TMail::Mail, then use that, whatever it is called)
-        def self.get_part_file_name(part)
+        def Mail.get_part_file_name(part)
             file_name = (part['content-location'] &&
                           part['content-location'].body) ||
                         part.sub_header("content-type", "name") ||
                         part.sub_header("content-disposition", "filename")
         end
+
+        # Monkeypatch! Return the name part of from address, or nil if there isn't one
+        def from_name_if_present
+            if self.from && self.from_addrs[0].name
+                return self.from_addrs[0].name
+            else
+                return nil
+            end
+        end 
+
     end
 
     class Address
@@ -28,16 +38,18 @@ module TMail
             if text.match(/[^A-Za-z0-9!#\$%&'*+\-\/=?^_`{|}~ ]/)
                 # Contains characters which aren't valid in atoms, so make a
                 # quoted-pair instead.
-                text.gsub!(/(["\\])/, "\\\\\\1")
+                text = text.gsub(/(["\\])/, "\\\\\\1")
                 text = '"' + text + '"'
             end
             return text
         end
 
         # Monkeypatch!
-        def quoted_full
+        def full_quoted_address
             if self.name
-                Address.encode_quoted_string(self.name) + " <" + self.spec + ">"
+                # sanitise name - some mail servers can't cope with @ in the name part
+                name = self.name.gsub(/@/, " ")
+                Address.encode_quoted_string(name) + " <" + self.spec + ">"
             else
                 self.spec
             end
diff --git a/spec/models/request_mailer_spec.rb b/spec/models/request_mailer_spec.rb
index ed2bb84e9..76eebd05f 100644
--- a/spec/models/request_mailer_spec.rb
+++ b/spec/models/request_mailer_spec.rb
@@ -68,10 +68,9 @@ end
 
 
 describe RequestMailer, " when working out follow up addresses" do
-    # doing this with fixtures as the code is a bit tangled with the way it
-    # calls TMail, and the TMail monkey patches in conf/environment.rb
-    # XXX untangle it and make these tests spread out and using mocks. Might
-    # mean properly patching TMail.
+    # 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
@@ -84,18 +83,54 @@ describe RequestMailer, " when working out follow up addresses" do
         RequestMailer.email_for_followup(ir, im).should == "foiperson@localhost"
     end
 
-#    it "should escape 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 work when there is only an email address" do
+        ir = info_requests(:fancy_dog_request) 
+        im = ir.incoming_messages[0]
 
-end
+        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 remove @ signs from name part in reply address as some mail servers hate it" 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