Skip to content
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

[WIP] [Epic] Show columns from all stages in the dashboard filters #47167

Open
wants to merge 58 commits into
base: master
Choose a base branch
from

Conversation

kamilmielnik
Copy link
Contributor

@kamilmielnik kamilmielnik commented Aug 23, 2024

Closes #46519
Closes #19744

Description

Integration branch for #46519


⚠️ currently not a draft only to run CI ⚠️

…gation columns (#46670)

* Extract getFilterStageIndexes

* Update comment

* Include 2 last stages in filterable columns in getParameterColumns

* Move getJoinQueryHelpers to test-helpers

* Add basic query creation

* Add breakout columns

* Make tableName optional

* Implement 1-stage test, refactor assertions

* Rename

* Move filter utils up

* Use getGroupItems and appendStageIfAggregated in getFilterableColums

* Introduce Lib.filterGroups

* Revert "Introduce Lib.filterGroups"

This reverts commit 5f8b2d0.

* Don't reuse getGroupItems or appendStageIfAggregated

* Move getFilterStageIndexes out of Lib

* Format code

* Revert types move

* Revert functions move

* Improve diff

* Group dashcard mapping options

* Adjust existing getParameterColumns usages (except unit tests)

* Remove redundant call

* Fix filtering

* Update unit tests

* Format code, remove TODO

* Add stage index assertions

* Add model test for 1-stage query

* Add partial boilerplate for 2-stage query

* Add a 2-stage case

* Add a 2-stage case for model

* Update unit test

* Fix click behavior mapping

* Remove duplicated functions

* Remove complex return type

* Move appendStageIfAggregated and getFilterStageIndexes to metabase-lib/filter.ts

* Re-group columns based on stage index instead of using ML-returned references for comparison

* Add new options to dimension types

* Use the new stage-number option

* Rename appendStageIfSummarized to ensureFilterStage and add explanatory comment
kamilmielnik and others added 20 commits August 26, 2024 21:10
* Extract getFilterStageIndexes

* Update comment

* Include 2 last stages in filterable columns in getParameterColumns

* Move getJoinQueryHelpers to test-helpers

* Add basic query creation

* Add breakout columns

* Make tableName optional

* Implement 1-stage test, refactor assertions

* Rename

* Move filter utils up

* Use getGroupItems and appendStageIfAggregated in getFilterableColums

* Introduce Lib.filterGroups

* Revert "Introduce Lib.filterGroups"

This reverts commit 5f8b2d0.

* Don't reuse getGroupItems or appendStageIfAggregated

* Move getFilterStageIndexes out of Lib

* Format code

* Revert types move

* Revert functions move

* Improve diff

* Group dashcard mapping options

* Adjust existing getParameterColumns usages (except unit tests)

* Remove redundant call

* Fix filtering

* Update unit tests

* Format code, remove TODO

* Add stage index assertions

* Add model test for 1-stage query

* Add partial boilerplate for 2-stage query

* Add a 2-stage case

* Add a 2-stage case for model

* Update unit test

* Fix click behavior mapping

* Remove duplicated functions

* Remove complex return type

* Remove unused @ts-expect-error directive
- use createQuestionAndDashboard helper instead of command
- sort imports

* Add boilerplate for query stages test suite

* Add createDashboard

* Try out the new function

* Organize code

* Add descriptions

* Move visitDashboard call out of createDashboard

* Move appendStageIfAggregated and getFilterStageIndexes to metabase-lib/filter.ts

* Re-group columns based on stage index instead of using ML-returned references for comparison

* Add boilerplate for base questions

* Add assertions for mapping options

* Simplify verifyDashcardMappingOptions interface

* Add assertions for all types of parameters

* Update test name

* Format code

* Add new options to dimension types

* Use the new stage-number option

* Add Q2 creation

* Introduce helper constants

* Rename appendStageIfSummarized to ensureFilterStage and add explanatory comment

* Add cases for models

* Extract subroutines

* Add Q3

* Extract createQ2Query and createQ3Query

* Extract createQ1uery, remove createM1

* Optimize verifyPopoverMappingOptions

* Add more assertions

* Add model assertions

* Add tooltip assertion

* Break large functions down

* Restructure tests to prevent memory-related crashes

* Refactor

* Update comment

* Update comment

* Remove createQ1

* Remove createQ2, createQ3

* Refactor

* Refactor

* Comment out failing assertions

* Add Q3 assertions

* Add Q3 assertions

* Add comment

* Introduce createAndVisitDashboardWithQueryMatrix

* Add Q4

* Fix assertions

* Add repro tags

* Add comment

* De-hardcode indexes

* Add Q5-Q8

* Rename

* Add Q5

* Add Q6

* Add Q7 & Q8

* Add Q9

* Fix structure

* Reuse Lib.ensureFilterStage

* Fix unit of time parameters

* Support explicit and implicit stage numbers for temporal-unit params

* Extend test with temporal unit parameter with stage number

* Update unit tests

* Let stage-path handle nil (default) stage-number

---------

Co-authored-by: Tamás Benkő <tamas@metabase.com>
* Allow filtering on columns from all stages in dashboards

* Update tests

* Clean up

* Update unit tests
…pping (#48261)

* Allow filtering on columns from all stages in dashboards

* Update tests

* Clean up

* Update unit tests

* Distinguish multiple "Summaries" column groups

* Fix imports

* Remove redundant attribute

* Update import
* Move filters on nested native stages to parent stage in expand-mbql-params

Similar to the existing handling in move-join-condition-to-source-query.

This fixes filter params that directly target a native stage with a non-negative :stage-number, e.g. when applying
dashboard filters to a card where the card query is native.

Fixes #48258

* Add test for filter params explicitly targeting nested native queries
@NevRA NevRA added the backport Automatically create PR on current release branch on merge label Oct 14, 2024
…ge filters with tests (#48726)

* Add JSDoc for entityPickerModalItem

* Add addToDashboard parameter to saveQuestion

* Add JSDoc for saveQuestion

* Fix import

* Add a test reproducing the bug

* Try to fix unit of time parameters

* Fix wrong stage-index for temporal unit parameters on composed ad hoc questions

* Fix ad-hoc question naming when there is an empty extra filter stage

* Fix unit tests
* Avoid using Lib.ensureFilterStage for pivoted questions

* Fix unit test setup

* Add tests for pivot tables

* Optimize tests

* Test filter modal

* Revert redundant changes
Copy link
Contributor

github-actions bot commented Oct 22, 2024

@kamilmielnik You have modified embedding SDK code. Please make sure the PR title follows Conventional Commits style.
Here are all the supported types that will be shown in the changelog:

  • feat
  • fix
  • perf

These types docs, style, refactor, test, build, and ci are also encouraged for non-changelog related tasks.

Please also make sure to include sdk scope, otherwise, the changelog will not include this task.

For example, these are valid PR titles:

  • feat(sdk): Add interactive dashboard component
  • feat(sdk): Support theming pie chart
  • fix(sdk): Fix static dashboard crash

kamilmielnik and others added 13 commits October 22, 2024 18:07
* Revert "Add a temporary workaround"

This reverts commit 4ea3748.

* Fix some of the issues and unskip some of the E2E tests

* Acknowledge that metrics are transparent

* Fix breakouts by the same column with different temporal-units

* Use non-negative stageIndex for temporal unit parameters

* Add stage if there is an explicit reference to stage after the last

Instead of duplicating the ensure-filter-stage logic, just check if there is a parameter referencing the filter
stage (which is always one after the last in the query). If there is such a reference, add that stage.

---------

Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>
…49067)

* Only require breakouts when adding new stage in ensure-filter-stage

Previously, this function required both breakouts and aggregations.

Closes #48339

* Update e2e tests after changes to Lib.ensureFilterStage

This resolves all-but-one of the TODOs related to #49339

Related to #49022
* Unskip #19744 repro

* Remove sanity checks

* Update comment

* Fix typo

* Remove invalid test

* Ignore pivoted "table" viz and and inline Question.prototype.isPivoted

* Remove test for pivoted table
* Unskip #19744 repro

* Remove sanity checks

* Update comment

* Fix typo

* Remove invalid test

* Ignore pivoted "table" viz and and inline Question.prototype.isPivoted

* Remove test for pivoted table

* Add a test

* Disable columns from non-last stage in click behavior

* Finish the test

* Refactor
* Add test for public dashboard

* Add test for embedded dashboard

* Add comment

* Add test for 2nd stage column

* Add test for 2nd stage aggregation column

* Add test for 2nd stage breakout column

* Add assertions for models

* Handle no auto-pivoting of ad-hoc questions from drill thrus

---------

Co-authored-by: Alexander Polyankin <alexander.polyankin@metabase.com>
@kamilmielnik
Copy link
Contributor Author

Reference for sanity: 20762b2 passes the CI 100%.

@kamilmielnik kamilmielnik changed the title [WIP] [Epic] Allow to use in dashboard filters both pre- and post- last aggregation columns [WIP] [Epic] Show columns from all stages in the dashboard filters Oct 30, 2024
metamben and others added 5 commits November 1, 2024 18:44
…#48828)

* Add JSDoc for entityPickerModalItem

* Add addToDashboard parameter to saveQuestion

* Add JSDoc for saveQuestion

* Fix import

* Add a test reproducing the bug

* Try to fix unit of time parameters

* Fix wrong stage-index for temporal unit parameters on composed ad hoc questions

* Fix ad-hoc question naming when there is an empty extra filter stage

* Fix unit tests

* Fix non-unique keys

* Add stage-number to click behavior dimension target

* Call Lib.ensureFilteringStage in click behavior

* Use describeEE because click behavior tests use setTokenFeatures("all")

* Stop testing implementation details

* Stop testing implementation details

* Stop testing implementation details

* Modernize test and make it less flaky

* Address flake source

* Fix SAMPLE_DB_ID

* Unskip repro for #19744

* Add a test

* Update test name

* Introduce verifyNotebookQuery

* Use verifyNotebookQuery

* Use verifyNotebookQuery

* Move verifyNotebookQuery to helpers

* Assert items count in verifyNotebookQuery

* Extract helper functions

* Rename helpers

* Refactor

* Consistent comments

* Extract verifyVizTypeIsLine

* Add scrollIntoView calls for extra safety

* Add TODO

* Use verifyNotebookQuery

* Make stages optional

* Add createMultiStageQuery

* Remove redundant scrollIntoView calls
- We don't need them because the assertions are not using "be.visible"

* Improve test setup

* Add more assertions

* Add more assertions

* Add basic join support to verifyNotebookJoins

* Unnest code

* Implement join assertions

* Implement expressions and sort assertions

* Simplify code

* Add commented-out assertions

* Yes, it is correct
- it's stageIndex which sourceColumn comes from in sourceQuery

* Conditionally apply filter stage

* Allow mapping to all stages in click behavior

* Remove redundant argument

* Simplify diff

* Simplify diff

* Update test setup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Automatically create PR on current release branch on merge do not merge .Team/Querying
Projects
None yet
4 participants