AO3-6467 Record Fannish Next of Kin changes in user history (#4582)

* Adding

https://otwarchive.atlassian.net/browse/AO3-6467

* Remove

* Replace many ifelse with case

* Display logs on admin page

* Forgot to log admin identity

* Also show logs on the FNOK side

* Test for display

Keeping it simple by not checking id for now

* Add Alterity

* Forgot to commit previously

* Hound

* Hound (bis)

* Hound (ter)

* Add i18n keys to exceptions

* Remove Alterity actually

Superseded by #4528

* Add pt-online-schema-change syntax for prod

* Remove explicit foreign key

Similar migrations don't appear to set one

* Implicit NULL default value

* Test id too

* Add user destroy controller test

Apparently didn't exist before?

* Log automatic removal of fnok on deletion

* Hound

* Use classical I18n syntax

* Simplify fnok name action

* Clean other I18n keys

* Hound

* reviewdog

* Normalize i18n

* Syntax more compliant with i18n-tasks

* Wordings

* Explicit over succinct

app/controllers/admin/admin_users_controller.rb

@@ -47,7 +47,7 @@ def show
     @user = authorize User.find_by!(login: params[:id])
     @hide_dashboard = true
     @page_subtitle = t(".page_title", login: @user.login)
-    @log_items = @user.log_items.sort_by(&:created_at).reverse
+    log_items
   end
 
   # POST admin/users/update
@@ -75,6 +75,12 @@ def update_next_of_kin
     if kin.blank? && kin_email.blank?
       if fnok.present?
         fnok.destroy
+        @user.create_log_item({
+                                action: ArchiveConfig.ACTION_REMOVE_FNOK,
+                                fnok_user_id: fnok.kin.id,
+                                admin_id: current_admin.id,
+                                note: "Change made by #{current_admin.login}"
+                              })
         flash[:notice] = ts("Fannish next of kin was removed.")
       end
       redirect_to admin_user_path(@user)
@@ -84,11 +90,17 @@ def update_next_of_kin
     fnok = @user.build_fannish_next_of_kin if fnok.blank?
     fnok.assign_attributes(kin: kin, kin_email: kin_email)
     if fnok.save
+      @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}"
+                            })
       flash[:notice] = ts("Fannish next of kin was updated.")
       redirect_to admin_user_path(@user)
     else
       @hide_dashboard = true
-      @log_items = @user.log_items.sort_by(&:created_at).reverse
+      log_items
       render :show
     end
   end
@@ -168,4 +180,8 @@ def activate
       redirect_to action: :show
     end
   end
+
+  def log_items
+    @log_items ||= (@user.log_items + LogItem.where(fnok_user_id: @user.id)).sort_by(&:created_at).reverse
+  end
 end

app/helpers/users_helper.rb

@@ -119,34 +119,56 @@ def authored_items(pseud, work_counts = {}, rec_counts = {})
     items.html_safe
   end
 
-  def log_item_action_name(action)
-    if action == ArchiveConfig.ACTION_ACTIVATE
-      t('users_helper.log_validated', default: 'Account Validated')
-    elsif action == ArchiveConfig.ACTION_ADD_ROLE
-      t('users_helper.log_role_added', default: 'Role Added: ')
-    elsif action == ArchiveConfig.ACTION_REMOVE_ROLE
-      t('users_helper.log_role_removed', default: 'Role Removed: ')
-    elsif action == ArchiveConfig.ACTION_SUSPEND
-      t('users_helper.log_suspended', default: 'Suspended until ')
-    elsif action == ArchiveConfig.ACTION_UNSUSPEND
-      t('users_helper.log_lift_suspension', default: 'Suspension Lifted')
-    elsif action == ArchiveConfig.ACTION_BAN
-      t('users_helper.log_ban', default: 'Suspended Permanently')
-    elsif action == ArchiveConfig.ACTION_WARN
-      t('users_helper.log_warn', default: 'Warned')
-    elsif action == ArchiveConfig.ACTION_RENAME
-      t('users_helper.log_rename', default: 'Username Changed')
-    elsif action == ArchiveConfig.ACTION_PASSWORD_RESET
-      t('users_helper.log_password_change', default: 'Password Changed')
-    elsif action == ArchiveConfig.ACTION_NEW_EMAIL
-      t('users_helper.log_email_change', default: 'Email Changed')
-    elsif action == ArchiveConfig.ACTION_TROUBLESHOOT
-      t('users_helper.log_troubleshot', default: 'Account Troubleshot')
-    elsif action == ArchiveConfig.ACTION_NOTE
-      t('users_helper.log_note', default: 'Note Added')
+  def log_item_action_name(item, user)
+    action = item.action
+
+    return fnok_action_name(item, user) if [ArchiveConfig.ACTION_ADD_FNOK, ArchiveConfig.ACTION_REMOVE_FNOK].include?(action)
+
+    case action
+    when ArchiveConfig.ACTION_ACTIVATE
+      t("users_helper.log.validated")
+    when ArchiveConfig.ACTION_ADD_ROLE
+      t("users_helper.log.role_added")
+    when ArchiveConfig.ACTION_REMOVE_ROLE
+      t("users_helper.log.role_removed")
+    when ArchiveConfig.ACTION_SUSPEND
+      t("users_helper.log.suspended")
+    when ArchiveConfig.ACTION_UNSUSPEND
+      t("users_helper.log.lift_suspension")
+    when ArchiveConfig.ACTION_BAN
+      t("users_helper.log.ban")
+    when ArchiveConfig.ACTION_WARN
+      t("users_helper.log.warn")
+    when ArchiveConfig.ACTION_RENAME
+      t("users_helper.log.rename")
+    when ArchiveConfig.ACTION_PASSWORD_RESET
+      t("users_helper.log.password_change")
+    when ArchiveConfig.ACTION_NEW_EMAIL
+      t("users_helper.log.email_change")
+    when ArchiveConfig.ACTION_TROUBLESHOOT
+      t("users_helper.log.troubleshot")
+    when ArchiveConfig.ACTION_NOTE
+      t("users_helper.log.note")
     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

app/models/log_item.rb

@@ -7,6 +7,8 @@ class LogItem < ApplicationRecord
 
   belongs_to :role
 
+  belongs_to :fnok_user, class_name: "User"
+
   validates_presence_of :note
   validates_presence_of :action
 

app/models/user.rb

@@ -36,6 +36,7 @@ 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
 
@@ -171,6 +172,15 @@ 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.action) %><%= item.role.name if item.role %><%= item.enddate if item.enddate %></td>
+          <td><%= log_item_action_name(item, @user) %><%= item.role&.name %><%= item.enddate %></td>
           <td><%= item.note %></td>
         </tr>
       <% end %>

config/config.yml

@@ -428,6 +428,8 @@ ACTION_PASSWORD_RESET: 8
 ACTION_NEW_EMAIL: 9
 ACTION_TROUBLESHOOT: 10
 ACTION_NOTE: 11
+ACTION_ADD_FNOK: 12
+ACTION_REMOVE_FNOK: 13
 
 
 # Elasticsearch index prefix

config/i18n-tasks.yml

@@ -153,19 +153,6 @@ ignore_missing:
   - successfully_sent # should be feedbacks.create.successfully_sent
   # Files: app/controllers/languages_controller.rb and app/controllers/locales_controller.rb
   - successfully_added # should be languages.create.successfully_added and locales.create.successfully_added
-  # File: app/helpers/users_helper.rb
-  - users_helper.log_ban
-  - users_helper.log_email_change
-  - users_helper.log_lift_suspension
-  - users_helper.log_note
-  - users_helper.log_password_change
-  - users_helper.log_rename
-  - users_helper.log_role_added
-  - users_helper.log_role_removed
-  - users_helper.log_suspended
-  - users_helper.log_troubleshot
-  - users_helper.log_validated
-  - users_helper.log_warn
   # Files: app/views/admin/_admin_nav.html.erb and app/views/admin_posts/show.html.erb
   - admin.admin_nav.delete
   # File: app/views/admin/admin_invitations/find.html.erb

config/locales/helpers/en.yml

@@ -20,3 +20,22 @@ en:
       user:
         bookmark: Bookmarker approval status
         work: Creator approval status
+  users_helper:
+    log:
+      ban: Suspended Permanently
+      email_change: Email Changed
+      fnok:
+        has_added: 'Fannish Next of Kin Added: %{user_id}'
+        has_removed: 'Fannish Next of Kin Removed: %{user_id}'
+        was_added: 'Added as Fannish Next of Kin for: %{user_id}'
+        was_removed: 'Removed as Fannish Next of Kin for: %{user_id}'
+      lift_suspension: Suspension Lifted
+      note: Note Added
+      password_change: Password Changed
+      rename: Username Changed
+      role_added: 'Role Added: '
+      role_removed: 'Role Removed: '
+      suspended: 'Suspended until '
+      troubleshot: Account Troubleshot
+      validated: Account Validated
+      warn: Warned

db/migrate/20230717161221_add_fnok_user_id_to_log_items.rb

@@ -0,0 +1,51 @@
+class AddFnokUserIdToLogItems < ActiveRecord::Migration[6.1]
+  def up
+    if Rails.env.staging? || Rails.env.production?
+      database = LogItem.connection.current_database
+
+      puts <<~PTOSC
+        Schema Change Command:
+
+        pt-online-schema-change D=#{database},t=log_items \\
+          --alter "ADD `fnok_user_id` int,
+                  ADD INDEX `index_log_items_on_fnok_user_id` (`fnok_user_id`)" \\
+          --no-drop-old-table \\
+          -uroot --ask-pass --chunk-size=5k --max-flow-ctl 0 --pause-file /tmp/pauseme \\
+          --max-load Threads_running=15 --critical-load Threads_running=100 \\
+          --set-vars innodb_lock_wait_timeout=2 --alter-foreign-keys-method=auto \\
+          --execute
+
+        Table Deletion Command:
+
+        DROP TABLE IF EXISTS `#{database}`.`_log_items_old`;
+      PTOSC
+    else
+      add_column :log_items, :fnok_user_id, :integer, nullable: true
+      add_index :log_items, :fnok_user_id
+    end
+  end
+
+  def down
+    if Rails.env.staging? || Rails.env.production?
+      database = LogItem.connection.current_database
+
+      puts <<~PTOSC
+        Schema Change Command:
+
+        pt-online-schema-change D=#{database},t=log_items \\
+          --alter "DROP COLUMN `fnok_user_id`"\\
+          --no-drop-old-table \\
+          -uroot --ask-pass --chunk-size=5k --max-flow-ctl 0 --pause-file /tmp/pauseme \\
+          --max-load Threads_running=15 --critical-load Threads_running=100 \\
+          --set-vars innodb_lock_wait_timeout=2 --alter-foreign-keys-method=auto \\
+          --execute
+
+        Table Deletion Command:
+
+        DROP TABLE IF EXISTS `#{database}`.`_log_items_old`;
+      PTOSC
+    else
+      remove_column :log_items, :fnok_user_id
+    end
+  end
+end

factories/fannish_next_of_kin.rb

@@ -0,0 +1,9 @@
+require "faker"
+
+FactoryBot.define do
+  factory :fannish_next_of_kin do
+    user { create(:user) }
+    kin { create(:user) }
+    kin_email { |u| u.kin.email }
+  end
+end

features/admins/users/admin_fnok.feature

@@ -15,6 +15,7 @@ Feature: Admin Fannish Next Of Kind actions
       And I fill in "Fannish next of kin's email" with "[email protected]"
       And I press "Update Fannish Next of Kin"
     Then I should see "Fannish next of kin was updated."
+      And the history table should show that "libby" was added as next of kin
 
     When I go to the manage users page
       And I fill in "Name" with "harrykim"
@@ -24,6 +25,9 @@ Feature: Admin Fannish Next Of Kind actions
     When I follow "libby"
     Then I should be on libby's user page
 
+    When I go to the user administration page for "libby"
+    Then the history table should show they were added as next of kin of "harrykim"
+
   Scenario: An invalid Fannish Next of Kin username is added
     Given the fannish next of kin "libby" for the user "harrykim"
       And I am logged in as a "support" admin
@@ -66,6 +70,9 @@ Feature: Admin Fannish Next Of Kind actions
       And I fill in "Fannish next of kin's email" with ""
       And I press "Update Fannish Next of Kin"
     Then I should see "Fannish next of kin was removed."
+      And the history table should show that "libby" was removed as next of kin
+    When I go to the user administration page for "libby"
+    Then the history table should show they were removed as next of kin of "harrykim"
 
   Scenario: A Fannish Next of Kin updates when the next of kin user changes their username
     Given the fannish next of kin "libby" for the user "harrykim"

features/step_definitions/admin_steps.rb

@@ -504,3 +504,13 @@
   expect(page).to have_selector(".original_creators",
                                 text: "#{user.id} (#{creator})")
 end
+
+Then "the history table should show that {string} was {word} as next of kin" do |username, action|
+  user_id = User.find_by(login: username).id
+  step %{I should see "Fannish Next of Kin #{action.capitalize}: #{user_id}" within "#user_history"}
+end
+
+Then "the history table should show they were {word} as next of kin of {string}" do |action, username|
+  user_id = User.find_by(login: username).id
+  step %{I should see "#{action.capitalize} as Fannish Next of Kin for: #{user_id}" within "#user_history"}
+end

spec/controllers/admin/admin_users_controller_spec.rb

@@ -226,6 +226,39 @@
         it_behaves_like "authorized admin can add next of kin"
       end
     end
+
+    it "logs adding a fannish next of kin" do
+      admin = create(:support_admin)
+      fake_login_admin(admin)
+
+      post :update_next_of_kin, params: {
+        user_login: user.login, next_of_kin_name: kin.login, next_of_kin_email: kin.email
+      }
+      user.reload
+      expect(user.fannish_next_of_kin.kin).to eq(kin)
+      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}")
+    end
+
+    it "logs removing a fannish next of kin" do
+      admin = create(:support_admin)
+      fake_login_admin(admin)
+      kin_user_id = create(:fannish_next_of_kin, user: user).kin_id
+
+      post :update_next_of_kin, params: {
+        user_login: user.login
+      }
+      user.reload
+      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}")
+    end
   end
 
   describe "POST #update_status" do

spec/controllers/users_controller_spec.rb

@@ -2,6 +2,7 @@
 
 describe UsersController do
   include RedirectExpectationHelper
+  include LoginMacros
 
   describe "GET #activate" do
     let(:user) { create(:user, confirmed_at: nil) }
@@ -58,4 +59,21 @@
       end
     end
   end
+
+  describe "destroy" do
+    let(:user) { create(:user) }
+
+    before do
+      fake_login_known_user(user)
+    end
+
+    context "no log items" do
+      it "successfully destroys and redirects with success message" do
+        login = user.login
+        delete :destroy, params: { id: login }
+        it_redirects_to_with_notice(delete_confirmation_path, "You have successfully deleted your account.")
+        expect(User.find_by(login: login)).to be_nil
+      end
+    end
+  end
 end