Merge pull request #1696 from basecamp/flavorjones/avoid-overlapping-bundles

Prevent overlapping bundles from being created

app/models/notification/bundle.rb

@@ -15,7 +15,7 @@ class Notification::Bundle < ApplicationRecord
     )
   end
 
-  before_create :set_default_window
+  before_validation :set_default_window, if: :new_record?
 
   validate :validate_no_overlapping
 
@@ -57,12 +57,12 @@ class Notification::Bundle < ApplicationRecord
     deliver_later
   end
 
-  private
-    def set_default_window
-      self.starts_at ||= Time.current
-      self.ends_at ||= self.starts_at + user.settings.bundle_aggregation_period
-    end
+  def set_default_window
+    self.starts_at ||= Time.current
+    self.ends_at ||= self.starts_at + user.settings.bundle_aggregation_period
+  end
 
+  private
     def window
       starts_at..ends_at
     end

app/models/user/notifiable.rb

@@ -16,13 +16,20 @@ module User::Notifiable
 
   private
     def find_or_create_bundle_for(notification)
-      find_bundle_for(notification) || create_bundle_for(notification)
+      find_bundle_for(notification) || expand_pending_bundle_for(notification) || create_bundle_for(notification)
     end
 
     def find_bundle_for(notification)
       notification_bundles.pending.containing(notification).last
     end
 
+    def expand_pending_bundle_for(notification)
+      pending = notification_bundles.pending.last
+      if pending.present? && notification.created_at < pending.starts_at
+        pending.update!(starts_at: notification.created_at) # expand the window to include this notification
+      end
+    end
+
     def create_bundle_for(notification)
       notification_bundles.create!(starts_at: notification.created_at)
     end

test/models/notification/bundle_test.rb

@@ -24,6 +24,8 @@ class Notification::BundleTest < ActiveSupport::TestCase
   end
 
   test "notifications are bundled withing the aggregation period" do
+    @user.notification_bundles.destroy_all
+
     notification_1 = assert_difference -> { @user.notification_bundles.pending.count }, 1 do
       @user.notifications.create!(source: events(:logo_published), creator: @user)
     end
@@ -38,7 +40,8 @@ class Notification::BundleTest < ActiveSupport::TestCase
       @user.notifications.create!(source: events(:logo_published), creator: @user)
     end
 
-    bundle_1, bundle_2 = @user.notification_bundles.last(2)
+    assert_equal 2, @user.notification_bundles.count
+    bundle_1, bundle_2 = @user.notification_bundles.all.to_a
     assert_includes bundle_1.notifications, notification_1
     assert_includes bundle_1.notifications, notification_2
     assert_includes bundle_2.notifications, notification_3
@@ -83,7 +86,19 @@ class Notification::BundleTest < ActiveSupport::TestCase
     assert bundle_5.valid?
   end
 
+  test "overlapping bundles that are created relying on set_default_window are not created" do
+    @user.notification_bundles.destroy_all
+
+    bundle = @user.notification_bundles.create!(starts_at: Time.current)
+
+    assert_raises ActiveRecord::RecordInvalid do
+      @user.notification_bundles.create!(starts_at: bundle.starts_at - 1.second)
+    end
+  end
+
   test "deliver_all delivers due bundles" do
+    @user.notification_bundles.destroy_all
+
     notification = @user.notifications.create!(source: events(:logo_published), creator: @user)
 
     bundle = @user.notification_bundles.pending.last
@@ -130,4 +145,19 @@ class Notification::BundleTest < ActiveSupport::TestCase
       assert_match /everything since 3pm/i, email.text_part&.body&.to_s
     end
   end
+
+  test "out-of-order notification bundling should still work" do
+    first_notification = @user.notifications.create!(source: events(:logo_published), creator: @user)
+    second_notification = @user.notifications.create!(source: events(:logo_published), creator: @user)
+    @user.notification_bundles.destroy_all
+
+    assert first_notification.created_at < second_notification.created_at
+    @user.bundle(second_notification)
+    @user.bundle(first_notification)
+
+    assert_equal 1, @user.notification_bundles.pending.count
+    assert_equal 2, @user.notification_bundles.last.notifications.count
+    assert_includes @user.notification_bundles.last.notifications, first_notification
+    assert_includes @user.notification_bundles.last.notifications, second_notification
+  end
 end