From e36cf51a9e1d328368b8fbe337893d8f3f3fc59b Mon Sep 17 00:00:00 2001 From: Jess Lee Date: Wed, 21 Nov 2018 16:55:26 -0500 Subject: [PATCH] Refine Banish User (#1168) * add banish user test * refactor banish user wip * banish user wip - bust comment cache synchronously * add more spec scenarios * Update Banish#strip_user_profile * Refactor specs --- app/controllers/internal/users_controller.rb | 12 +-- app/models/article.rb | 7 +- app/models/comment.rb | 6 +- app/models/reaction.rb | 2 +- app/models/user.rb | 10 +- app/services/moderator/banisher.rb | 94 +++++++++++++++++++ .../internal_users_controller_spec.rb | 70 ++++++++++++++ 7 files changed, 186 insertions(+), 15 deletions(-) create mode 100644 app/services/moderator/banisher.rb create mode 100644 spec/controllers/internal_users_controller_spec.rb diff --git a/app/controllers/internal/users_controller.rb b/app/controllers/internal/users_controller.rb index 51befcc1c..1a18509e2 100644 --- a/app/controllers/internal/users_controller.rb +++ b/app/controllers/internal/users_controller.rb @@ -92,7 +92,7 @@ class Internal::UsersController < Internal::ApplicationController def banish @user = User.find(params[:id]) - strip_user(@user) + Moderator::Banisher.call(admin: current_user, offender: @user) redirect_to "/internal/users/#{@user.id}/edit" end @@ -151,10 +151,10 @@ class Internal::UsersController < Internal::ApplicationController def user_params params.require(:user).permit(:seeking_mentorship, - :offering_mentorship, - :add_mentor, - :add_mentee, - :mentorship_note, - :ban_from_mentorship) + :offering_mentorship, + :add_mentor, + :add_mentee, + :mentorship_note, + :ban_from_mentorship) end end diff --git a/app/models/article.rb b/app/models/article.rb index 3382cacad..1b760db51 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -13,10 +13,10 @@ class Article < ApplicationRecord counter_culture :user belongs_to :organization, optional: true belongs_to :collection, optional: true + has_many :reactions, as: :reactable, dependent: :destroy has_many :comments, as: :commentable has_many :buffer_updates - has_many :reactions, as: :reactable, dependent: :destroy - has_many :notifications, as: :notifiable + has_many :notifications, as: :notifiable validates :slug, presence: { if: :published? }, format: /\A[0-9a-z-]*\z/, uniqueness: { scope: :user_id } @@ -392,8 +392,7 @@ class Article < ApplicationRecord def before_destroy_actions bust_cache remove_algolia_index - reactions.destroy_all - user.delay.resave_articles + user.cache_bust_all_articles organization&.delay&.resave_articles end diff --git a/app/models/comment.rb b/app/models/comment.rb index 0998040bd..05263d2f4 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -281,18 +281,18 @@ class Comment < ApplicationRecord handle_asynchronously :create_first_reaction def before_destroy_actions - bust_cache + bust_cache_without_delay remove_algolia_index - reactions.destroy_all end def bust_cache - Comment.delay.comment_async_bust(commentable, user.username) + Comment.comment_async_bust(commentable, user.username) expire_root_fragment cache_buster = CacheBuster.new cache_buster.bust(commentable.path.to_s) if commentable cache_buster.bust("#{commentable.path}/comments") if commentable end + handle_asynchronously :bust_cache def send_email_notification NotifyMailer.new_reply_email(self).deliver diff --git a/app/models/reaction.rb b/app/models/reaction.rb index a3da6fa18..fdc81ed9f 100644 --- a/app/models/reaction.rb +++ b/app/models/reaction.rb @@ -15,9 +15,9 @@ class Reaction < ApplicationRecord before_save :assign_points after_save :update_reactable - before_destroy :update_reactable_without_delay after_save :touch_user after_save :async_bust + before_destroy :update_reactable_without_delay before_destroy :clean_up_before_destroy scope :for_article, ->(id) { where(reactable_id: id, reactable_type: "Article") } diff --git a/app/models/user.rb b/app/models/user.rb index dc57821fd..18f636f9f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -356,6 +356,14 @@ class User < ApplicationRecord end end + def cache_bust_all_articles + cache_buster = CacheBuster.new + articles.each do |article| + cache_buster.bust(article.path) + cache_buster.bust(article.path + "?i=i") + end + end + def settings_tab_list tab_list = %w( Profile @@ -428,7 +436,7 @@ class User < ApplicationRecord end def conditionally_resave_articles - if core_profile_details_changed? + if core_profile_details_changed? && !user.banned delay.resave_articles end end diff --git a/app/services/moderator/banisher.rb b/app/services/moderator/banisher.rb new file mode 100644 index 000000000..0fb4d259a --- /dev/null +++ b/app/services/moderator/banisher.rb @@ -0,0 +1,94 @@ +module Moderator + class Banisher + attr_reader :user, :admin + + def self.call(admin:, offender:) + new(offender: offender, admin: admin).banish + end + + def initialize(admin:, offender:) + @user = offender + @admin = admin + end + + def banish + # return unless user.comments.where("created_at < ?", 7.days.ago).empty? + ban_offender + strip_user_profile + destroy_dependents + user.remove_from_index! + end + + def destroy_dependents + destroy_reactions + destroy_comments + destroy_articles + destroy_follows + end + + def ban_offender + user.add_role :banned + unless user.notes.where(reason: "banned").any? + user.notes. + create!(reason: "banned", content: "spam account", author_id: admin.id) + end + end + + def destroy_follows + user.follows.destroy_all + end + handle_asynchronously :destroy_follows + + def destroy_reactions + user.reactions.destroy_all + end + + def destroy_comments + user.comments.destroy_all + end + handle_asynchronously :destroy_comments + + def destroy_articles + user.articles.destroy_all + end + handle_asynchronously :destroy_articles + + def bust_user_cache(old_username) + CacheBuster.new.bust("/#{old_username}") + end + + def strip_user_profile + bust_user_cache(user.username) + new_name = "spam_#{rand(10000)}" + new_username = "spam_#{rand(10000)}" + if User.find_by(name: new_name) || User.find_by(username: new_username) + new_name = "spam_#{rand(10000)}" + new_username = "spam_#{rand(10000)}" + end + user.name = new_name + user.username = new_username + user.twitter_username = "" + user.github_username = "" + user.website_url = "" + user.summary = "" + user.location = "" + user.education = "" + user.employer_name = "" + user.employer_url = "" + user.employment_title = "" + user.mostly_work_with = "" + user.currently_learning = "" + user.currently_hacking_on = "" + user.available_for = "" + user.email_public = false + user.facebook_url = nil + user.dribbble_url = nil + user.medium_url = nil + user.stackoverflow_url = nil + user.behance_url = nil + user.linkedin_url = nil + user.save + user.update_columns(old_username: nil) + end + end +end diff --git a/spec/controllers/internal_users_controller_spec.rb b/spec/controllers/internal_users_controller_spec.rb new file mode 100644 index 000000000..74a2e9a8d --- /dev/null +++ b/spec/controllers/internal_users_controller_spec.rb @@ -0,0 +1,70 @@ +require "rails_helper" + +RSpec.describe "internal/users", type: :request do + context "when banishing user" do + let(:user) { create(:user) } + let(:article) { create(:article, user: user) } + let(:super_admin) { create(:user, :super_admin) } + let(:article2) { create(:article, user: super_admin) } + + before do + sign_in super_admin + Delayed::Worker.delay_jobs = true + Delayed::Worker.destroy_failed_jobs = false + end + + after do + Delayed::Worker.delay_jobs = false + end + + def banish_user + post "/internal/users/#{user.id}/banish" + Delayed::Worker.new(quiet: false).work_off + user.reload + end + + def create_dependents + # Article 1 + create(:reaction, reactable: article, reactable_type: "Article", user: user) + parent_comment = create(:comment, commentable_type: "Article", commentable: article, user: user) + create(:comment, commentable_type: "Article", commentable: article, user: user, parent: parent_comment) + + # Article 2 written by super_admin + create(:reaction, reactable: article2, reactable_type: "Article", user: user) + parent_comment2 = create(:comment, commentable_type: "Article", commentable: article2, user: user) + create(:comment, commentable_type: "Article", commentable: article2, user: super_admin, parent: parent_comment2) + + user.follow(super_admin) + + Delayed::Worker.new(quiet: true).work_off + end + + context "when offender is a spam user" do + before do + create(:reaction, reactable: article, reactable_type: "Article", user: user) + create(:comment, commentable_type: "Article", commentable: article, user: user) + Delayed::Worker.new(quiet: true).work_off + end + + it "works" do + banish_user + expect(Delayed::Job.where("failed_at IS NOT NULL").count).to eq(0) + expect(user.old_username).to eq(nil) + expect(user.twitter_username).to eq("") + end + end + + xcontext "when offender has rich activity " do + it "works" do + create_dependents + banish_user + # failed = Delayed::Job.where("failed_at IS NOT NULL").first + # failed = YAML.load(failed.handler) if failed + # binding.pry + expect(Delayed::Job.where("failed_at IS NOT NULL").count).to eq(0) + expect(user.old_username).to eq(nil) + expect(user.twitter_username).to eq("") + end + end + end +end