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.

Sign up for GitHub

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

Jump to bottom

Remove QuestionDataSelector #42562

Merged
merged 1 commit into from
May 14, 2024
Merged

Remove QuestionDataSelector #42562

merged 1 commit into from
May 14, 2024

Conversation

kamilmielnik
Copy link
Contributor

@kamilmielnik kamilmielnik commented May 13, 2024 •
edited

Closes #42559

Description

It took me a while to track down how QuestionDataSelector was used.
It was being rendered when isNewQuestion && queryBuilderMode === "view" but I could never achieve this condition in the current Metabase version - it's not possible for isNewQuestion to be true when queryBuilderMode === "view".

It was possible in the past though - Metabase 0.35 had a "Simple question" mode where this condition evaluated to true (see video of how things used to looked like) and there was no trigger button for the data source picker. Since this mode no longer exists, I think it's safe to get rid of this code.

I also created a temporary PR (#42561) to run CI for this change against master (the data-picker feature branch has almost a hundred failing test right now) and everything was green (run in replay.io).

@kamilmielnik kamilmielnik added no-backport Do not backport this PR to any branch .Team/QueryingComponents labels May 13, 2024
@kamilmielnik kamilmielnik self-assigned this May 13, 2024
@kamilmielnik kamilmielnik changed the base branch from master to data-picker May 13, 2024 11:56
@kamilmielnik kamilmielnik marked this pull request as ready for review May 13, 2024 12:25
@kamilmielnik kamilmielnik requested a review from a team May 13, 2024 12:25
uladzimirdev
uladzimirdev approved these changes May 13, 2024
View reviewed changes
Copy link
Contributor

@uladzimirdev uladzimirdev left a comment

Choose a reason for hiding this comment

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

I'd not merge it before @ranquild's review

@replay-io replay.io
Copy link

replay-io bot commented May 13, 2024 •
edited

Status Complete ↗︎
Commit c748497
Results
❌ 105 Failed
⚠️ 4 Flaky
✅ 2385 Passed

@ranquild
Copy link
Contributor

@kamilmielnik it should be used. I think when opening a ad-hoc question from url

ranquild
ranquild reviewed May 13, 2024
View reviewed changes
Copy link
Contributor

@ranquild ranquild left a comment

Choose a reason for hiding this comment

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

I would leave it until CI is fixed

@kamilmielnik
Copy link
Contributor Author

I think when opening a ad-hoc question from url

That's not it - in that case isNewQuestion will be false and QuestionDataSelector won't be used.

isNewQuestion will be true only if source table has not been selected.
Only once source table has been selected queryBuilderMode can change to "view".
So it looks like this condition will never be satisfied.

I also tried a case with ad-hoc question when user does not have access to source table.

I would leave it until CI is fixed

I tested this change against master (see last paragraph in PR description) and everything was green.

@kamilmielnik it should be used.

If that's true - we can prove it.
I failed to do so though.

@ranquild are you sure this is needed?

ranquild
ranquild approved these changes May 13, 2024
View reviewed changes
Copy link
Contributor

@ranquild ranquild left a comment

Choose a reason for hiding this comment

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

@kamilmielnik not, I'm not sure. I can approve this now but we might need to revert this later on.

@kamilmielnik
Copy link
Contributor Author

@kamilmielnik not, I'm not sure. I can approve this now but we might need to revert this later on.

I asked in Slack. I'll merge this PR tomorrow if there are no objections.

@kamilmielnik
Copy link
Contributor Author

I asked in Slack. I'll merge this PR tomorrow if there are no objections.

Ryan and Nemanja provided their inputs and we still can't think of a scenario where this code would be needed.
I'm merging this PR 🤞

@kamilmielnik kamilmielnik merged commit 58c6e10 into data-picker May 14, 2024
180 of 208 checks passed
@kamilmielnik kamilmielnik deleted the 42559-remove-question-data-selector branch May 14, 2024 14:40
@kamilmielnik kamilmielnik mentioned this pull request May 14, 2024
Closed
kamilmielnik added a commit that referenced this pull request May 17, 2024
@kamilmielnik @ranquild
Data picker (#41175)
784e869 
* Fix nil schema in /api/table/:id/query_metadata

* Fix nil schema in /api/table/:id/query_metadata

* Fix nil schema in /api/table/:id/query_metadata

* Fix nil schema in /api/table/:id/query_metadata

* Fix nil schema in /api/table/:id/query_metadata

* Revert "Fix nil schema in /api/table/:id/query_metadata"

This reverts commit 38ac092.

* Revert "Fix nil schema in /api/table/:id/query_metadata"

This reverts commit 2b8fe78.

* Revert "Fix nil schema in /api/table/:id/query_metadata"

This reverts commit 219506f.

* Revert "Fix nil schema in /api/table/:id/query_metadata"

This reverts commit 9430478.

* Revert "Fix nil schema in /api/table/:id/query_metadata"

This reverts commit 8c6cdd9.

* Table Picker (#40509)

* Generic types

* Generic types

* Generic types + make generateKey a prop

* Generic types

* Extract AutoScrollBox.styled.tsx

* Move AutoScrollBox to a separate directory

* Generic types

* Generic types

* Generic types

* Unhookify searchFilter

* Generic types

* Generic types

* Generic types

* Generic types

* Generic types

* Generic types

* Generic types

* Generic types

* Finish dealing with a cast

* Destructure import

* Inline type

* Generic types

* Remove cast

* Remove commented code

* Remove redundant fallback

* Move CollectionPicker out of EntityPicker

* Avoid as unknown

* Remove TODOs

* Rename TisFolder to IsFolder

* Fix any

* Fix anys

* Remove NestedItemPicker's storybook

* Remove a cast

* Remove a cast

* Remove a cast

* Revert "Remove a cast"

This reverts commit b762d0f.

* Use extends SearchModelType instead of extends string everywhere

* Revert "Use extends SearchModelType instead of extends string everywhere"

This reverts commit 708190d.

* Fix SearchResult["available_models"] type

* Handle options.allowCreateNew

* Add missing description attribute to Database type

* Move allowCreateNew to CollectionPickerOptions

* Add missing description attribute in Database mock

* Add types

* Add TableList component

* Add SchemaList component

* Fix naming

* Add DatabaseList component

* Add NotebookDataItemPickerResolver

* Fix typing

* Add TablePicker and NotebookDataPickerModal

* Update types

* Fix types

* Add folder type

* Fix initial state

* Update title

* Make query model-dependent

* Render tables

* Fix schema icon

* Adjust key generation

* Rename utilts to utils

* Use ItemList error prop

* Use value prop

* Rename NotebookDataPickerModal to DataPickerModal

* Automatically open datapicker if there is no value

* Highlight current item

* Rename value to initial value

* Fix item highlighting

* Rework TablePicker state

* Leave TODOs for names

* Fix selecting items

* Fix highlighting selected item

* Hide confirmation button

* Add fetchMetadata to tableApi

* Fix Table['schema'] type

* Fetch table metadata upon selection

* Remove options from DataPickerModal

* Fix collectionId typing in Question

* Add collectionId prop

* Remove title prop

* Add todo

* Fix confirmation button

* Remove old DataSourceSelector

* Bring back useSchemaListQuery

* Fix crash

* Fix picker not opening

* Hide dbs list and schemas list if there's only 1

* Remove unused ref

* Remove unused DataPickerListResolver

* Remove unused types

* Change conditional rendering to allow for loading state

* Revert collection-related changes

* Use RTK

* Avoid using stale data

* Fix highlighted db

* Remove todo

* Remove unused type

* Remove useSchemaListQuery

* Rename Value to TablePickerValue

* Move isValueEqual to utils and rename it to isTablePickerValueEqual

* Improve TODO

* Remove unused options prop

* Introduce tablePickerValueFromTable

* Avoid having 2 table metadata requests

* Update comment

* Update comment

* Remove unused function

* Allow null in tablePickerValueFromTable

* Extract helpers

* Rename schemaId to schemaName

* Remove description

* Improve naming

* Simplify types

* Add explanatory comments

* Fall back to empty state when there are no items

* Account for null schemas

* Account for null schema in tests

* Revert "Account for null schema in tests"

This reverts commit 0c96d1c.

* Revert "Account for null schemas"

This reverts commit 4f5c4e9.

* Improve typing around schema name

* Refactor auto-select logic

* Refactor

* Avoid comment

* Make useAutoSelectOnlyItem generic

* Do not enforce presence of at least 1 tab with TypeScript

* Fix types

* Remove obsolete comment

* Post-merge fix

* Integrate QuestionPicker in DataPicker (#42268)

* Add question and model pickers to Data Picker

* Add model attribute to data picker value

* Introduce DataPickerValue

* Start with the right tab when opening data picker modal

* Do not use stale sourceCard value

* Fix switching tabs when a table is selected

* Use undefined instead of null for consistency

* Do not show models/questions tabs when there are no models/questions

* Fix selecting models/questions in search results

* Remove dead code

* Remove dead code

* Ignore selecting collections

* Refactor useHasModels and useHasQuestions into useAvailableData

* Use models param to speed up the query

* Do not require `onConfirm` prop in `EntityPickerModal` (#42317)

* Make onConfirm prop optional

* Add a test for onConfirm prop assertion

* Table does not appear selected in single-schema dbs (#42336)

* Convert useAutoSelectOnlyItem arguments to param object

* Disable auto-selection when there already is a selection

* Use `DataPicker` in `JoinTablePicker` (#42330)

* Refactor DataStep:
- rename handleTableSelect to handleTableChange
- extract value and memoize it

* Remove redundant fragment

* Add new DataPicker props

* Use DataPickerModal in JoinTablePicker

* Remove initialTab prop since the default tab is always the models tab anyway

* Limit table selection to particular database

* Limit search results to a given database

* Support databaseId in useAvailableData

* Filter collection items by databaseId

* Use databaseId prop only for CollectionItemList component

* Fix types

* Fix filtering

* Use useLatest for onChange

* Fix typing

* Introduce shouldShowItem and use it instead of databaseId prop

* Improve PickerInfo type

* Use MLv2 to generate DataPickerModal value in JoinTablePicker

* Fix title

* Add isModel to TableDisplayInfo

* Add a hack to populate query metadata with info that table is a model

* Remove NewQuestionView & QuestionDataSelector (#42562)

* Temporarily disable recents tab in the data picker

* Update Data Picker tests (#42565)

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Fix metadata loading

* Update test

* Update test

* Update tests

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update tests

* Update test

* Update test

* Update test and make it pass

* Update test

* Update tests

* Update test

* Use entityPickerModal() instead of modal()

* Use entityPickerModal() instead of modal()

* Update test

* Update test

* Update test

* Revert change that broke tests

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Remove test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update tests

* Update test

* Update test

* Rename createDatabaseIdItemFilter to createShouldShowItem

* Update test

* Update and skip a test

* Remove test

* Update tests

* Update tests

* Update tests

* Remove test

* Update tests

* Update test

* Update test

* Update test

* Extract utils

* Use helpers

* Extract helpers

* Update test

* Update test

* Update test

* Update test

* Update test

* Filter out collections that do not have card or dataset

* Improve filtering

* Always show root and personal collections, see #42687

* Remove failing assertion

* Update tests

* Use entityPickerModalTab

* Accept strings in pickEntity

* Use entityPickerModalLevel

* Use entityPickerModalTab

* Use entityPickerModalTab

* Update tests

* Update test

* Update tests

* Update test

* Fix assertion

* Update test

* Update tests

* Update test

* Update tests

* Update tests

* Update test

* Update test

* Update tests
- ensure database list gets loaded like in a real app

* Update test

* Update test

* Integrate Recents tab in Data Picker (#42821)

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Fix metadata loading

* Update test

* Update test

* Update tests

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update tests

* Update test

* Update test

* Update test and make it pass

* Update test

* Update tests

* Update test

* Use entityPickerModal() instead of modal()

* Use entityPickerModal() instead of modal()

* Update test

* Update test

* Update test

* Revert change that broke tests

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Remove test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update test

* Update tests

* Update test

* Update test

* Rename createDatabaseIdItemFilter to createShouldShowItem

* Update test

* Update and skip a test

* Remove test

* Update tests

* Update tests

* Update tests

* Remove test

* Update tests

* Update test

* Update test

* Update test

* Extract utils

* Use helpers

* Extract helpers

* Update test

* Update test

* Update test

* Update test

* Update test

* Filter out collections that do not have card or dataset

* Improve filtering

* Always show root and personal collections, see #42687

* Remove failing assertion

* Update tests

* Use entityPickerModalTab

* Accept strings in pickEntity

* Use entityPickerModalLevel

* Use entityPickerModalTab

* Use entityPickerModalTab

* Update tests

* Update test

* Update tests

* Update test

* Fix assertion

* Update test

* Update tests

* Update test

* Update tests

* Update tests

* Update test

* Update test

* Update tests
- ensure database list gets loaded like in a real app

* Update test

* Enable recents tab and update tests

* Update test

* Update tests

* Add Recents-related assertions

* Update test

* Clean up mocks

---------

Co-authored-by: Alexander Polyankin <alexander.polyankin@metabase.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ranquild ranquild ranquild approved these changes

@uladzimirdev uladzimirdev uladzimirdev approved these changes

Assignees

@kamilmielnik kamilmielnik

Labels
no-backport Do not backport this PR to any branch .Team/QueryingComponents
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove QuestionDataSelector
3 participants
@kamilmielnik @ranquild @uladzimirdev