From d3176fa814734218ef8a95863ca2b07e6d03e275 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Sun, 3 Apr 2022 17:04:28 -0400 Subject: [PATCH 01/32] add rspec tests for model validations --- spec/factories/user.rb | 1 + spec/models/user_spec.rb | 24 +++++++++++++++++++++--- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/spec/factories/user.rb b/spec/factories/user.rb index 64e94afc8c..24b2dc6323 100644 --- a/spec/factories/user.rb +++ b/spec/factories/user.rb @@ -6,6 +6,7 @@ password { "12345678" } password_confirmation { "12345678" } case_assignments { [] } + phone_number { "4163218092" } trait :inactive do volunteer diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index eb855dcf92..3261ecf9e6 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -15,11 +15,29 @@ it { is_expected.to have_many(:notes) } - it "requires display name" do - user = build(:user, display_name: "") - expect(user.valid?).to be false + describe "model validations" do + it "requires display name" do + user = build(:user, display_name: "") + expect(user.valid?).to be false + end + + it "requires email" do + user = build(:user, email: "") + expect(user.valid?).to be false + end + + it "requires 10 digit phone numbers" do + user = build(:user, phone_number: "416321809") + expect(user.valid?).to be false + end + + it "requires phone number to only contain numbers" do + user = build(:user, phone_number: "416eee4325") + expect(user.valid?).to be false + end end + describe "#case_contacts_for" do let(:volunteer) { create(:volunteer, :with_casa_cases) } let(:case_of_interest) { volunteer.casa_cases.first } From e22169ff8d36fd5eea11710a6b0dd9ae4962c0c7 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Sun, 3 Apr 2022 17:23:48 -0400 Subject: [PATCH 02/32] modify rspec model validation test to check for 12 digit phone numbers including area code --- spec/models/user_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 3261ecf9e6..822def033b 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -26,13 +26,13 @@ expect(user.valid?).to be false end - it "requires 10 digit phone numbers" do - user = build(:user, phone_number: "416321809") + it "requires 12 digit phone numbers" do + user = build(:user, phone_number: "+1416321809") expect(user.valid?).to be false end it "requires phone number to only contain numbers" do - user = build(:user, phone_number: "416eee4325") + user = build(:user, phone_number: "+1416eee4325") expect(user.valid?).to be false end end From a66f98e4c345ea1739aee005b1f7bad21536bf5a Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Sun, 3 Apr 2022 18:37:58 -0400 Subject: [PATCH 03/32] model phone number field for users table in db --- .allow_skipping_tests | 1 + app/models/casa_admin.rb | 1 + app/models/supervisor.rb | 1 + app/models/user.rb | 2 ++ app/models/volunteer.rb | 1 + app/validators/user_validator.rb | 33 +++++++++++++++++++ ...0220403213338_add_phone_number_to_users.rb | 5 +++ db/schema.rb | 3 +- spec/factories/user.rb | 2 +- spec/models/user_spec.rb | 5 +++ 10 files changed, 52 insertions(+), 2 deletions(-) create mode 100644 app/validators/user_validator.rb create mode 100644 db/migrate/20220403213338_add_phone_number_to_users.rb diff --git a/.allow_skipping_tests b/.allow_skipping_tests index 9a869609e2..00ac8036ca 100644 --- a/.allow_skipping_tests +++ b/.allow_skipping_tests @@ -61,6 +61,7 @@ services/create_all_casa_admin_service.rb services/create_casa_admin_service.rb services/mileage_export_csv_service.rb validators/court_report_validator.rb +validators/user_validator.rb values/all_casa_admin_parameters.rb values/casa_admin_parameters.rb values/supervisor_parameters.rb diff --git a/app/models/casa_admin.rb b/app/models/casa_admin.rb index 11b13237d9..5a127b159f 100644 --- a/app/models/casa_admin.rb +++ b/app/models/casa_admin.rb @@ -36,6 +36,7 @@ def change_to_supervisor! # invited_by_type :string # last_sign_in_at :datetime # last_sign_in_ip :string +# phone_number :string default("") # reset_password_sent_at :datetime # reset_password_token :string # sign_in_count :integer default(0), not null diff --git a/app/models/supervisor.rb b/app/models/supervisor.rb index f004339403..343f5161c5 100644 --- a/app/models/supervisor.rb +++ b/app/models/supervisor.rb @@ -63,6 +63,7 @@ def recently_unassigned_volunteers # invited_by_type :string # last_sign_in_at :datetime # last_sign_in_ip :string +# phone_number :string default("") # reset_password_sent_at :datetime # reset_password_token :string # sign_in_count :integer default(0), not null diff --git a/app/models/user.rb b/app/models/user.rb index e9e855df3a..3ce7662e11 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -10,6 +10,7 @@ class User < ApplicationRecord validates :email, presence: true validates :display_name, presence: true + validates_with UserValidator belongs_to :casa_org @@ -175,6 +176,7 @@ def last_deactivated_by # invited_by_type :string # last_sign_in_at :datetime # last_sign_in_ip :string +# phone_number :string default("") # reset_password_sent_at :datetime # reset_password_token :string # sign_in_count :integer default(0), not null diff --git a/app/models/volunteer.rb b/app/models/volunteer.rb index 3ae6022064..5dd3bc5689 100644 --- a/app/models/volunteer.rb +++ b/app/models/volunteer.rb @@ -143,6 +143,7 @@ def cases_where_contact_made_in_days(num_days = CONTACT_MADE_IN_DAYS_NUM) # invited_by_type :string # last_sign_in_at :datetime # last_sign_in_ip :string +# phone_number :string default("") # reset_password_sent_at :datetime # reset_password_token :string # sign_in_count :integer default(0), not null diff --git a/app/validators/user_validator.rb b/app/validators/user_validator.rb new file mode 100644 index 0000000000..0d0fe316aa --- /dev/null +++ b/app/validators/user_validator.rb @@ -0,0 +1,33 @@ + +class UserValidator < ActiveModel::Validator + VALID_PHONE_NUMBER_LENGTH = 12 + VALID_COUNTRY_CODE = "+1" + PHONE_NUMBER_ERROR = "Phone number is not in correct format: 1XXXXXXXXXX" + + def validate(record) + if !valid_phone_number_contents(record.phone_number) + record.errors.add(:phone_number, PHONE_NUMBER_ERROR) + end + end + + private + + def valid_phone_number_contents(number) + if number.empty? + return true + end + + if number.length != VALID_PHONE_NUMBER_LENGTH + return false + end + + country_code = number[0..1] + phone_number = number[2..number.length] + + if country_code != VALID_COUNTRY_CODE || !phone_number.scan(/\D/).empty? + return false + end + + true + end +end \ No newline at end of file diff --git a/db/migrate/20220403213338_add_phone_number_to_users.rb b/db/migrate/20220403213338_add_phone_number_to_users.rb new file mode 100644 index 0000000000..9d9b7a0949 --- /dev/null +++ b/db/migrate/20220403213338_add_phone_number_to_users.rb @@ -0,0 +1,5 @@ +class AddPhoneNumberToUsers < ActiveRecord::Migration[7.0] + def change + add_column :users, :phone_number, :string, default: "" + end +end diff --git a/db/schema.rb b/db/schema.rb index a124f30657..e18be5e224 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2022_03_23_145733) do +ActiveRecord::Schema[7.0].define(version: 2022_04_03_213338) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -400,6 +400,7 @@ t.datetime "last_sign_in_at", precision: nil t.string "current_sign_in_ip" t.string "last_sign_in_ip" + t.string "phone_number", default: "" t.index ["casa_org_id"], name: "index_users_on_casa_org_id" t.index ["email"], name: "index_users_on_email", unique: true t.index ["invitation_token"], name: "index_users_on_invitation_token", unique: true diff --git a/spec/factories/user.rb b/spec/factories/user.rb index 24b2dc6323..c802546b0f 100644 --- a/spec/factories/user.rb +++ b/spec/factories/user.rb @@ -6,7 +6,7 @@ password { "12345678" } password_confirmation { "12345678" } case_assignments { [] } - phone_number { "4163218092" } + phone_number { "+14163218092" } trait :inactive do volunteer diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 822def033b..392455ee41 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -35,6 +35,11 @@ user = build(:user, phone_number: "+1416eee4325") expect(user.valid?).to be false end + + it "requires phone number with US area code" do + user = build(:user, phone_number: "+76758890432") + expect(user.valid?).to be false + end end From 6867630dbb58730aa7036ce933a273243e2df425 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Tue, 5 Apr 2022 00:30:37 -0400 Subject: [PATCH 04/32] refactor volunteer_controller specs --- .../controllers/volunteers_controller_spec.rb | 46 ++++++++++++++----- 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/spec/controllers/volunteers_controller_spec.rb b/spec/controllers/volunteers_controller_spec.rb index 225cc63ad5..d29053b584 100644 --- a/spec/controllers/volunteers_controller_spec.rb +++ b/spec/controllers/volunteers_controller_spec.rb @@ -2,31 +2,53 @@ RSpec.describe VolunteersController, type: :controller do describe "update" do + + before :each do + @east = create :casa_org, name: "East" + @west = create :casa_org, name: "West" + end + + def log_in_as_supervisor + @supervisor = create :supervisor, casa_org: @east + allow(controller).to receive(:current_user).and_return(@supervisor) + end + + def log_in_as_admin + @admin = create :casa_admin + allow(controller).to receive(:current_user).and_return(@admin) + end + context "as a supervisor" do it "only shows volunteers for the same casa org" do - east = create :casa_org, name: "East" - west = create :casa_org, name: "West" - supervisor = create :supervisor, casa_org: east allow(controller).to receive(:authenticate_user!).and_return(:supervisor) - allow(controller).to receive(:current_user).and_return(supervisor) - volunteer_east = create :volunteer, casa_org: east - _volunteer_west = create :volunteer, casa_org: west + log_in_as_supervisor + volunteer_east = create :volunteer, casa_org: @east + _volunteer_west = create :volunteer, casa_org: @west get :edit, params: {id: volunteer_east.id} expect(response).to have_http_status(:ok) - expect(assigns(:supervisors)).to eq([supervisor]) + expect(assigns(:supervisors)).to eq([@supervisor]) expect(assigns(:volunteer)).to eq(volunteer_east) end it "cannot edit volunteer for a different casa org" do - east = create :casa_org, name: "East" - west = create :casa_org, name: "West" - supervisor = create :supervisor, casa_org: east allow(controller).to receive(:authenticate_user!).and_return(:supervisor) - allow(controller).to receive(:current_user).and_return(supervisor) - volunteer_west = create :volunteer, casa_org: west + log_in_as_supervisor + volunteer_west = create :volunteer, casa_org: @west get :edit, params: {id: volunteer_west.id} expect(response).to redirect_to("/") end end + + context "as a admin" do + it "can have permission to edit volunteer info" do + # create volunteer in db + log_in_as_admin + volunteer = create :volunteer + # hit update route with params + patch :update, params: {id: volunteer.id} + # expect 302 response + expect(response).to have_http_status(:found) + end + end end end From f68c099fbfc3cf3277932333079b25f561d38bae Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Wed, 6 Apr 2022 23:10:43 -0400 Subject: [PATCH 05/32] create request rspec test for volunteer update as admin --- app/values/user_parameters.rb | 1 + spec/requests/volunteers_spec.rb | 6 ++++-- spec/views/volunteers/edit.html.erb_spec.rb | 13 +++++++++++++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/app/values/user_parameters.rb b/app/values/user_parameters.rb index 39523adeb3..52de01b7f8 100644 --- a/app/values/user_parameters.rb +++ b/app/values/user_parameters.rb @@ -5,6 +5,7 @@ def initialize(params, key = :user) :email, :casa_org_id, :display_name, + :phone_number, :password, :active, :type diff --git a/spec/requests/volunteers_spec.rb b/spec/requests/volunteers_spec.rb index 6b40065a03..c20d4db87d 100644 --- a/spec/requests/volunteers_spec.rb +++ b/spec/requests/volunteers_spec.rb @@ -154,13 +154,14 @@ context "with valid params" do it "updates the volunteer" do patch volunteer_path(volunteer), params: { - volunteer: {email: "newemail@gmail.com", display_name: "New Name"} + volunteer: {email: "newemail@gmail.com", display_name: "New Name", phone_number: "+14163218092"} } expect(response).to have_http_status(:redirect) volunteer.reload expect(volunteer.display_name).to eq "New Name" expect(volunteer.email).to eq "newemail@gmail.com" + expect(volunteer.phone_number).to eq "+14163218092" end end @@ -171,13 +172,14 @@ volunteer.supervisor = build(:supervisor) patch volunteer_path(volunteer), params: { - volunteer: {email: other_volunteer.email, display_name: "New Name"} + volunteer: {email: other_volunteer.email, display_name: "New Name", phone_number: "+14163218092"} } expect(response).to have_http_status(:success) # Re-renders form volunteer.reload expect(volunteer.display_name).to_not eq "New Name" expect(volunteer.email).to_not eq other_volunteer.email + expect(volunteer.email).to_not eq "+14163218092" end end diff --git a/spec/views/volunteers/edit.html.erb_spec.rb b/spec/views/volunteers/edit.html.erb_spec.rb index dd257ee7d6..916463e35c 100644 --- a/spec/views/volunteers/edit.html.erb_spec.rb +++ b/spec/views/volunteers/edit.html.erb_spec.rb @@ -16,6 +16,19 @@ expect(rendered).to_not have_field("volunteer_email", readonly: true) end + it "allows an administrator to edit a volunteers phone number" do + administrator = build_stubbed :casa_admin + enable_pundit(view, administrator) + allow(view).to receive(:current_user).and_return(administrator) + + assign :volunteer, volunteer + assign :supervisors, [] + + render template: "volunteers/edit" + + expect(rendered).to_not have_field("volunteer_email", readonly: true) + end + it "allows a supervisor to edit a volunteers email address" do supervisor = build_stubbed :supervisor enable_pundit(view, supervisor) From 56cf92505e852d332879cfc7d97cdea303a4e50a Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Wed, 6 Apr 2022 23:26:05 -0400 Subject: [PATCH 06/32] create request rspec test for supervisor update as admin --- spec/requests/supervisors_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/requests/supervisors_spec.rb b/spec/requests/supervisors_spec.rb index b3d49e636c..9e821e9a63 100644 --- a/spec/requests/supervisors_spec.rb +++ b/spec/requests/supervisors_spec.rb @@ -7,7 +7,7 @@ let(:supervisor) { create(:supervisor) } let(:update_supervisor_params) do - {supervisor: {email: "newemail@gmail.com", display_name: "New Name"}} + {supervisor: {email: "newemail@gmail.com", display_name: "New Name", phone_number: "+14163218092"}} end describe "GET /new" do @@ -84,6 +84,7 @@ expect(supervisor.display_name).to eq "New Name" expect(supervisor.email).to eq "newemail@gmail.com" + expect(supervisor.phone_number).to eq "+14163218092" end it "can set the supervisor to be inactive" do From 95dbd57e3675dd32d8417b53cb775f4e45ca54e8 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Wed, 6 Apr 2022 23:28:11 -0400 Subject: [PATCH 07/32] create request rspec test for admin update as admin --- spec/requests/casa_admins_spec.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/spec/requests/casa_admins_spec.rb b/spec/requests/casa_admins_spec.rb index ab9fd7c983..4f5a9db517 100644 --- a/spec/requests/casa_admins_spec.rb +++ b/spec/requests/casa_admins_spec.rb @@ -37,6 +37,7 @@ let(:casa_admin) { create(:casa_admin) } let(:expected_display_name) { "Admin 2" } let(:expected_email) { "admin2@casa.com" } + let(:expected_phone_number) { "+14163218092" } before do sign_in_as_admin @@ -46,13 +47,15 @@ put casa_admin_path(casa_admin), params: { casa_admin: { email: expected_email, - display_name: expected_display_name + display_name: expected_display_name, + phone_number: expected_phone_number } } casa_admin.reload expect(casa_admin.email).to eq expected_email expect(casa_admin.display_name).to eq expected_display_name + expect(casa_admin.phone_number).to eq expected_phone_number expect(response).to redirect_to casa_admins_path expect(response.request.flash[:notice]).to eq "New admin created successfully" end From 6bcbcd22ad9e57acf97349057fe3e88f7c679510 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Thu, 7 Apr 2022 23:03:07 -0400 Subject: [PATCH 08/32] add integration tests for form fields with phone number --- spec/system/casa_admins/edit_spec.rb | 48 ++++++++++++++++++++ spec/system/supervisors/edit_spec.rb | 66 ++++++++++++++++++++++++++++ spec/system/volunteers/edit_spec.rb | 32 ++++++++++++++ 3 files changed, 146 insertions(+) diff --git a/spec/system/casa_admins/edit_spec.rb b/spec/system/casa_admins/edit_spec.rb index ac348ec16c..40781d41d9 100644 --- a/spec/system/casa_admins/edit_spec.rb +++ b/spec/system/casa_admins/edit_spec.rb @@ -26,6 +26,54 @@ end context "with invalid data" do + it "shows error message for phone number < 12 digits" do + visit edit_casa_admin_path(admin) + + fill_in "volunteer_email", with: "newemail@example.com" + fill_in "volunteer_display_name", with: "Lumine" + fill_in "volunteer_phone_number", with: "+141632489" + + click_on "Submit" + + expect(page).to have_text "phone number too short" + end + + it "shows error message for phone number > 12 digits" do + visit edit_casa_admin_path(admin) + + fill_in "volunteer_email", with: "newemail@example.com" + fill_in "volunteer_display_name", with: "Raiden Shogun" + fill_in "volunteer_phone_number", with: "+141632180923" + + click_on "Submit" + + expect(page).to have_text "phone number too long" + end + + it "shows error message for bad phone number" do + visit edit_casa_admin_path(admin) + + fill_in "volunteer_email", with: "newemail@example.com" + fill_in "volunteer_display_name", with: "Nyan Cat" + fill_in "volunteer_phone_number", with: "+141632u809o" + + click_on "Submit" + + expect(page).to have_text "incorrect phone number format" + end + + it "shows error message for phone number without country code" do + visit edit_casa_admin_path(admin) + + fill_in "volunteer_email", with: "newemail@example.com" + fill_in "volunteer_display_name", with: "Patrick Star" + fill_in "volunteer_phone_number", with: "4163218092" + + click_on "Submit" + + expect(page).to have_text "phone number must have a country code" + end + it "shows error message for empty email" do visit edit_casa_admin_path(admin) diff --git a/spec/system/supervisors/edit_spec.rb b/spec/system/supervisors/edit_spec.rb index d2c60802cf..852125bcf0 100644 --- a/spec/system/supervisors/edit_spec.rb +++ b/spec/system/supervisors/edit_spec.rb @@ -36,6 +36,72 @@ expect(page).to have_text("Editing Supervisor") end + context "with invalid data" do + it "shows error message for phone number < 12 digits" do + supervisor_name = "Lesile Knope" + create(:supervisor, display_name: supervisor_name, casa_org: organization) + sign_in user + + visit supervisors_path + + within "#supervisors" do + click_on supervisor_name + fill_in "supervisor_phone_number", with: "+1416321" + end + + click_on "Submit" + expect(page).to have_text("phone number too short") + end + + it "shows error message for phone number > 12 digits" do + supervisor_name = "Lesile Knope" + create(:supervisor, display_name: supervisor_name, casa_org: organization) + sign_in user + + visit supervisors_path + + within "#supervisors" do + click_on supervisor_name + fill_in "supervisor_phone_number", with: "+1416321432443" + end + + click_on "Submit" + expect(page).to have_text("phone number too long") + end + + it "shows error message for bad phone number" do + supervisor_name = "Lesile Knope" + create(:supervisor, display_name: supervisor_name, casa_org: organization) + sign_in user + + visit supervisors_path + + within "#supervisors" do + click_on supervisor_name + fill_in "supervisor_phone_number", with: "+1416321809u" + end + + click_on "Submit" + expect(page).to have_text("incorrect phone number format") + end + + it "shows error message for phone number without country code" do + supervisor_name = "Lesile Knope" + create(:supervisor, display_name: supervisor_name, casa_org: organization) + sign_in user + + visit supervisors_path + + within "#supervisors" do + click_on supervisor_name + fill_in "supervisor_phone_number", with: "4163218092" + end + + click_on "Submit" + expect(page).to have_text("phone number must have a country code") + end + end + it "can go to the supervisor edit page and see red message when there are no active volunteers" do supervisor = create :supervisor, casa_org: organization diff --git a/spec/system/volunteers/edit_spec.rb b/spec/system/volunteers/edit_spec.rb index 193c613441..bf3a9fd289 100644 --- a/spec/system/volunteers/edit_spec.rb +++ b/spec/system/volunteers/edit_spec.rb @@ -31,6 +31,38 @@ end context "with invalid data" do + it "shows error message for phone number < 12 digits" do + fill_in "volunteer_email", with: "newemail@example.com" + fill_in "volunteer_display_name", with: "Driving Thunder" + fill_in "volunteer_phone_number", with: "+141632489" + click_on "Submit" + expect(page).to have_text "phone number too short" + end + + it "shows error message for phone number > 12 digits" do + fill_in "volunteer_email", with: "newemail@example.com" + fill_in "volunteer_display_name", with: "Kamisato Ayato" + fill_in "volunteer_phone_number", with: "+141632180923" + click_on "Submit" + expect(page).to have_text "phone number too long" + end + + it "shows error message for bad phone number" do + fill_in "volunteer_email", with: "newemail@example.com" + fill_in "volunteer_display_name", with: "Rex Lapis" + fill_in "volunteer_phone_number", with: "+141632u809o" + click_on "Submit" + expect(page).to have_text "incorrect phone number format" + end + + it "shows error message for phone number without country code" do + fill_in "volunteer_email", with: "newemail@example.com" + fill_in "volunteer_display_name", with: "Lara Croft" + fill_in "volunteer_phone_number", with: "4163218092" + click_on "Submit" + expect(page).to have_text "phone number must have a country code" + end + it "shows error message for duplicate email" do volunteer.supervisor = build(:supervisor) fill_in "volunteer_email", with: admin.email From 2b22a20e9a8111ddd248ec982808315d4e4b0225 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Fri, 8 Apr 2022 22:22:36 -0400 Subject: [PATCH 09/32] add phone number input to views --- app/views/casa_admins/_form.html.erb | 5 +++++ app/views/supervisors/edit.html.erb | 5 +++++ app/views/users/edit.html.erb | 5 +++++ app/views/volunteers/edit.html.erb | 5 +++++ spec/requests/volunteers_spec.rb | 2 +- 5 files changed, 21 insertions(+), 1 deletion(-) diff --git a/app/views/casa_admins/_form.html.erb b/app/views/casa_admins/_form.html.erb index 7cda79e709..1ae8e70e90 100644 --- a/app/views/casa_admins/_form.html.erb +++ b/app/views/casa_admins/_form.html.erb @@ -15,6 +15,11 @@ <%= form.text_field :display_name, class: "form-control" %> +
+ <%= form.label :phone_number, t("common.phone_number") %> + <%= form.text_field :phone_number, class: "form-control" %> +
+ <%= render "/shared/invite_login", resource: casa_admin %> <% if casa_admin.persisted? %> diff --git a/app/views/supervisors/edit.html.erb b/app/views/supervisors/edit.html.erb index da2e78c3e7..7a82e5a1e1 100644 --- a/app/views/supervisors/edit.html.erb +++ b/app/views/supervisors/edit.html.erb @@ -23,6 +23,11 @@ <% end %> +
+ <%= form.label :phone_number, t("common.phone_number") %> + <%= form.text_field :phone_number, class: "form-control" %> +
+

<%= render "/shared/invite_login", resource: @supervisor %>

diff --git a/app/views/users/edit.html.erb b/app/views/users/edit.html.erb index 30a78a6868..ba62024737 100644 --- a/app/views/users/edit.html.erb +++ b/app/views/users/edit.html.erb @@ -13,6 +13,11 @@ <%= form.text_field :display_name, class: "form-control" %> +
+ <%= form.label :phone_number, t("common.phone_number") %> + <%= form.text_field :phone_number, class: "form-control" %> +
+ <%= form.hidden_field :casa_org_id, value: current_user.casa_org_id %> <%= render "/shared/invite_login", resource: current_user %> diff --git a/app/views/volunteers/edit.html.erb b/app/views/volunteers/edit.html.erb index 7ab397ac01..1041c1be05 100644 --- a/app/views/volunteers/edit.html.erb +++ b/app/views/volunteers/edit.html.erb @@ -29,6 +29,11 @@ <%= form.text_field :display_name, class: "form-control" %> +
+ <%= form.label :phone_number, t("common.phone_number") %> + <%= form.text_field :phone_number, class: "form-control" %> +
+ <%= render "/shared/invite_login", resource: @volunteer %>

diff --git a/spec/requests/volunteers_spec.rb b/spec/requests/volunteers_spec.rb index c20d4db87d..861610dee8 100644 --- a/spec/requests/volunteers_spec.rb +++ b/spec/requests/volunteers_spec.rb @@ -179,7 +179,7 @@ volunteer.reload expect(volunteer.display_name).to_not eq "New Name" expect(volunteer.email).to_not eq other_volunteer.email - expect(volunteer.email).to_not eq "+14163218092" + expect(volunteer.phone_number).to_not eq "+14163218092" end end From c3f987143e12ab574a39538b5c29f23aa498548d Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Fri, 8 Apr 2022 22:38:03 -0400 Subject: [PATCH 10/32] refactor user_validator to spit out clearer errors for phone number --- app/validators/user_validator.rb | 19 +++++++++++-------- spec/system/volunteers/edit_spec.rb | 8 ++++---- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/app/validators/user_validator.rb b/app/validators/user_validator.rb index 0d0fe316aa..f7f76ee21b 100644 --- a/app/validators/user_validator.rb +++ b/app/validators/user_validator.rb @@ -1,33 +1,36 @@ - class UserValidator < ActiveModel::Validator VALID_PHONE_NUMBER_LENGTH = 12 VALID_COUNTRY_CODE = "+1" - PHONE_NUMBER_ERROR = "Phone number is not in correct format: 1XXXXXXXXXX" def validate(record) - if !valid_phone_number_contents(record.phone_number) - record.errors.add(:phone_number, PHONE_NUMBER_ERROR) - end + valid_phone_number_contents(record.phone_number, record) end private - def valid_phone_number_contents(number) + def valid_phone_number_contents(number, record) if number.empty? return true end if number.length != VALID_PHONE_NUMBER_LENGTH + record.errors.add(:phone_number, " must be 12 digits") return false end country_code = number[0..1] phone_number = number[2..number.length] - if country_code != VALID_COUNTRY_CODE || !phone_number.scan(/\D/).empty? + if country_code != VALID_COUNTRY_CODE + record.errors.add(:phone_number, " must have a valid country code") + return false + end + + if !phone_number.scan(/\D/).empty? + record.errors.add(:phone_number, " must have correct format") return false end true end -end \ No newline at end of file +end diff --git a/spec/system/volunteers/edit_spec.rb b/spec/system/volunteers/edit_spec.rb index bf3a9fd289..00636114af 100644 --- a/spec/system/volunteers/edit_spec.rb +++ b/spec/system/volunteers/edit_spec.rb @@ -36,7 +36,7 @@ fill_in "volunteer_display_name", with: "Driving Thunder" fill_in "volunteer_phone_number", with: "+141632489" click_on "Submit" - expect(page).to have_text "phone number too short" + expect(page).to have_text "phone number must be 12 digits" end it "shows error message for phone number > 12 digits" do @@ -44,7 +44,7 @@ fill_in "volunteer_display_name", with: "Kamisato Ayato" fill_in "volunteer_phone_number", with: "+141632180923" click_on "Submit" - expect(page).to have_text "phone number too long" + expect(page).to have_text "phone number must be 12 digits" end it "shows error message for bad phone number" do @@ -52,7 +52,7 @@ fill_in "volunteer_display_name", with: "Rex Lapis" fill_in "volunteer_phone_number", with: "+141632u809o" click_on "Submit" - expect(page).to have_text "incorrect phone number format" + expect(page).to have_text "phone number must have correct format" end it "shows error message for phone number without country code" do @@ -60,7 +60,7 @@ fill_in "volunteer_display_name", with: "Lara Croft" fill_in "volunteer_phone_number", with: "4163218092" click_on "Submit" - expect(page).to have_text "phone number must have a country code" + expect(page).to have_text "phone number must have a valid country code" end it "shows error message for duplicate email" do From 69ad2545bf6265fe852a6de2139fa143bcff9dcb Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Fri, 8 Apr 2022 23:12:05 -0400 Subject: [PATCH 11/32] add locales for en.common.phone_number --- config/locales/en.yml | 1 + spec/factories/user.rb | 2 +- spec/system/volunteers/edit_spec.rb | 10 +++++----- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 4526d0137a..d67ab754ae 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -94,6 +94,7 @@ en: email: Email name: Name display_name: Display name + phone_number: Phone number address: Address date: formats: diff --git a/spec/factories/user.rb b/spec/factories/user.rb index c802546b0f..b72d004358 100644 --- a/spec/factories/user.rb +++ b/spec/factories/user.rb @@ -6,7 +6,7 @@ password { "12345678" } password_confirmation { "12345678" } case_assignments { [] } - phone_number { "+14163218092" } + phone_number { "" } trait :inactive do volunteer diff --git a/spec/system/volunteers/edit_spec.rb b/spec/system/volunteers/edit_spec.rb index 00636114af..45a5d7af59 100644 --- a/spec/system/volunteers/edit_spec.rb +++ b/spec/system/volunteers/edit_spec.rb @@ -36,7 +36,7 @@ fill_in "volunteer_display_name", with: "Driving Thunder" fill_in "volunteer_phone_number", with: "+141632489" click_on "Submit" - expect(page).to have_text "phone number must be 12 digits" + expect(page).to have_text "Phone number must be 12 digits" end it "shows error message for phone number > 12 digits" do @@ -44,7 +44,7 @@ fill_in "volunteer_display_name", with: "Kamisato Ayato" fill_in "volunteer_phone_number", with: "+141632180923" click_on "Submit" - expect(page).to have_text "phone number must be 12 digits" + expect(page).to have_text "Phone number must be 12 digits" end it "shows error message for bad phone number" do @@ -52,15 +52,15 @@ fill_in "volunteer_display_name", with: "Rex Lapis" fill_in "volunteer_phone_number", with: "+141632u809o" click_on "Submit" - expect(page).to have_text "phone number must have correct format" + expect(page).to have_text "Phone number must have correct format" end it "shows error message for phone number without country code" do fill_in "volunteer_email", with: "newemail@example.com" fill_in "volunteer_display_name", with: "Lara Croft" - fill_in "volunteer_phone_number", with: "4163218092" + fill_in "volunteer_phone_number", with: "+24163218092" click_on "Submit" - expect(page).to have_text "phone number must have a valid country code" + expect(page).to have_text "Phone number must have a valid country code" end it "shows error message for duplicate email" do From d193df422a96d98ca7f9f0ab2c63c6f87ded969e Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Fri, 8 Apr 2022 23:50:09 -0400 Subject: [PATCH 12/32] update error message for invalid phone number --- app/validators/user_validator.rb | 4 ++-- spec/system/volunteers/edit_spec.rb | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/validators/user_validator.rb b/app/validators/user_validator.rb index f7f76ee21b..a9f9f0b91c 100644 --- a/app/validators/user_validator.rb +++ b/app/validators/user_validator.rb @@ -14,7 +14,7 @@ def valid_phone_number_contents(number, record) end if number.length != VALID_PHONE_NUMBER_LENGTH - record.errors.add(:phone_number, " must be 12 digits") + record.errors.add(:phone_number, " must be 12 digits including country code (+1)") return false end @@ -22,7 +22,7 @@ def valid_phone_number_contents(number, record) phone_number = number[2..number.length] if country_code != VALID_COUNTRY_CODE - record.errors.add(:phone_number, " must have a valid country code") + record.errors.add(:phone_number, " must have a valid country code (+1)") return false end diff --git a/spec/system/volunteers/edit_spec.rb b/spec/system/volunteers/edit_spec.rb index 45a5d7af59..1d18551ca1 100644 --- a/spec/system/volunteers/edit_spec.rb +++ b/spec/system/volunteers/edit_spec.rb @@ -36,7 +36,7 @@ fill_in "volunteer_display_name", with: "Driving Thunder" fill_in "volunteer_phone_number", with: "+141632489" click_on "Submit" - expect(page).to have_text "Phone number must be 12 digits" + expect(page).to have_text "Phone number must be 12 digits including country code (+1)" end it "shows error message for phone number > 12 digits" do @@ -44,7 +44,7 @@ fill_in "volunteer_display_name", with: "Kamisato Ayato" fill_in "volunteer_phone_number", with: "+141632180923" click_on "Submit" - expect(page).to have_text "Phone number must be 12 digits" + expect(page).to have_text "Phone number must be 12 digits including country code (+1)" end it "shows error message for bad phone number" do @@ -60,7 +60,7 @@ fill_in "volunteer_display_name", with: "Lara Croft" fill_in "volunteer_phone_number", with: "+24163218092" click_on "Submit" - expect(page).to have_text "Phone number must have a valid country code" + expect(page).to have_text "Phone number must have a valid country code (+1)" end it "shows error message for duplicate email" do From e964f0129244f4825a6f4245b88091c051f4294f Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Mon, 11 Apr 2022 17:54:05 -0400 Subject: [PATCH 13/32] fix tests --- spec/requests/casa_admins_spec.rb | 4 +++- spec/system/casa_admins/edit_spec.rb | 35 +++++++++++++++------------- spec/system/supervisors/edit_spec.rb | 20 +++++++++------- 3 files changed, 34 insertions(+), 25 deletions(-) diff --git a/spec/requests/casa_admins_spec.rb b/spec/requests/casa_admins_spec.rb index 4f5a9db517..b92f75cf67 100644 --- a/spec/requests/casa_admins_spec.rb +++ b/spec/requests/casa_admins_spec.rb @@ -83,11 +83,13 @@ it "cannot update the casa admin", :aggregate_failures do put casa_admin_path(casa_admin), params: { - casa_admin: {email: nil} + casa_admin: {email: nil}, + phone_number: {phone_number: "dsadw323"} } casa_admin.reload expect(casa_admin.email).not_to eq nil + expect(casa_admin.phone_number).not_to eq "dsadw323" expect(response).to render_template :edit end diff --git a/spec/system/casa_admins/edit_spec.rb b/spec/system/casa_admins/edit_spec.rb index 40781d41d9..f874d7920d 100644 --- a/spec/system/casa_admins/edit_spec.rb +++ b/spec/system/casa_admins/edit_spec.rb @@ -9,11 +9,13 @@ it "can successfully edit user email and display name" do expected_email = "root@casa.com" expected_display_name = "Root Admin" + expected_phone_number = "+14163218092" visit edit_casa_admin_path(admin) fill_in "Email", with: expected_email fill_in "Display name", with: expected_display_name + fill_in "Phone number", with: expected_phone_number click_on "Submit" @@ -22,6 +24,7 @@ expect(page).to have_text "New admin created successfully" expect(admin.email).to eq expected_email expect(admin.display_name).to eq expected_display_name + expect(admin.phone_number).to eq expected_phone_number end end @@ -29,49 +32,49 @@ it "shows error message for phone number < 12 digits" do visit edit_casa_admin_path(admin) - fill_in "volunteer_email", with: "newemail@example.com" - fill_in "volunteer_display_name", with: "Lumine" - fill_in "volunteer_phone_number", with: "+141632489" + fill_in "Email", with: "newemail@example.com" + fill_in "Display name", with: "Lumine" + fill_in "Phone number", with: "+141632489" click_on "Submit" - expect(page).to have_text "phone number too short" + expect(page).to have_text "Phone number must be 12 digits including country code (+1)" end it "shows error message for phone number > 12 digits" do visit edit_casa_admin_path(admin) - fill_in "volunteer_email", with: "newemail@example.com" - fill_in "volunteer_display_name", with: "Raiden Shogun" - fill_in "volunteer_phone_number", with: "+141632180923" + fill_in "Email", with: "newemail@example.com" + fill_in "Display name", with: "Raiden Shogun" + fill_in "Phone number", with: "+141632180923" click_on "Submit" - expect(page).to have_text "phone number too long" + expect(page).to have_text "Phone number must be 12 digits including country code (+1)" end it "shows error message for bad phone number" do visit edit_casa_admin_path(admin) - fill_in "volunteer_email", with: "newemail@example.com" - fill_in "volunteer_display_name", with: "Nyan Cat" - fill_in "volunteer_phone_number", with: "+141632u809o" + fill_in "Email", with: "newemail@example.com" + fill_in "Display name", with: "Nyan Cat" + fill_in "Phone number", with: "+141632u809o" click_on "Submit" - expect(page).to have_text "incorrect phone number format" + expect(page).to have_text "Phone number must have correct format" end it "shows error message for phone number without country code" do visit edit_casa_admin_path(admin) - fill_in "volunteer_email", with: "newemail@example.com" - fill_in "volunteer_display_name", with: "Patrick Star" - fill_in "volunteer_phone_number", with: "4163218092" + fill_in "Email", with: "newemail@example.com" + fill_in "Display name", with: "Patrick Star" + fill_in "Phone number", with: "4163218092" click_on "Submit" - expect(page).to have_text "phone number must have a country code" + expect(page).to have_text "Phone number must have a valid country code (+1)" end it "shows error message for empty email" do diff --git a/spec/system/supervisors/edit_spec.rb b/spec/system/supervisors/edit_spec.rb index 852125bcf0..3871615448 100644 --- a/spec/system/supervisors/edit_spec.rb +++ b/spec/system/supervisors/edit_spec.rb @@ -46,11 +46,12 @@ within "#supervisors" do click_on supervisor_name - fill_in "supervisor_phone_number", with: "+1416321" end + fill_in "supervisor_phone_number", with: "+1416321" click_on "Submit" - expect(page).to have_text("phone number too short") + + expect(page).to have_text("Phone number must be 12 digits including country code (+1)") end it "shows error message for phone number > 12 digits" do @@ -62,11 +63,12 @@ within "#supervisors" do click_on supervisor_name - fill_in "supervisor_phone_number", with: "+1416321432443" end + fill_in "supervisor_phone_number", with: "+1416321432443" click_on "Submit" - expect(page).to have_text("phone number too long") + + expect(page).to have_text("Phone number must be 12 digits including country code (+1)") end it "shows error message for bad phone number" do @@ -78,11 +80,12 @@ within "#supervisors" do click_on supervisor_name - fill_in "supervisor_phone_number", with: "+1416321809u" end + fill_in "supervisor_phone_number", with: "+1416321809u" click_on "Submit" - expect(page).to have_text("incorrect phone number format") + + expect(page).to have_text("Phone number must have correct format") end it "shows error message for phone number without country code" do @@ -94,11 +97,12 @@ within "#supervisors" do click_on supervisor_name - fill_in "supervisor_phone_number", with: "4163218092" end + fill_in "supervisor_phone_number", with: "+24163218092" click_on "Submit" - expect(page).to have_text("phone number must have a country code") + + expect(page).to have_text("Phone number must have a valid country code (+1)") end end From 68468bbf462b9b91c11002cc782a50e84d9be310 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Mon, 11 Apr 2022 18:10:37 -0400 Subject: [PATCH 14/32] include phone number as a param for casa admin controller --- app/controllers/casa_admins_controller.rb | 2 +- spec/system/casa_admins/edit_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/casa_admins_controller.rb b/app/controllers/casa_admins_controller.rb index c97930f9a7..c6d193ab35 100644 --- a/app/controllers/casa_admins_controller.rb +++ b/app/controllers/casa_admins_controller.rb @@ -123,6 +123,6 @@ def set_admin end def update_casa_admin_params - CasaAdminParameters.new(params).with_only(:email, :display_name) + CasaAdminParameters.new(params).with_only(:email, :display_name, :phone_number) end end diff --git a/spec/system/casa_admins/edit_spec.rb b/spec/system/casa_admins/edit_spec.rb index f874d7920d..ab28e15c99 100644 --- a/spec/system/casa_admins/edit_spec.rb +++ b/spec/system/casa_admins/edit_spec.rb @@ -70,7 +70,7 @@ fill_in "Email", with: "newemail@example.com" fill_in "Display name", with: "Patrick Star" - fill_in "Phone number", with: "4163218092" + fill_in "Phone number", with: "+24163218092" click_on "Submit" From e7110cb26fd890a6024bd7fba358201124470d88 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Mon, 11 Apr 2022 18:35:56 -0400 Subject: [PATCH 15/32] create system spec for users to check for invalid phone number form entries --- app/controllers/users_controller.rb | 4 ++-- spec/system/casa_admins/edit_spec.rb | 2 +- spec/system/users/edit_spec.rb | 24 ++++++++++++++++++++++++ 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index b230aba48e..dab4489196 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -60,9 +60,9 @@ def update_user_password def user_params if current_user.casa_admin? - params.require(:user).permit(:email, :display_name) + params.require(:user).permit(:email, :display_name, :phone_number) else - params.require(:user).permit(:display_name) + params.require(:user).permit(:display_name, :phone_number) end end diff --git a/spec/system/casa_admins/edit_spec.rb b/spec/system/casa_admins/edit_spec.rb index ab28e15c99..44660644a2 100644 --- a/spec/system/casa_admins/edit_spec.rb +++ b/spec/system/casa_admins/edit_spec.rb @@ -45,7 +45,7 @@ visit edit_casa_admin_path(admin) fill_in "Email", with: "newemail@example.com" - fill_in "Display name", with: "Raiden Shogun" + fill_in "Display name", with: "Kadehara Kazuha" fill_in "Phone number", with: "+141632180923" click_on "Submit" diff --git a/spec/system/users/edit_spec.rb b/spec/system/users/edit_spec.rb index 334ef66b1d..7a59dc0819 100644 --- a/spec/system/users/edit_spec.rb +++ b/spec/system/users/edit_spec.rb @@ -124,6 +124,30 @@ expect(page).to have_text("Display name can't be blank") end + it "is not able to update the profile when phone number is too short" do + fill_in "Phone number", with: "+1416321" + click_on "Update Profile" + expect(page).to have_text("Phone number must be 12 digits including country code (+1)") + end + + it "is not able to update the profile when phone number is too long" do + fill_in "Phone number", with: "+1416321809231" + click_on "Update Profile" + expect(page).to have_text("Phone number must be 12 digits including country code (+1)") + end + + it "is not able to update the profile when phone number is not valid" do + fill_in "Phone number", with: "+1416321809o" + click_on "Update Profile" + expect(page).to have_text("Phone number must have correct format") + end + + it "is not able to update the profile when phone number includes an invalid country code" do + fill_in "Phone number", with: "+24163218092" + click_on "Update Profile" + expect(page).to have_text("Phone number must have a valid country code (+1)") + end + it "is able to update the email if user is a admin" do expect(page).to have_field("Email", disabled: false) fill_in "Email", with: "new_admin@example.com" From 9bd38ae77b8aa715dff9b7c68befe64841c16f7b Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Wed, 13 Apr 2022 22:23:47 -0400 Subject: [PATCH 16/32] lint files --- spec/controllers/volunteers_controller_spec.rb | 1 - spec/models/user_spec.rb | 1 - 2 files changed, 2 deletions(-) diff --git a/spec/controllers/volunteers_controller_spec.rb b/spec/controllers/volunteers_controller_spec.rb index d29053b584..a7d1813e47 100644 --- a/spec/controllers/volunteers_controller_spec.rb +++ b/spec/controllers/volunteers_controller_spec.rb @@ -2,7 +2,6 @@ RSpec.describe VolunteersController, type: :controller do describe "update" do - before :each do @east = create :casa_org, name: "East" @west = create :casa_org, name: "West" diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 392455ee41..a501e2bb86 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -42,7 +42,6 @@ end end - describe "#case_contacts_for" do let(:volunteer) { create(:volunteer, :with_casa_cases) } let(:case_of_interest) { volunteer.casa_cases.first } From b372f86bbdcefe5c3e93a64f8cc656cfb6db6227 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Thu, 14 Apr 2022 21:57:48 -0400 Subject: [PATCH 17/32] remove duplicate test and used fake phone numbers for tests --- spec/controllers/volunteers_controller_spec.rb | 12 ------------ spec/requests/volunteers_spec.rb | 8 ++++---- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/spec/controllers/volunteers_controller_spec.rb b/spec/controllers/volunteers_controller_spec.rb index a7d1813e47..f2edc1a7db 100644 --- a/spec/controllers/volunteers_controller_spec.rb +++ b/spec/controllers/volunteers_controller_spec.rb @@ -37,17 +37,5 @@ def log_in_as_admin expect(response).to redirect_to("/") end end - - context "as a admin" do - it "can have permission to edit volunteer info" do - # create volunteer in db - log_in_as_admin - volunteer = create :volunteer - # hit update route with params - patch :update, params: {id: volunteer.id} - # expect 302 response - expect(response).to have_http_status(:found) - end - end end end diff --git a/spec/requests/volunteers_spec.rb b/spec/requests/volunteers_spec.rb index 861610dee8..a8ca19417b 100644 --- a/spec/requests/volunteers_spec.rb +++ b/spec/requests/volunteers_spec.rb @@ -154,14 +154,14 @@ context "with valid params" do it "updates the volunteer" do patch volunteer_path(volunteer), params: { - volunteer: {email: "newemail@gmail.com", display_name: "New Name", phone_number: "+14163218092"} + volunteer: {email: "newemail@gmail.com", display_name: "New Name", phone_number: "+15463457898"} } expect(response).to have_http_status(:redirect) volunteer.reload expect(volunteer.display_name).to eq "New Name" expect(volunteer.email).to eq "newemail@gmail.com" - expect(volunteer.phone_number).to eq "+14163218092" + expect(volunteer.phone_number).to eq "+15463457898" end end @@ -172,14 +172,14 @@ volunteer.supervisor = build(:supervisor) patch volunteer_path(volunteer), params: { - volunteer: {email: other_volunteer.email, display_name: "New Name", phone_number: "+14163218092"} + volunteer: {email: other_volunteer.email, display_name: "New Name", phone_number: "+15463457898"} } expect(response).to have_http_status(:success) # Re-renders form volunteer.reload expect(volunteer.display_name).to_not eq "New Name" expect(volunteer.email).to_not eq other_volunteer.email - expect(volunteer.phone_number).to_not eq "+14163218092" + expect(volunteer.phone_number).to_not eq "+15463457898" end end From 2692b685efa41c36dad5e876df2e7e9648f5e3d7 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Fri, 15 Apr 2022 00:19:30 -0400 Subject: [PATCH 18/32] refactor system specs to use shared_examples --- .../shows_error_for_invalid_phone_numbers.rb | 25 +++++++ spec/system/casa_admins/edit_spec.rb | 49 ++----------- spec/system/supervisors/edit_spec.rb | 70 ++----------------- spec/system/volunteers/edit_spec.rb | 36 ++-------- 4 files changed, 38 insertions(+), 142 deletions(-) create mode 100644 spec/support/shared_examples/shows_error_for_invalid_phone_numbers.rb diff --git a/spec/support/shared_examples/shows_error_for_invalid_phone_numbers.rb b/spec/support/shared_examples/shows_error_for_invalid_phone_numbers.rb new file mode 100644 index 0000000000..0d427dfa13 --- /dev/null +++ b/spec/support/shared_examples/shows_error_for_invalid_phone_numbers.rb @@ -0,0 +1,25 @@ +shared_examples_for "shows error for invalid phone numbers" do + it "shows error message for phone number < 12 digits" do + role == "admin" ? fill_in("Phone number", with: "+141632489") : fill_in("#{role}_phone_number", with: "+141632489") + click_on "Submit" + expect(page).to have_text "Phone number must be 12 digits including country code (+1)" + end + + it "shows error message for phone number > 12 digits" do + role == "admin" ? fill_in("Phone number", with: "+141632180923") : fill_in("#{role}_phone_number", with: "+141632180923") + click_on "Submit" + expect(page).to have_text "Phone number must be 12 digits including country code (+1)" + end + + it "shows error message for bad phone number" do + role == "admin" ? fill_in("Phone number", with: "+141632u809o") : fill_in("#{role}_phone_number", with: "+141632u809o") + click_on "Submit" + expect(page).to have_text "Phone number must have correct format" + end + + it "shows error message for phone number without country code" do + role == "admin" ? fill_in("Phone number", with: "+24163218092") : fill_in("#{role}_phone_number", with: "+24163218092") + click_on "Submit" + expect(page).to have_text "Phone number must have a valid country code (+1)" + end +end diff --git a/spec/system/casa_admins/edit_spec.rb b/spec/system/casa_admins/edit_spec.rb index 44660644a2..519ae988d0 100644 --- a/spec/system/casa_admins/edit_spec.rb +++ b/spec/system/casa_admins/edit_spec.rb @@ -9,7 +9,7 @@ it "can successfully edit user email and display name" do expected_email = "root@casa.com" expected_display_name = "Root Admin" - expected_phone_number = "+14163218092" + expected_phone_number = "+14398761234" visit edit_casa_admin_path(admin) @@ -29,57 +29,16 @@ end context "with invalid data" do - it "shows error message for phone number < 12 digits" do + let(:role) {"admin"} + before do visit edit_casa_admin_path(admin) - - fill_in "Email", with: "newemail@example.com" - fill_in "Display name", with: "Lumine" - fill_in "Phone number", with: "+141632489" - - click_on "Submit" - - expect(page).to have_text "Phone number must be 12 digits including country code (+1)" - end - - it "shows error message for phone number > 12 digits" do - visit edit_casa_admin_path(admin) - fill_in "Email", with: "newemail@example.com" fill_in "Display name", with: "Kadehara Kazuha" - fill_in "Phone number", with: "+141632180923" - - click_on "Submit" - - expect(page).to have_text "Phone number must be 12 digits including country code (+1)" end - it "shows error message for bad phone number" do - visit edit_casa_admin_path(admin) - - fill_in "Email", with: "newemail@example.com" - fill_in "Display name", with: "Nyan Cat" - fill_in "Phone number", with: "+141632u809o" - - click_on "Submit" - - expect(page).to have_text "Phone number must have correct format" - end - - it "shows error message for phone number without country code" do - visit edit_casa_admin_path(admin) - - fill_in "Email", with: "newemail@example.com" - fill_in "Display name", with: "Patrick Star" - fill_in "Phone number", with: "+24163218092" - - click_on "Submit" - - expect(page).to have_text "Phone number must have a valid country code (+1)" - end + it_should_behave_like "shows error for invalid phone numbers" it "shows error message for empty email" do - visit edit_casa_admin_path(admin) - fill_in "Email", with: "" fill_in "Display name", with: "" diff --git a/spec/system/supervisors/edit_spec.rb b/spec/system/supervisors/edit_spec.rb index 3871615448..9e17cf1633 100644 --- a/spec/system/supervisors/edit_spec.rb +++ b/spec/system/supervisors/edit_spec.rb @@ -37,73 +37,13 @@ end context "with invalid data" do - it "shows error message for phone number < 12 digits" do - supervisor_name = "Lesile Knope" - create(:supervisor, display_name: supervisor_name, casa_org: organization) + let(:role) {"supervisor"} + let(:supervisor) {create(:supervisor, display_name: "Lesile Knope", casa_org: organization)} + before do sign_in user - - visit supervisors_path - - within "#supervisors" do - click_on supervisor_name - end - - fill_in "supervisor_phone_number", with: "+1416321" - click_on "Submit" - - expect(page).to have_text("Phone number must be 12 digits including country code (+1)") - end - - it "shows error message for phone number > 12 digits" do - supervisor_name = "Lesile Knope" - create(:supervisor, display_name: supervisor_name, casa_org: organization) - sign_in user - - visit supervisors_path - - within "#supervisors" do - click_on supervisor_name - end - - fill_in "supervisor_phone_number", with: "+1416321432443" - click_on "Submit" - - expect(page).to have_text("Phone number must be 12 digits including country code (+1)") - end - - it "shows error message for bad phone number" do - supervisor_name = "Lesile Knope" - create(:supervisor, display_name: supervisor_name, casa_org: organization) - sign_in user - - visit supervisors_path - - within "#supervisors" do - click_on supervisor_name - end - - fill_in "supervisor_phone_number", with: "+1416321809u" - click_on "Submit" - - expect(page).to have_text("Phone number must have correct format") - end - - it "shows error message for phone number without country code" do - supervisor_name = "Lesile Knope" - create(:supervisor, display_name: supervisor_name, casa_org: organization) - sign_in user - - visit supervisors_path - - within "#supervisors" do - click_on supervisor_name - end - - fill_in "supervisor_phone_number", with: "+24163218092" - click_on "Submit" - - expect(page).to have_text("Phone number must have a valid country code (+1)") + visit edit_supervisor_path(supervisor) end + it_should_behave_like "shows error for invalid phone numbers" end it "can go to the supervisor edit page and see red message when there are no active volunteers" do diff --git a/spec/system/volunteers/edit_spec.rb b/spec/system/volunteers/edit_spec.rb index 1d18551ca1..928c8667fd 100644 --- a/spec/system/volunteers/edit_spec.rb +++ b/spec/system/volunteers/edit_spec.rb @@ -19,49 +19,21 @@ before do sign_in admin visit edit_volunteer_path(volunteer) + fill_in "volunteer_email", with: "newemail@example.com" + fill_in "volunteer_display_name", with: "Kamisato Ayato" end context "with valid data" do it "updates successfully" do - fill_in "volunteer_email", with: "newemail@example.com" - fill_in "volunteer_display_name", with: "Mickey Mouse" click_on "Submit" expect(page).to have_text "Volunteer was successfully updated." end end context "with invalid data" do - it "shows error message for phone number < 12 digits" do - fill_in "volunteer_email", with: "newemail@example.com" - fill_in "volunteer_display_name", with: "Driving Thunder" - fill_in "volunteer_phone_number", with: "+141632489" - click_on "Submit" - expect(page).to have_text "Phone number must be 12 digits including country code (+1)" - end - - it "shows error message for phone number > 12 digits" do - fill_in "volunteer_email", with: "newemail@example.com" - fill_in "volunteer_display_name", with: "Kamisato Ayato" - fill_in "volunteer_phone_number", with: "+141632180923" - click_on "Submit" - expect(page).to have_text "Phone number must be 12 digits including country code (+1)" - end + let(:role) {"volunteer"} - it "shows error message for bad phone number" do - fill_in "volunteer_email", with: "newemail@example.com" - fill_in "volunteer_display_name", with: "Rex Lapis" - fill_in "volunteer_phone_number", with: "+141632u809o" - click_on "Submit" - expect(page).to have_text "Phone number must have correct format" - end - - it "shows error message for phone number without country code" do - fill_in "volunteer_email", with: "newemail@example.com" - fill_in "volunteer_display_name", with: "Lara Croft" - fill_in "volunteer_phone_number", with: "+24163218092" - click_on "Submit" - expect(page).to have_text "Phone number must have a valid country code (+1)" - end + it_should_behave_like "shows error for invalid phone numbers" it "shows error message for duplicate email" do volunteer.supervisor = build(:supervisor) From cc08751b428df6a4eca3136b79a043f28f722415 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Fri, 15 Apr 2022 00:32:22 -0400 Subject: [PATCH 19/32] refactor system spec for users to use shared_examples --- .../shows_error_for_invalid_phone_numbers.rb | 16 ++++++------ spec/system/users/edit_spec.rb | 25 ++----------------- 2 files changed, 10 insertions(+), 31 deletions(-) diff --git a/spec/support/shared_examples/shows_error_for_invalid_phone_numbers.rb b/spec/support/shared_examples/shows_error_for_invalid_phone_numbers.rb index 0d427dfa13..140e153b9f 100644 --- a/spec/support/shared_examples/shows_error_for_invalid_phone_numbers.rb +++ b/spec/support/shared_examples/shows_error_for_invalid_phone_numbers.rb @@ -1,25 +1,25 @@ shared_examples_for "shows error for invalid phone numbers" do it "shows error message for phone number < 12 digits" do - role == "admin" ? fill_in("Phone number", with: "+141632489") : fill_in("#{role}_phone_number", with: "+141632489") - click_on "Submit" + role == "admin" || role == "user" ? fill_in("Phone number", with: "+141632489") : fill_in("#{role}_phone_number", with: "+141632489") + role == "user" ? click_on("Update Profile") : click_on("Submit") expect(page).to have_text "Phone number must be 12 digits including country code (+1)" end it "shows error message for phone number > 12 digits" do - role == "admin" ? fill_in("Phone number", with: "+141632180923") : fill_in("#{role}_phone_number", with: "+141632180923") - click_on "Submit" + role == "admin" || role == "user" ? fill_in("Phone number", with: "+141632180923") : fill_in("#{role}_phone_number", with: "+141632180923") + role == "user" ? click_on("Update Profile") : click_on("Submit") expect(page).to have_text "Phone number must be 12 digits including country code (+1)" end it "shows error message for bad phone number" do - role == "admin" ? fill_in("Phone number", with: "+141632u809o") : fill_in("#{role}_phone_number", with: "+141632u809o") - click_on "Submit" + role == "admin" || role == "user" ? fill_in("Phone number", with: "+141632u809o") : fill_in("#{role}_phone_number", with: "+141632u809o") + role == "user" ? click_on("Update Profile") : click_on("Submit") expect(page).to have_text "Phone number must have correct format" end it "shows error message for phone number without country code" do - role == "admin" ? fill_in("Phone number", with: "+24163218092") : fill_in("#{role}_phone_number", with: "+24163218092") - click_on "Submit" + role == "admin" || role == "user" ? fill_in("Phone number", with: "+24163218092") : fill_in("#{role}_phone_number", with: "+24163218092") + role == "user" ? click_on("Update Profile") : click_on("Submit") expect(page).to have_text "Phone number must have a valid country code (+1)" end end diff --git a/spec/system/users/edit_spec.rb b/spec/system/users/edit_spec.rb index 7a59dc0819..9d4ccc2c1a 100644 --- a/spec/system/users/edit_spec.rb +++ b/spec/system/users/edit_spec.rb @@ -113,6 +113,7 @@ end context "when admin" do + let(:role) {"user"} before do sign_in admin visit edit_users_path @@ -124,29 +125,7 @@ expect(page).to have_text("Display name can't be blank") end - it "is not able to update the profile when phone number is too short" do - fill_in "Phone number", with: "+1416321" - click_on "Update Profile" - expect(page).to have_text("Phone number must be 12 digits including country code (+1)") - end - - it "is not able to update the profile when phone number is too long" do - fill_in "Phone number", with: "+1416321809231" - click_on "Update Profile" - expect(page).to have_text("Phone number must be 12 digits including country code (+1)") - end - - it "is not able to update the profile when phone number is not valid" do - fill_in "Phone number", with: "+1416321809o" - click_on "Update Profile" - expect(page).to have_text("Phone number must have correct format") - end - - it "is not able to update the profile when phone number includes an invalid country code" do - fill_in "Phone number", with: "+24163218092" - click_on "Update Profile" - expect(page).to have_text("Phone number must have a valid country code (+1)") - end + it_should_behave_like "shows error for invalid phone numbers" it "is able to update the email if user is a admin" do expect(page).to have_field("Email", disabled: false) From 170d10e456bfac0160915f484c3f78b241fdb5c8 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Fri, 15 Apr 2022 15:01:57 -0400 Subject: [PATCH 20/32] create partial for the user form inputs to avoid duplication in views --- app/views/casa_admins/_form.html.erb | 15 +-------------- app/views/shared/_edit_form.html.erb | 22 ++++++++++++++++++++++ app/views/supervisors/edit.html.erb | 23 +---------------------- app/views/volunteers/edit.html.erb | 19 +------------------ 4 files changed, 25 insertions(+), 54 deletions(-) create mode 100644 app/views/shared/_edit_form.html.erb diff --git a/app/views/casa_admins/_form.html.erb b/app/views/casa_admins/_form.html.erb index 1ae8e70e90..5d1d72f92b 100644 --- a/app/views/casa_admins/_form.html.erb +++ b/app/views/casa_admins/_form.html.erb @@ -5,20 +5,7 @@ <%= form_with(model: casa_admin, local: true) do |form| %> <%= render "/shared/error_messages", resource: casa_admin %> -

- <%= form.label :email, t("common.email") %> - <%= form.text_field :email, class: "form-control" %> -
- -
- <%= form.label :display_name, t("common.display_name") %> - <%= form.text_field :display_name, class: "form-control" %> -
- -
- <%= form.label :phone_number, t("common.phone_number") %> - <%= form.text_field :phone_number, class: "form-control" %> -
+ <%= render "/shared/edit_form", resource: casa_admin, f: form %> <%= render "/shared/invite_login", resource: casa_admin %> diff --git a/app/views/shared/_edit_form.html.erb b/app/views/shared/_edit_form.html.erb new file mode 100644 index 0000000000..141d7d79e7 --- /dev/null +++ b/app/views/shared/_edit_form.html.erb @@ -0,0 +1,22 @@ +
+ <%= f.label :email, t("common.email") %> + <% if policy(resource).update_supervisor_email? %> + <%= f.text_field :email, class: "form-control" %> + <% else %> + + <% end %> +
+ +
+ <%= f.label :display_name, t("common.display_name") %> + <% if policy(resource).update_supervisor_name? %> + <%= f.text_field :display_name, class: "form-control" %> + <% else %> + + <% end %> +
+ +
+ <%= f.label :phone_number, t("common.phone_number") %> + <%= f.text_field :phone_number, class: "form-control" %> +
diff --git a/app/views/supervisors/edit.html.erb b/app/views/supervisors/edit.html.erb index 7a82e5a1e1..c8de7fc78c 100644 --- a/app/views/supervisors/edit.html.erb +++ b/app/views/supervisors/edit.html.erb @@ -5,28 +5,7 @@ <%= form_for @supervisor, as: :supervisor, url: supervisor_path(@supervisor) do |form| %> <%= render "/shared/error_messages", resource: @supervisor %> -
- <%= form.label :email, t("common.email") %> - <% if policy(@supervisor).update_supervisor_email? %> - <%= form.text_field :email, class: "form-control" %> - <% else %> - - <% end %> -
- -
- <%= form.label :display_name, t("common.display_name") %> - <% if policy(@supervisor).update_supervisor_name? %> - <%= form.text_field :display_name, class: "form-control" %> - <% else %> - - <% end %> -
- -
- <%= form.label :phone_number, t("common.phone_number") %> - <%= form.text_field :phone_number, class: "form-control" %> -
+ <%= render "/shared/edit_form", resource: @supervisor, f: form %>

<%= render "/shared/invite_login", resource: @supervisor %> diff --git a/app/views/volunteers/edit.html.erb b/app/views/volunteers/edit.html.erb index 1041c1be05..fd2cba8b82 100644 --- a/app/views/volunteers/edit.html.erb +++ b/app/views/volunteers/edit.html.erb @@ -15,24 +15,7 @@ <%= form_for @volunteer, url: volunteer_path(@volunteer) do |form| %> <%= render "/shared/error_messages", resource: @volunteer %> -

- <%= form.label :email, t("common.email") %> - <% if policy(:volunteer).update_volunteer_email? %> - <%= form.text_field :email, class: "form-control" %> - <% else %> - - <% end %> -
- -
- <%= form.label :display_name, t("common.display_name") %> - <%= form.text_field :display_name, class: "form-control" %> -
- -
- <%= form.label :phone_number, t("common.phone_number") %> - <%= form.text_field :phone_number, class: "form-control" %> -
+ <%= render "/shared/edit_form", resource: @volunteer, f: form %> <%= render "/shared/invite_login", resource: @volunteer %> From d375fb2871936237581f320881e6569728fe5672 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Fri, 15 Apr 2022 16:37:02 -0400 Subject: [PATCH 21/32] create a policy for previous partial --- app/policies/user_policy.rb | 8 ++++++++ app/views/shared/_edit_form.html.erb | 4 ++-- spec/policies/user_policy_spec.rb | 23 +++++++++++++++++++++++ 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 20efc3c3ce..bbb5b3f410 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -25,6 +25,14 @@ def update_supervisor_name? update_supervisor_email? end + def update_user_setting? + if is_supervisor? + # allow access to own record or volunteer record + return record == user || record.volunteer? + end + is_admin? + end + def edit_name?(viewed_user) is_admin? || viewed_user == user end diff --git a/app/views/shared/_edit_form.html.erb b/app/views/shared/_edit_form.html.erb index 141d7d79e7..813bd26427 100644 --- a/app/views/shared/_edit_form.html.erb +++ b/app/views/shared/_edit_form.html.erb @@ -1,6 +1,6 @@
<%= f.label :email, t("common.email") %> - <% if policy(resource).update_supervisor_email? %> + <% if policy(resource).update_user_setting? %> <%= f.text_field :email, class: "form-control" %> <% else %> @@ -9,7 +9,7 @@
<%= f.label :display_name, t("common.display_name") %> - <% if policy(resource).update_supervisor_name? %> + <% if policy(resource).update_user_setting? %> <%= f.text_field :display_name, class: "form-control" %> <% else %> diff --git a/spec/policies/user_policy_spec.rb b/spec/policies/user_policy_spec.rb index 83117297ae..085058d17f 100644 --- a/spec/policies/user_policy_spec.rb +++ b/spec/policies/user_policy_spec.rb @@ -20,4 +20,27 @@ is_expected.to permit(volunteer) end end + + permissions :update_user_setting? do + context "when user is an admin" do + it "allows update settings of all roles" do + is_expected.to permit(casa_admin) + end + end + + context "when user is a supervisor" do + it "allows supervisors to update another volunteer settings" do + is_expected.to permit(supervisor, volunteer) + end + + it "allows supervisors to update their own settings" do + is_expected.to permit(supervisor, supervisor) + end + + it "does not allow supervisor to update another supervisor settings" do + another_supervisor = build_stubbed(:supervisor) + is_expected.not_to permit(supervisor, another_supervisor) + end + end + end end From 7e6c6da856f45e28dc74f2a0db43e19e693058b7 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Fri, 15 Apr 2022 16:39:32 -0400 Subject: [PATCH 22/32] lint files --- spec/policies/user_policy_spec.rb | 2 +- spec/system/casa_admins/edit_spec.rb | 2 +- spec/system/supervisors/edit_spec.rb | 4 ++-- spec/system/users/edit_spec.rb | 2 +- spec/system/volunteers/edit_spec.rb | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/policies/user_policy_spec.rb b/spec/policies/user_policy_spec.rb index 085058d17f..ff0b2f6562 100644 --- a/spec/policies/user_policy_spec.rb +++ b/spec/policies/user_policy_spec.rb @@ -32,7 +32,7 @@ it "allows supervisors to update another volunteer settings" do is_expected.to permit(supervisor, volunteer) end - + it "allows supervisors to update their own settings" do is_expected.to permit(supervisor, supervisor) end diff --git a/spec/system/casa_admins/edit_spec.rb b/spec/system/casa_admins/edit_spec.rb index 519ae988d0..c0bd1fce2a 100644 --- a/spec/system/casa_admins/edit_spec.rb +++ b/spec/system/casa_admins/edit_spec.rb @@ -29,7 +29,7 @@ end context "with invalid data" do - let(:role) {"admin"} + let(:role) { "admin" } before do visit edit_casa_admin_path(admin) fill_in "Email", with: "newemail@example.com" diff --git a/spec/system/supervisors/edit_spec.rb b/spec/system/supervisors/edit_spec.rb index 9e17cf1633..d915f1fc15 100644 --- a/spec/system/supervisors/edit_spec.rb +++ b/spec/system/supervisors/edit_spec.rb @@ -37,8 +37,8 @@ end context "with invalid data" do - let(:role) {"supervisor"} - let(:supervisor) {create(:supervisor, display_name: "Lesile Knope", casa_org: organization)} + let(:role) { "supervisor" } + let(:supervisor) { create(:supervisor, display_name: "Lesile Knope", casa_org: organization) } before do sign_in user visit edit_supervisor_path(supervisor) diff --git a/spec/system/users/edit_spec.rb b/spec/system/users/edit_spec.rb index 9d4ccc2c1a..00bc89cbdd 100644 --- a/spec/system/users/edit_spec.rb +++ b/spec/system/users/edit_spec.rb @@ -113,7 +113,7 @@ end context "when admin" do - let(:role) {"user"} + let(:role) { "user" } before do sign_in admin visit edit_users_path diff --git a/spec/system/volunteers/edit_spec.rb b/spec/system/volunteers/edit_spec.rb index 928c8667fd..d299f6cc6d 100644 --- a/spec/system/volunteers/edit_spec.rb +++ b/spec/system/volunteers/edit_spec.rb @@ -31,7 +31,7 @@ end context "with invalid data" do - let(:role) {"volunteer"} + let(:role) { "volunteer" } it_should_behave_like "shows error for invalid phone numbers" From 3e795c3915014279cf0001e0de556da70026ef0d Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Fri, 15 Apr 2022 17:47:39 -0400 Subject: [PATCH 23/32] delete duplicate migration for phone number column --- db/migrate/20220403213338_add_phone_number_to_users.rb | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 db/migrate/20220403213338_add_phone_number_to_users.rb diff --git a/db/migrate/20220403213338_add_phone_number_to_users.rb b/db/migrate/20220403213338_add_phone_number_to_users.rb deleted file mode 100644 index 9d9b7a0949..0000000000 --- a/db/migrate/20220403213338_add_phone_number_to_users.rb +++ /dev/null @@ -1,5 +0,0 @@ -class AddPhoneNumberToUsers < ActiveRecord::Migration[7.0] - def change - add_column :users, :phone_number, :string, default: "" - end -end From 47c778919b24472fe5dc84c89f0d0a7159d42eba Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Fri, 15 Apr 2022 18:10:28 -0400 Subject: [PATCH 24/32] Revert "delete duplicate migration for phone number column" This reverts commit 3e795c3915014279cf0001e0de556da70026ef0d. revert back to working point --- db/migrate/20220403213338_add_phone_number_to_users.rb | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 db/migrate/20220403213338_add_phone_number_to_users.rb diff --git a/db/migrate/20220403213338_add_phone_number_to_users.rb b/db/migrate/20220403213338_add_phone_number_to_users.rb new file mode 100644 index 0000000000..9d9b7a0949 --- /dev/null +++ b/db/migrate/20220403213338_add_phone_number_to_users.rb @@ -0,0 +1,5 @@ +class AddPhoneNumberToUsers < ActiveRecord::Migration[7.0] + def change + add_column :users, :phone_number, :string, default: "" + end +end From b1218af08571547285940690107079c1fa138f2b Mon Sep 17 00:00:00 2001 From: Xihai Luo <86758440+xihai01@users.noreply.github.com> Date: Fri, 15 Apr 2022 18:20:44 -0400 Subject: [PATCH 25/32] delete duplicate migration --- db/migrate/20220403213338_add_phone_number_to_users.rb | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 db/migrate/20220403213338_add_phone_number_to_users.rb diff --git a/db/migrate/20220403213338_add_phone_number_to_users.rb b/db/migrate/20220403213338_add_phone_number_to_users.rb deleted file mode 100644 index 9d9b7a0949..0000000000 --- a/db/migrate/20220403213338_add_phone_number_to_users.rb +++ /dev/null @@ -1,5 +0,0 @@ -class AddPhoneNumberToUsers < ActiveRecord::Migration[7.0] - def change - add_column :users, :phone_number, :string, default: "" - end -end From 77b7030892ccb1c7716e516c84152681473d5994 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Fri, 15 Apr 2022 18:48:40 -0400 Subject: [PATCH 26/32] feature 1785 ready to PR --- spec/lib/importers/supervisor_importer_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/importers/supervisor_importer_spec.rb b/spec/lib/importers/supervisor_importer_spec.rb index d4d46d25f0..95d2f4a76c 100644 --- a/spec/lib/importers/supervisor_importer_spec.rb +++ b/spec/lib/importers/supervisor_importer_spec.rb @@ -119,7 +119,7 @@ expect(alert[:type]).to eq(:error) expect(alert[:message]).to eq("Not all rows were imported.") - expect(alert[:exported_rows]).to include("Phone number is not in correct format: 1XXXXXXXXXX") + expect(alert[:exported_rows]).to include("Phone number must be 12 digits including country code (+1)") end end From d10a2a441a6ba517e1ad0b722b5ca56e728183cf Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Fri, 15 Apr 2022 19:56:49 -0400 Subject: [PATCH 27/32] fix import spec with new error msgs --- spec/lib/importers/supervisor_importer_spec.rb | 2 +- spec/lib/importers/volunteer_importer_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/lib/importers/supervisor_importer_spec.rb b/spec/lib/importers/supervisor_importer_spec.rb index 95d2f4a76c..313c139e58 100644 --- a/spec/lib/importers/supervisor_importer_spec.rb +++ b/spec/lib/importers/supervisor_importer_spec.rb @@ -119,7 +119,7 @@ expect(alert[:type]).to eq(:error) expect(alert[:message]).to eq("Not all rows were imported.") - expect(alert[:exported_rows]).to include("Phone number must be 12 digits including country code (+1)") + expect(alert[:exported_rows]).to include("Phone number must be 12 digits including country code (+1)") end end diff --git a/spec/lib/importers/volunteer_importer_spec.rb b/spec/lib/importers/volunteer_importer_spec.rb index 8f50c1523e..daa0b15df9 100644 --- a/spec/lib/importers/volunteer_importer_spec.rb +++ b/spec/lib/importers/volunteer_importer_spec.rb @@ -91,7 +91,7 @@ expect(alert[:type]).to eq(:error) expect(alert[:message]).to eq("Not all rows were imported.") - expect(alert[:exported_rows]).to include("Phone number is not in correct format: 1XXXXXXXXXX") + expect(alert[:exported_rows]).to include("Phone number must be 12 digits including country code (+1)") end end end From 870c8f8f1daea8ddb2effdeb5547291ca672c745 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Sat, 16 Apr 2022 23:22:35 -0400 Subject: [PATCH 28/32] fix policy permissions and correct some people's names --- app/policies/user_policy.rb | 4 ++-- spec/policies/user_policy_spec.rb | 19 ++++++++++++++++++- spec/system/casa_admins/edit_spec.rb | 2 +- spec/system/supervisors/edit_spec.rb | 2 +- 4 files changed, 22 insertions(+), 5 deletions(-) diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index bbb5b3f410..851823e7a7 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -26,11 +26,11 @@ def update_supervisor_name? end def update_user_setting? - if is_supervisor? + if is_supervisor_same_org? # allow access to own record or volunteer record return record == user || record.volunteer? end - is_admin? + is_admin_same_org? end def edit_name?(viewed_user) diff --git a/spec/policies/user_policy_spec.rb b/spec/policies/user_policy_spec.rb index ff0b2f6562..56d903b948 100644 --- a/spec/policies/user_policy_spec.rb +++ b/spec/policies/user_policy_spec.rb @@ -7,6 +7,11 @@ let(:volunteer) { build_stubbed(:volunteer) } let(:supervisor) { build_stubbed(:supervisor) } + let(:org_b) { build_stubbed(:casa_org) } + let(:casa_admin_b) { build_stubbed(:casa_admin, casa_org: org_b) } + let(:supervisor_b) { build_stubbed(:supervisor, casa_org: org_b) } + let(:volunteer_b) { build_stubbed(:volunteer, casa_org: org_b) } + permissions :edit?, :update?, :update_password? do it "allows casa_admins" do is_expected.to permit(casa_admin) @@ -24,7 +29,15 @@ permissions :update_user_setting? do context "when user is an admin" do it "allows update settings of all roles" do - is_expected.to permit(casa_admin) + is_expected.to permit(casa_admin_b, casa_admin_b) + is_expected.to permit(casa_admin_b, supervisor_b) + is_expected.to permit(casa_admin_b, volunteer_b) + end + + it "does not allow update settings of roles in different casa org" do + is_expected.not_to permit(casa_admin, casa_admin_b) + is_expected.not_to permit(casa_admin, supervisor_b) + is_expected.not_to permit(casa_admin, volunteer_b) end end @@ -33,6 +46,10 @@ is_expected.to permit(supervisor, volunteer) end + it "does not allow supervisor to update a volunteer in a different casa org" do + is_expected.not_to permit(supervisor, volunteer_b) + end + it "allows supervisors to update their own settings" do is_expected.to permit(supervisor, supervisor) end diff --git a/spec/system/casa_admins/edit_spec.rb b/spec/system/casa_admins/edit_spec.rb index c0bd1fce2a..216d5d4128 100644 --- a/spec/system/casa_admins/edit_spec.rb +++ b/spec/system/casa_admins/edit_spec.rb @@ -33,7 +33,7 @@ before do visit edit_casa_admin_path(admin) fill_in "Email", with: "newemail@example.com" - fill_in "Display name", with: "Kadehara Kazuha" + fill_in "Display name", with: "Kaedehara Kazuha" end it_should_behave_like "shows error for invalid phone numbers" diff --git a/spec/system/supervisors/edit_spec.rb b/spec/system/supervisors/edit_spec.rb index d915f1fc15..dc8eb39958 100644 --- a/spec/system/supervisors/edit_spec.rb +++ b/spec/system/supervisors/edit_spec.rb @@ -38,7 +38,7 @@ context "with invalid data" do let(:role) { "supervisor" } - let(:supervisor) { create(:supervisor, display_name: "Lesile Knope", casa_org: organization) } + let(:supervisor) { create(:supervisor, display_name: "Leslie Knope", casa_org: organization) } before do sign_in user visit edit_supervisor_path(supervisor) From a6a92457cc232441e72e5b5ff5dddf2cdd96d958 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Sat, 16 Apr 2022 23:56:23 -0400 Subject: [PATCH 29/32] remove admin has to be from same org for policy permissions --- app/policies/user_policy.rb | 2 +- spec/policies/user_policy_spec.rb | 10 +--------- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 851823e7a7..8fcd480ae1 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -30,7 +30,7 @@ def update_user_setting? # allow access to own record or volunteer record return record == user || record.volunteer? end - is_admin_same_org? + is_admin? end def edit_name?(viewed_user) diff --git a/spec/policies/user_policy_spec.rb b/spec/policies/user_policy_spec.rb index 56d903b948..1274a2afb3 100644 --- a/spec/policies/user_policy_spec.rb +++ b/spec/policies/user_policy_spec.rb @@ -29,15 +29,7 @@ permissions :update_user_setting? do context "when user is an admin" do it "allows update settings of all roles" do - is_expected.to permit(casa_admin_b, casa_admin_b) - is_expected.to permit(casa_admin_b, supervisor_b) - is_expected.to permit(casa_admin_b, volunteer_b) - end - - it "does not allow update settings of roles in different casa org" do - is_expected.not_to permit(casa_admin, casa_admin_b) - is_expected.not_to permit(casa_admin, supervisor_b) - is_expected.not_to permit(casa_admin, volunteer_b) + is_expected.to permit(casa_admin) end end From 4a0aeef3ad502314498cf6df351d549e3155a484 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Sun, 17 Apr 2022 16:29:02 -0400 Subject: [PATCH 30/32] update db schema --- app/models/casa_admin.rb | 2 +- app/models/casa_case_contact_type.rb | 4 ++-- app/models/case_assignment.rb | 4 ++-- app/models/case_contact.rb | 4 ++-- app/models/case_contact_contact_type.rb | 4 ++-- app/models/contact_type.rb | 2 +- app/models/contact_type_group.rb | 2 +- app/models/court_date.rb | 2 +- app/models/emancipation_option.rb | 2 +- app/models/hearing_type.rb | 2 +- app/models/judge.rb | 2 +- app/models/supervisor.rb | 2 +- app/models/supervisor_volunteer.rb | 4 ++-- app/models/user.rb | 2 +- app/models/volunteer.rb | 2 +- db/schema.rb | 1 - 16 files changed, 20 insertions(+), 21 deletions(-) diff --git a/app/models/casa_admin.rb b/app/models/casa_admin.rb index c0dbf6ef5b..511bc62a3a 100644 --- a/app/models/casa_admin.rb +++ b/app/models/casa_admin.rb @@ -46,7 +46,7 @@ def change_to_supervisor! # created_at :datetime not null # updated_at :datetime not null # casa_org_id :bigint not null -# invited_by_id :bigint +# invited_by_id :integer # # Indexes # diff --git a/app/models/casa_case_contact_type.rb b/app/models/casa_case_contact_type.rb index 1a8c7f3816..3ce92ffd98 100644 --- a/app/models/casa_case_contact_type.rb +++ b/app/models/casa_case_contact_type.rb @@ -13,8 +13,8 @@ class CasaCaseContactType < ApplicationRecord # id :bigint not null, primary key # created_at :datetime not null # updated_at :datetime not null -# casa_case_id :bigint not null -# contact_type_id :bigint not null +# casa_case_id :integer not null +# contact_type_id :integer not null # # Indexes # diff --git a/app/models/case_assignment.rb b/app/models/case_assignment.rb index 4898c870ac..0bc81d1fdb 100644 --- a/app/models/case_assignment.rb +++ b/app/models/case_assignment.rb @@ -37,8 +37,8 @@ def casa_case_and_volunteer_must_belong_to_same_casa_org # active :boolean default(TRUE), not null # created_at :datetime not null # updated_at :datetime not null -# casa_case_id :bigint not null -# volunteer_id :bigint not null +# casa_case_id :integer not null +# volunteer_id :integer not null # # Indexes # diff --git a/app/models/case_contact.rb b/app/models/case_contact.rb index 8427ccb701..00da20f61c 100644 --- a/app/models/case_contact.rb +++ b/app/models/case_contact.rb @@ -215,8 +215,8 @@ def self.options_for_sorted_by # want_driving_reimbursement :boolean default(FALSE) # created_at :datetime not null # updated_at :datetime not null -# casa_case_id :bigint not null -# creator_id :bigint not null +# casa_case_id :integer not null +# creator_id :integer not null # # Indexes # diff --git a/app/models/case_contact_contact_type.rb b/app/models/case_contact_contact_type.rb index 205ea49de2..57da9ba9f4 100644 --- a/app/models/case_contact_contact_type.rb +++ b/app/models/case_contact_contact_type.rb @@ -13,8 +13,8 @@ class CaseContactContactType < ApplicationRecord # id :bigint not null, primary key # created_at :datetime not null # updated_at :datetime not null -# case_contact_id :bigint not null -# contact_type_id :bigint not null +# case_contact_id :integer not null +# contact_type_id :integer not null # # Indexes # diff --git a/app/models/contact_type.rb b/app/models/contact_type.rb index 6c0001402f..fe862eb4a7 100644 --- a/app/models/contact_type.rb +++ b/app/models/contact_type.rb @@ -22,7 +22,7 @@ class ContactType < ApplicationRecord # name :string not null # created_at :datetime not null # updated_at :datetime not null -# contact_type_group_id :bigint not null +# contact_type_group_id :integer not null # # Indexes # diff --git a/app/models/contact_type_group.rb b/app/models/contact_type_group.rb index f977a2f1ee..7f81398ecc 100644 --- a/app/models/contact_type_group.rb +++ b/app/models/contact_type_group.rb @@ -22,7 +22,7 @@ class ContactTypeGroup < ApplicationRecord # name :string not null # created_at :datetime not null # updated_at :datetime not null -# casa_org_id :bigint not null +# casa_org_id :integer not null # # Indexes # diff --git a/app/models/court_date.rb b/app/models/court_date.rb index 045b9bef0b..73ddd69fef 100644 --- a/app/models/court_date.rb +++ b/app/models/court_date.rb @@ -73,7 +73,7 @@ def case_court_orders_context_hash # date :datetime not null # created_at :datetime not null # updated_at :datetime not null -# casa_case_id :bigint not null +# casa_case_id :integer not null # hearing_type_id :bigint # judge_id :bigint # diff --git a/app/models/emancipation_option.rb b/app/models/emancipation_option.rb index 17ad42f96d..6f1293ff26 100644 --- a/app/models/emancipation_option.rb +++ b/app/models/emancipation_option.rb @@ -24,7 +24,7 @@ class EmancipationOption < ApplicationRecord # name :string not null # created_at :datetime not null # updated_at :datetime not null -# emancipation_category_id :bigint not null +# emancipation_category_id :integer not null # # Indexes # diff --git a/app/models/hearing_type.rb b/app/models/hearing_type.rb index 11cdbf1768..509c34e04e 100644 --- a/app/models/hearing_type.rb +++ b/app/models/hearing_type.rb @@ -17,7 +17,7 @@ class HearingType < ApplicationRecord # id :bigint not null, primary key # active :boolean default(TRUE), not null # name :string not null -# casa_org_id :bigint not null +# casa_org_id :integer not null # # Indexes # diff --git a/app/models/judge.rb b/app/models/judge.rb index dda04bc1e2..b6a458497e 100644 --- a/app/models/judge.rb +++ b/app/models/judge.rb @@ -18,7 +18,7 @@ class Judge < ApplicationRecord # name :string # created_at :datetime not null # updated_at :datetime not null -# casa_org_id :bigint not null +# casa_org_id :integer not null # # Indexes # diff --git a/app/models/supervisor.rb b/app/models/supervisor.rb index 55a445d483..3f9262abaa 100644 --- a/app/models/supervisor.rb +++ b/app/models/supervisor.rb @@ -73,7 +73,7 @@ def recently_unassigned_volunteers # created_at :datetime not null # updated_at :datetime not null # casa_org_id :bigint not null -# invited_by_id :bigint +# invited_by_id :integer # # Indexes # diff --git a/app/models/supervisor_volunteer.rb b/app/models/supervisor_volunteer.rb index 73c0c415c7..3a8f387453 100644 --- a/app/models/supervisor_volunteer.rb +++ b/app/models/supervisor_volunteer.rb @@ -25,8 +25,8 @@ def ensure_supervisor_and_volunteer_belong_to_same_casa_org # is_active :boolean default(TRUE) # created_at :datetime not null # updated_at :datetime not null -# supervisor_id :bigint not null -# volunteer_id :bigint not null +# supervisor_id :integer not null +# volunteer_id :integer not null # # Indexes # diff --git a/app/models/user.rb b/app/models/user.rb index 386773e519..dba2c6a58c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -194,7 +194,7 @@ def last_deactivated_by # created_at :datetime not null # updated_at :datetime not null # casa_org_id :bigint not null -# invited_by_id :bigint +# invited_by_id :integer # # Indexes # diff --git a/app/models/volunteer.rb b/app/models/volunteer.rb index 2a384ead3c..27ef1380fc 100644 --- a/app/models/volunteer.rb +++ b/app/models/volunteer.rb @@ -153,7 +153,7 @@ def cases_where_contact_made_in_days(num_days = CONTACT_MADE_IN_DAYS_NUM) # created_at :datetime not null # updated_at :datetime not null # casa_org_id :bigint not null -# invited_by_id :bigint +# invited_by_id :integer # # Indexes # diff --git a/db/schema.rb b/db/schema.rb index a1860475e6..49456d77e8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -127,7 +127,6 @@ t.string "footer_links", default: [], array: true t.string "slug" t.boolean "show_driving_reimbursement", default: true - t.boolean "show_fund_request", default: false t.index ["slug"], name: "index_casa_orgs_on_slug", unique: true end From b4709d96b82e8921b3c70a2c698841a0d66a46a0 Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Sun, 17 Apr 2022 16:57:45 -0400 Subject: [PATCH 31/32] create view spec for supervisors/edit to check for editable/disabled fields --- spec/views/supervisors/edit.html.erb_spec.rb | 28 ++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/spec/views/supervisors/edit.html.erb_spec.rb b/spec/views/supervisors/edit.html.erb_spec.rb index 7268f8881d..c1822e9fd4 100644 --- a/spec/views/supervisors/edit.html.erb_spec.rb +++ b/spec/views/supervisors/edit.html.erb_spec.rb @@ -60,6 +60,34 @@ expect(rendered).to have_text("Password reset last sent \n never") end + it "shows profile info form fields as editable for a supervisor editing their own profile" do + enable_pundit(view, supervisor) + + assign :supervisor, supervisor + assign :all_volunteers_ever_assigned, [volunteer] + assign :available_volunteers, [] + + render template: "supervisors/edit" + + expect(rendered).to have_field("supervisor_email") + expect(rendered).to have_field("supervisor_display_name") + expect(rendered).to have_field("supervisor_phone_number") + end + + it "shows profile info form fields as disabled for a supervisor editing another supervisor" do + enable_pundit(view, supervisor) + + assign :supervisor, build_stubbed(:supervisor, casa_org: build_stubbed(:casa_org)) + assign :all_volunteers_ever_assigned, [volunteer] + assign :available_volunteers, [] + + render template: "supervisors/edit" + + expect(rendered).not_to have_field("supervisor_email") + expect(rendered).not_to have_field("supervisor_display_name") + expect(rendered).not_to have_field("supervisor_phone_number") + end + it "shows for an admin editing a supervisor" do enable_pundit(view, supervisor) allow(view).to receive(:current_user).and_return(admin) From 1d6b67531d1e8a929adb225a5125e9bfc64c1c5e Mon Sep 17 00:00:00 2001 From: Xihai Luo Date: Sun, 17 Apr 2022 20:17:27 -0400 Subject: [PATCH 32/32] edit supervisors/edit view with permissions for phone number field --- app/views/shared/_edit_form.html.erb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/views/shared/_edit_form.html.erb b/app/views/shared/_edit_form.html.erb index 813bd26427..d5f4c9b6fa 100644 --- a/app/views/shared/_edit_form.html.erb +++ b/app/views/shared/_edit_form.html.erb @@ -18,5 +18,9 @@
<%= f.label :phone_number, t("common.phone_number") %> - <%= f.text_field :phone_number, class: "form-control" %> + <% if policy(resource).update_user_setting? %> + <%= f.text_field :phone_number, class: "form-control" %> + <% else %> + + <% end %>