Store transient bounces as well as permanent bounces

Fixes CNVS-22956

Test plan:
 - Amazon's documentation is ambiguous about whether
   ooto@simulator.amazonses.com will actually generate a
   transient failure, so this may not actually work.
 - Set up outgoing email through the Amazon SES test account
 - Set up bounce_notifications.yml with the SQS test creds
 - Set a user's email address to ooto@simulator.amazonses.com
 - Cause a message to be sent to that user
 - Wait for the bounce notification processor to run (~5 minutes)
 - At a Rails console, check the CommunicationChannel's
   last_transient_bounce_at and verify that it roughly corresponds
   to when the initial message was sent
 - Check the CommunicationChannel's last_transient_bounce_details and
   ensure that they have been filled in

Change-Id: Ic3a4110319673d6ec6f0789885c760faec4775b3
Reviewed-on: https://gerrit.instructure.com/62562
Tested-by: Jenkins
Reviewed-by: Jacob Fugal <jacob@instructure.com>
Reviewed-by: Steven Burnett <sburnett@instructure.com>
QA-Review: Heath Hales <hhales@instructure.com>
Product-Review: Alex Boyd <aboyd@instructure.com>

Author: javawizard

app/models/bounce_notification_processor.rb

@@ -65,13 +65,12 @@ def parse_message(message)
   end
 
   def process_bounce_notification(bounce_notification)
-    return unless is_permanent_bounce?(bounce_notification)
-
     bouncy_addresses(bounce_notification).each do |address|
       CommunicationChannel.bounce_for_path(
         path: address,
         timestamp: bounce_timestamp(bounce_notification),
         details: bounce_notification,
+        permanent_bounce: is_permanent_bounce?(bounce_notification),
         suppression_bounce: is_suppression_bounce?(bounce_notification)
       )
     end

app/models/communication_channel.rb

@@ -24,6 +24,7 @@ class CommunicationChannel < ActiveRecord::Base
   attr_accessible :user, :path, :path_type, :build_pseudonym_on_confirm, :pseudonym
 
   serialize :last_bounce_details
+  serialize :last_transient_bounce_details
 
   belongs_to :pseudonym
   has_many :pseudonyms
@@ -441,16 +442,21 @@ def check_if_bouncing_changed
   end
   private :check_if_bouncing_changed
 
-  def self.bounce_for_path(path:, timestamp:, details:, suppression_bounce:)
+  def self.bounce_for_path(path:, timestamp:, details:, permanent_bounce:, suppression_bounce:)
     Shard.with_each_shard(CommunicationChannel.associated_shards(path)) do
       CommunicationChannel.unretired.email.by_path(path).each do |channel|
-        channel.bounce_count = channel.bounce_count + 1
+        channel.bounce_count = channel.bounce_count + 1 if permanent_bounce
+
         if suppression_bounce
           channel.last_suppression_bounce_at = timestamp
-        else
+        elsif permanent_bounce
           channel.last_bounce_at = timestamp
           channel.last_bounce_details = details
+        else
+          channel.last_transient_bounce_at = timestamp
+          channel.last_transient_bounce_details = details
         end
+
         channel.save!
       end
     end

db/migrate/20150903204436_add_transient_bounce_columns_to_communication_channels.rb

@@ -0,0 +1,8 @@
+class AddTransientBounceColumnsToCommunicationChannels < ActiveRecord::Migration
+  tag :predeploy
+
+  def change
+    add_column :communication_channels, :last_transient_bounce_at, :datetime
+    add_column :communication_channels, :last_transient_bounce_details, :text, length: 32768
+  end
+end

spec/fixtures/bounces.json

@@ -64,16 +64,5 @@
         "Signature" : "example==",
         "SigningCertURL" : "https://example.com/example.pem",
         "UnsubscribeURL" : "https://example.com/unsubscribe"
-    },
-    {
-        "Type" : "Notification",
-        "MessageId" : "example-id",
-        "TopicArn" : "example-arn",
-        "Message" : "{\"notificationType\":\"Bounce\",\"bounce\":{\"bounceSubType\":\"General\",\"bounceType\":\"Transient\",\"reportingMTA\":\"dsn; a14-1.smtp-out.amazonses.com\",\"bouncedRecipients\":[{\"emailAddress\":\"soft@example.edu\",\"status\":\"4.4.7\",\"action\":\"failed\",\"diagnosticCode\":\"smtp; 554 4.4.7 Message expired: unable to deliver in 840 minutes.<421 4.4.0 Unable to lookup DNS for ms-wasatch-edu.mail.protection.outlook.com>\"}],\"timestamp\":\"2014-08-22T12:18:58.226Z\",\"feedbackId\":\"example-id\"},\"mail\":{\"timestamp\":\"2014-08-21T20:51:56.000Z\",\"source\":\"notifications@instructure.com\",\"messageId\":\"example-id\",\"destination\":[\"soft@example.edu\"]}}",
-        "Timestamp" : "2014-08-22T12:18:58.393Z",
-        "SignatureVersion" : "1",
-        "Signature" : "example==",
-        "SigningCertURL" : "https://example.com/example.pem",
-        "UnsubscribeURL" : "https://example.com/unsubscribe"
     }
 ]

spec/models/bounce_notification_processor_spec.rb

@@ -48,19 +48,24 @@ def mock_message(json)
       queue.expects(:poll).multiple_yields(*@all_bounce_messages_json.map {|m| mock_message(m)})
       bnp.stubs(:bounce_queue).returns(queue)
 
-      CommunicationChannel.expects(:bounce_for_path).with do |path:, timestamp:, details:, suppression_bounce:|
+      CommunicationChannel.expects(:bounce_for_path).with do |path:, timestamp:, details:, permanent_bounce:, suppression_bounce:|
         path == 'hard@example.edu' &&
         timestamp == '2014-08-22T12:25:46.786Z' &&
+        permanent_bounce == true &&
         suppression_bounce == false
       end.times(4)
-      CommunicationChannel.expects(:bounce_for_path).with do |path:, timestamp:, details:, suppression_bounce:|
+      CommunicationChannel.expects(:bounce_for_path).with do |path:, timestamp:, details:, permanent_bounce:, suppression_bounce:|
         path == 'suppressed@example.edu' &&
         timestamp == '2014-08-22T12:18:58.044Z' &&
+        permanent_bounce == true &&
         suppression_bounce == true
       end.times(1)
-      CommunicationChannel.expects(:bounce_for_path).with do |path:, timestamp:, details:, suppression_bounce:|
-        path == 'soft@example.edu'
-      end.times(0)
+      CommunicationChannel.expects(:bounce_for_path).with do |path:, timestamp:, details:, permanent_bounce:, suppression_bounce:|
+        path == 'soft@example.edu' &&
+        timestamp == '2014-08-22T13:24:31.000Z' &&
+        permanent_bounce == false &&
+        suppression_bounce == false
+      end.times(1)
 
       bnp.process
     end

spec/models/communication_channel_spec.rb

@@ -64,7 +64,7 @@
     expect(@cc.state).to eql(:retired)
     @cc.re_activate
     expect(@cc.state).to eql(:active)
-    
+
     communication_channel_model(:path => "another_path@example.com")
     expect(@cc.state).to eql(:unconfirmed)
     @cc.retire
@@ -89,27 +89,27 @@
     communication_channel_model
     expect(@cc.confirmation_code).to eql('abc123')
   end
-  
+
   it "should be able to reset a confirmation code" do
     communication_channel_model
     old_cc = @cc.confirmation_code
     @cc.set_confirmation_code(true)
     expect(@cc.confirmation_code).not_to eql(old_cc)
   end
-  
+
   it "should use a 15-digit confirmation code for default or email path_type settings" do
     communication_channel_model
     expect(@cc.path_type).to eql('email')
     expect(@cc.confirmation_code.size).to eql(25)
   end
-  
+
   it "should use a 4-digit confirmation_code for settings other than email" do
     communication_channel_model
     @cc.path_type = 'sms'
     @cc.set_confirmation_code(true)
     expect(@cc.confirmation_code.size).to eql(4)
   end
-  
+
   it "should default the path type to email" do
     communication_channel_model
     expect(@cc.path_type).to eql('email')
@@ -134,11 +134,11 @@
     @cc.path_type = 'not valid'; @cc.save
     expect(@cc.path_type).to eql('email')
   end
-  
+
   it "should act as list" do
     expect(CommunicationChannel).to be_respond_to(:acts_as_list)
   end
-  
+
   it "should scope the list to the user" do
     @u1 = User.create!
     @u2 = User.create!
@@ -161,13 +161,13 @@
     @cc3.reload
     expect(@cc3.position).to eql(1)
   end
-  
+
   context "can_notify?" do
     it "should normally be able to be used" do
       communication_channel_model
       expect(@communication_channel).to be_can_notify
     end
-    
+
     it "should not be able to be used if it has a policy to not use it" do
       communication_channel_model
       notification_policy_model(:frequency => "never", :communication_channel => @communication_channel)
@@ -298,6 +298,7 @@
             path: path,
             timestamp: nil,
             details: nil,
+            permanent_bounce: true,
             suppression_bounce: false
           )
         end
@@ -315,36 +316,63 @@
         cc = communication_channel_model(
           path: 'foo@bar.edu',
           last_bounce_at: '2015-01-01T01:01:01.000Z',
-          last_suppression_bounce_at: '2015-03-03T03:03:03.000Z'
+          last_suppression_bounce_at: '2015-03-03T03:03:03.000Z',
+          last_transient_bounce_at: '2015-04-04T04:04:04.000Z'
         )
         CommunicationChannel.bounce_for_path(
           path: 'foo@bar.edu',
           timestamp: '2015-02-02T02:02:02.000Z',
           details: nil,
+          permanent_bounce: true,
           suppression_bounce: false
         )
 
         cc.reload
         expect(cc.last_bounce_at).to eq('2015-02-02T02:02:02.000Z')
         expect(cc.last_suppression_bounce_at).to eq('2015-03-03T03:03:03.000Z')
+        expect(cc.last_transient_bounce_at).to eq('2015-04-04T04:04:04.000Z')
+      end
+
+      it "stores the date of the last soft bounce bounce" do
+        cc = communication_channel_model(
+          path: 'foo@bar.edu',
+          last_bounce_at: '2015-01-01T01:01:01.000Z',
+          last_suppression_bounce_at: '2015-03-03T03:03:03.000Z',
+          last_transient_bounce_at: '2015-04-04T04:04:04.000Z'
+        )
+        CommunicationChannel.bounce_for_path(
+          path: 'foo@bar.edu',
+          timestamp: '2015-05-05T05:05:05.000Z',
+          details: nil,
+          permanent_bounce: false,
+          suppression_bounce: false
+        )
+
+        cc.reload
+        expect(cc.last_bounce_at).to eq('2015-01-01T01:01:01.000Z')
+        expect(cc.last_suppression_bounce_at).to eq('2015-03-03T03:03:03.000Z')
+        expect(cc.last_transient_bounce_at).to eq('2015-05-05T05:05:05.000Z')
       end
 
       it "stores the date of the last suppression bounce" do
         cc = communication_channel_model(
           path: 'foo@bar.edu',
           last_bounce_at: '2015-01-01T01:01:01.000Z',
-          last_suppression_bounce_at: '2015-03-03T03:03:03.000Z'
+          last_suppression_bounce_at: '2015-03-03T03:03:03.000Z',
+          last_transient_bounce_at: '2015-04-04T04:04:04.000Z'
         )
         CommunicationChannel.bounce_for_path(
           path: 'foo@bar.edu',
           timestamp: '2015-02-02T02:02:02.000Z',
           details: nil,
+          permanent_bounce: true,
           suppression_bounce: true
         )
 
         cc.reload
         expect(cc.last_bounce_at).to eq('2015-01-01T01:01:01.000Z')
         expect(cc.last_suppression_bounce_at).to eq('2015-02-02T02:02:02.000Z')
+        expect(cc.last_transient_bounce_at).to eq('2015-04-04T04:04:04.000Z')
       end
 
       it "stores the details of the last hard bounce" do
@@ -353,11 +381,28 @@
           path: 'foo@bar.edu',
           timestamp: nil,
           details: {'some' => 'details', 'foo' => 'bar'},
+          permanent_bounce: true,
           suppression_bounce: false
         )
 
         cc.reload
         expect(cc.last_bounce_details).to eq('some' => 'details', 'foo' => 'bar')
+        expect(cc.last_transient_bounce_details).to be_nil
+      end
+
+      it "stores the details of the last soft bounce" do
+        cc = communication_channel_model(path: 'foo@bar.edu')
+        CommunicationChannel.bounce_for_path(
+          path: 'foo@bar.edu',
+          timestamp: nil,
+          details: {'some' => 'details', 'foo' => 'bar'},
+          permanent_bounce: false,
+          suppression_bounce: false
+        )
+
+        cc.reload
+        expect(cc.last_transient_bounce_details).to eq('some' => 'details', 'foo' => 'bar')
+        expect(cc.last_bounce_details).to be_nil
       end
 
       it "does not store the details of the last suppression bounce" do
@@ -369,11 +414,13 @@
           path: 'foo@bar.edu',
           timestamp: nil,
           details: {'some' => 'details', 'foo' => 'bar'},
+          permanent_bounce: true,
           suppression_bounce: true
         )
 
         cc.reload
         expect(cc.last_bounce_details).to eq('existing' => 'details')
+        expect(cc.last_transient_bounce_details).to be_nil
       end
     end
 
@@ -431,6 +478,7 @@
               path: path,
               timestamp: nil,
               details: nil,
+              permanent_bounce: true,
               suppression_bounce: false
             )
           end