diff --git a/app/models/notification/bundle.rb b/app/models/notification/bundle.rb index e6f63acaa..8617a0f99 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 @@ -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 bd45d0e1e..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 @@ -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