Update personal collection name when user name is updated (#11379)

* Update personal collection name when user name is updated

* Removed one line dev code

* Fixed lint errors

* Updated according to CR

* CR

src/metabase/api/user.clj

@@ -9,6 +9,7 @@
             [metabase.email.messages :as email]
             [metabase.integrations.ldap :as ldap]
             [metabase.models
+             [collection :as collection :refer [Collection]]
              [permissions-group :as group]
              [user :as user :refer [User]]]
             [metabase.util :as u]
@@ -45,6 +46,22 @@
       (api/check-superuser)
       (user/set-permissions-groups! user-or-id new-groups-or-ids))))
 
+(defn- updated-user-name [user-before-update first_name last_name]
+  (let [prev_first_name (:first_name user-before-update)
+        prev_last_name (:last_name user-before-update)
+        first_name (or first_name prev_first_name)
+        last_name (or last_name prev_last_name)]
+    (when (or (not= first_name prev_first_name)
+              (not= last_name prev_last_name))
+      [first_name last_name])))
+
+(defn- maybe-update-user-personal-collection-name! [user-before-update first_name last_name]
+  ;; If the user name is updated, we shall also update the personal collection name (if such collection exists).
+  (when-some [[first_name last_name] (updated-user-name user-before-update first_name last_name)]
+    (when-some [collection (collection/user->existing-personal-collection (u/get-id user-before-update))]
+      (let [new-collection-name (collection/format-personal-collection-name first_name last_name)]
+        (when-not (= new-collection-name (:name collection))
+          (db/update! Collection (:id collection) :name new-collection-name))))))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                   Fetching Users -- GET /api/user, GET /api/user/current, GET /api/user/:id                    |
@@ -140,17 +157,18 @@
     (api/check-403 (valid-email-update? user-before-update email))
     ;; can't change email if it's already taken BY ANOTHER ACCOUNT
     (api/checkp (not (db/exists? User, :email email, :id [:not= id]))
-      "email" (tru "Email address already associated to another user.")))
-  (db/transaction
-    (api/check-500
-     (db/update! User id
-       (u/select-keys-when body
-         :present (when api/*is-superuser?*
-                    #{:login_attributes})
-         :non-nil (set (concat [:first_name :last_name :email]
-                               (when api/*is-superuser?*
-                                 [:is_superuser]))))))
-    (maybe-set-user-permissions-groups! id group_ids is_superuser))
+      "email" (tru "Email address already associated to another user."))
+    (db/transaction
+      (api/check-500
+       (db/update! User id
+         (u/select-keys-when body
+           :present (when api/*is-superuser?*
+                      #{:login_attributes})
+           :non-nil (set (concat [:first_name :last_name :email]
+                                 (when api/*is-superuser?*
+                                   [:is_superuser]))))))
+      (maybe-set-user-permissions-groups! id group_ids is_superuser)
+      (maybe-update-user-personal-collection-name! user-before-update first_name last_name)))
   (-> (fetch-user :id id)
       (hydrate :group_ids)))
 

src/metabase/models/collection.clj

@@ -1012,6 +1012,11 @@
 ;;; |                                              Personal Collections                                              |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
+(defn format-personal-collection-name
+  "Constructs the personal collection name from user name."
+  [first-name last-name]
+  (tru "{0} {1}''s Personal Collection" first-name last-name))
+
 (s/defn ^:private user->personal-collection-name :- su/NonBlankString
   "Come up with a nice name for the Personal Collection for `user-or-id`."
   [user-or-id]
@@ -1019,12 +1024,16 @@
   ;; the same first & last name! This will *ruin* their lives :(
   (let [{first-name :first_name, last-name :last_name} (db/select-one ['User :first_name :last_name]
                                                          :id (u/get-id user-or-id))]
-    (tru "{0} {1}''s Personal Collection" first-name last-name)))
+    (format-personal-collection-name first-name last-name)))
+
+(s/defn user->existing-personal-collection :- (s/maybe CollectionInstance)
+  [user-or-id]
+  (db/select-one Collection :personal_owner_id (u/get-id user-or-id)))
 
 (s/defn user->personal-collection :- CollectionInstance
   "Return the Personal Collection for `user-or-id`, if it already exists; if not, create it and return it."
   [user-or-id]
-  (or (db/select-one Collection :personal_owner_id (u/get-id user-or-id))
+  (or (user->existing-personal-collection user-or-id)
       (try
         (db/insert! Collection
           :name              (user->personal-collection-name user-or-id)

test/metabase/api/user_test.clj

@@ -8,6 +8,7 @@
              [util :as u]]
             [metabase.middleware.util :as middleware.u]
             [metabase.models
+             [collection :as collection :refer [Collection]]
              [permissions-group :as group :refer [PermissionsGroup]]
              [permissions-group-membership :refer [PermissionsGroupMembership]]
              [user :refer [User]]
@@ -17,7 +18,9 @@
              [fixtures :as fixtures]
              [util :as tu :refer [random-name]]]
             [metabase.test.data.users :as test-users]
-            [toucan.db :as db]
+            [toucan
+             [db :as db]
+             [hydrate :refer [hydrate]]]
             [toucan.util.test :as tt]))
 
 (use-fixtures :once (fixtures/initialize :test-users-personal-collections))
@@ -326,9 +329,18 @@
 ;;; |                                      Updating a User -- PUT /api/user/:id                                      |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
+(defn include-personal-collection-name
+  {:hydrate :personal_collection_name}
+  [user]
+  (db/select-one-field :name Collection :id (:personal_collection_id user)))
+
 ;; Test that admins can edit other Users
 (expect
-  {:before   {:first_name "Cam", :last_name "Era", :is_superuser true, :email "cam.era@metabase.com"}
+  {:before   {:first_name "Cam",
+              :last_name "Era",
+              :is_superuser true,
+              :email "cam.era@metabase.com"
+              :personal_collection_name "Cam Era's Personal Collection"}
    :response (merge
               user-defaults
               {:common_name  "Cam Eron"
@@ -338,13 +350,19 @@
                :is_superuser true
                :group_ids    #{(u/get-id (group/all-users))
                                (u/get-id (group/admin))}})
-   :after    {:first_name "Cam", :last_name "Eron", :is_superuser true, :email "cam.eron@metabase.com"}}
-  (tt/with-temp User [{user-id :id} {:first_name   "Cam"
-                                     :last_name    "Era"
-                                     :email        "cam.era@metabase.com"
-                                     :is_superuser true}]
-    (let [user (fn [] (into {} (-> (db/select-one [User :first_name :last_name :is_superuser :email], :id user-id)
-                                   (dissoc :common_name))))]
+   :after    {:first_name "Cam"
+              :last_name "Eron"
+              :is_superuser true
+              :email "cam.eron@metabase.com"
+              :personal_collection_name "Cam Eron's Personal Collection"}}
+  (tt/with-temp* [User [{user-id :id} {:first_name   "Cam"
+                                       :last_name    "Era"
+                                       :email        "cam.era@metabase.com"
+                                       :is_superuser true}]
+                  Collection [coll]]
+    (let [user (fn [] (into {} (-> (db/select-one [User :id :first_name :last_name :is_superuser :email], :id user-id)
+                                   (hydrate :personal_collection_id :personal_collection_name)
+                                   (dissoc :id :personal_collection_id :common_name))))]
       (array-map
        :before   (user)
        :response (-> ((test-users/user->client :crowberto) :put 200 (str "user/" user-id)