Resolve "Remove ghost notification settings for groups and projects"

5 files changed, 73 insertions, 1 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index fdf3618b4dc..37f2e8b680e 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -130,7 +130,7 @@ class User < ActiveRecord::Base
   has_many :builds,                   dependent: :nullify, class_name: 'Ci::Build' # rubocop:disable Cop/ActiveRecordDependent
   has_many :pipelines,                dependent: :nullify, class_name: 'Ci::Pipeline' # rubocop:disable Cop/ActiveRecordDependent
   has_many :todos
-  has_many :notification_settings,    dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+  has_many :notification_settings
   has_many :award_emoji,              dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :triggers,                 dependent: :destroy, class_name: 'Ci::Trigger', foreign_key: :owner_id # rubocop:disable Cop/ActiveRecordDependent
 
diff --git a/changelogs/unreleased/44824-remove-ghost-notification-settings-for-group-and-project.yml b/changelogs/unreleased/44824-remove-ghost-notification-settings-for-group-and-project.yml
new file mode 100644
index 00000000000..ddc878ee710
--- /dev/null
+++ b/changelogs/unreleased/44824-remove-ghost-notification-settings-for-group-and-project.yml
@@ -0,0 +1,5 @@
+---
+title: Adds foreign key to notification_settings.user_id
+merge_request: 20567
+author: Jacopo Beschi @jacopo-beschi
+type: added
diff --git a/db/migrate/20180710162338_add_foreign_key_from_notification_settings_to_users.rb b/db/migrate/20180710162338_add_foreign_key_from_notification_settings_to_users.rb
new file mode 100644
index 00000000000..91656f194e5
--- /dev/null
+++ b/db/migrate/20180710162338_add_foreign_key_from_notification_settings_to_users.rb
@@ -0,0 +1,30 @@
+class AddForeignKeyFromNotificationSettingsToUsers < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  class NotificationSetting < ActiveRecord::Base
+    self.table_name = 'notification_settings'
+
+    include EachBatch
+  end
+
+  class User < ActiveRecord::Base
+    self.table_name = 'users'
+  end
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    NotificationSetting.each_batch(of: 1000) do |batch|
+      batch.where('NOT EXISTS (?)', User.select(1).where('users.id = notification_settings.user_id'))
+        .delete_all
+    end
+
+    add_concurrent_foreign_key(:notification_settings, :users, column: :user_id, on_delete: :cascade)
+  end
+
+  def down
+    remove_foreign_key(:notification_settings, column: :user_id)
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 769baa825a5..2bef2971f29 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -2350,6 +2350,7 @@ ActiveRecord::Schema.define(version: 20180726172057) do
   add_foreign_key "milestones", "projects", name: "fk_9bd0a0c791", on_delete: :cascade
   add_foreign_key "note_diff_files", "notes", column: "diff_note_id", on_delete: :cascade
   add_foreign_key "notes", "projects", name: "fk_99e097b079", on_delete: :cascade
+  add_foreign_key "notification_settings", "users", name: "fk_0c95e91db7", on_delete: :cascade
   add_foreign_key "oauth_openid_requests", "oauth_access_grants", column: "access_grant_id", name: "fk_oauth_openid_requests_oauth_access_grants_access_grant_id"
   add_foreign_key "pages_domains", "projects", name: "fk_ea2f6dfc6f", on_delete: :cascade
   add_foreign_key "personal_access_tokens", "users"
diff --git a/spec/migrations/add_foreign_key_from_notification_settings_to_users_spec.rb b/spec/migrations/add_foreign_key_from_notification_settings_to_users_spec.rb
new file mode 100644
index 00000000000..656d4f75e3b
--- /dev/null
+++ b/spec/migrations/add_foreign_key_from_notification_settings_to_users_spec.rb
@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20180710162338_add_foreign_key_from_notification_settings_to_users.rb')
+
+describe AddForeignKeyFromNotificationSettingsToUsers, :migration do
+  let(:notification_settings) { table(:notification_settings) }
+  let(:users) { table(:users) }
+  let(:projects) { table(:projects) }
+
+  before do
+    users.create!(email: 'email@email.com', name: 'foo', username: 'foo', projects_limit: 0)
+    projects.create!(name: 'gitlab', path: 'gitlab-org/gitlab-ce', namespace_id: 1)
+  end
+
+  describe 'removal of orphans without user' do
+    let!(:notification_setting_without_user) { create_notification_settings!(user_id: 123) }
+    let!(:notification_setting_with_user) { create_notification_settings!(user_id: users.last.id) }
+
+    it 'removes orphaned notification_settings without user' do
+      expect { migrate! }.to change { notification_settings.count }.by(-1)
+    end
+
+    it "doesn't remove notification_settings with valid user" do
+      expect { migrate! }.not_to change { notification_setting_with_user.reload }
+    end
+  end
+
+  def create_notification_settings!(**opts)
+    notification_settings.create!(
+      source_id: projects.last.id,
+      source_type: 'Project',
+      user_id: users.last.id,
+      **opts)
+  end
+end