From 9cc666e0f81a1b640bbb3a2b8a6cc1840494b4b4 Mon Sep 17 00:00:00 2001
From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com>
Date: Fri, 11 Mar 2022 10:58:20 +0000
Subject: [PATCH] Sentry fixes (#373)
* Show validation error if user email already exists instead of crashing
* Use translation strings
* When answer option is mapped just display directly
* Validate provider type presence
---
app/admin/organisations.rb | 6 +--
app/controllers/auth/passwords_controller.rb | 4 +-
app/controllers/users_controller.rb | 15 +++++---
app/models/form/question.rb | 18 +++++----
app/models/organisation.rb | 2 +
config/locales/en.yml | 4 ++
db/seeds.rb | 1 +
.../admin/organisations_controller_spec.rb | 2 +-
spec/factories/organisation.rb | 2 +-
spec/fixtures/exports/case_logs.xml | 2 +-
spec/models/form/question_spec.rb | 12 ++++++
spec/models/organisation_spec.rb | 5 +++
spec/requests/users_controller_spec.rb | 38 +++++++++++++++++++
13 files changed, 88 insertions(+), 23 deletions(-)
diff --git a/app/admin/organisations.rb b/app/admin/organisations.rb
index 129aa8ff9..a9e7fb17d 100644
--- a/app/admin/organisations.rb
+++ b/app/admin/organisations.rb
@@ -2,7 +2,7 @@ ActiveAdmin.register Organisation do
permit_params do
permitted = %i[name
phone
- providertype
+ provider_type
address_line1
address_line2
postcode
@@ -28,8 +28,4 @@ ActiveAdmin.register Organisation do
column :managing_agents
actions
end
-
- before_save do |org|
- org.provider_type = params[:organisation][:provider_type]
- end
end
diff --git a/app/controllers/auth/passwords_controller.rb b/app/controllers/auth/passwords_controller.rb
index acbe63e07..c1222764b 100644
--- a/app/controllers/auth/passwords_controller.rb
+++ b/app/controllers/auth/passwords_controller.rb
@@ -5,10 +5,10 @@ class Auth::PasswordsController < Devise::PasswordsController
self.resource = resource_class.new
@email = params["email"]
if @email.empty?
- resource.errors.add :email, "Enter an email address"
+ resource.errors.add :email, I18n.t("validations.email.blank")
render "devise/passwords/new", status: :unprocessable_entity
elsif !email_valid?(@email)
- resource.errors.add :email, "Enter an email address in the correct format, like name@example.com"
+ resource.errors.add :email, I18n.t("validations.email.invalid")
render "devise/passwords/new", status: :unprocessable_entity
else
render "devise/confirmations/reset"
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 63863c17f..7727e031b 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -27,16 +27,21 @@ class UsersController < ApplicationController
def create
@resource = User.new
if user_params["email"].empty?
- @resource.errors.add :email, "Enter an email address"
+ @resource.errors.add :email, I18n.t("validations.email.blank")
elsif !email_valid?(user_params["email"])
- @resource.errors.add :email, "Enter an email address in the correct format, like name@example.com"
+ @resource.errors.add :email, I18n.t("validations.email.invalid")
end
if @resource.errors.present?
render :new, status: :unprocessable_entity
else
- @user = User.create!(user_params.merge(org_params).merge(password_params))
- @user.send_reset_password_instructions
- redirect_to users_organisation_path(current_user.organisation)
+ user = User.create(user_params.merge(org_params).merge(password_params))
+ if user.persisted?
+ user.send_reset_password_instructions
+ redirect_to users_organisation_path(current_user.organisation)
+ else
+ @resource.errors.add :email, I18n.t("validations.email.taken")
+ render :new, status: :unprocessable_entity
+ end
end
end
diff --git a/app/models/form/question.rb b/app/models/form/question.rb
index 409200f49..cc53e949b 100644
--- a/app/models/form/question.rb
+++ b/app/models/form/question.rb
@@ -104,14 +104,16 @@ class Form::Question
def label_from_value(value)
return unless value
- case type
- when "radio"
- answer_options[value.to_s]["value"]
- when "select"
- answer_options[value.to_s]
- else
- value.to_s
- end
+ label = case type
+ when "radio"
+ labels = answer_options[value.to_s]
+ labels["value"] if labels
+ when "select"
+ answer_options[value.to_s]
+ else
+ value.to_s
+ end
+ label || value.to_s
end
def value_is_yes?(value)
diff --git a/app/models/organisation.rb b/app/models/organisation.rb
index 9cefe2a6c..b13a9d075 100644
--- a/app/models/organisation.rb
+++ b/app/models/organisation.rb
@@ -12,6 +12,8 @@ class Organisation < ApplicationRecord
enum provider_type: PROVIDER_TYPE
+ validates :provider_type, presence: true
+
def case_logs
CaseLog.for_organisation(self)
end
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 0acb9b5b5..2adf00a4e 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -43,6 +43,10 @@ en:
invalid_date: "Please enter a valid date"
outside_collection_window: "Date must be within the current collection windows"
postcode: "Enter a postcode in the correct format, for example AA1 1AA"
+ email:
+ taken: "Email already exists"
+ invalid: "Enter an email address in the correct format, like name@example.com"
+ blank: "Enter an email address"
property:
mrcdate:
diff --git a/db/seeds.rb b/db/seeds.rb
index 14b5ea456..0e63f2e5a 100644
--- a/db/seeds.rb
+++ b/db/seeds.rb
@@ -15,6 +15,7 @@ org = Organisation.create!(
holds_own_stock: false,
other_stock_owners: "None",
managing_agents: "None",
+ provider_type: "LA",
)
User.create!(
email: "test@example.com",
diff --git a/spec/controllers/admin/organisations_controller_spec.rb b/spec/controllers/admin/organisations_controller_spec.rb
index 9ed4dea9d..f639a3b7e 100644
--- a/spec/controllers/admin/organisations_controller_spec.rb
+++ b/spec/controllers/admin/organisations_controller_spec.rb
@@ -26,7 +26,7 @@ describe Admin::OrganisationsController, type: :controller do
end
describe "Create organisation" do
- let(:params) { { organisation: { name: "DLUHC" } } }
+ let(:params) { { organisation: { name: "DLUHC", provider_type: "LA" } } }
it "creates a organisation" do
expect { post :create, session: valid_session, params: params }.to change(Organisation, :count).by(1)
diff --git a/spec/factories/organisation.rb b/spec/factories/organisation.rb
index a81c268eb..593bba4ae 100644
--- a/spec/factories/organisation.rb
+++ b/spec/factories/organisation.rb
@@ -3,7 +3,7 @@ FactoryBot.define do
name { "DLUHC" }
address_line1 { "2 Marsham Street" }
address_line2 { "London" }
- provider_type { 2 }
+ provider_type { "LA" }
postcode { "SW1P 4DF" }
created_at { Time.zone.now }
updated_at { Time.zone.now }
diff --git a/spec/fixtures/exports/case_logs.xml b/spec/fixtures/exports/case_logs.xml
index d89fd07b1..14e68a723 100644
--- a/spec/fixtures/exports/case_logs.xml
+++ b/spec/fixtures/exports/case_logs.xml
@@ -121,7 +121,7 @@
{managing_org_id}
2
1
- 5
+ 7
1
1
false
diff --git a/spec/models/form/question_spec.rb b/spec/models/form/question_spec.rb
index 83fe9cc88..dd23d3ec6 100644
--- a/spec/models/form/question_spec.rb
+++ b/spec/models/form/question_spec.rb
@@ -115,6 +115,12 @@ RSpec.describe Form::Question, type: :model do
expect(question).to be_value_is_dont_know(7)
end
end
+
+ context "when the saved answer is not in the value map" do
+ it "displays the saved answer umapped" do
+ expect(question.label_from_value(9999)).to eq("9999")
+ end
+ end
end
context "when type is select" do
@@ -130,6 +136,12 @@ RSpec.describe Form::Question, type: :model do
it "can map label from value" do
expect(question.label_from_value("E06000014")).to eq("York")
end
+
+ context "when the saved answer is not in the value map" do
+ it "displays the saved answer umapped" do
+ expect(question.label_from_value(9999)).to eq("9999")
+ end
+ end
end
context "when type is checkbox" do
diff --git a/spec/models/organisation_spec.rb b/spec/models/organisation_spec.rb
index 9aa6c75cd..89cdb61bd 100644
--- a/spec/models/organisation_spec.rb
+++ b/spec/models/organisation_spec.rb
@@ -13,6 +13,11 @@ RSpec.describe Organisation, type: :model do
expect(organisation.users.first).to eq(user)
end
+ it "validates provider_type presence" do
+ expect { FactoryBot.create(:organisation, provider_type: nil) }
+ .to raise_error(ActiveRecord::RecordInvalid, "Validation failed: Provider type can't be blank")
+ end
+
context "with case logs" do
let(:other_organisation) { FactoryBot.create(:organisation) }
let!(:owned_case_log) do
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index f136250cb..674607179 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -254,6 +254,44 @@ RSpec.describe UsersController, type: :request do
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 "invites a new user" do
+ expect { request }.to change(User, :count).by(1)
+ end
+
+ it "redirects back to organisation users page" do
+ request
+ expect(response).to redirect_to("/organisations/#{user.organisation.id}/users")
+ end
+
+ context "when the email is already taken" do
+ before do
+ FactoryBot.create(:user, email: "new_user@example.com")
+ end
+
+ it "shows an error" do
+ request
+ expect(response).to have_http_status(:unprocessable_entity)
+ expect(page).to have_content(I18n.t("validations.email.taken"))
+ end
+ end
+ end
+
describe "title link" do
before do
sign_in user