[39177] Fix double mentioned notifications

By causing aggregated journals to update mentioned notifications before
the predecessor journal is destroyed, we can keep information about a
mention in the notification realm and skip creating another notification
for the same mention.

https://community.openproject.org/wp/39177

app/services/journals/create_service.rb

@@ -70,12 +70,11 @@ module Journals
       predecessor = journal.previous
 
       if aggregatable?(predecessor, journal)
+        notify_aggregation_destruction(predecessor, journal)
+
         predecessor.destroy
-        if predecessor.notes.present?
-          journal.update_columns(notes: predecessor.notes, version: predecessor.version)
-        else
-          journal.update_columns(version: predecessor.version)
-        end
+
+        take_over_journal_details(predecessor, journal)
       end
     end
 
@@ -493,5 +492,19 @@ module Journals
         predecessor.user_id == journal.user_id &&
         (predecessor.notes.empty? || journal.notes.empty?)
     end
+
+    def notify_aggregation_destruction(predecessor, journal)
+      OpenProject::Notifications.send(OpenProject::Events::JOURNAL_AGGREGATE_BEFORE_DESTROY,
+                                      journal: journal,
+                                      predecessor: predecessor)
+    end
+
+    def take_over_journal_details(predecessor, journal)
+      if predecessor.notes.present?
+        journal.update_columns(notes: predecessor.notes, version: predecessor.version)
+      else
+        journal.update_columns(version: predecessor.version)
+      end
+    end
   end
 end

app/services/notifications/aggregated_journal_service.rb

@@ -0,0 +1,41 @@
+#-- 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.
+#++
+
+class Notifications::AggregatedJournalService
+  ##
+  # Move existing notifications for aggregated events over
+  # if they have an immediate response associated
+  def self.relocate_immediate(journal:, predecessor:)
+    Notification
+      .where(journal_id: predecessor.id)
+      .where(reason: :mentioned)
+      .update_all(journal_id: journal.id, read_ian: false)
+  end
+end

app/services/notifications/create_from_model_service.rb

@@ -95,6 +95,8 @@ class Notifications::CreateFromModelService
   end
 
   def settings_of_mentioned
+    return NotificationSetting.none if has_aggregated_notification?(journal)
+
     project_applicable_settings(mentioned_ids,
                                 project,
                                 NotificationSetting::MENTIONED)
@@ -259,6 +261,14 @@ class Notifications::CreateFromModelService
     receivers.delete(user_with_fallback.id)
   end
 
+  ##
+  # If the journal has any mentioned notification
+  # then the mention was aggregated to a new journal
+  # by +Notifications::AggregatedJournalService+
+  def has_aggregated_notification?(journal)
+    Notification.exists?(journal_id: journal.id, reason: :mentioned)
+  end
+
   def receivers_hash
     Hash.new do |hash, user|
       hash[user] = []

config/initializers/subscribe_listeners.rb

@@ -42,6 +42,10 @@ OpenProject::Notifications.subscribe(OpenProject::Events::JOURNAL_CREATED) do |p
   Journals::CompletedJob.schedule(payload[:journal], payload[:send_notification])
 end
 
+OpenProject::Notifications.subscribe(OpenProject::Events::JOURNAL_AGGREGATE_BEFORE_DESTROY) do |payload|
+  Notifications::AggregatedJournalService.relocate_immediate(**payload.slice(:journal, :predecessor))
+end
+
 OpenProject::Notifications.subscribe(OpenProject::Events::WATCHER_ADDED) do |payload|
   next unless payload[:send_notifications]
 

lib/open_project/events.rb

@@ -45,6 +45,7 @@ module OpenProject
     ATTACHMENT_CREATED = 'attachment_created'.freeze
 
     JOURNAL_CREATED = 'journal_created'.freeze
+    JOURNAL_AGGREGATE_BEFORE_DESTROY = 'journal_aggregate_before_destroy'.freeze
 
     MEMBER_CREATED = 'member_created'.freeze
     MEMBER_UPDATED = 'member_updated'.freeze

spec/services/notifications/create_from_model_service_work_package_spec.rb

@@ -941,18 +941,34 @@ describe Notifications::CreateFromModelService,
       end
     end
 
-    context 'in the journal notes' do
+    describe 'in the journal notes' do
       let(:journal) { journal_2_with_notes }
       let(:journal_2_with_notes) do
         work_package.add_journal author, note
         work_package.save(validate: false)
         work_package.journals.last
       end
+      let(:note) do
+        <<~NOTE
+          Hello <mention class="mention" data-type="user" data-id="#{recipient.id}" data-text="@#{recipient.name}">@#{recipient.name}</mention>
+        NOTE
+      end
 
       it_behaves_like 'mentioned'
+
+      context 'when there is a notification for mentioned on the journal' do
+        let!(:mentioned_notification) do
+          FactoryBot.create :notification,
+                            journal: journal_2_with_notes,
+                            resource: journal_2_with_notes.journable,
+                            reason: :mentioned
+        end
+
+        it_behaves_like 'creates no notification'
+      end
     end
 
-    context 'in the description' do
+    describe 'in the description' do
       let(:journal) { journal_2_with_description }
       let(:journal_2_with_description) do
         work_package.description = note
@@ -963,7 +979,7 @@ describe Notifications::CreateFromModelService,
       it_behaves_like 'mentioned'
     end
 
-    context 'in the subject' do
+    describe 'in the subject' do
       let(:journal) { journal_2_with_subject }
       let(:journal_2_with_subject) do
         work_package.subject = note

spec/services/notifications/mail_service_mentioned_integration_spec.rb

@@ -0,0 +1,150 @@
+#-- 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 Notifications::MailService, 'Mentioned integration', type: :model do
+  let(:project) { FactoryBot.create :project }
+
+  let(:work_package) do
+    FactoryBot.create(:work_package, project: project).tap do |wp|
+      # Clear the initial journal job
+      wp.save!
+      clear_enqueued_jobs
+    end
+  end
+
+  let(:role) do
+    FactoryBot.create :role, permissions: %w[view_work_packages edit_work_packages]
+  end
+
+  let(:recipient) do
+    FactoryBot.create :user,
+                      preferences: {
+                        immediate_reminders: {
+                          mentioned: true
+                        }
+                      },
+                      notification_settings: [
+                        FactoryBot.build(:notification_setting,
+                                         mentioned: true,
+                                         involved: true)
+                      ],
+                      member_in_project: project,
+                      member_through_role: role
+  end
+  let(:actor) do
+    FactoryBot.create :user,
+                      member_in_project: project,
+                      member_through_role: role
+  end
+
+  let(:comment) do
+    <<~NOTE
+      Hello <mention class="mention" data-type="user" data-id="#{recipient.id}" data-text="@#{recipient.name}">@#{recipient.name}</mention>
+    NOTE
+  end
+
+  let(:mentioned_notification) do
+    Notification.find_by(recipient: recipient, journal: work_package.journals.last, reason: :mentioned)
+  end
+
+  let(:assigned_notification) do
+    Notification.find_by(recipient: recipient, journal: work_package.journals.last, reason: :assigned)
+  end
+
+  def trigger_comment!
+    User.execute_as(actor) do
+      work_package.journal_notes = comment
+      work_package.save!
+    end
+
+    perform_enqueued_jobs
+    work_package.reload
+  end
+
+  def update_assignee!
+    clear_enqueued_jobs
+
+    User.execute_as(actor) do
+      work_package.assigned_to = recipient
+      work_package.save!
+    end
+
+    perform_enqueued_jobs
+    work_package.reload
+  end
+
+  def expect_mentioned_notification
+    expect(mentioned_notification).to be_present
+    expect(mentioned_notification.reason).to eq 'mentioned'
+    expect(mentioned_notification.read_ian).to eq false
+    expect(mentioned_notification.mail_alert_sent).to eq true
+  end
+
+  def expect_mentioned_notification_updated
+    old_journal_id = mentioned_notification.journal_id
+    mentioned_notification.reload
+    expect(mentioned_notification.journal_id).not_to eq old_journal_id
+    expect(mentioned_notification.journal).to eq work_package.journals.last
+    expect(mentioned_notification.reason).to eq 'mentioned'
+    expect(mentioned_notification.read_ian).to eq false
+    expect(mentioned_notification.mail_alert_sent).to eq true
+  end
+
+  def expect_assigned_notification
+    expect(assigned_notification).to be_present
+    expect(assigned_notification.read_ian).to eq false
+    expect(assigned_notification.mail_alert_sent).to eq false
+  end
+
+  it 'will trigger only one mention notification mail when editing attributes afterwards' do
+    allow(WorkPackageMailer)
+      .to(receive(:mentioned))
+      .and_call_original
+
+    trigger_comment!
+
+    expect(WorkPackageMailer)
+      .to have_received(:mentioned)
+      .with(recipient, work_package.journals.last)
+
+    expect_mentioned_notification
+
+    update_assignee!
+
+    expect(WorkPackageMailer)
+      .not_to have_received(:mentioned)
+      .with(recipient, work_package.journals.last)
+
+    expect_mentioned_notification_updated
+    expect_assigned_notification
+  end
+end