Revert "fix[Op#59433]: simplify reminder model to directly linked to …

…notification"

This reverts commit ae3f77d.

Maintain a persistent record of reminder notifications as notifications are always retrievable by the user, e.g. via
email, viewing "all" notifications.

A reminder notification is only created IF there are no active unread notifications, hence when rescheduling, snoozing or editing a reminder then
ensure all previous notifications are marked as read- as we should not have duplicate notifications for a given reminder.

app/models/notification.rb

@@ -51,7 +51,8 @@ class Notification < ApplicationRecord
   belongs_to :journal
   belongs_to :resource, polymorphic: true
 
-  has_one :reminder, dependent: :nullify
+  has_one :reminder_notification, dependent: :destroy
+  has_one :reminder, through: :reminder_notification
 
   include Scopes::Scoped
   scopes :unsent_reminders_before,

app/models/reminder.rb

@@ -29,8 +29,15 @@
 class Reminder < ApplicationRecord
   belongs_to :remindable, polymorphic: true
   belongs_to :creator, class_name: "User"
-  belongs_to :notification, optional: true
 
-  def notified?  = notification_id.present?
-  def scheduled? = job_id.present?
+  has_many :reminder_notifications, dependent: :destroy
+  has_many :notifications, through: :reminder_notifications
+
+  def unread_notifications?
+    notifications.exists?(read_ian: [false, nil])
+  end
+
+  def scheduled?
+    job_id.present?
+  end
 end

app/models/reminder_notification.rb

@@ -0,0 +1,4 @@
+class ReminderNotification < ApplicationRecord
+  belongs_to :reminder
+  belongs_to :notification
+end

app/workers/reminders/schedule_reminder_job.rb

@@ -35,12 +35,12 @@ def self.schedule(reminder)
     end
 
     def perform(reminder)
-      return if reminder.notified?
+      return if reminder.unread_notifications?
 
       create_notification_service = create_notification_from_reminder(reminder)
 
       create_notification_service.on_success do |service_result|
-        reminder.update_columns(notification_id: service_result.result.id)
+        ReminderNotification.create!(reminder:, notification: service_result.result)
       end
 
       create_notification_service.on_failure do |service_result|

db/migrate/20241119131205_create_reminders.rb

@@ -3,17 +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.string :job_id
-      t.string :note
+      t.text :note
 
       t.timestamps
     end
-
-    add_index :reminders, :notification_id,
-              unique: true,
-              where: "notification_id IS NOT NULL",
-              name: "index_reminders_on_notification_id_unique"
   end
 end

db/migrate/20241121113638_create_reminder_notifications.rb

@@ -0,0 +1,14 @@
+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

spec/factories/reminders_factory.rb

@@ -37,9 +37,21 @@
       job_id { SecureRandom.uuid }
     end
 
-    trait :notified do
-      job_id { SecureRandom.uuid }
-      notification factory: :notification
+    trait :with_unread_notifications do
+      after(:create) do |reminder|
+        create(:reminder_notification, reminder: reminder, notification: create(:notification, read_ian: false))
+      end
+    end
+
+    trait :with_read_notifications do
+      after(:create) do |reminder|
+        create(:reminder_notification, reminder: reminder, notification: create(:notification, read_ian: true))
+      end
     end
   end
+
+  factory :reminder_notification do
+    reminder
+    notification
+  end
 end

spec/models/notification_spec.rb

@@ -34,7 +34,8 @@
     it { is_expected.to belong_to(:actor).class_name("User") }
     it { is_expected.to belong_to(:recipient).class_name("User") }
 
-    it { is_expected.to have_one(:reminder).dependent(:nullify) }
+    it { is_expected.to have_one(:reminder_notification).dependent(:destroy) }
+    it { is_expected.to have_one(:reminder).through(:reminder_notification) }
   end
 
   describe "Enums" do

spec/models/reminder_notification_spec.rb

@@ -0,0 +1,36 @@
+#-- 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

spec/models/reminder_spec.rb

@@ -32,24 +32,27 @@
   describe "Associations" do
     it { is_expected.to belong_to(:remindable) }
     it { is_expected.to belong_to(:creator).class_name("User") }
-    it { is_expected.to belong_to(:notification).optional }
+    it { is_expected.to have_many(:reminder_notifications).dependent(:destroy) }
+    it { is_expected.to have_many(:notifications).through(:reminder_notifications) }
   end
 
-  describe "DB Indexes" do
-    it { is_expected.to have_db_index(:notification_id).unique(true) }
-  end
+  describe "#unread_notifications?" do
+    context "with an unread notification" do
+      subject { create(:reminder, :with_unread_notifications) }
+
+      it { is_expected.to be_an_unread_notification }
+    end
 
-  describe "#notified?" do
-    it "returns true if notification_id is present" do
-      reminder = build_stubbed(:reminder, :notified)
+    context "with no unread notifications" do
+      subject { create(:reminder, :with_read_notifications) }
 
-      expect(reminder).to be_notified
+      it { is_expected.not_to be_an_unread_notification }
     end
 
-    it "returns false if notification_id is not present" do
-      reminder = build(:reminder, notification_id: nil)
+    context "with no notifications" do
+      subject { build(:reminder, notifications: []) }
 
-      expect(reminder).not_to be_notified
+      it { is_expected.not_to be_an_unread_notification }
     end
   end
 

spec/services/reminders/create_service_spec.rb

@@ -36,7 +36,7 @@
     let(:factory) { :reminder }
 
     before do
-      allow(model_instance).to receive(:update_columns)
+      allow(model_instance).to receive(:update_columns).and_return(true)
       allow(Reminders::ScheduleReminderJob).to receive(:schedule)
         .with(model_instance)
         .and_return(instance_double(Reminders::ScheduleReminderJob, job_id: 1))

spec/workers/reminders/schedule_reminder_job_spec.rb

@@ -49,32 +49,28 @@
 
     it "creates a notification from the reminder" do
       notification_svc = nil
-      expect { notification_svc = subject }.to change(Notification, :count).by(1)
-
-      notification = notification_svc.result
+      expect { notification_svc = subject }.to change(Notification, :count).by(1) & change(ReminderNotification, :count).by(1)
 
       aggregate_failures "notification attributes" do
+        notification = notification_svc.result
+
         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).to eq(notification)
+      aggregate_failures "marks reminder as having unread notifications" do
+        expect(reminder.reload).to be_an_unread_notification
       end
     end
 
     context "when the reminder is already notified" do
-      let(:reminder) { build_stubbed(:reminder, :notified) }
-
       before do
-        allow(Notifications::CreateService).to receive(:new).and_call_original
+        create(:reminder_notification, reminder: reminder, notification: create(:notification, read_ian: false))
       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