From 2e15707feb348a2cffa5f40d58cac5e740c9ffca Mon Sep 17 00:00:00 2001 From: meyric Date: Thu, 12 Dec 2024 14:42:19 +0000 Subject: [PATCH] Update for deactivated_at We recently dropped the `active` column and want to retain the behaviour using the `deactivated_at` value: - if deactivated_at is nil the user is active - if deactivated_at is present the user has been deactivated This works creates a `active` method on User and a `active?` alias, to replicate the old behaviour. We update the two scopes so they work based on deactivated_at. Finally, we adjust the specs setup to make sure we are setting a deactivated_at date not the old active value. At this point the application is working but the tests will fail and an attempt to activate or deactivate a user will fail, we'll fix that next. --- app/models/user.rb | 9 +++-- spec/factories/user.rb | 5 +-- spec/features/users_can_sign_in_spec.rb | 4 +-- spec/mailers/report_mailer_spec.rb | 12 +++---- spec/models/user_spec.rb | 36 +++++++++++++++---- .../report/send_state_change_emails_spec.rb | 4 +-- 6 files changed, 50 insertions(+), 20 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 0e73e2c58..9431adf56 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -14,11 +14,16 @@ class User < ApplicationRecord organisation_id: :organisation }.freeze - scope :active, -> { where(active: true) } - scope :inactive, -> { where(active: false) } + scope :active, -> { where(deactivated_at: nil) } + scope :inactive, -> { where.not(deactivated_at: nil) } delegate :service_owner?, :partner_organisation?, to: :organisation + def active + deactivated_at.blank? + end + alias_method :active?, :active + def organisation if Current.user_organisation return Organisation.find(Current.user_organisation) diff --git a/spec/factories/user.rb b/spec/factories/user.rb index bfa314425..4d886e369 100644 --- a/spec/factories/user.rb +++ b/spec/factories/user.rb @@ -2,7 +2,8 @@ factory :administrator, class: "User" do name { Faker::Name.name } email { Faker::Internet.email } - active { true } + deactivated_at { nil } + anonymised_at { nil } password { "Ab1!#{SecureRandom.uuid}" } mobile_number { Faker::PhoneNumber.phone_number } mobile_number_confirmed_at { 1.day.ago } @@ -19,7 +20,7 @@ end factory :inactive_user do - active { false } + deactivated_at { Date.yesterday } end trait :new_user do diff --git a/spec/features/users_can_sign_in_spec.rb b/spec/features/users_can_sign_in_spec.rb index 02732e82d..b00ef593b 100644 --- a/spec/features/users_can_sign_in_spec.rb +++ b/spec/features/users_can_sign_in_spec.rb @@ -352,7 +352,7 @@ def log_in_via_form(user, remember_me: false) context "when the user has been deactivated" do scenario "the user cannot log in and sees an informative message" do - user = create(:partner_organisation_user, active: false) + user = create(:partner_organisation_user, deactivated_at: DateTime.yesterday) visit root_path log_in_via_form(user) @@ -368,7 +368,7 @@ def log_in_via_form(user, remember_me: false) expect(page.current_path).to eql home_path - user.active = false + user.deactivated_at = DateTime.now user.save visit home_path diff --git a/spec/mailers/report_mailer_spec.rb b/spec/mailers/report_mailer_spec.rb index 057839b50..a4b5d427d 100644 --- a/spec/mailers/report_mailer_spec.rb +++ b/spec/mailers/report_mailer_spec.rb @@ -25,7 +25,7 @@ context "when the user is inactive" do before do - user.update!(active: false) + user.update!(deactivated_at: DateTime.yesterday) end it "should raise an error" do @@ -136,7 +136,7 @@ end context "when the user is inactive" do - let(:user) { create(:administrator, organisation: organisation, active: false) } + let(:user) { create(:administrator, organisation: organisation, deactivated_at: DateTime.yesterday) } it "should raise an error" do expect { mail.body }.to raise_error(ArgumentError, "User must be active to receive report-related emails") @@ -166,7 +166,7 @@ end context "when the user is inactive" do - before { user.update!(active: false) } + before { user.update!(deactivated_at: DateTime.yesterday) } it "should raise an error" do expect { mail.body }.to raise_error(ArgumentError, "User must be active to receive report-related emails") @@ -239,7 +239,7 @@ context "when the user is inactive" do before do - user.update!(active: false) + user.update!(deactivated_at: DateTime.yesterday) end it "should raise an error" do @@ -317,7 +317,7 @@ context "when the user is inactive" do before do - user.update!(active: false) + user.update!(deactivated_at: DateTime.yesterday) end it "should raise an error" do @@ -379,7 +379,7 @@ context "when the user is inactive" do before do - user.update!(active: false) + user.update!(deactivated_at: DateTime.yesterday) end it "should raise an error" do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 00333f501..7ea40ac71 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -104,18 +104,18 @@ describe "scopes" do it "shows active users for active scope" do - create(:administrator, active: true) - create(:administrator, active: true) - create(:administrator, active: false) + create(:administrator, deactivated_at: nil) + create(:administrator, deactivated_at: nil) + create(:administrator, deactivated_at: DateTime.yesterday) expect(User.active.size).to eq(2) expect(User.active.last.active).to eq(true) end it "shows inactive users for inactive scope" do - create(:administrator, active: true) - create(:administrator, active: true) - create(:administrator, active: false) + create(:administrator, deactivated_at: nil) + create(:administrator, deactivated_at: nil) + create(:administrator, deactivated_at: DateTime.yesterday) expect(User.inactive.size).to eq(1) expect(User.inactive.last.active).to eq(false) @@ -148,4 +148,28 @@ expect(user.valid?).to be_truthy end end + + describe "#active" do + it "is aliased as active?" do + user = build(:partner_organisation_user) + + expect(user.active?).to be true + end + + context "when the user has no deactivated date" do + it "is active" do + user = build(:partner_organisation_user, deactivated_at: nil) + + expect(user.active?).to be true + end + end + + context "when the user has a deactivated date" do + it "is not active" do + user = build(:partner_organisation_user, deactivated_at: DateTime.yesterday) + + expect(user.active?).to be false + end + end + end end diff --git a/spec/services/report/send_state_change_emails_spec.rb b/spec/services/report/send_state_change_emails_spec.rb index a67dccf76..5822e159a 100644 --- a/spec/services/report/send_state_change_emails_spec.rb +++ b/spec/services/report/send_state_change_emails_spec.rb @@ -5,9 +5,9 @@ let!(:report) { create(:report, state: state) } let!(:partner_organisation_users) { create_list(:administrator, 5, organisation: report.organisation) } - let!(:inactive_po_user) { create(:administrator, organisation: report.organisation, active: false) } + let!(:inactive_po_user) { create(:administrator, organisation: report.organisation, deactivated_at: DateTime.yesterday) } let!(:service_owners) { create_list(:beis_user, 2) } - let!(:inactive_service_owner) { create(:beis_user, active: false) } + let!(:inactive_service_owner) { create(:beis_user, deactivated_at: DateTime.yesterday) } let(:recipients) { ActionMailer::Base.deliveries.map { |delivery| delivery.to }.flatten }