From: Tim Otten <totten@civicrm.org>
Date: Wed, 20 Jul 2022 03:38:10 +0000 (-0700)
Subject: Status Check - Report the overall status (accurately)
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=f5aae98062cd243df5dde14abab02747321860ff;p=civicrm-core.git

Status Check - Report the overall status (accurately)

Overview
--------

The page-footer includes a summary message about the system status (eg "System
Status: Ok" or "System Status: Error").  The footer has inaccurately summarized
the status.

Before
------

Suppose you have 5 "Error" messages and 1 "OK" message. It will summarize as "System Status: Ok".

(It emphasizes the *least severe* status-message.)

Code is more complex.

After
------

Suppose you have 5 "Error" messages and 1 "OK" message. It will summarize as "System Status: Error".

(It emphasizes the *most severe* status-message.)

Code is less complex.

Comments
--------

The overall value is determined in `CRM_Utils_Check::checkAll()`.  Heretofore,
`checkAll()` sorted the messages by severity and then emphasized the *first*
visible message.

The problem is that the sort-order has flip-flopped, so "first" means different things:

* In v4.7, the messages were descending. So "first" was "most severe".
* In v5.39 (11d593edde82e7de962ec9056cfebe87975fb499), the order was ascending.
  So "first" was "least severe". (It appears accidental.)
* In v5.45 (728c1cc31d0d323f74f7b980a1e183fb499d4e9e), the order flipped back
  to descending, but *only* for the Angular (`civicrm/a/#/status`) -- not the footer.

IMHO, the flip-floppiness means that callers cannot rely on the order returned
by `checkAll()` -- they should instead do a higher-level sort (as in v5.45's
728c1cc31d0d323f74f7b980a1e183fb499d4e9e).  The `uasort()` within `checkAll()`
is therefore redundant.  We might as well skip it -- and simplify the logic.
---

diff --git a/CRM/Utils/Check.php b/CRM/Utils/Check.php
index 0c124a71c2..dd542d808b 100644
--- a/CRM/Utils/Check.php
+++ b/CRM/Utils/Check.php
@@ -116,23 +116,6 @@ class CRM_Utils_Check {
     }
   }
 
-  /**
-   * Sort messages based upon severity
-   *
-   * @param CRM_Utils_Check_Message $a
-   * @param CRM_Utils_Check_Message $b
-   * @return int
-   */
-  public static function severitySort($a, $b) {
-    $aSeverity = $a->getLevel();
-    $bSeverity = $b->getLevel();
-    if ($aSeverity == $bSeverity) {
-      return strcmp($a->getName(), $b->getName());
-    }
-    // The Message constructor guarantees that these will always be integers.
-    return ($aSeverity <=> $bSeverity);
-  }
-
   /**
    * Get the integer value (useful for thresholds) of the severity.
    *
@@ -201,15 +184,11 @@ class CRM_Utils_Check {
   public static function checkAll($max = FALSE) {
     $messages = self::checkStatus();
 
-    uasort($messages, [__CLASS__, 'severitySort']);
-
     $maxSeverity = 1;
     foreach ($messages as $message) {
-      if (!$message->isVisible()) {
-        continue;
+      if ($message->isVisible()) {
+        $maxSeverity = max(1, $message->getLevel());
       }
-      $maxSeverity = max(1, $message->getLevel());
-      break;
     }
 
     Civi::cache('checks')->set('systemStatusCheckResult', $maxSeverity);