From 163b6a71df06d48adbfc7cb1de23be78045f07c8 Mon Sep 17 00:00:00 2001
From: Case Nelson <case@metabase.com>
Date: Wed, 5 Apr 2023 11:18:58 -0600
Subject: [PATCH] [MLv2] Add direction arg to orderByClause (#29803)
* Add direction arg to orderByClause
* Update order_by.ts
* Fix TypeScript error
* Update order_by.ts
---------
Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com>
---
frontend/src/metabase-lib/order_by.ts | 17 ++++++++---
.../src/metabase-lib/order_by.unit.spec.ts | 4 +--
frontend/src/metabase-lib/query.ts | 6 ++--
src/metabase/lib/js.cljs | 6 ++--
src/metabase/lib/order_by.cljc | 28 +++++++++++++------
test/metabase/lib/order_by_test.cljc | 13 ++++++---
6 files changed, 51 insertions(+), 23 deletions(-)
diff --git a/frontend/src/metabase-lib/order_by.ts b/frontend/src/metabase-lib/order_by.ts
index de2d8d5a809..c9fec15c8eb 100644
--- a/frontend/src/metabase-lib/order_by.ts
+++ b/frontend/src/metabase-lib/order_by.ts
@@ -20,10 +20,19 @@ declare function OrderByFn(
export const orderBy: typeof OrderByFn = ML.order_by;
-export function orderByClause(
+type OrderByDirection = "asc" | "desc";
+
+declare function OrderByClauseFn(
query: Query,
stageNumber: number,
field: Field,
-): Field {
- return ML.order_by_clause(query, stageNumber, field);
-}
+ direction?: OrderByDirection,
+): OrderByClause;
+declare function OrderByClauseFn(
+ query: Query,
+ stageNumber: number,
+ field: Field,
+ direction?: OrderByDirection,
+): OrderByClause;
+
+export const orderByClause: typeof OrderByClauseFn = ML.order_by_clause;
diff --git a/frontend/src/metabase-lib/order_by.unit.spec.ts b/frontend/src/metabase-lib/order_by.unit.spec.ts
index 467fcb2a384..1befda4610b 100644
--- a/frontend/src/metabase-lib/order_by.unit.spec.ts
+++ b/frontend/src/metabase-lib/order_by.unit.spec.ts
@@ -79,11 +79,11 @@ describe("order by", () => {
const nextQuery = ML.replaceClause(
orderedQuery,
orderBys[0],
- ML.orderByClause(orderedQuery, -1, productCategory as Field) as Field,
+ ML.orderByClause(orderedQuery, -1, productCategory as Field, "desc"),
);
const nextOrderBys = ML.orderBys(nextQuery);
expect(ML.displayName(nextQuery, nextOrderBys[0])).toBe(
- "Category ascending",
+ "Category descending",
);
expect(orderBys[0]).not.toEqual(nextOrderBys[0]);
});
diff --git a/frontend/src/metabase-lib/query.ts b/frontend/src/metabase-lib/query.ts
index 824dbd687e9..99af10d039c 100644
--- a/frontend/src/metabase-lib/query.ts
+++ b/frontend/src/metabase-lib/query.ts
@@ -1,5 +1,5 @@
import * as ML from "cljs/metabase.lib.js";
-import type { DatabaseId, DatasetQuery, Field } from "metabase-types/api";
+import type { DatabaseId, DatasetQuery } from "metabase-types/api";
import type { Clause, MetadataProvider, Query } from "./types";
export function fromLegacyQuery(
@@ -30,13 +30,13 @@ export const removeClause: typeof RemoveClauseFn = ML.remove_clause;
declare function ReplaceClauseFn(
query: Query,
targetClause: Clause,
- newClause: Field,
+ newClause: Clause,
): Query;
declare function ReplaceClauseFn(
query: Query,
stageIndex: number,
targetClause: Clause,
- newClause: Field,
+ newClause: Clause,
): Query;
export const replaceClause: typeof ReplaceClauseFn = ML.replace_clause;
diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs
index 782917d771f..79dffcd0562 100644
--- a/src/metabase/lib/js.cljs
+++ b/src/metabase/lib/js.cljs
@@ -106,8 +106,10 @@
(defn ^:export order-by-clause
"Create an order-by clause independently of a query, e.g. for `replace` or whatever."
- [a-query stage-number x]
- (lib.order-by/order-by-clause a-query stage-number (lib.normalize/normalize (js->clj x :keywordize-keys true))))
+ ([a-query stage-number x]
+ (order-by-clause a-query stage-number x nil))
+ ([a-query stage-number x direction]
+ (lib.order-by/order-by-clause a-query stage-number (lib.normalize/normalize (js->clj x :keywordize-keys true)) direction)))
(defn ^:export order-by
"Add an `order-by` clause to `a-query`. Returns updated query."
diff --git a/src/metabase/lib/order_by.cljc b/src/metabase/lib/order_by.cljc
index 8edcbc7eadc..93d0b1c7905 100644
--- a/src/metabase/lib/order_by.cljc
+++ b/src/metabase/lib/order_by.cljc
@@ -56,20 +56,32 @@
[_query _stage-number x]
(lib.options/ensure-uuid [:asc (lib.ref/ref x)]))
-(defn order-by-clause
- "Create an order-by clause independently of a query, e.g. for `replace` or whatever."
- ([x]
- (fn [query stage-number]
- (order-by-clause query stage-number x)))
- ([query stage-number x]
- (->order-by-clause query stage-number x)))
-
(mu/defn ^:private with-direction :- ::lib.schema.order-by/order-by
"Update the direction of an order by clause."
[clause :- ::lib.schema.order-by/order-by
direction :- ::lib.schema.order-by/direction]
(assoc (vec clause) 0 direction))
+(mu/defn order-by-clause
+ "Create an order-by clause independently of a query, e.g. for `replace` or whatever."
+ ([x]
+ (fn [query stage-number]
+ (order-by-clause query stage-number x nil)))
+ ([x
+ direction :- [:maybe [:enum :asc :desc]]]
+ (fn [query stage-number]
+ (order-by-clause query stage-number x direction)))
+ ([query :- ::lib.schema/query
+ stage-number :- [:maybe :int]
+ x]
+ (order-by-clause query stage-number x nil))
+ ([query :- ::lib.schema/query
+ stage-number :- [:maybe :int]
+ x
+ direction :- [:maybe [:enum :asc :desc]]]
+ (-> (->order-by-clause query (or stage-number -1) x)
+ (with-direction (or direction :asc)))))
+
(mu/defn order-by
"Add an MBQL order-by clause (i.e., `:asc` or `:desc`) from something that you can theoretically sort by -- maybe a
Field, or `:field` clause, or expression of some sort, etc.
diff --git a/test/metabase/lib/order_by_test.cljc b/test/metabase/lib/order_by_test.cljc
index 32e72626d93..4623dff7459 100644
--- a/test/metabase/lib/order_by_test.cljc
+++ b/test/metabase/lib/order_by_test.cljc
@@ -72,10 +72,15 @@
(deftest ^:parallel order-by-field-metadata-test
(testing "Should be able to create an order by using raw Field metadata"
- (is (=? [:asc
- {:lib/uuid string?}
- [:field {:lib/uuid string?} (meta/id :venues :id)]]
- (lib/order-by-clause {} -1 (lib.metadata/field meta/metadata-provider nil "VENUES" "ID"))))))
+ (let [query (lib/query-for-table-name meta/metadata-provider "VENUES")]
+ (is (=? [:asc
+ {:lib/uuid string?}
+ [:field {:lib/uuid string?} (meta/id :venues :id)]]
+ (lib/order-by-clause query -1 (lib.metadata/field meta/metadata-provider nil "VENUES" "ID"))))
+ (is (=? [:desc
+ {:lib/uuid string?}
+ [:field {:lib/uuid string?} (meta/id :venues :id)]]
+ (lib/order-by-clause query -1 (lib.metadata/field meta/metadata-provider nil "VENUES" "ID") :desc))))))
(deftest ^:parallel append-order-by-field-metadata-test
(testing "Should be able to add an order by using raw Field metadata"
--
GitLab