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

[MLv2] Implement automatic-insights drill #36443

Merged
merged 4 commits into from
Dec 7, 2023

Conversation

bshepherdson
Copy link
Contributor

@bshepherdson bshepherdson commented Dec 5, 2023

[MLv2] Implement automatic-insights drill

Automatic insights drills have some unusual conditions.
This adds metabase.lib.metadata/editable? and checks it before
returning any drills.

On settings:
In the app, MetabaseSettings is a global singleton and the settings
are sometimes updated in place. In the JS testing environment several
mock settings instances can exist at once, and the global singleton does
not necessarily have the values we want for any given test.

This PR makes metabase.lib.metadata/setting check for describe and
it globals to see if this is the testing environment, and to trust the
metadata's settings in that case.

Implements the BE side of #33558.

Automatic insights drills have some unusual conditions.
This adds `metabase.lib.metadata/editable?` and checks it before
returning any drills.

On settings:
In the app, `MetabaseSettings` is a global singleton and the settings
are sometimes updated in place. In the JS testing environment several
mock settings instances can exist at once, and the global singleton does
not necessarily have the values we want for any given test.

This PR makes `metabase.lib.metadata/setting` check for `describe` and
`it` to see if we're in the testing environment, and to trust the
metadata's `settings` in that case.
Copy link
Contributor Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@bshepherdson bshepherdson requested a review from a team December 5, 2023 22:09
@metabase-bot metabase-bot bot added the .Team/QueryProcessor :hammer_and_wrench: label Dec 5, 2023
@bshepherdson bshepherdson added this to the 0.49 milestone Dec 5, 2023
@bshepherdson bshepherdson added the no-backport Do not backport this PR to any branch label Dec 5, 2023
Copy link
Member

@camsaul camsaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving just so we can get this in and keep the train rolling, but your tests look busted, it would be good to actually test automatic insights drills rather than just testing underlying records a second time

@bshepherdson bshepherdson enabled auto-merge (squash) December 7, 2023 01:15
@bshepherdson bshepherdson merged commit b95eb1a into master Dec 7, 2023
106 checks passed
@bshepherdson bshepherdson deleted the mblib-drills-auto-insights-integration branch December 7, 2023 01:36
bshepherdson added a commit that referenced this pull request Dec 7, 2023
bshepherdson added a commit that referenced this pull request Dec 7, 2023
This was introduced by #36443.
bshepherdson added a commit that referenced this pull request Dec 8, 2023
qnkhuat pushed a commit that referenced this pull request Dec 12, 2023
Automatic insights drills have some unusual conditions.
This adds `metabase.lib.metadata/editable?` and checks it before
returning any drills.

On settings:
In the app, `MetabaseSettings` is a global singleton and the settings
are sometimes updated in place. In the JS testing environment several
mock settings instances can exist at once, and the global singleton does
not necessarily have the values we want for any given test.

This PR makes `metabase.lib.metadata/setting` check for `describe` and
`it` to see if we're in the testing environment, and to trust the
metadata's `settings` in that case.
qnkhuat pushed a commit that referenced this pull request Dec 12, 2023
qnkhuat pushed a commit that referenced this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/QueryProcessor :hammer_and_wrench:
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants