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

@@ -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

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"

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
 

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)

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

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:

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",

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)

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 }

spec/fixtures/exports/case_logs.xml

@@ -121,7 +121,7 @@
     <managing_organisation_id>{managing_org_id}</managing_organisation_id>
     <renttype>2</renttype>
     <needstype>1</needstype>
-    <lettype>5</lettype>
+    <lettype>7</lettype>
     <postcode_known>1</postcode_known>
     <la_known>1</la_known>
     <is_la_inferred>false</is_la_inferred>

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

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

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