diff --git a/src/metabase/api/user.clj b/src/metabase/api/user.clj index dd8c717f4ccdad339e8aca4c7e7e18dd52462b12..58878e4a23640e2bd9c3e267e0e8580006e8239d 100644 --- a/src/metabase/api/user.clj +++ b/src/metabase/api/user.clj @@ -214,12 +214,13 @@ (let [user_group_ids (map :id (:user_group_memberships (-> (fetch-user :id api/*current-user-id*) (t2/hydrate :user_group_memberships)))) - data (t2/select - (vec (cons User (user-visible-columns))) - (cond-> (user-clauses nil nil (remove #{1} user_group_ids) nil) - true (sql.helpers/order-by [:%lower.last_name :asc] [:%lower.first_name :asc]) - (some? mw.offset-paging/*limit*) (sql.helpers/limit mw.offset-paging/*limit*) - (some? mw.offset-paging/*offset*) (sql.helpers/offset mw.offset-paging/*offset*)))] + data (into #{} + (t2/select + (vec (cons User (user-visible-columns))) + (cond-> (user-clauses nil nil (remove #{1} user_group_ids) nil) + true (sql.helpers/order-by [:%lower.last_name :asc] [:%lower.first_name :asc]) + (some? mw.offset-paging/*limit*) (sql.helpers/limit mw.offset-paging/*limit*) + (some? mw.offset-paging/*offset*) (sql.helpers/offset mw.offset-paging/*offset*))))] {:data data :total (count data) :limit mw.offset-paging/*limit* diff --git a/test/metabase/api/user_test.clj b/test/metabase/api/user_test.clj index 0c4eba9ac6d9a322127e52a1ea355f26b3ba2286..96b390a75d4b93e41a1d8981139e818f770f480b 100644 --- a/test/metabase/api/user_test.clj +++ b/test/metabase/api/user_test.clj @@ -106,13 +106,13 @@ (is (= [crowberto lucky rasta] (->> ((mt/user-http-request :rasta :get 200 "user/recipients") :data) (filter mt/test-user?) - (map :email))))) - (testing "Returns all users when admin" - (mt/with-temporary-setting-values [user-visibility "none"] - (is (= [crowberto lucky rasta] - (->> ((mt/user-http-request :crowberto :get 200 "user/recipients") :data) - (filter mt/test-user?) - (map :email))))))) + (map :email)))))) + (testing "Returns all users when admin" + (mt/with-temporary-setting-values [user-visibility "none"] + (is (= [crowberto lucky rasta] + (->> ((mt/user-http-request :crowberto :get 200 "user/recipients") :data) + (filter mt/test-user?) + (map :email)))))) (testing "Returns users in the group when user-visibility is same group" (mt/with-temporary-setting-values [user-visibility :group] (mt/with-temp* [PermissionsGroup [{group-id :id} {:name "Test delete group"}] @@ -120,13 +120,24 @@ PermissionsGroupMembership [_ {:user_id (mt/user->id :crowberto) :group_id group-id}]] (is (= [crowberto rasta] (->> ((mt/user-http-request :rasta :get 200 "user/recipients") :data) - (map :email)))))) - (testing "Returns only self when user-visibility is none" - (mt/with-temporary-setting-values [user-visibility :none] - (is (= [rasta] - (->> ((mt/user-http-request :rasta :get 200 "user/recipients") :data) - (filter mt/test-user?) - (map :email))))))))))) + (map :email))))))) + (testing "Doesn't return multiple of the same user when they share the same group" + (mt/with-temporary-setting-values [user-visibility :group] + (mt/with-temp* [PermissionsGroup [{group-id1 :id} {:name "Test delete group1"}] + PermissionsGroup [{group-id2 :id} {:name "Test delete group2"}] + PermissionsGroupMembership [_ {:user_id (mt/user->id :rasta) :group_id group-id1}] + PermissionsGroupMembership [_ {:user_id (mt/user->id :crowberto) :group_id group-id1}] + PermissionsGroupMembership [_ {:user_id (mt/user->id :rasta) :group_id group-id2}] + PermissionsGroupMembership [_ {:user_id (mt/user->id :crowberto) :group_id group-id2}]] + (is (= [crowberto rasta] + (->> ((mt/user-http-request :rasta :get 200 "user/recipients") :data) + (map :email))))))) + (testing "Returns only self when user-visibility is none" + (mt/with-temporary-setting-values [user-visibility :none] + (is (= [rasta] + (->> ((mt/user-http-request :rasta :get 200 "user/recipients") :data) + (filter mt/test-user?) + (map :email)))))))))) (deftest admin-user-list-test (testing "GET /api/user"