diff --git a/modules/storages/app/components/storages/admin/health_status_component.html.erb b/modules/storages/app/components/storages/admin/health_status_component.html.erb index 362dff286c85..319956de938d 100644 --- a/modules/storages/app/components/storages/admin/health_status_component.html.erb +++ b/modules/storages/app/components/storages/admin/health_status_component.html.erb @@ -2,7 +2,9 @@ <%= render(Primer::Beta::Heading.new(tag: :h4, mb: 2)) { I18n.t('storages.health.title') } %> <%= render(Primer::Box.new(py: 1)) do %> - <%= render(Primer::Beta::Text.new(test_selector: "storage-health-changed-at")) { I18n.t('storages.health.checked', datetime: helpers.format_time(storage.health_changed_at)) } %> + <%= render(Primer::Beta::Text.new(test_selector: "storage-health-checked-at")) { + I18n.t('storages.health.checked', datetime: helpers.format_time(storage.health_checked_at)) + } %> <% if storage.health_healthy? %> <%= render(Primer::Beta::Label.new(scheme: :success, test_selector: "storage-health-label-healthy")) { I18n.t('storages.health.label_healthy') @@ -19,6 +21,8 @@ <% end %> <% if storage.health_unhealthy? %> - <%= render(Primer::Beta::Text.new(tag: :div, color: :muted, py: 1, test_selector: "storage-health-reason")) { storage.formatted_health_reason } %> + <%= render(Primer::Beta::Text.new(tag: :div, color: :muted, py: 1, test_selector: "storage-health-reason")) { + formatted_health_reason + } %> <% end %> <% end %> diff --git a/modules/storages/app/components/storages/admin/health_status_component.rb b/modules/storages/app/components/storages/admin/health_status_component.rb index efd92ea08cfd..0e69b8b0c786 100644 --- a/modules/storages/app/components/storages/admin/health_status_component.rb +++ b/modules/storages/app/components/storages/admin/health_status_component.rb @@ -31,5 +31,12 @@ module Storages::Admin class HealthStatusComponent < ApplicationComponent alias_method :storage, :model + + # This method returns the health identifier, description and the time since when the error occurs in a + # formatted manner. e.g. "Not found: Outbound request destination not found since 12/07/2023 03:45 PM" + def formatted_health_reason + "#{storage.health_reason_identifier.tr('_', ' ').strip.capitalize}: #{storage.health_reason_description} " + + I18n.t('storages.health.since', datetime: helpers.format_time(storage.health_changed_at)) + end end end diff --git a/modules/storages/app/models/storages/storage.rb b/modules/storages/app/models/storages/storage.rb index 1a607462b2f6..a94c37c95023 100644 --- a/modules/storages/app/models/storages/storage.rb +++ b/modules/storages/app/models/storages/storage.rb @@ -100,12 +100,35 @@ def self.one_drive_without_ee_token?(provider_type) !EnterpriseToken.allows_to?(:one_drive_sharepoint_file_storage) end + def self.extract_part_from_piped_string(text, index) + return if text.nil? + + split_reason = text.split('|') + if split_reason.length > index + split_reason[index].strip + end + end + def mark_as_unhealthy(reason: nil) - update(health_status: 'unhealthy', health_changed_at: Time.now.utc, health_reason: reason) + if health_status == 'unhealthy' && reason_is_same(reason) + touch(:health_checked_at) + else + update(health_status: 'unhealthy', + health_changed_at: Time.now.utc, + health_checked_at: Time.now.utc, + health_reason: reason) + end end def mark_as_healthy - update(health_status: 'healthy', health_changed_at: Time.now.utc, health_reason: nil) + if health_status == 'healthy' + touch(:health_checked_at) + else + update(health_status: 'healthy', + health_changed_at: Time.now.utc, + health_checked_at: Time.now.utc, + health_reason: nil) + end end def configured? @@ -142,16 +165,18 @@ def provider_type_one_drive? provider_type == ::Storages::Storage::PROVIDER_TYPE_ONE_DRIVE end - # Currently the error messages saved look like: - # "unauthorized | Outbound request not authorized | #" - # This method returns the first two parts of the error message. - def formatted_health_reason - split_reason = health_reason.split('|') - if split_reason.length === 3 - "#{split_reason[0].strip.capitalize}: #{split_reason[1]}" - else - health_reason - end + def health_reason_identifier + @health_reason_identifier ||= self.class.extract_part_from_piped_string(health_reason, 0) + end + + def health_reason_description + @health_reason_description ||= self.class.extract_part_from_piped_string(health_reason, 1) + end + + private + + def reason_is_same(new_health_reason) + health_reason_identifier == self.class.extract_part_from_piped_string(new_health_reason, 0) end end end diff --git a/modules/storages/app/workers/storages/manage_nextcloud_integration_job_mixin.rb b/modules/storages/app/workers/storages/manage_nextcloud_integration_job_mixin.rb index 2e5906267839..01e8fd4025a2 100644 --- a/modules/storages/app/workers/storages/manage_nextcloud_integration_job_mixin.rb +++ b/modules/storages/app/workers/storages/manage_nextcloud_integration_job_mixin.rb @@ -43,12 +43,10 @@ def perform result = GroupFolderPropertiesSyncService.call(storage) result.match( on_success: ->(_) do - storage.mark_as_healthy unless storage.health_healthy? + storage.mark_as_healthy end, on_failure: ->(errors) do - if !storage.health_unhealthy? || storage.health_reason != errors.to_s - storage.mark_as_unhealthy(reason: errors.to_s) - end + storage.mark_as_unhealthy(reason: errors.to_s) end ) end diff --git a/modules/storages/config/locales/en.yml b/modules/storages/config/locales/en.yml index 4e350c09a4c0..d042c4e2f4e3 100644 --- a/modules/storages/config/locales/en.yml +++ b/modules/storages/config/locales/en.yml @@ -277,7 +277,8 @@ en: label_pending: "Pending" label_error: "Error" label_healthy: "Healthy" - checked: "Checked %{datetime}" + checked: "Last checked %{datetime}" + since: "since %{datetime}" upsale: title: "OneDrive/SharePoint integration" description: 'Integrate your OneDrive/SharePoint as a file storage with OpenProject. Upload files and link them directly to work packages in a project.' diff --git a/modules/storages/db/migrate/20231208143303_add_health_checked_at_to_storages.rb b/modules/storages/db/migrate/20231208143303_add_health_checked_at_to_storages.rb new file mode 100644 index 000000000000..65c9b5f5497f --- /dev/null +++ b/modules/storages/db/migrate/20231208143303_add_health_checked_at_to_storages.rb @@ -0,0 +1,9 @@ +class AddHealthCheckedAtToStorages < ActiveRecord::Migration[7.0] + def up + add_column(:storages, :health_checked_at, :datetime, null: false, default: -> { 'current_timestamp' }) + end + + def down + remove_column(:storages, :health_checked_at) + end +end diff --git a/modules/storages/spec/factories/storage_factory.rb b/modules/storages/spec/factories/storage_factory.rb index d6acdce69c16..71d1b9baffa3 100644 --- a/modules/storages/spec/factories/storage_factory.rb +++ b/modules/storages/spec/factories/storage_factory.rb @@ -69,24 +69,28 @@ health_status { 'healthy' } health_reason { nil } health_changed_at { Time.now.utc } + health_checked_at { Time.now.utc } end trait :as_unhealthy do health_status { 'unhealthy' } - health_reason { 'error reason' } + health_reason { 'error_code | description' } health_changed_at { Time.now.utc } + health_checked_at { Time.now.utc } end trait :as_unhealthy_long_reason do health_status { 'unhealthy' } health_reason { 'unauthorized | Outbound request not authorized | #' } health_changed_at { Time.now.utc } + health_checked_at { Time.now.utc } end trait :as_pending do health_status { 'pending' } health_reason { nil } health_changed_at { Time.now.utc } + health_checked_at { Time.now.utc } end end diff --git a/modules/storages/spec/features/admin_storages_spec.rb b/modules/storages/spec/features/admin_storages_spec.rb index 6af980fcda5d..b9ecaa9cfe94 100644 --- a/modules/storages/spec/features/admin_storages_spec.rb +++ b/modules/storages/spec/features/admin_storages_spec.rb @@ -664,7 +664,7 @@ it 'shows healthy status for storages that are healthy' do visit edit_admin_settings_storage_path(complete_nextcloud_storage_health_healthy) expect(page).to have_test_selector('storage-health-label-healthy', text: 'Healthy') - expect(page).to have_test_selector('storage-health-changed-at', text: "Checked 11/28/2023 01:02 AM") + expect(page).to have_test_selector('storage-health-checked-at', text: "Last checked 11/28/2023 01:02 AM") end it 'shows pending label for a storage that is pending' do @@ -675,13 +675,14 @@ it 'shows error status for storages that are unhealthy' do visit edit_admin_settings_storage_path(complete_nextcloud_storage_health_unhealthy) expect(page).to have_test_selector('storage-health-label-error', text: 'Error') - expect(page).to have_test_selector('storage-health-reason', text: 'error reason') + expect(page).to have_test_selector('storage-health-reason', text: 'Error code: description since 11/28/2023 01:02 AM') end it 'shows formatted error reason for storages that are unhealthy' do visit edit_admin_settings_storage_path(complete_nextcloud_storage_health_unhealthy_long_reason) expect(page).to have_test_selector('storage-health-label-error', text: 'Error') - expect(page).to have_test_selector('storage-health-reason', text: 'Unauthorized: Outbound request not authorized') + expect(page).to have_test_selector('storage-health-reason', text: + 'Unauthorized: Outbound request not authorized since 11/28/2023 01:02 AM') end end end diff --git a/modules/storages/spec/models/storages/nextcloud_storage_spec.rb b/modules/storages/spec/models/storages/nextcloud_storage_spec.rb index 90e7012a2058..6a3494913095 100644 --- a/modules/storages/spec/models/storages/nextcloud_storage_spec.rb +++ b/modules/storages/spec/models/storages/nextcloud_storage_spec.rb @@ -66,8 +66,48 @@ expect(storage.health_healthy?).to be(false) expect(storage.health_unhealthy?).to be(true) end + + it 'has the correct changed_at and checked_at attributes' do + healthy_time = Time.parse '2021-03-14T15:17:00Z' + unhealthy_time_a = Time.parse '2023-03-14T00:00:00Z' + unhealthy_time_b = Time.parse '2023-03-14T22:22:00Z' + unhealthy_time_c = Time.parse '2023-03-14T11:11:00Z' + reason_a = "thou_shall_not_pass_error" + reason_b = "inception_error" + + Timecop.freeze(healthy_time) do + expect do + storage.mark_as_healthy + end.to(change(storage, :health_changed_at).to(healthy_time) + .and(change(storage, :health_checked_at).to(healthy_time))) + end + + Timecop.freeze(unhealthy_time_a) do + expect do + storage.mark_as_unhealthy(reason: reason_a) + end.to(change(storage, :health_changed_at).from(healthy_time).to(unhealthy_time_a) + .and(change(storage, :health_reason).from(nil).to(reason_a)) + .and(change(storage, :health_checked_at).from(healthy_time).to(unhealthy_time_a))) + end + + Timecop.freeze(unhealthy_time_b) do + expect do + storage.mark_as_unhealthy(reason: reason_a) + end.to(change(storage, :health_checked_at).from(unhealthy_time_a).to(unhealthy_time_b)) + expect(storage.health_changed_at).to eq(unhealthy_time_a) + end + + Timecop.freeze(unhealthy_time_c) do + expect do + storage.mark_as_unhealthy(reason: reason_b) + end.to(change(storage, :health_checked_at).from(unhealthy_time_b).to(unhealthy_time_c) + .and(change(storage, :health_changed_at).from(unhealthy_time_a).to(unhealthy_time_c)) + .and(change(storage, :health_reason).from(reason_a).to(reason_b))) + end + end end + describe '#configured?' do context 'with a complete configuration' do let(:storage) do