From 8418e794263b274a0eb7ab03fbc92eacfa85381a Mon Sep 17 00:00:00 2001 From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com> Date: Wed, 9 Feb 2022 10:10:10 +0000 Subject: [PATCH] CLDC-961: Model data versioning (#277) * Replace Discard with Paper Trail * Track who created or changed records via UI * Track model updates by AdminUsers --- Gemfile | 6 +- Gemfile.lock | 13 +- app/concerns/admin/paper_trail.rb | 15 ++ app/controllers/application_controller.rb | 8 + app/controllers/case_logs_controller.rb | 2 +- app/models/admin_user.rb | 2 + app/models/case_log.rb | 4 +- app/models/organisation.rb | 3 + app/models/user.rb | 2 + config/initializers/active_admin.rb | 4 + db/migrate/20220207151239_create_versions.rb | 22 +++ ...1312_remove_discarded_at_from_case_logs.rb | 11 ++ db/schema.rb | 14 +- .../admin/admin_users_controller_spec.rb | 26 +++- .../admin/case_logs_controller_spec.rb | 48 +++++- .../admin/dashboard_controller_spec.rb | 4 +- .../admin/organisations_controller_spec.rb | 50 +++++- .../admin/users_controller_spec.rb | 24 ++- spec/factories/case_log.rb | 1 - spec/models/admin_user_spec.rb | 61 +++++--- spec/models/case_log_spec.rb | 13 ++ spec/models/organisation_spec.rb | 13 ++ spec/models/user_spec.rb | 13 ++ spec/requests/case_logs_controller_spec.rb | 145 ++++++++++-------- spec/requests/form_controller_spec.rb | 7 + .../requests/organisations_controller_spec.rb | 7 + spec/requests/users_controller_spec.rb | 7 + spec/support/controller_macros.rb | 8 - 28 files changed, 407 insertions(+), 126 deletions(-) create mode 100644 app/concerns/admin/paper_trail.rb create mode 100644 db/migrate/20220207151239_create_versions.rb create mode 100644 db/migrate/20220207151312_remove_discarded_at_from_case_logs.rb diff --git a/Gemfile b/Gemfile index 019e0245a..45832489b 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,10 @@ 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" +# Store active record objects in version whodunnits +gem "paper_trail-globalid" 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..937bb3f80 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,12 @@ GEM childprocess (>= 0.6.3, < 5) iniparse (~> 1.4) rexml (~> 3.2) + paper_trail (12.2.0) + activerecord (>= 5.2) + request_store (~> 1.1) + paper_trail-globalid (0.2.0) + globalid + paper_trail (>= 3.0.0) parallel (1.21.0) parser (3.1.0.0) ast (~> 2.4.1) @@ -326,6 +330,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 +462,6 @@ DEPENDENCIES capybara-lockstep chartkick devise! - discard dotenv-rails factory_bot_rails govuk-components @@ -466,6 +471,8 @@ DEPENDENCIES listen (~> 3.3) notifications-ruby-client overcommit (>= 0.37.0) + paper_trail + paper_trail-globalid pg (~> 1.1) postcodes_io pry-byebug diff --git a/app/concerns/admin/paper_trail.rb b/app/concerns/admin/paper_trail.rb new file mode 100644 index 000000000..5558d465b --- /dev/null +++ b/app/concerns/admin/paper_trail.rb @@ -0,0 +1,15 @@ +module Admin + module PaperTrail + extend ActiveSupport::Concern + + included do + before_action :set_paper_trail_whodunnit + end + + protected + + def user_for_paper_trail + current_admin_user + end + end +end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 776259fab..78b367d80 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,4 +1,6 @@ class ApplicationController < ActionController::Base + before_action :set_paper_trail_whodunnit + def render_not_found render "errors/not_found", status: :not_found end @@ -6,4 +8,10 @@ class ApplicationController < ActionController::Base def render_not_found_json(class_name, id) render json: { error: "#{class_name} #{id} not found" }, status: :not_found end + +protected + + def user_for_paper_trail + current_user + end end 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/config/initializers/active_admin.rb b/config/initializers/active_admin.rb index 47561059e..e8014c7e2 100644 --- a/config/initializers/active_admin.rb +++ b/config/initializers/active_admin.rb @@ -340,3 +340,7 @@ end Rails.application.config.after_initialize do ActiveAdmin.application.stylesheets.delete("active_admin/print.css") end + +Rails.application.config.after_initialize do + ActiveAdmin::BaseController.include Admin::PaperTrail +end 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/controllers/admin/admin_users_controller_spec.rb b/spec/controllers/admin/admin_users_controller_spec.rb index e5977d830..871a198c3 100644 --- a/spec/controllers/admin/admin_users_controller_spec.rb +++ b/spec/controllers/admin/admin_users_controller_spec.rb @@ -6,8 +6,11 @@ describe Admin::AdminUsersController, type: :controller do let(:page) { Capybara::Node::Simple.new(response.body) } let(:resource_title) { "Admin Users" } let(:valid_session) { {} } + let(:signed_in_admin_user) { FactoryBot.create(:admin_user) } - login_admin_user + before do + sign_in signed_in_admin_user + end describe "Get admin users" do before do @@ -27,22 +30,30 @@ describe Admin::AdminUsersController, type: :controller do it "creates a new admin user" do expect { post :create, session: valid_session, params: params }.to change(AdminUser, :count).by(1) end + + it "tracks who created the record" do + post :create, session: valid_session, params: params + created_id = response.location.match(/[0-9]+/)[0] + whodunnit_actor = AdminUser.find_by(id: created_id).versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(signed_in_admin_user.id) + end end describe "Update admin users" do - context "when editing the form" do + context "when viewing the form" do before do get :edit, session: valid_session, params: { id: AdminUser.first.id } end - it "shows an edit form" do + it "shows the correct fields" do expect(page).to have_field("admin_user_email") expect(page).to have_field("admin_user_password") expect(page).to have_field("admin_user_password_confirmation") end end - context "when updating the form" do + context "when updating an admin user" do let(:admin_user) { FactoryBot.create(:admin_user) } let(:email) { "new_email@example.com" } let(:params) { { id: admin_user.id, admin_user: { email: email } } } @@ -55,6 +66,13 @@ describe Admin::AdminUsersController, type: :controller do admin_user.reload expect(admin_user.email).to eq(email) end + + it "tracks who updated the record" do + admin_user.reload + whodunnit_actor = admin_user.versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(signed_in_admin_user.id) + end end end end diff --git a/spec/controllers/admin/case_logs_controller_spec.rb b/spec/controllers/admin/case_logs_controller_spec.rb index 7a27443a1..1ff62e77b 100644 --- a/spec/controllers/admin/case_logs_controller_spec.rb +++ b/spec/controllers/admin/case_logs_controller_spec.rb @@ -5,14 +5,14 @@ require_relative "../../request_helper" describe Admin::CaseLogsController, type: :controller do before do RequestHelper.stub_http_requests + sign_in admin_user end render_views let(:page) { Capybara::Node::Simple.new(response.body) } let(:resource_title) { "Logs" } let(:valid_session) { {} } - - login_admin_user + let(:admin_user) { FactoryBot.create(:admin_user) } describe "Get case logs" do let!(:case_log) { FactoryBot.create(:case_log, :in_progress) } @@ -44,5 +44,49 @@ describe Admin::CaseLogsController, type: :controller do it "creates a new case log" do expect { post :create, session: valid_session, params: params }.to change(CaseLog, :count).by(1) end + + it "tracks who created the record" do + post :create, session: valid_session, params: params + created_id = response.location.match(/[0-9]+/)[0] + whodunnit_actor = CaseLog.find_by(id: created_id).versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(admin_user.id) + end + end + + describe "Update case log" do + let!(:case_log) { FactoryBot.create(:case_log, :in_progress) } + + context "when viewing the edit form" do + before do + get :edit, session: valid_session, params: { id: case_log.id } + end + + it "has the correct fields" do + expect(page).to have_field("case_log_age1") + expect(page).to have_field("case_log_tenant_code") + end + end + + context "when updating the case_log" do + let(:tenant_code) { "New tenant code by Admin" } + let(:params) { { id: case_log.id, case_log: { tenant_code: tenant_code } } } + + before do + patch :update, session: valid_session, params: params + end + + it "updates the case log" do + case_log.reload + expect(case_log.tenant_code).to eq(tenant_code) + end + + it "tracks who updated the record" do + case_log.reload + whodunnit_actor = case_log.versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(admin_user.id) + end + end end end diff --git a/spec/controllers/admin/dashboard_controller_spec.rb b/spec/controllers/admin/dashboard_controller_spec.rb index f23255201..aeba5aacf 100644 --- a/spec/controllers/admin/dashboard_controller_spec.rb +++ b/spec/controllers/admin/dashboard_controller_spec.rb @@ -5,14 +5,14 @@ require_relative "../../request_helper" describe Admin::DashboardController, type: :controller do before do RequestHelper.stub_http_requests + sign_in admin_user end render_views let(:page) { Capybara::Node::Simple.new(response.body) } let(:resource_title) { "Dashboard" } let(:valid_session) { {} } - - login_admin_user + let(:admin_user) { FactoryBot.create(:admin_user) } describe "Get case logs" do before do diff --git a/spec/controllers/admin/organisations_controller_spec.rb b/spec/controllers/admin/organisations_controller_spec.rb index 16dedcb69..014d13e8c 100644 --- a/spec/controllers/admin/organisations_controller_spec.rb +++ b/spec/controllers/admin/organisations_controller_spec.rb @@ -7,8 +7,11 @@ describe Admin::OrganisationsController, type: :controller do let(:resource_title) { "Organisations" } let(:valid_session) { {} } let!(:organisation) { FactoryBot.create(:organisation) } + let!(:admin_user) { FactoryBot.create(:admin_user) } - login_admin_user + before do + sign_in admin_user + end describe "Organisations" do before do @@ -22,23 +25,54 @@ describe Admin::OrganisationsController, type: :controller do end end - describe "Create admin users" do + describe "Create organisation" do let(:params) { { organisation: { name: "DLUHC" } } } it "creates a organisation" do expect { post :create, session: valid_session, params: params }.to change(Organisation, :count).by(1) end + + it "tracks who created the record" do + post :create, session: valid_session, params: params + created_id = response.location.match(/[0-9]+/)[0] + whodunnit_actor = Organisation.find_by(id: created_id).versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(admin_user.id) + end end describe "Update organisation" do - before do - get :edit, session: valid_session, params: { id: organisation.id } + context "when viewing the edit form" do + before do + get :edit, session: valid_session, params: { id: organisation.id } + end + + it "has the correct fields" do + expect(page).to have_field("organisation_name") + expect(page).to have_field("organisation_provider_type") + expect(page).to have_field("organisation_phone") + end end - it "creates a new admin users" do - expect(page).to have_field("organisation_name") - expect(page).to have_field("organisation_provider_type") - expect(page).to have_field("organisation_phone") + context "when updating the organisation" do + let(:name) { "New Org Name by Admin" } + let(:params) { { id: organisation.id, organisation: { name: name } } } + + before do + patch :update, session: valid_session, params: params + end + + it "updates the organisation" do + organisation.reload + expect(organisation.name).to eq(name) + end + + it "tracks who updated the record" do + organisation.reload + whodunnit_actor = organisation.versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(admin_user.id) + end end end end diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 78d97405b..9a7f7e264 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -8,8 +8,11 @@ describe Admin::UsersController, type: :controller do let(:page) { Capybara::Node::Simple.new(response.body) } let(:resource_title) { "Users" } let(:valid_session) { {} } + let!(:admin_user) { FactoryBot.create(:admin_user) } - login_admin_user + before do + sign_in admin_user + end describe "Get users" do before do @@ -39,15 +42,23 @@ describe Admin::UsersController, type: :controller do it "creates a new user" do expect { post :create, session: valid_session, params: params }.to change(User, :count).by(1) end + + it "tracks who created the record" do + post :create, session: valid_session, params: params + created_id = response.location.match(/[0-9]+/)[0] + whodunnit_actor = User.find_by(id: created_id).versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(admin_user.id) + end end describe "Update users" do - context "when updating the form" do + context "when viewing the edit form" do before do get :edit, session: valid_session, params: { id: user.id } end - it "shows an edit form" do + it "has the correct fields" do expect(page).to have_field("user_email") expect(page).to have_field("user_name") expect(page).to have_field("user_organisation_id") @@ -69,6 +80,13 @@ describe Admin::UsersController, type: :controller do user.reload expect(user.name).to eq(name) end + + it "tracks who updated the record" do + user.reload + whodunnit_actor = user.versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(admin_user.id) + end end 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..8294af0b3 100644 --- a/spec/requests/case_logs_controller_spec.rb +++ b/spec/requests/case_logs_controller_spec.rb @@ -34,84 +34,104 @@ RSpec.describe CaseLogsController, type: :request do let(:in_progress) { "in_progress" } let(:completed) { "completed" } - let(:params) do - { - "owning_organisation_id": owning_organisation.id, - "managing_organisation_id": managing_organisation.id, - "tenant_code": tenant_code, - "age1": age1, - "property_postcode": property_postcode, - "offered": offered, - } - end - - before do - post "/logs", headers: headers, params: params.to_json - end - - it "returns http success" do - expect(response).to have_http_status(:success) - end - - it "returns a serialized Case Log" do - json_response = JSON.parse(response.body) - expect(json_response.keys).to match_array(CaseLog.new.attributes.keys) - end + context "when API" do + let(:params) do + { + "owning_organisation_id": owning_organisation.id, + "managing_organisation_id": managing_organisation.id, + "tenant_code": tenant_code, + "age1": age1, + "property_postcode": property_postcode, + "offered": offered, + } + end - it "creates a case log with the values passed" do - json_response = JSON.parse(response.body) - expect(json_response["tenant_code"]).to eq(tenant_code) - expect(json_response["age1"]).to eq(age1) - expect(json_response["property_postcode"]).to eq(property_postcode) - end + before do + post "/logs", headers: headers, params: params.to_json + end - context "with invalid json parameters" do - let(:age1) { 2000 } - let(:offered) { 21 } + it "returns http success" do + expect(response).to have_http_status(:success) + end - it "validates case log parameters" do + it "returns a serialized Case Log" do json_response = JSON.parse(response.body) - expect(response).to have_http_status(:unprocessable_entity) - expect(json_response["errors"]).to match_array([["offered", [I18n.t("validations.property.offered.relet_number")]], ["age1", [I18n.t("validations.household.age.must_be_valid", lower_bound: 16)]]]) + expect(json_response.keys).to match_array(CaseLog.new.attributes.keys) end - end - context "with a partial case log submission" do - it "marks the record as in_progress" do + it "creates a case log with the values passed" do json_response = JSON.parse(response.body) - expect(json_response["status"]).to eq(in_progress) + expect(json_response["tenant_code"]).to eq(tenant_code) + expect(json_response["age1"]).to eq(age1) + expect(json_response["property_postcode"]).to eq(property_postcode) end - end - context "with a complete case log submission" do - let(:org_params) do - { - "case_log" => { - "owning_organisation_id" => owning_organisation.id, - "managing_organisation_id" => managing_organisation.id, - }, - } + context "with invalid json parameters" do + let(:age1) { 2000 } + let(:offered) { 21 } + + it "validates case log parameters" do + json_response = JSON.parse(response.body) + expect(response).to have_http_status(:unprocessable_entity) + expect(json_response["errors"]).to match_array([["offered", [I18n.t("validations.property.offered.relet_number")]], ["age1", [I18n.t("validations.household.age.must_be_valid", lower_bound: 16)]]]) + end end - let(:case_log_params) { JSON.parse(File.open("spec/fixtures/complete_case_log.json").read) } - let(:params) do - case_log_params.merge(org_params) { |_k, a_val, b_val| a_val.merge(b_val) } + + context "with a partial case log submission" do + it "marks the record as in_progress" do + json_response = JSON.parse(response.body) + expect(json_response["status"]).to eq(in_progress) + end end - it "marks the record as completed" do - json_response = JSON.parse(response.body) + context "with a complete case log submission" do + let(:org_params) do + { + "case_log" => { + "owning_organisation_id" => owning_organisation.id, + "managing_organisation_id" => managing_organisation.id, + }, + } + end + let(:case_log_params) { JSON.parse(File.open("spec/fixtures/complete_case_log.json").read) } + let(:params) do + case_log_params.merge(org_params) { |_k, a_val, b_val| a_val.merge(b_val) } + end + + it "marks the record as completed" do + json_response = JSON.parse(response.body) + + expect(json_response).not_to have_key("errors") + expect(json_response["status"]).to eq(completed) + end + end + + context "with a request containing invalid credentials" do + let(:basic_credentials) do + ActionController::HttpAuthentication::Basic.encode_credentials(api_username, "Oops") + end - expect(json_response).not_to have_key("errors") - expect(json_response["status"]).to eq(completed) + it "returns 401" do + expect(response).to have_http_status(:unauthorized) + end end end - context "with a request containing invalid credentials" do - let(:basic_credentials) do - ActionController::HttpAuthentication::Basic.encode_credentials(api_username, "Oops") + context "when UI" do + let(:user) { FactoryBot.create(:user) } + let(:headers) { { "Accept" => "text/html" } } + + before do + RequestHelper.stub_http_requests + sign_in user + post "/logs", headers: headers end - it "returns 401" do - expect(response).to have_http_status(:unauthorized) + it "tracks who created the record" do + created_id = response.location.match(/[0-9]+/)[0] + whodunnit_actor = CaseLog.find_by(id: created_id).versions.last.actor + expect(whodunnit_actor).to be_a(User) + expect(whodunnit_actor.id).to eq(user.id) end end end @@ -401,9 +421,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 +447,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 diff --git a/spec/requests/form_controller_spec.rb b/spec/requests/form_controller_spec.rb index 3d77492b7..f4b49e129 100644 --- a/spec/requests/form_controller_spec.rb +++ b/spec/requests/form_controller_spec.rb @@ -155,6 +155,13 @@ RSpec.describe FormController, type: :request do expect(case_log.age1).to eq(answer) expect(case_log.age2).to be nil end + + it "tracks who updated the record" do + case_log.reload + whodunnit_actor = case_log.versions.last.actor + expect(whodunnit_actor).to be_a(User) + expect(whodunnit_actor.id).to eq(user.id) + end end end diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index 24ca82101..b92c8da53 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -185,6 +185,13 @@ RSpec.describe OrganisationsController, type: :request do follow_redirect! expect(page).to have_css(".govuk-notification-banner.govuk-notification-banner--success") end + + it "tracks who updated the record" do + organisation.reload + whodunnit_actor = organisation.versions.last.actor + expect(whodunnit_actor).to be_a(User) + expect(whodunnit_actor.id).to eq(user.id) + end end context "with an organisation that the user does not belong to" do diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 59997f3ce..66cbeb5c2 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -196,6 +196,13 @@ RSpec.describe UsersController, type: :request do user.reload expect(user.name).to eq(new_value) end + + it "tracks who updated the record" do + user.reload + whodunnit_actor = user.versions.last.actor + expect(whodunnit_actor).to be_a(User) + expect(whodunnit_actor.id).to eq(user.id) + end end context "when the update fails to persist" do diff --git a/spec/support/controller_macros.rb b/spec/support/controller_macros.rb index 2e21831dd..680663367 100644 --- a/spec/support/controller_macros.rb +++ b/spec/support/controller_macros.rb @@ -6,12 +6,4 @@ module ControllerMacros sign_in user end end - - def login_admin_user - before do - @request.env["devise.mapping"] = Devise.mappings[:admin_user] - admin_user = FactoryBot.create(:admin_user) - sign_in admin_user - end - end end