-
Notifications
You must be signed in to change notification settings - Fork 16.6k
feat(explore): allow opening charts with missing dataset #12705
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,57 +17,25 @@ | |
| * under the License. | ||
| */ | ||
| import React, { useEffect, useState } from 'react'; | ||
| import { styled, t, QueryFormData } from '@superset-ui/core'; | ||
| import { styled, t } from '@superset-ui/core'; | ||
| import { Collapse } from 'src/common/components'; | ||
| import { | ||
| ColumnOption, | ||
| MetricOption, | ||
| ControlType, | ||
| ControlConfig, | ||
| DatasourceMeta, | ||
| } from '@superset-ui/chart-controls'; | ||
| import { debounce } from 'lodash'; | ||
| import { matchSorter, rankings } from 'match-sorter'; | ||
| import { ExploreActions } from '../actions/exploreActions'; | ||
| import Control from './Control'; | ||
|
|
||
| interface DatasourceControl { | ||
| validationErrors: Array<any>; | ||
| mapStateToProps: QueryFormData; | ||
| type: ControlType; | ||
| label: string; | ||
| datasource?: DatasourceControl; | ||
| interface DatasourceControl extends ControlConfig { | ||
| datasource?: DatasourceMeta; | ||
|
||
| } | ||
|
|
||
| type Columns = { | ||
| column_name: string; | ||
| description: string | undefined; | ||
| expression: string | undefined; | ||
| filterable: boolean; | ||
| groupby: string | undefined; | ||
| id: number; | ||
| is_dttm: boolean; | ||
| python_date_format: string; | ||
| type: string; | ||
| verbose_name: string; | ||
| }; | ||
|
|
||
| type Metrics = { | ||
| certification_details: string | undefined; | ||
| certified_by: string | undefined; | ||
| d3format: string | undefined; | ||
| description: string | undefined; | ||
| expression: string; | ||
| id: number; | ||
| is_certified: boolean; | ||
| metric_name: string; | ||
| verbose_name: string; | ||
| warning_text: string; | ||
| }; | ||
|
|
||
| interface Props { | ||
| datasource: { | ||
| columns: Array<Columns>; | ||
| metrics: Array<Metrics>; | ||
| }; | ||
| datasource: DatasourceMeta; | ||
| controls: { | ||
| datasource: DatasourceControl; | ||
| }; | ||
|
|
@@ -228,15 +196,8 @@ export default function DataSourcePanel({ | |
| const metricSlice = lists.metrics.slice(0, 50); | ||
| const columnSlice = lists.columns.slice(0, 50); | ||
|
|
||
| return ( | ||
| <DatasourceContainer> | ||
| <Control | ||
| {...datasourceControl} | ||
| name="datasource" | ||
| validationErrors={datasourceControl.validationErrors} | ||
| actions={actions} | ||
| formData={datasourceControl.mapStateToProps} | ||
| /> | ||
| const mainBody = ( | ||
| <> | ||
| <input | ||
| type="text" | ||
| onChange={evt => { | ||
|
|
@@ -279,6 +240,13 @@ export default function DataSourcePanel({ | |
| </Collapse.Panel> | ||
| </Collapse> | ||
| </div> | ||
| </> | ||
| ); | ||
|
|
||
| return ( | ||
| <DatasourceContainer> | ||
| <Control {...datasourceControl} name="datasource" actions={actions} /> | ||
| {datasource.id != null && mainBody} | ||
|
||
| </DatasourceContainer> | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,8 @@ import Icon from 'src/components/Icon'; | |
| import ChangeDatasourceModal from 'src/datasource/ChangeDatasourceModal'; | ||
| import DatasourceModal from 'src/datasource/DatasourceModal'; | ||
| import { postForm } from 'src/explore/exploreUtils'; | ||
| import Button from 'src/components/Button'; | ||
| import ErrorAlert from 'src/components/ErrorMessage/ErrorAlert'; | ||
|
|
||
| const propTypes = { | ||
| actions: PropTypes.object.isRequired, | ||
|
|
@@ -51,6 +53,9 @@ const Styles = styled.div` | |
| border-bottom: 1px solid ${({ theme }) => theme.colors.grayscale.light2}; | ||
| padding: ${({ theme }) => 2 * theme.gridUnit}px; | ||
| } | ||
| .error-alert { | ||
| margin: ${({ theme }) => 2 * theme.gridUnit}px; | ||
| } | ||
| .ant-dropdown-trigger { | ||
| margin-left: ${({ theme }) => 2 * theme.gridUnit}px; | ||
| box-shadow: none; | ||
|
|
@@ -152,6 +157,7 @@ class DatasourceControl extends React.PureComponent { | |
| render() { | ||
| const { showChangeDatasourceModal, showEditDatasourceModal } = this.state; | ||
| const { datasource, onChange } = this.props; | ||
| const isMissingDatasource = datasource; | ||
| const datasourceMenu = ( | ||
| <Menu onClick={this.handleMenuItemClick}> | ||
| {this.props.isEditable && ( | ||
|
|
@@ -164,16 +170,22 @@ class DatasourceControl extends React.PureComponent { | |
| </Menu> | ||
| ); | ||
|
|
||
| // eslint-disable-next-line camelcase | ||
| const { health_check_message: healthCheckMessage } = datasource; | ||
|
|
||
| return ( | ||
| <Styles className="DatasourceControl"> | ||
| <div className="data-container"> | ||
| <Icon name="dataset-physical" className="dataset-svg" /> | ||
| <Tooltip title={datasource.name}> | ||
| <span className="title-select">{datasource.name}</span> | ||
| </Tooltip> | ||
| {/* Add a tooltip only for long dataset names */} | ||
| {!isMissingDatasource && datasource.name.length > 25 ? ( | ||
| <Tooltip title={datasource.name}> | ||
| <span className="title-select">{datasource.name}</span> | ||
| </Tooltip> | ||
| ) : ( | ||
| <span title={datasource.name} className="title-select"> | ||
| {datasource.name} | ||
| </span> | ||
| )} | ||
| {healthCheckMessage && ( | ||
| <Tooltip title={healthCheckMessage}> | ||
| <Icon | ||
|
|
@@ -196,6 +208,35 @@ class DatasourceControl extends React.PureComponent { | |
| </Tooltip> | ||
| </Dropdown> | ||
| </div> | ||
| {/* missing dataset */} | ||
| {isMissingDatasource && ( | ||
| <div className="error-alert"> | ||
| <ErrorAlert | ||
| level="warning" | ||
| title={t('Missing dataset')} | ||
| source="explore" | ||
| subtitle={ | ||
| <> | ||
| <p> | ||
| {t( | ||
| 'The dataset linked to this chart may have been deleted.', | ||
|
||
| )} | ||
| </p> | ||
| <p> | ||
| <Button | ||
| buttonStyle="primary" | ||
| onClick={() => | ||
| this.handleMenuItemClick({ key: CHANGE_DATASET }) | ||
| } | ||
| > | ||
| {t('Change dataset')} | ||
| </Button> | ||
| </p> | ||
| </> | ||
| } | ||
| /> | ||
| </div> | ||
| )} | ||
| {showEditDatasourceModal && ( | ||
| <DatasourceModal | ||
| datasource={datasource} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ | |
| from sqlalchemy import or_ | ||
| from sqlalchemy.orm import Session, subqueryload | ||
|
|
||
| from superset.datasets.commands.exceptions import DatasetNotFoundError | ||
|
|
||
| if TYPE_CHECKING: | ||
| from collections import OrderedDict | ||
|
|
||
|
|
@@ -44,12 +46,23 @@ def register_sources(cls, datasource_config: "OrderedDict[str, List[str]]") -> N | |
| def get_datasource( | ||
| cls, datasource_type: str, datasource_id: int, session: Session | ||
| ) -> "BaseDatasource": | ||
| return ( | ||
| """Safely get a datasource instance, raises `DatasetNotFoundError` if | ||
| `datasource_type` is not registered or `datasource_id` does not | ||
| exist.""" | ||
| if datasource_type not in cls.sources: | ||
| raise DatasetNotFoundError() | ||
|
||
|
|
||
| datasource = ( | ||
| session.query(cls.sources[datasource_type]) | ||
| .filter_by(id=datasource_id) | ||
| .one() | ||
| .one_or_none() | ||
| ) | ||
|
|
||
| if not datasource: | ||
| raise DatasetNotFoundError() | ||
|
|
||
| return datasource | ||
|
|
||
| @classmethod | ||
| def get_all_datasources(cls, session: Session) -> List["BaseDatasource"]: | ||
| datasources: List["BaseDatasource"] = [] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,10 @@ | |
| ImportFailedError, | ||
| UpdateFailedError, | ||
| ) | ||
| from superset.views.base import get_datasource_exist_error_msg | ||
|
||
|
|
||
|
|
||
| def get_dataset_exist_error_msg(full_name: str) -> str: | ||
| return _("Dataset %(name)s already exists", name=full_name) | ||
|
|
||
|
|
||
| class DatabaseNotFoundValidationError(ValidationError): | ||
|
|
@@ -54,7 +57,7 @@ class DatasetExistsValidationError(ValidationError): | |
|
|
||
| def __init__(self, table_name: str) -> None: | ||
| super().__init__( | ||
| get_datasource_exist_error_msg(table_name), field_name="table_name" | ||
| [get_dataset_exist_error_msg(table_name)], field_name="table_name" | ||
|
||
| ) | ||
|
|
||
|
|
||
|
|
@@ -142,7 +145,8 @@ def __init__(self) -> None: | |
|
|
||
|
|
||
| class DatasetNotFoundError(CommandException): | ||
| message = "Dataset not found." | ||
| status = 404 | ||
| message = _("Dataset does not exist") | ||
|
|
||
|
|
||
| class DatasetInvalidError(CommandInvalidError): | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -206,7 +206,7 @@ def digest(self) -> str: | |||||
| """ | ||||||
| Returns a MD5 HEX digest that makes this dashboard unique | ||||||
| """ | ||||||
| return utils.md5_hex(self.params) | ||||||
| return utils.md5_hex(self.params or "") | ||||||
|
||||||
| return utils.md5_hex(self.params or "") | |
| return utils.md5_hex(self.params) if self.params else '' |
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.
I think I'll return None here just to be more explicit. The original change is for avoiding an uncaught exception, explicitly returning None forces the downstream handle the case of missing params more properly.
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.
Nvm. I'll keep utils.md5_hex(self.params or "") since this key is also used in thumbnail URLs:
return f"/api/v1/chart/{self.id}/thumbnail/{self.digest}/"We can make a follow up PR to rename digest to thumbnail_key (for both chart and dashboard) if we think it's necessary.
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.
Wanted to use Antd Alert, but it looks inconsistent with the error box in chart container.