AO3-6467 Log on kin side when a user with a fnok is deleted

app/controllers/admin/admin_users_controller.rb

@@ -72,7 +72,7 @@ def update_next_of_kin
     kin_email = params[:next_of_kin_email]
 
     fnok = @user.fannish_next_of_kin
-    previous_fnok_user_id = fnok&.kin&.id
+    previous_kin = fnok&.kin
     fnok ||= @user.build_fannish_next_of_kin
     fnok.assign_attributes(kin: kin, kin_email: kin_email)
 
@@ -84,19 +84,14 @@ def update_next_of_kin
     # Remove FNOK that already exists.
     if fnok.persisted? && kin.blank? && kin_email.blank?
       fnok.destroy
-      log_next_of_kin_removed(previous_fnok_user_id)
+      @user.log_removal_of_next_of_kin(previous_kin, admin: current_admin)
       flash[:notice] = ts("Fannish next of kin was removed.")
       redirect_to admin_user_path(@user) and return
     end
 
     if fnok.save
-      log_next_of_kin_removed(previous_fnok_user_id)
-      @user.create_log_item({
-                              action: ArchiveConfig.ACTION_ADD_FNOK,
-                              fnok_user_id: fnok.kin.id,
-                              admin_id: current_admin.id,
-                              note: "Change made by #{current_admin.login}"
-                            })
+      @user.log_removal_of_next_of_kin(previous_kin, admin: current_admin)
+      @user.log_assignment_of_next_of_kin(kin, admin: current_admin)
       flash[:notice] = ts("Fannish next of kin was updated.")
       redirect_to admin_user_path(@user)
     else
@@ -183,19 +178,6 @@ def activate
   end
 
   def log_items
-    @log_items ||= (@user.log_items + LogItem.where(fnok_user_id: @user.id)).sort_by(&:created_at).reverse
-  end
-
-  private
-
-  def log_next_of_kin_removed(user_id)
-    return if user_id.blank?
-
-    @user.create_log_item({
-                            action: ArchiveConfig.ACTION_REMOVE_FNOK,
-                            fnok_user_id: user_id,
-                            admin_id: current_admin.id,
-                            note: "Change made by #{current_admin.login}"
-                          })
+    @log_items ||= @user.log_items.sort_by(&:created_at).reverse
   end
 end

app/helpers/users_helper.rb

@@ -119,10 +119,10 @@ def authored_items(pseud, work_counts = {}, rec_counts = {})
     items.html_safe
   end
 
-  def log_item_action_name(item, user)
+  def log_item_action_name(item)
     action = item.action
 
-    return fnok_action_name(item, user) if [ArchiveConfig.ACTION_ADD_FNOK, ArchiveConfig.ACTION_REMOVE_FNOK].include?(action)
+    return fnok_action_name(item) if fnok_action?(action)
 
     case action
     when ArchiveConfig.ACTION_ACTIVATE
@@ -152,23 +152,6 @@ def log_item_action_name(item, user)
     end
   end
 
-  def fnok_action_name(item, user)
-    action = item.action == ArchiveConfig.ACTION_REMOVE_FNOK ? "removed" : "added"
-
-    if item.fnok_user_id == user.id
-      user_id = item.user_id
-      action_leaf = "was_#{action}"
-    else
-      user_id = item.fnok_user_id
-      action_leaf = "has_#{action}"
-    end
-
-    t(
-      "users_helper.log.fnok.#{action_leaf}",
-      user_id: user_id
-    )
-  end
-
   # Give the TOS field in the new user form a different name in non-production environments
   # so that it can be filtered out of the log, for ease of debugging
   def tos_field_name
@@ -178,4 +161,34 @@ def tos_field_name
       'terms_of_service_non_production'
     end
   end
+
+  private
+
+  def fnok_action?(action)
+    [
+      ArchiveConfig.ACTION_ADD_FNOK,
+      ArchiveConfig.ACTION_REMOVE_FNOK,
+      ArchiveConfig.ACTION_ADDED_AS_FNOK,
+      ArchiveConfig.ACTION_REMOVED_AS_FNOK
+    ].include?(action)
+  end
+
+  def fnok_action_name(item)
+    action_leaf =
+      case item.action
+      when ArchiveConfig.ACTION_ADD_FNOK
+        "has_added"
+      when ArchiveConfig.ACTION_REMOVE_FNOK
+        "has_removed"
+      when ArchiveConfig.ACTION_ADDED_AS_FNOK
+        "was_added"
+      when ArchiveConfig.ACTION_REMOVED_AS_FNOK
+        "was_removed"
+      end
+
+    t(
+      "users_helper.log.fnok.#{action_leaf}",
+      user_id: item.fnok_user_id
+    )
+  end
 end

app/models/concerns/user_loggable.rb

@@ -0,0 +1,61 @@
+module UserLoggable
+  extend ActiveSupport::Concern
+
+  included do
+    before_destroy :log_removal_of_self_from_fnok_relationships
+  end
+
+  def log_removal_of_self_from_fnok_relationships
+    fannish_next_of_kins.each do |fnok|
+      fnok.user.log_removal_of_next_of_kin(self)
+    end
+
+    successor = fannish_next_of_kin&.kin
+    log_removal_of_next_of_kin(successor)
+  end
+
+  def log_assignment_of_next_of_kin(kin, admin:)
+    log_user_history(
+      ArchiveConfig.ACTION_ADD_FNOK,
+      options: { fnok_user_id: kin.id },
+      admin: admin
+    )
+
+    kin.log_user_history(
+      ArchiveConfig.ACTION_ADDED_AS_FNOK,
+      options: { fnok_user_id: self.id },
+      admin: admin
+    )
+  end
+
+  def log_removal_of_next_of_kin(kin, admin: nil)
+    return if kin.blank?
+
+    log_user_history(
+      ArchiveConfig.ACTION_REMOVE_FNOK,
+      options: { fnok_user_id: kin.id },
+      admin: admin
+    )
+
+    kin.log_user_history(
+      ArchiveConfig.ACTION_REMOVED_AS_FNOK,
+      options: { fnok_user_id: self.id },
+      admin: admin
+    )
+  end
+
+  def log_user_history(action, options: {}, admin: nil)
+    if admin.present?
+      options = {
+        admin_id: admin.id,
+        note: "Change made by #{admin.login}",
+        **options
+      }
+    end
+
+    create_log_item({
+                      action: action,
+                      **options
+                    })
+  end
+end

app/models/user.rb

@@ -2,6 +2,7 @@ class User < ApplicationRecord
   audited
   include WorksOwner
   include PasswordResetsLimitable
+  include UserLoggable
 
   devise :database_authenticatable,
          :confirmable,
@@ -37,7 +38,6 @@ class User < ApplicationRecord
   has_many :external_authors, dependent: :destroy
   has_many :external_creatorships, foreign_key: "archivist_id"
 
-  before_destroy :log_removal_as_next_of_kin
   has_many :fannish_next_of_kins, dependent: :delete_all, inverse_of: :kin, foreign_key: :kin_id
   has_one :fannish_next_of_kin, dependent: :destroy
 
@@ -173,15 +173,6 @@ def remove_user_from_kudos
     Kudo.where(user: self).update_all(user_id: nil)
   end
 
-  def log_removal_as_next_of_kin
-    fannish_next_of_kins.each do |fnok|
-      fnok.user.create_log_item({
-                                  action: ArchiveConfig.ACTION_REMOVE_FNOK,
-                                  fnok_user_id: self.id
-                                })
-    end
-  end
-
   def read_inbox_comments
     inbox_comments.where(read: true)
   end

app/views/admin/admin_users/_user_history.html.erb

@@ -36,7 +36,7 @@
          @log_items.each do |item| %>
         <tr>
           <td><%= item.created_at %></td>
-          <td><%= log_item_action_name(item, @user) %><%= item.role&.name %><%= item.enddate %></td>
+          <td><%= log_item_action_name(item) %><%= item.role&.name %><%= item.enddate %></td>
           <td><%= item.note %></td>
         </tr>
       <% end %>

config/config.yml

@@ -437,6 +437,8 @@ ACTION_TROUBLESHOOT: 10
 ACTION_NOTE: 11
 ACTION_ADD_FNOK: 12
 ACTION_REMOVE_FNOK: 13
+ACTION_ADDED_AS_FNOK: 14
+ACTION_REMOVED_AS_FNOK: 15
 
 
 # Elasticsearch index prefix

db/migrate/20230920094945_remove_fnok_user_id_index_from_log_items.rb

@@ -0,0 +1,5 @@
+class RemoveFnokUserIdIndexFromLogItems < ActiveRecord::Migration[6.1]
+  def change
+    remove_index :log_items, :fnok_user_id
+  end
+end

spec/controllers/admin/admin_users_controller_spec.rb

@@ -241,12 +241,16 @@
         log_item = user.log_items.last
         expect(log_item.action).to eq(ArchiveConfig.ACTION_ADD_FNOK)
         expect(log_item.fnok_user.id).to eq(kin.id)
-        expect(log_item.admin_id).to eq(admin.id)
-        expect(log_item.note).to eq("Change made by #{admin.login}")
+
+        added_log_item = kin.reload.log_items.last
+        expect(added_log_item.action).to eq(ArchiveConfig.ACTION_ADDED_AS_FNOK)
+        expect(added_log_item.fnok_user.id).to eq(user.id)
+
+        expect_changes_made_by(admin, [log_item, added_log_item])
       end
 
       it "logs removing a fannish next of kin" do
-        kin_user_id = create(:fannish_next_of_kin, user: user).kin_id
+        kin = create(:fannish_next_of_kin, user: user).kin
 
         post :update_next_of_kin, params: {
           user_login: user.login
@@ -255,13 +259,17 @@
         expect(user.fannish_next_of_kin).to be_nil
         log_item = user.log_items.last
         expect(log_item.action).to eq(ArchiveConfig.ACTION_REMOVE_FNOK)
-        expect(log_item.fnok_user.id).to eq(kin_user_id)
-        expect(log_item.admin_id).to eq(admin.id)
-        expect(log_item.note).to eq("Change made by #{admin.login}")
+        expect(log_item.fnok_user.id).to eq(kin.id)
+
+        removed_log_item = kin.reload.log_items.last
+        expect(removed_log_item.action).to eq(ArchiveConfig.ACTION_REMOVED_AS_FNOK)
+        expect(removed_log_item.fnok_user.id).to eq(user.id)
+
+        expect_changes_made_by(admin, [log_item, removed_log_item])
       end
 
       it "logs updating a fannish next of kin" do
-        previous_kin_user_id = create(:fannish_next_of_kin, user: user).kin_id
+        previous_kin = create(:fannish_next_of_kin, user: user).kin
 
         post :update_next_of_kin, params: {
           user_login: user.login, next_of_kin_name: kin.login, next_of_kin_email: kin.email
@@ -271,15 +279,28 @@
 
         remove_log_item = user.log_items[-2]
         expect(remove_log_item.action).to eq(ArchiveConfig.ACTION_REMOVE_FNOK)
-        expect(remove_log_item.fnok_user.id).to eq(previous_kin_user_id)
-        expect(remove_log_item.admin_id).to eq(admin.id)
-        expect(remove_log_item.note).to eq("Change made by #{admin.login}")
+        expect(remove_log_item.fnok_user.id).to eq(previous_kin.id)
 
         add_log_item = user.log_items.last
         expect(add_log_item.action).to eq(ArchiveConfig.ACTION_ADD_FNOK)
         expect(add_log_item.fnok_user.id).to eq(kin.id)
-        expect(add_log_item.admin_id).to eq(admin.id)
-        expect(add_log_item.note).to eq("Change made by #{admin.login}")
+
+        removed_log_item = previous_kin.reload.log_items.last
+        expect(removed_log_item.action).to eq(ArchiveConfig.ACTION_REMOVED_AS_FNOK)
+        expect(removed_log_item.fnok_user.id).to eq(user.id)
+
+        added_log_item = kin.reload.log_items.last
+        expect(added_log_item.action).to eq(ArchiveConfig.ACTION_ADDED_AS_FNOK)
+        expect(added_log_item.fnok_user.id).to eq(user.id)
+
+        expect_changes_made_by(admin, [remove_log_item, add_log_item, removed_log_item, added_log_item])
+      end
+
+      def expect_changes_made_by(admin, log_items)
+        log_items.each do |log_item|
+          expect(log_item.admin_id).to eq(admin.id)
+          expect(log_item.note).to eq("Change made by #{admin.login}")
+        end
       end
 
       it "does nothing if changing the fnok to themselves" do

spec/models/user_spec.rb

@@ -34,6 +34,23 @@
       end
     end
 
+    context "when the user has a fnok" do
+      let(:fnok) { create(:fannish_next_of_kin) }
+      let(:user) { fnok.user }
+      let(:kin) { fnok.kin }
+
+      it "logs the fnok removal on the kin side" do
+        user_id = user.id
+        user.destroy!
+
+        log_item = kin.reload.log_items.last
+        expect(log_item.action).to eq(ArchiveConfig.ACTION_REMOVED_AS_FNOK)
+        expect(log_item.fnok_user_id).to eq(user_id)
+        expect(log_item.admin_id).to be_nil
+        expect(log_item.note).to eq("System Generated")
+      end
+    end
+
     context "when the user is set as someone else's fnok" do
       let(:fnok) { create(:fannish_next_of_kin) }
       let(:user) { fnok.kin }