From 32067246df724b7f6455f3ef95f747391c48d043 Mon Sep 17 00:00:00 2001
From: Benjamin Neff <benjamin@coding4coffee.ch>
Date: Fri, 29 Sep 2017 01:38:16 +0200
Subject: [PATCH 01/14] Delete people with invalid diaspora IDs (friendica with
 path)

closes #7630
---
 Changelog.md                                  |  1 +
 app/models/aspect_membership.rb               |  1 -
 app/models/notification_actor.rb              |  2 --
 ...0928233609_cleanup_invalid_diaspora_ids.rb | 25 +++++++++++++++++++
 4 files changed, 26 insertions(+), 3 deletions(-)
 create mode 100644 db/migrate/20170928233609_cleanup_invalid_diaspora_ids.rb

diff --git a/Changelog.md b/Changelog.md
index 4d0a518a5..05094101b 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -26,6 +26,7 @@
 * 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)
 
 ## Features
 * Ask for confirmation when leaving a submittable comment field [#7530](https://github.com/diaspora/diaspora/pull/7530)
diff --git a/app/models/aspect_membership.rb b/app/models/aspect_membership.rb
index f7d042a99..18c5869bb 100644
--- a/app/models/aspect_membership.rb
+++ b/app/models/aspect_membership.rb
@@ -5,7 +5,6 @@
 #   the COPYRIGHT file.
 
 class AspectMembership < ApplicationRecord
-
   belongs_to :aspect
   belongs_to :contact
   has_one :user, :through => :contact
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/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

From 2711b9dc831d8c58bc9ba7c1bd917e6e16aa107e Mon Sep 17 00:00:00 2001
From: Benjamin Neff <benjamin@coding4coffee.ch>
Date: Tue, 10 Oct 2017 23:50:13 +0200
Subject: [PATCH 02/14] Cleanup relayables where the signature is missing

When we should have the signature but don't have it, the user data
export fails. There are a few comments from back in 2011 where the
signature is missing.

Also some podmins maybe messed with signatures in their database, which
would also break the exports now.

closes #7637
---
 Changelog.md                                  |  1 +
 ...54_cleanup_relayables_without_signature.rb | 81 +++++++++++++++++++
 2 files changed, 82 insertions(+)
 create mode 100644 db/migrate/20171009232054_cleanup_relayables_without_signature.rb

diff --git a/Changelog.md b/Changelog.md
index 05094101b..643dda44b 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -27,6 +27,7 @@
 * 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)
 
 ## Features
 * Ask for confirmation when leaving a submittable comment field [#7530](https://github.com/diaspora/diaspora/pull/7530)
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

From faff140a3fe35cab05603632aaf0d638c25193e8 Mon Sep 17 00:00:00 2001
From: flaburgan <flaburgan@geexxx.fr>
Date: Wed, 11 Oct 2017 14:30:05 +0200
Subject: [PATCH 03/14] Avoid page to jump to top after a post deletion, fixes
 #7628

closes #7638
---
 Changelog.md                                           | 1 +
 app/assets/javascripts/app/views/post_controls_view.js | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/Changelog.md b/Changelog.md
index 643dda44b..f886d08b2 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -28,6 +28,7 @@
 * 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)
 
 ## 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();
   }
 });

From 4e6d92ce63c96d577cef04e338bed8a17372b635 Mon Sep 17 00:00:00 2001
From: Benjamin Neff <benjamin@coding4coffee.ch>
Date: Thu, 12 Oct 2017 04:38:56 +0200
Subject: [PATCH 04/14] Remove wrapping transaction for account deletion

This uses a lot of memory for big accounts. Also it doesn't make much
sense to rollback everything when something fails, it's better to delete
everything we can.
---
 lib/account_deleter.rb | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/lib/account_deleter.rb b/lib/account_deleter.rb
index d7ca63d1d..b14582980 100644
--- a/lib/account_deleter.rb
+++ b/lib/account_deleter.rb
@@ -25,17 +25,15 @@ 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
+    remove_conversation_visibilities
+    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

From f4902421eaa50dea98dc5046848f95c959cb3c88 Mon Sep 17 00:00:00 2001
From: Benjamin Neff <benjamin@coding4coffee.ch>
Date: Thu, 12 Oct 2017 02:43:37 +0200
Subject: [PATCH 05/14] Destroy user and person associations in batches

---
 lib/account_deleter.rb           | 21 ++++++++++++---------
 spec/lib/account_deleter_spec.rb | 18 +++++++++++++-----
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/lib/account_deleter.rb b/lib/account_deleter.rb
index b14582980..356c5a54c 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
@@ -44,7 +43,7 @@ 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]
@@ -61,13 +60,17 @@ class AccountDeleter
 
   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 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
 
@@ -78,7 +81,7 @@ 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
+    ShareVisibility.for_a_user(user).find_each(batch_size: 20, &:destroy)
   end
 
   def remove_conversation_visibilities
@@ -86,16 +89,16 @@ class AccountDeleter
   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
+    Contact.all_contacts_of_person(person).find_each(batch_size: 20, &:destroy)
   end
 
   def normal_ar_person_associates_to_delete
diff --git a/spec/lib/account_deleter_spec.rb b/spec/lib/account_deleter_spec.rb
index a82bbe578..a6445a3b9 100644
--- a/spec/lib/account_deleter_spec.rb
+++ b/spec/lib/account_deleter_spec.rb
@@ -96,8 +96,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 +115,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 +141,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
@@ -163,7 +171,7 @@ describe AccountDeleter 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

From f704f305727feeb9fa528de6a08ded56d40674d2 Mon Sep 17 00:00:00 2001
From: Benjamin Neff <benjamin@coding4coffee.ch>
Date: Thu, 12 Oct 2017 02:46:48 +0200
Subject: [PATCH 06/14] Remove conversation visibilities with standard person
 associations

---
 lib/account_deleter.rb           | 9 ++-------
 spec/lib/account_deleter_spec.rb | 9 ---------
 2 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/lib/account_deleter.rb b/lib/account_deleter.rb
index 356c5a54c..3158de873 100644
--- a/lib/account_deleter.rb
+++ b/lib/account_deleter.rb
@@ -26,7 +26,6 @@ class AccountDeleter
   def perform!
     # close person
     delete_standard_person_associations
-    remove_conversation_visibilities
     delete_contacts_of_me
     tombstone_person_and_profile
 
@@ -84,10 +83,6 @@ class AccountDeleter
     ShareVisibility.for_a_user(user).find_each(batch_size: 20, &:destroy)
   end
 
-  def remove_conversation_visibilities
-    ConversationVisibility.where(:person_id => person.id).destroy_all
-  end
-
   def tombstone_person_and_profile
     person.lock_access!
     person.clear_profile!
@@ -102,12 +97,12 @@ class AccountDeleter
   end
 
   def normal_ar_person_associates_to_delete
-    %i[posts photos mentions participations roles blocks]
+    %i[posts photos mentions participations roles blocks conversation_visibilities]
   end
 
   def ignored_or_special_ar_person_associations
     %i[comments likes poll_participations contacts notification_actors notifications owner profile
-       conversation_visibilities pod conversations messages]
+       pod conversations messages]
   end
 
   def mark_account_deletion_complete
diff --git a/spec/lib/account_deleter_spec.rb b/spec/lib/account_deleter_spec.rb
index a6445a3b9..3752d91a6 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
@@ -157,14 +156,6 @@ 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

From b920ddbff55dd0facd54da884943d16e568860f2 Mon Sep 17 00:00:00 2001
From: Benjamin Neff <benjamin@coding4coffee.ch>
Date: Thu, 12 Oct 2017 02:56:40 +0200
Subject: [PATCH 07/14] Move special and ignored associations to tests

---
 lib/account_deleter.rb           | 22 ++++------------------
 spec/lib/account_deleter_spec.rb | 18 ++++++++++++------
 2 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/lib/account_deleter.rb b/lib/account_deleter.rb
index 3158de873..2136e7741 100644
--- a/lib/account_deleter.rb
+++ b/lib/account_deleter.rb
@@ -48,15 +48,6 @@ class AccountDeleter
        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|
       user.send(asso).ids.each_slice(20) do |ids|
@@ -65,6 +56,10 @@ class AccountDeleter
     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|
       person.send(asso).ids.each_slice(20) do |ids|
@@ -96,15 +91,6 @@ class AccountDeleter
     Contact.all_contacts_of_person(person).find_each(batch_size: 20, &:destroy)
   end
 
-  def normal_ar_person_associates_to_delete
-    %i[posts photos mentions participations roles blocks conversation_visibilities]
-  end
-
-  def ignored_or_special_ar_person_associations
-    %i[comments likes poll_participations contacts notification_actors notifications owner profile
-       pod conversations messages]
-  end
-
   def mark_account_deletion_complete
     AccountDeletion.find_by(person: person)&.update_attributes(completed_at: Time.now.utc)
   end
diff --git a/spec/lib/account_deleter_spec.rb b/spec/lib/account_deleter_spec.rb
index 3752d91a6..7a3a7faf6 100644
--- a/spec/lib/account_deleter_spec.rb
+++ b/spec/lib/account_deleter_spec.rb
@@ -175,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
 

From 6d5647ec112d3fd0c93d7f217dc2f6beed9fde04 Mon Sep 17 00:00:00 2001
From: Benjamin Neff <benjamin@coding4coffee.ch>
Date: Thu, 12 Oct 2017 03:37:35 +0200
Subject: [PATCH 08/14] Handle duplicate account deletions

---
 lib/diaspora/federation/receive.rb           |  6 +++++-
 spec/lib/diaspora/federation/receive_spec.rb | 21 ++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/lib/diaspora/federation/receive.rb b/lib/diaspora/federation/receive.rb
index adddf4000..13e235de1 100644
--- a/lib/diaspora/federation/receive.rb
+++ b/lib/diaspora/federation/receive.rb
@@ -10,7 +10,11 @@ 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)
diff --git a/spec/lib/diaspora/federation/receive_spec.rb b/spec/lib/diaspora/federation/receive_spec.rb
index 466dd4bfe..1fd76b08d 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

From 2bd9c663c509f4f5d1ccae0e20fa13e220510627 Mon Sep 17 00:00:00 2001
From: Benjamin Neff <benjamin@coding4coffee.ch>
Date: Mon, 16 Oct 2017 00:27:43 +0200
Subject: [PATCH 09/14] Add rake task to rerun failed account deletions

closes #7639
---
 Changelog.md              |  9 +++++++++
 lib/tasks/migrations.rake | 11 +++++++++++
 2 files changed, 20 insertions(+)

diff --git a/Changelog.md b/Changelog.md
index f886d08b2..5787dc9ca 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -1,5 +1,12 @@
 # 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)
@@ -14,6 +21,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)
@@ -29,6 +37,7 @@
 * 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)
 
 ## Features
 * Ask for confirmation when leaving a submittable comment field [#7530](https://github.com/diaspora/diaspora/pull/7530)
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

From a36d22d72b1d38124ae8400194059b0cf5bbe0d7 Mon Sep 17 00:00:00 2001
From: Benjamin Neff <benjamin@coding4coffee.ch>
Date: Mon, 16 Oct 2017 23:00:21 +0200
Subject: [PATCH 10/14] Handle duplicate account migrations

closes #7641
---
 Changelog.md                                  |  1 +
 lib/diaspora/federation/receive.rb            | 11 ++++++----
 .../receive_federation_messages_spec.rb       |  9 ++++----
 spec/lib/diaspora/federation/receive_spec.rb  | 21 +++++++++++++++++++
 4 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/Changelog.md b/Changelog.md
index 5787dc9ca..b6ce3f066 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -38,6 +38,7 @@ after you've upgraded.
 * 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)
 
 ## Features
 * Ask for confirmation when leaving a submittable comment field [#7530](https://github.com/diaspora/diaspora/pull/7530)
diff --git a/lib/diaspora/federation/receive.rb b/lib/diaspora/federation/receive.rb
index 13e235de1..d4dcea34d 100644
--- a/lib/diaspora/federation/receive.rb
+++ b/lib/diaspora/federation/receive.rb
@@ -18,11 +18,14 @@ module Diaspora
       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/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/diaspora/federation/receive_spec.rb b/spec/lib/diaspora/federation/receive_spec.rb
index 1fd76b08d..2c2e7e110 100644
--- a/spec/lib/diaspora/federation/receive_spec.rb
+++ b/spec/lib/diaspora/federation/receive_spec.rb
@@ -47,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

From f212b51f7f83f63cb59dff45f38f82480bd7bb14 Mon Sep 17 00:00:00 2001
From: cmrd Senya <senya@riseup.net>
Date: Sun, 1 Oct 2017 03:05:57 +0300
Subject: [PATCH 11/14] Check for user existence on aspect membership
 destruction

---
 app/models/aspect_membership.rb       | 4 +---
 spec/models/aspect_membership_spec.rb | 8 ++++++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/app/models/aspect_membership.rb b/app/models/aspect_membership.rb
index 18c5869bb..87bc6c574 100644
--- a/app/models/aspect_membership.rb
+++ b/app/models/aspect_membership.rb
@@ -11,9 +11,7 @@ class AspectMembership < ApplicationRecord
   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/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

From 973e9d98c60226d216ecdd5874311e7b03e1445d Mon Sep 17 00:00:00 2001
From: cmrd Senya <senya@riseup.net>
Date: Sun, 1 Oct 2017 04:22:20 +0300
Subject: [PATCH 12/14] Raise sensible error message when user is missing

That's for the case when podmin has messed up the database
---
 app/models/user/connecting.rb       |  1 +
 spec/models/user/connecting_spec.rb | 10 ++++++++++
 2 files changed, 11 insertions(+)

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/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
 

From ad025850cece51672257929ab9e54b93f7fa70aa Mon Sep 17 00:00:00 2001
From: cmrd Senya <senya@riseup.net>
Date: Sun, 1 Oct 2017 04:28:46 +0300
Subject: [PATCH 13/14] Forbid user destruction

---
 app/models/user.rb                        |  4 ++++
 spec/controllers/posts_controller_spec.rb |  2 +-
 spec/models/user_spec.rb                  | 15 +++++----------
 3 files changed, 10 insertions(+), 11 deletions(-)

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/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/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
 

From 2e9c6f46dc6c01a63123445da876f23780b3398d Mon Sep 17 00:00:00 2001
From: cmrd Senya <senya@riseup.net>
Date: Thu, 12 Oct 2017 18:14:29 +0300
Subject: [PATCH 14/14] Clean up invalid contacts from the DB

Cleans invalid contacts where the referenced user was deleted from the DB or where
the referenced person owner was deleted from the DB.

closes #7632
---
 Changelog.md                                  |  1 +
 app/models/notification.rb                    |  2 +-
 ...20171012202650_cleanup_invalid_contacts.rb | 44 +++++++++++++++++++
 3 files changed, 46 insertions(+), 1 deletion(-)
 create mode 100644 db/migrate/20171012202650_cleanup_invalid_contacts.rb

diff --git a/Changelog.md b/Changelog.md
index b6ce3f066..b1be0a091 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -39,6 +39,7 @@ after you've upgraded.
 * 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/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/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