From 245ad9e04de7f65e2cc40f2c02d4462021c12ea6 Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Sun, 13 Aug 2017 16:51:08 +0200 Subject: [PATCH] Remove diaspora_handle from AccountDeletions and add unique index --- app/models/account_deletion.rb | 16 ++++---------- app/models/user.rb | 2 +- ..._account_deletions_and_add_unique_index.rb | 19 +++++++++++++++++ lib/account_deleter.rb | 2 +- lib/diaspora/federation/receive.rb | 2 +- spec/factories.rb | 3 --- .../receive_federation_messages_spec.rb | 21 ++++++++++--------- spec/lib/diaspora/federation/receive_spec.rb | 4 +--- 8 files changed, 38 insertions(+), 31 deletions(-) create mode 100644 db/migrate/20170813141631_cleanup_account_deletions_and_add_unique_index.rb diff --git a/app/models/account_deletion.rb b/app/models/account_deletion.rb index cc9445310..11a1bfcb3 100644 --- a/app/models/account_deletion.rb +++ b/app/models/account_deletion.rb @@ -5,23 +5,15 @@ class AccountDeletion < ApplicationRecord include Diaspora::Federated::Base - scope :uncompleted, -> { where('completed_at is null') } + scope :uncompleted, -> { where("completed_at is null") } belongs_to :person - after_commit :queue_delete_account, :on => :create + after_commit :queue_delete_account, on: :create - def person=(person) - self[:diaspora_handle] = person.diaspora_handle - self[:person_id] = person.id - end - - def diaspora_handle=(diaspora_handle) - self[:diaspora_handle] = diaspora_handle - self[:person_id] ||= Person.find_by_diaspora_handle(diaspora_handle).id - end + delegate :diaspora_handle, to: :person def queue_delete_account - Workers::DeleteAccount.perform_async(self.id) + Workers::DeleteAccount.perform_async(id) end def perform! diff --git a/app/models/user.rb b/app/models/user.rb index 629649d4e..3d98b5eb3 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -507,7 +507,7 @@ class User < ApplicationRecord def close_account! self.person.lock_access! self.lock_access! - AccountDeletion.create(:person => self.person) + AccountDeletion.create(person: person) end def closed_account? diff --git a/db/migrate/20170813141631_cleanup_account_deletions_and_add_unique_index.rb b/db/migrate/20170813141631_cleanup_account_deletions_and_add_unique_index.rb new file mode 100644 index 000000000..6d188278d --- /dev/null +++ b/db/migrate/20170813141631_cleanup_account_deletions_and_add_unique_index.rb @@ -0,0 +1,19 @@ +class CleanupAccountDeletionsAndAddUniqueIndex < ActiveRecord::Migration[5.1] + def up + remove_column :account_deletions, :diaspora_handle + + duplicate_query = "WHERE a1.person_id = a2.person_id AND a1.id > a2.id" + if AppConfig.postgres? + execute("DELETE FROM account_deletions AS a1 USING account_deletions AS a2 #{duplicate_query}") + else + execute("DELETE a1 FROM account_deletions a1, account_deletions a2 #{duplicate_query}") + end + + add_index :account_deletions, :person_id, name: :index_account_deletions_on_person_id, unique: true + end + + def down + remove_index :account_deletions, name: :index_account_deletions_on_person_id + add_column :account_deletions, :diaspora_handle, :string + end +end diff --git a/lib/account_deleter.rb b/lib/account_deleter.rb index a466a3f2e..0b917c402 100644 --- a/lib/account_deleter.rb +++ b/lib/account_deleter.rb @@ -106,6 +106,6 @@ class AccountDeleter end def mark_account_deletion_complete - AccountDeletion.where(:diaspora_handle => self.person.diaspora_handle).where(:person_id => self.person.id).update_all(["completed_at = ?", Time.now]) + AccountDeletion.find_by(person: person)&.update_attributes(completed_at: Time.now.utc) end end diff --git a/lib/diaspora/federation/receive.rb b/lib/diaspora/federation/receive.rb index a3e8cadd4..7b1faea26 100644 --- a/lib/diaspora/federation/receive.rb +++ b/lib/diaspora/federation/receive.rb @@ -8,7 +8,7 @@ module Diaspora end def self.account_deletion(entity) - AccountDeletion.create!(person: author_of(entity), diaspora_handle: entity.author) + AccountDeletion.create!(person: author_of(entity)) end def self.comment(entity) diff --git a/spec/factories.rb b/spec/factories.rb index d3fc152d8..02519966a 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -51,9 +51,6 @@ FactoryGirl.define do factory :account_deletion do association :person - after(:build) do |delete| - delete.diaspora_handle = delete.person.diaspora_handle - end end factory :like do diff --git a/spec/integration/federation/receive_federation_messages_spec.rb b/spec/integration/federation/receive_federation_messages_spec.rb index 651c65150..f689d55d2 100644 --- a/spec/integration/federation/receive_federation_messages_spec.rb +++ b/spec/integration/federation/receive_federation_messages_spec.rb @@ -14,18 +14,19 @@ describe "Receive federation messages feature" do context "with public receive" do let(:recipient) { nil } - it "receives account deletion correctly" do - post_message(generate_payload(DiasporaFederation::Entities::AccountDeletion.new(diaspora_id: sender_id), sender)) + context "account deletion" do + it "receives account deletion correctly" do + post_message(generate_payload(DiasporaFederation::Entities::AccountDeletion.new(author: sender_id), sender)) - expect(AccountDeletion.exists?(diaspora_handle: sender_id)).to be_truthy - end + expect(AccountDeletion.exists?(person: sender.person)).to be_truthy + end - it "rejects account deletion with wrong diaspora_id" do - delete_id = Fabricate.sequence(:diaspora_id) - post_message(generate_payload(DiasporaFederation::Entities::AccountDeletion.new(diaspora_id: delete_id), sender)) - - expect(AccountDeletion.exists?(diaspora_handle: delete_id)).to be_falsey - expect(AccountDeletion.exists?(diaspora_handle: sender_id)).to be_falsey + it "rejects account deletion with wrong author" do + delete_id = Fabricate.sequence(:diaspora_id) + expect { + post_message(generate_payload(DiasporaFederation::Entities::AccountDeletion.new(author: delete_id), sender)) + }.not_to change(AccountDeletion, :count) + end end context "reshare" do diff --git a/spec/lib/diaspora/federation/receive_spec.rb b/spec/lib/diaspora/federation/receive_spec.rb index 720e3c574..d8b0dcaa5 100644 --- a/spec/lib/diaspora/federation/receive_spec.rb +++ b/spec/lib/diaspora/federation/receive_spec.rb @@ -8,9 +8,7 @@ describe Diaspora::Federation::Receive do it "saves the account deletion" do Diaspora::Federation::Receive.account_deletion(account_deletion_entity) - account_deletion = AccountDeletion.find_by!(diaspora_handle: sender.diaspora_handle) - - expect(account_deletion.person).to eq(sender) + expect(AccountDeletion.exists?(person: sender)).to be_truthy end end