Browse Source

CLDC-1171 update design for inviting and editing users (#787)

pull/808/head v0.2.3
J G 2 years ago committed by GitHub
parent
commit
6528999530
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 41
      app/controllers/users_controller.rb
  2. 5
      app/models/form/setup/questions/created_by_id.rb
  3. 16
      app/models/user.rb
  4. 26
      app/views/users/dpo.html.erb
  5. 13
      app/views/users/edit.html.erb
  6. 27
      app/views/users/key_contact.html.erb
  7. 31
      app/views/users/new.html.erb
  8. 4
      app/views/users/show.html.erb
  9. 2
      config/initializers/filter_parameter_logging.rb
  10. 15
      config/locales/en.yml
  11. 3
      config/routes.rb
  12. 6
      db/seeds.rb
  13. 40
      spec/features/user_spec.rb
  14. 8
      spec/models/form/setup/questions/created_by_id_spec.rb
  15. 4
      spec/models/user_spec.rb
  16. 91
      spec/requests/users_controller_spec.rb

41
app/controllers/users_controller.rb

@ -30,6 +30,10 @@ class UsersController < ApplicationController
def show; end
def dpo; end
def key_contact; end
def edit
redirect_to user_path(@user) unless @user.active?
end
@ -68,24 +72,17 @@ class UsersController < ApplicationController
end
def create
@resource = User.new
if user_params["email"].empty?
@resource.errors.add :email, I18n.t("validations.email.blank")
elsif !email_valid?(user_params["email"])
@resource.errors.add :email, I18n.t("validations.email.invalid")
elsif user_params[:role] && !current_user.assignable_roles.key?(user_params[:role].to_sym)
@resource.errors.add :role, I18n.t("validations.role.invalid")
end
if @resource.errors.present?
render :new, status: :unprocessable_entity
@resource = User.new(user_params.merge(org_params).merge(password_params))
validate_attributes
if @resource.errors.empty? && @resource.save
redirect_to created_user_redirect_path
else
user = User.create(user_params.merge(org_params).merge(password_params))
if user.persisted?
redirect_to created_user_redirect_path
else
@resource.errors.add :email, I18n.t("validations.email.taken")
render :new, status: :unprocessable_entity
unless @resource.errors[:organisation].empty?
@resource.errors.add(:organisation_id, message: @resource.errors[:organisation])
@resource.errors.delete(:organisation)
end
render :new, status: :unprocessable_entity
end
end
@ -112,6 +109,16 @@ class UsersController < ApplicationController
private
def validate_attributes
@resource.validate
if user_params[:role].present? && !current_user.assignable_roles.key?(user_params[:role].to_sym)
@resource.errors.add :role, I18n.t("validations.role.invalid")
end
if user_params[:role].empty?
@resource.errors.add :role, I18n.t("activerecord.errors.models.user.attributes.role.blank")
end
end
def format_error_messages
errors = @user.errors.to_hash
@user.errors.clear
@ -161,7 +168,7 @@ private
end
def find_resource
@user = params[:id] ? User.find_by(id: params[:id]) : current_user
@user = User.find_by(id: params[:user_id]) || User.find_by(id: params[:id]) || current_user
end
def authenticate_scope!

5
app/models/form/setup/questions/created_by_id.rb

@ -13,9 +13,8 @@ class Form::Setup::Questions::CreatedById < ::Form::Question
answer_opts = { "" => "Select an option" }
return answer_opts unless ActiveRecord::Base.connected?
User.select(:id, :name, :email).each_with_object(answer_opts) do |user, hsh|
hsh[user.id] = user.name if user.name.present?
hsh[user.id] = user.email if user.name.blank?
User.select(:id, :name).each_with_object(answer_opts) do |user, hsh|
hsh[user.id] = user.name
hsh
end
end

16
app/models/user.rb

@ -1,6 +1,7 @@
class User < ApplicationRecord
# Include default devise modules. Others available are:
# :omniauthable
include Helpers::Email
devise :database_authenticatable, :recoverable, :rememberable, :validatable,
:trackable, :lockable, :two_factor_authenticatable, :confirmable, :timeoutable
@ -8,6 +9,9 @@ class User < ApplicationRecord
has_many :owned_case_logs, through: :organisation
has_many :managed_case_logs, through: :organisation
validate :validate_email
validates :name, :email, presence: true
has_paper_trail ignore: %w[last_sign_in_at
current_sign_in_at
current_sign_in_ip
@ -149,4 +153,16 @@ class User < ApplicationRecord
def valid_for_authentication?
super && active?
end
private
def validate_email
unless email_valid?(email)
if User.exists?(["email LIKE ?", "%#{email}%"])
errors.add :email, I18n.t("activerecord.errors.models.user.attributes.email.taken")
else
errors.add :email, I18n.t("activerecord.errors.models.user.attributes.email.invalid")
end
end
end
end

26
app/views/users/dpo.html.erb

@ -0,0 +1,26 @@
<% content_for :title, "Is this user your organisation’s data protection officer?" %>
<% content_for :before_content do %>
<%= govuk_back_link(href: :back) %>
<% end %>
<%= render partial: "organisations/headings", locals: { main: "Is this user your organisation’s data protection officer?", sub: @user.name } %>
<%= form_for(@user, as: :user, html: { method: :patch }) do |f| %>
<div class="govuk-grid-row">
<div class="govuk-grid-column-two-thirds">
<%= f.govuk_error_summary %>
<% if current_user.data_coordinator? || current_user.support? %>
<%= f.govuk_collection_radio_buttons :is_dpo,
[OpenStruct.new(id: true, name: "Yes"), OpenStruct.new(id: false, name: "No")],
:id,
:name,
legend: nil %>
<% end %>
<%= f.govuk_submit "Save changes" %>
</div>
</div>
<% end %>

13
app/views/users/edit.html.erb

@ -30,19 +30,6 @@
:id,
:name,
legend: { text: "Role", size: "m" } %>
<%= f.govuk_collection_radio_buttons :is_dpo,
[OpenStruct.new(id: true, name: "Yes"), OpenStruct.new(id: false, name: "No")],
:id,
:name,
legend: { text: "#{perspective(@user, current_user)} the organisation’s data protection officer?", size: "m" } %>
<%= f.govuk_collection_radio_buttons :is_key_contact,
[OpenStruct.new(id: true, name: "Yes"), OpenStruct.new(id: false, name: "No")],
:id,
:name,
legend: { text: "#{perspective(@user, current_user)} a key contact for this service?", size: "m" },
hint: { text: "This is a person responsible for sharing information about social housing lettings and sales data within the organisation." } %>
<% end %>
<%= f.govuk_submit "Save changes" %>

27
app/views/users/key_contact.html.erb

@ -0,0 +1,27 @@
<% content_for :title, "Is this user a key contact for this service?" %>
<% content_for :before_content do %>
<%= govuk_back_link(href: :back) %>
<% end %>
<%= render partial: "organisations/headings", locals: { main: "Is this user a key contact for this service?", sub: @user.name } %>
<%= form_for(@user, as: :user, html: { method: :patch }) do |f| %>
<div class="govuk-grid-row">
<div class="govuk-grid-column-two-thirds">
<%= f.govuk_error_summary %>
<% if current_user.data_coordinator? || current_user.support? %>
<%= f.govuk_collection_radio_buttons :is_key_contact,
[OpenStruct.new(id: true, name: "Yes"), OpenStruct.new(id: false, name: "No")],
:id,
:name,
legend: nil,
hint: { text: "This is a person responsible for sharing information about social housing lettings and sales data within the organisation." } %>
<% end %>
<%= f.govuk_submit "Save changes" %>
</div>
</div>
<% end %>

31
app/views/users/new.html.erb

@ -15,7 +15,7 @@
<%= f.govuk_text_field :name,
autocomplete: "name",
label: { text: "Name (optional)", size: "m" } %>
label: { text: "Name", size: "m" } %>
<%= f.govuk_email_field :email,
label: { text: "Email address", size: "m" },
@ -27,6 +27,7 @@
<% null_option = [OpenStruct.new(id: "", name: "Select an option")] %>
<% organisations = Organisation.all.map { |org| OpenStruct.new(id: org.id, name: org.name) } %>
<% answer_options = null_option + organisations %>
<% if @organisation_id %>
<% organisation = Organisation.find(@organisation_id) %>
<% answer_options = [OpenStruct.new(id: organisation.id, name: organisation.name)] %>
@ -40,26 +41,18 @@
options: { disabled: [""], selected: @organisation_id ? answer_options.first : "" } %>
<% end %>
<% roles = current_user.assignable_roles.map { |key, _| OpenStruct.new(id: key, name: key.to_s.humanize) } %>
<%= f.govuk_collection_radio_buttons :role,
roles,
:id,
:name,
legend: { text: "Role", size: "m" } %>
<% hints_for_roles = { data_provider: ["Default role for this organisation", "Can view and submit logs for this organisation"], data_coordinator: ["Can view and submit logs for this organisation and any of its managing agents", "Can manage details for this organisation", "Can manage users for this organisation"], support: nil } %>
<%= f.govuk_collection_radio_buttons :is_dpo,
[OpenStruct.new(id: true, name: "Yes"), OpenStruct.new(id: false, name: "No")],
:id,
:name,
legend: { text: "#{perspective(@user, current_user)} the organisation’s data protection officer?", size: "m" } %>
<% roles_with_hints = current_user.assignable_roles.map { |key, _| OpenStruct.new(id: key, name: key.to_s.humanize, description: hints_for_roles[key.to_sym]) } %>
<%= f.govuk_collection_radio_buttons :is_key_contact,
[OpenStruct.new(id: true, name: "Yes"), OpenStruct.new(id: false, name: "No")],
:id,
:name,
legend: { text: "#{perspective(@user, current_user)} a key contact for this service?", size: "m" },
hint: { text: "This is a person responsible for sharing information about social housing lettings and sales data within the organisation." } %>
<%= f.govuk_collection_radio_buttons :role,
roles_with_hints,
:id,
:name,
lambda { |option|
option.description&.map { |hint| content_tag(:li, hint) }&.reduce(:+)
},
legend: { text: "Role", size: "m" } %>
<%= f.govuk_submit "Continue" %>
</div>

4
app/views/users/show.html.erb

@ -81,7 +81,7 @@
if can_edit_dpo?(@user, current_user)
row.action(
visually_hidden_text: "if data protection officer",
href: aliased_user_edit(@user, current_user),
href: user_edit_dpo_path(@user),
html_attributes: { "data-qa": "change-data-protection-officer" },
)
else
@ -95,7 +95,7 @@
if can_edit_key_contact?(@user, current_user)
row.action(
visually_hidden_text: "if a key contact",
href: aliased_user_edit(@user, current_user),
href: user_edit_key_contact_path(@user),
html_attributes: { "data-qa": "change-key-contact" },
)
else

2
config/initializers/filter_parameter_logging.rb

@ -2,5 +2,5 @@
# Configure sensitive parameters which will be filtered from the log file.
Rails.application.config.filter_parameters += %i[
passw secret token _key crypt salt certificate otp ssn
passw secret token crypt salt certificate otp ssn
]

15
config/locales/en.yml

@ -69,13 +69,26 @@ en:
attributes:
startdate:
invalid: "Enter a date in the correct format, for example 31 1 2022"
units:
blank: "Enter the total number of units at this location"
type_of_unit:
blank: "Select the most common type of unit at this location"
mobility_type:
blank: "Select the mobility standards for the majority of units in this location"
user:
attributes:
organisation_id:
blank: "Enter the existing organisation’s name"
invalid: "Enter the existing organisation’s name"
name:
blank: "Enter a name"
email:
invalid: "Enter an email address in the correct format, like name@example.com"
blank: "Enter an email address"
taken: "Enter an email address that hasn’t already been used to sign up"
role:
invalid: "Role must be data accessor, data provider or data coordinator"
blank: "Select role"
validations:
organisation:

3
config/routes.rb

@ -53,6 +53,9 @@ Rails.application.routes.draw do
end
resources :users do
get "edit-dpo", to: "users#dpo"
get "edit-key-contact", to: "users#key_contact"
member do
get "deactivate", to: "users#deactivate"
get "reactivate", to: "users#reactivate"

6
db/seeds.rb

@ -83,6 +83,8 @@ unless Rails.env.test?
primary_client_group: "O",
secondary_client_group: "H",
owning_organisation: org,
managing_organisation: org,
arrangement_type: "D",
confirmed: true,
created_at: Time.zone.now,
)
@ -97,6 +99,8 @@ unless Rails.env.test?
primary_client_group: "D",
secondary_client_group: "E",
owning_organisation: org,
managing_organisation: org,
arrangement_type: "D",
confirmed: true,
created_at: Time.zone.now,
)
@ -111,6 +115,8 @@ unless Rails.env.test?
primary_client_group: "G",
secondary_client_group: "R",
owning_organisation: dummy_org,
managing_organisation: dummy_org,
arrangement_type: "D",
confirmed: true,
created_at: Time.zone.now,
)

40
spec/features/user_spec.rb

@ -327,27 +327,20 @@ RSpec.describe "User Features" do
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(User.find_by(
name: "New User",
email: "newuser@example.com",
role: "data_provider",
is_dpo: true,
is_key_contact: true,
is_dpo: false,
is_key_contact: false,
)).to be_a(User)
end
it "defaults to is_dpo false" do
visit("users/new")
expect(page).to have_field("user[is_dpo]", with: false)
end
end
context "when editing someone elses account details" do
let!(:user) { FactoryBot.create(:user, :data_coordinator, last_sign_in_at: Time.zone.now) }
let!(:other_user) { FactoryBot.create(:user, name: "Other name", is_dpo: true, organisation: user.organisation) }
let!(:other_user) { FactoryBot.create(:user, name: "Other name", is_dpo: false, organisation: user.organisation) }
before do
visit("/logs")
@ -362,19 +355,36 @@ RSpec.describe "User Features" do
click_link(other_user.name)
expect(page).to have_title("Other name’s account")
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,
is_key_contact: true,
)).to be_a(User)
end
context "when updating other user DPO and key contact information" do
it "allows updating users dpo details" do
visit("/organisations/#{user.organisation.id}")
click_link("Users")
click_link(other_user.name)
find("a[href='#{user_edit_dpo_path(other_user)}']").click
choose("Yes")
click_button("Save changes")
expect(User.find_by(name: "Other name", role: "data_provider", is_dpo: true)).to be_a(User)
end
it "allows updating users key contact details" do
visit("/organisations/#{user.organisation.id}")
click_link("Users")
click_link(other_user.name)
find("a[href='#{user_edit_key_contact_path(other_user)}']").click
choose("Yes")
click_button("Save changes")
expect(User.find_by(name: "Other name", role: "data_provider", is_key_contact: true)).to be_a(User)
end
end
end
context "when deactivating a user" do

8
spec/models/form/setup/questions/created_by_id_spec.rb

@ -8,15 +8,13 @@ RSpec.describe Form::Setup::Questions::CreatedById, type: :model do
let(:page) { instance_double(Form::Page) }
let(:subsection) { instance_double(Form::Subsection) }
let(:form) { instance_double(Form) }
let!(:user_1) { FactoryBot.create(:user, name: "first user") }
let!(:user_2) { FactoryBot.create(:user, name: "second user") }
let!(:user_3) { FactoryBot.create(:user, name: nil, email: "madeupmail@example.com") }
let(:expected_answer_options) do
let(:user_1) { FactoryBot.create(:user, name: "first user") }
let(:user_2) { FactoryBot.create(:user, name: "second user") }
let!(:expected_answer_options) do
{
"" => "Select an option",
user_1.id => user_1.name,
user_2.id => user_2.name,
user_3.id => user_3.email,
}
end

4
spec/models/user_spec.rb

@ -195,8 +195,8 @@ RSpec.describe User, type: :model do
let!(:user_1) { FactoryBot.create(:user, name: "Joe Bloggs", email: "joe@example.com", organisation: organisation_1, role: "support") }
let!(:user_3) { FactoryBot.create(:user, name: "Tom Smith", email: "tom@example.com", organisation: organisation_1, role: "data_provider") }
let!(:user_2) { FactoryBot.create(:user, name: "Jenny Ford", email: "jenny@smith.com", organisation: organisation_1, role: "data_coordinator") }
let!(:user_4) { FactoryBot.create(:user, name: "Greg Thomas", email: "greg@org_2.com", organisation: organisation_2, role: "data_coordinator") }
let!(:user_5) { FactoryBot.create(:user, name: "Adam Thomas", email: "adam@org_2.com", organisation: organisation_2, role: "data_coordinator") }
let!(:user_4) { FactoryBot.create(:user, name: "Greg Thomas", email: "greg@org2.com", organisation: organisation_2, role: "data_coordinator") }
let!(:user_5) { FactoryBot.create(:user, name: "Adam Thomas", email: "adam@org2.com", organisation: organisation_2, role: "data_coordinator") }
context "when searching by name" do
it "returns case insensitive matching records" do

91
spec/requests/users_controller_spec.rb

@ -423,7 +423,7 @@ RSpec.describe UsersController, type: :request do
context "when a search parameter is passed" do
let!(:other_user_2) { FactoryBot.create(:user, organisation: user.organisation, name: "joe", email: "other@example.com") }
let!(:other_user_3) { FactoryBot.create(:user, name: "User 5", organisation: user.organisation, email: "joe@example.com") }
let!(:other_org_user) { FactoryBot.create(:user, name: "User 4", email: "joe@other_example.com") }
let!(:other_org_user) { FactoryBot.create(:user, name: "User 4", email: "joe@otherexample.com") }
before do
get "/organisations/#{user.organisation.id}/users?search=#{search_param}"
@ -604,8 +604,6 @@ RSpec.describe UsersController, type: :request do
expect(page).to have_field("user[name]")
expect(page).to have_field("user[email]")
expect(page).to have_field("user[role]")
expect(page).to have_field("user[is_dpo]")
expect(page).to have_field("user[is_key_contact]")
end
it "does not allow setting the role to `support`" do
@ -632,8 +630,6 @@ RSpec.describe UsersController, type: :request do
expect(page).to have_field("user[name]")
expect(page).to have_field("user[email]")
expect(page).to have_field("user[role]")
expect(page).to have_field("user[is_dpo]")
expect(page).to have_field("user[is_key_contact]")
end
end
@ -792,19 +788,6 @@ RSpec.describe UsersController, type: :request do
expect { patch "/users/#{other_user.id}", headers:, params: }
.to change { other_user.reload.active }.from(true).to(false)
end
context "when the user name is missing" do
let(:other_user) { FactoryBot.create(:user, name: nil, organisation: user.organisation) }
before do
patch "/users/#{other_user.id}", headers:, params:
end
it "uses the user's email" do
follow_redirect!
expect(page).to have_content(other_user.email)
end
end
end
context "and tries to activate deactivated user" do
@ -818,19 +801,6 @@ RSpec.describe UsersController, type: :request do
expect { patch "/users/#{other_user.id}", headers:, params: }
.to change { other_user.reload.active }.from(false).to(true)
end
context "when the user name is missing" do
let(:other_user) { FactoryBot.create(:user, name: nil, organisation: user.organisation) }
before do
patch "/users/#{other_user.id}", headers:, params:
end
it "uses the user's email" do
follow_redirect!
expect(page).to have_content(other_user.email)
end
end
end
end
end
@ -935,6 +905,26 @@ RSpec.describe UsersController, type: :request do
expect(page).to have_content(I18n.t("validations.role.invalid"))
end
end
context "when validating the required fields" do
let(:params) do
{
"user": {
name: "",
email: "",
role: "",
},
}
end
it "shows an error" do
request
expect(response).to have_http_status(:unprocessable_entity)
expect(page).to have_content(I18n.t("activerecord.errors.models.user.attributes.name.blank"))
expect(page).to have_content(I18n.t("activerecord.errors.models.user.attributes.email.blank"))
expect(page).to have_content(I18n.t("activerecord.errors.models.user.attributes.role.blank"))
end
end
end
describe "#new" do
@ -1011,7 +1001,7 @@ RSpec.describe UsersController, type: :request do
describe "#index" do
let!(:other_user) { FactoryBot.create(:user, organisation: user.organisation, name: "User 2", email: "other@example.com") }
let!(:inactive_user) { FactoryBot.create(:user, organisation: user.organisation, active: false, name: "User 3", email: "inactive@example.com") }
let!(:other_org_user) { FactoryBot.create(:user, name: "User 4", email: "other_org@other_example.com") }
let!(:other_org_user) { FactoryBot.create(:user, name: "User 4", email: "otherorg@otherexample.com") }
before do
sign_in user
@ -1078,7 +1068,7 @@ RSpec.describe UsersController, type: :request do
end
context "when our search term matches an email" do
let(:search_param) { "other_org@other_example.com" }
let(:search_param) { "otherorg@otherexample.com" }
it "returns only matching result" do
expect(page).not_to have_content(user.name)
@ -1090,7 +1080,7 @@ RSpec.describe UsersController, type: :request do
context "when our search term matches an email and a name" do
let!(:other_user) { FactoryBot.create(:user, organisation: user.organisation, name: "joe", email: "other@example.com") }
let!(:other_org_user) { FactoryBot.create(:user, name: "User 4", email: "joe@other_example.com") }
let!(:other_org_user) { FactoryBot.create(:user, name: "User 4", email: "joe@otherexample.com") }
let(:search_param) { "joe" }
it "returns any results including joe" do
@ -1259,8 +1249,6 @@ RSpec.describe UsersController, type: :request do
expect(page).to have_field("user[name]")
expect(page).to have_field("user[email]")
expect(page).to have_field("user[role]")
expect(page).to have_field("user[is_dpo]")
expect(page).to have_field("user[is_key_contact]")
end
it "allows setting the role to `support`" do
@ -1287,8 +1275,6 @@ RSpec.describe UsersController, type: :request do
expect(page).to have_field("user[name]")
expect(page).to have_field("user[email]")
expect(page).to have_field("user[role]")
expect(page).to have_field("user[is_dpo]")
expect(page).to have_field("user[is_key_contact]")
end
end
@ -1307,8 +1293,6 @@ RSpec.describe UsersController, type: :request do
expect(page).to have_field("user[name]")
expect(page).to have_field("user[email]")
expect(page).to have_field("user[role]")
expect(page).to have_field("user[is_dpo]")
expect(page).to have_field("user[is_key_contact]")
end
end
@ -1587,6 +1571,31 @@ RSpec.describe UsersController, type: :request do
expect(response).to redirect_to("/users")
end
context "when validations fail" do
let(:params) do
{
"user": {
name: "",
email: "",
role: "",
organisation_id: nil,
},
}
end
before do
FactoryBot.create(:user, email: "new_user@example.com")
end
it "shows an error messages for all failed validations" do
request
expect(response).to have_http_status(:unprocessable_entity)
expect(page).to have_content(I18n.t("activerecord.errors.models.user.attributes.name.blank"))
expect(page).to have_content(I18n.t("activerecord.errors.models.user.attributes.email.blank"))
expect(page).to have_content(I18n.t("activerecord.errors.models.user.attributes.organisation_id.blank"))
end
end
context "when the email is already taken" do
before do
FactoryBot.create(:user, email: "new_user@example.com")
@ -1595,7 +1604,7 @@ RSpec.describe UsersController, type: :request do
it "shows an error" do
request
expect(response).to have_http_status(:unprocessable_entity)
expect(page).to have_content(I18n.t("validations.email.taken"))
expect(page).to have_content(I18n.t("activerecord.errors.models.user.attributes.email.taken"))
end
end

Loading…
Cancel
Save