diff --git a/Gemfile b/Gemfile index 019e0245a..590a0a28c 100644 --- a/Gemfile +++ b/Gemfile @@ -23,8 +23,6 @@ gem "govuk_design_system_formbuilder" gem "notifications-ruby-client" # Turbo and Stimulus gem "hotwire-rails" -# Soft delete ActiveRecords objects -gem "discard" # Administration framework gem "activeadmin", git: "https://github.com/tagliala/activeadmin.git", branch: "feature/railties-7" # Admin charts @@ -47,6 +45,8 @@ gem "postcodes_io" gem "view_component" # Use the AWS S3 SDK as storage mechanism gem "aws-sdk-s3" +# Track changes to models for auditing or versioning. +gem "paper_trail" group :development, :test do # Call 'byebug' anywhere in the code to stop execution and get a debugger console diff --git a/Gemfile.lock b/Gemfile.lock index ae61f693c..59ca0abf3 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -156,8 +156,6 @@ GEM deep_merge (1.2.2) diff-lcs (1.5.0) digest (3.1.0) - discard (1.2.1) - activerecord (>= 4.2, < 8) docile (1.4.0) dotenv (2.7.6) dotenv-rails (2.7.6) @@ -266,6 +264,9 @@ GEM childprocess (>= 0.6.3, < 5) iniparse (~> 1.4) rexml (~> 3.2) + paper_trail (12.2.0) + activerecord (>= 5.2) + request_store (~> 1.1) parallel (1.21.0) parser (3.1.0.0) ast (~> 2.4.1) @@ -326,6 +327,8 @@ GEM rb-inotify (0.10.1) ffi (~> 1.0) regexp_parser (2.2.0) + request_store (1.5.1) + rack (>= 1.4) responders (3.0.1) actionpack (>= 5.0) railties (>= 5.0) @@ -456,7 +459,6 @@ DEPENDENCIES capybara-lockstep chartkick devise! - discard dotenv-rails factory_bot_rails govuk-components @@ -466,6 +468,7 @@ DEPENDENCIES listen (~> 3.3) notifications-ruby-client overcommit (>= 0.37.0) + paper_trail pg (~> 1.1) postcodes_io pry-byebug diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb index e6a3a3ecd..8d6f75c5c 100644 --- a/app/controllers/case_logs_controller.rb +++ b/app/controllers/case_logs_controller.rb @@ -60,7 +60,7 @@ class CaseLogsController < ApplicationController def destroy if @case_log - if @case_log.discard + if @case_log.delete head :no_content else render json: { errors: @case_log.errors.messages }, status: :unprocessable_entity diff --git a/app/models/admin_user.rb b/app/models/admin_user.rb index 7fc666a7e..351bf834d 100644 --- a/app/models/admin_user.rb +++ b/app/models/admin_user.rb @@ -6,6 +6,8 @@ class AdminUser < ApplicationRecord has_one_time_password(encrypted: true) + has_paper_trail + validates :phone, presence: true, numericality: true MFA_SMS_TEMPLATE_ID = "bf309d93-804e-4f95-b1f4-bd513c48ecb0".freeze diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 0261f7d6a..a4c331b4b 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -30,11 +30,11 @@ private end class CaseLog < ApplicationRecord - include Discard::Model include Validations::SoftValidations include Constants::CaseLog include Constants::IncomeRanges - default_scope -> { kept } + + has_paper_trail validates_with CaseLogValidator before_validation :process_postcode_changes!, if: :property_postcode_changed? diff --git a/app/models/organisation.rb b/app/models/organisation.rb index 6d31f8a94..ff85f55a8 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -3,7 +3,10 @@ class Organisation < ApplicationRecord has_many :owned_case_logs, class_name: "CaseLog", foreign_key: "owning_organisation_id" has_many :managed_case_logs, class_name: "CaseLog", foreign_key: "managing_organisation_id" + has_paper_trail + include Constants::Organisation + enum provider_type: PROVIDER_TYPE def case_logs diff --git a/app/models/user.rb b/app/models/user.rb index 8e18a55db..217b0a1ac 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -10,6 +10,8 @@ class User < ApplicationRecord has_many :owned_case_logs, through: :organisation has_many :managed_case_logs, through: :organisation + has_paper_trail + enum role: ROLES def case_logs diff --git a/db/migrate/20220207151239_create_versions.rb b/db/migrate/20220207151239_create_versions.rb new file mode 100644 index 000000000..2d69235bb --- /dev/null +++ b/db/migrate/20220207151239_create_versions.rb @@ -0,0 +1,22 @@ +# This migration creates the `versions` table, the only schema PT requires. +# All other migrations PT provides are optional. +class CreateVersions < ActiveRecord::Migration[7.0] + # The largest text column available in all supported RDBMS is + # 1024^3 - 1 bytes, roughly one gibibyte. We specify a size + # so that MySQL will use `longtext` instead of `text`. Otherwise, + # when serializing very large objects, `text` might not be big enough. + TEXT_BYTES = 1_073_741_823 + + def change + create_table :versions do |t| + t.string :item_type, null: false, limit: 191 + t.bigint :item_id, null: false + t.string :event, null: false + t.string :whodunnit + t.text :object, limit: TEXT_BYTES + + t.datetime :created_at + end + add_index :versions, %i[item_type item_id] + end +end diff --git a/db/migrate/20220207151312_remove_discarded_at_from_case_logs.rb b/db/migrate/20220207151312_remove_discarded_at_from_case_logs.rb new file mode 100644 index 000000000..00886f520 --- /dev/null +++ b/db/migrate/20220207151312_remove_discarded_at_from_case_logs.rb @@ -0,0 +1,11 @@ +class RemoveDiscardedAtFromCaseLogs < ActiveRecord::Migration[7.0] + def up + remove_index :case_logs, :discarded_at + remove_column :case_logs, :discarded_at + end + + def down + add_column :case_logs, :discarded_at, :datetime + add_index :case_logs, :discarded_at + end +end diff --git a/db/schema.rb b/db/schema.rb index 9646f5029..891c11299 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -93,6 +93,7 @@ ActiveRecord::Schema.define(version: 202202071123100) do t.integer "beds" t.integer "offered" t.integer "wchair" + t.integer "earnings" t.integer "incfreq" t.integer "benefits" t.integer "period" @@ -127,7 +128,6 @@ ActiveRecord::Schema.define(version: 202202071123100) do t.integer "rp_medwel" t.integer "rp_hardship" t.integer "rp_dontknow" - t.datetime "discarded_at" t.string "tenancyother" t.integer "override_net_income_validation" t.string "property_owner_organisation" @@ -182,7 +182,6 @@ ActiveRecord::Schema.define(version: 202202071123100) do t.integer "is_carehome" t.integer "letting_in_sheltered_accomodation" t.integer "household_charge" - t.integer "earnings" t.integer "referral" t.decimal "brent", precision: 10, scale: 2 t.decimal "scharge", precision: 10, scale: 2 @@ -192,7 +191,6 @@ ActiveRecord::Schema.define(version: 202202071123100) do t.decimal "tshortfall", precision: 10, scale: 2 t.decimal "chcharge", precision: 10, scale: 2 t.integer "declaration" - t.index ["discarded_at"], name: "index_case_logs_on_discarded_at" t.index ["managing_organisation_id"], name: "index_case_logs_on_managing_organisation_id" t.index ["owning_organisation_id"], name: "index_case_logs_on_owning_organisation_id" end @@ -252,4 +250,14 @@ ActiveRecord::Schema.define(version: 202202071123100) do t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true end + create_table "versions", force: :cascade do |t| + t.string "item_type", limit: 191, null: false + t.bigint "item_id", null: false + t.string "event", null: false + t.string "whodunnit" + t.text "object" + t.datetime "created_at", precision: 6 + t.index ["item_type", "item_id"], name: "index_versions_on_item_type_and_item_id" + end + end diff --git a/spec/factories/case_log.rb b/spec/factories/case_log.rb index 71940ab99..4e59c1656 100644 --- a/spec/factories/case_log.rb +++ b/spec/factories/case_log.rb @@ -108,7 +108,6 @@ FactoryBot.define do rp_medwel { "No" } rp_hardship { "No" } rp_dontknow { "No" } - discarded_at { nil } tenancyother { nil } override_net_income_validation { nil } net_income_known { "Yes" } diff --git a/spec/models/admin_user_spec.rb b/spec/models/admin_user_spec.rb index a112bfb5b..ca25d39b8 100644 --- a/spec/models/admin_user_spec.rb +++ b/spec/models/admin_user_spec.rb @@ -20,33 +20,46 @@ RSpec.describe AdminUser, type: :model do ) }.to raise_error(ActiveRecord::RecordInvalid) end - end - it "requires an email" do - expect { - described_class.create!( - password: "password123", - phone: "075752137", - ) - }.to raise_error(ActiveRecord::RecordInvalid) - end + it "requires an email" do + expect { + described_class.create!( + password: "password123", + phone: "075752137", + ) + }.to raise_error(ActiveRecord::RecordInvalid) + end - it "requires a password" do - expect { - described_class.create!( - email: "admin_test@example.com", - phone: "075752137", - ) - }.to raise_error(ActiveRecord::RecordInvalid) + it "requires a password" do + expect { + described_class.create!( + email: "admin_test@example.com", + phone: "075752137", + ) + }.to raise_error(ActiveRecord::RecordInvalid) + end + + it "can be created" do + expect { + described_class.create!( + email: "admin_test@example.com", + password: "password123", + phone: "075752137", + ) + }.to change(described_class, :count).by(1) + end end - it "can be created" do - expect { - described_class.create!( - email: "admin_test@example.com", - password: "password123", - phone: "075752137", - ) - }.to change(described_class, :count).by(1) + describe "paper trail" do + let(:admin_user) { FactoryBot.create(:admin_user) } + + it "creates a record of changes to a log" do + expect { admin_user.update!(phone: "09673867853") }.to change(admin_user.versions, :count).by(1) + end + + it "allows case logs to be restored to a previous version" do + admin_user.update!(phone: "09673867853") + expect(admin_user.paper_trail.previous_version.phone).to eq("07563867654") + end end end diff --git a/spec/models/case_log_spec.rb b/spec/models/case_log_spec.rb index 8b704cf0e..6fb4f43be 100644 --- a/spec/models/case_log_spec.rb +++ b/spec/models/case_log_spec.rb @@ -1172,4 +1172,17 @@ RSpec.describe CaseLog do end end end + + describe "paper trail" do + let(:case_log) { FactoryBot.create(:case_log, :in_progress) } + + it "creates a record of changes to a log" do + expect { case_log.update!(age1: 64) }.to change(case_log.versions, :count).by(1) + end + + it "allows case logs to be restored to a previous version" do + case_log.update!(age1: 63) + expect(case_log.paper_trail.previous_version.age1).to eq(17) + end + end end diff --git a/spec/models/organisation_spec.rb b/spec/models/organisation_spec.rb index a1b4f011a..a8e2c32d3 100644 --- a/spec/models/organisation_spec.rb +++ b/spec/models/organisation_spec.rb @@ -54,4 +54,17 @@ RSpec.describe Organisation, type: :model do end end end + + describe "paper trail" do + let(:organisation) { FactoryBot.create(:organisation) } + + it "creates a record of changes to a log" do + expect { organisation.update!(name: "new test name") }.to change(organisation.versions, :count).by(1) + end + + it "allows case logs to be restored to a previous version" do + organisation.update!(name: "new test name") + expect(organisation.paper_trail.previous_version.name).to eq("DLUHC") + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index f24280a5c..a84c2343f 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -52,4 +52,17 @@ RSpec.describe User, type: :model do expect(user.data_coordinator?).to be false end end + + describe "paper trail" do + let(:user) { FactoryBot.create(:user) } + + it "creates a record of changes to a log" do + expect { user.update!(name: "new test name") }.to change(user.versions, :count).by(1) + end + + it "allows case logs to be restored to a previous version" do + user.update!(name: "new test name") + expect(user.paper_trail.previous_version.name).to eq("Danny Rojas") + end + end end diff --git a/spec/requests/case_logs_controller_spec.rb b/spec/requests/case_logs_controller_spec.rb index 09f14089e..de33846cf 100644 --- a/spec/requests/case_logs_controller_spec.rb +++ b/spec/requests/case_logs_controller_spec.rb @@ -401,9 +401,8 @@ RSpec.describe CaseLogsController, type: :request do expect(response).to have_http_status(:success) end - it "soft deletes the case log" do + it "deletes the case log" do expect { CaseLog.find(id) }.to raise_error(ActiveRecord::RecordNotFound) - expect(CaseLog.with_discarded.find(id)).to be_a(CaseLog) end context "with an invalid case log id" do @@ -428,7 +427,7 @@ RSpec.describe CaseLogsController, type: :request do context "when a case log deletion fails" do before do allow(CaseLog).to receive(:find_by).and_return(case_log) - allow(case_log).to receive(:discard).and_return(false) + allow(case_log).to receive(:delete).and_return(false) delete "/logs/#{id}", headers: headers end