Role saving: Improve diff.

* `Account edited` message includes role changes. * Improve detection of removal of last sysadmin or chair.
kohler · Nov 24, 2023 · ecb9f8d · ecb9f8d
1 parent 9ea50eb
commit ecb9f8d
Show file tree

Hide file tree

Showing 2 changed files with 58 additions and 27 deletions.
diff --git a/src/contact.php b/src/contact.php
@@ -1993,37 +1993,35 @@ function abort_prop() {
 
     /** @param int $new_roles
      * @param ?Contact $actor
-     * @return bool */
-    function save_roles($new_roles, $actor) {
+     * @param bool $skip_log
+     * @return int */
+    function save_roles($new_roles, $actor, $skip_log = false) {
         assert(($new_roles & self::ROLE_DBMASK) === $new_roles);
         $old_roles = $this->roles & self::ROLE_DBMASK;
-        // ensure there's at least one system administrator
-        if (!($new_roles & self::ROLE_ADMIN)
-            && ($old_roles & self::ROLE_ADMIN)
-            && !$this->conf->fetch_ivalue("select contactId from ContactInfo where roles!=0 and (roles&" . self::ROLE_ADMIN . ")!=0 and contactId!=" . $this->contactId . " limit 1")) {
-            $new_roles |= self::ROLE_ADMIN;
-        }
-        // log role change
-        foreach ([self::ROLE_PC => "pc", self::ROLE_ADMIN => "sysadmin", self::ROLE_CHAIR => "chair"]
-                 as $role => $type) {
-            if (($new_roles & $role) && !($old_roles & $role)) {
-                $this->conf->log_for($actor ? : $this, $this, "Added as {$type}");
-            } else if (!($new_roles & $role) && ($old_roles & $role)) {
-                $this->conf->log_for($actor ? : $this, $this, "Removed as {$type}");
-            }
+        if (($old_roles & (self::ROLE_ADMIN | self::ROLE_CHAIR)) !== 0
+            && ($new_roles & (self::ROLE_ADMIN | self::ROLE_CHAIR)) === 0) {
+            // ensure there's at least one chair or system administrator
+            $result = $this->conf->qe("update ContactInfo set roles=? where contactId=? and exists (select * from ContactInfo where roles!=0 and (roles&?)!=0 and contactId!=?)",
+                $new_roles, $this->contactId,
+                self::ROLE_ADMIN | self::ROLE_CHAIR, $this->contactId);
+        } else {
+            $result = $this->conf->qe("update ContactInfo set roles=? where contactId=?",
+                $new_roles, $this->contactId);
         }
         // save the roles bits
-        if ($old_roles !== $new_roles) {
-            $this->conf->qe("update ContactInfo set roles={$new_roles} where contactId={$this->contactId}");
+        if ($result->affected_rows > 0) {
+            if (!$skip_log) {
+                $this->conf->log_for($actor ?? $this, $this, "Account edited: roles [" . UserStatus::unparse_roles_diff($old_roles, $new_roles) . "]");
+            }
             $this->roles = $new_roles;
             $this->_session_roles = (($this->_session_roles ?? 0) & ~self::ROLE_DBMASK) | $new_roles;
             $this->set_roles_properties();
             $this->conf->invalidate_caches(["pc" => true]);
             $this->conf->invalidate_user($this, true);
             $this->update_cdb_roles();
-            return true;
+            return $new_roles;
         } else {
-            return false;
+            return $old_roles;
         }
     }
 

diff --git a/src/userstatus.php b/src/userstatus.php
@@ -53,13 +53,15 @@ class UserStatus extends MessageSet {
     public $created;
     /** @var bool */
     public $notified;
-    /** @var associative-array<string,true> */
+    /** @var associative-array<string,true|string> */
     public $diffs = [];
 
     /** @var ?ComponentSet */
     private $_cs;
 
-    public static $watch_keywords = [
+    /** @var array<string,int>
+     * @readonly */
+    static public $watch_keywords = [
         "register" => Contact::WATCH_PAPER_REGISTER_ALL,
         "submit" => Contact::WATCH_PAPER_NEWSUBMIT_ALL,
         "latewithdraw" => Contact::WATCH_LATE_WITHDRAWAL_ALL,
@@ -82,6 +84,8 @@ class UserStatus extends MessageSet {
         "uemail" => "email"
     ];
 
+    /** @var array<string,-2|1|0|1|2>
+     * @readonly */
     static public $topic_interest_name_map = [
         "low" => -2, "lo" => -2,
         "medium-low" => -1, "medium_low" => -1, "mediumlow" => -1, "mlow" => -1,
@@ -92,6 +96,14 @@ class UserStatus extends MessageSet {
         "high" => 2, "hi" => 2
     ];
 
+    /** @var array<int,string>
+     * @readonly */
+    static public $role_map = [
+        Contact::ROLE_PC => "pc",
+        Contact::ROLE_CHAIR => "chair",
+        Contact::ROLE_ADMIN => "sysadmin"
+    ];
+
     function __construct(Contact $viewer) {
         $this->conf = $viewer->conf;
         $this->viewer = $viewer;
@@ -271,7 +283,8 @@ function autocomplete($what) {
         }
     }
 
-    /** @return ?list<string> */
+    /** @param int $roles
+     * @return ?list<string> */
     static function unparse_roles_json($roles) {
         if ($roles) {
             $rj = [];
@@ -290,6 +303,19 @@ static function unparse_roles_json($roles) {
         }
     }
 
+    /** @param int $old_roles
+     * @param int $new_roles
+     * @return string */
+    static function unparse_roles_diff($old_roles, $new_roles) {
+        $t = [];
+        foreach (self::$role_map as $bit => $name) {
+            if ((($old_roles ^ $new_roles) & $bit) !== 0) {
+                $t[] = (($old_roles & $bit) !== 0 ? "-" : "+") . $name;
+            }
+        }
+        return join(" ", $t);
+    }
+
     static function unparse_json_main(UserStatus $us) {
         $user = $us->user;
         $cj = $us->jval;
@@ -913,6 +939,9 @@ function save_user($cj, $old_user = null) {
         $old_disablement = $user->disabled_flags();
 
         // initialize
+        if (isset($cj->email) && strcasecmp($cj->email, $user->email) !== 0) {
+            error_log(debug_string_backtrace());
+        }
         assert(!isset($cj->email) || strcasecmp($cj->email, $user->email) === 0);
         $this->created = !$old_user;
         $this->set_user($user);
@@ -932,9 +961,9 @@ function save_user($cj, $old_user = null) {
             && ($old_roles & Contact::ROLE_PCLIKE) !== 0) {
             $roles = ($roles & ~Contact::ROLE_PCLIKE) | ($old_roles & Contact::ROLE_PCLIKE);
         }
-        if ($roles !== $old_roles) {
-            $user->save_roles($roles, $actor);
-            $this->diffs["roles"] = true;
+        if ($roles !== $old_roles
+            && ($roles = $user->save_roles($roles, $actor, true)) !== $old_roles) {
+            $this->diffs["roles"] = self::unparse_roles_diff($old_roles, $roles);
         }
 
         // Contact DB (must precede password)
@@ -962,7 +991,11 @@ function save_user($cj, $old_user = null) {
             $user->mark_activity();
         }
         if (!empty($this->diffs)) {
-            $user->conf->log_for($this->viewer, $user, "Account edited: " . join(", ", array_keys($this->diffs)));
+            $t = [];
+            foreach ($this->diffs as $k => $v) {
+                $t[] = is_string($v) && $v !== "" ? "{$k} [{$v}]" : $k;
+            }
+            $user->conf->log_for($this->viewer, $user, "Account edited: " . join(", ", $t));
         } else if ($this->created) {
             $this->diffs["create"] = true;
         }