Browse Source

CLDC-961: Model data versioning (#277)

* Replace Discard with Paper Trail

* Track who created or changed records via UI

* Track model updates by AdminUsers
pull/286/head
baarkerlounger 3 years ago committed by GitHub
parent
commit
8418e79426
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 6
      Gemfile
  2. 13
      Gemfile.lock
  3. 15
      app/concerns/admin/paper_trail.rb
  4. 8
      app/controllers/application_controller.rb
  5. 2
      app/controllers/case_logs_controller.rb
  6. 2
      app/models/admin_user.rb
  7. 4
      app/models/case_log.rb
  8. 3
      app/models/organisation.rb
  9. 2
      app/models/user.rb
  10. 4
      config/initializers/active_admin.rb
  11. 22
      db/migrate/20220207151239_create_versions.rb
  12. 11
      db/migrate/20220207151312_remove_discarded_at_from_case_logs.rb
  13. 14
      db/schema.rb
  14. 26
      spec/controllers/admin/admin_users_controller_spec.rb
  15. 48
      spec/controllers/admin/case_logs_controller_spec.rb
  16. 4
      spec/controllers/admin/dashboard_controller_spec.rb
  17. 40
      spec/controllers/admin/organisations_controller_spec.rb
  18. 24
      spec/controllers/admin/users_controller_spec.rb
  19. 1
      spec/factories/case_log.rb
  20. 15
      spec/models/admin_user_spec.rb
  21. 13
      spec/models/case_log_spec.rb
  22. 13
      spec/models/organisation_spec.rb
  23. 13
      spec/models/user_spec.rb
  24. 25
      spec/requests/case_logs_controller_spec.rb
  25. 7
      spec/requests/form_controller_spec.rb
  26. 7
      spec/requests/organisations_controller_spec.rb
  27. 7
      spec/requests/users_controller_spec.rb
  28. 8
      spec/support/controller_macros.rb

6
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

13
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

15
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

8
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

2
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

2
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

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

3
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

2
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

4
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

22
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

11
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

14
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

26
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

48
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

4
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

40
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
context "when viewing the edit form" do
before do
get :edit, session: valid_session, params: { id: organisation.id }
end
it "creates a new admin users" do
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
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

24
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

1
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" }

15
spec/models/admin_user_spec.rb

@ -20,7 +20,6 @@ RSpec.describe AdminUser, type: :model do
)
}.to raise_error(ActiveRecord::RecordInvalid)
end
end
it "requires an email" do
expect {
@ -50,3 +49,17 @@ RSpec.describe AdminUser, type: :model do
}.to change(described_class, :count).by(1)
end
end
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

13
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

13
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

13
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

25
spec/requests/case_logs_controller_spec.rb

@ -34,6 +34,7 @@ RSpec.describe CaseLogsController, type: :request do
let(:in_progress) { "in_progress" }
let(:completed) { "completed" }
context "when API" do
let(:params) do
{
"owning_organisation_id": owning_organisation.id,
@ -116,6 +117,25 @@ RSpec.describe CaseLogsController, type: :request do
end
end
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 "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
describe "GET" do
let(:user) { FactoryBot.create(:user) }
let(:organisation) { user.organisation }
@ -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

7
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

7
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

7
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

8
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

Loading…
Cancel
Save