feat(dashboard): confirm overwrite to prevent unintended changes #21819

justinpark · 2022-10-14T17:16:51Z

SUMMARY

At Airbnb, there's reasonable amount of reporting about unintended overwritten of filter scopes settings.
We believe there must be unknown overwriting on json_metadata by other authors during editing. (json_metadata can be easily overwritten since it passes all attributes including unchanged values.)
This commit introduces the inspection step for this high volatility value changes and then proceed to update with the user's confirmation. (the inspection items are defined here)
This commit also adds the logging for the user confirmation.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

review-overwrite-values.mov

TESTING INSTRUCTIONS

set ENABLE_CONFIRM_OVERWRITE_DASHBOARD_METADATA feature flag to true
go to any dashboard page
update css or/and filter scopes
click save to confirm the overwrite values

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags: ENABLE_CONFIRM_OVERWRITE_DASHBOARD_METADATA
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

cc: @ktmud @john-bodley

codecov · 2022-10-15T04:36:07Z

Codecov Report

Merging #21819 (c6e9b24) into master (72598a5) will increase coverage by 1.22%.
The diff coverage is 87.67%.

❗ Current head c6e9b24 differs from pull request most recent head 0ca291d. Consider uploading reports for the commit 0ca291d to get more accurate results

@@            Coverage Diff             @@
##           master   #21819      +/-   ##
==========================================
+ Coverage   65.72%   66.94%   +1.22%     
==========================================
  Files        1809     1809              
  Lines       69335    69178     -157     
  Branches     7413     7401      -12     
==========================================
+ Hits        45571    46313     +742     
+ Misses      21853    20952     -901     
- Partials     1911     1913       +2

Flag	Coverage Δ
javascript	`53.43% <87.67%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
...ackages/superset-ui-core/src/utils/featureFlags.ts	`100.00% <ø> (ø)`
...frontend/src/dashboard/components/Header/index.jsx	`59.85% <ø> (+0.86%)`	⬆️
...-frontend/src/dashboard/reducers/dashboardState.js	`74.46% <ø> (-1.62%)`	⬇️
...ponents/OverwriteConfirm/OverwriteConfirmModal.tsx	`82.14% <82.14%> (ø)`
...t-frontend/src/dashboard/actions/dashboardState.js	`52.94% <88.00%> (+15.84%)`	⬆️
...t-frontend/src/dashboard/util/getOverwriteItems.ts	`90.90% <90.90%> (ø)`
...rc/dashboard/components/OverwriteConfirm/index.tsx	`100.00% <100.00%> (ø)`
superset-frontend/src/dashboard/constants.ts	`100.00% <100.00%> (ø)`
superset-frontend/src/dashboard/util/constants.ts	`100.00% <100.00%> (ø)`
superset-frontend/src/logger/LogUtils.ts	`96.15% <100.00%> (ø)`
... and 160 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

john-bodley · 2022-10-17T16:58:08Z

superset-frontend/spec/fixtures/mockDashboardState.js

+    {
+      keyPath: 'json_metadata.filter_scopes',
+      oldValue:
+        '{\n  "122": {\n    "ethnic_minority": {\n      "scope": [\n        "ROOT_ID"\n      ],\n      "immune": []\n    },\n    "gender": {\n      "scope": [\n        "ROOT_ID"\n      ],\n      "immune": []\n    },\n    "developer_type": {\n      "scope": [\n        "ROOT_ID"\n      ],\n      "immune": []\n    },\n    "lang_at_home": {\n      "scope": [\n        "ROOT_ID"\n      ],\n      "immune": []\n    },\n    "country_live": {\n      "scope": [\n        "ROOT_ID"\n      ],\n      "immune": []\n    }\n  }\n}',


Should we add \t here and/or compare the old/new without the formatting, i.e., as a parsed JSON blob?

michael-s-molina · 2022-10-17T20:57:03Z

Thanks for this exciting feature @justinpark! Some first-pass comments:

1 - Can we rename the feature flag to a shorter name like DASHBOARD_METADATA_DIFF or anything else?

2 - Can you add the new feature flag to RESOURCES/FEATURE_FLAGS.md?

3 - Can we set a min/max width/height to the modal to avoid the situation where it takes the whole screen?

4 - Can we make the modal appear only when there's a change to the scope or CSS? Unrelated changes shouldn't present it.

5 - Is it possible to make the scope changes more user-friendly? It's showing ID diffs that are not traceable to the screen. Maybe add the chart names to the diff or use a different kind of scope representation when comparing.

ktmud

++ to @michael-s-molina 's comments.

A diff modal is a useful add when some of the updates may be implicit but I agree more visual diffs would be nice. I'm also wondering if in the long run we should strive for not requiring diff confirmation as we get more confident that the updates would not accidentally make unwanted changes.

If this is only meant to be temporary---which I'd assume is the case since it's in a feature flag, I think it'd be okay to just show JSON diffs.

ktmud · 2022-10-18T16:36:51Z

superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts

@@ -48,6 +48,7 @@ export enum FeatureFlag {
  ENABLE_JAVASCRIPT_CONTROLS = 'ENABLE_JAVASCRIPT_CONTROLS',
  ENABLE_TEMPLATE_PROCESSING = 'ENABLE_TEMPLATE_PROCESSING',
  ENABLE_TEMPLATE_REMOVE_FILTERS = 'ENABLE_TEMPLATE_REMOVE_FILTERS',
+  ENABLE_CONFIRM_OVERWRITE_DASHBOARD_METADATA = 'ENABLE_CONFIRM_OVERWRITE_DASHBOARD_METADATA',


Suggested change

ENABLE_CONFIRM_OVERWRITE_DASHBOARD_METADATA = 'ENABLE_CONFIRM_OVERWRITE_DASHBOARD_METADATA',

CONFIRM_DASHBOARD_DIFF = 'CONFIRM_DASHBOARD_DIFF',

ktmud · 2022-10-18T16:39:45Z

superset-frontend/spec/fixtures/mockDashboardState.js

+      keyPath: 'css',
+      oldValue: '',
+      newValue:
+        ".navbar {\n    transition: opacity 0.5s ease;\n    opacity: 0.05;\n}\n.navbar:hover {\n    opacity: 1;\n}\n.chart-header .header{\n    font-weight: @font-weight-normal;\n    font-size: 12px;\n}\n/*\nvar bnbColors = [\n    //rausch    hackb      kazan      babu      lima        beach     tirol\n    '#ff5a5f', '#7b0051', '#007A87', '#00d1c1', '#8ce071', '#ffb400', '#b4a76c',\n    '#ff8083', '#cc0086', '#00a1b3', '#00ffeb', '#bbedab', '#ffd266', '#cbc29a',\n    '#ff3339', '#ff1ab1', '#005c66', '#00b3a5', '#55d12e', '#b37e00', '#988b4e',\n ];\n*/\n",


If this file is manually generated, could we use template literals to display multiline strings instead?

ktmud · 2022-10-18T16:53:33Z

superset-frontend/spec/fixtures/mockDashboardState.js

+  latestUpdatedAt: '2022-10-07T16:35:30.924212',
+  latestUpdatedBy: 'Superset Admin',


Suggested change

latestUpdatedAt: '2022-10-07T16:35:30.924212',

latestUpdatedBy: 'Superset Admin',

updatedAt: '2022-10-07T16:35:30.924212',

updatedBy: 'Superset Admin'

Nit: I think this is clear enough.

ktmud · 2022-10-18T17:14:27Z

superset-frontend/src/dashboard/actions/dashboardState.js


-      return SupersetClient.put({


If this dispatch function returns a promise, can we just rewrite it with async/await?

It should be better to have a separate PR for the async/await migration.

ktmud · 2022-10-18T17:16:25Z

superset-frontend/src/dashboard/components/OverwriteConfirm/OverwriteConfirmModal.tsx

+      }
+      onHide={onHide}
+    >
+      <>


Is this fragment needed?

good catch. removed

ktmud · 2022-10-18T17:18:03Z

superset-frontend/src/dashboard/components/OverwriteConfirm/OverwriteConfirmModal.tsx

+      title={t('Confirm overwrite')}
+      footer={
+        <>
+          {t('* Scroll down to the bottom to complete the review. ')}


Suggested change

{t('* Scroll down to the bottom to complete the review. ')}

{t('Scroll down to the bottom to complete the review. ')}

Not sure if * is needed.

ktmud · 2022-10-18T17:18:36Z

superset-frontend/src/dashboard/components/OverwriteConfirm/OverwriteConfirmModal.tsx

+            cta
+            buttonStyle="primary"
+            onClick={onConfirmOverwrite}
+            disabled={!hasReviewed}


Add a tooltip to explain the disabled state?

The following description should be enough to explain the disabled state.

ktmud · 2022-10-18T17:20:45Z

superset-frontend/src/dashboard/components/OverwriteConfirm/OverwriteConfirmModal.tsx

+   `}
+`;
+
+const StackableHeader = styled(Button)<{ top: number }>`


This header is currently rendered as all caps (probably inherited style from Button). Is that what we want?

justinpark · 2022-10-18T22:34:48Z

1 - Can we rename the feature flag to a shorter name like DASHBOARD_METADATA_DIFF or anything else?

I updated to CONFIRM_DASHBOARD_DIFF

2 - Can you add the new feature flag to RESOURCES/FEATURE_FLAGS.md?

Yes. updated.

3 - Can we set a min/max width/height to the modal to avoid the situation where it takes the whole screen?

Yes I added the maxWidth using responsive. (its size up to 1024px)

4 - Can we make the modal appear only when there's a change to the scope or CSS? Unrelated changes shouldn't present it.

Sure. The modal only pops up when the following values are changed.

export const OVERWRITE_INSPECT_FIELDS = ['css', 'json_metadata.filter_scopes'];

5 - Is it possible to make the scope changes more user-friendly? It's showing ID diffs that are not traceable to the screen. Maybe add the chart names to the diff or use a different kind of scope representation when comparing.

as @ktmud commented, it becomes complicated to access the scope labels and its hierarchy. I'll follow up this improvement in the next step.

justinpark · 2022-11-02T19:13:34Z

1 - Can we rename the feature flag to a shorter name like DASHBOARD_METADATA_DIFF or anything else?

I updated to CONFIRM_DASHBOARD_DIFF

2 - Can you add the new feature flag to RESOURCES/FEATURE_FLAGS.md?

Yes. updated.

3 - Can we set a min/max width/height to the modal to avoid the situation where it takes the whole screen?

Yes I added the maxWidth using responsive. (its size up to 1024px)

4 - Can we make the modal appear only when there's a change to the scope or CSS? Unrelated changes shouldn't present it.

Sure. The modal only pops up when the following values are changed.

export const OVERWRITE_INSPECT_FIELDS = ['css', 'json_metadata.filter_scopes'];

5 - Is it possible to make the scope changes more user-friendly? It's showing ID diffs that are not traceable to the screen. Maybe add the chart names to the diff or use a different kind of scope representation when comparing.

as ktmud commented, it becomes complicated to access the scope labels and its hierarchy. I'll follow up this improvement in the next step.

@michael-s-molina do you have any objections with the approach? We'd like to kick off the feature with JSON diffs first. (and then will follow up the improved labels version)

michael-s-molina · 2022-11-03T12:56:38Z

@michael-s-molina do you have any objections with the approach? We'd like to kick off the feature with JSON diffs first. (and then will follow up the improved labels version)

No problem 👍🏼

justinpark · 2022-11-03T18:43:02Z

@michael-s-molina do you have any objections with the approach? We'd like to kick off the feature with JSON diffs first. (and then will follow up the improved labels version)

No problem 👍🏼

Great! can you approve the change? (it's blocked by the reviewers)

…che#21819) (cherry picked from commit ef6b9a9)

pull-request-size bot added the size/XL label Oct 14, 2022

john-bodley requested review from geido, michael-s-molina, rusackas and ktmud October 17, 2022 16:56

john-bodley reviewed Oct 17, 2022

View reviewed changes

ktmud reviewed Oct 18, 2022

View reviewed changes

justinpark added 8 commits November 1, 2022 16:02

feat(dashboard): confirm overwrite to prevent unintended changes

0c81529

unused data-test

1a234af

rename featureFlag to CONFIRM_DASHBOARD_DIFF

3a8898c

set maxWidth

b11ec21

fix for null case

267f332

remove default_scopes

ea16969

update schema name and mock

ad31df6

update label

0ca291d

justinpark force-pushed the feat--dashboard-confirm-overwrite-unintended-changes branch from c6e9b24 to 0ca291d Compare November 1, 2022 23:02

ktmud approved these changes Nov 3, 2022

View reviewed changes

john-bodley merged commit ef6b9a9 into apache:master Nov 8, 2022

john-bodley pushed a commit to airbnb/superset-fork that referenced this pull request Nov 8, 2022

feat(dashboard): confirm overwrite to prevent unintended changes (apa…

1d7d73d

…che#21819) (cherry picked from commit ef6b9a9)

justinpark mentioned this pull request Nov 24, 2022

chore(dashboard): Ignore empty json value for overwrite confirm #22214

Merged

9 tasks

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

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

mistercrunch removed the 🚢 2.1.3 label Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dashboard): confirm overwrite to prevent unintended changes #21819

feat(dashboard): confirm overwrite to prevent unintended changes #21819

justinpark commented Oct 14, 2022 •

edited

Loading

codecov bot commented Oct 15, 2022 •

edited

Loading

john-bodley Oct 17, 2022

michael-s-molina commented Oct 17, 2022

ktmud left a comment

ktmud Oct 18, 2022

ktmud Oct 18, 2022

ktmud Oct 18, 2022

ktmud Oct 18, 2022

justinpark Oct 19, 2022

ktmud Oct 18, 2022

justinpark Oct 19, 2022

ktmud Oct 18, 2022

ktmud Oct 18, 2022

justinpark Oct 18, 2022

ktmud Oct 18, 2022

justinpark commented Oct 18, 2022

justinpark commented Nov 2, 2022

michael-s-molina commented Nov 3, 2022

justinpark commented Nov 3, 2022

	ENABLE_CONFIRM_OVERWRITE_DASHBOARD_METADATA = 'ENABLE_CONFIRM_OVERWRITE_DASHBOARD_METADATA',
	CONFIRM_DASHBOARD_DIFF = 'CONFIRM_DASHBOARD_DIFF',

		latestUpdatedAt: '2022-10-07T16:35:30.924212',
		latestUpdatedBy: 'Superset Admin',

	{t('* Scroll down to the bottom to complete the review. ')}
	{t('Scroll down to the bottom to complete the review. ')}

feat(dashboard): confirm overwrite to prevent unintended changes #21819

feat(dashboard): confirm overwrite to prevent unintended changes #21819

Conversation

justinpark commented Oct 14, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Oct 15, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

michael-s-molina commented Oct 17, 2022

ktmud 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

Choose a reason for hiding this comment

justinpark commented Oct 18, 2022

justinpark commented Nov 2, 2022

michael-s-molina commented Nov 3, 2022

justinpark commented Nov 3, 2022

justinpark commented Oct 14, 2022 •

edited

Loading

codecov bot commented Oct 15, 2022 •

edited

Loading