-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expressions in AggregationPicker without legacy query #36031
Changes from all commits
e48ab06
e4df56e
e93d114
76054bf
0994d76
55b4f31
95697f7
d6eb1dc
eb24d0d
3233e98
9ffa599
3d59ab3
3e4fb53
4eb9eda
3eb2f3c
cadf4ed
20e9406
248009a
35dbf4b
7ae92bd
8646317
71ac3aa
30b9b27
2f50b3f
24a131c
5c75c83
d64c7c4
89041b9
39496c1
221e950
0573ab3
a9d7b5b
079abb5
7658ec9
1f13259
94bc358
f36fc18
3953f85
2074e8b
ee9d538
f18cd75
7baa434
f28b10a
bdad147
1e2485c
505d11f
bacf371
95ddd4c
58bd581
c4866fd
8afeac3
7f7152e
3eb2181
c36cebd
594b389
dcd7781
0a0802a
f7d14cf
92aa48f
e719fc2
0aec027
45a50d3
b5bfee8
db71907
3b8f15b
fd32c5f
33aec8f
e80ac60
6a0c758
2795c3d
9f85ea9
a7a9ec6
49dfc00
a7ed491
fc35005
da11fb9
42d5ff1
9acc7ba
91be09a
cb68901
f699c90
73283d9
2d6ca04
31ce8e7
86c6a80
3796af4
1ee3df6
04e4c16
0bfcedd
c3e015e
f6d7c0a
7cf0bb7
e6a0bed
8780eee
98ab29f
34b87a2
dd0da11
a0777d8
8d14a11
3ef1bcb
da95422
277766a
e582696
db821e0
254913d
a76248a
13571b1
6563742
f6901a6
24937fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,8 @@ describe("issue 17512", () => { | |
"CE", | ||
); | ||
|
||
cy.findByTestId("aggregate-step").contains("CE").should("exist"); | ||
|
||
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage | ||
cy.findByText("Pick a column to group by").click(); | ||
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage | ||
|
@@ -33,10 +35,8 @@ describe("issue 17512", () => { | |
expect(body.error).to.not.exist; | ||
}); | ||
|
||
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage | ||
cy.findByText("CE"); | ||
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage | ||
cy.findByText("CC"); | ||
cy.findAllByTestId("header-cell").contains("CE").should("exist"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: actually |
||
cy.findAllByTestId("header-cell").contains("CC").should("exist"); | ||
}); | ||
}); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,10 +21,9 @@ export function expression( | |
return ML.expression(query, stageIndex, expressionName, clause); | ||
} | ||
|
||
export function withExpressionName( | ||
clause: ExpressionClause, | ||
newName: string, | ||
): ExpressionClause { | ||
export function withExpressionName< | ||
Clause extends AggregationClause | ExpressionClause, | ||
>(clause: Clause, newName: string): Clause { | ||
Comment on lines
+24
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could not make it work with overloads, TS would complain when calling the function in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I expect one letter generic, otherwise it looks like a type and it took me for 10s to understand where is this Clause definfed 😄 |
||
return ML.with_expression_name(clause, newName); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
import { t } from "ttag"; | ||
|
||
import * as Lib from "metabase-lib"; | ||
|
||
import { parse, adjustBooleans } from "./recursive-parser"; | ||
import { resolve } from "./resolver"; | ||
|
||
|
@@ -31,13 +33,23 @@ export function processSource(options) { | |
} | ||
}; | ||
|
||
const { source, startRule } = options; | ||
const { source, query, stageIndex, startRule } = options; | ||
|
||
let expression; | ||
let expression = null; | ||
let expressionClause = null; | ||
Comment on lines
+38
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adjusting to match |
||
let compileError; | ||
try { | ||
const parsed = parse(source); | ||
expression = adjustBooleans(resolve(parsed, startRule, resolveMBQLField)); | ||
|
||
// query and stageIndex are not available outside of notebook editor (e.g. in Metrics or Segments). | ||
if (query && typeof stageIndex !== "undefined") { | ||
expressionClause = Lib.expressionClauseForLegacyExpression( | ||
query, | ||
stageIndex, | ||
expression, | ||
); | ||
} | ||
} catch (e) { | ||
console.warn("compile error", e); | ||
compileError = e; | ||
|
@@ -46,6 +58,7 @@ export function processSource(options) { | |
return { | ||
source, | ||
expression, | ||
expressionClause, | ||
compileError, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why it was skipped - the test passes, the issue is closed (#28193).