Use `DataPicker` in `JoinTablePicker` #42330

kamilmielnik · 2024-05-07T12:55:43Z

How to verify

Create a new question and choose data source
Try to join something to it
- the selection of questions/tables/models should be limited to the database of data source selected in step 1
- the database selection step in "Tables" tab should not be visible
- "Models" and "Saved questions" tabs should not be visible if data source database does not have any models or questions respectively

Note: changing data source of a question to a one from a different database should remove the no-longer-valid join clause, but it does not work - I reported it separately: #42385

Demo

2024-05-08.21-45-07.mp4

- rename handleTableSelect to handleTableChange - extract value and memoize it

… anyway

replay-io · 2024-05-08T15:04:03Z

Status	In Progress ↗︎ 51 / 52
Commit	`0442440`
Results	❌ 102 Failed ⚠️ 12 Flaky ✅ 2360 Passed

iethree · 2024-05-08T17:40:21Z

frontend/src/metabase/common/components/CollectionPicker/components/CollectionItemList.tsx

+  const filteredItems = items.filter(item => {
+    if (isNullOrUndefined(databaseId) || isNullOrUndefined(item.database_id)) {
+      return true;
+    }
+
+    return item.database_id === databaseId;
+  });


I don't feel super-strongly about this, but rather than build join-specific logic into this component, I'd rather pass a shouldShowItem (or whatever) filter function kind of like shouldDisableItem that would be more generally useful

Done: 2b944f0

QuestionPicker & EntityPickerModal still accept databaseId prop due to it being useful in EntityPickerSearchInput.

ranquild

Inline comment

ranquild · 2024-05-09T12:27:53Z

...etabase/query_builder/components/notebook/steps/JoinStep/JoinTablePicker/JoinTablePicker.tsx

-  const tableFilter = (table: Table) => !tableId || table.db_id === databaseId;
+  const sourceCardId = pickerInfo?.cardId;
+  const { data: sourceCard } = useGetCardQuery(
+    sourceCardId ? { id: sourceCardId } : skipToken,


This seems wrong. joinable is already a card. We need to fix initialization to be able to use it directly

4c9fc81 + 721568b + 8a58579

kamilmielnik · 2024-05-10T09:40:22Z

...etabase/query_builder/components/notebook/steps/JoinStep/JoinTablePicker/JoinTablePicker.tsx

+    if (typeof tableId === "string") {
+      await dispatch(
+        Questions.actions.fetch({
+          id: getQuestionIdFromVirtualTableId(tableId),
+        }),
+      );
+    }


Hacky way to include changes from metrics-v2 branch without changing many things

See Slack.

Without it Lib.displayInfo and Lib.pickerInfo won't properly recognize the joined thing as a model, and isModel will not be true.

* 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>

Refactor DataStep:

2e67ba1

- rename handleTableSelect to handleTableChange - extract value and memoize it

kamilmielnik added no-backport Do not backport this PR to any branch .Team/QueryingComponents labels May 7, 2024

kamilmielnik self-assigned this May 7, 2024

kamilmielnik changed the base branch from master to data-picker May 7, 2024 12:55

kamilmielnik added 13 commits May 7, 2024 20:31

Remove redundant fragment

a635cf4

Add new DataPicker props

46f6379

Use DataPickerModal in JoinTablePicker

77fac6d

Remove initialTab prop since the default tab is always the models tab…

426da19

… anyway

Limit table selection to particular database

1060d02

Merge branch 'data-picker' into 40952-data-picker-in-join-table-picker

58872be

Limit search results to a given database

31eed71

Support databaseId in useAvailableData

8386850

Filter collection items by databaseId

e76541b

Use databaseId prop only for CollectionItemList component

adee125

Fix types

a9cda5a

Fix filtering

ff192f8

Use useLatest for onChange

90d5031

kamilmielnik marked this pull request as ready for review May 8, 2024 14:48

kamilmielnik requested review from iethree and a team May 8, 2024 14:48

Fix typing

be75d28

iethree reviewed May 8, 2024

View reviewed changes

Introduce shouldShowItem and use it instead of databaseId prop

2b944f0

kamilmielnik requested a review from iethree May 9, 2024 09:09

ranquild reviewed May 9, 2024

View reviewed changes

kamilmielnik added 3 commits May 9, 2024 21:04

Improve PickerInfo type

4c9fc81

Use MLv2 to generate DataPickerModal value in JoinTablePicker

721568b

Fix title

5367f58

kamilmielnik requested a review from camsaul as a code owner May 9, 2024 15:06

Add isModel to TableDisplayInfo

8a58579

kamilmielnik requested a review from ranquild May 9, 2024 15:27

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

0442440

kamilmielnik commented May 10, 2024

View reviewed changes

ranquild approved these changes May 10, 2024

View reviewed changes

kamilmielnik merged commit 31f7aaf into data-picker May 10, 2024
92 of 110 checks passed

kamilmielnik deleted the 40952-data-picker-in-join-table-picker branch May 10, 2024 11:28

kamilmielnik mentioned this pull request May 10, 2024

Use DataPicker in JoinTablePicker #40952

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `DataPicker` in `JoinTablePicker` #42330

Use `DataPicker` in `JoinTablePicker` #42330

kamilmielnik commented May 7, 2024 •

edited

replay-io bot commented May 8, 2024 •

edited

iethree May 8, 2024 •

edited

kamilmielnik May 9, 2024

ranquild left a comment

ranquild May 9, 2024

kamilmielnik May 9, 2024

kamilmielnik May 10, 2024

Use DataPicker in JoinTablePicker #42330

Use DataPicker in JoinTablePicker #42330

Conversation

kamilmielnik commented May 7, 2024 • edited

How to verify

Demo

replay-io bot commented May 8, 2024 • edited

iethree May 8, 2024 • edited

Choose a reason for hiding this comment

kamilmielnik May 9, 2024

Choose a reason for hiding this comment

ranquild left a comment

Choose a reason for hiding this comment

ranquild May 9, 2024

Choose a reason for hiding this comment

kamilmielnik May 9, 2024

Choose a reason for hiding this comment

kamilmielnik May 10, 2024

Choose a reason for hiding this comment

Use `DataPicker` in `JoinTablePicker` #42330

Use `DataPicker` in `JoinTablePicker` #42330

kamilmielnik commented May 7, 2024 •

edited

replay-io bot commented May 8, 2024 •

edited

iethree May 8, 2024 •

edited