CLDC-1063: Key Contacts (#435)

* Add data field

* Add field to views

* Only data coordinators can change role, dpo, keycontact

* Import key contact field

* Rubocop

* Import data accessors

* Handle non-unqiue email imports

app/controllers/users_controller.rb

@@ -3,7 +3,7 @@ class UsersController < ApplicationController
   include Helpers::Email
   before_action :authenticate_user!
   before_action :find_resource, except: %i[new create]
-  before_action :authenticate_scope!, except: %i[new create]
+  before_action :authenticate_scope!, except: %i[new]
 
   def update
     if @user.update(user_params)
@@ -76,9 +76,13 @@ private
 
   def user_params
     if @user == current_user
-      params.require(:user).permit(:email, :name, :password, :password_confirmation, :role, :is_dpo)
+      if current_user.data_coordinator?
+        params.require(:user).permit(:email, :name, :password, :password_confirmation, :role, :is_dpo, :is_key_contact)
       else
-      params.require(:user).permit(:email, :name, :role, :is_dpo)
+        params.require(:user).permit(:email, :name, :password, :password_confirmation)
+      end
+    elsif current_user.data_coordinator?
+      params.require(:user).permit(:email, :name, :role, :is_dpo, :is_key_contact)
     end
   end
 
@@ -87,8 +91,12 @@ private
   end
 
   def authenticate_scope!
+    if action_name == "create"
+      head :unauthorized and return unless current_user.data_coordinator?
+    else
       render_not_found and return unless current_user.organisation == @user.organisation
       render_not_found and return if action_name == "edit_password" && current_user != @user
-    render_not_found and return unless current_user.role == "data_coordinator" || current_user == @user
+      render_not_found and return unless current_user.data_coordinator? || current_user == @user
+    end
   end
 end

app/models/user.rb

@@ -37,6 +37,14 @@ class User < ApplicationRecord
     last_sign_in_at ? RESET_PASSWORD_TEMPLATE_ID : SET_PASSWORD_TEMPLATE_ID
   end
 
+  def is_key_contact?
+    is_key_contact
+  end
+
+  def is_key_contact!
+    update(is_key_contact: true)
+  end
+
   def is_data_protection_officer?
     is_dpo
   end

app/services/imports/user_import_service.rb

@@ -14,17 +14,24 @@ module Imports
       organisation = Organisation.find_by(old_org_id: user_field_value(xml_document, "institution"))
       old_user_id = user_field_value(xml_document, "id")
       name = user_field_value(xml_document, "full-name")
+      email = user_field_value(xml_document, "email")
       if User.find_by(old_user_id:, organisation:)
         @logger.warn("User #{name} with old user id #{old_user_id} is already present, skipping.")
+      elsif (user = User.find_by(email:, organisation:))
+        is_dpo = user.is_data_protection_officer? || is_dpo?(user_field_value(xml_document, "user-type"))
+        role = highest_role(user.role, role(user_field_value(xml_document, "user-type")))
+        user.update!(role:, is_dpo:)
       else
         User.create!(
-          email: user_field_value(xml_document, "user-name"),
+          email:,
           name:,
           password: Devise.friendly_token,
           phone: user_field_value(xml_document, "telephone-no"),
           old_user_id:,
           organisation:,
-          role: PROVIDER_TYPE[user_field_value(xml_document, "user-type")],
+          role: role(user_field_value(xml_document, "user-type")),
+          is_dpo: is_dpo?(user_field_value(xml_document, "user-type")),
+          is_key_contact: is_key_contact?(user_field_value(xml_document, "contact-priority-id")),
         )
       end
     end
@@ -32,5 +39,35 @@ module Imports
     def user_field_value(xml_document, field)
       field_value(xml_document, "user", field)
     end
+
+    def role(field_value)
+      return unless field_value
+
+      {
+        "co-ordinator" => "data_coordinator",
+        "data provider" => "data_provider",
+        "private data downloader" => "data_accessor",
+      }[field_value.downcase.strip]
+    end
+
+    def highest_role(role_a, role_b)
+      return unless role_a || role_b
+      return role_a unless role_b
+      return role_b unless role_a
+
+      [role_a, role_b].map(&:to_sym).sort! { |a, b| User::ROLES[b] <=> User::ROLES[a] }.first
+    end
+
+    def is_dpo?(field_value)
+      return false if field_value.blank?
+
+      field_value.downcase.strip == "data protection officer"
+    end
+
+    def is_key_contact?(field_value)
+      return false if field_value.blank?
+
+      ["ecore contact", "key performance contact"].include?(field_value.downcase.strip)
+    end
   end
 end

app/views/users/edit.html.erb

@@ -26,6 +26,7 @@
         spellcheck: "false"
       %>
 
+      <% if current_user.data_coordinator? %>
         <%= f.govuk_collection_radio_buttons :is_dpo,
           [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")],
           :id,
@@ -34,6 +35,15 @@
           legend: { text: "Are #{current_user == @user ? "you" : "they"} a data protection officer?", size: "m" }
         %>
 
+        <%= f.govuk_collection_radio_buttons :is_key_contact,
+          [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")],
+          :id,
+          :name,
+          inline: true,
+          legend: { text: "Are #{current_user == @user ? "you" : "they"} a key contact?", size: "m" }
+        %>
+      <% end %>
+
       <%= f.govuk_submit "Save changes" %>
     </div>
   </div>

app/views/users/new.html.erb

@@ -31,6 +31,7 @@
         f.govuk_collection_radio_buttons :role, roles, :id, :name, legend: { text: "Role", size: "m" }
       %>
 
+      <% if current_user.data_coordinator? %>
         <%= f.govuk_collection_radio_buttons :is_dpo,
           [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")],
           :id,
@@ -39,6 +40,15 @@
           legend: { text: "Are #{current_user == @user ? "you" : "they"} a data protection officer?", size: "m" }
         %>
 
+        <%= f.govuk_collection_radio_buttons :is_key_contact,
+          [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")],
+          :id,
+          :name,
+          inline: true,
+          legend: { text: "Are #{current_user == @user ? "you" : "they"} a key contact?", size: "m" }
+        %>
+      <% end %>
+
       <%= f.govuk_submit "Continue" %>
     </div>
   </div>

app/views/users/show.html.erb

@@ -48,7 +48,21 @@
       <%= summary_list.row do |row|
         row.key { 'Data protection officer' }
         row.value { @user.is_data_protection_officer? ? "Yes" : "No" }
+        if current_user.data_coordinator?
           row.action(visually_hidden_text: "are #{current_user == @user ? "you" : "they"} a data protection officer?", href: edit_user_path, html_attributes: { "data-qa": "change-are-#{current_user == @user ? "you" : "they"}-a-data-protection-officer" })
+        else
+          row.action()
+        end
+      end %>
+
+      <%= summary_list.row do |row|
+        row.key { 'Key contact' }
+        row.value { @user.is_key_contact? ? "Yes" : "No" }
+        if current_user.data_coordinator?
+          row.action(visually_hidden_text: "are #{current_user == @user ? "you" : "they"} a key contact?", href: edit_user_path, html_attributes: { "data-qa": "change-are-#{current_user == @user ? "you" : "they"}-a-key-contact" })
+        else
+          row.action()
+        end
       end %>
     <% end %>
   </div>

db/migrate/20220329125601_add_key_contact_field_to_user.rb

@@ -0,0 +1,7 @@
+class AddKeyContactFieldToUser < ActiveRecord::Migration[7.0]
+  def change
+    change_table :users, bulk: true do |t|
+      t.column :is_key_contact, :boolean, default: false
+    end
+  end
+end

db/schema.rb

@@ -317,6 +317,7 @@ ActiveRecord::Schema[7.0].define(version: 202202071123100) do
     t.string "unlock_token"
     t.datetime "locked_at", precision: nil
     t.boolean "is_dpo", default: false
+    t.boolean "is_key_contact", default: false
     t.string "phone"
     t.index ["email"], name: "index_users_on_email", unique: true
     t.index ["organisation_id"], name: "index_users_on_organisation_id"

spec/features/user_spec.rb

@@ -172,6 +172,32 @@ RSpec.describe "User Features" do
     end
   end
 
+  context "when signed in as a data provider" do
+    context "when viewing your account" do
+      before do
+        visit("/logs")
+        fill_in("user[email]", with: user.email)
+        fill_in("user[password]", with: "pAssword1")
+        click_button("Sign in")
+      end
+
+      it "does not have change links for dpo and key contact" do
+        visit("/users/#{user.id}")
+        expect(page).not_to have_selector('[data-qa="change-are-you-a-data-protection-officer"]')
+        expect(page).not_to have_selector('[data-qa="change-are-you-a-key-contact"]')
+      end
+
+      it "does not have dpo and key contact as editable fields" do
+        visit("/users/#{user.id}/edit")
+        expect(page).not_to have_field("user[is_dpo]")
+        expect(page).not_to have_field("user[is_key_contact]")
+      end
+    end
+  end
+
+  context "when signed in as a data coordinator" do
+    let!(:user) { FactoryBot.create(:user, :data_coordinator, last_sign_in_at: Time.zone.now) }
+
     context "when viewing your account" do
       before do
         visit("/logs")
@@ -206,6 +232,12 @@ RSpec.describe "User Features" do
         expect(page).to have_current_path("/users/#{user.id}")
         expect(page).to have_content("Test New")
       end
+
+      it "has dpo and key contact as editable fields" do
+        visit("/users/#{user.id}")
+        expect(page).to have_selector('[data-qa="change-are-you-a-data-protection-officer"]')
+        expect(page).to have_selector('[data-qa="change-are-you-a-key-contact"]')
+      end
     end
 
     context "when adding a new user" do
@@ -237,16 +269,21 @@ RSpec.describe "User Features" do
         expect(page).to have_title("Error")
       end
 
-    it "sets name, email, role and is_dpo" do
+      it "sets name, email, role, is_dpo and is_key_contact fields" do
         visit("users/new")
         fill_in("user[name]", with: "New User")
         fill_in("user[email]", with: "newuser@example.com")
         choose("user-role-data-provider-field")
         choose("user-is-dpo-true-field")
+        choose("user-is-key-contact-true-field")
         click_button("Continue")
-      expect(
+        expect(User.find_by(
-        User.find_by(name: "New User", email: "newuser@example.com", role: "data_provider", is_dpo: true),
+                 name: "New User",
-      ).to be_a(User)
+                 email: "newuser@example.com",
+                 role: "data_provider",
+                 is_dpo: true,
+                 is_key_contact: true,
+               )).to be_a(User)
       end
 
       it "defaults to is_dpo false" do
@@ -274,10 +311,17 @@ RSpec.describe "User Features" do
         first(:link, "Change").click
         expect(page).to have_field("user[is_dpo]", with: true)
         choose("user-is-dpo-field")
+        choose("user-is-key-contact-true-field")
         fill_in("user[name]", with: "Updated new name")
         click_button("Save changes")
         expect(page).to have_title("Updated new name’s account")
-      expect(User.find_by(name: "Updated new name", role: "data_provider", is_dpo: false)).to be_a(User)
+        expect(User.find_by(
+                 name: "Updated new name",
+                 role: "data_provider",
+                 is_dpo: false,
+                 is_key_contact: true,
+               )).to be_a(User)
+      end
     end
   end
 end

spec/fixtures/softwire_imports/users/10c887710550844e2551b3e0fb88dc9b4a8a642b.xml

@@ -0,0 +1,13 @@
+<user:user xmlns:user="dclg:user">
+  <user:id>10c887710550844e2551b3e0fb88dc9b4a8a642b</user:id>
+  <user:password>xxx</user:password>
+  <user:full-name>John Doe</user:full-name>
+  <user:user-name>john.doe</user:user-name>
+  <user:institution>7c5bd5fb549c09a2c55d7cb90d7ba84927e64618</user:institution>
+  <user:email>john.doe@gov.uk</user:email>
+  <user:user-type>Data Protection Officer</user:user-type>
+  <user:active>true</user:active>
+  <user:deleted>false</user:deleted>
+  <user:contact-priority-id>None</user:contact-priority-id>
+  <user:telephone-no>02012345678</user:telephone-no>
+</user:user>

spec/fixtures/softwire_imports/users/b7829b1a5dfb68bb1e01c08445830c0add40907c.xml

@@ -0,0 +1,13 @@
+<user:user xmlns:user="dclg:user">
+  <user:id>b7829b1a5dfb68bb1e01c08445830c0add40907c</user:id>
+  <user:password>xxx</user:password>
+  <user:full-name>John Doe</user:full-name>
+  <user:user-name>john.doe</user:user-name>
+  <user:institution>7c5bd5fb549c09a2c55d7cb90d7ba84927e64618</user:institution>
+  <user:email>john.doe@gov.uk</user:email>
+  <user:user-type>Private Data Downloader</user:user-type>
+  <user:active>true</user:active>
+  <user:deleted>false</user:deleted>
+  <user:contact-priority-id>None</user:contact-priority-id>
+  <user:telephone-no>02012345678</user:telephone-no>
+</user:user>

spec/fixtures/softwire_imports/users/d4729b1a5dfb68bb1e01c08445830c0add40907c.xml

@@ -0,0 +1,13 @@
+<user:user xmlns:user="dclg:user">
+  <user:id>d4729b1a5dfb68bb1e01c08445830c0add40907c</user:id>
+  <user:password>xxx</user:password>
+  <user:full-name>John Doe</user:full-name>
+  <user:user-name>john.doe</user:user-name>
+  <user:institution>7c5bd5fb549c09a2c55d7cb90d7ba84927e64618</user:institution>
+  <user:email>john.doe@gov.uk</user:email>
+  <user:user-type>Co-ordinator</user:user-type>
+  <user:active>true</user:active>
+  <user:deleted>false</user:deleted>
+  <user:contact-priority-id>Key Performance Contact</user:contact-priority-id>
+  <user:telephone-no>02012345678</user:telephone-no>
+</user:user>

spec/fixtures/softwire_imports/users/d6717836154cd9a58f9e2f1d3077e3ab81e07613.xml

@@ -0,0 +1,13 @@
+<user:user xmlns:user="dclg:user">
+  <user:id>d6717836154cd9a58f9e2f1d3077e3ab81e07613</user:id>
+  <user:password>xxx</user:password>
+  <user:full-name>John Doe</user:full-name>
+  <user:user-name>john.doe</user:user-name>
+  <user:institution>7c5bd5fb549c09a2c55d7cb90d7ba84927e64618</user:institution>
+  <user:email>john.doe@gov.uk</user:email>
+  <user:user-type>Co-ordinator</user:user-type>
+  <user:active>true</user:active>
+  <user:deleted>false</user:deleted>
+  <user:contact-priority-id>eCORE Contact</user:contact-priority-id>
+  <user:telephone-no>02012345678</user:telephone-no>
+</user:user>

spec/fixtures/softwire_imports/users/fc7625a02b24ae16162aa63ae7cb33feeec0c373.xml

@@ -2,7 +2,7 @@
   <user:id>fc7625a02b24ae16162aa63ae7cb33feeec0c373</user:id>
   <user:password>xxx</user:password>
   <user:full-name>John Doe</user:full-name>
-  <user:user-name>john.doe@gov.uk</user:user-name>
+  <user:user-name>john.doe</user:user-name>
   <user:institution>7c5bd5fb549c09a2c55d7cb90d7ba84927e64618</user:institution>
   <user:email>john.doe@gov.uk</user:email>
   <user:user-type>Data Provider</user:user-type>

spec/models/user_spec.rb

@@ -47,13 +47,17 @@ RSpec.describe User, type: :model do
       expect(user.data_coordinator?).to be false
     end
 
+    it "is not a key contact by default" do
+      expect(user.is_key_contact?).to be false
+    end
+
     it "is not a data protection officer by default" do
       expect(user.is_data_protection_officer?).to be false
     end
 
-    it "can be set to data protection officer" do
+    it "can be set to key contact" do
-      expect { user.is_data_protection_officer! }
+      expect { user.is_key_contact! }
-        .to change { user.reload.is_data_protection_officer? }.from(false).to(true)
+        .to change { user.reload.is_key_contact? }.from(false).to(true)
     end
   end
 

spec/requests/users_controller_spec.rb

@@ -210,13 +210,14 @@ RSpec.describe UsersController, type: :request do
           expect(whodunnit_actor.id).to eq(user.id)
         end
 
-        context "when user changes email and dpo" do
+        context "when user changes email, dpo, key_contact" do
-          let(:params) { { id: user.id, user: { name: new_name, email: new_email, is_dpo: "true" } } }
+          let(:params) { { id: user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } }
 
-          it "allows changing email and dpo" do
+          it "allows changing email but not dpo or key_contact" do
             user.reload
             expect(user.email).to eq(new_email)
-            expect(user.is_data_protection_officer?).to be true
+            expect(user.is_data_protection_officer?).to be false
+            expect(user.is_key_contact?).to be false
           end
         end
       end
@@ -265,6 +266,32 @@ RSpec.describe UsersController, type: :request do
         end
       end
     end
+
+    describe "#create" do
+      let(:params) do
+        {
+          "user": {
+            name: "new user",
+            email: "new_user@example.com",
+            role: "data_coordinator",
+          },
+        }
+      end
+      let(:request) { post "/users/", headers: headers, params: params }
+
+      before do
+        sign_in user
+      end
+
+      it "does not invite a new user" do
+        expect { request }.not_to change(User, :count)
+      end
+
+      it "returns 401 unauthorized" do
+        request
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
   end
 
   context "when user is signed in as a data coordinator" do
@@ -399,12 +426,13 @@ RSpec.describe UsersController, type: :request do
         end
 
         context "when user changes email and dpo" do
-          let(:params) { { id: user.id, user: { name: new_name, email: new_email, is_dpo: "true" } } }
+          let(:params) { { id: user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } }
 
           it "allows changing email and dpo" do
             user.reload
             expect(user.email).to eq(new_email)
             expect(user.is_data_protection_officer?).to be true
+            expect(user.is_key_contact?).to be true
           end
         end
 
@@ -443,14 +471,15 @@ RSpec.describe UsersController, type: :request do
               .to change { other_user.reload.versions.last.actor&.id }.from(nil).to(user.id)
           end
 
-          context "when user changes email and dpo" do
+          context "when user changes email, dpo, key_contact" do
-            let(:params) { { id: other_user.id, user: { name: new_name, email: new_email, is_dpo: "true" } } }
+            let(:params) { { id: other_user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } }
 
-            it "allows changing email and dpo" do
+            it "allows changing email, dpo, key_contact" do
               patch "/users/#{other_user.id}", headers: headers, params: params
               other_user.reload
               expect(other_user.email).to eq(new_email)
               expect(other_user.is_data_protection_officer?).to be true
+              expect(other_user.is_key_contact?).to be true
             end
           end
 
@@ -510,9 +539,9 @@ RSpec.describe UsersController, type: :request do
         end
       end
     end
-  end
 
     describe "#create" do
+      let(:user) { FactoryBot.create(:user, :data_coordinator) }
       let(:params) do
         {
           "user": {
@@ -549,6 +578,7 @@ RSpec.describe UsersController, type: :request do
         end
       end
     end
+  end
 
   describe "title link" do
     before do

spec/services/imports/user_import_service_spec.rb

@@ -29,6 +29,7 @@ RSpec.describe Imports::UserImportService do
       expect(user.phone).to eq("02012345678")
       expect(user).to be_data_provider
       expect(user.organisation.old_org_id).to eq(old_org_id)
+      expect(user.is_key_contact?).to be false
     end
 
     it "refuses to create a user belonging to a non existing organisation" do
@@ -36,6 +37,62 @@ RSpec.describe Imports::UserImportService do
         .to raise_error(ActiveRecord::RecordInvalid, /Organisation must exist/)
     end
 
+    context "when the user is a data coordinator" do
+      let(:old_user_id) { "d4729b1a5dfb68bb1e01c08445830c0add40907c" }
+
+      it "sets their role correctly" do
+        FactoryBot.create(:organisation, old_org_id:)
+        import_service.create_users("user_directory")
+        expect(User.find_by(old_user_id:)).to be_data_coordinator
+      end
+    end
+
+    context "when the user is a data accessor" do
+      let(:old_user_id) { "b7829b1a5dfb68bb1e01c08445830c0add40907c" }
+
+      it "sets their role correctly" do
+        FactoryBot.create(:organisation, old_org_id:)
+        import_service.create_users("user_directory")
+        expect(User.find_by(old_user_id:)).to be_data_accessor
+      end
+    end
+
+    context "when the user is a data protection officer" do
+      let(:old_user_id) { "10c887710550844e2551b3e0fb88dc9b4a8a642b" }
+
+      it "marks them as a data protection officer" do
+        FactoryBot.create(:organisation, old_org_id:)
+        import_service.create_users("user_directory")
+
+        user = User.find_by(old_user_id:)
+        expect(user.is_data_protection_officer?).to be true
+      end
+    end
+
+    context "when the user was a 'Key Performance Contact' in the old system" do
+      let(:old_user_id) { "d4729b1a5dfb68bb1e01c08445830c0add40907c" }
+
+      it "marks them as a key contact" do
+        FactoryBot.create(:organisation, old_org_id:)
+        import_service.create_users("user_directory")
+
+        user = User.find_by(old_user_id:)
+        expect(user.is_key_contact?).to be true
+      end
+    end
+
+    context "when the user was a 'eCORE Contact' in the old system" do
+      let(:old_user_id) { "d6717836154cd9a58f9e2f1d3077e3ab81e07613" }
+
+      it "marks them as a key contact" do
+        FactoryBot.create(:organisation, old_org_id:)
+        import_service.create_users("user_directory")
+
+        user = User.find_by(old_user_id:)
+        expect(user.is_key_contact?).to be true
+      end
+    end
+
     context "when the user has already been imported previously" do
       before do
         org = FactoryBot.create(:organisation, old_org_id:)
@@ -47,5 +104,54 @@ RSpec.describe Imports::UserImportService do
         import_service.create_users("user_directory")
       end
     end
+
+    context "when a user has already been imported with that email" do
+      let!(:org) { FactoryBot.create(:organisation, old_org_id:) }
+      let!(:user) { FactoryBot.create(:user, :data_provider, organisation: org, email: "john.doe@gov.uk") }
+
+      context "when the duplicate role is higher than the original role" do
+        let(:old_user_id) { "d4729b1a5dfb68bb1e01c08445830c0add40907c" }
+
+        it "upgrades their role" do
+          import_service.create_users("user_directory")
+          expect(user.reload).to be_data_coordinator
+        end
+
+        it "does not create a new record" do
+          expect { import_service.create_users("user_directory") }
+            .not_to change(User, :count)
+        end
+      end
+
+      context "when the duplicate role is lower than the original role" do
+        let!(:user) { FactoryBot.create(:user, :data_coordinator, organisation: org, email: "john.doe@gov.uk") }
+        let(:old_user_id) { "fc7625a02b24ae16162aa63ae7cb33feeec0c373" }
+
+        it "does not change their role" do
+          expect { import_service.create_users("user_directory") }
+            .not_to(change { user.reload.role })
+        end
+
+        it "does not create a new record" do
+          expect { import_service.create_users("user_directory") }
+            .not_to change(User, :count)
+        end
+      end
+
+      context "when the duplicate record is a data protection officer role" do
+        let!(:user) { FactoryBot.create(:user, :data_coordinator, organisation: org, email: "john.doe@gov.uk") }
+        let(:old_user_id) { "10c887710550844e2551b3e0fb88dc9b4a8a642b" }
+
+        it "marks them as a data protection officer" do
+          import_service.create_users("user_directory")
+          expect(user.reload.is_data_protection_officer?).to be true
+        end
+
+        it "does not create a new record" do
+          expect { import_service.create_users("user_directory") }
+            .not_to change(User, :count)
+        end
+      end
+    end
   end
 end