Refactor update owner audit activity to store metadata, and remove ca…

…llbacks and dependency on User.current (#700)

psd-web/app/controllers/investigations/ownership_controller.rb

@@ -2,42 +2,37 @@ class Investigations::OwnershipController < ApplicationController
   include Wicked::Wizard
   before_action :set_investigation
   before_action :authorize_user
-  before_action :potential_owner, only: %i[show create]
-  before_action :store_owner, only: %i[update]
 
   steps :"select-owner", :confirm
 
   def show
-    @potential_owner = potential_owner&.decorate
+    @potential_owner = form.owner&.decorate
     render_wizard
   end
 
   def new
-    clear_session
+    session[session_store_key] = nil
     redirect_to wizard_path(steps.first, request.query_parameters)
   end
 
   def update
-    if owner_valid?
-      redirect_to next_wizard_path
-    else
-      render_wizard
-    end
+    return render_wizard unless form.valid?
+
+    session[session_store_key] = form.attributes.compact
+    redirect_to next_wizard_path
   end
 
   def create
-    @investigation.owner = potential_owner
-    @investigation.owner_rationale = params[:investigation][:owner_rationale]
-    @investigation.save
-    redirect_to investigation_path(@investigation), flash: { success: "#{@investigation.case_type.upcase_first} owner changed to #{potential_owner.decorate.display_name}" }
-  end
+    ChangeCaseOwner.call!(investigation: @investigation, owner: form.owner, rationale: form.owner_rationale, user: current_user)
 
-private
+    session[session_store_key] = nil
 
-  def clear_session
-    session[:owner_id] = nil
+    message = "#{@investigation.case_type.upcase_first} owner changed to #{form.owner.decorate.display_name(viewer: current_user)}"
+    redirect_to investigation_path(@investigation), flash: { success: message }
   end
 
+private
+
   def set_investigation
     @investigation = Investigation.find_by!(pretty_id: params[:investigation_pretty_id]).decorate
   end
@@ -46,7 +41,12 @@ def authorize_user
     authorize @investigation, :change_owner_or_status?
   end
 
-  def owner_params
+  def session_store_key
+    "update_case_owner_#{@investigation.pretty_id}_params"
+  end
+
+  def form_params
+    params[:investigation] ||= {}
     params[:investigation][:owner_id] = case params[:investigation][:owner_id]
                                         when "someone_in_your_team"
                                           params[:investigation][:select_team_member]
@@ -59,23 +59,14 @@ def owner_params
                                         else
                                           params[:investigation][:owner_id]
                                         end
-    params.require(:investigation).permit(:owner_id)
-  end
-
-  def store_owner
-    session[:owner_id] = owner_params[:owner_id]
+    params.require(:investigation).permit(:owner_id, :owner_rationale).merge(session_params)
   end
 
-  def owner_valid?
-    if step == :"select-owner"
-      if potential_owner.nil?
-        @investigation.errors.add(:owner_id, :invalid, message: "Select case owner")
-      end
-    end
-    @investigation.errors.empty?
+  def session_params
+    session[session_store_key] || {}
   end
 
-  def potential_owner
-    User.find_by(id: session[:owner_id]) || Team.find_by(id: session[:owner_id])
+  def form
+    @form ||= ChangeCaseOwnerForm.new(form_params)
   end
 end

psd-web/app/controllers/investigations_controller.rb

@@ -124,6 +124,6 @@ def build_breadcrumbs
   end
 
   def set_suggested_previous_owners
-    @suggested_previous_owners = suggested_previous_owners
+    @suggested_previous_owners = suggested_previous_owners(@investigation)
   end
 end

psd-web/app/forms/change_case_owner_form.rb

@@ -0,0 +1,28 @@
+class ChangeCaseOwnerForm
+  include ActiveModel::Model
+  include ActiveModel::Attributes
+
+  attribute :owner_id
+  attribute :owner_rationale
+
+  validates_presence_of :owner_id
+  validate :new_owner_must_be_active_user_or_team, if: -> { owner_id.present? }
+
+  def owner
+    user || team
+  end
+
+private
+
+  def new_owner_must_be_active_user_or_team
+    errors.add(:owner_id, :not_found) unless owner
+  end
+
+  def user
+    @user ||= User.active.find_by(id: owner_id)
+  end
+
+  def team
+    @team ||= Team.find_by(id: owner_id)
+  end
+end

psd-web/app/helpers/investigations_helper.rb

@@ -241,8 +241,8 @@ def user_ids_from_team(team)
     [team.id] + team.users.map(&:id)
   end
 
-  def suggested_previous_owners
-    all_past_owners = @investigation.past_owners + @investigation.past_teams
+  def suggested_previous_owners(investigation)
+    all_past_owners = investigation.past_owners
     return [] if all_past_owners.empty? || all_past_owners == [current_user]
 
     all_past_owners || []

psd-web/app/models/activity.rb

@@ -17,8 +17,13 @@ def attachments
     {}
   end
 
-  def subtitle
-    "#{subtitle_slug} by #{source&.show}, #{pretty_date_stamp}"
+  # Can be overridden by child classes but sometimes need to pass in user
+  def title(_user)
+    super()
+  end
+
+  def subtitle(viewer)
+    "#{subtitle_slug} by #{source&.show(viewer)}, #{pretty_date_stamp}"
   end
 
   def search_index;  end
@@ -31,7 +36,7 @@ def can_display_all_data?(_user)
     true
   end
 
-  def restricted_title
+  def restricted_title(_user)
     # where necessary should be implemented by subclasses
   end
 

psd-web/app/models/audit_activity/correspondence/add_email.rb

@@ -48,7 +48,7 @@ def self.build_email_address(correspondence)
     output + "<br>"
   end
 
-  def restricted_title
+  def restricted_title(_user)
     "Email added"
   end
 

psd-web/app/models/audit_activity/correspondence/add_meeting.rb

@@ -23,7 +23,7 @@ def activity_type
     "meeting"
   end
 
-  def restricted_title
+  def restricted_title(_user)
     "Meeting added"
   end
 

psd-web/app/models/audit_activity/correspondence/add_phone_call.rb

@@ -39,7 +39,7 @@ def self.build_file_body(correspondence)
     file.attached? ? "Attached: #{sanitize_text file.filename}<br>" : ""
   end
 
-  def restricted_title
+  def restricted_title(_user)
     "Phone call added"
   end
 

psd-web/app/models/audit_activity/document/add.rb

@@ -8,7 +8,7 @@ def email_update_text(viewer = nil)
     "Document was attached to the #{investigation.case_type.upcase_first} by #{source&.show(viewer)}."
   end
 
-  def restricted_title
+  def restricted_title(_user)
     "Document added"
   end
 

psd-web/app/models/audit_activity/document/base.rb

@@ -21,7 +21,7 @@ def attached_image?
     attachment.image?
   end
 
-  def restricted_title; end
+  def restricted_title(_user); end
 
   def can_display_all_data?(user)
     can_be_displayed?(attachment, investigation, user)

psd-web/app/models/audit_activity/document/destroy.rb

@@ -4,7 +4,7 @@ def self.from(document, investigation)
     super(document, investigation, title)
   end
 
-  def restricted_title
+  def restricted_title(_user)
     "Document deleted"
   end
 

psd-web/app/models/audit_activity/document/update.rb

@@ -22,7 +22,7 @@ def self.description_changed?(document, previous_data)
     document.metadata[:description] != previous_data[:description]
   end
 
-  def restricted_title
+  def restricted_title(_user)
     "Document updated"
   end
 

psd-web/app/models/audit_activity/investigation/add.rb

@@ -31,7 +31,7 @@ def complainant
 
   # title may change after investigation was created, so we retrieve from the
   # metadata stored at the time of creation
-  def title
+  def title(_user)
     return self[:title] if self[:title] # older activities stored this in the database
 
     "#{investigation.case_type.titleize} logged: #{metadata['investigation']['title']}"
@@ -49,8 +49,8 @@ def can_display_all_data?(user)
   end
 
   # Only used for old records prior to metadata implementation
-  def restricted_title
-    title
+  def restricted_title(user)
+    title(user)
   end
 
 private

psd-web/app/models/audit_activity/investigation/automatically_update_owner.rb

@@ -1,31 +1,30 @@
 class AuditActivity::Investigation::AutomaticallyUpdateOwner < AuditActivity::Investigation::Base
-  def self.from(investigation)
-    title = investigation.owner.id.to_s
-    super(investigation, title)
+  def self.from(*)
+    raise "Deprecated - use DeleteUser.call instead"
   end
 
-  def owner_id
-    # We store owner_id in title field, this is getting it back
-    # Using alias for accessing parent method causes errors elsewhere :(
-    AuditActivity::Investigation::Base.instance_method(:title).bind(self).call
+  def self.build_metadata(owner)
+    {
+      owner_id: owner.id
+    }
   end
 
-  # We store owner_id in title field, this is computing title based on that
-  def title
+  def title(user)
     type = investigation.case_type.capitalize
-    new_owner = (User.find_by(id: owner_id) || Team.find_by(id: owner_id))&.decorate&.display_name
+    new_owner = owner.decorate.display_name(viewer: user)
     "Case owner automatically changed on #{type} to #{new_owner}"
   end
 
-  def subtitle
+  def subtitle(_viewer)
     "Case owner automatically changed, #{pretty_date_stamp}"
   end
 
-  def entities_to_notify
-    []
-  end
+private
 
-  def email_subject_text; end
+  def owner
+    User.find_by(id: metadata["owner_id"]) || Team.find_by(id: metadata["owner_id"])
+  end
 
-  def email_update_text(viewer = nil); end
+  # Do not send investigation_updated mail. This is handled by the DeleteUser service
+  def notify_relevant_users; end
 end

psd-web/app/models/audit_activity/investigation/team_added.rb

@@ -1,6 +1,6 @@
 class AuditActivity::Investigation::TeamAdded < AuditActivity::Investigation::Base
-  def subtitle
-    "Team added by #{source&.show}, #{pretty_date_stamp}"
+  def subtitle(viewer)
+    "Team added by #{source&.show(viewer)}, #{pretty_date_stamp}"
   end
 
 private

psd-web/app/models/audit_activity/investigation/team_deleted.rb

@@ -1,6 +1,6 @@
 class AuditActivity::Investigation::TeamDeleted < AuditActivity::Investigation::Base
-  def subtitle
-    "Team removed by #{source&.show}, #{pretty_date_stamp}"
+  def subtitle(viewer)
+    "Team removed by #{source&.show(viewer)}, #{pretty_date_stamp}"
   end
 
 private

psd-web/app/models/audit_activity/investigation/update_owner.rb

@@ -1,37 +1,25 @@
 class AuditActivity::Investigation::UpdateOwner < AuditActivity::Investigation::Base
-  include NotifyHelper
-
-  def self.from(investigation)
-    title = investigation.owner.id.to_s
-    body = investigation.owner_rationale
-    super(investigation, title, sanitize_text(body))
+  def self.from(*)
+    raise "Deprecated - use ChangeCaseOwner.call instead"
   end
 
-  def owner_id
-    # We store owner_id in title field, this is getting it back
-    # Using alias for accessing parent method causes errors elsewhere :(
-    AuditActivity::Investigation::Base.instance_method(:title).bind(self).call
+  def self.build_metadata(owner, rationale)
+    {
+      owner_id: owner.id,
+      rationale: rationale
+    }
   end
 
-  def title
-    # We store owner_id in title field, this is computing title based on that
-    "Case owner changed to #{(User.find_by(id: owner_id) || Team.find_by(id: owner_id))&.decorate&.display_name}"
+  def title(user)
+    "Case owner changed to #{owner.decorate.display_name(viewer: user)}"
   end
 
-  def email_update_text(viewer = nil)
-    body = []
-    body << "Case owner changed on #{investigation.case_type} to #{investigation.owner.decorate.display_name(viewer: viewer)} by #{source&.show(viewer)}."
-
-    if investigation.owner_rationale.present?
-      body << "Message from #{source&.show(viewer)}:"
-      body << inset_text_for_notify(investigation.owner_rationale)
-    end
-
-    body.join("\n\n")
+  def body
+    metadata["rationale"]
   end
 
-  def email_subject_text
-    "Case owner changed for #{investigation.case_type}"
+  def owner
+    User.find_by(id: metadata["owner_id"]) || Team.find_by(id: metadata["owner_id"])
   end
 
 private
@@ -40,27 +28,6 @@ def subtitle_slug
     "Changed"
   end
 
-  def users_to_notify
-    compute_relevant_entities(model: User, compute_users_from_entity: proc { |user| [user] })
-  end
-
-  def teams_to_notify
-    compute_relevant_entities(model: Team, compute_users_from_entity: proc { |team| team.users })
-  end
-
-  def compute_relevant_entities(model:, compute_users_from_entity:)
-    return [] unless investigation.saved_changes.key?("owner_id")
-
-    previous_owner_id = investigation.saved_changes["owner_id"][0]
-    previous_owner = model.find_by(id: previous_owner_id)
-    new_owner = investigation.owner
-    owner_changed_by = source.user
-
-    old_users = previous_owner.present? ? compute_users_from_entity.call(previous_owner) : []
-    old_entities = previous_owner.present? ? [previous_owner] : []
-    new_entities = new_owner.is_a?(model) ? [new_owner] : []
-    return new_entities if old_users.include? owner_changed_by
-
-    (new_entities + old_entities).uniq
-  end
+  # Do not send investigation_updated mail. This is handled by the ChangeCaseOwner service
+  def notify_relevant_users; end
 end