Merge branch 'issues/alaveteli-pro-140/all-site-emails-for-pro' into …

…issues/alaveteli-pro-138/pro-support-address

app/mailers/application_mailer.rb

@@ -10,6 +10,7 @@ class ApplicationMailer < ActionMailer::Base
   # Include all the functions views get, as emails call similar things.
   helper :application
   include MailerHelper
+  include AlaveteliFeatures::Helpers
 
   # This really should be the default - otherwise you lose any information
   # about the errors, and have to do error checking on return codes.
@@ -19,6 +20,47 @@ def blackhole_email
     AlaveteliConfiguration::blackhole_prefix+"@"+AlaveteliConfiguration::incoming_email_domain
   end
 
+  def mail_user(user, subject)
+    auto_generated_headers('Reply-To' => contact_for_user(user))
+    mail({
+      :from => contact_for_user(user),
+      :to => user.name_and_email,
+      :subject => subject,
+    })
+  end
+
+  def contact_for_user(user)
+    if feature_enabled?(:alaveteli_pro) and user and user.pro?
+      pro_contact_from_name_and_email
+    else
+      contact_from_name_and_email
+    end
+  end
+
+  # Set Return-Path, Reply-To and other miscellaneous headers that give us a
+  # better mail sending experience (from hard won experience).
+  #
+  # Note:
+  # - We set Return-Path, so you should always set Reply-To to be different
+  #   from From, since some email clients seem to erroneously use the envelope
+  #   from when they shouldn't, and this might help. (Have had mysterious
+  #   cases of a reply coming in duplicate from a public body to both From and
+  #   envelope from).
+  # - Return-Path is a special address we control so that SPF checks are done
+  #   on it.
+  # - When sending emails from one user to another, do not set envelope from
+  #   address to the from_user, so they can't get someone's email addresses
+  #   from transitory bounce messages.
+  def auto_generated_headers(opts = {})
+    default_opts = {
+      'Return-Path' => blackhole_email,
+      'Auto-Submitted' => 'auto-generated', # http://tools.ietf.org/html/rfc3834
+      'X-Auto-Response-Suppress' => 'OOF',
+    }
+    default_opts.merge!(opts)
+    headers(default_opts)
+  end
+
   # URL generating functions are needed by all controllers (for redirects),
   # views (for links) and mailers (for use in emails), so include them into
   # all of all.

app/mailers/contact_mailer.rb

@@ -12,36 +12,22 @@ class ContactMailer < ApplicationMailer
   def to_admin_message(name, email, subject, message, logged_in_user, last_request, last_body)
     @message, @logged_in_user, @last_request, @last_body = message, logged_in_user, last_request, last_body
 
-    to = if feature_enabled?(:alaveteli_pro) && @logged_in_user && @logged_in_user.pro?
-      pro_contact_from_name_and_email
-    else
-      contact_from_name_and_email
-    end
-
-    # Return path is an address we control so that SPF checks are done on it.
-    headers('Return-Path' => blackhole_email,
-            'Reply-To' => MailHandler.address_from_name_and_email(name, email))
+    reply_to_address = MailHandler.address_from_name_and_email(name, email)
+    auto_generated_headers('Reply-To' => reply_to_address)
 
     # From is an address we control so that strict DMARC senders don't get refused
     mail(:from => MailHandler.address_from_name_and_email(name, blackhole_email),
-         :to => to,
+         :to => contact_for_user(@logged_in_user),
          :subject => subject)
   end
 
-  # We always set Reply-To when we set Return-Path to be different from From,
-  # since some email clients seem to erroneously use the envelope from when
-  # they shouldn't, and this might help. (Have had mysterious cases of a
-  # reply coming in duplicate from a public body to both From and envelope
-  # from)
-
   # Send message to another user
   def user_message(from_user, recipient_user, from_user_url, subject, message)
     @message, @from_user, @recipient_user, @from_user_url = message, from_user, recipient_user, from_user_url
 
-    # Do not set envelope from address to the from_user, so they can't get
-    # someone's email addresses from transitory bounce messages.
-    headers('Return-Path' => blackhole_email, 'Reply-To' => from_user.name_and_email)
+    auto_generated_headers('Reply-To' => from_user.name_and_email)
 
+    # From is an address we control so that strict DMARC senders don't get refused
     mail(:from => MailHandler.address_from_name_and_email(from_user.name, blackhole_email),
          :to => recipient_user.name_and_email,
          :subject => subject)
@@ -53,13 +39,8 @@ def from_admin_message(recipient_name, recipient_email, subject, message)
     @recipient_name, @recipient_email = recipient_name, recipient_email
 
     recipient_user = User.find_by_email(recipient_email)
-    @from_user = if feature_enabled?(:alaveteli_pro) && recipient_user && recipient_user.pro?
-      pro_contact_from_name_and_email
-    else
-      contact_from_name_and_email
-    end
 
-    mail(:from => @from_user,
+    mail(:from => contact_for_user(recipient_user),
          :to => MailHandler.address_from_name_and_email(@recipient_name, @recipient_email),
          :bcc => AlaveteliConfiguration::contact_email,
          :subject => subject)

app/mailers/info_request_batch_mailer.rb

@@ -9,18 +9,18 @@ class InfoRequestBatchMailer < ApplicationMailer
 
   def batch_sent(info_request_batch, unrequestable, user)
     @info_request_batch, @unrequestable = info_request_batch, unrequestable
-    headers('Return-Path' => blackhole_email, 'Reply-To' => contact_from_name_and_email)
 
     # Make a link going to the info request batch page, which logs the user in.
     post_redirect = PostRedirect.new(
       :uri => info_request_batch_url(@info_request_batch),
-    :user_id => info_request_batch.user_id)
+      :user_id => info_request_batch.user_id)
     post_redirect.save!
     @url = confirm_url(:email_token => post_redirect.email_token)
 
-    mail(:from => contact_from_name_and_email,
-         :to => user.name_and_email,
-         :subject => _("Your batch request \"{{title}}\" has been sent",
-                       :title => info_request_batch.title.html_safe))
+    mail_user(
+      user,
+      _("Your batch request \"{{title}}\" has been sent",
+        :title => info_request_batch.title.html_safe)
+    )
   end
 end

app/mailers/request_mailer.rb

@@ -57,27 +57,20 @@ def stopped_responses(info_request, email, raw_email_data)
   # An FOI response is outside the scope of the system, and needs admin attention
   def requires_admin(info_request, set_by = nil, message = "")
     user = set_by || info_request.user
-    to = if feature_enabled?(:alaveteli_pro) and user and user.pro?
-     pro_contact_from_name_and_email
-    else
-      contact_from_name_and_email
-    end
     @reported_by = user
     @url = request_url(info_request)
     @admin_url = admin_request_url(info_request)
     @info_request = info_request
     @message = message
 
-    # Return path is an address we control so that SPF checks are done on it.
-    headers('Return-Path' => blackhole_email,
-            'Reply-To' => user.name_and_email)
+    auto_generated_headers(:reply_to => user.name_and_email)
 
     # From is an address we control so that strict DMARC senders don't get refused
     mail(:from => MailHandler.address_from_name_and_email(
                     user.name,
                     blackhole_email
                   ),
-         :to => to,
+         :to => contact_for_user(user),
          :subject => _("FOI response requires admin ({{reason}}) - " \
                         "{{request_title}}",
                        :reason => info_request.described_state,
@@ -91,17 +84,11 @@ def new_response(info_request, incoming_message)
     @url = incoming_message_url(incoming_message, :cachebust => true)
     @incoming_message, @info_request = incoming_message, info_request
 
-    headers('Return-Path' => blackhole_email,
-            'Auto-Submitted' => 'auto-generated', # http://tools.ietf.org/html/rfc3834
-            'X-Auto-Response-Suppress' => 'OOF')
-
-    mail(:from => contact_from_name_and_email,
-         :to => info_request.user.name_and_email,
-         :subject => _("New response to your FOI request - {{request_title}}",
-                       :request_title => info_request.title.html_safe),
-         :charset => "UTF-8",
-         # not much we can do if the user's email is broken
-         :reply_to => contact_from_name_and_email)
+    mail_user(
+      info_request.user,
+      _("New response to your FOI request - {{request_title}}",
+        :request_title => info_request.title.html_safe)
+    )
   end
 
   # Tell the requester that the public body is late in replying
@@ -117,7 +104,6 @@ def overdue_alert(info_request, user)
     @url = confirm_url(:email_token => post_redirect.email_token)
     @info_request = info_request
 
-    auto_generated_headers
     mail_user(
       user,
       _("Delayed response to your FOI request - {{request_title}}",
@@ -136,7 +122,6 @@ def very_overdue_alert(info_request, user)
     @url = confirm_url(:email_token => post_redirect.email_token)
     @info_request = info_request
 
-    auto_generated_headers
     mail_user(
       user,
       _("You're long overdue a response to your FOI request - {{request_title}}",
@@ -157,7 +142,6 @@ def new_response_reminder_alert(info_request, incoming_message)
     @incoming_message = incoming_message
     @info_request = info_request
 
-    auto_generated_headers
     mail_user(info_request.user, _("Was the response you got to your FOI " \
                                       "request any good?"))
   end
@@ -167,7 +151,6 @@ def old_unclassified_updated(info_request)
     @url = request_url(info_request)
     @info_request = info_request
 
-    auto_generated_headers
     mail_user(info_request.user, _("Someone has updated the status of " \
                                       "your request"))
   end
@@ -185,7 +168,6 @@ def not_clarified_alert(info_request, incoming_message)
     @incoming_message = incoming_message
     @info_request = info_request
 
-    auto_generated_headers
     mail_user(
       info_request.user,
       _("Clarify your FOI request - {{request_title}}",
@@ -198,7 +180,6 @@ def comment_on_alert(info_request, comment)
     @comment, @info_request = comment, info_request
     @url = comment_url(comment)
 
-    auto_generated_headers
     mail_user(
       info_request.user,
       _("Somebody added a note to your FOI request - {{request_title}}",
@@ -212,7 +193,6 @@ def comment_on_alert_plural(info_request, count, earliest_unalerted_comment)
     @count, @info_request = count, info_request
     @url = comment_url(earliest_unalerted_comment)
 
-    auto_generated_headers
     mail_user(
       info_request.user,
       _("Some notes have been added to your FOI request - {{request_title}}",
@@ -521,23 +501,4 @@ def self.alert_comment_on_request
     end
   end
 
-  private
-
-  def auto_generated_headers
-    headers({
-      'Return-Path' => blackhole_email,
-      'Reply-To' => contact_from_name_and_email, # not much we can do if the user's email is broken
-      'Auto-Submitted' => 'auto-generated', # http://tools.ietf.org/html/rfc3834
-      'X-Auto-Response-Suppress' => 'OOF',
-    })
-  end
-
-  def mail_user(user, subject)
-    mail({
-      :from => contact_from_name_and_email,
-      :to => user.name_and_email,
-      :subject => subject,
-    })
-  end
-
 end

app/mailers/track_mailer.rb

@@ -6,6 +6,12 @@
 # Email: hello@mysociety.org; WWW: http://www.mysociety.org/
 
 class TrackMailer < ApplicationMailer
+  # Note that this is different from all the other mailers, as tracks are
+  # sent from a different email address and have different bounce handling.
+  def contact_from_name_and_email
+    "#{AlaveteliConfiguration::track_sender_name} <#{AlaveteliConfiguration::track_sender_email}>"
+  end
+
   def event_digest(user, email_about_things)
     @user, @email_about_things = user, email_about_things
 
@@ -27,10 +33,6 @@ def event_digest(user, email_about_things)
                        :site_name => site_name.html_safe))
   end
 
-  def contact_from_name_and_email
-    "#{AlaveteliConfiguration::track_sender_name} <#{AlaveteliConfiguration::track_sender_email}>"
-  end
-
   # Send email alerts for tracked things.  Never more than one email
   # a day, nor about events which are more than a week old, nor
   # events about which emails have been sent within the last two

app/mailers/user_mailer.rb

@@ -8,36 +8,30 @@
 class UserMailer < ApplicationMailer
   def confirm_login(user, reasons, url)
     @reasons, @name, @url = reasons, user.name, url
-    headers('Return-Path' => blackhole_email, 'Reply-To' => contact_from_name_and_email) # we don't care about bounces when people are fiddling with their account
 
-    mail(:from => contact_from_name_and_email,
-         :to => user.name_and_email,
-         :subject => reasons[:email_subject])
+    mail_user(user, reasons[:email_subject])
   end
 
   def already_registered(user, reasons, url)
     @reasons, @name, @url = reasons, user.name, url
-    headers('Return-Path' => blackhole_email, 'Reply-To' => contact_from_name_and_email) # we don't care about bounces when people are fiddling with their account
 
-    mail(:from => contact_from_name_and_email,
-         :to => user.name_and_email,
-         :subject => reasons[:email_subject])
+    mail_user(user, reasons[:email_subject])
   end
 
   def changeemail_confirm(user, new_email, url)
     @name, @url, @old_email, @new_email = user.name, url, user.email, new_email
-    headers('Return-Path' => blackhole_email, 'Reply-To' => contact_from_name_and_email) # we don't care about bounces when people are fiddling with their account
+    auto_generated_headers('Reply-To' => contact_for_user(user))
 
-    mail(:from => contact_from_name_and_email,
+    mail(:from => contact_for_user(user),
          :to => new_email,
          :subject => _("Confirm your new email address on {{site_name}}", :site_name => site_name))
   end
 
   def changeemail_already_used(old_email, new_email)
     @old_email, @new_email = old_email, new_email
-    headers('Return-Path' => blackhole_email, 'Reply-To' => contact_from_name_and_email) # we don't care about bounces when people are fiddling with their account
+    auto_generated_headers('Reply-To' => contact_for_user(user))
 
-    mail(:from => contact_from_name_and_email,
+    mail(:from => contact_for_user(user),
          :to => new_email,
          :subject => _("Unable to change email address on {{site_name}}", :site_name=>site_name))
   end