Merge remote-tracking branch 'origin/master' into attachments-changes

* origin/master:
  Bump rubocop and rubocop-govuk in /psd-web (#704)
  Only show complainant details on case added activity if user is collaborating on the case (#696)
  Fix error when trying to display automatically updated case ownership change activity (#708)
  Remove User.current dependency from display_name and show methods (#701)
  Refactor update owner audit activity to store metadata, and remove callbacks and dependency on User.current (#700)

CHANGELOG.md

@@ -1,6 +1,9 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## 2020-06-12
+- Users of all teams added to a case can now view complainant contact details on the case activity page.
+
 ## 2020-06-05
 – Added the ability to view the details of correspondence added to the case on their own page.
 

psd-web/Gemfile

@@ -63,7 +63,7 @@ group :development, :test do
   gem "rspec-mocks"
   gem "rspec-rails"
   gem "rubocop"
-  gem "rubocop-govuk", "~> 3.14.0"
+  gem "rubocop-govuk", "~> 3.16.0"
   gem "rubocop-performance"
   gem "rubocop-rspec", "~> 1.39", require: false
   gem "ruby-debug-ide"

psd-web/Gemfile.lock

@@ -72,7 +72,7 @@ GEM
       public_suffix (>= 2.0.2, < 5.0)
     ansi (1.5.0)
     arel (9.0.0)
-    ast (2.4.0)
+    ast (2.4.1)
     awesome_print (1.8.0)
     aws-eventstream (1.1.0)
     aws-partitions (1.323.0)
@@ -391,24 +391,28 @@ GEM
       rspec-mocks (~> 3.9)
       rspec-support (~> 3.9)
     rspec-support (3.9.3)
-    rubocop (0.83.0)
+    rubocop (0.85.1)
       parallel (~> 1.10)
       parser (>= 2.7.0.1)
       rainbow (>= 2.2.2, < 4.0)
+      regexp_parser (>= 1.7)
       rexml
+      rubocop-ast (>= 0.0.3)
       ruby-progressbar (~> 1.7)
       unicode-display_width (>= 1.4.0, < 2.0)
-    rubocop-govuk (3.14.0)
-      rubocop (= 0.83.0)
-      rubocop-rails (~> 2)
-      rubocop-rake (~> 0.5.1)
-      rubocop-rspec (~> 1.28)
+    rubocop-ast (0.0.3)
+      parser (>= 2.7.0.1)
+    rubocop-govuk (3.16.0)
+      rubocop (= 0.85.1)
+      rubocop-rails (= 2.6.0)
+      rubocop-rake (= 0.5.1)
+      rubocop-rspec (= 1.39.0)
     rubocop-performance (1.6.1)
       rubocop (>= 0.71.0)
-    rubocop-rails (2.5.2)
-      activesupport
+    rubocop-rails (2.6.0)
+      activesupport (>= 4.2.0)
       rack (>= 1.1)
-      rubocop (>= 0.72.0)
+      rubocop (>= 0.82.0)
     rubocop-rake (0.5.1)
       rubocop
     rubocop-rspec (1.39.0)
@@ -588,7 +592,7 @@ DEPENDENCIES
   rspec-mocks
   rspec-rails
   rubocop
-  rubocop-govuk (~> 3.14.0)
+  rubocop-govuk (~> 3.16.0)
   rubocop-performance
   rubocop-rspec (~> 1.39)
   ruby-debug-ide

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/decorators/user_decorator.rb

@@ -13,7 +13,7 @@ def full_name
   end
 
   # viewer could be a Team or User
-  def display_name(viewer: User.current)
+  def display_name(viewer:)
     suffix = " (#{team.name})" if team != viewer&.team
     "#{name}#{suffix}#{deleted_suffix}"
   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/display_text_helper.rb

@@ -91,7 +91,7 @@ def gdpr_restriction_text
 
   def should_be_hidden(result, source, investigation)
     return true if correspondence_should_be_hidden(result, source, investigation)
-    return true if (source.include? "complainant") && !investigation&.complainant&.can_be_displayed?(current_user)
+    return true if (source.include? "complainant") && !policy(investigation).view_protected_details?
 
     false
   end

psd-web/app/helpers/investigations/user_filters_helper.rb

@@ -27,7 +27,7 @@ def other_owner(form)
     render "form_components/govuk_select",
            key: :case_owner_is_someone_else_id,
            form: form,
-           items: entities.map { |e| { text: e.display_name, value: e.id } },
+           items: entities.map { |e| { text: e.display_name(viewer: current_user), value: e.id } },
            label: { text: "Name" },
            is_autocomplete: true
   end
@@ -36,7 +36,7 @@ def other_creator(form)
     render "form_components/govuk_select",
            key: :created_by_someone_else_id,
            form: form,
-           items: entities.map { |e| { text: e.display_name, value: e.id } },
+           items: entities.map { |e| { text: e.display_name(viewer: current_user), value: e.id } },
            label: { text: "Name" },
            is_autocomplete: true
   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']}"
@@ -45,12 +45,12 @@ def title
   def can_display_all_data?(user)
     return true if self[:metadata].present? || investigation.complainant.blank?
 
-    investigation.complainant.can_be_displayed?(user)
+    Pundit.policy(user, investigation).view_protected_details?
   end
 
   # Only used for old records prior to metadata implementation
-  def restricted_title
-    title
+  def restricted_title(user)
+    title(user)
   end
 
 private