[Pro] Site email handling for pro users #3706

stevenday · 2017-01-31T13:09:31Z

This combines mysociety/alaveteli-professional#75 mysociety/alaveteli-professional#138 and
mysociety/alaveteli-professional#140 into a single branch, since they're all related
and need each other to really work correctly.

This PR:

Refactors bounce handling so that non-bounce replies to emails are sent to
two different addresses, one for replies from pro users and one for
everything else
Adds a new contact address for pro user support
Refactors all of the mailers to:
- Share Return-Path, Reply-To and other related header setting wherever
  possible
- Be aware of the new pro contact address and use it when emailing pros

Closes mysociety/alaveteli-professional#75
Closes mysociety/alaveteli-professional#138
Closes mysociety/alaveteli-professional#140

stevenday · 2017-01-31T13:42:44Z

@crowbot - I think the only spec failures here are the ones occurring everywhere in pro regarding month arithmetic, so this should be ready for a review when you have a moment.

crowbot

I think this PR is making an unintended change which adds the headers that indicate that the message was autogenerated and that out of office replies should be suppressed to lots of messages that didn't previously have them. A couple of other less significant style/naming comments inline.

crowbot · 2017-01-31T14:19:05Z

app/mailers/contact_mailer.rb

-    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)


This is going to set the 'Auto-Submitted' => 'auto-generated' and 'X-Auto-Response-Suppress' => 'OOF' headers here, I don't think the 'Auto-Submitted' => 'auto-generated' one is appropriate as it shouldn't be used on manually generated messages, only on messages that are automatically created.

crowbot · 2017-01-31T14:20:51Z

app/mailers/contact_mailer.rb

-    # 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)


Again, I don't think the autogenerated headers for 'Auto-Submitted' and 'X-Auto-Response-Suppress' are right for this email

crowbot · 2017-01-31T14:29:06Z

app/mailers/info_request_batch_mailer.rb

-         :to => user.name_and_email,
-         :subject => _("Your batch request \"{{title}}\" has been sent",
-                       :title => info_request_batch.title.html_safe))
+    mail_user(


This is going to use the headers that we don't want.

crowbot · 2017-01-31T14:30:10Z

app/mailers/request_mailer.rb

-    # 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)


Same issue, some of these headers aren't appropriate.

crowbot · 2017-01-31T14:31:48Z

app/mailers/request_mailer.rb

-         :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",


Looks like we've lost the charset here?

crowbot · 2017-01-31T14:38:04Z

app/mailers/user_mailer.rb

-    mail(:from => contact_from_name_and_email,
-         :to => user.name_and_email,
-         :subject => reasons[:email_subject])
+    mail_user(user, reasons[:email_subject])


Again, we don't want the `auto-submitted' and 'auto-reply-suppress' headers here.

crowbot · 2017-01-31T14:38:27Z

app/mailers/user_mailer.rb

  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


Same issue here

crowbot · 2017-01-31T14:39:25Z

app/mailers/user_mailer.rb

  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))


We don't want some of these headers, as in previous cases.

crowbot · 2017-01-31T14:40:09Z

app/mailers/user_mailer.rb

         :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
+    user = User.find_by_email(@old_email)
+    auto_generated_headers('Reply-To' => contact_for_user(user))


Same again.

crowbot · 2017-01-31T14:46:38Z

lib/reply_handler.rb

@@ -0,0 +1,125 @@
+# -*- encoding : utf-8 -*-


Maybe this ought to be a submodule of `mail_handler?

stevenday · 2017-02-02T09:48:07Z

@crowbot - sorry, I hadn't really thought through the impact of setting those headers everywhere.

I've made some changes to application_mailer.rb to separate setting reply_to headers and auto_generated headers, and then updated all the mailers so that they only call the appropriate things. I think there are now no unexpected changes to any of the mail headers.

I've also put ReplyHandler into MailHandler::ReplyHandler as suggested, though I'm not sure if there's a convention for where the file should live? (It seems a bit odd being alongside mail_handler.rb)

crowbot

One comment inline, but other than that, looks great!

crowbot · 2017-02-06T12:56:14Z

app/mailers/contact_mailer.rb

-    mail(:from => contact_from_name_and_email,
+
+    recipient_user = User.find_by_email(recipient_email)
+    set_reply_to_headers(recipient_user)


We're adding new reply-to and return-path headers here. I don't think it should make any substantial difference, but maybe better to leave it as it is, as it's not the focus of this PR?

…iler

So that we can only set the appropriate things. To be re-ordered before changes to individual mailers

To make it more obvious that this file uses a custom contact address

coveralls · 2017-02-06T15:32:13Z

Coverage decreased (-0.5%) to 92.934% when pulling 8bb6946 on alaveteli-pro-site-email-handling into 71e0672 on alaveteli-pro-develop.

stevenday force-pushed the alaveteli-pro-site-email-handling branch from 925dcf8 to fb7e6f5 Compare January 31, 2017 13:17

This comment has been minimized.

Sign in to view

stevenday requested a review from crowbot January 31, 2017 13:41

stevenday added the awaiting-review label Jan 31, 2017

This comment has been minimized.

Sign in to view

crowbot requested changes Jan 31, 2017

View reviewed changes

stevenday added the 4 - now label Jan 31, 2017

stevenday self-assigned this Jan 31, 2017

stevenday removed the awaiting-review label Jan 31, 2017

This comment has been minimized.

Sign in to view

stevenday added awaiting-review and removed 4 - now labels Feb 3, 2017

stevenday removed their assignment Feb 3, 2017

crowbot requested changes Feb 6, 2017

View reviewed changes

crowbot assigned stevenday Feb 6, 2017

crowbot added 3 - current sprint and removed awaiting-review labels Feb 6, 2017

Steven Day added 10 commits February 6, 2017 15:03

Send admin contact emails from pros to pro address

a5b88df

Set from: address to pro contact when admins email pros

c539073

Set @contact_email appropriately on contact form for pros

b09ff28

Move mail_user and auto_generated_headers helpers into application_ma…

f99ec30

…iler

Make new mailer helpers use correct from/reply-to for pros

ed051c5

Make auto_generated_headers accept a dict of headers to use

b2e2a25

Separate auto-generated and reply-to mail header setting

b1def0c

So that we can only set the appropriate things. To be re-ordered before changes to individual mailers

Use headers helper in contact_mailer where possible

cb0db99

Use mail_user helper in info_request_batch_mailer

9fd1996

Use new application_mailer helper methods in request_mailer

cce7113

Steven Day added 7 commits February 6, 2017 15:03

Move custom contact email to top of track_mailer

c30338b

To make it more obvious that this file uses a custom contact address

Use new mailer helper methods in user_mailer

63fbfde

Add a config setting for pro email to be forwarded to

b657053

Add fixture emails for explicitly testing non-bounce replies

a496759

Test that reply forwarding actually calls sendmail

c10e60d

Forward pro and normal non-bounce replies to different addresses

e17c830

Refactor reply-handling into a module in /lib

8bb6946

This comment has been minimized.

Sign in to view

stevenday force-pushed the alaveteli-pro-site-email-handling branch from 75bef5e to 8bb6946 Compare February 6, 2017 15:13

stevenday merged commit 8bb6946 into alaveteli-pro-develop Feb 6, 2017

stevenday removed the 3 - current sprint label Feb 6, 2017

stevenday deleted the alaveteli-pro-site-email-handling branch February 6, 2017 15:37

This was referenced Jul 6, 2021

Mail sent To team@ with Cc pro_team@ does not get delivered to team@ #5649

Closed

[Pro] Forward pro and non-pro non-bounce replies to different addresses #3699

Closed

[Pro] Redirect contact emails to/from pro users #3684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Pro] Site email handling for pro users #3706

[Pro] Site email handling for pro users #3706

stevenday commented Jan 31, 2017 •

edited by garethrees

Loading

This comment has been minimized.

stevenday commented Jan 31, 2017

This comment has been minimized.

crowbot left a comment

crowbot Jan 31, 2017

crowbot Jan 31, 2017

crowbot Jan 31, 2017

crowbot Jan 31, 2017

crowbot Jan 31, 2017

crowbot Jan 31, 2017

crowbot Jan 31, 2017

crowbot Jan 31, 2017

crowbot Jan 31, 2017

crowbot Jan 31, 2017

This comment has been minimized.

stevenday commented Feb 2, 2017

This comment has been minimized.

crowbot left a comment

crowbot Feb 6, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

coveralls commented Feb 6, 2017 •

edited

Loading

[Pro] Site email handling for pro users #3706

[Pro] Site email handling for pro users #3706

Conversation

stevenday commented Jan 31, 2017 • edited by garethrees Loading

This comment has been minimized.

stevenday commented Jan 31, 2017

This comment has been minimized.

crowbot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

stevenday commented Feb 2, 2017

This comment has been minimized.

crowbot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

coveralls commented Feb 6, 2017 • edited Loading

stevenday commented Jan 31, 2017 •

edited by garethrees

Loading

coveralls commented Feb 6, 2017 •

edited

Loading