ensure notifications are linked to existing journals

There can be a race condition when a user continues to alter a work package or other resource after a first change has been made resulting in a journal and the background process creating a notification for that journal. If the second journal caused by the users continued interaction removes the first journal after the job for creating a notification has already started, all checks within ruby are blind. Therefore a foreign key constraint is needed.

app/services/notifications/create_service.rb

@@ -29,4 +29,13 @@
 #++
 
 class Notifications::CreateService < ::BaseServices::Create
+  protected
+
+  def persist(service_result)
+    super
+  rescue ActiveRecord::InvalidForeignKey
+    service_result.success = false
+    service_result.errors.add(:journal_id, :does_not_exist)
+    service_result
+  end
 end

db/migrate/20210922123908_notification_foreign_key_constraint.rb

@@ -0,0 +1,6 @@
+class NotificationForeignKeyConstraint < ActiveRecord::Migration[6.1]
+  def change
+    add_foreign_key :notifications, :journals
+    add_index :notifications, :journal_id
+  end
+end

spec/models/notification_spec.rb

@@ -0,0 +1,44 @@
+#-- encoding: UTF-8
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2021 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'
+
+describe Notification,
+         type: :model do
+  describe '.save' do
+    context 'for a non existing journal (e.g. because it has been deleted)' do
+      let(:notification) { FactoryBot.build(:notification, journal_id: 55) }
+
+      it 'raises an error' do
+        expect { notification.save }
+          .to raise_error ActiveRecord::InvalidForeignKey
+      end
+    end
+  end
+end

spec/services/notifications/create_service_intergration_spec.rb

@@ -0,0 +1,89 @@
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2021 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'
+
+describe Notifications::CreateService, 'integration', type: :model do
+  let(:work_package) { FactoryBot.create(:work_package) }
+  let(:project) { work_package.project }
+  let(:journal) { work_package.journals.first }
+  let(:instance) { described_class.new(user: actor) }
+  let(:attributes) { {} }
+  let(:actor) { current_user }
+  let(:recipient) { FactoryBot.create(:user) }
+  let(:service_result) do
+    instance
+      .call(**attributes)
+  end
+
+  current_user { FactoryBot.create(:user) }
+
+  describe '#call' do
+    let(:attributes) do
+      {
+        recipient: recipient,
+        project: project,
+        resource: work_package,
+        journal: journal,
+        actor: actor,
+        read_ian: false,
+        reason_ian: :mentioned,
+        read_mail: nil,
+        read_mail_digest: nil
+      }
+    end
+
+    it 'creates a notification' do
+      # successful
+      expect { service_result }
+        .to change(Notification, :count)
+              .by(1)
+
+      expect(service_result)
+        .to be_success
+    end
+
+    context 'with the journal being deleted in the meantime (e.g. via a different process)' do
+      before do
+        Journal.where(id: journal.id).delete_all
+      end
+
+      it 'creates no notification' do
+        # successful
+        expect { service_result }
+          .not_to change(Notification, :count)
+
+        expect(service_result)
+          .to be_failure
+
+        expect(service_result.errors.details[:journal_id])
+          .to match_array [{ error: :does_not_exist }]
+      end
+    end
+  end
+end