ktmud commented on a change in pull request #12705:
URL: https://github.com/apache/superset/pull/12705#discussion_r563973045
##########
File path:
superset-frontend/src/explore/components/controls/DatasourceControl.jsx
##########
@@ -164,16 +169,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 */}
+ {datasource.name.length > 20 ? (
Review comment:
This ideally should be more sophisticated where we display tooltip only
when we detect text has been cutoff. This is just a heuristic number that works
for the smallest panel width (300px).
It's a very local feature so I wouldn't put it into another file.
##########
File path:
superset-frontend/src/explore/components/controls/DatasourceControl.jsx
##########
@@ -196,6 +207,35 @@ class DatasourceControl extends React.PureComponent {
</Tooltip>
</Dropdown>
</div>
+ {/* missing dataset */}
+ {datasource.id == null && (
+ <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.',
Review comment:
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.
##########
File path: superset/connectors/connector_registry.py
##########
@@ -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 without raising any errors.
+ Returns None if `datasource_type` is not registered or `datasource_id`
does
+ not exists."""
Review comment:
This comment is inaccurate. I decided to still raise an error to avoid
making too many changes.
Will delete.
##########
File path: superset/models/slice.py
##########
@@ -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 "")
Review comment:
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.
##########
File path: superset/models/slice.py
##########
@@ -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 "")
Review comment:
Nvm. I'll keep `utils.md5_hex(self.params or "")` since this key is also
used in thumbnail URLs:
```python
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
