From 048b03e28b30af86b0344319dfab42037c4d7141 Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Wed, 29 Apr 2026 17:00:57 +0200 Subject: [PATCH] Move storage of HealthReports into dedicated model So far they have only been stored in the Rails cache, making them pretty volatile. They are now properly stored in the database, theoretically allowing to also retrieve older health check results and compare them to newer ones. Translation responsibilities have been moved into respective components, that are rendering the results. This is part of a refactoring that moves health reports and their components out of the storages module into the core, allowing them to be reused by different modules. --- app/models/health_report.rb | 51 ++++++++++ app/models/health_report/result.rb | 82 ++++++++++++++++ .../models/health_report/result_group.rb | 87 +++++++++-------- config/locales/en.yml | 10 ++ .../20260428133700_create_health_reports.rb | 39 ++++++++ .../base_connection_validator.rb | 15 ++- .../base_validator_group.rb | 20 ++-- .../validation_group_result.rb | 95 ------------------- .../connection_validators/validator_result.rb | 86 ----------------- .../health/check_result_component.html.erb | 24 ++--- .../admin/health/check_result_component.rb | 17 ++-- .../health/health_report_component.html.erb | 14 +-- .../admin/health/health_report_component.rb | 11 +++ .../side_panel/health_status_component.rb | 3 +- .../side_panel/validation_result_component.rb | 8 +- .../admin/health_status_controller.rb | 18 ++-- .../storages/app/models/storages/storage.rb | 1 + .../admin/health_status/show.html.erb | 2 +- modules/storages/config/locales/en.yml | 7 -- .../base_connection_validator_spec.rb | 16 ++-- .../storage_configuration_validator_spec.rb | 4 +- .../ampf_configuration_validator_spec.rb | 4 +- .../storage_configuration_validator_spec.rb | 4 +- .../ampf_configuration_validator_spec.rb | 4 +- .../storage_configuration_validator_spec.rb | 4 +- .../health/check_result_component_spec.rb | 8 +- .../health/health_report_component_spec.rb | 18 ++-- .../admin/health_status_controller_spec.rb | 24 ++--- 28 files changed, 342 insertions(+), 334 deletions(-) create mode 100644 app/models/health_report.rb create mode 100644 app/models/health_report/result.rb rename modules/storages/app/common/storages/adapters/connection_validators/check_result.rb => app/models/health_report/result_group.rb (50%) create mode 100644 db/migrate/20260428133700_create_health_reports.rb delete mode 100644 modules/storages/app/common/storages/adapters/connection_validators/validation_group_result.rb delete mode 100644 modules/storages/app/common/storages/adapters/connection_validators/validator_result.rb diff --git a/app/models/health_report.rb b/app/models/health_report.rb new file mode 100644 index 00000000000..d5a7f816e84 --- /dev/null +++ b/app/models/health_report.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +class HealthReport < ApplicationRecord + belongs_to :subject, polymorphic: true + + serialize :results, coder: HealthReport::ResultGroup + + def healthy? = results.all?(&:success?) + + def unhealthy? = results.any?(&:failure?) + + def warning? = results.any?(&:warning?) + + def group(key) + results.find { |group| group.key == key } + end + + def tally + results.reduce({}) do |tally, group| + tally.merge(group.tally) { |_, v1, v2| v1 + v2 } + end + end +end diff --git a/app/models/health_report/result.rb b/app/models/health_report/result.rb new file mode 100644 index 00000000000..9a94523cbb2 --- /dev/null +++ b/app/models/health_report/result.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +class HealthReport + class Result + class << self + def skipped(key) + new(key:, state: :skipped, code: nil, context: nil) + end + + def success(key) + new(key:, state: :success, code: nil, context: nil) + end + + def failure(key, code, context) + new(key:, state: :failure, code:, context:) + end + + def warning(key, code, context) + new(key:, state: :warning, code:, context:) + end + + # Used for deserialization + def load(parsed_json) + new( + key: parsed_json.fetch("key"), + state: parsed_json.fetch("state"), + code: parsed_json.fetch("code"), + context: parsed_json.fetch("context") + ) + end + end + + attr_reader :key, :state, :code, :context + + def initialize(key:, state:, code:, context:) + @key = key + @state = state.to_sym + @code = code + @context = context + end + + def success? = state == :success + + def failure? = state == :failure + + def warning? = state == :warning + + def skipped? = state == :skipped + + def to_h + { key:, state:, code:, context: } + end + end +end diff --git a/modules/storages/app/common/storages/adapters/connection_validators/check_result.rb b/app/models/health_report/result_group.rb similarity index 50% rename from modules/storages/app/common/storages/adapters/connection_validators/check_result.rb rename to app/models/health_report/result_group.rb index e375efcb1e3..ebfca71aa97 100644 --- a/modules/storages/app/common/storages/adapters/connection_validators/check_result.rb +++ b/app/models/health_report/result_group.rb @@ -23,53 +23,60 @@ # # You should have received a copy of the GNU General Public License # along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. # # See COPYRIGHT and LICENSE files for more details. #++ -module Storages - module Adapters - module ConnectionValidators - CheckResult = Data.define(:key, :state, :code, :timestamp, :context) do - private_class_method :new +class HealthReport + class ResultGroup + class << self + # Used for serialization in health report + # Note: Because we deserialize from jsonb, we don't expect a string + # but already parsed json + def load(parsed_json) + Array(parsed_json).map { |h| new(key: h.fetch("key"), results: h.fetch("results").map { |r| Result.load(r) }) } + end - def self.skipped(key) - new(key:, state: :skipped, code: nil, timestamp: nil, context: nil) - end - - def self.failure(key, code, context) - new(key:, state: :failure, code:, timestamp: Time.zone.now, context:) - end - - def self.success(key) - new(key:, state: :success, code: nil, timestamp: Time.zone.now, context: nil) - end - - def self.warning(key, code, context) - new(key:, state: :warning, code:, timestamp: Time.zone.now, context:) - end - - def success? = state == :success - - def failure? = state == :failure - - def warning? = state == :warning - - def skipped? = state == :skipped - - def humanize_title(group) = I18n.t("storages.health.checks.#{group}.#{key}") - - def humanize_error_message - return nil if code.nil? - - I18n.t("storages.health.connection_validation.#{code}", **context) - end - - def to_h - { state: state.to_s, code:, context:, timestamp: timestamp&.iso8601 } + # Used for serialization in health report + # Note: Because we serialize into jsonb, we don't return a string (JSON.dump) + # but return a hash/array directly. + def dump(value) + if value.is_a?(Array) + value.map(&:to_h) + else + value.to_h end end end + + attr_reader :key, :results + + def initialize(key:, results: []) + @key = key + @results = results + end + + def success? = results.all?(&:success?) + + def non_failure? = results.none?(&:failure?) + + def failure? = results.any?(&:failure?) + + def warning? = results.any?(&:warning?) + + def result_for(key) + results.find { |r| r.key == key } + end + + alias [] result_for + + def tally + results.map(&:state).tally + end + + def to_h + { key:, results: results.map(&:to_h) } + end end end diff --git a/config/locales/en.yml b/config/locales/en.yml index ff066488173..62a1b5b611c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3524,6 +3524,16 @@ en: gui_validation_error: "1 error" gui_validation_error_plural: "%{count} errors" + health_report: + checks: + failures: + one: "%{count} check failed" + other: "%{count} checks failed" + success: All checks passed + warnings: + one: "%{count} check returned a warning" + other: "%{count} checks returned a warning" + homescreen: additional: projects: "Newest visible projects in this instance." diff --git a/db/migrate/20260428133700_create_health_reports.rb b/db/migrate/20260428133700_create_health_reports.rb new file mode 100644 index 00000000000..d0f11358640 --- /dev/null +++ b/db/migrate/20260428133700_create_health_reports.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +class CreateHealthReports < ActiveRecord::Migration[8.1] + def change + create_table :health_reports do |t| + t.belongs_to :subject, null: false, polymorphic: true, index: true + t.jsonb :results, null: false, default: {} + t.timestamps null: false + end + end +end diff --git a/modules/storages/app/common/storages/adapters/connection_validators/base_connection_validator.rb b/modules/storages/app/common/storages/adapters/connection_validators/base_connection_validator.rb index bd2030a1d05..b26a9f4004d 100644 --- a/modules/storages/app/common/storages/adapters/connection_validators/base_connection_validator.rb +++ b/modules/storages/app/common/storages/adapters/connection_validators/base_connection_validator.rb @@ -34,11 +34,11 @@ module Storages class BaseConnectionValidator class << self def validation_groups - @validation_groups ||= {} + @validation_groups ||= [] end def register_group(klass, precondition: ->(*) { true }) - validation_groups[klass.key] = { klass:, precondition: } + validation_groups << { klass:, precondition: } end end @@ -47,15 +47,14 @@ module Storages end def call - validation_groups.each_with_object(ValidatorResult.new) do |(key, group_metadata), result| - if group_metadata[:precondition].call(@storage, result) - result.add_group_result(key, group_metadata[:klass].call(@storage)) + health_report = @storage.health_reports.build + validation_groups.each do |group_metadata| + if group_metadata[:precondition].call(@storage, health_report) + health_report.results << group_metadata[:klass].call(@storage) end end - end - def report_cache_key - "#{@storage}_storage_#{@storage.id}_health_status_report" + health_report end private diff --git a/modules/storages/app/common/storages/adapters/connection_validators/base_validator_group.rb b/modules/storages/app/common/storages/adapters/connection_validators/base_validator_group.rb index e56f1a467c7..1cf350dd89a 100644 --- a/modules/storages/app/common/storages/adapters/connection_validators/base_validator_group.rb +++ b/modules/storages/app/common/storages/adapters/connection_validators/base_validator_group.rb @@ -42,7 +42,8 @@ module Storages def initialize(storage) @storage = storage - @results = ValidationGroupResult.new(self.class.key) + @group = HealthReport::ResultGroup.new(key: self.class.key) + @pending_checks = [] end def call @@ -50,7 +51,9 @@ module Storages validate end - @results + @pending_checks.each { @group.results << HealthReport::Result.skipped(it) } + + @group end private @@ -58,24 +61,25 @@ module Storages def validate = raise SubclassResponsibilityError def register_checks(*keys) - keys.each { @results.register_check(it) } + @pending_checks.concat(keys) end - def update_result(...) - @results.update_result(...) + def add_result(key, result) + @group.results << result + @pending_checks.delete(key) end def pass_check(key) - update_result(key, CheckResult.success(key)) + add_result(key, HealthReport::Result.success(key)) end def fail_check(key, code, context: nil) - update_result(key, CheckResult.failure(key, code, context)) + add_result(key, HealthReport::Result.failure(key, code, context)) throw :interrupted end def warn_check(key, code, context: nil, halt_validation: false) - update_result(key, CheckResult.warning(key, code, context)) + add_result(key, HealthReport::Result.warning(key, code, context)) throw :interrupted if halt_validation end end diff --git a/modules/storages/app/common/storages/adapters/connection_validators/validation_group_result.rb b/modules/storages/app/common/storages/adapters/connection_validators/validation_group_result.rb deleted file mode 100644 index 06aadf9fd91..00000000000 --- a/modules/storages/app/common/storages/adapters/connection_validators/validation_group_result.rb +++ /dev/null @@ -1,95 +0,0 @@ -# frozen_string_literal: true - -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -module Storages - module Adapters - module ConnectionValidators - class ValidationGroupResult - delegate :[], :each_pair, to: :@results - - attr_reader :key - - def initialize(key) - @key = key - @results = {} - end - - def success? = @results.values.all?(&:success?) - - def non_failure? = @results.values.none?(&:failure?) - - def failure? = @results.values.any?(&:failure?) - - def warning? = @results.values.any?(&:warning?) - - def tally - @results.values.map(&:state).tally - end - - def register_checks(keys) - Array(keys).each { register_check(it) } - end - - def register_check(key) - warn("Overriding already defined check") if @results.key?(key) - - @results[key] = CheckResult.skipped(key) - end - - def update_result(key, value) - raise(ArgumentError, "Check #{key} not registered.") unless @results.key?(key) - - @results[key] = value - end - - def timestamp - @results.values.filter_map(&:timestamp).max - end - - def humanize_title = I18n.t("storages.health.checks.#{key}.header") - - def humanize_summary - case tally - in { failure: 1.. } - I18n.t("storages.health.checks.failures", count: tally[:failure]) - in { warning: 1.. } - I18n.t("storages.health.checks.warnings", count: tally[:warning]) - else - I18n.t("storages.health.checks.success") - end - end - - def to_h - @results.transform_values(&:to_h) - end - end - end - end -end diff --git a/modules/storages/app/common/storages/adapters/connection_validators/validator_result.rb b/modules/storages/app/common/storages/adapters/connection_validators/validator_result.rb deleted file mode 100644 index 526ebb0ebbb..00000000000 --- a/modules/storages/app/common/storages/adapters/connection_validators/validator_result.rb +++ /dev/null @@ -1,86 +0,0 @@ -# frozen_string_literal: true - -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -module Storages - module Adapters - module ConnectionValidators - class ValidatorResult - private attr_reader :group_results - - delegate :each_pair, :empty?, to: :group_results - - def initialize - @group_results = {} - end - - def healthy? = group_results.values.all?(&:success?) - - def unhealthy? = group_results.values.any?(&:failure?) - - def warning? = group_results.values.any?(&:warning?) - - def group(key) = group_results.fetch(key) - - alias_method :fetch, :group - - def add_group_result(key, result) - Kernel.warn "Overwriting #{key} results" if group_results.key?(key) - - group_results[key] = result - end - - def tally - group_results.reduce({}) do |tally, (_, group)| - tally.merge(group.tally) { |_, v1, v2| v1 + v2 } - end - end - - def latest_timestamp - group_results.values.filter_map(&:timestamp).max - end - - def humanize_summary - case tally - in { failure: 1.. } - I18n.t("storages.health.checks.failures", count: tally[:failure]) - in { warning: 1.. } - I18n.t("storages.health.checks.warnings", count: tally[:warning]) - else - I18n.t("storages.health.checks.success") - end - end - - def to_h - group_results.transform_values(&:to_h) - end - end - end - end -end diff --git a/modules/storages/app/components/storages/admin/health/check_result_component.html.erb b/modules/storages/app/components/storages/admin/health/check_result_component.html.erb index 16ae000b0a8..b6c39d345b4 100644 --- a/modules/storages/app/components/storages/admin/health/check_result_component.html.erb +++ b/modules/storages/app/components/storages/admin/health/check_result_component.html.erb @@ -31,23 +31,23 @@ See COPYRIGHT and LICENSE files for more details. flex_layout do |cell| cell.with_row do flex_layout(justify_content: :space_between, classes: "flex-wrap") do |row| - row.with_column(flex_layout: true, classes: "flex-wrap") do |text| - text.with_column(mr: 2) do - render(Primer::Beta::Text.new(font_weight: :bold)) { data[:text] } + row.with_column(flex_layout: true, classes: "flex-wrap") do |line| + line.with_column(mr: 2) do + render(Primer::Beta::Text.new(font_weight: :bold)) { text } end - text.with_column(mr: 2) do - render(Primer::Beta::Text.new(font_size: :small, color: data[:status_color])) { data[:status_text] } + line.with_column(mr: 2) do + render(Primer::Beta::Text.new(font_size: :small, color: status_color)) { status_text } end - if data[:error_code].present? - text.with_column do - render(Primer::Beta::Label.new(scheme: data[:status_color])) { data[:error_code] } + if error_code.present? + line.with_column do + render(Primer::Beta::Label.new(scheme: status_color)) { error_code } end end end - if data[:error_code].present? + if error_code.present? row.with_column do - helpers.static_link_to(href: data[:docs_href], + helpers.static_link_to(href: docs_href, label: I18n.t(:label_more_information), underline: true) end @@ -55,10 +55,10 @@ See COPYRIGHT and LICENSE files for more details. end end - if data[:error_text].present? + if error_text.present? cell.with_row(mt: 1) do render(Primer::Beta::Text.new(test_selector: "op-storages--health-status-check-information")) do - data[:error_text] + error_text end end end diff --git a/modules/storages/app/components/storages/admin/health/check_result_component.rb b/modules/storages/app/components/storages/admin/health/check_result_component.rb index f1134ce3521..5d7077a4841 100644 --- a/modules/storages/app/components/storages/admin/health/check_result_component.rb +++ b/modules/storages/app/components/storages/admin/health/check_result_component.rb @@ -41,17 +41,16 @@ module Storages private - def data - @data ||= { - text: model.humanize_title(@group), - status_color:, - status_text:, - error_code:, - error_text: model.humanize_error_message, - docs_href: ::OpenProject::Static::Links.url_for(:storage_docs, :health_status) - } + def text = I18n.t("storages.health.checks.#{@group}.#{model.key}") + + def error_text + return nil if model.code.nil? + + I18n.t("storages.health.connection_validation.#{model.code}", **model.context&.symbolize_keys) end + def docs_href = ::OpenProject::Static::Links.url_for(:storage_docs, :health_status) + def error_code if model.failure? "ERR_#{model.code.upcase}" diff --git a/modules/storages/app/components/storages/admin/health/health_report_component.html.erb b/modules/storages/app/components/storages/admin/health/health_report_component.html.erb index 93481d86aeb..f19f34e8245 100644 --- a/modules/storages/app/components/storages/admin/health/health_report_component.html.erb +++ b/modules/storages/app/components/storages/admin/health/health_report_component.html.erb @@ -48,7 +48,7 @@ See COPYRIGHT and LICENSE files for more details. flex_layout do |report_container| report_container.with_row do concat(render(Primer::Beta::Octicon.new(mr: 2, **summary_icon(@report.tally)))) - concat(render(Primer::Beta::Text.new(font_weight: :bold)) { @report.humanize_summary }) + concat(render(Primer::Beta::Text.new(font_weight: :bold)) { humanize_summary(@report.tally) }) end report_container.with_row(mt: 2) do @@ -63,25 +63,25 @@ See COPYRIGHT and LICENSE files for more details. end end - @report.each_pair do |_key, group_result| + @report.results.each do |result_group| report_container.with_row(mt: 3) do render(Primer::Beta::BorderBox.new(test_selector: "op-storages--health-report-group")) do |box| box.with_header do flex_layout(justify_content: :space_between, classes: "flex-wrap") do |header| header.with_column do - render(Primer::Beta::Text.new(font_weight: :bold)) { group_result.humanize_title } + render(Primer::Beta::Text.new(font_weight: :bold)) { I18n.t("storages.health.checks.#{result_group.key}.header") } end header.with_column do - concat(render(Primer::Beta::Octicon.new(mr: 2, **summary_icon(group_result.tally)))) - concat(render(Primer::Beta::Text.new) { group_result.humanize_summary }) + concat(render(Primer::Beta::Octicon.new(mr: 2, **summary_icon(result_group.tally)))) + concat(render(Primer::Beta::Text.new) { humanize_summary(result_group.tally) }) end end end - group_result.each_pair do |_key, value| + result_group.results.each do |value| box.with_row do - render(Storages::Admin::Health::CheckResultComponent.new(group: group_result.key, result: value)) + render(Storages::Admin::Health::CheckResultComponent.new(group: result_group.key, result: value)) end end end diff --git a/modules/storages/app/components/storages/admin/health/health_report_component.rb b/modules/storages/app/components/storages/admin/health/health_report_component.rb index 015df6d0563..b7e9ab84ff7 100644 --- a/modules/storages/app/components/storages/admin/health/health_report_component.rb +++ b/modules/storages/app/components/storages/admin/health/health_report_component.rb @@ -52,6 +52,17 @@ module Storages { icon: :"check-circle", color: :success } end end + + def humanize_summary(check_tally) + case check_tally + in { failure: 1.. } + I18n.t("health_report.checks.failures", count: check_tally[:failure]) + in { warning: 1.. } + I18n.t("health_report.checks.warnings", count: check_tally[:warning]) + else + I18n.t("health_report.checks.success") + end + end end end end diff --git a/modules/storages/app/components/storages/admin/side_panel/health_status_component.rb b/modules/storages/app/components/storages/admin/side_panel/health_status_component.rb index 7346d67360e..63dd0cb1795 100644 --- a/modules/storages/app/components/storages/admin/side_panel/health_status_component.rb +++ b/modules/storages/app/components/storages/admin/side_panel/health_status_component.rb @@ -39,8 +39,7 @@ module Storages private def report - validator = Adapters::Registry.resolve("#{model}.validators.connection").new(model) - Rails.cache.read(validator.report_cache_key) + model.health_reports.order(created_at: :asc).last end end end diff --git a/modules/storages/app/components/storages/admin/side_panel/validation_result_component.rb b/modules/storages/app/components/storages/admin/side_panel/validation_result_component.rb index 0861b4c26dc..291033b62d6 100644 --- a/modules/storages/app/components/storages/admin/side_panel/validation_result_component.rb +++ b/modules/storages/app/components/storages/admin/side_panel/validation_result_component.rb @@ -49,16 +49,16 @@ module Storages { icon: :alert, icon_color: :danger, - text: I18n.t("storages.health.checks.failures", count: tally[:failure]) + text: I18n.t("health_report.checks.failures", count: tally[:failure]) } in { warning: 1.. } { icon: :alert, icon_color: :attention, - text: I18n.t("storages.health.checks.warnings", count: tally[:warning]) + text: I18n.t("health_report.checks.warnings", count: tally[:warning]) } else - { icon: :"check-circle", icon_color: :success, text: I18n.t("storages.health.checks.success") } + { icon: :"check-circle", icon_color: :success, text: I18n.t("health_report.checks.success") } end end @@ -71,7 +71,7 @@ module Storages I18n.t("storages.health.summary.warning") end - "#{text} #{I18n.t('storages.health.checked', datetime: helpers.format_time(@result.latest_timestamp))}" + "#{text} #{I18n.t('storages.health.checked', datetime: helpers.format_time(@result.created_at))}" end end end diff --git a/modules/storages/app/controllers/storages/admin/health_status_controller.rb b/modules/storages/app/controllers/storages/admin/health_status_controller.rb index 47c38968e66..90f9a52f385 100644 --- a/modules/storages/app/controllers/storages/admin/health_status_controller.rb +++ b/modules/storages/app/controllers/storages/admin/health_status_controller.rb @@ -45,12 +45,12 @@ module Storages end def show - @report = Rails.cache.read(validator.report_cache_key) + @report = @storage.health_reports.order(created_at: :asc).last respond_to do |format| format.html format.text do - timestamp = (@report&.latest_timestamp || Time.zone.now).iso8601 + timestamp = (@report&.created_at || Time.zone.now).iso8601 filename = "#{@storage.name.underscore}_health_report_#{timestamp}.txt" send_data text_report(timestamp), filename:, type: "text/plain", disposition: :attachment end @@ -58,13 +58,13 @@ module Storages end def create - create_and_cache_report + create_and_store_report redirect_to admin_settings_storage_health_status_report_path(@storage), status: :see_other end def create_health_status_report - report = create_and_cache_report + report = create_and_store_report update_via_turbo_stream(component: SidePanel::ValidationResultComponent.new(storage: @storage, result: report)) respond_to_with_turbo_streams @@ -77,18 +77,18 @@ module Storages storage: @storage.name, storage_type: @storage.to_s, configuration: @storage.non_confidential_configuration, - ran_at: timestamp - }.merge(@report.to_h).to_yaml(stringify_names: true) + ran_at: timestamp, + results: @report ? @report.results.map(&:to_h) : [] + }.to_yaml(stringify_names: true) end def find_storage @storage = ::Storages::Storage.visible.find(params[:storage_id]) end - def create_and_cache_report + def create_and_store_report report = validator.call - Rails.cache.write(validator.report_cache_key, report, expires_in: 6.hours) - + report.save! report end diff --git a/modules/storages/app/models/storages/storage.rb b/modules/storages/app/models/storages/storage.rb index 5eb02f46c08..e7ecbef4efa 100644 --- a/modules/storages/app/models/storages/storage.rb +++ b/modules/storages/app/models/storages/storage.rb @@ -52,6 +52,7 @@ module Storages has_one :oauth_client, as: :integration, dependent: :destroy has_one :oauth_application, class_name: "::Doorkeeper::Application", as: :integration, dependent: :destroy has_many :remote_identities, as: :integration, dependent: :destroy + has_many :health_reports, as: :subject, dependent: :delete_all validates :host, uniqueness: { allow_nil: true } validates :name, uniqueness: { case_sensitive: false } diff --git a/modules/storages/app/views/storages/admin/health_status/show.html.erb b/modules/storages/app/views/storages/admin/health_status/show.html.erb index e1e3e1e6191..e40350ad320 100644 --- a/modules/storages/app/views/storages/admin/health_status/show.html.erb +++ b/modules/storages/app/views/storages/admin/health_status/show.html.erb @@ -37,7 +37,7 @@ See COPYRIGHT and LICENSE files for more details. header.with_title { page_title } header.with_description do if @report.present? - I18n.t("storages.health.checked", datetime: format_time(@report.latest_timestamp)) + I18n.t("storages.health.checked", datetime: format_time(@report.created_at)) else I18n.t("storages.health.no_report") end diff --git a/modules/storages/config/locales/en.yml b/modules/storages/config/locales/en.yml index eb7a546569e..02d6576168f 100644 --- a/modules/storages/config/locales/en.yml +++ b/modules/storages/config/locales/en.yml @@ -314,13 +314,6 @@ en: host_url_accessible: Host URL accessible storage_configured: Configuration complete tenant_id: Tenant ID - failures: - one: "%{count} check failed" - other: "%{count} checks failed" - success: All checks passed - warnings: - one: "%{count} check returned a warning" - other: "%{count} checks returned a warning" connection_validation: client_id_invalid: The configured OAuth 2 client id is invalid. Please check the configuration. client_secret_invalid: The configured OAuth 2 client secret is invalid. Please check the configuration. diff --git a/modules/storages/spec/common/storages/adapters/connection_validators/base_connection_validator_spec.rb b/modules/storages/spec/common/storages/adapters/connection_validators/base_connection_validator_spec.rb index 74064e03536..b8e98498440 100644 --- a/modules/storages/spec/common/storages/adapters/connection_validators/base_connection_validator_spec.rb +++ b/modules/storages/spec/common/storages/adapters/connection_validators/base_connection_validator_spec.rb @@ -47,16 +47,16 @@ module Storages after { TestValidator.reset_groups! } - it "returns a ValidationResult" do - expect(validator.call).to be_a(ValidatorResult) + it "returns a HealthReport" do + expect(validator.call).to be_a(HealthReport) end it "only runs a verification if the precondition evaluates as truthy" do test_group = class_spy(Providers::Nextcloud::Validators::StorageConfigurationValidator) TestValidator.register_group test_group, precondition: ->(_, _) { false } - result = validator.call - expect(result).to be_empty + report = validator.call + expect(report.results).to be_empty expect(test_group).not_to have_received(:call) end @@ -69,11 +69,11 @@ module Storages ).non_failure? end - results = TestValidator.new(create(:nextcloud_storage_with_local_connection)).call + report = TestValidator.new(create(:nextcloud_storage_with_local_connection)).call - expect(results).to be_warning - expect(results.group(Providers::Nextcloud::Validators::StorageConfigurationValidator.key)).to be_success - expect(results.group(Providers::Nextcloud::Validators::AuthenticationValidator.key)).to be_warning + expect(report).to be_warning + expect(report.group(Providers::Nextcloud::Validators::StorageConfigurationValidator.key)).to be_success + expect(report.group(Providers::Nextcloud::Validators::AuthenticationValidator.key)).to be_warning end end end diff --git a/modules/storages/spec/common/storages/adapters/providers/nextcloud/validators/storage_configuration_validator_spec.rb b/modules/storages/spec/common/storages/adapters/providers/nextcloud/validators/storage_configuration_validator_spec.rb index 10975ec5bd2..c39ed18696c 100644 --- a/modules/storages/spec/common/storages/adapters/providers/nextcloud/validators/storage_configuration_validator_spec.rb +++ b/modules/storages/spec/common/storages/adapters/providers/nextcloud/validators/storage_configuration_validator_spec.rb @@ -41,10 +41,10 @@ module Storages subject(:validator) { described_class.new(storage) } - it "returns a GroupValidationResult", vcr: "nextcloud/capabilities_success" do + it "returns a ResultGroup", vcr: "nextcloud/capabilities_success" do results = validator.call - expect(results).to be_a(ConnectionValidators::ValidationGroupResult) + expect(results).to be_a(HealthReport::ResultGroup) expect(results).to be_success end diff --git a/modules/storages/spec/common/storages/adapters/providers/one_drive/validators/ampf_configuration_validator_spec.rb b/modules/storages/spec/common/storages/adapters/providers/one_drive/validators/ampf_configuration_validator_spec.rb index 9036e78a17c..7569d1e3d3c 100644 --- a/modules/storages/spec/common/storages/adapters/providers/one_drive/validators/ampf_configuration_validator_spec.rb +++ b/modules/storages/spec/common/storages/adapters/providers/one_drive/validators/ampf_configuration_validator_spec.rb @@ -43,10 +43,10 @@ module Storages subject(:validator) { described_class.new(storage) } - it "returns a GroupValidationResult", vcr: "one_drive/validator_ampf_clean_run" do + it "returns a ResultGroup", vcr: "one_drive/validator_ampf_clean_run" do results = validator.call - expect(results).to be_a(ConnectionValidators::ValidationGroupResult) + expect(results).to be_a(HealthReport::ResultGroup) expect(results).to be_success end diff --git a/modules/storages/spec/common/storages/adapters/providers/one_drive/validators/storage_configuration_validator_spec.rb b/modules/storages/spec/common/storages/adapters/providers/one_drive/validators/storage_configuration_validator_spec.rb index ab3b0478e9e..16fe975bb47 100644 --- a/modules/storages/spec/common/storages/adapters/providers/one_drive/validators/storage_configuration_validator_spec.rb +++ b/modules/storages/spec/common/storages/adapters/providers/one_drive/validators/storage_configuration_validator_spec.rb @@ -43,10 +43,10 @@ module Storages subject(:validator) { described_class.new(storage) } - it "returns a GroupValidationResult", vcr: "one_drive/files_query_userless" do + it "returns a ResultGroup", vcr: "one_drive/files_query_userless" do results = validator.call - expect(results).to be_a(ConnectionValidators::ValidationGroupResult) + expect(results).to be_a(HealthReport::ResultGroup) expect(results).to be_success end diff --git a/modules/storages/spec/common/storages/adapters/providers/sharepoint/validators/ampf_configuration_validator_spec.rb b/modules/storages/spec/common/storages/adapters/providers/sharepoint/validators/ampf_configuration_validator_spec.rb index 63719717e14..9067f370c93 100644 --- a/modules/storages/spec/common/storages/adapters/providers/sharepoint/validators/ampf_configuration_validator_spec.rb +++ b/modules/storages/spec/common/storages/adapters/providers/sharepoint/validators/ampf_configuration_validator_spec.rb @@ -47,10 +47,10 @@ module Storages subject(:validator) { described_class.new(storage) } - it "returns a GroupValidationResult", vcr: "sharepoint/validator_ampf_clean_run" do + it "returns a ResultGroup", vcr: "sharepoint/validator_ampf_clean_run" do results = validator.call - expect(results).to be_a(ConnectionValidators::ValidationGroupResult) + expect(results).to be_a(HealthReport::ResultGroup) expect(results).to be_success end diff --git a/modules/storages/spec/common/storages/adapters/providers/sharepoint/validators/storage_configuration_validator_spec.rb b/modules/storages/spec/common/storages/adapters/providers/sharepoint/validators/storage_configuration_validator_spec.rb index 11f13f30703..1af849738e2 100644 --- a/modules/storages/spec/common/storages/adapters/providers/sharepoint/validators/storage_configuration_validator_spec.rb +++ b/modules/storages/spec/common/storages/adapters/providers/sharepoint/validators/storage_configuration_validator_spec.rb @@ -43,10 +43,10 @@ module Storages subject(:validator) { described_class.new(storage) } describe "success", vcr: "sharepoint/files_query_userless" do - it "returns a GroupValidationResult" do + it "returns a ResultGroup" do results = validator.call - expect(results).to be_a(ConnectionValidators::ValidationGroupResult) + expect(results).to be_a(HealthReport::ResultGroup) expect(results).to be_success end end diff --git a/modules/storages/spec/components/storages/admin/health/check_result_component_spec.rb b/modules/storages/spec/components/storages/admin/health/check_result_component_spec.rb index d473b8da305..b34d239839e 100644 --- a/modules/storages/spec/components/storages/admin/health/check_result_component_spec.rb +++ b/modules/storages/spec/components/storages/admin/health/check_result_component_spec.rb @@ -40,7 +40,7 @@ RSpec.describe Storages::Admin::Health::CheckResultComponent, type: :component d end context "if check result is successful" do - let(:check_result) { Storages::Adapters::ConnectionValidators::CheckResult.success(:capabilities_request) } + let(:check_result) { HealthReport::Result.success(:capabilities_request) } it "renders the component" do expect(page).to have_text(I18n.t("storages.health.checks.#{group_key}.#{check_result.key}")) @@ -52,7 +52,7 @@ RSpec.describe Storages::Admin::Health::CheckResultComponent, type: :component d end context "if check result is skipped" do - let(:check_result) { Storages::Adapters::ConnectionValidators::CheckResult.skipped(:capabilities_request) } + let(:check_result) { HealthReport::Result.skipped(:capabilities_request) } it "renders the component" do expect(page).to have_text(I18n.t("storages.health.checks.#{group_key}.#{check_result.key}")) @@ -66,7 +66,7 @@ RSpec.describe Storages::Admin::Health::CheckResultComponent, type: :component d context "if check result is a warning" do let(:group_key) { :ampf_configuration } let(:check_result) do - Storages::Adapters::ConnectionValidators::CheckResult.warning(:drive_contents, :od_unexpected_content, nil) + HealthReport::Result.warning(:drive_contents, :od_unexpected_content, nil) end it "renders the component" do @@ -80,7 +80,7 @@ RSpec.describe Storages::Admin::Health::CheckResultComponent, type: :component d context "if check result is a failure" do let(:check_result) do - Storages::Adapters::ConnectionValidators::CheckResult.failure(:capabilities_request, :unknown_error, nil) + HealthReport::Result.failure(:capabilities_request, :unknown_error, nil) end it "renders the component" do diff --git a/modules/storages/spec/components/storages/admin/health/health_report_component_spec.rb b/modules/storages/spec/components/storages/admin/health/health_report_component_spec.rb index e66a8e4b099..dde82acd44f 100644 --- a/modules/storages/spec/components/storages/admin/health/health_report_component_spec.rb +++ b/modules/storages/spec/components/storages/admin/health/health_report_component_spec.rb @@ -87,23 +87,22 @@ RSpec.describe Storages::Admin::Health::HealthReportComponent, type: :component private def generate_test_group(group_key, checks) - group = Storages::Adapters::ConnectionValidators::ValidationGroupResult.new(group_key) + group = HealthReport::ResultGroup.new(key: group_key) checks.each_with_index do |check, idx| key = :"check_#{idx + 1}" result = case check when :success - Storages::Adapters::ConnectionValidators::CheckResult.success(key) + HealthReport::Result.success(key) when :warning - Storages::Adapters::ConnectionValidators::CheckResult.warning(key, :"#{key}_warning", nil) + HealthReport::Result.warning(key, :"#{key}_warning", nil) when :failure - Storages::Adapters::ConnectionValidators::CheckResult.failure(key, :"#{key}_failure", nil) + HealthReport::Result.failure(key, :"#{key}_failure", nil) else - Storages::Adapters::ConnectionValidators::CheckResult.skipped(key) + HealthReport::Result.skipped(key) end - group.register_check(key) - group.update_result(key, result) + group.results << result allow(I18n).to receive(:t).with("storages.health.checks.#{group_key}.#{key}").and_return(key.to_s.humanize) if result.code.present? allow(I18n).to receive(:t).with("storages.health.connection_validation.#{result.code}") @@ -116,11 +115,10 @@ RSpec.describe Storages::Admin::Health::HealthReportComponent, type: :component def generate_test_report(map) allow(I18n).to receive(:t).and_call_original - report = Storages::Adapters::ConnectionValidators::ValidatorResult.new + report = HealthReport.new map.each_pair do |key, values| - result = generate_test_group(key, values) - report.add_group_result(key, result) + report.results << generate_test_group(key, values) allow(I18n).to receive(:t).with("storages.health.checks.#{key}.header").and_return(key.to_s.humanize) end diff --git a/modules/storages/spec/controllers/storages/admin/health_status_controller_spec.rb b/modules/storages/spec/controllers/storages/admin/health_status_controller_spec.rb index 0179578ea8d..69efd9c15c9 100644 --- a/modules/storages/spec/controllers/storages/admin/health_status_controller_spec.rb +++ b/modules/storages/spec/controllers/storages/admin/health_status_controller_spec.rb @@ -32,7 +32,7 @@ require "spec_helper" RSpec.describe Storages::Admin::HealthStatusController do let(:user) { build_stubbed(:admin) } - let(:storage) { build_stubbed(:nextcloud_storage_configured) } + let(:storage) { create(:nextcloud_storage_configured) } let(:params) { { storage_id: storage.id } } before do @@ -72,8 +72,8 @@ RSpec.describe Storages::Admin::HealthStatusController do it "sends the text version of the report when requested" do # Creating an actual report result and caching it so we can test the rendering of the response validator = Storages::Adapters::Registry["nextcloud.validators.connection"].new(storage) - result = validator.call - Rails.cache.write validator.report_cache_key, result + report = validator.call + report.save! get :show, params: params.merge(format: :txt) @@ -84,36 +84,32 @@ RSpec.describe Storages::Admin::HealthStatusController do yaml = YAML.load(response.body) expect(yaml["storage"]).to eq storage.name expect(yaml["storage_type"]).to eq storage.to_s - expect(yaml.dig("base_configuration", "storage_configured", "state")).to eq("failure") + expect(yaml.dig("results", 0, "results", 0, "state")).to eq(:success) expect(yaml.dig("configuration", "host")).to eq(storage.host) end end describe "#create" do - let(:cache_key) { "my_cache_key" } - before do validator = instance_double(Storages::Adapters::Providers::Nextcloud::Validators::ConnectionValidator) - report = Storages::Adapters::ConnectionValidators::ValidatorResult.new + report = storage.health_reports.build allow(Storages::Adapters::Providers::Nextcloud::Validators::ConnectionValidator).to receive(:new).and_return(validator) - allow(validator).to receive_messages(call: report, report_cache_key: cache_key) + allow(validator).to receive_messages(call: report) end - it "creates and caches a health status report and redirects to show" do + it "creates a health report and redirects to show" do post :create, params: params expect(response.status).to redirect_to admin_settings_storage_health_status_report_path(storage) - expect(Rails.cache.read(cache_key)).to be_a(Storages::Adapters::ConnectionValidators::ValidatorResult) + expect(storage.reload.health_reports.count).to eq(1) end end describe "#create_health_status_report" do - let(:cache_key) { "my_cache_key" } - before do validator = instance_double(Storages::Adapters::Providers::Nextcloud::Validators::ConnectionValidator) - report = Storages::Adapters::ConnectionValidators::ValidatorResult.new + report = storage.health_reports.build allow(Storages::Adapters::Providers::Nextcloud::Validators::ConnectionValidator).to receive(:new).and_return(validator) - allow(validator).to receive_messages(call: report, report_cache_key: cache_key) + allow(validator).to receive_messages(call: report) end it "creates and caches a health status report and updates page via turbo stream" do