✅ Add specs for batch email notifications

This commit will add a few specs and also redo the logic for the batch
email notification send frequency.  We needed a migration to the users
model to track when the last email was sent to check if it's time to
send another one based on their batch_email_frequency setting.

app/jobs/batch_email_notification_job.rb

@@ -11,6 +11,7 @@ def perform(account)
       # Query for all users that have email_frequency turned off
       users = User.where.not(batch_email_frequency: "never")
       users.each do |user|
+        next unless send_email_today?(user)
         # Find all undelivered messages within the frequency range of a user and any emails that haven't been sent
         undelivered_messages =
           Mailboxer::Message.joins(:receipts)
@@ -21,14 +22,16 @@ def perform(account)
                             .to_a
 
         next if undelivered_messages.blank?
-        send_email(user, undelivered_messages)
+        send_email(user, undelivered_messages, account)
 
         # Mark the as read
         undelivered_messages.each do |message|
           message.receipts.each do |receipt|
             receipt.update(is_delivered: true)
           end
         end
+
+        user.update(last_emailed_at: Time.current)
       end
     end
   end
@@ -39,6 +42,21 @@ def reenqueue(account)
     BatchEmailNotificationJob.set(wait_until: Date.tomorrow.midnight).perform_later(account)
   end
 
+  def send_email_today?(user)
+    return true if user.last_emailed_at.nil?
+
+    next_email_date = case user.batch_email_frequency
+                      when "daily"
+                        user.last_emailed_at + 1.day
+                      when "weekly"
+                        user.last_emailed_at + 1.week
+                      when "monthly"
+                        user.last_emailed_at + 1.month
+                      end
+
+    Time.current >= next_email_date
+  end
+
   def frequency_date(frequency)
     case frequency
     when "daily"
@@ -50,8 +68,7 @@ def frequency_date(frequency)
     end
   end
 
-  def send_email(user, undelivered_messages)
-    mailer = HykuMailer.new
-    mailer.summary_email(user, undelivered_messages)
+  def send_email(user, undelivered_messages, account)
+    HykuMailer.summary_email(user, undelivered_messages, account).deliver_now
   end
 end

app/mailers/hyku_mailer.rb

@@ -6,29 +6,26 @@ def default_url_options
     { host: host_for_tenant }
   end
 
-  def summary_email(user, messages)
+  def summary_email(user, messages, account)
     @user = user
     @messages = messages || []
-    @url = notifications_url
+    @account = account
+    @url = notifications_url_for(@account)
 
     mail(to: @user.email,
          subject: "You have #{messages.count} new message(s)",
-         from: current_tenant.contact_email,
+         from: @account.contact_email,
          template_path: 'hyku_mailer',
          template_name: 'summary_email')
   end
 
   private
 
   def host_for_tenant
-    current_tenant&.cname || Account.admin_host
+    Account.find_by(tenant: Apartment::Tenant.current)&.cname || Account.admin_host
   end
 
-  def current_tenant
-    Account.find_by(tenant: Apartment::Tenant.current)
-  end
-
-  def notifications_url
-    "https://#{current_tenant.cname}/notifications"
+  def notifications_url_for(account)
+    "https://#{account.cname}/notifications"
   end
 end

app/models/user.rb

@@ -169,8 +169,7 @@ def add_default_group_membership!
   # (really the receipts of the messages) to is_delivered tru
   def mark_all_undelivered_messages_as_delivered!
     mailbox.receipts.where(is_delivered: false).find_each do |receipt|
-      receipt.is_delivered = true
-      receipt.save
+      receipt.update(is_delivered: true)
     end
   end
 end

db/migrate/20240502230546_add_last_emailed_at_to_users.rb

@@ -0,0 +1,5 @@
+class AddLastEmailedAtToUsers < ActiveRecord::Migration[6.1]
+  def change
+    add_column :users, :last_emailed_at, :datetime unless column_exists?(:users, :last_emailed_at)
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2024_04_29_213539) do
+ActiveRecord::Schema.define(version: 2024_05_02_230546) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "hstore"
@@ -915,6 +915,7 @@
     t.string "provider"
     t.string "uid"
     t.string "batch_email_frequency", default: "never"
+    t.datetime "last_emailed_at"
     t.index ["email"], name: "index_users_on_email", unique: true
     t.index ["invitation_token"], name: "index_users_on_invitation_token", unique: true
     t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true

spec/factories/mailboxer_conversations.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :mailboxer_conversation, class: 'Mailboxer::Conversation' do
+    subject { 'Conversation subject' }
+  end
+end

spec/factories/mailboxer_messages.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :mailboxer_message, class: 'Mailboxer::Message' do
+    type { 'Mailboxer::Message' }
+    body { 'Message body' }
+    subject { 'Message subject' }
+    association :sender, factory: :user
+    association :conversation, factory: :mailboxer_conversation
+  end
+end

spec/factories/mailboxer_notifications.rb

@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :mailboxer_notification, class: 'Mailboxer::Notification' do
+    type { 'Mailboxer::Notification' }
+    body { 'Notification body' }
+    subject { 'Notification subject' }
+    association :sender, factory: :user
+  end
+end

spec/factories/mailboxer_receipts.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :mailboxer_receipt, class: 'Mailboxer::Receipt' do
+    association :receiver, factory: :user
+    association :notification, factory: :mailboxer_message
+    is_read { false }
+    trashed { false }
+    deleted { false }
+    mailbox_type { "inbox" }
+    is_delivered { false }
+  end
+end

spec/jobs/batch_email_notification_job_spec.rb

@@ -0,0 +1,79 @@
+# frozen_string_literal: true
+
+RSpec.describe BatchEmailNotificationJob do
+  let(:account) { FactoryBot.create(:account) }
+  let(:receipt) { FactoryBot.create(:mailboxer_receipt, receiver: user) }
+  let!(:message) { receipt.notification }
+  let!(:user) { FactoryBot.create(:user, batch_email_frequency: frequency) }
+
+  before do
+    allow(Apartment::Tenant).to receive(:switch).and_yield
+    ActionMailer::Base.deliveries.clear
+  end
+
+  after do
+    clear_enqueued_jobs
+  end
+
+  describe '#perform' do
+    let(:frequency) { 'daily' }
+
+    it 'marks the message as delivered' do
+      expect { BatchEmailNotificationJob.perform_now(account) }.to change { message.receipts.first.is_delivered }.from(false).to(true)
+    end
+
+    it 're-enqueues the job' do
+      expect { BatchEmailNotificationJob.perform_now(account) }.to have_enqueued_job(BatchEmailNotificationJob).with(account)
+    end
+
+    context 'when the user has a daily frequency' do
+      let(:frequency) { 'daily' }
+
+      it 'sends email to users with batch_email_frequency set' do
+        expect { BatchEmailNotificationJob.perform_now(account) }.to change { ActionMailer::Base.deliveries.count }.by(1)
+      end
+    end
+
+    context 'when the user has a weekly frequency' do
+      let(:frequency) { 'weekly' }
+      let(:user) { FactoryBot.create(:user, batch_email_frequency: frequency, last_emailed_at:) }
+
+      context 'when the user was last emailed less than a week ago' do
+        let(:last_emailed_at) { 5.days.ago }
+
+        it 'does not send an email to users with batch_email_frequency set' do
+          expect { BatchEmailNotificationJob.perform_now(account) }.to_not change { ActionMailer::Base.deliveries.count }
+        end
+      end
+
+      context 'when the user was last emailed more than a week ago' do
+        let(:last_emailed_at) { 8.days.ago }
+
+        it 'sends email to users with batch_email_frequency set' do
+          expect { BatchEmailNotificationJob.perform_now(account) }.to change { ActionMailer::Base.deliveries.count }.by(1)
+        end
+      end
+    end
+
+    context 'when the user has a monthly frequency' do
+      let(:frequency) { 'monthly' }
+      let(:user) { FactoryBot.create(:user, batch_email_frequency: frequency, last_emailed_at:) }
+
+      context 'when the user was last emailed less than a month ago' do
+        let(:last_emailed_at) { 20.days.ago }
+
+        it 'does not send an email to users with batch_email_frequency set' do
+          expect { BatchEmailNotificationJob.perform_now(account) }.to_not change { ActionMailer::Base.deliveries.count }
+        end
+      end
+
+      context 'when the user was last emailed more than a month ago' do
+        let(:last_emailed_at) { 40.days.ago }
+
+        it 'sends email to users with batch_email_frequency set' do
+          expect { BatchEmailNotificationJob.perform_now(account) }.to change { ActionMailer::Base.deliveries.count }.by(1)
+        end
+      end
+    end
+  end
+end

spec/models/mailboxer/receipt_decorator_spec.rb

@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+RSpec.describe Mailboxer::Receipt, type: :decorator do
+  describe '#mark_as_delivered' do
+    context 'when the user has batch_email_frequency set to never' do
+      let(:user) { create(:user, batch_email_frequency: 'never') }
+      let(:receipt) { create(:mailboxer_receipt, receiver: user) }
+
+      before do
+        # ensure we have a undelivered receipt
+        receipt.update(is_delivered: false)
+      end
+
+      it 'marks the receipt as delivered' do
+        expect { receipt.mark_as_delivered }.to change { receipt.is_delivered }.from(false).to(true)
+      end
+    end
+
+    context 'when the user does not have batch_email_frequency set to never' do
+      let(:user) { create(:user, batch_email_frequency: 'daily') }
+      let(:receipt) { create(:mailboxer_receipt, receiver: user) }
+
+      before do
+        # ensure we have a undelivered receipt
+        receipt.update(is_delivered: false)
+      end
+
+      it 'does not mark the receipt as delivered' do
+        expect { receipt.mark_as_delivered }.not_to change { receipt.is_delivered }
+      end
+    end
+  end
+end

spec/models/user_spec.rb

@@ -1,14 +1,14 @@
 # frozen_string_literal: true
 
 RSpec.describe User, type: :model do
+  subject { FactoryBot.create(:user) }
+
   it 'validates email and password' do
     is_expected.to validate_presence_of(:email)
     is_expected.to validate_presence_of(:password)
   end
 
   context 'the first created user in global tenant' do
-    subject { FactoryBot.create(:user) }
-
     before do
       allow(Account).to receive(:global_tenant?).and_return true
     end
@@ -21,8 +21,6 @@
   end
 
   context 'the first created user on a tenant' do
-    subject { FactoryBot.create(:user) }
-
     it 'is not given the admin role' do
       expect(subject).not_to have_role :admin
       expect(subject).not_to have_role :admin, Site.instance
@@ -48,8 +46,6 @@
   end
 
   describe '#site_roles=' do
-    subject { FactoryBot.create(:user) }
-
     it 'assigns global roles to the user' do
       expect(subject.site_roles.pluck(:name)).to be_empty
 
@@ -66,17 +62,13 @@
   end
 
   describe '#hyrax_groups' do
-    subject { FactoryBot.create(:user) }
-
     it 'returns an array of Hyrax::Groups' do
       expect(subject.hyrax_groups).to be_an_instance_of(Array)
       expect(subject.hyrax_groups.first).to be_an_instance_of(Hyrax::Group)
     end
   end
 
   describe '#groups' do
-    subject { FactoryBot.create(:user) }
-
     before do
       FactoryBot.create(:group, name: 'group1', member_users: [subject])
     end
@@ -87,8 +79,6 @@
   end
 
   describe '#hyrax_group_names' do
-    subject { FactoryBot.create(:user) }
-
     before do
       FactoryBot.create(:group, name: 'group1', member_users: [subject])
     end
@@ -136,4 +126,27 @@
       end
     end
   end
+
+  describe '#mark_all_undelivered_messages_as_delivered!' do
+    let(:receipt) { create(:mailboxer_receipt, receiver: subject) }
+
+    before do
+      # ensure we have a undelivered receipt
+      receipt.update(is_delivered: false)
+    end
+
+    context 'when batch_email_frequency is set to never' do
+      it 'marks all undelivered messages as delivered' do
+        subject.update(batch_email_frequency: 'never')
+        expect(receipt.reload.is_delivered).to be true
+      end
+    end
+
+    context 'when batch_email_frequency is not set to never' do
+      it 'does not mark all undelivered messages as delivered' do
+        subject.update(batch_email_frequency: 'daily')
+        expect(receipt.reload.is_delivered).to be false
+      end
+    end
+  end
 end