diff --git a/app/models/audit_log.rb b/app/models/audit_log.rb index c7dc18335..d1be55b4f 100644 --- a/app/models/audit_log.rb +++ b/app/models/audit_log.rb @@ -1,5 +1,3 @@ class AuditLog < ApplicationRecord - belongs_to :user - - validates :user_id, presence: true + belongs_to :user, optional: true end diff --git a/app/models/user.rb b/app/models/user.rb index 6cb49ed2f..0c1b1245b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -19,52 +19,52 @@ class User < ApplicationRecord acts_as_followable acts_as_follower - has_many :organization_memberships, dependent: :destroy - has_many :organizations, through: :organization_memberships + has_many :access_grants, class_name: "Doorkeeper::AccessGrant", foreign_key: :resource_owner_id, inverse_of: :resource_owner, dependent: :delete_all + has_many :access_tokens, class_name: "Doorkeeper::AccessToken", foreign_key: :resource_owner_id, inverse_of: :resource_owner, dependent: :delete_all + has_many :affected_feedback_messages, class_name: "FeedbackMessage", inverse_of: :affected, foreign_key: :affected_id, dependent: :nullify has_many :api_secrets, dependent: :destroy has_many :articles, dependent: :destroy + has_many :audit_logs, dependent: :nullify + has_many :authored_notes, inverse_of: :author, class_name: "Note", foreign_key: :author_id, dependent: :delete_all + has_many :backup_data, foreign_key: "instance_user_id", inverse_of: :instance_user, class_name: "BackupData", dependent: :delete_all has_many :badge_achievements, dependent: :destroy has_many :badges, through: :badge_achievements + has_many :blocked_blocks, class_name: "UserBlock", foreign_key: :blocked_id, inverse_of: :blocked, dependent: :delete_all + has_many :blocker_blocks, class_name: "UserBlock", foreign_key: :blocker_id, inverse_of: :blocker, dependent: :delete_all + has_many :chat_channel_memberships, dependent: :destroy + has_many :chat_channels, through: :chat_channel_memberships + has_many :classified_listings, dependent: :destroy has_many :collections, dependent: :destroy has_many :comments, dependent: :destroy - has_many :email_messages, class_name: "Ahoy::Message", dependent: :destroy + has_many :created_podcasts, class_name: "Podcast", foreign_key: :creator_id, inverse_of: :creator, dependent: :nullify + has_many :credits, dependent: :destroy + has_many :display_ad_events, dependent: :destroy has_many :email_authorizations, dependent: :delete_all + has_many :email_messages, class_name: "Ahoy::Message", dependent: :destroy has_many :github_repos, dependent: :destroy + has_many :html_variants, dependent: :destroy has_many :identities, dependent: :destroy has_many :mentions, dependent: :destroy has_many :messages, dependent: :destroy has_many :notes, as: :noteable, inverse_of: :noteable - has_many :profile_pins, as: :profile, inverse_of: :profile, dependent: :delete_all - has_many :authored_notes, inverse_of: :author, class_name: "Note", foreign_key: :author_id, dependent: :delete_all - has_many :notifications, dependent: :destroy - has_many :reactions, dependent: :destroy - has_many :tweets, dependent: :destroy - has_many :chat_channel_memberships, dependent: :destroy - has_many :chat_channels, through: :chat_channel_memberships has_many :notification_subscriptions, dependent: :destroy - + has_many :notifications, dependent: :destroy has_many :offender_feedback_messages, class_name: "FeedbackMessage", inverse_of: :offender, foreign_key: :offender_id, dependent: :nullify - has_many :reporter_feedback_messages, class_name: "FeedbackMessage", inverse_of: :reporter, foreign_key: :reporter_id, dependent: :nullify - has_many :affected_feedback_messages, class_name: "FeedbackMessage", inverse_of: :affected, foreign_key: :affected_id, dependent: :nullify - - has_many :rating_votes, dependent: :destroy - has_many :response_templates, foreign_key: :user_id, inverse_of: :user, dependent: :destroy - has_many :html_variants, dependent: :destroy + has_many :organization_memberships, dependent: :destroy + has_many :organizations, through: :organization_memberships has_many :page_views, dependent: :destroy - has_many :credits, dependent: :destroy - has_many :classified_listings, dependent: :destroy - has_many :poll_votes, dependent: :destroy has_many :poll_skips, dependent: :destroy - has_many :backup_data, foreign_key: "instance_user_id", inverse_of: :instance_user, class_name: "BackupData", dependent: :delete_all - has_many :display_ad_events, dependent: :destroy - has_many :access_grants, class_name: "Doorkeeper::AccessGrant", foreign_key: :resource_owner_id, inverse_of: :resource_owner, dependent: :delete_all - has_many :access_tokens, class_name: "Doorkeeper::AccessToken", foreign_key: :resource_owner_id, inverse_of: :resource_owner, dependent: :delete_all + has_many :poll_votes, dependent: :destroy + has_many :profile_pins, as: :profile, inverse_of: :profile, dependent: :delete_all + has_many :rating_votes, dependent: :destroy + has_many :reactions, dependent: :destroy + has_many :reporter_feedback_messages, class_name: "FeedbackMessage", inverse_of: :reporter, foreign_key: :reporter_id, dependent: :nullify + has_many :response_templates, foreign_key: :user_id, inverse_of: :user, dependent: :destroy + has_many :tweets, dependent: :destroy has_many :webhook_endpoints, class_name: "Webhook::Endpoint", foreign_key: :user_id, inverse_of: :user, dependent: :delete_all - has_many :blocker_blocks, class_name: "UserBlock", foreign_key: :blocker_id, inverse_of: :blocker, dependent: :delete_all - has_many :blocked_blocks, class_name: "UserBlock", foreign_key: :blocked_id, inverse_of: :blocked, dependent: :delete_all - has_one :pro_membership, dependent: :destroy + has_one :counters, class_name: "UserCounter", dependent: :destroy - has_many :created_podcasts, class_name: "Podcast", foreign_key: :creator_id, inverse_of: :creator, dependent: :nullify + has_one :pro_membership, dependent: :destroy mount_uploader :profile_image, ProfileImageUploader diff --git a/app/views/internal/moderator_actions/index.html.erb b/app/views/internal/moderator_actions/index.html.erb index cbb00b0a9..126f2e736 100644 --- a/app/views/internal/moderator_actions/index.html.erb +++ b/app/views/internal/moderator_actions/index.html.erb @@ -30,7 +30,7 @@ <% @moderator_actions.each do |action| %> <%= action.id %> - <%= link_to action.user.username, internal_user_path(action.user_id) %> + <%= link_to(action.user.username, internal_user_path(action.user_id)) if action.user %> <%= "#{action.data['action'].humanize} #{action.data['controller'].humanize.singularize.titleize}" if action.data.present? %> <%= action.data %> <%= action.created_at %> diff --git a/spec/models/audit_log_spec.rb b/spec/models/audit_log_spec.rb index 077642696..dcdbd398d 100644 --- a/spec/models/audit_log_spec.rb +++ b/spec/models/audit_log_spec.rb @@ -1,6 +1,5 @@ require "rails_helper" RSpec.describe AuditLog, type: :model do - it { is_expected.to belong_to(:user) } - it { is_expected.to validate_presence_of(:user_id) } + it { is_expected.to belong_to(:user).optional } end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 243702c5c..06c76cec4 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -18,34 +18,40 @@ RSpec.describe User, type: :model do describe "validations" do describe "builtin validations" do - it { is_expected.to have_many(:api_secrets) } - it { is_expected.to have_many(:articles) } + it { is_expected.to have_many(:api_secrets).dependent(:destroy) } + it { is_expected.to have_many(:articles).dependent(:destroy) } + it { is_expected.to have_many(:audit_logs).dependent(:nullify) } it { is_expected.to have_many(:badge_achievements).dependent(:destroy) } it { is_expected.to have_many(:badges).through(:badge_achievements) } - it { is_expected.to have_many(:collections).dependent(:destroy) } - it { is_expected.to have_many(:comments) } - it { is_expected.to have_many(:email_messages).class_name("Ahoy::Message") } - it { is_expected.to have_many(:identities).dependent(:destroy) } - it { is_expected.to have_many(:mentions).dependent(:destroy) } - it { is_expected.to have_many(:notes) } - it { is_expected.to have_many(:notifications).dependent(:destroy) } - it { is_expected.to have_many(:reactions).dependent(:destroy) } - it { is_expected.to have_many(:tweets).dependent(:destroy) } - it { is_expected.to have_many(:github_repos).dependent(:destroy) } it { is_expected.to have_many(:chat_channel_memberships).dependent(:destroy) } it { is_expected.to have_many(:chat_channels).through(:chat_channel_memberships) } + it { is_expected.to have_many(:classified_listings).dependent(:destroy) } + it { is_expected.to have_many(:collections).dependent(:destroy) } + it { is_expected.to have_many(:comments).dependent(:destroy) } + it { is_expected.to have_many(:credits).dependent(:destroy) } + it { is_expected.to have_many(:display_ad_events).dependent(:destroy) } + it { is_expected.to have_many(:email_authorizations).dependent(:delete_all) } + it { is_expected.to have_many(:email_messages).class_name("Ahoy::Message").dependent(:destroy) } + it { is_expected.to have_many(:github_repos).dependent(:destroy) } + it { is_expected.to have_many(:html_variants).dependent(:destroy) } + it { is_expected.to have_many(:identities).dependent(:destroy) } + it { is_expected.to have_many(:mentions).dependent(:destroy) } + it { is_expected.to have_many(:messages).dependent(:destroy) } + it { is_expected.to have_many(:notes) } it { is_expected.to have_many(:notification_subscriptions).dependent(:destroy) } - it { is_expected.to have_one(:pro_membership).dependent(:destroy) } - it { is_expected.to have_one(:counters).dependent(:destroy) } + it { is_expected.to have_many(:notifications).dependent(:destroy) } + it { is_expected.to have_many(:organization_memberships).dependent(:destroy) } + it { is_expected.to have_many(:organizations).through(:organization_memberships) } + it { is_expected.to have_many(:page_views).dependent(:destroy) } + it { is_expected.to have_many(:poll_skips).dependent(:destroy) } + it { is_expected.to have_many(:poll_votes).dependent(:destroy) } + it { is_expected.to have_many(:profile_pins).dependent(:delete_all) } + it { is_expected.to have_many(:rating_votes).dependent(:destroy) } + it { is_expected.to have_many(:reactions).dependent(:destroy) } + it { is_expected.to have_many(:response_templates).dependent(:destroy) } + it { is_expected.to have_many(:tweets).dependent(:destroy) } # rubocop:disable RSpec/NamedSubject - it "has created_podcasts" do - expect(subject).to have_many(:created_podcasts). - class_name("Podcast"). - with_foreign_key(:creator_id). - dependent(:nullify) - end - it do expect(subject).to have_many(:access_grants). class_name("Doorkeeper::AccessGrant"). @@ -59,9 +65,73 @@ RSpec.describe User, type: :model do with_foreign_key("resource_owner_id"). dependent(:delete_all) end + + it do + expect(subject).to have_many(:affected_feedback_messages). + class_name("FeedbackMessage"). + with_foreign_key("affected_id"). + dependent(:nullify) + end + + it do + expect(subject).to have_many(:authored_notes). + class_name("Note"). + with_foreign_key("author_id"). + dependent(:delete_all) + end + + it do + expect(subject).to have_many(:backup_data). + class_name("BackupData"). + with_foreign_key("instance_user_id"). + dependent(:delete_all) + end + + it do + expect(subject).to have_many(:blocked_blocks). + class_name("UserBlock"). + with_foreign_key("blocked_id"). + dependent(:delete_all) + end + + it do + expect(subject).to have_many(:blocker_blocks). + class_name("UserBlock"). + with_foreign_key("blocker_id"). + dependent(:delete_all) + end + + it do + expect(subject).to have_many(:created_podcasts). + class_name("Podcast"). + with_foreign_key(:creator_id). + dependent(:nullify) + end + + it do + expect(subject).to have_many(:offender_feedback_messages). + class_name("FeedbackMessage"). + with_foreign_key(:offender_id). + dependent(:nullify) + end + + it do + expect(subject).to have_many(:reporter_feedback_messages). + class_name("FeedbackMessage"). + with_foreign_key(:reporter_id). + dependent(:nullify) + end + + it do + expect(subject).to have_many(:webhook_endpoints). + class_name("Webhook::Endpoint"). + with_foreign_key(:user_id). + dependent(:delete_all) + end # rubocop:enable RSpec/NamedSubject - it { is_expected.to have_many(:organization_memberships).dependent(:destroy) } + it { is_expected.to have_one(:counters).class_name("UserCounter").dependent(:destroy) } + it { is_expected.to have_one(:pro_membership).dependent(:destroy) } it { is_expected.to validate_uniqueness_of(:username).case_insensitive } it { is_expected.to validate_uniqueness_of(:github_username).allow_nil } diff --git a/spec/requests/internal/moderator_actions_spec.rb b/spec/requests/internal/moderator_actions_spec.rb index 19bf4227e..ffc276112 100644 --- a/spec/requests/internal/moderator_actions_spec.rb +++ b/spec/requests/internal/moderator_actions_spec.rb @@ -16,35 +16,55 @@ RSpec.describe "/internal/moderator_reactions", type: :request do end context "when the user is a single resource admin" do - let(:single_resource_admin) { create(:user, :single_resource_admin, resource: ModeratorAction) } + let(:single_resource_admin) do + create(:user, :single_resource_admin, resource: ModeratorAction) + end - it "renders with status 200" do + it "renders the page" do sign_in single_resource_admin + get internal_moderator_actions_path - expect(response.status).to eq 200 + expect(response).to have_http_status(:ok) end end context "when the user is an admin" do - let(:admin) { create(:user, :admin) } - let!(:audit_log) { create(:audit_log, category: "moderator.audit.log", user_id: admin.id, roles: admin.roles.pluck(:name)) } + let(:admin) { create(:user, :admin) } before do sign_in admin end it "does not block the request" do - expect do - get internal_moderator_actions_path - end.not_to raise_error + get internal_moderator_actions_path + expect(response).to have_http_status(:ok) end - describe "GETS /internal/moderator_actions" do - it "renders to appropriate page" do - get internal_moderator_actions_path - expect(response.body).to include(admin.username) - expect(response.body).to include(audit_log.id.to_s) - end + it "renders the page with a user's audit log" do + audit_log = create( + :audit_log, + category: "moderator.audit.log", + user: admin, + roles: admin.roles.pluck(:name), + ) + + get internal_moderator_actions_path + + expect(response.body).to include(admin.username) + expect(response.body).to include(audit_log.id.to_s) + end + + it "renders the page with an audit log not belonging to a specific user" do + audit_log = create( + :audit_log, + category: "moderator.audit.log", + user: nil, + ) + + get internal_moderator_actions_path + + expect(response.body).not_to include(admin.username) + expect(response.body).to include(audit_log.id.to_s) end end end diff --git a/spec/services/users/delete_spec.rb b/spec/services/users/delete_spec.rb index 6ab71472c..50df6c9f2 100644 --- a/spec/services/users/delete_spec.rb +++ b/spec/services/users/delete_spec.rb @@ -37,6 +37,16 @@ RSpec.describe Users::Delete, type: :service do expect(Rails.cache).to have_received(:delete).with("user-destroy-token-#{user.id}") end + it "does not delete user's audit logs" do + audit_log = create(:audit_log, user: user) + + expect do + described_class.call(user) + end.to change(AuditLog, :count).by(0) + + expect(audit_log.reload.user_id).to be(nil) + end + it "removes user from Elasticsearch" do sidekiq_perform_enqueued_jobs { user } expect(user.elasticsearch_doc).not_to be_nil @@ -48,7 +58,12 @@ RSpec.describe Users::Delete, type: :service do # check that all the associated records are being destroyed, except for those that are kept explicitly (kept_associations) describe "deleting associations" do - let(:kept_association_names) { %i[created_podcasts notes offender_feedback_messages reporter_feedback_messages affected_feedback_messages] } + let(:kept_association_names) do + %i[ + affected_feedback_messages audit_logs created_podcasts notes + offender_feedback_messages reporter_feedback_messages + ] + end let(:direct_associations) { User.reflect_on_all_associations.reject { |a| a.options.key?(:join_table) || a.options.key?(:through) } } let!(:user_associations) do create_associations(direct_associations.reject { |a| kept_association_names.include?(a.name) })