Nullify audit logs on users delete (#7275)

* Nullify audit logs on users delete

* Fix specs

* Set user as optional and test moderator audit log page

* Fix spec

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

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
 

app/views/internal/moderator_actions/index.html.erb

@@ -30,7 +30,7 @@
     <% @moderator_actions.each do |action| %>
       <tr>
         <td><%= action.id %></td>
-        <td><%= link_to action.user.username, internal_user_path(action.user_id) %></td>
+        <td><%= link_to(action.user.username, internal_user_path(action.user_id)) if action.user %></td>
         <td><%= "#{action.data['action'].humanize} #{action.data['controller'].humanize.singularize.titleize}" if action.data.present? %></td>
         <td><%= action.data %></td>
         <td><%= action.created_at %></td>

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

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 }

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

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) })