FIX: CategoryUser#batch_set (#7787)

* Remove unused method

* Prefabricate user in category_user_spec.rb

* FIX: Remove notification_level from category_users unique indexes

* FIX: CategoryUser#batch_set wasn't updating pre-existing records

* Improve tests for CategoryUser#batch_set

* FIX: changed was being reported incorrectly

* DEV: Rewrote query to do a bulk insert

* DEV: remove unnecessary parentheses

app/models/category_user.rb

@@ -10,10 +10,6 @@ class CategoryUser < ActiveRecord::Base
     self.where(user: user, notification_level: notification_levels[level])
   end
 
-  def self.lookup_by_category(user, category)
-    self.where(user: user, category: category)
-  end
-
   def self.notification_levels
     NotificationLevels.all
   end
@@ -23,21 +19,43 @@ class CategoryUser < ActiveRecord::Base
   end
 
   def self.batch_set(user, level, category_ids)
-    records = CategoryUser.where(user: user, notification_level: notification_levels[level])
-    old_ids = records.pluck(:category_id)
+    level_num = notification_levels[level]
+    category_ids = Category.where(id: category_ids).pluck(:id)
+
     changed = false
 
-    category_ids = Category.where('id in (?)', category_ids).pluck(:id)
-
-    remove = (old_ids - category_ids)
-    if remove.present?
-      records.where('category_id in (?)', remove).destroy_all
-      changed = true
+    # Update pre-existing category users
+    unless category_ids.empty?
+      changed ||=
+        CategoryUser
+          .where(user_id: user.id, category_id: category_ids)
+          .where.not(notification_level: level_num)
+          .update_all(notification_level: level_num) > 0
     end
 
-    (category_ids - old_ids).each do |id|
-      CategoryUser.create!(user: user, category_id: id, notification_level: notification_levels[level])
-      changed = true
+    # Remove extraneous category users
+    changed ||=
+      CategoryUser
+        .where(user_id: user.id, notification_level: level_num)
+        .where.not(category_id: category_ids)
+        .delete_all > 0
+
+    # Create new category users
+    unless category_ids.empty?
+      # TODO: rewrite this when we upgrade to rails 6
+      changed ||=
+        DB.exec(
+          "
+            INSERT INTO category_users (user_id, category_id, notification_level)
+            SELECT :user_id, category_id, :level_num
+            FROM
+              (VALUES (:category_ids)) AS category_ids (category_id)
+            ON CONFLICT DO NOTHING
+          ",
+          user_id: user.id,
+          level_num: level_num,
+          category_ids: category_ids
+        ) > 0
     end
 
     if changed
@@ -188,6 +206,6 @@ end
 #
 # Indexes
 #
-#  idx_category_users_u1  (user_id,category_id,notification_level) UNIQUE
-#  idx_category_users_u2  (category_id,user_id,notification_level) UNIQUE
+#  idx_category_users_category_id_user_id  (category_id,user_id) UNIQUE
+#  idx_category_users_user_id_category_id  (user_id,category_id) UNIQUE
 #

db/migrate/20190621095105_remove_notification_level_from_category_user_indexes.rb

@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+class RemoveNotificationLevelFromCategoryUserIndexes < ActiveRecord::Migration[5.2]
+  def up
+    execute <<SQL
+DELETE FROM category_users cu USING category_users cu1
+  WHERE cu.user_id = cu1.user_id AND
+        cu.category_id = cu1.category_id AND
+        cu.notification_level < cu1.notification_level
+SQL
+
+    remove_index :category_users, name: 'idx_category_users_u1'
+    remove_index :category_users, name: 'idx_category_users_u2'
+
+    add_index :category_users, [:user_id, :category_id],
+        name: 'idx_category_users_user_id_category_id', unique: true
+    add_index :category_users, [:category_id, :user_id],
+        name: 'idx_category_users_category_id_user_id', unique: true
+  end
+
+  def down
+    remove_index :category_users, name: 'idx_category_users_user_id_category_id'
+    remove_index :category_users, name: 'idx_category_users_category_id_user_id'
+
+    add_index :category_users, [:user_id, :category_id, :notification_level],
+        name: 'idx_category_users_u1', unique: true
+    add_index :category_users, [:category_id, :user_id, :notification_level],
+        name: 'idx_category_users_u2', unique: true
+  end
+end

spec/models/category_user_spec.rb

@@ -5,6 +5,7 @@ require 'rails_helper'
 require_dependency 'post_creator'
 
 describe CategoryUser do
+  fab!(:user) { Fabricate(:user) }
 
   def tracking
     CategoryUser.notification_levels[:tracking]
@@ -14,26 +15,60 @@ describe CategoryUser do
     CategoryUser.notification_levels[:regular]
   end
 
-  it 'allows batch set' do
-    user = Fabricate(:user)
-    category1 = Fabricate(:category)
-    category2 = Fabricate(:category)
+  context '#batch_set' do
+    fab!(:category) { Fabricate(:category) }
 
-    watching = CategoryUser.where(user_id: user.id, notification_level: CategoryUser.notification_levels[:watching])
+    def category_ids_at_level(level)
+      CategoryUser.where(
+        user_id: user.id,
+        notification_level: CategoryUser.notification_levels[level]
+      ).pluck(:category_id)
+    end
 
-    CategoryUser.batch_set(user, :watching, [category1.id, category2.id])
-    expect(watching.pluck(:category_id).sort).to eq [category1.id, category2.id]
+    it "should add new records where required" do
+      CategoryUser.batch_set(user, :watching, [category.id])
 
-    CategoryUser.batch_set(user, :watching, [])
-    expect(watching.count).to eq 0
+      expect(category_ids_at_level(:watching)).to eq([category.id])
+    end
 
-    CategoryUser.batch_set(user, :watching, [category2.id])
-    expect(watching.count).to eq 1
+    it "should change existing records where required" do
+      CategoryUser.create!(
+        user_id: user.id,
+        category_id: category.id,
+        notification_level: CategoryUser.notification_levels[:muted]
+      )
+
+      CategoryUser.batch_set(user, :watching, [category.id])
+
+      expect(category_ids_at_level(:watching)).to eq([category.id])
+      expect(category_ids_at_level(:muted)).to eq([])
+    end
+
+    it "should delete extraneous records where required" do
+      CategoryUser.create!(
+        user_id: user.id,
+        category_id: category.id,
+        notification_level: CategoryUser.notification_levels[:watching]
+      )
+
+      CategoryUser.batch_set(user, :watching, [])
+
+      expect(category_ids_at_level(:watching)).to eq([])
+    end
+
+    it "should return true when something changed" do
+      expect(CategoryUser.batch_set(user, :watching, [category.id])).to eq(true)
+    end
+
+    it "should return false when nothing changed" do
+      CategoryUser.batch_set(user, :watching, [category.id])
+
+      expect(CategoryUser.batch_set(user, :watching, [category.id])).to eq(false)
+    end
   end
 
   it 'should correctly auto_track' do
     tracking_user = Fabricate(:user)
-    user = Fabricate(:user)
     topic = Fabricate(:post).topic
 
     TopicUser.change(user.id, topic.id, total_msecs_viewed: 10)
@@ -48,7 +83,6 @@ describe CategoryUser do
 
   it 'allows updating notification level' do
     category = Fabricate(:category)
-    user = Fabricate(:user)
 
     CategoryUser.set_notification_level_for_category(user,
                                                      NotificationLevels.all[:watching_first_post],
@@ -78,8 +112,6 @@ describe CategoryUser do
       muted_category = Fabricate(:category)
       tracked_category = Fabricate(:category)
 
-      user = Fabricate(:user)
-
       early_watched_post = create_post(category: watched_category)
 
       CategoryUser.create!(user: user, category: watched_category, notification_level: CategoryUser.notification_levels[:watching])
@@ -104,8 +136,6 @@ describe CategoryUser do
     end
 
     it "topics that move to a tracked category should auto track" do
-      user = Fabricate(:user)
-
       first_post = create_post
       tracked_category = first_post.topic.category
 
@@ -120,7 +150,6 @@ describe CategoryUser do
     end
 
     it "unwatches categories that have been changed" do
-      user = Fabricate(:user)
       watched_category = Fabricate(:category)
       CategoryUser.create!(user: user, category: watched_category, notification_level: CategoryUser.notification_levels[:watching])
 
@@ -141,7 +170,6 @@ describe CategoryUser do
       # this is done so as to maintain topic notification state when topic category is changed and the new category has same notification level for the user
       # see: https://meta.discourse.org/t/changing-topic-from-one-watched-category-to-another-watched-category-makes-topic-new-again/36517/15
 
-      user = Fabricate(:user)
       watched_category_1 = Fabricate(:category)
       watched_category_2 = Fabricate(:category)
       category_3 = Fabricate(:category)
@@ -167,7 +195,6 @@ describe CategoryUser do
     end
 
     it "deletes TopicUser record when topic category is changed, and new category has different notification level" do
-      user = Fabricate(:user)
       watched_category = Fabricate(:category)
       tracked_category = Fabricate(:category)
       CategoryUser.create!(user: user, category: watched_category, notification_level: CategoryUser.notification_levels[:watching])
@@ -183,7 +210,6 @@ describe CategoryUser do
     end
 
     it "is destroyed when a user is deleted" do
-      user = Fabricate(:user)
       category = Fabricate(:category)
 
       CategoryUser.create!(user: user, category: category, notification_level: CategoryUser.notification_levels[:watching])