From 5140ec9acf29df220ec00cc6a704b73773a86bdc Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 3 Nov 2020 12:38:54 +0000 Subject: [PATCH] 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 --- .../controllers/preferences/notifications.js | 1 - .../app/controllers/preferences/users.js | 1 - app/controllers/users_controller.rb | 1 - .../scheduled/purge_expired_ignored_users.rb | 1 - app/models/ignored_user.rb | 2 + app/services/user_updater.rb | 26 ---------- ...103103401_add_not_null_to_ignored_users.rb | 17 ++++++ ...red_user.rb => ignored_user_fabricator.rb} | 1 + spec/jobs/purge_expired_ignored_users_spec.rb | 13 ----- .../post_action_users_controller_spec.rb | 2 +- spec/services/post_alerter_spec.rb | 2 +- spec/services/user_merger_spec.rb | 14 ++--- spec/services/user_updater_spec.rb | 52 ------------------- 13 files changed, 29 insertions(+), 104 deletions(-) create mode 100644 db/migrate/20201103103401_add_not_null_to_ignored_users.rb rename spec/fabricators/{ignored_user.rb => ignored_user_fabricator.rb} (68%) diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/notifications.js b/app/assets/javascripts/discourse/app/controllers/preferences/notifications.js index 7c78bdf0b61..e231d043f95 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/notifications.js +++ b/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", diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/users.js b/app/assets/javascripts/discourse/app/controllers/preferences/users.js index 7ea9a5d0ef4..f4b1173c02f 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/users.js +++ b/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", ]; diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index b3fdf66926b..a98c986819a 100644 --- a/app/controllers/users_controller.rb +++ b/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, diff --git a/app/jobs/scheduled/purge_expired_ignored_users.rb b/app/jobs/scheduled/purge_expired_ignored_users.rb index 0c2a87c8c99..e53ecac2be2 100644 --- a/app/jobs/scheduled/purge_expired_ignored_users.rb +++ b/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 diff --git a/app/models/ignored_user.rb b/app/models/ignored_user.rb index e298fd316d6..3a334e8fc40 100644 --- a/app/models/ignored_user.rb +++ b/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 diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb index 713fc2b9999..9471f0c8d86 100644 --- a/app/services/user_updater.rb +++ b/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 } diff --git a/db/migrate/20201103103401_add_not_null_to_ignored_users.rb b/db/migrate/20201103103401_add_not_null_to_ignored_users.rb new file mode 100644 index 00000000000..ba0ecdcb953 --- /dev/null +++ b/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 diff --git a/spec/fabricators/ignored_user.rb b/spec/fabricators/ignored_user_fabricator.rb similarity index 68% rename from spec/fabricators/ignored_user.rb rename to spec/fabricators/ignored_user_fabricator.rb index 39447499d77..a06e6b021d9 100644 --- a/spec/fabricators/ignored_user.rb +++ b/spec/fabricators/ignored_user_fabricator.rb @@ -2,4 +2,5 @@ Fabricator(:ignored_user) do user + expiring_at 4.months.from_now end diff --git a/spec/jobs/purge_expired_ignored_users_spec.rb b/spec/jobs/purge_expired_ignored_users_spec.rb index e0627de9596..ce28b15072a 100644 --- a/spec/jobs/purge_expired_ignored_users_spec.rb +++ b/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") } diff --git a/spec/requests/post_action_users_controller_spec.rb b/spec/requests/post_action_users_controller_spec.rb index ec6bc28592b..97c4f489d20 100644 --- a/spec/requests/post_action_users_controller_spec.rb +++ b/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] diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index 15f631ca521..0b8ab126d9c 100644 --- a/spec/services/post_alerter_spec.rb +++ b/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) diff --git a/spec/services/user_merger_spec.rb b/spec/services/user_merger_spec.rb index b7a8aef5071..88b704590c3 100644 --- a/spec/services/user_merger_spec.rb +++ b/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) - IgnoredUser.create!(user_id: source_user.id, ignored_user_id: ignored2.id) - IgnoredUser.create!(user_id: target_user.id, ignored_user_id: ignored2.id) - IgnoredUser.create!(user_id: target_user.id, ignored_user_id: ignored3.id) - IgnoredUser.create!(user_id: walter.id, ignored_user_id: source_user.id) - IgnoredUser.create!(user_id: coding_horror.id, ignored_user_id: source_user.id) - IgnoredUser.create!(user_id: coding_horror.id, ignored_user_id: target_user.id) + Fabricate(:ignored_user, user: source_user, ignored_user: ignored1) + Fabricate(:ignored_user, user: source_user, ignored_user: ignored2) + Fabricate(:ignored_user, user: target_user, ignored_user: ignored2) + Fabricate(:ignored_user, user: target_user, ignored_user: ignored3) + Fabricate(:ignored_user, user: walter, ignored_user: source_user) + Fabricate(:ignored_user, user: coding_horror, ignored_user: source_user) + Fabricate(:ignored_user, user: coding_horror, ignored_user: target_user) merge_users! diff --git a/spec/services/user_updater_spec.rb b/spec/services/user_updater_spec.rb index e36a44d744b..b9163a2a8c6 100644 --- a/spec/services/user_updater_spec.rb +++ b/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) }