From cfafc54a814a3bf476e6325d1b2e395ed83cdf7e Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Tue, 26 Aug 2025 13:22:22 +0200 Subject: [PATCH] Deliver emails for bundled notifications (initial WIP) --- README.md | 2 +- .../notification/bundle/deliver_all_job.rb | 5 + app/jobs/notification/bundle/deliver_job.rb | 5 + app/mailers/notification/bundle_mailer.rb | 11 +++ app/models/notification.rb | 5 + app/models/notification/bundle.rb | 51 +++++++++++ app/models/user.rb | 4 +- app/models/user/notifiable.rb | 19 ++++ .../bundle_mailer/notification.html.erb | 9 ++ .../bundle_mailer/notification.text.erb | 1 + config/recurring.yml | 3 + ...50826084559_create_notification_bundles.rb | 15 +++ db/schema.rb | 15 ++- db/schema_cache.yml | 91 ++++++++++++++++++- .../notification/bundle_mailer_preview.rb | 6 ++ test/models/notification/bundle_test.rb | 35 +++++++ 16 files changed, 271 insertions(+), 6 deletions(-) create mode 100644 app/jobs/notification/bundle/deliver_all_job.rb create mode 100644 app/jobs/notification/bundle/deliver_job.rb create mode 100644 app/mailers/notification/bundle_mailer.rb create mode 100644 app/models/notification/bundle.rb create mode 100644 app/models/user/notifiable.rb create mode 100644 app/views/notification/bundle_mailer/notification.html.erb create mode 100644 app/views/notification/bundle_mailer/notification.text.erb create mode 100644 db/migrate/20250826084559_create_notification_bundles.rb create mode 100644 test/mailers/previews/notification/bundle_mailer_preview.rb create mode 100644 test/models/notification/bundle_test.rb diff --git a/README.md b/README.md index 9fc392287..dba2f4c3d 100644 --- a/README.md +++ b/README.md @@ -67,7 +67,7 @@ You need to make sure those timestamps are always the same across executions. #### Development -You can view email previews at http://know-it-all.localhost:3005/rails/mailers. +You can view email previews at http://fizzy.localhost:3006/rails/mailers. You can enable or disable [`letter_opener`](https://github.com/ryanb/letter_opener) to open sent emails automatically with: diff --git a/app/jobs/notification/bundle/deliver_all_job.rb b/app/jobs/notification/bundle/deliver_all_job.rb new file mode 100644 index 000000000..cdd6093ce --- /dev/null +++ b/app/jobs/notification/bundle/deliver_all_job.rb @@ -0,0 +1,5 @@ +class Notification::Bundle::DeliverAllJob < ApplicationJob + def perform + Notification::Bundle.deliver_all + end +end diff --git a/app/jobs/notification/bundle/deliver_job.rb b/app/jobs/notification/bundle/deliver_job.rb new file mode 100644 index 000000000..b4c64a6c2 --- /dev/null +++ b/app/jobs/notification/bundle/deliver_job.rb @@ -0,0 +1,5 @@ +class Notification::Bundle::DeliverJob < ApplicationJob + def perform(bundle) + bundle.deliver + end +end diff --git a/app/mailers/notification/bundle_mailer.rb b/app/mailers/notification/bundle_mailer.rb new file mode 100644 index 000000000..a3e830b16 --- /dev/null +++ b/app/mailers/notification/bundle_mailer.rb @@ -0,0 +1,11 @@ +class Notification::BundleMailer < ApplicationMailer + def notification(bundle) + @bundle = bundle + @notifications = bundle.notifications + + mail( + to: bundle.user.email_address, + subject: "You have #{@notifications.count} notifications" + ) + end +end \ No newline at end of file diff --git a/app/models/notification.rb b/app/models/notification.rb index 943cafd7e..0b1e2e377 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -10,6 +10,7 @@ class Notification < ApplicationRecord scope :ordered, -> { order(read_at: :desc, created_at: :desc) } after_create_commit :broadcast_unread + after_create :bundle delegate :notifiable_target, to: :source delegate :card, to: :source @@ -40,4 +41,8 @@ class Notification < ApplicationRecord def broadcast_read broadcast_remove_to user, :notifications end + + def bundle + user.bundle(self) + end end diff --git a/app/models/notification/bundle.rb b/app/models/notification/bundle.rb new file mode 100644 index 000000000..31b16ac7e --- /dev/null +++ b/app/models/notification/bundle.rb @@ -0,0 +1,51 @@ +class Notification::Bundle < ApplicationRecord + belongs_to :user + + enum :status, %i[ pending processing delivered ] + + before_create :set_default_window + + scope :due, -> { pending.where("ends_at <= ?", Time.current) } + + class << self + def deliver_all + due.in_batches do |batch| + DeliverJob.perform_all_later batch + end + end + + def deliver_all_later + DeliverAllJob.perform_later + end + end + + def add(notification) + update! ends_at: [ ends_at, notification.created_at ].max + end + + def notifications + user.notifications.where(created_at: window) + end + + def deliver + processing! + BundleMailer.notification(self).deliver + delivered! + end + + def deliver_later + DeliverJob.perform_later(self) + end + + private + AGGREGATION_PERIOD = 4.hours + + def set_default_window + self.starts_at ||= Time.current + self.ends_at ||= AGGREGATION_PERIOD.from_now + end + + def window + starts_at..ends_at + end +end diff --git a/app/models/user.rb b/app/models/user.rb index bd8cf990d..21007d711 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,5 +1,5 @@ class User < ApplicationRecord - include Accessor, Attachable, Assignee, Mentionable, Named, Role, Searcher, + include Accessor, Attachable, Assignee, Mentionable, Named, Notifiable, Role, Searcher, SignalUser, Staff, Transferable, Conversational include Timelined # Depends on Accessor @@ -10,8 +10,6 @@ class User < ApplicationRecord has_many :comments, inverse_of: :creator, dependent: :destroy - has_many :notifications, dependent: :destroy - has_many :filters, foreign_key: :creator_id, inverse_of: :creator, dependent: :destroy has_many :closures, dependent: :nullify has_many :pins, dependent: :destroy diff --git a/app/models/user/notifiable.rb b/app/models/user/notifiable.rb new file mode 100644 index 000000000..0f6cf3b94 --- /dev/null +++ b/app/models/user/notifiable.rb @@ -0,0 +1,19 @@ +module User::Notifiable + extend ActiveSupport::Concern + + included do + has_many :notifications, dependent: :destroy + has_many :notification_bundles, class_name: "Notification::Bundle", dependent: :destroy + end + + def bundle(notification) + transaction do + find_or_create_pending_bundle_for(notification).add(notification) + end + end + + private + def find_or_create_pending_bundle_for(notification) + notification_bundles.pending.last || notification_bundles.create!(starts_at: notification.created_at) + end +end diff --git a/app/views/notification/bundle_mailer/notification.html.erb b/app/views/notification/bundle_mailer/notification.html.erb new file mode 100644 index 000000000..1fd9371f0 --- /dev/null +++ b/app/views/notification/bundle_mailer/notification.html.erb @@ -0,0 +1,9 @@ +

Your Notifications

+ +

You have <%= @notifications.count %> notifications:

+ + \ No newline at end of file diff --git a/app/views/notification/bundle_mailer/notification.text.erb b/app/views/notification/bundle_mailer/notification.text.erb new file mode 100644 index 000000000..8b7c8f424 --- /dev/null +++ b/app/views/notification/bundle_mailer/notification.text.erb @@ -0,0 +1 @@ +PENDING... diff --git a/config/recurring.yml b/config/recurring.yml index 99b3c1f33..66188ec7a 100644 --- a/config/recurring.yml +++ b/config/recurring.yml @@ -17,6 +17,9 @@ production: &production clear_solid_queue_finished_jobs: command: "SolidQueue::Job.clear_finished_in_batches(sleep_between_batches: 0.3)" schedule: every hour at minute 12 + deliver_bundled_notifications: + command: "Notification::Bundle.deliver_all_later" + schedule: every 30 minutes beta: *production staging: *production diff --git a/db/migrate/20250826084559_create_notification_bundles.rb b/db/migrate/20250826084559_create_notification_bundles.rb new file mode 100644 index 000000000..5c3f3ebfb --- /dev/null +++ b/db/migrate/20250826084559_create_notification_bundles.rb @@ -0,0 +1,15 @@ +class CreateNotificationBundles < ActiveRecord::Migration[8.1] + def change + create_table :notification_bundles do |t| + t.references :user, null: false, foreign_key: true, index: { unique: true } + t.datetime :starts_at, null: false + t.datetime :ends_at, null: false + t.integer :status, default: 0, null: false + + t.timestamps + end + + add_index :notification_bundles, %i[ user_id starts_at ends_at ] + add_index :notification_bundles, %i[ user_id status ] + end +end diff --git a/db/schema.rb b/db/schema.rb index b31a7dc51..c27e4b231 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2025_08_19_105245) do +ActiveRecord::Schema[8.1].define(version: 2025_08_26_084559) do create_table "accesses", force: :cascade do |t| t.datetime "accessed_at" t.integer "collection_id", null: false @@ -295,6 +295,18 @@ ActiveRecord::Schema[8.1].define(version: 2025_08_19_105245) do t.index ["source_type", "source_id"], name: "index_mentions_on_source" end + create_table "notification_bundles", force: :cascade do |t| + t.datetime "created_at", null: false + t.datetime "ends_at", null: false + t.datetime "starts_at", null: false + t.integer "status", default: 0, null: false + t.datetime "updated_at", null: false + t.integer "user_id", null: false + t.index ["user_id", "starts_at", "ends_at"], name: "idx_on_user_id_starts_at_ends_at_7eae5d3ac5" + t.index ["user_id", "status"], name: "index_notification_bundles_on_user_id_and_status" + t.index ["user_id"], name: "index_notification_bundles_on_user_id", unique: true + end + create_table "notifications", force: :cascade do |t| t.datetime "created_at", null: false t.integer "creator_id" @@ -451,6 +463,7 @@ ActiveRecord::Schema[8.1].define(version: 2025_08_19_105245) do add_foreign_key "events", "collections" add_foreign_key "mentions", "users", column: "mentionee_id" add_foreign_key "mentions", "users", column: "mentioner_id" + add_foreign_key "notification_bundles", "users" add_foreign_key "notifications", "users" add_foreign_key "notifications", "users", column: "creator_id" add_foreign_key "pins", "cards" diff --git a/db/schema_cache.yml b/db/schema_cache.yml index 58f3518f3..811f78807 100644 --- a/db/schema_cache.yml +++ b/db/schema_cache.yml @@ -909,6 +909,41 @@ columns: collation: comment: - *9 + notification_bundles: + - *5 + - !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column + auto_increment: + name: ends_at + cast_type: *1 + sql_type_metadata: *2 + 'null': false + default: + default_function: + collation: + comment: + - *6 + - !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column + auto_increment: + name: starts_at + cast_type: *1 + sql_type_metadata: *2 + 'null': false + default: + default_function: + collation: + comment: + - !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column + auto_increment: + name: status + cast_type: *3 + sql_type_metadata: *4 + 'null': false + default: 0 + default_function: + collation: + comment: + - *9 + - *25 notifications: - *5 - !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column @@ -1284,6 +1319,7 @@ primary_keys: filters_stages: filters_tags: mentions: id + notification_bundles: id notifications: id pins: id push_subscriptions: id @@ -1332,6 +1368,7 @@ data_sources: filters_stages: true filters_tags: true mentions: true + notification_bundles: true notifications: true pins: true push_subscriptions: true @@ -2294,6 +2331,58 @@ indexes: nulls_not_distinct: comment: valid: true + notification_bundles: + - !ruby/object:ActiveRecord::ConnectionAdapters::IndexDefinition + table: notification_bundles + name: idx_on_user_id_starts_at_ends_at_7eae5d3ac5 + unique: false + columns: + - user_id + - starts_at + - ends_at + lengths: {} + orders: {} + opclasses: {} + where: + type: + using: + include: + nulls_not_distinct: + comment: + valid: true + - !ruby/object:ActiveRecord::ConnectionAdapters::IndexDefinition + table: notification_bundles + name: index_notification_bundles_on_user_id + unique: true + columns: + - user_id + lengths: {} + orders: {} + opclasses: {} + where: + type: + using: + include: + nulls_not_distinct: + comment: + valid: true + - !ruby/object:ActiveRecord::ConnectionAdapters::IndexDefinition + table: notification_bundles + name: index_notification_bundles_on_user_id_and_status + unique: false + columns: + - user_id + - status + lengths: {} + orders: {} + opclasses: {} + where: + type: + using: + include: + nulls_not_distinct: + comment: + valid: true notifications: - !ruby/object:ActiveRecord::ConnectionAdapters::IndexDefinition table: notifications @@ -2771,4 +2860,4 @@ indexes: comment: valid: true workflows: [] -version: 20250819105245 +version: 20250826084559 diff --git a/test/mailers/previews/notification/bundle_mailer_preview.rb b/test/mailers/previews/notification/bundle_mailer_preview.rb new file mode 100644 index 000000000..3c279e916 --- /dev/null +++ b/test/mailers/previews/notification/bundle_mailer_preview.rb @@ -0,0 +1,6 @@ +class Notification::BundleMailerPreview < ActionMailer::Preview + def notification + ApplicationRecord.current_tenant = "1065895976" + Notification::BundleMailer.notification Notification::Bundle.take! + end +end diff --git a/test/models/notification/bundle_test.rb b/test/models/notification/bundle_test.rb new file mode 100644 index 000000000..1033a46a7 --- /dev/null +++ b/test/models/notification/bundle_test.rb @@ -0,0 +1,35 @@ +require "test_helper" + +class Notification::BundleTest < ActiveSupport::TestCase + setup do + @user = users(:david) + end + + test "new notifications are bundled" do + notification = assert_difference -> { @user.notification_bundles.pending.count }, 1 do + @user.notifications.create!(source: events(:logo_published), creator: @user) + end + + bundle = @user.notification_bundles.pending.last + assert_includes bundle.notifications, notification + end + + test "notifications are bundled withing the aggregation period" do + notification_1 = assert_difference -> { @user.notification_bundles.pending.count }, 1 do + @user.notifications.create!(source: events(:logo_published), creator: @user) + end + travel_to 3.hours.from_now + + notification_2 = assert_no_difference -> { @user.notification_bundles.count } do + @user.notifications.create!(source: events(:logo_published), creator: @user) + end + travel_to 3.hours.from_now + + notification_3 = assert_difference -> { @user.notification_bundles.pending.count }, 1 do + @user.notifications.create!(source: events(:logo_published), creator: @user) + end + + bundle_1, bundle_2 = @user.notification_bundles.last(2) + + end +end