From 35a437a03f4e1f606afea8953f0be62807da91cc Mon Sep 17 00:00:00 2001 From: David Roetzel Date: Fri, 12 Jul 2024 14:09:52 +0200 Subject: [PATCH] Destroy `NotificationRequest`s that are dismissed (#31008) --- .../v1/notifications/requests_controller.rb | 8 ++--- app/models/notification_policy.rb | 2 +- app/models/notification_request.rb | 5 ++- ...ve_dismissed_from_notification_requests.rb | 14 ++++++++ db/schema.rb | 4 +-- .../notification_request_fabricator.rb | 1 - spec/models/notification_request_spec.rb | 12 +------ .../api/v1/notifications/requests_spec.rb | 18 ++-------- spec/services/notify_service_spec.rb | 33 +++++++++++++++++++ 9 files changed, 57 insertions(+), 40 deletions(-) create mode 100644 db/post_migrate/20240712064044_remove_dismissed_from_notification_requests.rb diff --git a/app/controllers/api/v1/notifications/requests_controller.rb b/app/controllers/api/v1/notifications/requests_controller.rb index 0e58379a38..9ae80c28ed 100644 --- a/app/controllers/api/v1/notifications/requests_controller.rb +++ b/app/controllers/api/v1/notifications/requests_controller.rb @@ -28,14 +28,14 @@ class Api::V1::Notifications::RequestsController < Api::BaseController end def dismiss - @request.update!(dismissed: true) + @request.destroy! render_empty end private def load_requests - requests = NotificationRequest.where(account: current_account).where(dismissed: truthy_param?(:dismissed) || false).includes(:last_status, from_account: [:account_stat, :user]).to_a_paginated_by_id( + requests = NotificationRequest.where(account: current_account).includes(:last_status, from_account: [:account_stat, :user]).to_a_paginated_by_id( limit_param(DEFAULT_ACCOUNTS_LIMIT), params_slice(:max_id, :since_id, :min_id) ) @@ -68,8 +68,4 @@ class Api::V1::Notifications::RequestsController < Api::BaseController def pagination_since_id @requests.first.id end - - def pagination_params(core_params) - params.slice(:dismissed).permit(:dismissed).merge(core_params) - end end diff --git a/app/models/notification_policy.rb b/app/models/notification_policy.rb index f10b0c2a81..2bb58004e3 100644 --- a/app/models/notification_policy.rb +++ b/app/models/notification_policy.rb @@ -31,6 +31,6 @@ class NotificationPolicy < ApplicationRecord private def pending_notification_requests - @pending_notification_requests ||= notification_requests.where(dismissed: false).limit(MAX_MEANINGFUL_COUNT).pick(Arel.sql('count(*), coalesce(sum(notifications_count), 0)::bigint')) + @pending_notification_requests ||= notification_requests.limit(MAX_MEANINGFUL_COUNT).pick(Arel.sql('count(*), coalesce(sum(notifications_count), 0)::bigint')) end end diff --git a/app/models/notification_request.rb b/app/models/notification_request.rb index 6e9cae6625..2f601ac36b 100644 --- a/app/models/notification_request.rb +++ b/app/models/notification_request.rb @@ -9,12 +9,13 @@ # from_account_id :bigint(8) not null # last_status_id :bigint(8) # notifications_count :bigint(8) default(0), not null -# dismissed :boolean default(FALSE), not null # created_at :datetime not null # updated_at :datetime not null # class NotificationRequest < ApplicationRecord + self.ignored_columns += %w(dismissed) + include Paginable MAX_MEANINGFUL_COUNT = 100 @@ -34,8 +35,6 @@ class NotificationRequest < ApplicationRecord end def reconsider_existence! - return if dismissed? - prepare_notifications_count if notifications_count.positive? diff --git a/db/post_migrate/20240712064044_remove_dismissed_from_notification_requests.rb b/db/post_migrate/20240712064044_remove_dismissed_from_notification_requests.rb new file mode 100644 index 0000000000..0d85838073 --- /dev/null +++ b/db/post_migrate/20240712064044_remove_dismissed_from_notification_requests.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class RemoveDismissedFromNotificationRequests < ActiveRecord::Migration[7.1] + def up + safety_assured do + execute 'DELETE FROM notification_requests WHERE dismissed' + remove_column :notification_requests, :dismissed + end + end + + def down + add_column :notification_requests, :dismissed, :boolean, default: false, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 5f8c7e693f..87c49d7415 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_12_064044) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -706,11 +706,9 @@ ActiveRecord::Schema[7.1].define(version: 2024_06_07_094856) do t.bigint "from_account_id", null: false t.bigint "last_status_id" t.bigint "notifications_count", default: 0, null: false - 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", "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" end diff --git a/spec/fabricators/notification_request_fabricator.rb b/spec/fabricators/notification_request_fabricator.rb index 05a13b8ef8..a20d3b3ef2 100644 --- a/spec/fabricators/notification_request_fabricator.rb +++ b/spec/fabricators/notification_request_fabricator.rb @@ -4,5 +4,4 @@ Fabricator(:notification_request) do account from_account { Fabricate.build(:account) } last_status { Fabricate.build(:status) } - dismissed false end diff --git a/spec/models/notification_request_spec.rb b/spec/models/notification_request_spec.rb index 07bbc3e0a8..4adddc194f 100644 --- a/spec/models/notification_request_spec.rb +++ b/spec/models/notification_request_spec.rb @@ -4,9 +4,7 @@ require 'rails_helper' RSpec.describe NotificationRequest do describe '#reconsider_existence!' do - subject { Fabricate(:notification_request, dismissed: dismissed) } - - let(:dismissed) { false } + subject { Fabricate(:notification_request) } context 'when there are remaining notifications' do before do @@ -28,14 +26,6 @@ RSpec.describe NotificationRequest do subject.reconsider_existence! end - context 'when dismissed' do - let(:dismissed) { true } - - it 'leaves request intact' do - expect(subject.destroyed?).to be false - end - end - it 'removes the request' do expect(subject.destroyed?).to be true end diff --git a/spec/requests/api/v1/notifications/requests_spec.rb b/spec/requests/api/v1/notifications/requests_spec.rb index 23ddfd2bda..d3a9753246 100644 --- a/spec/requests/api/v1/notifications/requests_spec.rb +++ b/spec/requests/api/v1/notifications/requests_spec.rb @@ -17,7 +17,6 @@ RSpec.describe 'Requests' do before do Fabricate(:notification_request, account: user.account) - Fabricate(:notification_request, account: user.account, dismissed: true) end it_behaves_like 'forbidden for wrong scope', 'write write:notifications' @@ -29,16 +28,6 @@ RSpec.describe 'Requests' do expect(response).to have_http_status(200) end end - - context 'with dismissed' do - let(:params) { { dismissed: '1' } } - - it 'returns http success', :aggregate_failures do - subject - - expect(response).to have_http_status(200) - end - end end describe 'POST /api/v1/notifications/requests/:id/accept' do @@ -78,15 +67,14 @@ RSpec.describe 'Requests' do post "/api/v1/notifications/requests/#{notification_request.id}/dismiss", headers: headers end - let(:notification_request) { Fabricate(:notification_request, account: user.account) } + let!(:notification_request) { Fabricate(:notification_request, account: user.account) } it_behaves_like 'forbidden for wrong scope', 'read read:notifications' - it 'returns http success and dismisses the notification request', :aggregate_failures do - subject + it 'returns http success and destroys the notification request', :aggregate_failures do + expect { subject }.to change(NotificationRequest, :count).by(-1) expect(response).to have_http_status(200) - expect(notification_request.reload.dismissed?).to be true end context 'when notification request belongs to someone else' do diff --git a/spec/services/notify_service_spec.rb b/spec/services/notify_service_spec.rb index c695855bec..c7e00129b2 100644 --- a/spec/services/notify_service_spec.rb +++ b/spec/services/notify_service_spec.rb @@ -129,6 +129,39 @@ 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)) + end + + 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 + end + describe NotifyService::DismissCondition do subject { described_class.new(notification) }