From a29dd89683e0d64b38c73a99a3f1f0d76b09abde Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sat, 22 Nov 2025 22:19:05 -0500 Subject: [PATCH 1/2] Make sure Bundle#ends_at is set before validation otherwise we risk creating overlapping bundles and not catching it. --- app/models/notification/bundle.rb | 2 +- test/models/notification/bundle_test.rb | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/models/notification/bundle.rb b/app/models/notification/bundle.rb index e6f63acaa..fa9c9a111 100644 --- a/app/models/notification/bundle.rb +++ b/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 diff --git a/test/models/notification/bundle_test.rb b/test/models/notification/bundle_test.rb index bd45d0e1e..dedd12362 100644 --- a/test/models/notification/bundle_test.rb +++ b/test/models/notification/bundle_test.rb @@ -83,6 +83,16 @@ 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 notification = @user.notifications.create!(source: events(:logo_published), creator: @user) From db51b616bb27b9ce33edfe6d6bf4d0588a544f50 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sat, 22 Nov 2025 22:19:49 -0500 Subject: [PATCH 2/2] Handle notifs created before the current bundle starts_at --- app/models/notification/bundle.rb | 10 +++++----- app/models/user/notifiable.rb | 9 ++++++++- test/models/notification/bundle_test.rb | 22 +++++++++++++++++++++- 3 files changed, 34 insertions(+), 7 deletions(-) diff --git a/app/models/notification/bundle.rb b/app/models/notification/bundle.rb index fa9c9a111..8617a0f99 100644 --- a/app/models/notification/bundle.rb +++ b/app/models/notification/bundle.rb @@ -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 diff --git a/app/models/user/notifiable.rb b/app/models/user/notifiable.rb index ef5249611..f6788aaac 100644 --- a/app/models/user/notifiable.rb +++ b/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 diff --git a/test/models/notification/bundle_test.rb b/test/models/notification/bundle_test.rb index dedd12362..dc4bf9e43 100644 --- a/test/models/notification/bundle_test.rb +++ b/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 @@ -94,6 +97,8 @@ class Notification::BundleTest < ActiveSupport::TestCase 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 @@ -140,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