DEV: Cleanup ignored user logic (#11107)

- IgnoredUser records should all now have an expiring_at value. This commit enforces that in the DB, and fixes any corrupt rows
- Changes to the ignored user list are now handled by the `/u/{username}/notification_level` endpoint. This allows setting expiration dates on the ignore. This commit removes the old logic for saving a list of usernames in the user preferences.
- Many specs were calling `IgnoredUser.create`. This commit changes them to use `Fabricate(:ignored_user)` for consistency

app/assets/javascripts/discourse/app/controllers/preferences/notifications.js

@@ -9,7 +9,6 @@ export default Controller.extend({
 
     this.saveAttrNames = [
       "muted_usernames",
-      "ignored_usernames",
       "new_topic_duration_minutes",
       "auto_track_topics_after_msecs",
       "notification_level_when_replying",

app/assets/javascripts/discourse/app/controllers/preferences/users.js

@@ -43,7 +43,6 @@ export default Controller.extend({
 
     this.saveAttrNames = [
       "muted_usernames",
-      "ignored_usernames",
       "allowed_pm_usernames",
       "enable_allowed_pm_users",
     ];

app/controllers/users_controller.rb

@@ -1586,7 +1586,6 @@ class UsersController < ApplicationController
       :title,
       :date_of_birth,
       :muted_usernames,
-      :ignored_usernames,
       :allowed_pm_usernames,
       :theme_ids,
       :locale,

app/jobs/scheduled/purge_expired_ignored_users.rb

@@ -6,7 +6,6 @@ module Jobs
 
     def execute(args)
       IgnoredUser.where("expiring_at <= ?", Time.zone.now).delete_all
-      IgnoredUser.where("created_at <= ? AND expiring_at IS NULL", 4.months.ago).delete_all
     end
   end
 end

app/models/ignored_user.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 class IgnoredUser < ActiveRecord::Base
+  validates :expiring_at, presence: true
+
   belongs_to :user
   belongs_to :ignored_user, class_name: "User"
 end

app/services/user_updater.rb

@@ -165,10 +165,6 @@ class UserUpdater
         update_muted_users(attributes[:muted_usernames])
       end
 
-      if attributes.key?(:ignored_usernames)
-        update_ignored_users(attributes[:ignored_usernames])
-      end
-
       if attributes.key?(:allowed_pm_usernames)
         update_allowed_pm_users(attributes[:allowed_pm_usernames])
       end
@@ -212,28 +208,6 @@ class UserUpdater
     end
   end
 
-  def update_ignored_users(usernames)
-    return unless guardian.can_ignore_users?
-
-    usernames ||= ""
-    desired_usernames = usernames.split(",").reject { |username| user.username == username }
-    desired_ids = User.where(username: desired_usernames).where(admin: false, moderator: false).pluck(:id)
-    if desired_ids.empty?
-      IgnoredUser.where(user_id: user.id).destroy_all
-    else
-      IgnoredUser.where('user_id = ? AND ignored_user_id not in (?)', user.id, desired_ids).destroy_all
-
-      # SQL is easier here than figuring out how to do the same in AR
-      DB.exec(<<~SQL, now: Time.now, user_id: user.id, desired_ids: desired_ids)
-        INSERT into ignored_users(user_id, ignored_user_id, created_at, updated_at)
-        SELECT :user_id, id, :now, :now
-        FROM users
-        WHERE id in (:desired_ids)
-        ON CONFLICT DO NOTHING
-      SQL
-    end
-  end
-
   def update_allowed_pm_users(usernames)
     usernames ||= ""
     desired_usernames = usernames.split(",").reject { |username| user.username == username }

db/migrate/20201103103401_add_not_null_to_ignored_users.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class AddNotNullToIgnoredUsers < ActiveRecord::Migration[6.0]
+  def up
+    execute <<~SQL
+      UPDATE ignored_users
+      SET expiring_at = created_at + interval '4 months'
+      WHERE expiring_at IS NULL
+    SQL
+
+    change_column_null :ignored_users, :expiring_at, false
+  end
+
+  def down
+    change_column_null :ignored_users, :expiring_at, true
+  end
+end

spec/fabricators/ignored_user_fabricator.rb

@@ -2,4 +2,5 @@
 
 Fabricator(:ignored_user) do
   user
+  expiring_at 4.months.from_now
 end

spec/jobs/purge_expired_ignored_users_spec.rb

@@ -27,19 +27,6 @@ describe Jobs::PurgeExpiredIgnoredUsers do
       end
     end
 
-    context "when there are expired ignored users" do
-      fab!(:fred) { Fabricate(:user, username: "fred") }
-
-      it "purges expired ignored users" do
-        freeze_time(5.months.ago) do
-          Fabricate(:ignored_user, user: tarek, ignored_user: fred)
-        end
-
-        subject
-        expect(IgnoredUser.find_by(ignored_user: fred)).to be_nil
-      end
-    end
-
     context "when there are expired ignored users by expiring_at" do
       fab!(:fred) { Fabricate(:user, username: "fred") }
 

spec/requests/post_action_users_controller_spec.rb

@@ -64,7 +64,7 @@ describe PostActionUsersController do
     PostActionCreator.like(ignored_user, post)
     regular_user = Fabricate(:user)
     PostActionCreator.like(regular_user, post)
-    IgnoredUser.create(user: user, ignored_user: ignored_user)
+    Fabricate(:ignored_user, user: user, ignored_user: ignored_user)
 
     get "/post_action_users.json", params: {
       id: post.id, post_action_type_id: PostActionType.types[:like]

spec/services/post_alerter_spec.rb

@@ -222,7 +222,7 @@ describe PostAlerter do
 
     it 'does not notify for ignored users' do
       post = Fabricate(:post, raw: '[quote="EvilTrout, post:1"]whatup[/quote]', topic: topic)
-      IgnoredUser.create!(user_id: evil_trout.id, ignored_user_id: post.user_id)
+      Fabricate(:ignored_user, user: evil_trout, ignored_user: post.user)
 
       expect {
         PostAlerter.post_created(post)

spec/services/user_merger_spec.rb

@@ -299,13 +299,13 @@ describe UserMerger do
     ignored3 = Fabricate(:user)
     coding_horror = Fabricate(:coding_horror)
 
-    IgnoredUser.create!(user_id: source_user.id, ignored_user_id: ignored1.id)
+    Fabricate(:ignored_user, user: source_user, ignored_user: ignored1)
-    IgnoredUser.create!(user_id: source_user.id, ignored_user_id: ignored2.id)
+    Fabricate(:ignored_user, user: source_user, ignored_user: ignored2)
-    IgnoredUser.create!(user_id: target_user.id, ignored_user_id: ignored2.id)
+    Fabricate(:ignored_user, user: target_user, ignored_user: ignored2)
-    IgnoredUser.create!(user_id: target_user.id, ignored_user_id: ignored3.id)
+    Fabricate(:ignored_user, user: target_user, ignored_user: ignored3)
-    IgnoredUser.create!(user_id: walter.id, ignored_user_id: source_user.id)
+    Fabricate(:ignored_user, user: walter, ignored_user: source_user)
-    IgnoredUser.create!(user_id: coding_horror.id, ignored_user_id: source_user.id)
+    Fabricate(:ignored_user, user: coding_horror, ignored_user: source_user)
-    IgnoredUser.create!(user_id: coding_horror.id, ignored_user_id: target_user.id)
+    Fabricate(:ignored_user, user: coding_horror, ignored_user: target_user)
 
     merge_users!
 

spec/services/user_updater_spec.rb

@@ -36,58 +36,6 @@ describe UserUpdater do
     end
   end
 
-  describe '#update_ignored_users' do
-    it 'updates ignored users' do
-      u1 = Fabricate(:user, trust_level: 2)
-      u2 = Fabricate(:user, trust_level: 2)
-      u3 = Fabricate(:user, trust_level: 2)
-
-      updater = UserUpdater.new(u1, u1)
-      updater.update_ignored_users("#{u2.username},#{u3.username}")
-
-      updater = UserUpdater.new(u2, u2)
-      updater.update_ignored_users("#{u3.username},#{u1.username}")
-
-      updater = UserUpdater.new(u3, u3)
-      updater.update_ignored_users("")
-
-      expect(IgnoredUser.where(user_id: u2.id).pluck(:ignored_user_id)).to match_array([u3.id, u1.id])
-      expect(IgnoredUser.where(user_id: u1.id).pluck(:ignored_user_id)).to match_array([u2.id, u3.id])
-      expect(IgnoredUser.where(user_id: u3.id).count).to eq(0)
-    end
-
-    it 'excludes acting user' do
-      u1 = Fabricate(:user, trust_level: 2)
-      u2 = Fabricate(:user)
-      updater = UserUpdater.new(u1, u1)
-      updater.update_ignored_users("#{u1.username},#{u2.username}")
-
-      expect(IgnoredUser.where(user_id: u1.id).pluck(:ignored_user_id)).to match_array([u2.id])
-    end
-
-    context 'when acting user\'s trust level is below tl2' do
-      it 'excludes acting user' do
-        u1 = Fabricate(:user, trust_level: 1)
-        u2 = Fabricate(:user)
-        updater = UserUpdater.new(u1, u1)
-        updater.update_ignored_users("#{u2.username}")
-
-        expect(IgnoredUser.where(ignored_user_id: u2.id).count).to eq(0)
-      end
-    end
-
-    context 'when acting user is admin' do
-      it 'excludes acting user' do
-        u1 = Fabricate(:admin)
-        u2 = Fabricate(:user)
-        updater = UserUpdater.new(u1, u1)
-        updater.update_ignored_users("#{u1.username},#{u2.username}")
-
-        expect(IgnoredUser.where(user_id: u1.id).pluck(:ignored_user_id)).to match_array([u2.id])
-      end
-    end
-  end
-
   describe '#update' do
     fab!(:category) { Fabricate(:category) }
     fab!(:tag) { Fabricate(:tag) }