From 83c76e6d3c98043c3026c1bc702eab8ea824e084 Mon Sep 17 00:00:00 2001
From: Jerry Huang <34140255+qwef@users.noreply.github.com>
Date: Thu, 13 Jul 2023 09:40:29 -0700
Subject: [PATCH] Remove duplicate users in api/user/recipients request
(#32340)
---
src/metabase/api/user.clj | 13 ++++++-----
test/metabase/api/user_test.clj | 39 +++++++++++++++++++++------------
2 files changed, 32 insertions(+), 20 deletions(-)
diff --git a/src/metabase/api/user.clj b/src/metabase/api/user.clj
index dd8c717f4cc..58878e4a236 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 0c4eba9ac6d..96b390a75d4 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"
--
GitLab