From cc873977ec37b77bb35b64f17286f055a53b254d Mon Sep 17 00:00:00 2001
From: Natalie Tay <natalie.tay@discourse.org>
Date: Tue, 3 Sep 2024 15:43:02 +0800
Subject: [PATCH] DEV: Ensure unique notification level per tag user (#28638)

TagUser.rb is used to set user notification levels for a tag, we don't have a unique index on the notification level itself. This means that there might be some weird case where a user may have multiple of the same notification level on a tag.

This PR adds a migration which de-duplicates this based on defaults, where we keep the earliest record in the event there is multiple notification level per-user-per-tag.
---
 app/models/tag_user.rb                        |  5 +++--
 ...sure_unique_tag_user_notification_level.rb | 19 +++++++++++++++++++
 ...nique_constraint_from_tag_users_indexes.rb | 16 ++++++++++++++++
 spec/fabricators/tag_user_fabricator.rb       |  7 +++++++
 4 files changed, 45 insertions(+), 2 deletions(-)
 create mode 100644 db/migrate/20240829083823_ensure_unique_tag_user_notification_level.rb
 create mode 100644 db/migrate/20240829142639_remove_unique_constraint_from_tag_users_indexes.rb
 create mode 100644 spec/fabricators/tag_user_fabricator.rb

diff --git a/app/models/tag_user.rb b/app/models/tag_user.rb
index 04993eef4ad..f91ad7ffb48 100644
--- a/app/models/tag_user.rb
+++ b/app/models/tag_user.rb
@@ -259,6 +259,7 @@ end
 #
 # Indexes
 #
-#  idx_tag_users_ix1  (user_id,tag_id,notification_level) UNIQUE
-#  idx_tag_users_ix2  (tag_id,user_id,notification_level) UNIQUE
+#  index_tag_users_on_tag_id_and_user_id_and_notification_level  (tag_id,user_id,notification_level)
+#  index_tag_users_on_user_id_and_tag_id                         (user_id,tag_id) UNIQUE
+#  index_tag_users_on_user_id_and_tag_id_and_notification_level  (user_id,tag_id,notification_level)
 #
diff --git a/db/migrate/20240829083823_ensure_unique_tag_user_notification_level.rb b/db/migrate/20240829083823_ensure_unique_tag_user_notification_level.rb
new file mode 100644
index 00000000000..72f976ebd90
--- /dev/null
+++ b/db/migrate/20240829083823_ensure_unique_tag_user_notification_level.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+class EnsureUniqueTagUserNotificationLevel < ActiveRecord::Migration[7.1]
+  def up
+    execute <<-SQL
+      DELETE FROM tag_users
+      USING tag_users AS dupe
+      WHERE tag_users.id > dupe.id
+      AND tag_users.user_id = dupe.user_id
+      AND tag_users.tag_id = dupe.tag_id;
+    SQL
+
+    add_index :tag_users, %i[user_id tag_id], unique: true
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/db/migrate/20240829142639_remove_unique_constraint_from_tag_users_indexes.rb b/db/migrate/20240829142639_remove_unique_constraint_from_tag_users_indexes.rb
new file mode 100644
index 00000000000..22795fc7d03
--- /dev/null
+++ b/db/migrate/20240829142639_remove_unique_constraint_from_tag_users_indexes.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+class RemoveUniqueConstraintFromTagUsersIndexes < ActiveRecord::Migration[7.1]
+  disable_ddl_transaction!
+
+  def up
+    remove_index :tag_users, name: :idx_tag_users_ix1, algorithm: :concurrently
+    remove_index :tag_users, name: :idx_tag_users_ix2, algorithm: :concurrently
+
+    add_index :tag_users, %i[user_id tag_id notification_level], algorithm: :concurrently
+    add_index :tag_users, %i[tag_id user_id notification_level], algorithm: :concurrently
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/spec/fabricators/tag_user_fabricator.rb b/spec/fabricators/tag_user_fabricator.rb
new file mode 100644
index 00000000000..038da79a3f8
--- /dev/null
+++ b/spec/fabricators/tag_user_fabricator.rb
@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+Fabricator(:tag_user) do
+  user
+  tag
+  notification_level { TagUser.notification_levels[:tracking] }
+end