From bc8a2955510ed4134301c5ac5c4d7be7f242c75c Mon Sep 17 00:00:00 2001
From: Nick Fitzpatrick <nick@metabase.com>
Date: Thu, 20 Jul 2023 03:31:16 -0300
Subject: [PATCH] ordering users by first name in API responses (#32486)
* ordering users by first name in API responses
* adjusting e2e tests
* Adding Normal Personal Collection ID to cypress data
---
e2e/support/cypress_sample_instance_data.js | 5 +++
e2e/test/scenarios/onboarding/urls.cy.spec.js | 11 ++++--
.../permissions/sandboxes.cy.spec.js | 2 +-
src/metabase/api/user.clj | 8 ++---
test/metabase/api/user_test.clj | 36 +++++++++----------
5 files changed, 37 insertions(+), 25 deletions(-)
diff --git a/e2e/support/cypress_sample_instance_data.js b/e2e/support/cypress_sample_instance_data.js
index 36be9adad51..0accec58fde 100644
--- a/e2e/support/cypress_sample_instance_data.js
+++ b/e2e/support/cypress_sample_instance_data.js
@@ -29,3 +29,8 @@ export const ADMIN_PERSONAL_COLLECTION_ID = _.findWhere(
SAMPLE_INSTANCE_DATA.collections,
{ name: "Bobby Tables's Personal Collection" },
).id;
+
+export const NORMAL_PERSONAL_COLLECTION_ID = _.findWhere(
+ SAMPLE_INSTANCE_DATA.collections,
+ { name: "Robert Tableton's Personal Collection" },
+).id;
diff --git a/e2e/test/scenarios/onboarding/urls.cy.spec.js b/e2e/test/scenarios/onboarding/urls.cy.spec.js
index b34d9a7cf3d..0a17224464d 100644
--- a/e2e/test/scenarios/onboarding/urls.cy.spec.js
+++ b/e2e/test/scenarios/onboarding/urls.cy.spec.js
@@ -8,6 +8,7 @@ import { USERS, SAMPLE_DB_ID } from "e2e/support/cypress_data";
import {
ORDERS_QUESTION_ID,
ADMIN_PERSONAL_COLLECTION_ID,
+ NORMAL_PERSONAL_COLLECTION_ID,
} from "e2e/support/cypress_sample_instance_data";
import { SAVED_QUESTIONS_VIRTUAL_DB_ID } from "metabase-lib/metadata/utils/saved-questions";
@@ -106,7 +107,9 @@ describe("URLs", () => {
cy.findByText(getFullName(normal)).click();
cy.location("pathname").should(
"eq",
- `/collection/9-${getUsersPersonalCollectionSlug(normal)}`,
+ `/collection/${NORMAL_PERSONAL_COLLECTION_ID}-${getUsersPersonalCollectionSlug(
+ normal,
+ )}`,
);
});
@@ -127,7 +130,11 @@ describe("URLs", () => {
`${getFullName(admin)}'s Personal Collection`,
);
- cy.visit(`/collection/9-${getUsersPersonalCollectionSlug(normal)}`);
+ cy.visit(
+ `/collection/${NORMAL_PERSONAL_COLLECTION_ID}-${getUsersPersonalCollectionSlug(
+ normal,
+ )}`,
+ );
cy.findByTestId("collection-name-heading").should(
"have.text",
`${getFullName(normal)}'s Personal Collection`,
diff --git a/e2e/test/scenarios/permissions/sandboxes.cy.spec.js b/e2e/test/scenarios/permissions/sandboxes.cy.spec.js
index a964e46e016..d5823e90c32 100644
--- a/e2e/test/scenarios/permissions/sandboxes.cy.spec.js
+++ b/e2e/test/scenarios/permissions/sandboxes.cy.spec.js
@@ -45,7 +45,7 @@ describeEE("formatting > sandboxes", () => {
});
it("should add key attributes to an existing user", () => {
- cy.icon("ellipsis").last().click();
+ cy.icon("ellipsis").first().click();
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("Edit user").click();
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
diff --git a/src/metabase/api/user.clj b/src/metabase/api/user.clj
index 15dd2b30e36..e741d82f529 100644
--- a/src/metabase/api/user.clj
+++ b/src/metabase/api/user.clj
@@ -177,7 +177,7 @@
(vec (cons User (user-visible-columns)))
(cond-> (user-clauses status query (if (some? group_id) [group_id] nil) include_deactivated)
(some? group_id) (sql.helpers/order-by [:core_user.is_superuser :desc] [:is_group_manager :desc])
- true (sql.helpers/order-by [:%lower.last_name :asc] [:%lower.first_name :asc])
+ true (sql.helpers/order-by [:%lower.first_name :asc] [:%lower.last_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*)))
;; For admins also include the IDs of Users' Personal Collections
@@ -203,7 +203,7 @@
{:data (t2/select
(vec (cons User (user-visible-columns)))
(cond-> (user-clauses nil nil nil nil)
- true (sql.helpers/order-by [:%lower.last_name :asc] [:%lower.first_name :asc])
+ true (sql.helpers/order-by [:%lower.first_name :asc] [:%lower.last_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*)))
:total (t2/count User (user-clauses nil nil nil nil))
@@ -213,11 +213,11 @@
(let [user_group_ids (map :id (:user_group_memberships
(-> (fetch-user :id api/*current-user-id*)
(t2/hydrate :user_group_memberships))))
- data (into #{}
+ data (distinct
(t2/select
(vec (cons User (user-visible-columns)))
(cond-> (user-clauses nil nil (remove #{(u/the-id (perms-group/all-users))} user_group_ids) nil)
- true (sql.helpers/order-by [:%lower.last_name :asc] [:%lower.first_name :asc])
+ true (sql.helpers/order-by [:%lower.first_name :asc] [:%lower.last_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
diff --git a/test/metabase/api/user_test.clj b/test/metabase/api/user_test.clj
index e4727620e6f..1b115c0e9ac 100644
--- a/test/metabase/api/user_test.clj
+++ b/test/metabase/api/user_test.clj
@@ -198,14 +198,7 @@
(is (= (t2/count User)
((mt/user-http-request :crowberto :get 200 "user" :status "all") :total))))
(testing "for admins, it should include those inactive users as we'd expect"
- (is (= (->> [{:email "trashbird@metabase.com"
- :first_name "Trash"
- :last_name "Bird"
- :is_active false
- :group_ids #{(u/the-id (perms-group/all-users))}
- :personal_collection_id true
- :common_name "Trash Bird"}
- {:email "crowberto@metabase.com"
+ (is (= (->> [{:email "crowberto@metabase.com"
:first_name "Crowberto"
:last_name "Corv"
:is_superuser true
@@ -224,7 +217,14 @@
:last_name "Toucan"
:group_ids #{(u/the-id (perms-group/all-users))}
:personal_collection_id true
- :common_name "Rasta Toucan"}]
+ :common_name "Rasta Toucan"}
+ {:email "trashbird@metabase.com"
+ :first_name "Trash"
+ :last_name "Bird"
+ :is_active false
+ :group_ids #{(u/the-id (perms-group/all-users))}
+ :personal_collection_id true
+ :common_name "Trash Bird"}]
(map (partial merge @user-defaults))
(map #(dissoc % :is_qbnewb :last_login)))
(->> ((mt/user-http-request :crowberto :get 200 "user", :include_deactivated true) :data)
@@ -232,14 +232,7 @@
group-ids->sets
mt/boolean-ids-and-timestamps
(map #(dissoc % :is_qbnewb :last_login)))))
- (is (= (->> [{:email "trashbird@metabase.com"
- :first_name "Trash"
- :last_name "Bird"
- :is_active false
- :group_ids #{(u/the-id (perms-group/all-users))}
- :personal_collection_id true
- :common_name "Trash Bird"}
- {:email "crowberto@metabase.com"
+ (is (= (->> [{:email "crowberto@metabase.com"
:first_name "Crowberto"
:last_name "Corv"
:is_superuser true
@@ -258,7 +251,14 @@
:last_name "Toucan"
:group_ids #{(u/the-id (perms-group/all-users))}
:personal_collection_id true
- :common_name "Rasta Toucan"}]
+ :common_name "Rasta Toucan"}
+ {:email "trashbird@metabase.com"
+ :first_name "Trash"
+ :last_name "Bird"
+ :is_active false
+ :group_ids #{(u/the-id (perms-group/all-users))}
+ :personal_collection_id true
+ :common_name "Trash Bird"}]
(map (partial merge @user-defaults))
(map #(dissoc % :is_qbnewb :last_login)))
(->> ((mt/user-http-request :crowberto :get 200 "user", :status "all") :data)
--
GitLab