Fix error when suspending user with an already-existing canonical email block (#17036)

* Fix error when suspending user with an already-existing canonical email block

Fixes #17033

While attempting to create a `CanonicalEmailBlock` with an existing hash would
raise an `ActiveRecord::RecordNotUnique` error, this being done within a
transaction would cancel the whole transaction. For this reason, checking for
uniqueness in Rails would query the database within the transaction and avoid
invalidating the whole transaction for this reason.

A race condition is still possible, where multiple accounts sharing a canonical
email would be blocked in concurrent transactions, in which only one would
succeed, but that is way less likely to happen that the current issue, and can
always be retried after the first failure, unlike the current situation.

* Add tests
This commit is contained in:
Claire 2021-11-24 17:41:03 +01:00 committed by GitHub
parent 9c44cf205f
commit 02a87431cf
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 32 additions and 1 deletions

View file

@ -15,7 +15,7 @@ class CanonicalEmailBlock < ApplicationRecord
belongs_to :reference_account, class_name: 'Account' belongs_to :reference_account, class_name: 'Account'
validates :canonical_email_hash, presence: true validates :canonical_email_hash, presence: true, uniqueness: true
def email=(email) def email=(email)
self.canonical_email_hash = email_to_canonical_email_hash(email) self.canonical_email_hash = email_to_canonical_email_hash(email)

View file

@ -5,6 +5,37 @@ RSpec.describe Account, type: :model do
let(:bob) { Fabricate(:account, username: 'bob') } let(:bob) { Fabricate(:account, username: 'bob') }
subject { Fabricate(:account) } subject { Fabricate(:account) }
describe '#suspend!' do
it 'marks the account as suspended' do
subject.suspend!
expect(subject.suspended?).to be true
end
it 'creates a deletion request' do
subject.suspend!
expect(AccountDeletionRequest.where(account: subject).exists?).to be true
end
context 'when the account is of a local user' do
let!(:subject) { Fabricate(:account, user: Fabricate(:user, email: 'foo+bar@domain.org')) }
it 'creates a canonical domain block' do
subject.suspend!
expect(CanonicalEmailBlock.block?(subject.user_email)).to be true
end
context 'when a canonical domain block already exists for that email' do
before do
Fabricate(:canonical_email_block, email: subject.user_email)
end
it 'does not raise an error' do
expect { subject.suspend! }.not_to raise_error
end
end
end
end
describe '#follow!' do describe '#follow!' do
it 'creates a follow' do it 'creates a follow' do
follow = subject.follow!(bob) follow = subject.follow!(bob)