Merge branch 'main' into CLDC-3382-Support-user-functionality-merge-organisations

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)
+    fugit (1.11.1)
-      et-orbi (~> 1, >= 1.2.7)
+      et-orbi (~> 1, >= 1.2.11)
       raabro (~> 1.4)
     globalid (1.2.1)
       activesupport (>= 6.1)

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
 

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

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.

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
 

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? %>

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) } %>

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

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

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

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

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
+      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::BETA_ONBOARDING_TEMPLATE_ID, personalisation:).once
+        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

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

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

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

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

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