Merge pull request #14355 from opf/feature/51423-display-since-when-a…

…-failure-state-is-occurring

[#51423] Display since when a failure state is occurring

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 %>

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

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 | #<Storages::StorageErrorData:0x0000ffff646ac570>"
-    # 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

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

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.'

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

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 | #<Storages::StorageErrorData:0x0000ffff646ac570>' }
       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
 

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

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