Merge pull request #2541 from basecamp/notification-fixes

Fix race conditions with notifications

app/mailers/notification/bundle_mailer.rb

@@ -7,10 +7,14 @@ class Notification::BundleMailer < ApplicationMailer
     @user = bundle.user
     @bundle = bundle
     @notifications = bundle.notifications
+      .preload(:card, :creator, source: [ :board, :creator ])
+      .reject { |n| n.source.nil? || n.card.nil? }
     @unsubscribe_token = @user.generate_token_for(:unsubscribe)
 
-    mail \
-      to: bundle.user.identity.email_address,
-      subject: "Fizzy#{ " (#{ Current.account.name })" if @user.identity.accounts.many? }: New notifications"
+    if @notifications.any?
+      mail \
+        to: bundle.user.identity.email_address,
+        subject: "Fizzy#{ " (#{ Current.account.name })" if @user.identity.accounts.many? }: New notifications"
+    end
   end
 end

app/models/notifier.rb

@@ -16,12 +16,16 @@ class Notifier
     if should_notify?
       # Processing recipients in order avoids deadlocks if notifications overlap.
       recipients.sort_by(&:id).map do |recipient|
-        notification = Notification.find_or_initialize_by(user: recipient, card: source.card)
-        notification.source = source
-        notification.creator = creator
-        notification.read_at = nil
-        notification.unread_count = (notification.unread_count || 0) + 1
-        notification.save!
+        notification = Notification.create_or_find_by(user: recipient, card: source.card) do |n|
+          n.source = source
+          n.creator = creator
+          n.unread_count = 1
+        end
+
+        unless notification.previously_new_record?
+          notification.update!(source: source, creator: creator, read_at: nil, unread_count: notification.unread_count + 1)
+        end
+
         notification
       end
     end

test/mailers/notification/bundle_mailer_test.rb

@@ -97,6 +97,15 @@ class Notification::BundleMailerTest < ActionMailer::TestCase
     assert_equal "#1 Fix the <code>bug</code> in production", title_link.inner_html
   end
 
+  test "skips notifications whose source event was deleted" do
+    notification = create_notification(@user)
+    notification.source.destroy
+
+    email = Notification::BundleMailer.notification(@bundle)
+    assert_not email.respond_to?(:deliver) && email.message.is_a?(Mail::Message),
+      "Should not generate a real email when all notifications are stale"
+  end
+
   private
     def create_notification(user, source: events(:logo_published))
       Notification.create!(user: user, creator: user, source: source, created_at: 30.minutes.ago)