diff --git a/psd-web/app/controllers/investigations/ownership_controller.rb b/psd-web/app/controllers/investigations/ownership_controller.rb index dc638ff805..516bbebad2 100644 --- a/psd-web/app/controllers/investigations/ownership_controller.rb +++ b/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 diff --git a/psd-web/app/forms/change_case_owner_form.rb b/psd-web/app/forms/change_case_owner_form.rb new file mode 100644 index 0000000000..e39a2f297f --- /dev/null +++ b/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 + + 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 diff --git a/psd-web/app/models/activity.rb b/psd-web/app/models/activity.rb index 16aaa5f493..377db73b33 100644 --- a/psd-web/app/models/activity.rb +++ b/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(user) + "#{subtitle_slug} by #{source&.show(user)}, #{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 diff --git a/psd-web/app/models/audit_activity/correspondence/add_email.rb b/psd-web/app/models/audit_activity/correspondence/add_email.rb index 7955ab660f..a9ac92087e 100644 --- a/psd-web/app/models/audit_activity/correspondence/add_email.rb +++ b/psd-web/app/models/audit_activity/correspondence/add_email.rb @@ -48,7 +48,7 @@ def self.build_email_address(correspondence) output + "
" end - def restricted_title + def restricted_title(_user) "Email added" end diff --git a/psd-web/app/models/audit_activity/correspondence/add_meeting.rb b/psd-web/app/models/audit_activity/correspondence/add_meeting.rb index 585be78a56..4cc9b660c2 100644 --- a/psd-web/app/models/audit_activity/correspondence/add_meeting.rb +++ b/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 diff --git a/psd-web/app/models/audit_activity/correspondence/add_phone_call.rb b/psd-web/app/models/audit_activity/correspondence/add_phone_call.rb index 88adf4a726..a0d6a828c2 100644 --- a/psd-web/app/models/audit_activity/correspondence/add_phone_call.rb +++ b/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}
" : "" end - def restricted_title + def restricted_title(_user) "Phone call added" end diff --git a/psd-web/app/models/audit_activity/document/add.rb b/psd-web/app/models/audit_activity/document/add.rb index fb10d31cfd..27da39f938 100644 --- a/psd-web/app/models/audit_activity/document/add.rb +++ b/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 diff --git a/psd-web/app/models/audit_activity/document/base.rb b/psd-web/app/models/audit_activity/document/base.rb index 2a4bb3168e..ac713fb088 100644 --- a/psd-web/app/models/audit_activity/document/base.rb +++ b/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) diff --git a/psd-web/app/models/audit_activity/document/destroy.rb b/psd-web/app/models/audit_activity/document/destroy.rb index 16b19991d5..9d6fd6cfb2 100644 --- a/psd-web/app/models/audit_activity/document/destroy.rb +++ b/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 diff --git a/psd-web/app/models/audit_activity/document/update.rb b/psd-web/app/models/audit_activity/document/update.rb index dc3dd004cb..25b1ef6192 100644 --- a/psd-web/app/models/audit_activity/document/update.rb +++ b/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 diff --git a/psd-web/app/models/audit_activity/investigation/add.rb b/psd-web/app/models/audit_activity/investigation/add.rb index ab016d82dd..f3074f05dd 100644 --- a/psd-web/app/models/audit_activity/investigation/add.rb +++ b/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 diff --git a/psd-web/app/models/audit_activity/investigation/automatically_update_owner.rb b/psd-web/app/models/audit_activity/investigation/automatically_update_owner.rb index 073245e6f8..15b0b50eaa 100644 --- a/psd-web/app/models/audit_activity/investigation/automatically_update_owner.rb +++ b/psd-web/app/models/audit_activity/investigation/automatically_update_owner.rb @@ -1,31 +1,31 @@ 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(_user) "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 when case added. This overrides + # inherited functionality in the Activity model :( + def notify_relevant_users; end end diff --git a/psd-web/app/models/audit_activity/investigation/team_added.rb b/psd-web/app/models/audit_activity/investigation/team_added.rb index 2641221a82..d22e4604dc 100644 --- a/psd-web/app/models/audit_activity/investigation/team_added.rb +++ b/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(user) + "Team added by #{source&.show(user)}, #{pretty_date_stamp}" end private diff --git a/psd-web/app/models/audit_activity/investigation/team_deleted.rb b/psd-web/app/models/audit_activity/investigation/team_deleted.rb index 715d5497d8..2f0b42c2b1 100644 --- a/psd-web/app/models/audit_activity/investigation/team_deleted.rb +++ b/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(user) + "Team removed by #{source&.show(user)}, #{pretty_date_stamp}" end private diff --git a/psd-web/app/models/audit_activity/investigation/update_owner.rb b/psd-web/app/models/audit_activity/investigation/update_owner.rb index 59d9aed90d..11567f802e 100644 --- a/psd-web/app/models/audit_activity/investigation/update_owner.rb +++ b/psd-web/app/models/audit_activity/investigation/update_owner.rb @@ -1,66 +1,34 @@ 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)) - 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.from(*) + raise "Deprecated - use ChangeCaseOwner.call instead" 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 self.build_metadata(owner, rationale) + { + owner_id: owner.id, + rationale: rationale + } 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 title(user) + "Case owner changed to #{owner.decorate.display_name(viewer: user)}" end - def email_subject_text - "Case owner changed for #{investigation.case_type}" + def body + metadata["rationale"] end private - def subtitle_slug - "Changed" + def owner + User.find_by(id: metadata["owner_id"]) || Team.find_by(id: metadata["owner_id"]) 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 }) + def subtitle_slug + "Changed" 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 when case added. This overrides + # inherited functionality in the Activity model :( + def notify_relevant_users; end end diff --git a/psd-web/app/models/comment_activity.rb b/psd-web/app/models/comment_activity.rb index cd9b92c896..cc753d64c6 100644 --- a/psd-web/app/models/comment_activity.rb +++ b/psd-web/app/models/comment_activity.rb @@ -4,7 +4,7 @@ class CommentActivity < Activity validates :body, presence: true validates :body, length: { maximum: 10_000 } - def title + def title(_user) "Comment added" end diff --git a/psd-web/app/models/investigation.rb b/psd-web/app/models/investigation.rb index bd8bf53d0d..7537f4fec8 100644 --- a/psd-web/app/models/investigation.rb +++ b/psd-web/app/models/investigation.rb @@ -20,15 +20,16 @@ class Investigation < ApplicationRecord validates :type, presence: true # Prevent saving instances of Investigation; must use a subclass instead validates :description, presence: true, on: :update - validates :owner_id, presence: { message: "Select case owner" }, on: :update + + # Can currently only validate this on update since create case wizards validate Investigation instances + validates :owner_id, presence: true, on: :update validates :user_title, length: { maximum: 100 } validates :description, length: { maximum: 10_000 } validates :non_compliant_reason, length: { maximum: 10_000 } validates :hazard_description, length: { maximum: 10_000 } - after_update :create_audit_activity_for_owner, - :create_audit_activity_for_status, + after_update :create_audit_activity_for_status, :create_audit_activity_for_visibility, :create_audit_activity_for_summary @@ -171,14 +172,6 @@ def create_audit_activity_for_visibility end end - def create_audit_activity_for_owner - # TODO: User.current check is here to avoid triggering activity and emails from migrations - # Can be safely removed once the migration PopulateAssigneeAndDescription has run - if ((saved_changes.key? :owner_id) || (saved_changes.key? :owner_type)) && User.current - AuditActivity::Investigation::UpdateOwner.from(self) - end - end - def create_audit_activity_for_summary # TODO: User.current check is here to avoid triggering activity and emails from migrations # Can be safely removed once the migration PopulateAssigneeAndDescription has run diff --git a/psd-web/app/services/change_case_owner.rb b/psd-web/app/services/change_case_owner.rb new file mode 100644 index 0000000000..926be0ac34 --- /dev/null +++ b/psd-web/app/services/change_case_owner.rb @@ -0,0 +1,77 @@ +class ChangeCaseOwner + include Interactor + include NotifyHelper + + delegate :investigation, :owner, :rationale, :user, :old_owner, to: :context + + def call + context.fail!(error: "No investigation supplied") unless investigation.is_a?(Investigation) + context.fail!(error: "No owner supplied") unless owner.is_a?(User) || owner.is_a?(Team) + context.fail!(error: "No user supplied") unless user.is_a?(User) + + context.old_owner = investigation.owner + + if old_owner != owner + investigation.owner = owner + + ActiveRecord::Base.transaction do + investigation.save! + + create_audit_activity_for_case_owner_changed + end + + send_notification_email + end + end + +private + + def create_audit_activity_for_case_owner_changed + metadata = activity_class.build_metadata(owner, rationale) + + activity_class.create!( + source: UserSource.new(user: user), + investigation: investigation, + title: nil, + body: nil, + metadata: metadata + ) + end + + def activity_class + AuditActivity::Investigation::UpdateOwner + end + + def send_notification_email + entities_to_notify.each do |recipient| + email = recipient.is_a?(Team) ? recipient.team_recipient_email : recipient.email + + NotifyMailer.investigation_updated( + investigation.pretty_id, + recipient.name, + email, + email_body(recipient), + "Case owner changed for #{investigation.case_type}" + ).deliver_later + end + end + + # Notify the new owner, and the old one if it's not the user making the change + def entities_to_notify + entities = [owner] + entities << old_owner if old_owner != user + + entities.map { |entity| + return entity.users.active if entity.is_a?(Team) && !entity.team_recipient_email + + entity + }.flatten.uniq + end + + def email_body(viewer = nil) + user_name = user.decorate.display_name(viewer: viewer) + body = "Case owner changed on #{investigation.case_type} to #{owner.decorate.display_name(viewer: viewer)} by #{user_name}." + body << "\n\nMessage from #{user_name}: #{inset_text_for_notify(rationale)}" if rationale + body + end +end diff --git a/psd-web/app/services/delete_user.rb b/psd-web/app/services/delete_user.rb index e8c28d3e4c..c1328e002f 100644 --- a/psd-web/app/services/delete_user.rb +++ b/psd-web/app/services/delete_user.rb @@ -1,12 +1,14 @@ class DeleteUser include Interactor + delegate :user, to: :context + def call - context.fail!(error: "No user supplied") unless context.user - context.fail!(error: "User already deleted") if context.user.deleted? + context.fail!(error: "No user supplied") unless user.is_a?(User) + context.fail!(error: "User already deleted") if user.deleted? ActiveRecord::Base.transaction do - context.user.mark_as_deleted! + user.mark_as_deleted! change_user_investigations_ownership_to_their_team end end @@ -16,10 +18,27 @@ def call def change_user_investigations_ownership_to_their_team context.team = context.user.team - context.user.investigations.each do |investigation| + user.investigations.each do |investigation| investigation.owner = context.team investigation.save! - AuditActivity::Investigation::AutomaticallyUpdateOwner.from(investigation) + + create_audit_activity_for_case_owner_automatically_changed(investigation) end end + + def create_audit_activity_for_case_owner_automatically_changed(investigation) + metadata = activity_class.build_metadata(investigation.owner) + + activity_class.create!( + source: nil, # DeleteUser is called from rake user:delete where no user source is available + investigation: investigation, + title: nil, + body: nil, + metadata: metadata + ) + end + + def activity_class + AuditActivity::Investigation::AutomaticallyUpdateOwner + end end diff --git a/psd-web/app/views/investigations/activities/_activity_card.html.erb b/psd-web/app/views/investigations/activities/_activity_card.html.erb index f4232c1036..4eba9a9ec2 100644 --- a/psd-web/app/views/investigations/activities/_activity_card.html.erb +++ b/psd-web/app/views/investigations/activities/_activity_card.html.erb @@ -1,12 +1,12 @@ <% restricted = !activity.can_display_all_data?(current_user) - title = restricted ? activity.restricted_title : activity.title + title = restricted ? activity.restricted_title(current_user) : activity.title(current_user) %>
  • <%= title %>

    -

    <%= activity.subtitle %>

    +

    <%= activity.subtitle(current_user) %>

    <% if restricted %> <%= render "restricted", activity: activity %> diff --git a/psd-web/app/views/investigations/activities/investigation/_update_owner.html.erb b/psd-web/app/views/investigations/activities/investigation/_update_owner.html.erb new file mode 100644 index 0000000000..4854ec1e62 --- /dev/null +++ b/psd-web/app/views/investigations/activities/investigation/_update_owner.html.erb @@ -0,0 +1 @@ +<%= markdown simple_format(activity.metadata["rationale"]) %> diff --git a/psd-web/config/locales/en.yml b/psd-web/config/locales/en.yml index cc56107760..456f2910f1 100644 --- a/psd-web/config/locales/en.yml +++ b/psd-web/config/locales/en.yml @@ -129,6 +129,10 @@ en: activemodel: errors: models: + change_case_owner_form: + attributes: + owner_id: + not_found: Select case owner invite_user_to_team_form: attributes: email: diff --git a/psd-web/lib/tasks/move_update_owner_activity_metadata.rb b/psd-web/lib/tasks/move_update_owner_activity_metadata.rb new file mode 100644 index 0000000000..4116047cb5 --- /dev/null +++ b/psd-web/lib/tasks/move_update_owner_activity_metadata.rb @@ -0,0 +1,38 @@ +desc "One-time task to move AuditActivity::Investigation::UpdateOwner metadata to metadata column" +task move_update_owner_activity_metadata: :environment do + klass = AuditActivity::Investigation::UpdateOwner + + ActiveRecord::Base.transaction do + klass.all.each do |activity| + next if activity.metadata + + # case owner ID is stored in the title column... + next unless (owner_id = activity.title) + + owner = User.find_by(id: owner_id) || Team.find_by(id: owner_id) + + metadata = klass.build_metadata(owner, activity.body) + + activity.update!(metadata: metadata, title: nil, body: nil) + end + end + + klass = AuditActivity::Investigation::AutomaticallyUpdateOwner + + ActiveRecord::Base.transaction do + klass.all.each do |activity| + next if activity.metadata + + # case owner ID is stored in the title column... + next unless (owner_id = activity.title) + + owner = User.find_by(id: owner_id) || Team.find_by(id: owner_id) + + metadata = klass.build_metadata(owner) + + activity.update!(metadata: metadata, title: nil, body: nil) + end + end + + puts "Done." +end diff --git a/psd-web/spec/features/change_ownership_for_investigation_spec.rb b/psd-web/spec/features/change_ownership_for_investigation_spec.rb index ab462a42c6..e618108125 100644 --- a/psd-web/spec/features/change_ownership_for_investigation_spec.rb +++ b/psd-web/spec/features/change_ownership_for_investigation_spec.rb @@ -12,7 +12,7 @@ before do sign_in(user) - visit "/cases/#{investigation.pretty_id}/assign/select-owner" + visit "/cases/#{investigation.pretty_id}/assign/new" end scenario "does not show inactive users" do diff --git a/psd-web/spec/features/record_email_activity_spec.rb b/psd-web/spec/features/record_email_activity_spec.rb index 8c5f809b23..06113b06e7 100644 --- a/psd-web/spec/features/record_email_activity_spec.rb +++ b/psd-web/spec/features/record_email_activity_spec.rb @@ -81,7 +81,7 @@ expect_to_be_on_case_activity_page(case_id: investigation.pretty_id) # Consumer info is not hidden from case owner - expect_case_activity_page_to_show_entered_information(name: name, email: email, date: date, file: file) + expect_case_activity_page_to_show_entered_information(user_name: user.name, name: name, email: email, date: date, file: file) click_link "View email" @@ -109,7 +109,7 @@ visit "/cases/#{investigation.pretty_id}/activity" expect_to_be_on_case_activity_page(case_id: investigation.pretty_id) - expect_case_activity_page_to_show_entered_information(name: name, email: email, date: date, file: file) + expect_case_activity_page_to_show_entered_information(user_name: user.name, name: name, email: email, date: date, file: file) end scenario "with non-consumer contact details and summary and subject and body" do @@ -135,7 +135,7 @@ click_on "Activity" expect_to_be_on_case_activity_page(case_id: investigation.pretty_id) - expect_case_activity_page_to_show_entered_information(name: name, email: email, date: date, summary: summary, subject: email_subject, body: body) + expect_case_activity_page_to_show_entered_information(user_name: user.name, name: name, email: email, date: date, summary: summary, subject: email_subject, body: body) # Test that another user in a different organisation can see all info sign_out @@ -145,7 +145,7 @@ visit "/cases/#{investigation.pretty_id}/activity" expect_to_be_on_case_activity_page(case_id: investigation.pretty_id) - expect_case_activity_page_to_show_entered_information(name: name, email: email, date: date, summary: summary, subject: email_subject, body: body) + expect_case_activity_page_to_show_entered_information(user_name: "#{user.name} (#{user.team.name})", name: name, email: email, date: date, summary: summary, subject: email_subject, body: body) end def fill_in_record_email_form(name:, email:, consumer:, date:) @@ -198,8 +198,8 @@ def expect_record_email_form_to_have_entered_information(name:, email:, consumer expect(find_field("Year").value).to eq date.year.to_s end - def expect_case_activity_page_to_show_entered_information(name:, email:, date:, file: nil, summary: nil, subject: nil, body: nil) - item = page.find("p", text: "Email recorded by #{user.name} (#{user.team.name})").find(:xpath, "..") + def expect_case_activity_page_to_show_entered_information(user_name:, name:, email:, date:, file: nil, summary: nil, subject: nil, body: nil) + item = page.find("p", text: "Email recorded by #{user_name}").find(:xpath, "..") expect(item).to have_text("From: #{name} (#{email})") expect(item).to have_text("Date sent: #{date.strftime('%d/%m/%Y')}") diff --git a/psd-web/spec/features/record_phone_call_activity_spec.rb b/psd-web/spec/features/record_phone_call_activity_spec.rb index d7dfc2fb9f..175436d6fa 100644 --- a/psd-web/spec/features/record_phone_call_activity_spec.rb +++ b/psd-web/spec/features/record_phone_call_activity_spec.rb @@ -78,7 +78,7 @@ expect_to_be_on_case_activity_page(case_id: investigation.pretty_id) # Consumer info is not hidden from case owner - expect_case_activity_page_to_show_entered_information(name: name, phone: phone, date: date, file: file) + expect_case_activity_page_to_show_entered_information(user_name: user.name, name: name, phone: phone, date: date, file: file) click_link "View phone call" @@ -106,7 +106,7 @@ visit "/cases/#{investigation.pretty_id}/activity" expect_to_be_on_case_activity_page(case_id: investigation.pretty_id) - expect_case_activity_page_to_show_entered_information(name: name, phone: phone, date: date, file: file) + expect_case_activity_page_to_show_entered_information(user_name: user.name, name: name, phone: phone, date: date, file: file) end scenario "with non-consumer contact details and summary and notes" do @@ -132,7 +132,7 @@ click_on "Activity" expect_to_be_on_case_activity_page(case_id: investigation.pretty_id) - expect_case_activity_page_to_show_entered_information(name: name, phone: phone, date: date, summary: summary, notes: notes) + expect_case_activity_page_to_show_entered_information(user_name: user.name, name: name, phone: phone, date: date, summary: summary, notes: notes) # Test that another user in a different organisation can see all info sign_out @@ -142,7 +142,7 @@ visit "/cases/#{investigation.pretty_id}/activity" expect_to_be_on_case_activity_page(case_id: investigation.pretty_id) - expect_case_activity_page_to_show_entered_information(name: name, phone: phone, date: date, summary: summary, notes: notes) + expect_case_activity_page_to_show_entered_information(user_name: "#{user.name} (#{user.team.name})", name: name, phone: phone, date: date, summary: summary, notes: notes) end def fill_in_record_phone_call_form(name:, phone:, consumer:, date:) @@ -189,8 +189,8 @@ def expect_record_phone_call_form_to_have_entered_information(name:, phone:, con expect(find_field("Year").value).to eq date.year.to_s end - def expect_case_activity_page_to_show_entered_information(name:, phone:, date:, file: nil, summary: nil, notes: nil) - item = page.find("p", text: "Phone call by #{user.name} (#{user.team.name})").find(:xpath, "..") + def expect_case_activity_page_to_show_entered_information(user_name:, name:, phone:, date:, file: nil, summary: nil, notes: nil) + item = page.find("p", text: "Phone call by #{user_name}").find(:xpath, "..") expect(item).to have_text("Call with: #{name} (#{phone})") expect(item).to have_text("Date: #{date.strftime('%d/%m/%Y')}") diff --git a/psd-web/spec/forms/edit_investigation_collaborator_form_spec.rb b/psd-web/spec/forms/edit_investigation_collaborator_form_spec.rb index 8e5e1bfb67..ca0df213f2 100644 --- a/psd-web/spec/forms/edit_investigation_collaborator_form_spec.rb +++ b/psd-web/spec/forms/edit_investigation_collaborator_form_spec.rb @@ -72,7 +72,7 @@ last_added_activity = investigation.activities.reload.order(created_at: :desc) .find_by!(type: "AuditActivity::Investigation::TeamDeleted") - expect(last_added_activity.title).to eql("test team removed from allegation") + expect(last_added_activity.title(user)).to eql("test team removed from allegation") expect(last_added_activity.source.user_id).to eql(user.id) end end diff --git a/psd-web/spec/models/audit_activity/investigation/automatically_update_owner_spec.rb b/psd-web/spec/models/audit_activity/investigation/automatically_update_owner_spec.rb deleted file mode 100644 index 67614dd4f4..0000000000 --- a/psd-web/spec/models/audit_activity/investigation/automatically_update_owner_spec.rb +++ /dev/null @@ -1,19 +0,0 @@ -require "rails_helper" - -RSpec.describe AuditActivity::Investigation::AutomaticallyUpdateOwner, :with_stubbed_mailer, :with_stubbed_elasticsearch do - subject(:audit_activity) { described_class.from(investigation) } - - let(:team) { create(:team) } - let(:investigation) { create(:enquiry, owner: team, owner_rationale: "Test owner") } - - describe ".from" do - it "creates an audit activity without information for email notification" do - expect(audit_activity).to have_attributes( - title: "Case owner automatically changed on Enquiry to #{team.display_name}", - body: nil, - email_update_text: nil, - email_subject_text: nil - ) - end - end -end diff --git a/psd-web/spec/models/audit_activity/investigation/update_coronavirus_status_spec.rb b/psd-web/spec/models/audit_activity/investigation/update_coronavirus_status_spec.rb index f8e52d215b..f4a729f816 100644 --- a/psd-web/spec/models/audit_activity/investigation/update_coronavirus_status_spec.rb +++ b/psd-web/spec/models/audit_activity/investigation/update_coronavirus_status_spec.rb @@ -11,10 +11,10 @@ describe ".from" do it "creates an audit activity", :aggregate_failures do expect(audit_activity).to have_attributes( - title: "Status updated: not coronavirus related", body: "The case is not related to the coronavirus outbreak.", email_subject_text: "Coronavirus status updated on #{investigation.case_type.downcase}" ) + expect(audit_activity.title(user)).to eq("Status updated: not coronavirus related") expect(audit_activity.email_update_text(user)).to eq("#{investigation.case_type.capitalize} #{investigation.pretty_id} is not related to the coronavirus outbreak. This status was updated by #{user.name}.") end end diff --git a/psd-web/spec/models/audit_activity/investigation/update_owner_spec.rb b/psd-web/spec/models/audit_activity/investigation/update_owner_spec.rb deleted file mode 100644 index 52febdf944..0000000000 --- a/psd-web/spec/models/audit_activity/investigation/update_owner_spec.rb +++ /dev/null @@ -1,21 +0,0 @@ -require "rails_helper" - -RSpec.describe AuditActivity::Investigation::UpdateOwner, :with_stubbed_mailer, :with_stubbed_elasticsearch do - subject(:audit_activity) { described_class.from(investigation) } - - let(:user) { create(:user).decorate } - let(:investigation) { create(:enquiry, owner: user, owner_rationale: "Test owner") } - - before { User.current = user } - - describe ".from" do - it "creates an audit activity with information for email notification", :aggregate_failures do - expect(audit_activity).to have_attributes( - title: "Case owner changed to #{user.display_name}", - body: investigation.owner_rationale, - email_subject_text: "Case owner changed for enquiry" - ) - expect(audit_activity.email_update_text(user)).to start_with("Case owner changed on enquiry to #{user.name} by #{user.name}") - end - end -end diff --git a/psd-web/spec/services/add_product_to_case_spec.rb b/psd-web/spec/services/add_product_to_case_spec.rb index 2b067b83ea..7297fe7a57 100644 --- a/psd-web/spec/services/add_product_to_case_spec.rb +++ b/psd-web/spec/services/add_product_to_case_spec.rb @@ -59,7 +59,7 @@ expect(activity).to be_a(AuditActivity::Product::Add) expect(activity.source.user).to eq(user) expect(activity.product_id).to eq(product.id) - expect(activity.title).to eq(product.name) + expect(activity.title(nil)).to eq(product.name) end context "when the case owner is a user" do diff --git a/psd-web/spec/services/add_team_to_an_investigation_spec.rb b/psd-web/spec/services/add_team_to_an_investigation_spec.rb index 4213bc7d9e..cbce7adbb8 100644 --- a/psd-web/spec/services/add_team_to_an_investigation_spec.rb +++ b/psd-web/spec/services/add_team_to_an_investigation_spec.rb @@ -51,7 +51,7 @@ aggregate_failures do expect(last_added_activity).to be_a(AuditActivity::Investigation::TeamAdded) - expect(last_added_activity.title).to eql("Testing team added to allegation") + expect(last_added_activity.title(nil)).to eql("Testing team added to allegation") expect(last_added_activity.source.user_id).to eql(user.id) end end diff --git a/psd-web/spec/services/create_case_spec.rb b/psd-web/spec/services/create_case_spec.rb index 8f80ae8d65..ac34727fe9 100644 --- a/psd-web/spec/services/create_case_spec.rb +++ b/psd-web/spec/services/create_case_spec.rb @@ -77,7 +77,7 @@ expect(activity).to be_a(AuditActivity::Product::Add) expect(activity.source.user).to eq(user) expect(activity.product_id).to eq(product.id) - expect(activity.title).to eq(product.name) + expect(activity.title(user)).to eq(product.name) end end diff --git a/psd-web/spec/services/delete_user_spec.rb b/psd-web/spec/services/delete_user_spec.rb index fe63390a2c..60848142d2 100644 --- a/psd-web/spec/services/delete_user_spec.rb +++ b/psd-web/spec/services/delete_user_spec.rb @@ -67,8 +67,8 @@ expect { delete_call }.to change(update_owner_activities, :count).by(3) update_owner_activities.last(3).each do |activity| - expect(activity.title).to start_with "Case owner automatically changed on" - expect(activity.title).to end_with "to #{user.team.display_name}" + expect(activity.title(user)).to start_with "Case owner automatically changed on" + expect(activity.title(user)).to end_with "to #{user.team.name}" end end # rubocop:enable RSpec/ExampleLength diff --git a/psd-web/spec/services/remove_product_from_case_spec.rb b/psd-web/spec/services/remove_product_from_case_spec.rb index eeabbbf297..ed9bed5ad7 100644 --- a/psd-web/spec/services/remove_product_from_case_spec.rb +++ b/psd-web/spec/services/remove_product_from_case_spec.rb @@ -59,7 +59,7 @@ expect(activity).to be_a(AuditActivity::Product::Destroy) expect(activity.source.user).to eq(user) expect(activity.product_id).to eq(product.id) - expect(activity.title).to eq("Removed: #{product.name}") + expect(activity.title(nil)).to eq("Removed: #{product.name}") end context "when the case owner is a user" do diff --git a/psd-web/spec/support/audit_activity_investigation.rb b/psd-web/spec/support/audit_activity_investigation.rb index 35b69791c3..2c4c96b0e0 100644 --- a/psd-web/spec/support/audit_activity_investigation.rb +++ b/psd-web/spec/support/audit_activity_investigation.rb @@ -95,7 +95,7 @@ end describe "#title" do - subject(:title) { activity.title } + subject(:title) { activity.title(nil) } let(:activity) { described_class.create(investigation: investigation, metadata: metadata, title: test_title) } let(:test_title) { nil } @@ -170,14 +170,14 @@ end describe "#restricted_title" do - # This metod will only ever be called for older records with no metadata, + # This method will only ever be called for older records with no metadata, # where the title is pre-generated and stored in the database, so we will # set the title here subject(:activity) { described_class.create(investigation: investigation, title: "Test title") } # titles never contain GDPR data for these activity classes so just return the title it "returns the title" do - expect(activity.restricted_title).to eq("Test title") + expect(activity.restricted_title(nil)).to eq("Test title") end end end