From eefbc1bc5f795fdf4d7c06f8330f31d6fc955cb8 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 22 Nov 2024 22:52:03 +0300 Subject: [PATCH] fix[Op#59433]: simplify reminder model to directly linked to notification Introduce simple status enums to track various reminder states --- app/models/reminder.rb | 6 ++-- app/models/reminder_notification.rb | 4 --- app/services/reminders/create_service.rb | 2 +- .../reminders/schedule_reminder_job.rb | 3 +- db/migrate/20241119131205_create_reminders.rb | 5 +-- ...121113638_create_reminder_notifications.rb | 14 -------- spec/factories/reminders_factory.rb | 4 ++- spec/models/reminder_notification_spec.rb | 36 ------------------- spec/models/reminder_spec.rb | 32 ++++------------- .../services/reminders/create_service_spec.rb | 4 +-- .../reminders/schedule_reminder_job_spec.rb | 12 ++++--- 11 files changed, 26 insertions(+), 96 deletions(-) delete mode 100644 app/models/reminder_notification.rb delete mode 100644 db/migrate/20241121113638_create_reminder_notifications.rb delete mode 100644 spec/models/reminder_notification_spec.rb diff --git a/app/models/reminder.rb b/app/models/reminder.rb index bc4d9802b5a8..8218ea972218 100644 --- a/app/models/reminder.rb +++ b/app/models/reminder.rb @@ -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 diff --git a/app/models/reminder_notification.rb b/app/models/reminder_notification.rb deleted file mode 100644 index cbdd70905a9b..000000000000 --- a/app/models/reminder_notification.rb +++ /dev/null @@ -1,4 +0,0 @@ -class ReminderNotification < ApplicationRecord - belongs_to :reminder - belongs_to :notification -end diff --git a/app/services/reminders/create_service.rb b/app/services/reminders/create_service.rb index 426c08819590..7674e63e059c 100644 --- a/app/services/reminders/create_service.rb +++ b/app/services/reminders/create_service.rb @@ -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 diff --git a/app/workers/reminders/schedule_reminder_job.rb b/app/workers/reminders/schedule_reminder_job.rb index 395c95123dc6..95d64057b92f 100644 --- a/app/workers/reminders/schedule_reminder_job.rb +++ b/app/workers/reminders/schedule_reminder_job.rb @@ -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| diff --git a/db/migrate/20241119131205_create_reminders.rb b/db/migrate/20241119131205_create_reminders.rb index bcc84c73ae97..e74d16016420 100644 --- a/db/migrate/20241119131205_create_reminders.rb +++ b/db/migrate/20241119131205_create_reminders.rb @@ -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 diff --git a/db/migrate/20241121113638_create_reminder_notifications.rb b/db/migrate/20241121113638_create_reminder_notifications.rb deleted file mode 100644 index eaa711b431e5..000000000000 --- a/db/migrate/20241121113638_create_reminder_notifications.rb +++ /dev/null @@ -1,14 +0,0 @@ -class CreateReminderNotifications < ActiveRecord::Migration[7.1] - def change - create_table :reminder_notifications do |t| - t.references :reminder, foreign_key: true - t.references :notification, foreign_key: true - - t.timestamps - end - - add_index :reminder_notifications, :notification_id, - unique: true, - name: "index_reminder_notifications_unique" - end -end diff --git a/spec/factories/reminders_factory.rb b/spec/factories/reminders_factory.rb index e6335941afaa..cda7d6a84e9c 100644 --- a/spec/factories/reminders_factory.rb +++ b/spec/factories/reminders_factory.rb @@ -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 diff --git a/spec/models/reminder_notification_spec.rb b/spec/models/reminder_notification_spec.rb deleted file mode 100644 index 95c8193813c2..000000000000 --- a/spec/models/reminder_notification_spec.rb +++ /dev/null @@ -1,36 +0,0 @@ -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -require "spec_helper" - -RSpec.describe ReminderNotification do - describe "Associations" do - it { is_expected.to belong_to(:reminder) } - it { is_expected.to belong_to(:notification) } - end -end diff --git a/spec/models/reminder_spec.rb b/spec/models/reminder_spec.rb index f8a6bbb6cb77..77c95514526c 100644 --- a/spec/models/reminder_spec.rb +++ b/spec/models/reminder_spec.rb @@ -32,34 +32,14 @@ describe "Associations" do it { is_expected.to belong_to(:remindable) } it { is_expected.to belong_to(:creator).class_name("User") } - it { is_expected.to have_many(:reminder_notifications).dependent(:destroy) } + it { is_expected.to belong_to(:notification).optional } end - describe "#notified?" do - context "when notified_at is present" do - subject { build(:reminder, :notified) } - - it { is_expected.to be_notified } - end - - context "when notified_at is not present" do - subject { build(:reminder) } - - it { is_expected.not_to be_notified } - end - end - - describe "#scheduled?" do - context "when job_id is present" do - subject { build(:reminder, :scheduled) } - - it { is_expected.to be_scheduled } - end - - context "when job_id is not present" do - subject { build(:reminder) } - - it { is_expected.not_to be_scheduled } + describe "Enumns" do + it do + expect(subject).to define_enum_for(:status) + .with_values({ pending: 0, scheduled: 1, notified: 2, done: 9 }) + .backed_by_column_of_type(:integer) end end end diff --git a/spec/services/reminders/create_service_spec.rb b/spec/services/reminders/create_service_spec.rb index 78ae257ed710..76a89a0bc27f 100644 --- a/spec/services/reminders/create_service_spec.rb +++ b/spec/services/reminders/create_service_spec.rb @@ -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)) @@ -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 diff --git a/spec/workers/reminders/schedule_reminder_job_spec.rb b/spec/workers/reminders/schedule_reminder_job_spec.rb index 4b3ed9fca65c..036d251e164a 100644 --- a/spec/workers/reminders/schedule_reminder_job_spec.rb +++ b/spec/workers/reminders/schedule_reminder_job_spec.rb @@ -49,11 +49,11 @@ 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") @@ -61,16 +61,20 @@ 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