Merge pull request #3836 from metabase/return-empty-groups-with-permissions-groups-endpoint

Make sure empty groups come back with GET /api/permissions/group 

src/metabase/api/permissions.clj

@@ -77,20 +77,34 @@
 ;;; |                                                             PERMISSIONS GROUP ENDPOINTS                                                              |
 ;;; +------------------------------------------------------------------------------------------------------------------------------------------------------+
 
+(defn- group-id->num-members
+  "Return a map of `PermissionsGroup` ID -> number of members in the group.
+  (This doesn't include entries for empty groups.)"
+  []
+  (into {} (for [{:keys [group_id members]} (db/query {:select    [[:pgm.group_id :group_id] [:%count.pgm.id :members]]
+                                                       :from      [[:permissions_group_membership :pgm]]
+                                                       :left-join [[:core_user :user] [:= :pgm.user_id :user.id]]
+                                                       :where     [:= :user.is_active true]
+                                                       :group-by  [:pgm.group_id]})]
+             {group_id members})))
+
+(defn- ordered-groups
+  "Return a sequence of ordered `PermissionsGroups`, including the `MetaBot` group only if MetaBot is enabled."
+  []
+  (db/select PermissionsGroup
+    {:where    (if (metabot/metabot-enabled)
+                 true
+                 [:not= :id (u/get-id (group/metabot))])
+     :order-by [:%lower.name]}))
+
 (defendpoint GET "/group"
-  "Fetch all `PermissionsGroups`."
+  "Fetch all `PermissionsGroups`, including a count of the number of `:members` in that group."
   []
   (check-superuser)
-  (db/query {:select    [:pg.id :pg.name [:%count.pgm.id :members]]
-             :from      [[:permissions_group :pg]]
-             :left-join [[:permissions_group_membership :pgm] [:= :pg.id :pgm.group_id]
-                         [:core_user :user]                   [:= :pgm.user_id :user.id]]
-             :where     [:and [:= :user.is_active true]
-                              (if (metabot/metabot-enabled)
-                                true
-                                [:not= :pg.id (:id (group/metabot))])]
-             :group-by  [:pg.id :pg.name]
-             :order-by  [:%lower.pg.name]}))
+  (let [group-id->members (group-id->num-members)]
+    (for [group (ordered-groups)]
+      (assoc group :members (or (group-id->members (u/get-id group))
+                                0)))))
 
 (defendpoint GET "/group/:id"
   "Fetch the details for a certain permissions group."

test/metabase/api/permissions_test.clj

 (ns metabase.api.permissions-test
-  "Tests for `api/permissions` endpoints."
+  "Tests for `/api/permissions` endpoints."
   (:require [expectations :refer :all]
-            [metabase.test.data.users :as tu]
-            [metabase.models.permissions-group :as group]
+            [metabase.models.permissions-group :refer [PermissionsGroup], :as group]
+            [metabase.test.data.users :as test-users]
+            [metabase.test.util :as tu]
             [metabase.util :as u]))
 
 
-;; GET /group
+;; GET /permissions/group
 ;; Should *not* include inactive users in the counts.
 ;; It should also *not* include the MetaBot group because MetaBot should *not* be enabled
+(defn- fetch-groups []
+  (test-users/delete-temp-users!)
+  (set ((test-users/user->client :crowberto) :get 200 "permissions/group")))
+
 (expect
   #{{:id (u/get-id (group/all-users)), :name "All Users",      :members 3}
     {:id (u/get-id (group/admin)),     :name "Administrators", :members 1}}
-  (do
-    (tu/delete-temp-users!)
-    (set ((tu/user->client :crowberto) :get 200 "permissions/group"))))
+  (fetch-groups))
+
+;; The endpoint should however return empty groups!
+(tu/expect-with-temp [PermissionsGroup [group]]
+  #{{:id (u/get-id (group/all-users)), :name "All Users",      :members 3}
+    {:id (u/get-id (group/admin)),     :name "Administrators", :members 1}
+    (assoc (into {} group) :members 0)}
+  (fetch-groups))
+
 
-;; GET /group/:id
+;; GET /permissions/group/:id
 ;; Should *not* include inactive users
 (expect
-  #{{:first_name "Crowberto", :last_name "Corv",   :email "crowberto@metabase.com", :user_id (tu/user->id :crowberto), :membership_id true}
-    {:first_name "Lucky",     :last_name "Pigeon", :email "lucky@metabase.com",     :user_id (tu/user->id :lucky),     :membership_id true}
-    {:first_name "Rasta",     :last_name "Toucan", :email "rasta@metabase.com",     :user_id (tu/user->id :rasta),     :membership_id true}}
+  #{{:first_name "Crowberto", :last_name "Corv",   :email "crowberto@metabase.com", :user_id (test-users/user->id :crowberto), :membership_id true}
+    {:first_name "Lucky",     :last_name "Pigeon", :email "lucky@metabase.com",     :user_id (test-users/user->id :lucky),     :membership_id true}
+    {:first_name "Rasta",     :last_name "Toucan", :email "rasta@metabase.com",     :user_id (test-users/user->id :rasta),     :membership_id true}}
   (do
-    (tu/delete-temp-users!)
-    (set (for [member (:members ((tu/user->client :crowberto) :get 200 (str "permissions/group/" (u/get-id (group/all-users)))))]
+    (test-users/delete-temp-users!)
+    (set (for [member (:members ((test-users/user->client :crowberto) :get 200 (str "permissions/group/" (u/get-id (group/all-users)))))]
            (update member :membership_id (complement nil?))))))