diff --git a/Gemfile.lock b/Gemfile.lock index 7fdef3710..2f51d6a29 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -141,7 +141,7 @@ GEM coderay (1.1.3) coercible (1.0.0) descendants_tracker (~> 0.0.1) - concurrent-ruby (1.3.3) + concurrent-ruby (1.3.4) connection_pool (2.4.1) crack (1.0.0) bigdecimal @@ -180,7 +180,7 @@ GEM rubocop smart_properties erubi (1.13.0) - et-orbi (1.2.7) + et-orbi (1.2.11) tzinfo event_stream_parser (1.0.0) excon (0.109.0) @@ -198,8 +198,8 @@ GEM faraday-net_http (3.1.0) net-http ffi (1.16.3) - fugit (1.10.0) - et-orbi (~> 1, >= 1.2.7) + fugit (1.11.1) + et-orbi (~> 1, >= 1.2.11) raabro (~> 1.4) globalid (1.2.1) activesupport (>= 6.1) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 2f7bb2bd6..c5c5241f9 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -192,14 +192,14 @@ private def user_params if @user == current_user if current_user.data_coordinator? || current_user.support? - params.require(:user).permit(:email, :phone, :name, :password, :password_confirmation, :role, :is_dpo, :is_key_contact, :initial_confirmation_sent) + params.require(:user).permit(:email, :phone, :phone_extension, :name, :password, :password_confirmation, :role, :is_dpo, :is_key_contact, :initial_confirmation_sent) else - params.require(:user).permit(:email, :phone, :name, :password, :password_confirmation, :initial_confirmation_sent) + params.require(:user).permit(:email, :phone, :phone_extension, :name, :password, :password_confirmation, :initial_confirmation_sent) end elsif current_user.data_coordinator? - params.require(:user).permit(:email, :phone, :name, :role, :is_dpo, :is_key_contact, :active, :initial_confirmation_sent) + params.require(:user).permit(:email, :phone, :phone_extension, :name, :role, :is_dpo, :is_key_contact, :active, :initial_confirmation_sent) elsif current_user.support? - params.require(:user).permit(:email, :phone, :name, :role, :is_dpo, :is_key_contact, :organisation_id, :active, :initial_confirmation_sent) + params.require(:user).permit(:email, :phone, :phone_extension, :name, :role, :is_dpo, :is_key_contact, :organisation_id, :active, :initial_confirmation_sent) end end diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb index 6ac2979c8..ad6984e07 100644 --- a/app/models/sales_log.rb +++ b/app/models/sales_log.rb @@ -542,4 +542,8 @@ class SalesLog < Log def address_search_given? address_line1_input.present? && postcode_full_input.present? end + + def is_resale? + resale == 1 + end end diff --git a/app/models/user.rb b/app/models/user.rb index c79ceb0d9..31956e54d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -19,6 +19,7 @@ class User < ApplicationRecord validates :password, presence: { if: :password_required? } validates :password, length: { within: Devise.password_length, allow_blank: true } validates :password, confirmation: { if: :password_required? } + validates :phone_extension, format: { with: /\A\d+\z/, allow_blank: true, message: I18n.t("validations.numeric.format", field: "") } after_validation :send_data_protection_confirmation_reminder, if: :is_dpo_changed? @@ -142,6 +143,7 @@ class User < ApplicationRecord sign_in_count: 0, initial_confirmation_sent: false, reactivate_with_organisation:, + unconfirmed_email: nil, ) end @@ -156,7 +158,6 @@ class User < ApplicationRecord RESET_PASSWORD_TEMPLATE_ID = "2c410c19-80a7-481c-a531-2bcb3264f8e6".freeze CONFIRMABLE_TEMPLATE_ID = "3fc2e3a7-0835-4b84-ab7a-ce51629eb614".freeze RECONFIRMABLE_TEMPLATE_ID = "bcdec787-f0a7-46e9-8d63-b3e0a06ee455".freeze - BETA_ONBOARDING_TEMPLATE_ID = "b48bc2cd-5887-4611-8296-d0ab3ed0e7fd".freeze USER_REACTIVATED_TEMPLATE_ID = "ac45a899-490e-4f59-ae8d-1256fc0001f9".freeze FOR_OLD_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID = "3eb80517-1051-4dfc-b4cc-cb18228a3829".freeze FOR_NEW_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID = "0cdd0be1-7fa5-4808-8225-ae4c5a002352".freeze @@ -168,8 +169,6 @@ class User < ApplicationRecord def confirmable_template if last_sign_in_at.present? && (unconfirmed_email.blank? || unconfirmed_email == email) USER_REACTIVATED_TEMPLATE_ID - elsif was_migrated_from_softwire? && last_sign_in_at.blank? - BETA_ONBOARDING_TEMPLATE_ID elsif initial_confirmation_sent && !confirmed? RECONFIRMABLE_TEMPLATE_ID else @@ -177,10 +176,6 @@ class User < ApplicationRecord end end - def was_migrated_from_softwire? - legacy_users.any? || old_user_id.present? - end - def send_confirmation_instructions return unless active? @@ -269,6 +264,12 @@ class User < ApplicationRecord save!(validate: false) end + def phone_with_extension + return phone if phone_extension.blank? + + "#{phone}, Ext. #{phone_extension}" + end + protected # Checks whether a password is needed or not. For validations only. diff --git a/app/models/validations/sales/financial_validations.rb b/app/models/validations/sales/financial_validations.rb index 3e1d3dfb8..9a119475f 100644 --- a/app/models/validations/sales/financial_validations.rb +++ b/app/models/validations/sales/financial_validations.rb @@ -96,9 +96,10 @@ module Validations::Sales::FinancialValidations if record.equity < range.min record.errors.add :type, I18n.t("validations.financial.equity.under_min", min_equity: range.min) record.errors.add :equity, :under_min, message: I18n.t("validations.financial.equity.under_min", min_equity: range.min) - elsif record.equity > range.max + elsif !record.is_resale? && record.equity > range.max record.errors.add :type, I18n.t("validations.financial.equity.over_max", max_equity: range.max) record.errors.add :equity, :over_max, message: I18n.t("validations.financial.equity.over_max", max_equity: range.max) + record.errors.add :resale, I18n.t("validations.financial.equity.over_max", max_equity: range.max) end end diff --git a/app/views/users/edit.html.erb b/app/views/users/edit.html.erb index 3f7259bc3..6fdfdb5aa 100644 --- a/app/views/users/edit.html.erb +++ b/app/views/users/edit.html.erb @@ -24,7 +24,12 @@ <%= f.govuk_phone_field :phone, label: { text: "Telephone number", size: "m" }, - autocomplete: "phone", + autocomplete: "tel-national", + spellcheck: "false" %> + + <%= f.govuk_phone_field :phone_extension, + label: { text: "Extension number (optional)", size: "m" }, + autocomplete: "tel-extension", spellcheck: "false" %> <% if current_user.data_coordinator? || current_user.support? %> diff --git a/app/views/users/new.html.erb b/app/views/users/new.html.erb index 925f1ada7..f9f9b1e48 100644 --- a/app/views/users/new.html.erb +++ b/app/views/users/new.html.erb @@ -25,10 +25,16 @@ <%= f.govuk_phone_field :phone, label: { text: "Telephone number", size: "m" }, - autocomplete: "phone", + autocomplete: "tel-national", spellcheck: "false", value: @user.phone %> + <%= f.govuk_phone_field :phone_extension, + label: { text: "Extension number (optional)", size: "m" }, + autocomplete: "tel-extension", + spellcheck: "false", + value: @user.phone_extension %> + <% if current_user.support? %> <% null_option = [OpenStruct.new(id: "", name: "Select an option")] %> <% organisations = Organisation.filter_by_active.map { |org| OpenStruct.new(id: org.id, name: org.name) } %> diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index acb53d51d..df8c0e915 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -43,7 +43,7 @@ <%= summary_list.with_row do |row| row.with_key { "Telephone number" } - row.with_value { @user.phone } + row.with_value { @user.phone_with_extension } if UserPolicy.new(current_user, @user).edit_telephone_numbers? row.with_action(visually_hidden_text: "telephone number", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-telephone-number" }) else diff --git a/db/migrate/20240819143150_add_phone_extension_to_users.rb b/db/migrate/20240819143150_add_phone_extension_to_users.rb new file mode 100644 index 000000000..0dcd8d0c5 --- /dev/null +++ b/db/migrate/20240819143150_add_phone_extension_to_users.rb @@ -0,0 +1,5 @@ +class AddPhoneExtensionToUsers < ActiveRecord::Migration[7.0] + def change + add_column :users, :phone_extension, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 57df3eb3f..cb4a44e44 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: 2024_08_19_100411) do +ActiveRecord::Schema[7.0].define(version: 2024_08_19_143150) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -792,6 +792,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_08_19_100411) do t.boolean "initial_confirmation_sent" t.boolean "reactivate_with_organisation" t.datetime "discarded_at" + t.string "phone_extension" t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true t.index ["email"], name: "index_users_on_email", unique: true t.index ["encrypted_otp_secret_key"], name: "index_users_on_encrypted_otp_secret_key", unique: true diff --git a/lib/tasks/clear_unconfirmed_emails.rake b/lib/tasks/clear_unconfirmed_emails.rake new file mode 100644 index 000000000..f53a0c893 --- /dev/null +++ b/lib/tasks/clear_unconfirmed_emails.rake @@ -0,0 +1,4 @@ +desc "Clear unconfimed emails for deactivated users" +task clear_unconfirmed_emails: :environment do + User.deactivated.where.not(unconfirmed_email: nil).update(unconfirmed_email: nil) +end diff --git a/spec/features/user_spec.rb b/spec/features/user_spec.rb index c30abe1e9..119dbfa52 100644 --- a/spec/features/user_spec.rb +++ b/spec/features/user_spec.rb @@ -600,8 +600,8 @@ RSpec.describe "User Features" do visit(user_path(other_user)) end - it "sends beta onboarding email to be sent when user is legacy" do - expect(notify_client).to receive(:send_email).with(email_address: "new_user@example.com", template_id: User::BETA_ONBOARDING_TEMPLATE_ID, personalisation:).once + it "sends initial confirmable template email when user is legacy" do + expect(notify_client).to receive(:send_email).with(email_address: "new_user@example.com", template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation:).once click_button("Resend invite link") end end diff --git a/spec/lib/tasks/clear_unconfirmed_emails_spec.rb b/spec/lib/tasks/clear_unconfirmed_emails_spec.rb new file mode 100644 index 000000000..0ff7a3315 --- /dev/null +++ b/spec/lib/tasks/clear_unconfirmed_emails_spec.rb @@ -0,0 +1,36 @@ +require "rails_helper" +require "rake" + +RSpec.describe "clear_unconfirmed_emails" do + describe ":clear_unconfirmed_emails", type: :task do + subject(:task) { Rake::Task["clear_unconfirmed_emails"] } + + before do + Rake.application.rake_require("tasks/clear_unconfirmed_emails") + Rake::Task.define_task(:environment) + task.reenable + end + + context "when the rake task is run" do + context "and there are deactivated users with unconfirmed emails" do + let!(:user) { create(:user, active: false, unconfirmed_email: "some_email@example.com") } + + it "clears unconfirmed_email" do + task.invoke + + expect(user.reload.unconfirmed_email).to eq(nil) + end + end + + context "and there are active users with unconfirmed emails" do + let!(:user) { create(:user, active: true, unconfirmed_email: "some_email@example.com") } + + it "does not clear unconfirmed_email" do + task.invoke + + expect(user.reload.unconfirmed_email).not_to eq(nil) + end + end + end + end +end diff --git a/spec/mailers/resend_invitation_mailer_spec.rb b/spec/mailers/resend_invitation_mailer_spec.rb index 2b76484c3..9dec88ace 100644 --- a/spec/mailers/resend_invitation_mailer_spec.rb +++ b/spec/mailers/resend_invitation_mailer_spec.rb @@ -42,7 +42,7 @@ RSpec.describe ResendInvitationMailer do it "sends an initial invitation" do FactoryBot.create(:legacy_user, old_user_id: new_active_migrated_user.old_user_id, user: new_active_migrated_user) - expect(notify_client).to receive(:send_email).with(email_address: "new_active_migrated_user@example.com", template_id: User::BETA_ONBOARDING_TEMPLATE_ID, personalisation:).once + expect(notify_client).to receive(:send_email).with(email_address: "new_active_migrated_user@example.com", template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation:).once described_class.new.resend_invitation_email(new_active_migrated_user) end end diff --git a/spec/models/validations/sales/financial_validations_spec.rb b/spec/models/validations/sales/financial_validations_spec.rb index dd71d0b24..d9f47d39a 100644 --- a/spec/models/validations/sales/financial_validations_spec.rb +++ b/spec/models/validations/sales/financial_validations_spec.rb @@ -338,7 +338,7 @@ RSpec.describe Validations::Sales::FinancialValidations do end describe "#validate_equity_in_range_for_year_and_type" do - let(:record) { FactoryBot.build(:sales_log, saledate:) } + let(:record) { FactoryBot.build(:sales_log, saledate:, resale: nil) } context "with a log in the 22/23 collection year" do let(:saledate) { Time.zone.local(2023, 1, 1) } @@ -373,6 +373,14 @@ RSpec.describe Validations::Sales::FinancialValidations do expect(record.errors["equity"]).to include(match I18n.t("validations.financial.equity.over_max", max_equity: 75)) expect(record.errors["type"]).to include(match I18n.t("validations.financial.equity.over_max", max_equity: 75)) end + + it "does not add an error if it's a resale" do + record.type = 2 + record.equity = 90 + record.resale = 1 + financial_validator.validate_equity_in_range_for_year_and_type(record) + expect(record.errors).to be_empty + end end context "with a log in 23/24 collection year" do diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index 64955f702..91dc07094 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -1701,7 +1701,7 @@ RSpec.describe OrganisationsController, type: :request do end it "sends invitation emails" do - expect(notify_client).to have_received(:send_email).with(email_address: user_to_reactivate.email, template_id: User::BETA_ONBOARDING_TEMPLATE_ID, personalisation: expected_personalisation).once + expect(notify_client).to have_received(:send_email).with(email_address: user_to_reactivate.email, template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation: expected_personalisation).once end end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 8e87f7f28..dac2806a5 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -434,7 +434,7 @@ RSpec.describe UsersController, type: :request do context "when user is signed in as a data coordinator" do let(:user) { create(:user, :data_coordinator, email: "coordinator@example.com", organisation: create(:organisation, :without_dpc)) } - let!(:other_user) { create(:user, organisation: user.organisation, name: "filter name", email: "filter@example.com") } + let!(:other_user) { create(:user, organisation: user.organisation, name: "filter name", email: "filter@example.com", unconfirmed_email: "email@something.com") } before do sign_in user @@ -885,6 +885,11 @@ RSpec.describe UsersController, type: :request do expect { patch "/users/#{other_user.id}", headers:, params: } .to change { other_user.reload.active }.from(true).to(false) end + + it "discards unconfirmed email" do + expect { patch "/users/#{other_user.id}", headers:, params: } + .to change { other_user.reload.unconfirmed_email }.from("email@something.com").to(nil) + end end context "and tries to activate deactivated user" do @@ -997,6 +1002,7 @@ RSpec.describe UsersController, type: :request do email: "new_user@example.com", role: "data_coordinator", phone: "12345678910", + phone_extension: "1234", }, } end @@ -1020,12 +1026,14 @@ RSpec.describe UsersController, type: :request do request end - it "creates a new scheme for user organisation with valid params" do + it "creates a new user for user organisation with valid params" do request expect(User.last.name).to eq("new user") expect(User.last.email).to eq("new_user@example.com") expect(User.last.role).to eq("data_coordinator") + expect(User.last.phone).to eq("12345678910") + expect(User.last.phone_extension).to eq("1234") end it "redirects back to organisation users page" do @@ -1612,11 +1620,12 @@ RSpec.describe UsersController, type: :request do expect(page).to have_content("Change your personal details") end - it "has fields for name, email, role, dpo and key contact" do + it "has fields for name, email, role, phone number and phone extension" do expect(page).to have_field("user[name]") expect(page).to have_field("user[email]") expect(page).to have_field("user[role]") expect(page).to have_field("user[phone]") + expect(page).to have_field("user[phone_extension]") end it "allows setting the role to `support`" do @@ -1638,10 +1647,12 @@ RSpec.describe UsersController, type: :request do expect(page).to have_content("Change #{other_user.name}’s personal details") end - it "has fields for name, email, role, dpo and key contact" do + it "has fields for name, email, role, phone number and phone extension" do expect(page).to have_field("user[name]") expect(page).to have_field("user[email]") expect(page).to have_field("user[role]") + expect(page).to have_field("user[phone]") + expect(page).to have_field("user[phone_extension]") end end @@ -1656,10 +1667,12 @@ RSpec.describe UsersController, type: :request do expect(page).to have_content("Change #{other_user.name}’s personal details") end - it "has fields for name, email, role, dpo and key contact" do + it "has fields for name, email, role, phone number and phone extension" do expect(page).to have_field("user[name]") expect(page).to have_field("user[email]") expect(page).to have_field("user[role]") + expect(page).to have_field("user[phone]") + expect(page).to have_field("user[phone_extension]") end end @@ -1989,6 +2002,7 @@ RSpec.describe UsersController, type: :request do email:, role: "data_coordinator", phone: "12345612456", + phone_extension: "1234", organisation_id: organisation.id, }, } @@ -2004,6 +2018,14 @@ RSpec.describe UsersController, type: :request do expect(User.find_by(email:).organisation).to eq(organisation) end + it "sets expected values on the user" do + request + user = User.find_by(email:) + expect(user.name).to eq("new user") + expect(user.phone).to eq("12345612456") + expect(user.phone_extension).to eq("1234") + end + it "redirects back to users page" do request expect(response).to redirect_to("/users")