From ed223e0bd1f4230c6b0d1eede1fc1bb4146e6887 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 3 Nov 2019 00:34:06 +0100 Subject: [PATCH] Use audited to track investment changes Our manual implementation had a few issues. In particular, it didn't track changes related to associations, which became more of an issue when we made investments translatable. Using audited gives us more functionality while at the same time simplifies our code. However, it adds one more external dependency to our project. The reason for choosing audited over paper trail is audited seems to make it easier to handle associations. --- .rubocop.yml | 1 + Gemfile | 1 + Gemfile.lock | 3 ++ app/assets/stylesheets/admin.scss | 11 ----- .../admin/budget_investments_controller.rb | 13 ++---- app/controllers/application_controller.rb | 5 -- app/models/budget/investment.rb | 18 +------- app/models/budget/investment/change_log.rb | 11 ----- app/models/user.rb | 12 ----- app/views/admin/audits/_audits.html.erb | 46 +++++++++++++++++++ app/views/admin/audits/show.html.erb | 19 ++++++++ .../admin/budget_investments/show.html.erb | 2 +- .../admin/change_logs/_change_log.html.erb | 44 ------------------ app/views/admin/change_logs/show.html.erb | 12 ----- config/locales/en/admin.yml | 2 +- config/locales/es/admin.yml | 2 +- config/routes/admin.rb | 2 +- db/migrate/20191102002206_install_audited.rb | 26 +++++++++++ ...2238_drop_budget_investment_change_logs.rb | 13 ++++++ db/schema.rb | 34 +++++++++----- 20 files changed, 142 insertions(+), 135 deletions(-) delete mode 100644 app/models/budget/investment/change_log.rb create mode 100644 app/views/admin/audits/_audits.html.erb create mode 100644 app/views/admin/audits/show.html.erb delete mode 100644 app/views/admin/change_logs/_change_log.html.erb delete mode 100644 app/views/admin/change_logs/show.html.erb create mode 100644 db/migrate/20191102002206_install_audited.rb create mode 100644 db/migrate/20191102002238_drop_budget_investment_change_logs.rb diff --git a/.rubocop.yml b/.rubocop.yml index 255a9f392..03b0861e6 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -208,6 +208,7 @@ Rails/CreateTableWithTimestamps: Enabled: true Exclude: - "db/migrate/201[5-8]*" + - "db/migrate/*install_audited.rb" Rails/Date: Enabled: true diff --git a/Gemfile b/Gemfile index 9732bbfa1..0ef7a702f 100644 --- a/Gemfile +++ b/Gemfile @@ -6,6 +6,7 @@ gem "acts-as-taggable-on", "~> 5.0.0" gem "acts_as_votable", "~> 0.11.1" gem "ahoy_matey", "~> 1.6.0" gem "ancestry", "~> 3.0.7" +gem "audited", "~> 4.9.0" gem "autoprefixer-rails", "~> 8.2.0" gem "axlsx", "~> 3.0.0.pre" gem "axlsx_rails", "~> 0.5.2" diff --git a/Gemfile.lock b/Gemfile.lock index a265583a7..9fb2d77f8 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -72,6 +72,8 @@ GEM activerecord (>= 3.2.0) arel (7.1.4) ast (2.4.0) + audited (4.9.0) + activerecord (>= 4.2, < 6.1) autoprefixer-rails (8.2.0) execjs axlsx (3.0.0.pre) @@ -588,6 +590,7 @@ DEPENDENCIES acts_as_votable (~> 0.11.1) ahoy_matey (~> 1.6.0) ancestry (~> 3.0.7) + audited (~> 4.9.0) autoprefixer-rails (~> 8.2.0) axlsx (~> 3.0.0.pre) axlsx_rails (~> 0.5.2) diff --git a/app/assets/stylesheets/admin.scss b/app/assets/stylesheets/admin.scss index 55ab8193c..e56f6d68e 100644 --- a/app/assets/stylesheets/admin.scss +++ b/app/assets/stylesheets/admin.scss @@ -595,17 +595,6 @@ code { } } -.log-value { - max-height: rem-calc(65); - overflow: hidden; - max-width: rem-calc(200); - - &:hover { - max-height: rem-calc(1000); - transition: max-height 0.9s; - } -} - // 04. Stats // --------- diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index a9639ccb1..0f3ed3e6a 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -9,12 +9,11 @@ class Admin::BudgetInvestmentsController < Admin::BaseController has_orders %w[oldest], only: [:show, :edit] has_filters %w[all], only: [:index, :toggle_selection] - before_action :load_budget, except: :show_investment_log + before_action :load_budget, except: :audit before_action :load_investment, only: [:show, :edit, :update, :toggle_selection] before_action :load_ballot, only: [:show, :index] before_action :parse_valuation_filters before_action :load_investments, only: [:index, :toggle_selection] - before_action :load_change_log, only: [:show] def index load_tags @@ -61,9 +60,9 @@ class Admin::BudgetInvestmentsController < Admin::BaseController load_investments end - def show_investment_log - @log = Budget::Investment::ChangeLog.find_by(id: params[:id]) - render "admin/change_logs/show" + def audit + @audit = Audited::Audit.find(params[:id]) + render "admin/audits/show" end private @@ -132,8 +131,4 @@ class Admin::BudgetInvestmentsController < Admin::BaseController end end end - - def load_change_log - @logs = Budget::Investment::ChangeLog.by_investment(@investment.id) - end end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index ffe4f4f4a..2dbe3b953 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -15,7 +15,6 @@ class ApplicationController < ActionController::Base before_action :set_locale before_action :track_email_campaign before_action :set_return_url - before_action :set_current_user check_authorization unless: :devise_controller? self.responder = ApplicationResponder @@ -121,8 +120,4 @@ class ApplicationController < ActionController::Base def current_budget Budget.current end - - def set_current_user - User.current_user = current_user - end end diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 28f52023d..bb94476af 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -3,7 +3,6 @@ class Budget class Investment < ApplicationRecord SORTING_OPTIONS = { id: "id", supports: "cached_votes_up" }.freeze - include ActiveModel::Dirty include Rails.application.routes.url_helpers include Measurable include Sanitizable @@ -33,6 +32,8 @@ class Budget translates :description, touch: true include Globalizable + audited on: [:update, :destroy] + belongs_to :author, -> { with_hidden }, class_name: "User", inverse_of: :budget_investments belongs_to :heading belongs_to :group @@ -115,7 +116,6 @@ class Budget after_save :recalculate_heading_winners before_validation :set_responsible_name before_validation :set_denormalized_ids - after_update :change_log def comments_count comments.count @@ -404,20 +404,6 @@ class Budget self.original_heading_id = heading_id end - def change_log - self.changed.each do |field| - unless field == "updated_at" - log = Budget::Investment::ChangeLog.new - log.field = field - log.author_id = User.current_user.id unless User.current_user.nil? - log.investment_id = self.id - log.new_value = self.send field - log.old_value = self.send "#{field}_was" - log.save! - end - end - end - def searchable_translations_definitions { title => "A", description => "D" } diff --git a/app/models/budget/investment/change_log.rb b/app/models/budget/investment/change_log.rb deleted file mode 100644 index 5b5123ae0..000000000 --- a/app/models/budget/investment/change_log.rb +++ /dev/null @@ -1,11 +0,0 @@ -class Budget::Investment::ChangeLog < ApplicationRecord - belongs_to :author, -> { with_hidden }, - class_name: "User", - foreign_key: "author_id", - inverse_of: :budget_investment_change_logs, - required: false - - validates :field, presence: true - - scope :by_investment, ->(investment_id) { where(investment_id: investment_id) } -end diff --git a/app/models/user.rb b/app/models/user.rb index d8c53200d..4bf778d91 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -27,10 +27,6 @@ class User < ApplicationRecord class_name: "Budget::Investment", foreign_key: :author_id, inverse_of: :author - has_many :budget_investment_change_logs, - foreign_key: :author_id, - inverse_of: :author, - class_name: "Budget::Investment::ChangeLog" has_many :comments, -> { with_hidden }, inverse_of: :user has_many :failed_census_calls has_many :notifications @@ -400,14 +396,6 @@ class User < ApplicationRecord followables.compact.map { |followable| followable.tags.map(&:name) }.flatten.compact.uniq end - def self.current_user - Thread.current[:user] - end - - def self.current_user=(user) - Thread.current[:user] = user - end - def send_devise_notification(notification, *args) devise_mailer.send(notification, self, *args).deliver_later end diff --git a/app/views/admin/audits/_audits.html.erb b/app/views/admin/audits/_audits.html.erb new file mode 100644 index 000000000..a00b1fa88 --- /dev/null +++ b/app/views/admin/audits/_audits.html.erb @@ -0,0 +1,46 @@ +

<%= t("admin.audits.title") %>

+ +<% if resource.audits.empty? %> +

<%= t("admin.audits.empty") %>

+<% else %> + + + + + + + + + + + + + + <% resource.audits.order(:created_at).each do |audit| %> + <% audit.audited_changes.each do |field, (old_value, new_value)| %> + + + + + + + + + + <% end %> + <% end %> + +
<%= t("admin.audits.id") %><%= t("admin.audits.field") %><%= t("admin.audits.old_value") %><%= t("admin.audits.new_value") %><%= t("admin.audits.edited_at") %><%= t("admin.audits.edited_by") %><%= t("admin.audits.actions") %>
<%= audit.id %><%= field.capitalize %> +
<%= old_value %>
+ 		+
<%= new_value %>
+ 		+ <%= audit.created_at.to_date %> + + <%= audit.user&.name %> + + <%= link_to t("shared.show"), + admin_audit_path(audit), + class: "button hollow primary" %> +
+<% end %> diff --git a/app/views/admin/audits/show.html.erb b/app/views/admin/audits/show.html.erb new file mode 100644 index 000000000..f788d2612 --- /dev/null +++ b/app/views/admin/audits/show.html.erb @@ -0,0 +1,19 @@ +

<%= t("admin.audits.title") %>

+ + +

<%= @audit.id %>

+ +<% @audit.audited_changes.each do |field, (old_value, new_value)| %> + +

<%= field %>

+ +

<%= old_value %>

+ +

<%= new_value %>

+<% end %> + + +

<%= @audit.created_at.to_date %>

+<%= t("admin.audits.edited_by") %> + +

<%= @audit.user&.name %>

diff --git a/app/views/admin/budget_investments/show.html.erb b/app/views/admin/budget_investments/show.html.erb index fe73c2a5f..5c9f85203 100644 --- a/app/views/admin/budget_investments/show.html.erb +++ b/app/views/admin/budget_investments/show.html.erb @@ -66,6 +66,6 @@ <%= render "valuation/budget_investments/valuation_comments" %> -<%= render "admin/change_logs/change_log", logs: @logs %> +<%= render "admin/audits/audits", resource: @investment %> <%= render "admin/milestones/milestones", milestoneable: @investment %> diff --git a/app/views/admin/change_logs/_change_log.html.erb b/app/views/admin/change_logs/_change_log.html.erb deleted file mode 100644 index e3105b965..000000000 --- a/app/views/admin/change_logs/_change_log.html.erb +++ /dev/null @@ -1,44 +0,0 @@ -

<%= t("admin.change_log.title") %>

- -<% if logs.empty? %> - -<% else %> - - - - - - - - - - - - - - <% logs.each do |log| %> - - - - - - - - - - <% end %> - -
<%= t("admin.change_log.id") %><%= t("admin.change_log.field") %><%= t("admin.change_log.old_value") %><%= t("admin.change_log.new_value") %><%= t("admin.change_log.edited_at") %><%= t("admin.change_log.edited_by") %><%= t("admin.change_log.actions") %>
<%= log.id %><%= log.field.capitalize %> -
<%= log.old_value %>
- 		-
<%= log.new_value %>
- 		- <%= log.created_at.to_date %> - - <%= log.author.name unless log.author.nil? %> - - <%= link_to admin_change_log_path(id: log) do %> - - <% end %> -
-<% end %> diff --git a/app/views/admin/change_logs/show.html.erb b/app/views/admin/change_logs/show.html.erb deleted file mode 100644 index 5666e4b58..000000000 --- a/app/views/admin/change_logs/show.html.erb +++ /dev/null @@ -1,12 +0,0 @@ -

<%= t("admin.change_log.title") %>

- - -

<%= @log.id %>

- -

<%= @log.old_value %>

- -

<%= @log.new_value %>

- -

<%= @log.created_at.to_date %>

- -

<%= @log.author.name unless @log.author.nil? %>

diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index e315b5142..cdde55e9e 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -1583,7 +1583,7 @@ en: submit_header: Save header card_title: Edit card submit_card: Save card - change_log: + audits: title: "Change Log" id: "ID" field: "Field" diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index e7f998dd5..5f376fe44 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -1582,7 +1582,7 @@ es: submit_header: Guardar encabezado card_title: Editar tarjeta submit_card: Guardar tarjeta - change_log: + audits: title: "Historial" id: "ID" field: "Campo" diff --git a/config/routes/admin.rb b/config/routes/admin.rb index eb8af3fbf..ea37989d0 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -250,7 +250,7 @@ namespace :admin do get "download_settings/:resource", to: "download_settings#edit", as: "edit_download_settings" put "download_settings/:resource", to: "download_settings#update", as: "update_download_settings" - get "/change_log/:id", to: "budget_investments#show_investment_log", as: "change_log" + get "/admin/audits/:id", to: "budget_investments#audit", as: "audit" resources :local_census_records namespace :local_census_records do diff --git a/db/migrate/20191102002206_install_audited.rb b/db/migrate/20191102002206_install_audited.rb new file mode 100644 index 000000000..91e8204b7 --- /dev/null +++ b/db/migrate/20191102002206_install_audited.rb @@ -0,0 +1,26 @@ +class InstallAudited < ActiveRecord::Migration[5.0] + def change + create_table :audits, force: true do |t| + t.column :auditable_id, :integer + t.column :auditable_type, :string + t.column :associated_id, :integer + t.column :associated_type, :string + t.column :user_id, :integer + t.column :user_type, :string + t.column :username, :string + t.column :action, :string + t.column :audited_changes, :jsonb + t.column :version, :integer, default: 0 + t.column :comment, :string + t.column :remote_address, :string + t.column :request_uuid, :string + t.column :created_at, :datetime + end + + add_index :audits, [:auditable_type, :auditable_id, :version], name: "auditable_index" + add_index :audits, [:associated_type, :associated_id], name: "associated_index" + add_index :audits, [:user_id, :user_type], name: "user_index" + add_index :audits, :request_uuid + add_index :audits, :created_at + end +end diff --git a/db/migrate/20191102002238_drop_budget_investment_change_logs.rb b/db/migrate/20191102002238_drop_budget_investment_change_logs.rb new file mode 100644 index 000000000..29582d068 --- /dev/null +++ b/db/migrate/20191102002238_drop_budget_investment_change_logs.rb @@ -0,0 +1,13 @@ +class DropBudgetInvestmentChangeLogs < ActiveRecord::Migration[5.0] + def change + drop_table :budget_investment_change_logs do |t| + t.integer :investment_id + t.integer :author_id + t.string :field + t.string :new_value + t.string :old_value + + t.timestamps null: false + end + end +end diff --git a/db/schema.rb b/db/schema.rb index c6f941612..aad613106 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20191101183155) do +ActiveRecord::Schema.define(version: 20191102002238) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -84,6 +84,28 @@ ActiveRecord::Schema.define(version: 20191101183155) do t.index ["visit_id"], name: "index_ahoy_events_on_visit_id", using: :btree end + create_table "audits", force: :cascade do |t| + t.integer "auditable_id" + t.string "auditable_type" + t.integer "associated_id" + t.string "associated_type" + t.integer "user_id" + t.string "user_type" + t.string "username" + t.string "action" + t.jsonb "audited_changes" + t.integer "version", default: 0 + t.string "comment" + t.string "remote_address" + t.string "request_uuid" + t.datetime "created_at" + t.index ["associated_type", "associated_id"], name: "associated_index", using: :btree + t.index ["auditable_type", "auditable_id", "version"], name: "auditable_index", using: :btree + t.index ["created_at"], name: "index_audits_on_created_at", using: :btree + t.index ["request_uuid"], name: "index_audits_on_request_uuid", using: :btree + t.index ["user_id", "user_type"], name: "user_index", using: :btree + end + create_table "banner_sections", force: :cascade do |t| t.integer "banner_id" t.integer "web_section_id" @@ -203,16 +225,6 @@ ActiveRecord::Schema.define(version: 20191101183155) do t.index ["group_id"], name: "index_budget_headings_on_group_id", using: :btree end - create_table "budget_investment_change_logs", force: :cascade do |t| - t.integer "investment_id" - t.integer "author_id" - t.string "field" - t.string "new_value" - t.string "old_value" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - end - create_table "budget_investment_milestone_translations", force: :cascade do |t| t.integer "budget_investment_milestone_id", null: false t.string "locale", null: false