FIX: CategoryUser#batch_set (#7787)

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

diff --git a/app/models/category_user.rb b/app/models/category_user.rb
index 974f09e..6ce68c5 100644
--- a/app/models/category_user.rb
+++ b/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)
-    changed = false
+    level_num = notification_levels[level]
+    category_ids = Category.where(id: category_ids).pluck(:id)
 
-    category_ids = Category.where('id in (?)', category_ids).pluck(:id)
+    changed = false
 
-    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
 #
diff --git a/db/migrate/20190621095105_remove_notification_level_from_category_user_indexes.rb b/db/migrate/20190621095105_remove_notification_level_from_category_user_indexes.rb
new file mode 100644
index 0000000..587e2cf
--- /dev/null
+++ b/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
diff --git a/spec/models/category_user_spec.rb b/spec/models/category_user_spec.rb
index f87bec3..a386a57 100644
--- a/spec/models/category_user_spec.rb
+++ b/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
+
+    it "should add new records where required" do
+      CategoryUser.batch_set(user, :watching, [category.id])
+
+      expect(category_ids_at_level(:watching)).to eq([category.id])
+    end
+
+    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
 
-    CategoryUser.batch_set(user, :watching, [category1.id, category2.id])
-    expect(watching.pluck(:category_id).sort).to eq [category1.id, category2.id]
+    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(watching.count).to eq 0
+      CategoryUser.batch_set(user, :watching, [])
 
-    CategoryUser.batch_set(user, :watching, [category2.id])
-    expect(watching.count).to eq 1
+      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

[... diff too long, it was truncated ...]

GitHub sha: bc03c509

1 Like

DEV: Correct batch setting of categories