♻️ Remove user formatting from dmz/list-congregation-users

Why:
- The data is no longer transmitted over a JSON API, so the attributes
  no longer need to be formatted. Using the unmodified user/get-users
  response simplifies the code.
- The canonical user subject is the one in the user.subject column, so
  it's better to hide the sub attribute from the user.attributes column,
  even though it's also there. The presence of that attribute is an
  implementation detail of dmz/save-user-from-jwt!.

src/territory_bro/domain/dmz.clj

@@ -171,11 +171,7 @@
 (defn list-congregation-users [cong-id]
   (when (allowed? [:view-congregation cong-id])
     (let [user-ids (congregation/get-users *state* cong-id)]
-      (vec (for [user (user/get-users *conn* {:ids user-ids})]
-             ;; TODO: remove this unnecessary format change, return users as-is
-             (-> (:user/attributes user)
-                 (assoc :id (:user/id user))
-                 (assoc :sub (:user/subject user))))))))
+      (user/get-users *conn* {:ids user-ids}))))
 
 (defn download-qgis-project [cong-id]
   (let [congregation (get-congregation cong-id)

src/territory_bro/infra/user.clj

@@ -12,7 +12,9 @@
 (defn- format-user [user]
   {:user/id (:id user)
    :user/subject (:subject user)
-   :user/attributes (:attributes user)})
+   :user/attributes (-> (:attributes user)
+                        ;; avoid repeating the subject needlessly - the canonical value is the one in :user/subject
+                        (dissoc :sub))})
 
 (defn ^:dynamic get-users
   ([conn]

src/territory_bro/ui/settings_page.clj

@@ -34,11 +34,11 @@
                                      (:congregation/loans-csv-url congregation))
      :users (->> users
                  (mapv (fn [user]
-                         (assoc user :new? (= new-user (:id user)))))
+                         (assoc user :new? (= new-user (:user/id user)))))
                  (sort-by (fn [user]
                             ;; new user first, then alphabetically by name
                             [(if (:new? user) 1 2)
-                             (str/lower-case (str (:name user)))])))
+                             (-> user :user/attributes :name str str/lower-case)])))
      :permissions {:configure-congregation (dmz/allowed? [:configure-congregation cong-id])
                    :gis-access (dmz/allowed? [:gis-access cong-id])}
      :form/user-id (get-in request [:params :user-id])}))
@@ -135,38 +135,39 @@
 
 
 (defn identity-provider [user]
-  (let [sub (or (:sub user) "")]
+  (let [sub (str (:user/subject user))]
     (cond
       (str/starts-with? sub "google-oauth2|") "Google"
       (str/starts-with? sub "facebook|") "Facebook"
       :else sub)))
 
 (defn users-table-row [user model]
   (let [styles (:UserManagement (css/modules))
-        current-user? (= (:id user)
-                         (:user/id auth/*user*))]
+        current-user? (= (:user/id user)
+                         (:user/id auth/*user*))
+        {:keys [name email email_verified picture]} (:user/attributes user)]
     (h/html
      [:tr {:class (when (:new? user)
                     (:newUser styles))}
       [:td {:class (:profilePicture styles)}
-       (when (some? (:picture user))
-         [:img {:src (:picture user)
+       (when (some? picture)
+         [:img {:src picture
                 :alt ""}])]
       [:td
-       (if (str/blank? (:name user))
-         (:id user)
-         (:name user))
+       (if (str/blank? name)
+         (:user/id user)
+         name)
        (when current-user?
          (h/html " " [:em "(" (i18n/t "UserManagement.you") ")"]))]
       [:td
-       (:email user)
-       (when (and (some? (:email user))
-                  (not (:emailVerified user)))
+       email
+       (when (and (some? email)
+                  (not email_verified))
          (h/html " " [:em "(" (i18n/t "UserManagement.unverified") ")"]))]
       [:td (identity-provider user)]
       [:td [:button.pure-button {:type "button"
                                  :class (:removeUser styles)
-                                 :hx-delete (str html/*page-path* "/users?user-id=" (codec/url-encode (:id user)))
+                                 :hx-delete (str html/*page-path* "/users?user-id=" (codec/url-encode (:user/id user)))
                                  :hx-confirm (when current-user?
                                                (-> (i18n/t "UserManagement.removeYourselfWarning")
                                                    (str/replace "{{congregation}}" (:congregation/name model))))}

test/territory_bro/domain/dmz_test.clj

@@ -186,23 +186,18 @@
 ;;;; Settings
 
 (deftest list-congregation-users-test
-  (let [expected [{:id user-id
-                   :sub "user1"
-                   :name "User One"}
-                  {:id user-id2
-                   :sub "user2"
-                   :name "User Two"}]
+  (let [expected [{:user/id user-id
+                   :user/subject "user1"
+                   :user/attributes {:name "User One"
+                                     :email "[email protected]"}}
+                  {:user/id user-id2
+                   :user/subject "user2"
+                   :user/attributes {:name "User Two"
+                                     :email "[email protected]"}}]
         fake-get-users (fn [_conn query]
                          (is (= {:ids [user-id user-id2]} query)
                              "get-users query")
-                         [{:user/id user-id
-                           :user/subject "user1"
-                           :user/attributes {:sub "user1"
-                                             :name "User One"}}
-                          {:user/id user-id2
-                           :user/subject "user2"
-                           :user/attributes {:sub "user2"
-                                             :name "User Two"}}])]
+                         expected)]
 
     (binding [territory-bro.infra.user/get-users fake-get-users]
       (testutil/with-events test-events

test/territory_bro/infra/user_test.clj

@@ -44,8 +44,19 @@
                (user/get-by-id conn user-id))
             "should update attributes"))
 
-      (testing "list users"
-        (is (= ["user1" "user2"]
+      (testing "subject is not repeated in the user attributes, but all other attributes are kept"
+        (let [user3 {:sub "user3"
+                     :name "User 3"
+                     :stuff "another attribute"}
+              user-id3 (user/save-user! conn (:sub user3) user3)]
+          (is (= {:user/id user-id3
+                  :user/subject "user3"
+                  :user/attributes {:name "User 3"
+                                    :stuff "another attribute"}}
+                 (user/get-by-id conn user-id3)))))
+
+      (testing "list all users"
+        (is (= ["user1" "user2" "user3"]
                (subjects (user/get-users conn)))))
 
       (testing "find users by IDs"

test/territory_bro/ui/settings_page_test.clj

@@ -30,11 +30,11 @@
 (def model
   {:congregation/name "Congregation Name"
    :congregation/loans-csv-url "https://docs.google.com/spreadsheets/123"
-   :users [{:id user-id
-            :name "Esko Luontola"
-            :nickname "esko.luontola"
-            :picture "https://lh6.googleusercontent.com/-AmDv-VVhQBU/AAAAAAAAAAI/AAAAAAAAAeI/bHP8lVNY1aA/photo.jpg"
-            :sub "google-oauth2|102883237794451111459"
+   :users [{:user/id user-id
+            :user/subject "google-oauth2|102883237794451111459"
+            :user/attributes {:name "Esko Luontola"
+                              :nickname "esko.luontola"
+                              :picture "https://lh6.googleusercontent.com/-AmDv-VVhQBU/AAAAAAAAAAI/AAAAAAAAAeI/bHP8lVNY1aA/photo.jpg"}
             :new? false}]
    :permissions {:configure-congregation true
                  :gis-access true}
@@ -56,8 +56,7 @@
         "get-users query")
     [{:user/id user-id
       :user/subject "google-oauth2|102883237794451111459"
-      :user/attributes {:sub "google-oauth2|102883237794451111459"
-                        :name "Esko Luontola"
+      :user/attributes {:name "Esko Luontola"
                         :nickname "esko.luontola"
                         :picture "https://lh6.googleusercontent.com/-AmDv-VVhQBU/AAAAAAAAAAI/AAAAAAAAAeI/bHP8lVNY1aA/photo.jpg"}}]))
 
@@ -108,7 +107,7 @@
                 (is (= ["c" "a" "B" "D" "e"]
                        (->> (settings-page/model! request)
                             :users
-                            (map :name))))))))))))
+                            (mapv (comp :name :user/attributes)))))))))))))
 
 (deftest congregation-settings-section-test
   (testing "requires the configure-congregation permission"
@@ -123,19 +122,19 @@
 
 (deftest identity-provider-test
   (is (= "" (settings-page/identity-provider nil)))
-  (is (= "Google" (settings-page/identity-provider {:sub "google-oauth2|123456789"})))
-  (is (= "Facebook" (settings-page/identity-provider {:sub "facebook|10224970701722883"})))
-  (is (= "developer" (settings-page/identity-provider {:sub "developer"}))))
+  (is (= "Google" (settings-page/identity-provider {:user/subject "google-oauth2|123456789"})))
+  (is (= "Facebook" (settings-page/identity-provider {:user/subject "facebook|10224970701722883"})))
+  (is (= "developer" (settings-page/identity-provider {:user/subject "developer"}))))
 
 (deftest users-table-row-test
   (let [user-id (UUID. 0 1)
         current-user-id (UUID. 0 2)
-        user {:id user-id
-              :name "John Doe"
-              :picture "http://example.com/picture.jpg"
-              :email "john.doe@example.com"
-              :emailVerified true
-              :sub "google-oauth2|123456789"
+        user {:user/id user-id
+              :user/subject "google-oauth2|123456789"
+              :user/attributes {:name "John Doe"
+                                :picture "http://example.com/picture.jpg"
+                                :email "[email protected]"
+                                :email_verified true}
               :new? false}
         self-delete-confirmation "hx-confirm=\"Are you sure you want to REMOVE YOURSELF from Congregation Name? You will not be able to add yourself back.\""]
     (binding [auth/*user* {:user/id current-user-id}]
@@ -153,7 +152,7 @@
             "no delete confirmation"))
 
       (testing "highlights the current user"
-        (let [current-user (assoc user :id current-user-id)]
+        (let [current-user (replace-in user [:user/id] user-id current-user-id)]
           (is (= (html/normalize-whitespace
                   "John Doe (You)   [email protected]   Google   Remove user")
                  (-> (settings-page/users-table-row current-user model)
@@ -173,13 +172,13 @@
       (testing "highlights unverified emails"
         (is (= (html/normalize-whitespace
                 "John Doe   [email protected] (Unverified)   Google   Remove user")
-               (-> (settings-page/users-table-row (assoc user :emailVerified false) model)
+               (-> (settings-page/users-table-row (replace-in user [:user/attributes :email_verified] true false) model)
                    html/visible-text))))
 
       (testing "user data missing, show ID as placeholder"
         (is (= (html/normalize-whitespace
                 "00000000-0000-0000-0000-000000000001   Remove user")
-               (-> (settings-page/users-table-row {:id user-id} model)
+               (-> (settings-page/users-table-row (select-keys user [:user/id]) model)
                    html/visible-text)))))))
 
 (deftest user-management-section-test