From 35a437a03f4e1f606afea8953f0be62807da91cc Mon Sep 17 00:00:00 2001 From: David Roetzel Date: Fri, 12 Jul 2024 14:09:52 +0200 Subject: [PATCH 1/4] 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) } From b87c41115e99becdfc8a6a66300b4fc6fe39bf3b Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Fri, 12 Jul 2024 14:20:32 +0200 Subject: [PATCH 2/4] chore(deps): update dependency rubocop-rspec to v3.0.3 (#31009) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 5c673a6282..98a3571731 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -756,7 +756,7 @@ GEM rack (>= 1.1) rubocop (>= 1.33.0, < 2.0) rubocop-ast (>= 1.31.1, < 2.0) - rubocop-rspec (3.0.2) + rubocop-rspec (3.0.3) rubocop (~> 1.61) rubocop-rspec_rails (2.30.0) rubocop (~> 1.61) From c953dca1dee71f32b547e68977568cd076a6f7ae Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Fri, 12 Jul 2024 14:23:09 +0200 Subject: [PATCH 3/4] Streaming: use pgPool.query instead of manually acquiring & releasing a connection (#30964) --- streaming/index.js | 77 +++++++++++++++------------------------------- 1 file changed, 24 insertions(+), 53 deletions(-) diff --git a/streaming/index.js b/streaming/index.js index 2ba8b15407..dd9ea0c7f6 100644 --- a/streaming/index.js +++ b/streaming/index.js @@ -524,43 +524,27 @@ const startServer = async () => { * @param {any} req * @returns {Promise} */ - const accountFromToken = (token, req) => new Promise((resolve, reject) => { - pgPool.connect((err, client, done) => { - if (err) { - reject(err); - return; - } + const accountFromToken = async (token, req) => { + const result = await pgPool.query('SELECT oauth_access_tokens.id, oauth_access_tokens.resource_owner_id, users.account_id, users.chosen_languages, oauth_access_tokens.scopes, devices.device_id FROM oauth_access_tokens INNER JOIN users ON oauth_access_tokens.resource_owner_id = users.id LEFT OUTER JOIN devices ON oauth_access_tokens.id = devices.access_token_id WHERE oauth_access_tokens.token = $1 AND oauth_access_tokens.revoked_at IS NULL LIMIT 1', [token]); - // @ts-ignore - client.query('SELECT oauth_access_tokens.id, oauth_access_tokens.resource_owner_id, users.account_id, users.chosen_languages, oauth_access_tokens.scopes, devices.device_id FROM oauth_access_tokens INNER JOIN users ON oauth_access_tokens.resource_owner_id = users.id LEFT OUTER JOIN devices ON oauth_access_tokens.id = devices.access_token_id WHERE oauth_access_tokens.token = $1 AND oauth_access_tokens.revoked_at IS NULL LIMIT 1', [token], (err, result) => { - done(); + if (result.rows.length === 0) { + throw new AuthenticationError('Invalid access token'); + } - if (err) { - reject(err); - return; - } + req.accessTokenId = result.rows[0].id; + req.scopes = result.rows[0].scopes.split(' '); + req.accountId = result.rows[0].account_id; + req.chosenLanguages = result.rows[0].chosen_languages; + req.deviceId = result.rows[0].device_id; - if (result.rows.length === 0) { - reject(new AuthenticationError('Invalid access token')); - return; - } - - req.accessTokenId = result.rows[0].id; - req.scopes = result.rows[0].scopes.split(' '); - req.accountId = result.rows[0].account_id; - req.chosenLanguages = result.rows[0].chosen_languages; - req.deviceId = result.rows[0].device_id; - - resolve({ - accessTokenId: result.rows[0].id, - scopes: result.rows[0].scopes.split(' '), - accountId: result.rows[0].account_id, - chosenLanguages: result.rows[0].chosen_languages, - deviceId: result.rows[0].device_id - }); - }); - }); - }); + return { + accessTokenId: result.rows[0].id, + scopes: result.rows[0].scopes.split(' '), + accountId: result.rows[0].account_id, + chosenLanguages: result.rows[0].chosen_languages, + deviceId: result.rows[0].device_id + }; + }; /** * @param {any} req @@ -771,28 +755,15 @@ const startServer = async () => { * @param {any} req * @returns {Promise.} */ - const authorizeListAccess = (listId, req) => new Promise((resolve, reject) => { + const authorizeListAccess = async (listId, req) => { const { accountId } = req; - pgPool.connect((err, client, done) => { - if (err) { - reject(); - return; - } + const result = await pgPool.query('SELECT id, account_id FROM lists WHERE id = $1 AND account_id = $2 LIMIT 1', [listId, accountId]); - // @ts-ignore - client.query('SELECT id, account_id FROM lists WHERE id = $1 LIMIT 1', [listId], (err, result) => { - done(); - - if (err || result.rows.length === 0 || result.rows[0].account_id !== accountId) { - reject(); - return; - } - - resolve(); - }); - }); - }); + if (result.rows.length === 0) { + throw new AuthenticationError('List not found'); + } + }; /** * @param {string[]} channelIds From 00cb4a0313190bfa118966692a649db9c8328094 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Fri, 12 Jul 2024 10:09:16 -0400 Subject: [PATCH 4/4] Avoid repeated factory creation in media_attachments_vacuum_spec (#31000) --- .../vacuum/media_attachments_vacuum_spec.rb | 39 +++++++------------ 1 file changed, 14 insertions(+), 25 deletions(-) diff --git a/spec/lib/vacuum/media_attachments_vacuum_spec.rb b/spec/lib/vacuum/media_attachments_vacuum_spec.rb index 3c17ecb000..1039c36cea 100644 --- a/spec/lib/vacuum/media_attachments_vacuum_spec.rb +++ b/spec/lib/vacuum/media_attachments_vacuum_spec.rb @@ -17,32 +17,21 @@ RSpec.describe Vacuum::MediaAttachmentsVacuum do let!(:old_unattached_media) { Fabricate(:media_attachment, account_id: nil, created_at: 10.days.ago) } let!(:new_unattached_media) { Fabricate(:media_attachment, account_id: nil, created_at: 1.hour.ago) } - before do - subject.perform - end + before { subject.perform } - it 'deletes cache of remote media attachments past the retention period' do - expect(old_remote_media.reload.file).to be_blank - end - - it 'does not touch local media attachments past the retention period' do - expect(old_local_media.reload.file).to_not be_blank - end - - it 'does not delete cache of remote media attachments within the retention period' do - expect(new_remote_media.reload.file).to_not be_blank - end - - it 'does not touch local media attachments within the retention period' do - expect(new_local_media.reload.file).to_not be_blank - end - - it 'deletes unattached media attachments past TTL' do - expect { old_unattached_media.reload }.to raise_error(ActiveRecord::RecordNotFound) - end - - it 'does not delete unattached media attachments within TTL' do - expect(new_unattached_media.reload).to be_persisted + it 'handles attachments based on metadata details' do + expect(old_remote_media.reload.file) # Remote and past retention period + .to be_blank + expect(old_local_media.reload.file) # Local and past retention + .to_not be_blank + expect(new_remote_media.reload.file) # Remote and within retention + .to_not be_blank + expect(new_local_media.reload.file) # Local and within retention + .to_not be_blank + expect { old_unattached_media.reload } # Unattached and past TTL + .to raise_error(ActiveRecord::RecordNotFound) + expect(new_unattached_media.reload) # Unattached and within TTL + .to be_persisted end end end