Do not dismiss notification requests for ever.fixes/dismissing-notification-requests-dismisses-too-much

"Dismiss" now only applies to a single notification request.
If the same sender mentions the recipient again, a new
notification request is created.

4 files changed, 58 insertions, 3 deletions

diff --git a/app/services/notify_service.rb b/app/services/notify_service.rb
index d69b5af1412..db0da271feb 100644
--- a/app/services/notify_service.rb
+++ b/app/services/notify_service.rb
@@ -235,7 +235,7 @@ class NotifyService < BaseService
   def update_notification_request!
     return unless @notification.type == :mention
 
-    notification_request = NotificationRequest.find_or_initialize_by(account_id: @recipient.id, from_account_id: @notification.from_account_id)
+    notification_request = NotificationRequest.find_or_initialize_by(account_id: @recipient.id, from_account_id: @notification.from_account_id, dismissed: false)
     notification_request.last_status_id = @notification.target_status.id
     notification_request.save
   end
diff --git a/db/migrate/20240710153313_change_unique_index_on_notification_requests.rb b/db/migrate/20240710153313_change_unique_index_on_notification_requests.rb
new file mode 100644
index 00000000000..33ec62efee3
--- /dev/null
+++ b/db/migrate/20240710153313_change_unique_index_on_notification_requests.rb
@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+class ChangeUniqueIndexOnNotificationRequests < ActiveRecord::Migration[7.1]
+  disable_ddl_transaction!
+
+  def change
+    remove_index :notification_requests, [:account_id, :from_account_id], unique: true
+    add_index :notification_requests, [:account_id, :from_account_id, :last_status_id], unique: true, algorithm: :concurrently
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 5f8c7e693f4..79d16b13804 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.1].define(version: 2024_06_07_094856) do
+ActiveRecord::Schema[7.1].define(version: 2024_07_10_153313) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -709,7 +709,7 @@ ActiveRecord::Schema[7.1].define(version: 2024_06_07_094856) do
     t.boolean "dismissed", default: false, null: false
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
-    t.index ["account_id", "from_account_id"], name: "index_notification_requests_on_account_id_and_from_account_id", unique: true
+    t.index ["account_id", "from_account_id", "last_status_id"], name: "idx_on_account_id_from_account_id_last_status_id_bbe7648aec", unique: true
     t.index ["account_id", "id"], name: "index_notification_requests_on_account_id_and_id", order: { id: :desc }, where: "(dismissed = false)"
     t.index ["from_account_id"], name: "index_notification_requests_on_from_account_id"
     t.index ["last_status_id"], name: "index_notification_requests_on_last_status_id"
diff --git a/spec/services/notify_service_spec.rb b/spec/services/notify_service_spec.rb
index c695855bec0..2ac3990ce32 100644
--- a/spec/services/notify_service_spec.rb
+++ b/spec/services/notify_service_spec.rb
@@ -129,6 +129,51 @@ RSpec.describe NotifyService do
     end
   end
 
+  context 'with filtered notifications' do
+    let(:unknown)  { Fabricate(:account, username: 'unknown') }
+    let(:status)   { Fabricate(:status, account: unknown) }
+    let(:activity) { Fabricate(:mention, account: recipient, status: status) }
+    let(:type)     { :mention }
+
+    before do
+      Fabricate(:notification_policy, account: recipient, filter_not_following: true)
+    end
+
+    it 'creates a filtered notification' do
+      expect { subject }.to change(Notification, :count)
+      expect(Notification.last).to be_filtered
+    end
+
+    context 'when no notification request exists' do
+      it 'creates a notification request' do
+        expect { subject }.to change(NotificationRequest, :count)
+      end
+    end
+
+    context 'when a notification request exists' do
+      let!(:notification_request) do
+        Fabricate(:notification_request, account: recipient, from_account: unknown, last_status: Fabricate(:status, account: unknown), dismissed: dismissed)
+      end
+
+      context 'when it is not dismissed' do
+        let(:dismissed) { false }
+
+        it 'updates the existing notification request' do
+          expect { subject }.to_not change(NotificationRequest, :count)
+          expect(notification_request.reload.last_status).to eq status
+        end
+      end
+
+      context 'when it is dismissed' do
+        let(:dismissed) { true }
+
+        it 'creates a new notification request' do
+          expect { subject }.to change(NotificationRequest, :count)
+        end
+      end
+    end
+  end
+
   describe NotifyService::DismissCondition do
     subject { described_class.new(notification) }