chore[#51423] Refactor feature to display since when a failure state …

…is occurring

modules/storages/app/components/storages/admin/health_status_component.html.erb

@@ -2,8 +2,8 @@
   <%= 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")) {

modules/storages/app/components/storages/admin/health_status_component.rb

@@ -32,15 +32,11 @@ module Storages::Admin
   class HealthStatusComponent < ApplicationComponent
     alias_method :storage, :model
 
-    # This method returns the health identifier, description and optionally the time since when the error occurs in a
+    # 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
-      reason = "#{storage.health_reason_identifier.tr('_', ' ').strip.capitalize}: #{storage.health_reason_description}"
-
-      unless storage.health_reason_since_time.nil?
-        reason += " #{I18n.t('storages.health.since', datetime: helpers.format_time(storage.health_reason_since_time))}"
-      end
-      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

@@ -110,12 +110,25 @@ def self.extract_part_from_piped_string(text, index)
     end
 
     def mark_as_unhealthy(reason: nil)
-      reason = forward_or_add_health_reason_since_time(reason.to_s) unless 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?
@@ -160,22 +173,10 @@ def health_reason_description
       @health_reason_description ||= self.class.extract_part_from_piped_string(health_reason, 1)
     end
 
-    def health_reason_since_time
-      @health_reason_since_time ||= self.class.extract_part_from_piped_string(health_reason, 3)
-    end
-
     private
 
-    # The error messages from the ServiceResult look like:
-    # "unauthorized | Outbound request not authorized | #<Storages::StorageErrorData:0x0000ffff646ac570>"
-    # This method adds the Time.now.utc when the error occurs the first time and forwards it subsequently till the
-    # error identifier changes.
-    def forward_or_add_health_reason_since_time(new_health_reason)
-      if health_reason_since_time && health_reason_identifier == self.class.extract_part_from_piped_string(new_health_reason, 0)
-        "#{new_health_reason} | #{ health_reason_since_time }"
-      else
-        "#{new_health_reason} | #{ Time.now.utc }"
-      end
+    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/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

@@ -647,7 +647,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
@@ -658,13 +658,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