From e08b516284a383a1e8d90430a4dbd0721516df11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 2 Nov 2019 00:01:49 +0100 Subject: [PATCH 1/9] Reduce changelog entries generated by dev seeds Since we were saving the same budget investments several times, once for every language, we were creating more than 1000 changelog entries. Assigning the language attributes when creating the investments generates less entries, making it easier to work with them. --- db/dev_seeds/budgets.rb | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/db/dev_seeds/budgets.rb b/db/dev_seeds/budgets.rb index 3904ee694..a1e4738f2 100644 --- a/db/dev_seeds/budgets.rb +++ b/db/dev_seeds/budgets.rb @@ -121,13 +121,16 @@ section "Creating Investments" do 100.times do heading = Budget::Heading.all.sample - investment = Budget::Investment.create!( + translation_attributes = random_locales.each_with_object({}) do |locale, attributes| + attributes["title_#{locale.to_s.underscore}"] = "Title for locale #{locale}" + attributes["description_#{locale.to_s.underscore}"] = "

Description for locale #{locale}

" + end + + investment = Budget::Investment.create!({ author: User.all.sample, heading: heading, group: heading.group, budget: heading.group.budget, - title: Faker::Lorem.sentence(3).truncate(60), - description: "

#{Faker::Lorem.paragraphs.join("

")}

", created_at: rand((Time.current - 1.week)..Time.current), feasibility: %w[undecided unfeasible feasible feasible feasible feasible].sample, unfeasibility_explanation: Faker::Lorem.paragraph, @@ -136,15 +139,7 @@ section "Creating Investments" do price: rand(1..100) * 100000, skip_map: "1", terms_of_service: "1" - ) - - random_locales.map do |locale| - Globalize.with_locale(locale) do - investment.title = "Title for locale #{locale}" - investment.description = "

Description for locale #{locale}

" - investment.save! - end - end + }.merge(translation_attributes)) add_image_to(investment) if Random.rand > 0.5 end From 6f4dc11dc4c254241ebd0a5d0a7dc05c62d967d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 2 Nov 2019 00:36:56 +0100 Subject: [PATCH 2/9] Remove validations in investment changelog entries If we validate the presence of the old value and the new value, changes in optional fields will not be stored if either the old value or the new value are blank. --- app/models/budget/investment.rb | 2 +- app/models/budget/investment/change_log.rb | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 9a6fcedde..28f52023d 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -413,7 +413,7 @@ class Budget log.investment_id = self.id log.new_value = self.send field log.old_value = self.send "#{field}_was" - !log.save + log.save! end end end diff --git a/app/models/budget/investment/change_log.rb b/app/models/budget/investment/change_log.rb index fe5839b14..5b5123ae0 100644 --- a/app/models/budget/investment/change_log.rb +++ b/app/models/budget/investment/change_log.rb @@ -5,8 +5,6 @@ class Budget::Investment::ChangeLog < ApplicationRecord inverse_of: :budget_investment_change_logs, required: false - validates :old_value, presence: true - validates :new_value, presence: true validates :field, presence: true scope :by_investment, ->(investment_id) { where(investment_id: investment_id) } From 3f3fe9a3d3532f9b2fe7d0f4d1907220c2bd5243 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 2 Nov 2019 00:54:22 +0100 Subject: [PATCH 3/9] Don't define controller actions in helpers --- app/controllers/admin/budget_investments_controller.rb | 6 +++++- app/helpers/change_log_helper.rb | 6 ------ 2 files changed, 5 insertions(+), 7 deletions(-) delete mode 100644 app/helpers/change_log_helper.rb diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index 78fcfc806..a9639ccb1 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -2,7 +2,6 @@ class Admin::BudgetInvestmentsController < Admin::BaseController include FeatureFlags include CommentableActions include DownloadSettingsHelper - include ChangeLogHelper include Translatable feature_flag :budgets @@ -62,6 +61,11 @@ 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" + end + private def load_comments diff --git a/app/helpers/change_log_helper.rb b/app/helpers/change_log_helper.rb deleted file mode 100644 index f6bea8c3f..000000000 --- a/app/helpers/change_log_helper.rb +++ /dev/null @@ -1,6 +0,0 @@ -module ChangeLogHelper - def show_investment_log - @log = Budget::Investment::ChangeLog.find_by(id: params[:id]) - render "admin/change_logs/show" - end -end 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 4/9] 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 From e0c2468bd22ce7f6efc014bfe997e7b06daa603f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 3 Nov 2019 01:05:53 +0100 Subject: [PATCH 5/9] Use a different controller for investment audits The same way we do for milestones. We also make the code more consistent since the view was already in a separate folder. --- .../admin/budget_investment_audits_controller.rb | 8 ++++++++ app/controllers/admin/budget_investments_controller.rb | 7 +------ app/models/audit.rb | 2 ++ app/views/admin/audits/_audits.html.erb | 2 +- config/initializers/audited.rb | 3 +++ config/initializers/routes_hierarchy.rb | 2 ++ config/routes/admin.rb | 3 +-- 7 files changed, 18 insertions(+), 9 deletions(-) create mode 100644 app/controllers/admin/budget_investment_audits_controller.rb create mode 100644 app/models/audit.rb create mode 100644 config/initializers/audited.rb diff --git a/app/controllers/admin/budget_investment_audits_controller.rb b/app/controllers/admin/budget_investment_audits_controller.rb new file mode 100644 index 000000000..ec38952a6 --- /dev/null +++ b/app/controllers/admin/budget_investment_audits_controller.rb @@ -0,0 +1,8 @@ +class Admin::BudgetInvestmentAuditsController < Admin::BaseController + def show + investment = Budget::Investment.find(params[:budget_investment_id]) + @audit = investment.audits.find(params[:id]) + + render "admin/audits/show" + end +end diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index 0f3ed3e6a..c90c4f8e9 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -9,7 +9,7 @@ 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: :audit + before_action :load_budget before_action :load_investment, only: [:show, :edit, :update, :toggle_selection] before_action :load_ballot, only: [:show, :index] before_action :parse_valuation_filters @@ -60,11 +60,6 @@ class Admin::BudgetInvestmentsController < Admin::BaseController load_investments end - def audit - @audit = Audited::Audit.find(params[:id]) - render "admin/audits/show" - end - private def load_comments diff --git a/app/models/audit.rb b/app/models/audit.rb new file mode 100644 index 000000000..9cc175a63 --- /dev/null +++ b/app/models/audit.rb @@ -0,0 +1,2 @@ +class Audit < Audited::Audit +end diff --git a/app/views/admin/audits/_audits.html.erb b/app/views/admin/audits/_audits.html.erb index a00b1fa88..672abd93e 100644 --- a/app/views/admin/audits/_audits.html.erb +++ b/app/views/admin/audits/_audits.html.erb @@ -35,7 +35,7 @@ <%= link_to t("shared.show"), - admin_audit_path(audit), + polymorphic_path([:admin, *resource_hierarchy_for(audit)]), class: "button hollow primary" %> diff --git a/config/initializers/audited.rb b/config/initializers/audited.rb new file mode 100644 index 000000000..74ad9b5a3 --- /dev/null +++ b/config/initializers/audited.rb @@ -0,0 +1,3 @@ +Audited.config do |config| + config.audit_class = ::Audit +end diff --git a/config/initializers/routes_hierarchy.rb b/config/initializers/routes_hierarchy.rb index 82b99cf2d..c98f3b123 100644 --- a/config/initializers/routes_hierarchy.rb +++ b/config/initializers/routes_hierarchy.rb @@ -13,6 +13,8 @@ module ActionDispatch::Routing::UrlFor [*resource_hierarchy_for(resource.milestoneable), resource] when "ProgressBar" [*resource_hierarchy_for(resource.progressable), resource] + when "Audit" + [*resource_hierarchy_for(resource.auditable), resource] when "Legislation::Annotation" [resource.draft_version.process, resource.draft_version, resource] when "Legislation::Proposal", "Legislation::Question", "Legislation::DraftVersion" diff --git a/config/routes/admin.rb b/config/routes/admin.rb index ea37989d0..d28093d52 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -63,6 +63,7 @@ namespace :admin do resources :budget_investments, only: [:index, :show, :edit, :update] do member { patch :toggle_selection } + resources :audits, only: :show, controller: "budget_investment_audits" resources :milestones, controller: "budget_investment_milestones" resources :progress_bars, except: :show, controller: "budget_investment_progress_bars" end @@ -250,8 +251,6 @@ 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 "/admin/audits/:id", to: "budget_investments#audit", as: "audit" - resources :local_census_records namespace :local_census_records do resources :imports, only: [:new, :create, :show] From 04cd3b460e346ea8546eb873f6f1399faa4dfabe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 3 Nov 2019 12:58:44 +0100 Subject: [PATCH 6/9] Audit changes in investment translations Note the user interface could certainly be improved, as it doesn't show which languages have changed. --- .../admin/budget_investment_audits_controller.rb | 2 +- app/models/budget/investment.rb | 5 +++++ app/views/admin/audits/_audits.html.erb | 2 +- config/initializers/routes_hierarchy.rb | 2 +- spec/features/admin/change_log_spec.rb | 8 +++----- 5 files changed, 11 insertions(+), 8 deletions(-) diff --git a/app/controllers/admin/budget_investment_audits_controller.rb b/app/controllers/admin/budget_investment_audits_controller.rb index ec38952a6..208367220 100644 --- a/app/controllers/admin/budget_investment_audits_controller.rb +++ b/app/controllers/admin/budget_investment_audits_controller.rb @@ -1,7 +1,7 @@ class Admin::BudgetInvestmentAuditsController < Admin::BaseController def show investment = Budget::Investment.find(params[:budget_investment_id]) - @audit = investment.audits.find(params[:id]) + @audit = investment.own_and_associated_audits.find(params[:id]) render "admin/audits/show" end diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index bb94476af..7f0bdb0f5 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -33,6 +33,11 @@ class Budget include Globalizable audited on: [:update, :destroy] + has_associated_audits + translation_class.class_eval do + audited associated_with: :globalized_model, + only: Budget::Investment.translated_attribute_names + end belongs_to :author, -> { with_hidden }, class_name: "User", inverse_of: :budget_investments belongs_to :heading diff --git a/app/views/admin/audits/_audits.html.erb b/app/views/admin/audits/_audits.html.erb index 672abd93e..dc7bf87b1 100644 --- a/app/views/admin/audits/_audits.html.erb +++ b/app/views/admin/audits/_audits.html.erb @@ -16,7 +16,7 @@ - <% resource.audits.order(:created_at).each do |audit| %> + <% resource.own_and_associated_audits.order(:created_at).each do |audit| %> <% audit.audited_changes.each do |field, (old_value, new_value)| %> <%= audit.id %> diff --git a/config/initializers/routes_hierarchy.rb b/config/initializers/routes_hierarchy.rb index c98f3b123..6223d9e74 100644 --- a/config/initializers/routes_hierarchy.rb +++ b/config/initializers/routes_hierarchy.rb @@ -14,7 +14,7 @@ module ActionDispatch::Routing::UrlFor when "ProgressBar" [*resource_hierarchy_for(resource.progressable), resource] when "Audit" - [*resource_hierarchy_for(resource.auditable), resource] + [*resource_hierarchy_for(resource.associated || resource.auditable), resource] when "Legislation::Annotation" [resource.draft_version.process, resource.draft_version, resource] when "Legislation::Proposal", "Legislation::Question", "Legislation::DraftVersion" diff --git a/spec/features/admin/change_log_spec.rb b/spec/features/admin/change_log_spec.rb index 74636f7c7..963e63554 100644 --- a/spec/features/admin/change_log_spec.rb +++ b/spec/features/admin/change_log_spec.rb @@ -48,11 +48,9 @@ describe "Admin change log" do expect(page).to have_content(budget_investment.heading.name) expect(page).to have_content("There are not changes logged") - budget_investment.update!(title: "test") - - visit admin_budget_budget_investments_path(budget_investment.budget) - - click_link budget_investment.title + click_link "Edit" + fill_in "Title", with: "test" + click_button "Update" expect(page).not_to have_content("There are not changes logged") expect(page).to have_content("Change Log") From 98e836ea83842285b13ad374500e608780bb0c96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 3 Nov 2019 16:31:24 +0100 Subject: [PATCH 7/9] Improve interface of change log table The name of the changed field is translated, values are truncated so descriptions with thousands of character would make this table huge and impossible to read, dates are localized, and values like arrays and booleans are displayed properly. --- app/helpers/audits_helper.rb | 11 +++++++++++ app/views/admin/audits/_audits.html.erb | 8 ++++---- .../budget_investments/_written_by_author.html.erb | 2 +- config/locales/en/activerecord.yml | 2 ++ config/locales/en/admin.yml | 4 +--- config/locales/es/activerecord.yml | 2 ++ config/locales/es/admin.yml | 2 -- spec/features/admin/change_log_spec.rb | 6 +++--- 8 files changed, 24 insertions(+), 13 deletions(-) create mode 100644 app/helpers/audits_helper.rb diff --git a/app/helpers/audits_helper.rb b/app/helpers/audits_helper.rb new file mode 100644 index 000000000..e69c65fa7 --- /dev/null +++ b/app/helpers/audits_helper.rb @@ -0,0 +1,11 @@ +module AuditsHelper + def truncate_audit_value(resource, field, value) + if value.is_a?(Array) + truncate(value.join(","), length: 50) + elsif resource.type_for_attribute(field.to_s).type == :boolean + resource.class.human_attribute_name("#{field}_#{value}") + else + truncate(value.to_s, length: 50) + end + end +end diff --git a/app/views/admin/audits/_audits.html.erb b/app/views/admin/audits/_audits.html.erb index dc7bf87b1..f607b907d 100644 --- a/app/views/admin/audits/_audits.html.erb +++ b/app/views/admin/audits/_audits.html.erb @@ -20,15 +20,15 @@ <% audit.audited_changes.each do |field, (old_value, new_value)| %> <%= audit.id %> - <%= field.capitalize %> + <%= sanitize(resource.class.human_attribute_name(field)) %> -
<%= old_value %>
+
<%= truncate_audit_value(resource, field, old_value) %>
-
<%= new_value %>
+
<%= truncate_audit_value(resource, field, new_value) %>
- <%= audit.created_at.to_date %> + <%= l audit.created_at.to_date %> <%= audit.user&.name %> diff --git a/app/views/admin/budget_investments/_written_by_author.html.erb b/app/views/admin/budget_investments/_written_by_author.html.erb index f030aac91..63e48d4b6 100644 --- a/app/views/admin/budget_investments/_written_by_author.html.erb +++ b/app/views/admin/budget_investments/_written_by_author.html.erb @@ -41,7 +41,7 @@

<%= t("admin.budget_investments.show.selection.title") %>: - <%= t("admin.budget_investments.show.selection.#{@investment.selected?}") %> + <%= @investment.class.human_attribute_name("selected_#{@investment.selected?}") %>

diff --git a/config/locales/en/activerecord.yml b/config/locales/en/activerecord.yml index 82091f0f4..e4acbae92 100644 --- a/config/locales/en/activerecord.yml +++ b/config/locales/en/activerecord.yml @@ -161,6 +161,8 @@ en: milestone_tag_list: "Milestone tags" price_explanation: "Price explanation" selected: "Mark as selected" + selected_true: "Selected" + selected_false: "Not selected" unfeasibility_explanation: "Feasibility explanation" valuation_finished: "Valuation finished" valuator_ids: "Groups" diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index cdde55e9e..568a1410b 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -241,8 +241,6 @@ en: "false": Compatible selection: title: Selection - "true": Selected - "false": Not selected winner: title: Winner "true": "Yes" @@ -1592,7 +1590,7 @@ en: edited_at: "Edited at" edited_by: "Edited by" actions: "Actions" - empty: "There are not changes logged" + empty: "There are no changes logged" local_census_records: index: title: Manage local census diff --git a/config/locales/es/activerecord.yml b/config/locales/es/activerecord.yml index 5fe4b0d77..69c5f1956 100644 --- a/config/locales/es/activerecord.yml +++ b/config/locales/es/activerecord.yml @@ -163,6 +163,8 @@ es: milestone_tag_list: "Etiquetas de Seguimiento" price_explanation: "Informe de coste (opcional, dato público)" selected: "Marcar como seleccionado" + selected_true: "Seleccionado" + selected_false: "No seleccionado" unfeasibility_explanation: "Informe de inviabilidad (en caso de que lo sea, dato público)" valuation_finished: "Informe finalizado" valuator_ids: "Grupos" diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index 5f376fe44..89eeeb289 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -241,8 +241,6 @@ es: "false": Compatible selection: title: Selección - "true": Seleccionado - "false": No seleccionado winner: title: Ganador "true": "Si" diff --git a/spec/features/admin/change_log_spec.rb b/spec/features/admin/change_log_spec.rb index 963e63554..a472ba5e1 100644 --- a/spec/features/admin/change_log_spec.rb +++ b/spec/features/admin/change_log_spec.rb @@ -27,7 +27,7 @@ describe "Admin change log" do expect(page).to have_content(budget_investment.description) expect(page).to have_content(budget_investment.author.name) expect(page).to have_content(budget_investment.heading.name) - expect(page).to have_content("There are not changes logged") + expect(page).to have_content("There are no changes logged") end scenario "Changes" do @@ -46,13 +46,13 @@ describe "Admin change log" do expect(page).to have_content(budget_investment.description) expect(page).to have_content(budget_investment.author.name) expect(page).to have_content(budget_investment.heading.name) - expect(page).to have_content("There are not changes logged") + expect(page).to have_content("There are no changes logged") click_link "Edit" fill_in "Title", with: "test" click_button "Update" - expect(page).not_to have_content("There are not changes logged") + expect(page).not_to have_content("There are no changes logged") expect(page).to have_content("Change Log") expect(page).to have_content("Title") expect(page).to have_content("test") From 5192ac052f7fb6d7fb88d7ea4cfc7fc634857e57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 3 Nov 2019 16:47:21 +0100 Subject: [PATCH 8/9] Improve user interface showing a change log entry Don't use