-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
feat(explore): allow opening charts with missing dataset #12705
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bycatch: reuse typing from |
||
} | ||
|
||
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} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Datasource ID is |
||
</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.', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we be more explicit here as in the other error - "Dataset does not exist". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thinking is this: the dataset does not exist for two reasons, either it never existed or has been deleted. Since the latter is the most likely case, we inform users about this possibility, but also show reservations with "may" to be more accurate. I think a little bit more color in copy is more helpful comparing to repeating the same general message over and over. |
||
)} | ||
</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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move to reduce the change of circular imports. |
||
|
||
|
||
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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch 👍 |
||
) | ||
|
||
|
||
|
@@ -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 "") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that it matters, but I've always been allergic to md5 on empty string:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'll return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nvm. I'll keep return f"/api/v1/chart/{self.id}/thumbnail/{self.digest}/" We can make a follow up PR to rename |
||||||
|
||||||
@property | ||||||
def thumbnail_url(self) -> str: | ||||||
|
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.