Skip to content

Commit

Permalink
fix[Op#59433]: simplify reminder model to directly linked to notifica…
Browse files Browse the repository at this point in the history
…tion

Introduce simple status enums to track various reminder states
  • Loading branch information
akabiru committed Nov 22, 2024
1 parent 4c01600 commit f30c92a
Show file tree
Hide file tree
Showing 10 changed files with 20 additions and 70 deletions.
6 changes: 2 additions & 4 deletions app/models/reminder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,7 @@
class Reminder < ApplicationRecord
belongs_to :remindable, polymorphic: true
belongs_to :creator, class_name: "User"
belongs_to :notification, optional: true

has_many :reminder_notifications, dependent: :destroy

def notified? = notified_at.present?
def scheduled? = job_id.present?
enum :status, { pending: 0, scheduled: 1, notified: 2, done: 9 }
end
4 changes: 0 additions & 4 deletions app/models/reminder_notification.rb

This file was deleted.

2 changes: 1 addition & 1 deletion app/services/reminders/create_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class CreateService < ::BaseServices::Create
def after_perform(service_call)
reminder = service_call.result
job = Reminders::ScheduleReminderJob.schedule(reminder)
reminder.update_column(:job_id, job.job_id)
reminder.update_columns(status: :scheduled, job_id: job.job_id)

service_call
end
Expand Down
3 changes: 1 addition & 2 deletions app/workers/reminders/schedule_reminder_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,7 @@ def perform(reminder)
create_notification_service = create_notification_from_reminder(reminder)

create_notification_service.on_success do |service_result|
ReminderNotification.create!(reminder:, notification: service_result.result)
reminder.update_column(:notified_at, Time.current)
reminder.update_columns(status: :notified, notification_id: service_result.result.id)
end

create_notification_service.on_failure do |service_result|
Expand Down
5 changes: 3 additions & 2 deletions db/migrate/20241119131205_create_reminders.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ def change
create_table :reminders do |t|
t.references :remindable, polymorphic: true, null: false
t.references :creator, null: false, foreign_key: { to_table: :users }
t.references :notification, foreign_key: true
t.datetime :remind_at, null: false
t.datetime :notified_at
t.integer :status, default: 0, null: false
t.string :job_id
t.text :note
t.string :note

t.timestamps
end
Expand Down
14 changes: 0 additions & 14 deletions db/migrate/20241121113638_create_reminder_notifications.rb

This file was deleted.

4 changes: 3 additions & 1 deletion spec/factories/reminders_factory.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@
end

trait :notified do
notified_at { remind_at + 1.second }
job_id { SecureRandom.uuid }
status { :notified }
notification factory: :notification
end
end
end
36 changes: 0 additions & 36 deletions spec/models/reminder_notification_spec.rb

This file was deleted.

4 changes: 2 additions & 2 deletions spec/services/reminders/create_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
let(:factory) { :reminder }

before do
allow(model_instance).to receive(:update_column).and_return(true)
allow(model_instance).to receive(:update_columns)
allow(Reminders::ScheduleReminderJob).to receive(:schedule)
.with(model_instance)
.and_return(instance_double(Reminders::ScheduleReminderJob, job_id: 1))
Expand All @@ -46,7 +46,7 @@
subject

expect(Reminders::ScheduleReminderJob).to have_received(:schedule).with(model_instance)
expect(model_instance).to have_received(:update_column).with(:job_id, 1)
expect(model_instance).to have_received(:update_columns).with(status: :scheduled, job_id: 1)
end
end
end
12 changes: 8 additions & 4 deletions spec/workers/reminders/schedule_reminder_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,28 +49,32 @@

it "creates a notification from the reminder" do
notification_svc = nil
expect { notification_svc = subject }.to change(Notification, :count).by(1) & change(ReminderNotification, :count).by(1)
expect { notification_svc = subject }.to change(Notification, :count).by(1) & change(reminder, :status).to("notified")

aggregate_failures "notification attributes" do
notification = notification_svc.result
notification = notification_svc.result

aggregate_failures "notification attributes" do
expect(notification.recipient_id).to eq(reminder.creator_id)
expect(notification.resource).to eq(reminder.remindable)
expect(notification.reason).to eq("reminder")
end

aggregate_failures "marks the reminder as notified" do
expect(reminder.reload).to be_notified
expect(reminder.notification_id).to eq(notification.id)
end
end

context "when the reminder is already notified" do
let(:reminder) { build_stubbed(:reminder, :notified) }

before do
reminder.update_column(:notified_at, Time.current)
allow(Notifications::CreateService).to receive(:new).and_call_original
end

it "does not create a notification from the reminder" do
expect { subject }.not_to change(Notification, :count)
expect(Notifications::CreateService).not_to have_received(:new)
end
end
end
Expand Down

0 comments on commit f30c92a

Please sign in to comment.