diff --git a/Changelog.md b/Changelog.md index b14c79792..ff3f3024c 100644 --- a/Changelog.md +++ b/Changelog.md @@ -8,6 +8,13 @@ # 0.7.1.0 +## Ensure account deletions are run + +There were some issues causing accounts deletions to not properly perform in some cases, see +[#7631](https://github.com/diaspora/diaspora/issues/7631) and [#7639](https://github.com/diaspora/diaspora/pull/7639). +To ensure these are reexecuted properly, please run `RAILS_ENV=production bin/rake migrations:run_account_deletions` +after you've upgraded. + ## Refactor * Remove title from profile photo upload button [#7551](https://github.com/diaspora/diaspora/pull/7551) * Remove Internet Explorer workarounds [#7557](https://github.com/diaspora/diaspora/pull/7557) @@ -22,6 +29,7 @@ * Remove `rails_admin_histories` table [#7597](https://github.com/diaspora/diaspora/pull/7597) * Optimize memory usage on profile export [#7627](https://github.com/diaspora/diaspora/pull/7627) * Limit the number of parallel exports [#7629](https://github.com/diaspora/diaspora/pull/7629) +* Reduce memory usage for account deletion [#7639](https://github.com/diaspora/diaspora/pull/7639) ## Bug fixes * Fix displaying polls with long answers [#7579](https://github.com/diaspora/diaspora/pull/7579) @@ -34,6 +42,12 @@ * Fix local migration run without old private key [#7558](https://github.com/diaspora/diaspora/pull/7558) * Fix export not downloadable because the filename was resetted on access [#7622](https://github.com/diaspora/diaspora/pull/7622) * Delete invalid oEmbed caches with binary titles [#7620](https://github.com/diaspora/diaspora/pull/7620) +* Delete invalid diaspora IDs from friendica [#7630](https://github.com/diaspora/diaspora/pull/7630) +* Cleanup relayables where the signature is missing [#7637](https://github.com/diaspora/diaspora/pull/7637) +* Avoid page to jump to top after a post deletion [#7638](https://github.com/diaspora/diaspora/pull/7638) +* Handle duplicate account deletions [#7639](https://github.com/diaspora/diaspora/pull/7639) +* Handle duplicate account migrations [#7641](https://github.com/diaspora/diaspora/pull/7641) +* Handle bugs related to missing users [#7632](https://github.com/diaspora/diaspora/pull/7632) ## Features * Ask for confirmation when leaving a submittable comment field [#7530](https://github.com/diaspora/diaspora/pull/7530) diff --git a/app/assets/javascripts/app/views/post_controls_view.js b/app/assets/javascripts/app/views/post_controls_view.js index 879d4d623..ed2f691c4 100644 --- a/app/assets/javascripts/app/views/post_controls_view.js +++ b/app/assets/javascripts/app/views/post_controls_view.js @@ -78,7 +78,8 @@ app.views.PostControls = app.views.Base.extend({ }.bind(this)); }, - destroyModel: function() { + destroyModel: function(evt) { + if (evt) { evt.preventDefault(); } this.post.destroyModel(); } }); diff --git a/app/models/aspect_membership.rb b/app/models/aspect_membership.rb index f7d042a99..87bc6c574 100644 --- a/app/models/aspect_membership.rb +++ b/app/models/aspect_membership.rb @@ -5,16 +5,13 @@ # the COPYRIGHT file. class AspectMembership < ApplicationRecord - belongs_to :aspect belongs_to :contact has_one :user, :through => :contact has_one :person, :through => :contact before_destroy do - if self.contact && self.contact.aspects.size == 1 - self.user.disconnect(self.contact) - end + user&.disconnect(contact) if contact&.aspects&.size == 1 true end diff --git a/app/models/notification.rb b/app/models/notification.rb index f2dd7df9e..3f8fe719e 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -6,7 +6,7 @@ # class Notification < ApplicationRecord belongs_to :recipient, class_name: "User" - has_many :notification_actors, dependent: :destroy + has_many :notification_actors, dependent: :delete_all has_many :actors, class_name: "Person", through: :notification_actors, source: :person belongs_to :target, polymorphic: true diff --git a/app/models/notification_actor.rb b/app/models/notification_actor.rb index 6bdfe48bd..9b1f30796 100644 --- a/app/models/notification_actor.rb +++ b/app/models/notification_actor.rb @@ -5,8 +5,6 @@ # the COPYRIGHT file. class NotificationActor < ApplicationRecord - belongs_to :notification belongs_to :person - end diff --git a/app/models/user.rb b/app/models/user.rb index 308048dfb..ac16de682 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -90,6 +90,10 @@ class User < ApplicationRecord after_save :remove_invalid_unconfirmed_emails + before_destroy do + raise "Never destroy users!" + end + def self.all_sharing_with_person(person) User.joins(:contacts).where(:contacts => {:person_id => person.id}) end diff --git a/app/models/user/connecting.rb b/app/models/user/connecting.rb index 8b9da90d0..4ddd46e3e 100644 --- a/app/models/user/connecting.rb +++ b/app/models/user/connecting.rb @@ -34,6 +34,7 @@ class User logger.info "event=disconnect user=#{diaspora_handle} target=#{contact.person.diaspora_handle}" if contact.person.local? + raise "FATAL: user entry is missing from the DB. Aborting" if contact.person.owner.nil? contact.person.owner.disconnected_by(contact.user.person) else ContactRetraction.for(contact).defer_dispatch(self) diff --git a/db/migrate/20170928233609_cleanup_invalid_diaspora_ids.rb b/db/migrate/20170928233609_cleanup_invalid_diaspora_ids.rb new file mode 100644 index 000000000..f88fec22b --- /dev/null +++ b/db/migrate/20170928233609_cleanup_invalid_diaspora_ids.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class CleanupInvalidDiasporaIds < ActiveRecord::Migration[5.1] + def up + ids = Person.where("diaspora_handle LIKE '%@%/%'").ids + return if ids.empty? + + AspectMembership.joins(:contact).where(contacts: {person_id: ids}).delete_all + + Person.where(id: ids).each do |person| + destroy_notifications_for_person(person) + person.destroy + end + end + + def destroy_notifications_for_person(person) + Notification.joins(:notification_actors).where(notification_actors: {person_id: person.id}).each do |notification| + if notification.notification_actors.count > 1 + notification.notification_actors.where(person_id: person.id).delete_all + else + notification.destroy + end + end + end +end diff --git a/db/migrate/20171009232054_cleanup_relayables_without_signature.rb b/db/migrate/20171009232054_cleanup_relayables_without_signature.rb new file mode 100644 index 000000000..12af91f19 --- /dev/null +++ b/db/migrate/20171009232054_cleanup_relayables_without_signature.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +class CleanupRelayablesWithoutSignature < ActiveRecord::Migration[5.1] + class Comment < ApplicationRecord + belongs_to :commentable, polymorphic: true + + before_destroy do + Like.where(target_type: "Comment", target_id: id).destroy_all + ActsAsTaggableOn::Tagging.where(taggable_type: "Comment", taggable_id: id).destroy_all + end + + after_destroy do + commentable.update_comments_counter + end + end + + class Like < ApplicationRecord + belongs_to :target, polymorphic: true + + has_one :signature, class_name: "LikeSignature", dependent: :delete + + after_destroy do + target.update_likes_counter + end + end + + class PollParticipation < ApplicationRecord + belongs_to :poll_answer, counter_cache: :vote_count + end + + def up + cleanup_comments + cleanup_likes + cleanup_poll_participations + end + + def cleanup_comments + Comment.joins("INNER JOIN posts as post ON post.id = comments.commentable_id AND " \ + "comments.commentable_type = 'Post'") + .joins("INNER JOIN people as comment_author ON comment_author.id = comments.author_id") + .joins("INNER JOIN people as post_author ON post_author.id = post.author_id") + .where("comment_author.owner_id IS NULL AND post_author.owner_id IS NOT NULL " \ + "AND NOT EXISTS(" \ + "SELECT NULL FROM comment_signatures WHERE comment_signatures.comment_id = comments.id" \ + ")") + .destroy_all + end + + def cleanup_likes + Like.joins("INNER JOIN posts as post ON post.id = likes.target_id AND likes.target_type = 'Post'") + .joins("INNER JOIN people as like_author ON like_author.id = likes.author_id") + .joins("INNER JOIN people as post_author ON post_author.id = post.author_id") + .where("like_author.owner_id IS NULL AND post_author.owner_id IS NOT NULL " \ + "AND NOT EXISTS(" \ + "SELECT NULL FROM like_signatures WHERE like_signatures.like_id = likes.id" \ + ")") + .destroy_all + Like.joins("INNER JOIN comments as comment ON comment.id = likes.target_id AND likes.target_type = 'Comment'") + .joins("INNER JOIN posts as post ON post.id = comment.commentable_id AND comment.commentable_type = 'Post'") + .joins("INNER JOIN people as like_author ON like_author.id = likes.author_id") + .joins("INNER JOIN people as post_author ON post_author.id = post.author_id") + .where("like_author.owner_id IS NULL AND post_author.owner_id IS NOT NULL " \ + "AND NOT EXISTS(" \ + "SELECT NULL FROM like_signatures WHERE like_signatures.like_id = likes.id" \ + ")") + .destroy_all + end + + def cleanup_poll_participations + PollParticipation.joins("INNER JOIN polls as poll ON poll.id = poll_participations.poll_id") + .joins("INNER JOIN posts as post ON post.id = poll.status_message_id") + .joins("INNER JOIN people as pp_author ON pp_author.id = poll_participations.author_id") + .joins("INNER JOIN people as post_author ON post_author.id = post.author_id") + .where("pp_author.owner_id IS NULL AND post_author.owner_id IS NOT NULL " \ + "AND NOT EXISTS(" \ + "SELECT NULL FROM poll_participation_signatures " \ + "WHERE poll_participation_signatures.poll_participation_id = poll_participations.id" \ + ")") + .destroy_all + end +end diff --git a/db/migrate/20171012202650_cleanup_invalid_contacts.rb b/db/migrate/20171012202650_cleanup_invalid_contacts.rb new file mode 100644 index 000000000..709724723 --- /dev/null +++ b/db/migrate/20171012202650_cleanup_invalid_contacts.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +class CleanupInvalidContacts < ActiveRecord::Migration[5.1] + class Contact < ApplicationRecord + belongs_to :user + belongs_to :person + + has_many :aspect_memberships, dependent: :delete_all + + before_destroy :destroy_notifications + + def destroy_notifications + Notification.where( + target_type: "Person", + target_id: person_id, + recipient_id: user_id, + type: "Notifications::StartedSharing" + ).destroy_all + end + end + + class User < ApplicationRecord + end + + class Person < ApplicationRecord + belongs_to :owner, class_name: "User", optional: true + end + + class Notification < ApplicationRecord + self.inheritance_column = nil + has_many :notification_actors, dependent: :delete_all + end + + class NotificationActor < ApplicationRecord + end + + class AspectMembership < ApplicationRecord + end + + def up + Contact.left_outer_joins(:user).where("users.id is NULL").destroy_all + Contact.left_outer_joins(person: :owner).where("people.owner_id is NOT NULL").where("users.id is NULL").destroy_all + end +end diff --git a/lib/account_deleter.rb b/lib/account_deleter.rb index d7ca63d1d..2136e7741 100644 --- a/lib/account_deleter.rb +++ b/lib/account_deleter.rb @@ -5,7 +5,6 @@ # the COPYRIGHT file. class AccountDeleter - # Things that are not removed from the database: # - Comments # - Likes @@ -25,17 +24,14 @@ class AccountDeleter end def perform! - ActiveRecord::Base.transaction do - #person - delete_standard_person_associations - remove_conversation_visibilities - delete_contacts_of_me - tombstone_person_and_profile + # close person + delete_standard_person_associations + delete_contacts_of_me + tombstone_person_and_profile - close_user if user + close_user if user - mark_account_deletion_complete - end + mark_account_deletion_complete end # user deletion methods @@ -46,30 +42,29 @@ class AccountDeleter tombstone_user end - #user deletions + # user deletions def normal_ar_user_associates_to_delete %i[tag_followings services aspects user_preferences notifications blocks authorizations o_auth_applications pairwise_pseudonymous_identifiers] end - def special_ar_user_associations - %i[person profile contacts auto_follow_back_aspect] - end - - def ignored_ar_user_associations - %i[followed_tags invited_by invited_users contact_people aspect_memberships - ignored_people share_visibilities conversation_visibilities conversations reports] - end - def delete_standard_user_associations normal_ar_user_associates_to_delete.each do |asso| - self.user.send(asso).each{|model| model.destroy } + user.send(asso).ids.each_slice(20) do |ids| + User.reflect_on_association(asso).class_name.constantize.where(id: ids).destroy_all + end end end + def normal_ar_person_associates_to_delete + %i[posts photos mentions participations roles blocks conversation_visibilities] + end + def delete_standard_person_associations normal_ar_person_associates_to_delete.each do |asso| - self.person.send(asso).destroy_all + person.send(asso).ids.each_slice(20) do |ids| + Person.reflect_on_association(asso).class_name.constantize.where(id: ids).destroy_all + end end end @@ -80,33 +75,20 @@ class AccountDeleter # Currently this would get deleted due to the db foreign key constrainsts, # but we'll keep this method here for completeness def remove_share_visibilities_on_contacts_posts - ShareVisibility.for_a_user(user).destroy_all - end - - def remove_conversation_visibilities - ConversationVisibility.where(:person_id => person.id).destroy_all + ShareVisibility.for_a_user(user).find_each(batch_size: 20, &:destroy) end def tombstone_person_and_profile - self.person.lock_access! - self.person.clear_profile! + person.lock_access! + person.clear_profile! end def tombstone_user - self.user.clear_account! + user.clear_account! end def delete_contacts_of_me - Contact.all_contacts_of_person(self.person).destroy_all - end - - def normal_ar_person_associates_to_delete - %i[posts photos mentions participations roles blocks] - end - - def ignored_or_special_ar_person_associations - %i[comments likes poll_participations contacts notification_actors notifications owner profile - conversation_visibilities pod conversations messages] + Contact.all_contacts_of_person(person).find_each(batch_size: 20, &:destroy) end def mark_account_deletion_complete diff --git a/lib/diaspora/federation/receive.rb b/lib/diaspora/federation/receive.rb index adddf4000..d4dcea34d 100644 --- a/lib/diaspora/federation/receive.rb +++ b/lib/diaspora/federation/receive.rb @@ -10,15 +10,22 @@ module Diaspora end def self.account_deletion(entity) - AccountDeletion.create!(person: author_of(entity)) + person = author_of(entity) + AccountDeletion.create!(person: person) unless AccountDeletion.where(person: person).exists? + rescue => e # rubocop:disable Lint/RescueWithoutErrorClass + raise e unless AccountDeletion.where(person: person).exists? + logger.warn "ignoring error on receive AccountDeletion:#{entity.author}: #{e.class}: #{e.message}" end def self.account_migration(entity) + old_person = author_of(entity) profile = profile(entity.profile) - AccountMigration.create!( - old_person: Person.by_account_identifier(entity.author), - new_person: profile.person - ) + return if AccountMigration.where(old_person: old_person, new_person: profile.person).exists? + AccountMigration.create!(old_person: old_person, new_person: profile.person) + rescue => e # rubocop:disable Lint/RescueWithoutErrorClass + raise e unless AccountMigration.where(old_person: old_person, new_person: profile.person).exists? + logger.warn "ignoring error on receive #{entity}: #{e.class}: #{e.message}" + nil end def self.comment(entity) diff --git a/lib/tasks/migrations.rake b/lib/tasks/migrations.rake index 6951b3ffc..a93e55ed4 100644 --- a/lib/tasks/migrations.rake +++ b/lib/tasks/migrations.rake @@ -59,4 +59,15 @@ namespace :migrations do queue.clear end end + + desc "Run uncompleted account deletions" + task run_account_deletions: :environment do + if AccountDeletion.uncompleted.count > 0 + puts "Running account deletions.." + AccountDeletion.uncompleted.find_each(&:perform!) + puts "OK." + else + puts "No acccount deletions to run." + end + end end diff --git a/spec/controllers/posts_controller_spec.rb b/spec/controllers/posts_controller_spec.rb index 9e69b569e..47c669ba4 100644 --- a/spec/controllers/posts_controller_spec.rb +++ b/spec/controllers/posts_controller_spec.rb @@ -29,7 +29,7 @@ describe PostsController, type: :controller do msg = alice.post(:status_message, text: "Mention @{User ; #{user.diaspora_handle}}", public: true) expect(msg.mentioned_people.count).to eq(1) - user.destroy + user.close_account! get :show, params: {id: msg.id} expect(response).to be_success diff --git a/spec/integration/federation/receive_federation_messages_spec.rb b/spec/integration/federation/receive_federation_messages_spec.rb index 5ab47dbcc..c9a67aeb4 100644 --- a/spec/integration/federation/receive_federation_messages_spec.rb +++ b/spec/integration/federation/receive_federation_messages_spec.rb @@ -58,11 +58,12 @@ describe "Receive federation messages feature" do expect(AccountMigration.find_by(old_person: sender.person, new_person: new_user.person)).to be_performed end - it "doesn't accept the same migration for the second time" do + it "doesn't run the same migration for the second time" do run_migration - expect { - run_migration - }.to raise_error(ActiveRecord::RecordInvalid) + expect_any_instance_of(AccountMigration).not_to receive(:perform!) + run_migration + expect(AccountMigration.where(old_person: sender.person, new_person: new_user.person).count).to eq(1) + expect(AccountMigration.find_by(old_person: sender.person, new_person: new_user.person)).to be_performed end it "doesn't accept second migration for the same sender" do diff --git a/spec/lib/account_deleter_spec.rb b/spec/lib/account_deleter_spec.rb index a82bbe578..7a3a7faf6 100644 --- a/spec/lib/account_deleter_spec.rb +++ b/spec/lib/account_deleter_spec.rb @@ -20,7 +20,6 @@ describe AccountDeleter do delete_contacts_of_me delete_standard_person_associations tombstone_person_and_profile - remove_conversation_visibilities ] context "user deletion" do @@ -96,8 +95,12 @@ describe AccountDeleter do it 'removes all standard user associaltions' do @account_deletion.normal_ar_user_associates_to_delete.each do |asso| association_double = double - expect(association_double).to receive(:destroy) - expect(bob).to receive(asso).and_return([association_double]) + expect(association_double).to receive(:ids).and_return([42]) + expect(bob).to receive(asso).and_return(association_double) + batch_double = double + expect(User.reflect_on_association(asso).class_name.constantize).to receive(:where) + .with(id: [42]).and_return(batch_double) + expect(batch_double).to receive(:destroy_all) end @account_deletion.delete_standard_user_associations @@ -111,8 +114,12 @@ describe AccountDeleter do it 'removes all standard person associaltions' do @account_deletion.normal_ar_person_associates_to_delete.each do |asso| association_double = double - expect(association_double).to receive(:destroy_all) + expect(association_double).to receive(:ids).and_return([42]) expect(bob.person).to receive(asso).and_return(association_double) + batch_double = double + expect(Person.reflect_on_association(asso).class_name.constantize).to receive(:where) + .with(id: [42]).and_return(batch_double) + expect(batch_double).to receive(:destroy_all) end @account_deletion.delete_standard_person_associations @@ -133,7 +140,7 @@ describe AccountDeleter do it 'deletes all the local contact objects where deleted account is the person' do contacts = double expect(Contact).to receive(:all_contacts_of_person).with(bob.person).and_return(contacts) - expect(contacts).to receive(:destroy_all) + expect(contacts).to receive(:find_each).with(batch_size: 20) @account_deletion.delete_contacts_of_me end end @@ -149,21 +156,13 @@ describe AccountDeleter do @account_deletion.tombstone_person_and_profile end end - describe "#remove_conversation_visibilities" do - it "removes the conversation visibility for the deleted user" do - vis = double - expect(ConversationVisibility).to receive(:where).with(hash_including(:person_id => bob.person.id)).and_return(vis) - expect(vis).to receive(:destroy_all) - @account_deletion.remove_conversation_visibilities - end - end end describe "#remove_share_visibilities_by_contacts_of_user" do it "removes the share visibilities for a user" do s_vis = double expect(ShareVisibility).to receive(:for_a_user).with(bob).and_return(s_vis) - expect(s_vis).to receive(:destroy_all) + expect(s_vis).to receive(:find_each).with(batch_size: 20) @account_deletion.remove_share_visibilities_on_contacts_posts end @@ -176,14 +175,20 @@ describe AccountDeleter do end end - it 'has all user association keys accounted for' do - all_keys = (@account_deletion.normal_ar_user_associates_to_delete + @account_deletion.special_ar_user_associations + @account_deletion.ignored_ar_user_associations) - expect(all_keys.sort{|x, y| x.to_s <=> y.to_s}).to eq(User.reflections.keys.sort{|x, y| x.to_s <=> y.to_s}.map(&:to_sym)) + it "has all user association keys accounted for" do + special_ar_user_associations = %i[person profile contacts auto_follow_back_aspect] + ignored_ar_user_associations = %i[followed_tags invited_by invited_users contact_people aspect_memberships + ignored_people share_visibilities conversation_visibilities conversations reports] + all_keys = @account_deletion.normal_ar_user_associates_to_delete + + special_ar_user_associations + ignored_ar_user_associations + expect(all_keys.sort_by(&:to_s)).to eq(User.reflections.keys.sort_by(&:to_s).map(&:to_sym)) end - it 'has all person association keys accounted for' do - all_keys = (@account_deletion.normal_ar_person_associates_to_delete + @account_deletion.ignored_or_special_ar_person_associations) - expect(all_keys.sort{|x, y| x.to_s <=> y.to_s}).to eq(Person.reflections.keys.sort{|x, y| x.to_s <=> y.to_s}.map(&:to_sym)) + it "has all person association keys accounted for" do + ignored_or_special_ar_person_associations = %i[comments likes poll_participations contacts notification_actors + notifications owner profile pod conversations messages] + all_keys = @account_deletion.normal_ar_person_associates_to_delete + ignored_or_special_ar_person_associations + expect(all_keys.sort_by(&:to_s)).to eq(Person.reflections.keys.sort_by(&:to_s).map(&:to_sym)) end end diff --git a/spec/lib/diaspora/federation/receive_spec.rb b/spec/lib/diaspora/federation/receive_spec.rb index 466dd4bfe..2c2e7e110 100644 --- a/spec/lib/diaspora/federation/receive_spec.rb +++ b/spec/lib/diaspora/federation/receive_spec.rb @@ -12,6 +12,27 @@ describe Diaspora::Federation::Receive do expect(AccountDeletion.exists?(person: sender)).to be_truthy end + + it "ignores duplicate the account deletion" do + AccountDeletion.create(person: sender) + + expect(AccountDeletion).not_to receive(:create!) + + Diaspora::Federation::Receive.account_deletion(account_deletion_entity) + + expect(AccountDeletion.exists?(person: sender)).to be_truthy + end + + it "handles race conditions on parallel receive" do + expect(AccountDeletion).to receive(:create!) do + AccountDeletion.create(person: sender) + raise "Some database error" + end + + Diaspora::Federation::Receive.account_deletion(account_deletion_entity) + + expect(AccountDeletion.exists?(person: sender)).to be_truthy + end end describe ".account_migration" do @@ -26,6 +47,27 @@ describe Diaspora::Federation::Receive do expect(AccountMigration.exists?(old_person: sender, new_person: new_person)).to be_truthy end + + it "ignores duplicate the account migrations" do + AccountMigration.create(old_person: sender, new_person: new_person) + + expect(AccountMigration).not_to receive(:create!) + + expect(Diaspora::Federation::Receive.account_migration(account_migration_entity)).to be_nil + + expect(AccountMigration.exists?(old_person: sender, new_person: new_person)).to be_truthy + end + + it "handles race conditions on parallel receive" do + expect(AccountMigration).to receive(:create!) do + AccountMigration.create(old_person: sender, new_person: new_person) + raise "Some database error" + end + + expect(Diaspora::Federation::Receive.account_migration(account_migration_entity)).to be_nil + + expect(AccountMigration.exists?(old_person: sender, new_person: new_person)).to be_truthy + end end describe ".comment" do diff --git a/spec/models/aspect_membership_spec.rb b/spec/models/aspect_membership_spec.rb index d9585c554..d8f1de0fe 100644 --- a/spec/models/aspect_membership_spec.rb +++ b/spec/models/aspect_membership_spec.rb @@ -26,5 +26,13 @@ describe AspectMembership, :type => :model do alice.add_contact_to_aspect(contact, aspect) aspect_membership.destroy end + + it "doesn't fail destruction if the user entry was deleted in prior" do + aspect_membership.user.delete + id = aspect_membership.id + + expect { AspectMembership.find(id).destroy }.not_to raise_error + expect(AspectMembership.where(id: id)).not_to exist + end end end diff --git a/spec/models/user/connecting_spec.rb b/spec/models/user/connecting_spec.rb index be4125cc4..c31ce1004 100644 --- a/spec/models/user/connecting_spec.rb +++ b/spec/models/user/connecting_spec.rb @@ -104,6 +104,16 @@ describe User::Connecting, type: :model do alice.disconnect(contact) }.to change(contact.aspects, :count).from(2).to(0) end + + it "raises when a contact for an improperly deleted user was passed" do + contact = alice.contact_for(bob.person) + + bob.delete + expect { + alice.disconnect(contact) + }.to raise_error "FATAL: user entry is missing from the DB. Aborting" + expect(Contact.where(id: contact.id)).to exist + end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index d399929b1..913e3c735 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -565,16 +565,11 @@ describe User, :type => :model do end end - describe 'account deletion' do - describe '#destroy' do - it 'removes all service connections' do - Services::Facebook.create(:access_token => 'what', :user_id => alice.id) - expect { - alice.destroy - }.to change { - alice.services.count - }.by(-1) - end + describe "#destroy" do + it "raises error" do + expect { + alice.destroy + }.to raise_error "Never destroy users!" end end