From 9734c9b6fe2d2e4a980d8ad94c5de0300b23c809 Mon Sep 17 00:00:00 2001 From: ThibG Date: Sun, 20 Dec 2020 18:05:03 +0100 Subject: [PATCH 1/4] Add clean error message when RAILS_ENV is unset (#15381) By default, an unset RAILS_ENV is understood as RAILS_ENV=development. However, this is not what most people expect, and due to development-only dependencies, users are often left with confusing error messages. This commit changes it so that an explicit RAILS_ENV is required, and failing that, an error message is displayed before loading the app. Co-authored-by: Claire --- config/boot.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/config/boot.rb b/config/boot.rb index f3e36203a..6cde5319d 100644 --- a/config/boot.rb +++ b/config/boot.rb @@ -1,3 +1,8 @@ +unless ENV.key?('RAILS_ENV') + STDERR.puts 'ERROR: Missing RAILS_ENV environment variable, please set it to "production", "development", or "test".' + exit 1 +end + ENV['BUNDLE_GEMFILE'] ||= File.expand_path('../Gemfile', __dir__) require 'bundler/setup' # Set up gems listed in the Gemfile. From 7bf3c6e57b52cd9390f2140a1cc17292c281aacf Mon Sep 17 00:00:00 2001 From: ThibG Date: Sun, 20 Dec 2020 18:25:00 +0100 Subject: [PATCH 2/4] Fix AccountDeletionWorker crashing and clogging sidekiq queues (#15380) * Fix account deletion workers being queued multiple times for a single account * Fix poll votes being unnecessarily instantiated on poll deletion * Fix favourites being unnecessarily instantiated on status deletion * Remove inaccurate comments * Delete polls instead of destroying them Co-authored-by: Claire --- app/models/poll.rb | 2 +- app/models/status.rb | 2 +- app/services/batched_remove_status_service.rb | 3 --- app/services/delete_account_service.rb | 4 +++- app/workers/account_deletion_worker.rb | 2 +- 5 files changed, 6 insertions(+), 7 deletions(-) diff --git a/app/models/poll.rb b/app/models/poll.rb index b5deafcc2..e1ca55252 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -25,7 +25,7 @@ class Poll < ApplicationRecord belongs_to :account belongs_to :status - has_many :votes, class_name: 'PollVote', inverse_of: :poll, dependent: :destroy + has_many :votes, class_name: 'PollVote', inverse_of: :poll, dependent: :delete_all has_many :notifications, as: :activity, dependent: :destroy diff --git a/app/models/status.rb b/app/models/status.rb index 96d90e1c2..f1b3b75ce 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -56,7 +56,7 @@ class Status < ApplicationRecord belongs_to :thread, foreign_key: 'in_reply_to_id', class_name: 'Status', inverse_of: :replies, optional: true belongs_to :reblog, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblogs, optional: true - has_many :favourites, inverse_of: :status, dependent: :destroy + has_many :favourites, inverse_of: :status, dependent: :delete_all has_many :bookmarks, inverse_of: :status, dependent: :destroy has_many :reblogs, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblog, dependent: :destroy has_many :replies, foreign_key: 'in_reply_to_id', class_name: 'Status', inverse_of: :thread diff --git a/app/services/batched_remove_status_service.rb b/app/services/batched_remove_status_service.rb index 2295a01dc..28e5468b3 100644 --- a/app/services/batched_remove_status_service.rb +++ b/app/services/batched_remove_status_service.rb @@ -4,8 +4,6 @@ class BatchedRemoveStatusService < BaseService include Redisable # Delete given statuses and reblogs of them - # Dispatch PuSH updates of the deleted statuses, but only local ones - # Dispatch Salmon deletes, unique per domain, of the deleted statuses, but only local ones # Remove statuses from home feeds # Push delete events to streaming API for home feeds and public feeds # @param [Enumerable] statuses A preferably batched array of statuses @@ -19,7 +17,6 @@ class BatchedRemoveStatusService < BaseService @json_payloads = statuses.each_with_object({}) { |s, h| h[s.id] = Oj.dump(event: :delete, payload: s.id.to_s) } - # Ensure that rendered XML reflects destroyed state statuses.each do |status| status.mark_for_mass_destruction! status.destroy diff --git a/app/services/delete_account_service.rb b/app/services/delete_account_service.rb index 9cb80c95a..fe9b30b17 100644 --- a/app/services/delete_account_service.rb +++ b/app/services/delete_account_service.rb @@ -122,7 +122,9 @@ class DeleteAccountService < BaseService @account.polls.reorder(nil).find_each do |poll| next if @options[:reserve_username] && reported_status_ids.include?(poll.status_id) - poll.destroy + # We can safely delete the poll rather than destroy it, as any non-reported + # status should have been deleted already + poll.delete end associations_for_destruction.each do |association_name| diff --git a/app/workers/account_deletion_worker.rb b/app/workers/account_deletion_worker.rb index 98b67419d..fdf013e01 100644 --- a/app/workers/account_deletion_worker.rb +++ b/app/workers/account_deletion_worker.rb @@ -3,7 +3,7 @@ class AccountDeletionWorker include Sidekiq::Worker - sidekiq_options queue: 'pull' + sidekiq_options queue: 'pull', lock: :until_executed def perform(account_id, options = {}) reserve_username = options.with_indifferent_access.fetch(:reserve_username, true) From 6f51fd743590c8fd8dd95e48dc24e4472d46480b Mon Sep 17 00:00:00 2001 From: ThibG Date: Mon, 21 Dec 2020 00:47:02 +0100 Subject: [PATCH 3/4] Fix invitation links not working when invite request text is required (#15385) Fixes #15383 Co-authored-by: Claire --- app/models/user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index 6088f1994..dd96bbf8c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -83,7 +83,7 @@ class User < ApplicationRecord has_one :invite_request, class_name: 'UserInviteRequest', inverse_of: :user, dependent: :destroy accepts_nested_attributes_for :invite_request, reject_if: ->(attributes) { attributes['text'].blank? && !Setting.require_invite_text } - validates :invite_request, presence: true, on: :create, if: -> { Setting.require_invite_text } + validates :invite_request, presence: true, on: :create, if: -> { Setting.require_invite_text && !invited? } validates :locale, inclusion: I18n.available_locales.map(&:to_s), if: :locale? validates_with BlacklistedEmailValidator, on: :create From 43961035a906cd8bccdf4c1ac023980b37823bb3 Mon Sep 17 00:00:00 2001 From: ThibG Date: Mon, 21 Dec 2020 18:22:17 +0100 Subject: [PATCH 4/4] Fix some notifications not being deleted on poll/status deletion (#15402) * Fix deleting polls not deleting notifications * Fix fav notification deletion when deleting a toot * Refactor DeleteAccountService spec * Add DeleteAccountService tests for other associations and notifications * Add favourite handling spec in status removal Co-authored-by: Claire --- app/models/status.rb | 2 +- app/services/delete_account_service.rb | 4 +- spec/services/delete_account_service_spec.rb | 116 ++++++++++--------- spec/services/remove_status_service_spec.rb | 14 ++- 4 files changed, 80 insertions(+), 56 deletions(-) diff --git a/app/models/status.rb b/app/models/status.rb index f1b3b75ce..96d90e1c2 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -56,7 +56,7 @@ class Status < ApplicationRecord belongs_to :thread, foreign_key: 'in_reply_to_id', class_name: 'Status', inverse_of: :replies, optional: true belongs_to :reblog, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblogs, optional: true - has_many :favourites, inverse_of: :status, dependent: :delete_all + has_many :favourites, inverse_of: :status, dependent: :destroy has_many :bookmarks, inverse_of: :status, dependent: :destroy has_many :reblogs, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblog, dependent: :destroy has_many :replies, foreign_key: 'in_reply_to_id', class_name: 'Status', inverse_of: :thread diff --git a/app/services/delete_account_service.rb b/app/services/delete_account_service.rb index fe9b30b17..fa834e775 100644 --- a/app/services/delete_account_service.rb +++ b/app/services/delete_account_service.rb @@ -123,7 +123,9 @@ class DeleteAccountService < BaseService next if @options[:reserve_username] && reported_status_ids.include?(poll.status_id) # We can safely delete the poll rather than destroy it, as any non-reported - # status should have been deleted already + # status should have been deleted already, as long as we take care of + # notifications. + Notification.where(poll: poll).delete_all poll.delete end diff --git a/spec/services/delete_account_service_spec.rb b/spec/services/delete_account_service_spec.rb index d208b25b8..cd7d32d59 100644 --- a/spec/services/delete_account_service_spec.rb +++ b/spec/services/delete_account_service_spec.rb @@ -1,28 +1,31 @@ require 'rails_helper' RSpec.describe DeleteAccountService, type: :service do - describe '#call on local account' do - before do - stub_request(:post, "https://alice.com/inbox").to_return(status: 201) - stub_request(:post, "https://bob.com/inbox").to_return(status: 201) - end + shared_examples 'common behavior' do + let!(:status) { Fabricate(:status, account: account) } + let!(:mention) { Fabricate(:mention, account: local_follower) } + let!(:status_with_mention) { Fabricate(:status, account: account, mentions: [mention]) } + let!(:media_attachment) { Fabricate(:media_attachment, account: account) } + let!(:notification) { Fabricate(:notification, account: account) } + let!(:favourite) { Fabricate(:favourite, account: account, status: Fabricate(:status, account: local_follower)) } + let!(:poll) { Fabricate(:poll, account: account) } + let!(:poll_vote) { Fabricate(:poll_vote, account: local_follower, poll: poll) } + + let!(:active_relationship) { Fabricate(:follow, account: account, target_account: local_follower) } + let!(:passive_relationship) { Fabricate(:follow, account: local_follower, target_account: account) } + let!(:endorsement) { Fabricate(:account_pin, account: local_follower, target_account: account) } + + let!(:mention_notification) { Fabricate(:notification, account: local_follower, activity: mention, type: :mention) } + let!(:status_notification) { Fabricate(:notification, account: local_follower, activity: status, type: :status) } + let!(:poll_notification) { Fabricate(:notification, account: local_follower, activity: poll, type: :poll) } + let!(:favourite_notification) { Fabricate(:notification, account: local_follower, activity: favourite, type: :favourite) } + let!(:follow_notification) { Fabricate(:notification, account: local_follower, activity: active_relationship, type: :follow) } subject do -> { described_class.new.call(account) } end - let!(:account) { Fabricate(:account) } - let!(:status) { Fabricate(:status, account: account) } - let!(:media_attachment) { Fabricate(:media_attachment, account: account) } - let!(:notification) { Fabricate(:notification, account: account) } - let!(:favourite) { Fabricate(:favourite, account: account) } - let!(:active_relationship) { Fabricate(:follow, account: account) } - let!(:passive_relationship) { Fabricate(:follow, target_account: account) } - let!(:remote_alice) { Fabricate(:account, inbox_url: 'https://alice.com/inbox', protocol: :activitypub) } - let!(:remote_bob) { Fabricate(:account, inbox_url: 'https://bob.com/inbox', protocol: :activitypub) } - let!(:endorsment) { Fabricate(:account_pin, account: passive_relationship.account, target_account: account) } - - it 'deletes associated records' do + it 'deletes associated owned records' do is_expected.to change { [ account.statuses, @@ -31,15 +34,46 @@ RSpec.describe DeleteAccountService, type: :service do account.favourites, account.active_relationships, account.passive_relationships, - AccountPin.where(target_account: account), + account.polls, ].map(&:count) - }.from([1, 1, 1, 1, 1, 1, 1]).to([0, 0, 0, 0, 0, 0, 0]) + }.from([2, 1, 1, 1, 1, 1, 1]).to([0, 0, 0, 0, 0, 0, 0]) end - it 'sends a delete actor activity to all known inboxes' do - subject.call - expect(a_request(:post, "https://alice.com/inbox")).to have_been_made.once - expect(a_request(:post, "https://bob.com/inbox")).to have_been_made.once + it 'deletes associated target records' do + is_expected.to change { + [ + AccountPin.where(target_account: account), + ].map(&:count) + }.from([1]).to([0]) + end + + it 'deletes associated target notifications' do + is_expected.to change { + [ + 'poll', 'favourite', 'status', 'mention', 'follow' + ].map { |type| Notification.where(type: type).count } + }.from([1, 1, 1, 1, 1]).to([0, 0, 0, 0, 0]) + end + end + + describe '#call on local account' do + before do + stub_request(:post, "https://alice.com/inbox").to_return(status: 201) + stub_request(:post, "https://bob.com/inbox").to_return(status: 201) + end + + let!(:remote_alice) { Fabricate(:account, inbox_url: 'https://alice.com/inbox', protocol: :activitypub) } + let!(:remote_bob) { Fabricate(:account, inbox_url: 'https://bob.com/inbox', protocol: :activitypub) } + + include_examples 'common behavior' do + let!(:account) { Fabricate(:account) } + let!(:local_follower) { Fabricate(:account) } + + it 'sends a delete actor activity to all known inboxes' do + subject.call + expect(a_request(:post, "https://alice.com/inbox")).to have_been_made.once + expect(a_request(:post, "https://bob.com/inbox")).to have_been_made.once + end end end @@ -49,36 +83,14 @@ RSpec.describe DeleteAccountService, type: :service do stub_request(:post, "https://bob.com/inbox").to_return(status: 201) end - subject do - -> { described_class.new.call(remote_bob) } - end + include_examples 'common behavior' do + let!(:account) { Fabricate(:account, inbox_url: 'https://bob.com/inbox', protocol: :activitypub) } + let!(:local_follower) { Fabricate(:account) } - let!(:account) { Fabricate(:account) } - let!(:remote_alice) { Fabricate(:account, inbox_url: 'https://alice.com/inbox', protocol: :activitypub) } - let!(:remote_bob) { Fabricate(:account, inbox_url: 'https://bob.com/inbox', protocol: :activitypub) } - let!(:status) { Fabricate(:status, account: remote_bob) } - let!(:media_attachment) { Fabricate(:media_attachment, account: remote_bob) } - let!(:notification) { Fabricate(:notification, account: remote_bob) } - let!(:favourite) { Fabricate(:favourite, account: remote_bob) } - let!(:active_relationship) { Fabricate(:follow, account: remote_bob, target_account: account) } - let!(:passive_relationship) { Fabricate(:follow, target_account: remote_bob) } - - it 'deletes associated records' do - is_expected.to change { - [ - remote_bob.statuses, - remote_bob.media_attachments, - remote_bob.notifications, - remote_bob.favourites, - remote_bob.active_relationships, - remote_bob.passive_relationships, - ].map(&:count) - }.from([1, 1, 1, 1, 1, 1]).to([0, 0, 0, 0, 0, 0]) - end - - it 'sends a reject follow to follwer inboxes' do - subject.call - expect(a_request(:post, remote_bob.inbox_url)).to have_been_made.once + it 'sends a reject follow to follwer inboxes' do + subject.call + expect(a_request(:post, account.inbox_url)).to have_been_made.once + end end end end diff --git a/spec/services/remove_status_service_spec.rb b/spec/services/remove_status_service_spec.rb index 06676ec45..7ce75b2c7 100644 --- a/spec/services/remove_status_service_spec.rb +++ b/spec/services/remove_status_service_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe RemoveStatusService, type: :service do subject { RemoveStatusService.new } - let!(:alice) { Fabricate(:account) } + let!(:alice) { Fabricate(:account, user: Fabricate(:user)) } let!(:bob) { Fabricate(:account, username: 'bob', domain: 'example.com', salmon_url: 'http://example.com/salmon') } let!(:jeff) { Fabricate(:account) } let!(:hank) { Fabricate(:account, username: 'hank', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') } @@ -17,23 +17,33 @@ RSpec.describe RemoveStatusService, type: :service do hank.follow!(alice) @status = PostStatusService.new.call(alice, text: 'Hello @bob@example.com') + FavouriteService.new.call(jeff, @status) Fabricate(:status, account: bill, reblog: @status, uri: 'hoge') - subject.call(@status) end it 'removes status from author\'s home feed' do + subject.call(@status) expect(HomeFeed.new(alice).get(10)).to_not include(@status.id) end it 'removes status from local follower\'s home feed' do + subject.call(@status) expect(HomeFeed.new(jeff).get(10)).to_not include(@status.id) end it 'sends delete activity to followers' do + subject.call(@status) expect(a_request(:post, 'http://example.com/inbox')).to have_been_made.twice end it 'sends delete activity to rebloggers' do + subject.call(@status) expect(a_request(:post, 'http://example2.com/inbox')).to have_been_made end + + it 'remove status from notifications' do + expect { subject.call(@status) }.to change { + Notification.where(activity_type: 'Favourite', from_account: jeff, account: alice).count + }.from(1).to(0) + end end