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
Expressions in AggregationPicker without legacy query #36031
Conversation
…ause-for-legacy-expression
…y-expression-clause-filters-and-aggregations
let expression = null; | ||
let expressionClause = null; |
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.
Adjusting to match onChange
and onCommit
props in ExpressionEditorTextfield
which were/are typed with null
.
...query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.tsx
Show resolved
Hide resolved
frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx
Outdated
Show resolved
Hide resolved
…947-legacy-expression-clause-filters-and-aggregations
This reverts commit 254913d.
…947-legacy-expression-clause-filters-and-aggregations
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.
LGTM 🤞
@kamilmielnik Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
* Add expression-clause-for-legacy-expression Fixes #34830. * Add legacy-expression-for-expression-clause * Fix wrapper function name * Include ExpressionClause in Aggregatable type * Use Aggregatable type in aggregate function * Rename query to legacyQuery in Expressions (part 1) * Rename query to legacyQuery in Expressions (part 2) * Rename query to legacyQuery in Expressions (part 3) * Support MLv2 expressionClause in processSource and ExpressionWidget * Revert changes to ExpressionStep as it's out of scope of this task * Use new expressionClause in ExpressionWidget * Use onSelect instead of onSelectLegacy in handleExpressionChange * Remove onSelectLegacy from AggregationPicker * Use nullish coalescing operator * Assert new onChangeExpression argument in test * Assert new onChangeExpression argument in test * Update AggregationPicker tests * Replace toEqual + expect.objectContaining with toMatchObject * Add stageIndex to setup, use destructuring for onSelect arguments * Deal with awkward assertions * Use props in a conservative way * Replace legacyQuery.database() with query + metadata * Add a TODO comment * Add clause prop to AggregationPicker * Migrate clause name * Replace 3rd argument in onChangeExpression with onChangeExpressionClause * Use destructuring * Allow AggregationClause, drop expression from ExpressionEditorTextfield * Make withExpressionName work with AggregationClause * Rename expressionClause to clause * Update tests with new interface * Use overloading instead of generics * Add function body * Rename operator to clause * Revert "Use overloading instead of generics" This reverts commit 3953f85. * Fix tests failing due to useSelect usage in AggregationPicker * Omit aggregation options converting expressions to legacy Fixes #36120. * Remove temporary hack * Get rid of props spread * Migrate isExpressionEditorInitiallyOpen to MLv2 * Omit aggregation options converting expressions to legacy Fixes #36120. * Normalize legacy expressions as MBQL expressions * Fix isExpressionEditorInitiallyOpen * Update expressionName signature * Pass props in ExpressionStep tests in a usual way * Drop a conditional statement * Bring back expression prop for backwards-compatibility * Update ExpressionWidget validation & tests * Bring back using expression in componentWillReceiveProps * Bring back removed assertion * Remove legacyQuery prop from AggregationPicker * Add a test case for isExpressionEditorInitiallyOpen (which uses Lib.expressionName) * Fix legacyQuery creation * Add assertion for expression name * Fix committing expression with done button * Improve assertions in tests * Make with-expression-name set the display-name of the clause Fixes #36167. This is needed to be able to set/change the display name of custom aggregation columns too. * Add expression name assertion * Add expression name assertion * Add expression name assertion * Add expression name assertion * Add expression name assertion * Add expression name assertion * Add expression name assertion * Unskip an e2e test * Add expression name assertion * Add expression name assertion * Use displayInfo instead of displayName - see #36203 (comment) * Deprecate displayName in favor of displayInfo - see https://metaboat.slack.com/archives/C0645JP1W81/p1701341786914219?thread_ts=1701335646.088359&cid=C0645JP1W81 * Use clearer notation * Format code * Use displayInfo instead of expressionName in tests * Add isNamed attribute to ColumnDisplayInfo and ClauseDisplayInfo * Use displayInfo instead of expressionName in AggregationPicker * Drop expressionName * Drop redundant cast * Simplify isExpressionEditorInitiallyOpen interface * Use display-info with :named? instead of expression-name * Improve comment * Add :named? to the schema of display-info * Use conditional types for ExpressionWidget prop * Use getNotebookStep * Simplify condition * Simplify assertions * Use .lastCall * Remove isNamed attribute from ColumnDisplayInfo * Use .lastCall * Fix typing * Bring back legacyQuery prop to AggregationPicker * Fix isExpressionEditorInitiallyOpen for new clauses * Fix unit tests * Remove wrong assertion * Fix assertion * Trigger a change * Revert "Trigger a change" This reverts commit 254913d. * Fix assertion --------- Co-authored-by: Tamás Benkő <tamas@metabase.com>
Closes #35947
Blocked by
#36075,#36120, #36167,#36168Target branch includes fixes for #36167.
Description
AggregationPicker
no longer useslegacyQuery
orexpression
.ExpressionWidget
,ExpressionEditorTextfield
&AggregationPicker
still receivelegacyQuery
andexpression
props for backwards-compatibility but now they also work without them.How to test it