feat(explore): allow opening charts with missing dataset #12705

ktmud · 2021-01-23T02:05:53Z

SUMMARY

Allow opening charts with missing dataset as proposed in #12464 (comment). Alternative solution for #12468 .

Also did some refactoring on get_datasource error handling.

Closes #12464

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

Opening a chart with missing dataset redirects to chart list with a flash message.

After

Chart can open, all control params retained; users are offered a way to quickly switch to a valid dataset.

TEST PLAN

Manual

Open an existing chart
Manually change the datasource id in URL parameters:

ADDITIONAL INFORMATION

Has associated issue: see SUMMARY
Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

ktmud · 2021-01-23T02:08:11Z

superset-frontend/src/common/components/index.tsx

@@ -28,6 +28,7 @@ import { DropDownProps } from 'antd/lib/dropdown';
 */
 // eslint-disable-next-line no-restricted-imports
 export {
+  Alert,


Wanted to use Antd Alert, but it looks inconsistent with the error box in chart container.

ktmud · 2021-01-23T02:08:44Z

superset-frontend/src/explore/components/DatasourcePanel.tsx

-  label: string;
-  datasource?: DatasourceControl;
+interface DatasourceControl extends ControlConfig {
+  datasource?: DatasourceMeta;


Bycatch: reuse typing from @superset-ui/chart-controls.

ktmud · 2021-01-23T02:09:16Z

superset-frontend/src/explore/components/DatasourcePanel.tsx

+  return (
+    <DatasourceContainer>
+      <Control {...datasourceControl} name="datasource" actions={actions} />
+      {datasource.id != null && mainBody}


Datasource ID is null when it is unavailable.

ktmud · 2021-01-23T02:09:49Z

superset/views/core.py

+            "name": datasource_name,
+            "columns": [],
+            "metrics": [],
+        }


Adding a dummy datasource is easier than fixing countless places on the frontend where it's expected to exist.

ktmud · 2021-01-23T02:11:25Z

superset/connectors/connector_registry.py

+        Returns None if `datasource_type` is not registered or `datasource_id` does
+        not exists."""
+        if datasource_type not in cls.sources:
+            raise DatasetNotFoundError()


handle_api_exception will be able to render the error properly.

ktmud · 2021-01-23T02:11:36Z

superset/views/datasource.py

-                return json_error_response(
-                    f"{DatasetForbiddenError.message}", DatasetForbiddenError.status
-                )
+                raise DatasetForbiddenError()


handle_api_exception will be able to render the error properly.

ktmud · 2021-01-23T02:16:58Z

superset/utils/core.py

@@ -504,6 +505,8 @@ def base_json_conv(  # pylint: disable=inconsistent-return-statements,too-many-r
            return obj.decode("utf-8")
        except Exception:  # pylint: disable=broad-except
            return "[bytes]"
+    if isinstance(obj, LazyString):
+        return str(obj)


Make sure translations can be properly outputted in json API response.

ktmud · 2021-01-23T02:20:12Z

superset/datasets/commands/exceptions.py

@@ -26,7 +26,10 @@
    ImportFailedError,
    UpdateFailedError,
 )
-from superset.views.base import get_datasource_exist_error_msg


Move to reduce the change of circular imports.

codecov-io · 2021-01-23T02:22:36Z

Codecov Report

Merging #12705 (aa350ef) into master (0fed1e0) will decrease coverage by 7.71%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master   #12705      +/-   ##
==========================================
- Coverage   66.89%   59.17%   -7.72%     
==========================================
  Files        1022      965      -57     
  Lines       49994    47218    -2776     
  Branches     4892     4405     -487     
==========================================
- Hits        33442    27942    -5500     
- Misses      16427    19276    +2849     
+ Partials      125        0     -125

Flag	Coverage Δ
cypress	`51.03% <81.81%> (+0.11%)`	⬆️
javascript	`?`
python	`63.79% <77.96%> (-0.29%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset-frontend/src/common/components/index.tsx	`100.00% <ø> (ø)`
...perset-frontend/src/explore/components/Control.tsx	`100.00% <ø> (+4.76%)`	⬆️
superset/connectors/druid/views.py	`69.79% <0.00%> (ø)`
superset/datasets/api.py	`89.49% <ø> (-1.83%)`	⬇️
superset/views/base.py	`75.31% <50.00%> (-0.11%)`	⬇️
superset/utils/core.py	`88.04% <66.66%> (-0.10%)`	⬇️
superset/views/core.py	`72.93% <68.00%> (-2.37%)`	⬇️
superset/views/datasource.py	`90.16% <71.42%> (+0.76%)`	⬆️
.../explore/components/controls/DatasourceControl.jsx	`62.68% <75.00%> (-0.03%)`	⬇️
...rontend/src/explore/components/DatasourcePanel.tsx	`80.95% <100.00%> (-1.66%)`	⬇️
... and 421 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fed1e0...aa350ef. Read the comment docs.

junlincc · 2021-01-23T19:57:31Z

I agree - this offers better UX. thanks for the (mid-term) solution. sad to see the toast message go though. Thank you both! @kkucharc @ktmud 🙏
LGTM

Screen.Recording.2021-01-23.at.11.52.15.AM.mov

cc @mihir174 we can tweak the styling and msg content later.

villebro

LGTM. Minor comments, thanks fixing the translations, too!

villebro · 2021-01-25T08:25:09Z

superset-frontend/src/explore/components/controls/DatasourceControl.jsx

-            <span className="title-select">{datasource.name}</span>
-          </Tooltip>
+          {/* Add a tooltip only for long dataset names */}
+          {datasource.name.length > 20 ? (


Could be nice to have this in a constants.ts

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.

villebro · 2021-01-25T08:26:09Z

superset-frontend/src/explore/components/controls/DatasourceControl.jsx

+                <>
+                  <p>
+                    {t(
+                      'The dataset linked to this chart may have been deleted.',


Could we be more explicit here as in the other error - "Dataset does not exist".

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.

villebro · 2021-01-25T08:28:15Z

superset/connectors/connector_registry.py

-        return (
+        """Safely get a datasource without raising any errors.
+        Returns None if `datasource_type` is not registered or `datasource_id` does
+        not exists."""


nit:

Suggested change

not exists."""

not exist."""

This comment is inaccurate. I decided to still raise an error to avoid making too many changes.

Will delete.

villebro · 2021-01-25T08:31:03Z

superset/datasets/commands/exceptions.py

@@ -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"


Nice catch 👍

villebro · 2021-01-25T08:34:05Z

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 "")


Not that it matters, but I've always been allergic to md5 on empty string:

>>> import hashlib >>> hashlib.md5(''.encode()).hexdigest() 'd41d8cd98f00b204e9800998ecf8427e'

Suggested change

return utils.md5_hex(self.params or "")

return utils.md5_hex(self.params) if self.params else ''

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.

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.

ktmud · 2021-01-25T23:08:37Z

@junlincc @mihir174 feel free to create another issue to track followups if you want to update the design or copy.

ktmud · 2021-01-25T23:12:10Z

A possible followup would be to add "Delete chart" shortcut CTA to make it easier for chart owners to delete outdated charts.

* master: (52 commits) docs: Updates to Superset Site for 1.0 (apache#12626) test(native-filters): scoping tree in native filters modal (apache#12655) Fix tests errors and warnings - iteration 3 (apache#12212) (apache#12219) Fix tests errors and warnings - iteration 5 (apache#12212) (apache#12224) Fix tests errors and warnings - iteration 6 (apache#12212) (apache#12227) feat(native-filters): apply scoping of native filters to dashboard (apache#12716) Fix tests errors and warnings - iteration 4 (apache#12212) (apache#12223) Fix tests errors and warnings - iteration 7 (apache#12212) (apache#12245) fix: missing select menu background (apache#12759) fix(explore): incorrect missing datasource condition (apache#12758) feat: default timepicker to last week when dataset is changed (apache#12609) feat(explore): allow opening charts with missing dataset (apache#12705) chore: upgrade Cypress to 6.2.1 (apache#12605) refactor(explore): Enhance Dataset and Control panel Collapse components (apache#12218) feat: Adding option to set_database_uri CLI command (apache#12740) docs: Fixed typo on line 348 (apache#12739) Fix tests errors and warnings - iteration 2 (apache#12212) (apache#12214) docs: Remove gatsby-plugin-offline (apache#12693) test: oracle engine spec (apache#12615) test: hive db engine spec (apache#12520) ...

pull-request-size bot added the size/L label Jan 23, 2021

ktmud force-pushed the datasource-not-found branch 3 times, most recently from a65f780 to 2d58d3f Compare January 23, 2021 02:19

ktmud commented Jan 23, 2021

View reviewed changes

ktmud requested review from villebro, kkucharc and betodealmeida and removed request for villebro January 23, 2021 02:19

ktmud commented Jan 23, 2021

View reviewed changes

ktmud marked this pull request as draft January 23, 2021 02:22

ktmud force-pushed the datasource-not-found branch 4 times, most recently from 4b6409f to 41b3192 Compare January 23, 2021 04:15

junlincc added the explore:datapanel Related to the Data panel of Explore label Jan 23, 2021

ktmud force-pushed the datasource-not-found branch from 41b3192 to bc75a7c Compare January 23, 2021 08:50

pull-request-size bot added size/XL and removed size/L labels Jan 23, 2021

ktmud force-pushed the datasource-not-found branch 2 times, most recently from 5ad2399 to d2f47a4 Compare January 23, 2021 09:23

ktmud marked this pull request as ready for review January 23, 2021 09:23

junlincc added the v1.0.1 label Jan 23, 2021

villebro approved these changes Jan 25, 2021

View reviewed changes

junlincc added the need:design-review Requires input/approval from a Designer label Jan 25, 2021

ktmud force-pushed the datasource-not-found branch from d2f47a4 to 04fab5e Compare January 25, 2021 20:42

feat(explore): allow opening charts with missing dataset

00c33b2

ktmud added 3 commits January 25, 2021 13:46

Fix tests

3c5ab2f

Address PR comments

526c5e4

Add a variable for missingdatasource

aa350ef

ktmud force-pushed the datasource-not-found branch from 04fab5e to aa350ef Compare January 25, 2021 21:51

ktmud merged commit 55c8f9b into apache:master Jan 25, 2021

ktmud deleted the datasource-not-found branch January 25, 2021 23:11

This was referenced Jan 26, 2021

[explore]chart missing dataset open error msg #12753

Closed

[explore]missing dataset error is showing in all charts #12757

Closed

ktmud mentioned this pull request Jan 26, 2021

fix(explore): incorrect missing datasource condition #12758

Merged

6 tasks

villebro pushed a commit that referenced this pull request Jan 29, 2021

feat(explore): allow opening charts with missing dataset (#12705)

c1c798a

ktmud added the need:tests This PR requires tests label Feb 24, 2021

mistercrunch added 🍒 1.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(explore): allow opening charts with missing dataset #12705

feat(explore): allow opening charts with missing dataset #12705

ktmud commented Jan 23, 2021 •

edited

Loading

ktmud Jan 23, 2021

ktmud Jan 23, 2021

ktmud Jan 23, 2021

ktmud Jan 23, 2021

ktmud Jan 23, 2021

ktmud Jan 23, 2021

ktmud Jan 23, 2021

ktmud Jan 23, 2021

codecov-io commented Jan 23, 2021 •

edited

Loading

junlincc commented Jan 23, 2021 •

edited

Loading

villebro left a comment

villebro Jan 25, 2021

ktmud Jan 25, 2021

villebro Jan 25, 2021

ktmud Jan 25, 2021

villebro Jan 25, 2021

ktmud Jan 25, 2021

villebro Jan 25, 2021

villebro Jan 25, 2021

ktmud Jan 25, 2021

ktmud Jan 25, 2021

ktmud commented Jan 25, 2021

ktmud commented Jan 25, 2021

	return utils.md5_hex(self.params or "")
	return utils.md5_hex(self.params) if self.params else ''

feat(explore): allow opening charts with missing dataset #12705

feat(explore): allow opening charts with missing dataset #12705

Conversation

ktmud commented Jan 23, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TEST PLAN

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 23, 2021 • edited Loading

Codecov Report

junlincc commented Jan 23, 2021 • edited Loading

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud commented Jan 25, 2021

ktmud commented Jan 25, 2021

ktmud commented Jan 23, 2021 •

edited

Loading

codecov-io commented Jan 23, 2021 •

edited

Loading

junlincc commented Jan 23, 2021 •

edited

Loading