fix: Save properties after applying changes in Dashboard #17570

geido · 2021-11-29T09:20:17Z

SUMMARY

This PR fixes a severe issue for which changes in the properties modal of a Dashboard won't be fully saved when the onlyApply prop was used. This PR #17392 enabled the onlyApply tag. Later, manual tests have shown that the tag wasn't ready to be used as both the frontend and the backend were not fully implemented for that to work properly. In addition to that, this PR does the following:

Refactors the PropertiesModal to use typescript
Refactors the form component to use the functionalities provided by Antdesign
Fixes an issue in the Dashboard detail page for which when applying changes and re-opening the properties modal it did not show the latest changes
Fixes an issue for which changes in the json_metadata for color_namespace, expanded_slices, timed_refresh_immune_slices were not saved
Fixes an issue for which the dashboard wasn't redirecting back to the original URL when the slug was deleted
Fixes an issue for which changes to native filters would not update the json_metadata immediately, causing potential overwrites
Changes the endpoint that was used for updating the dashboard (save_dash) with the standard PUT endpoint. Also, it adds the set_dash_metadata function to the update command in the backend. The set_dash_metadata function was only used by the save_dash endpoint before, causing potential discrepancies in the way the json_metatada was handled by the standard PUT endpoint.
Removes unnecessary fetch requests

TESTING INSTRUCTIONS

Open a Dashboard
Edit the properties, including the json metadata
Apply the changes
Reopen the modal and make sure the latest changes are there
Finally, save the changes
Reload the Dashboard and make sure the properties were fully saved

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
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

…dashboard_properties_onapply

codecov · 2021-12-02T15:19:06Z

Codecov Report

Merging #17570 (c79aa5f) into master (8c25f2f) will decrease coverage by 0.03%.
The diff coverage is 63.63%.

❗ Current head c79aa5f differs from pull request most recent head 9bb937e. Consider uploading reports for the commit 9bb937e to get more accurate results

@@            Coverage Diff             @@
##           master   #17570      +/-   ##
==========================================
- Coverage   68.86%   68.82%   -0.04%     
==========================================
  Files        1598     1598              
  Lines       65297    65370      +73     
  Branches     6952     6961       +9     
==========================================
+ Hits        44966    44993      +27     
- Misses      18446    18486      +40     
- Partials     1885     1891       +6

Flag	Coverage Δ
javascript	`57.42% <57.84%> (-0.05%)`	⬇️

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

Impacted Files	Coverage Δ
...et-frontend/src/dashboard/components/SaveModal.tsx	`51.16% <0.00%> (+1.16%)`	⬆️
...uperset-frontend/src/utils/getClientErrorObject.ts	`71.87% <ø> (ø)`
...t-frontend/src/dashboard/actions/dashboardState.js	`35.79% <36.84%> (+0.45%)`	⬆️
...frontend/src/dashboard/components/Header/index.jsx	`61.11% <37.50%> (+2.08%)`	⬆️
superset/dashboards/api.py	`91.86% <57.14%> (ø)`
superset/dashboards/commands/update.py	`83.07% <66.66%> (ø)`
...src/dashboard/components/PropertiesModal/index.tsx	`66.87% <66.87%> (ø)`
superset/dashboards/dao.py	`96.09% <97.36%> (ø)`
superset/dashboards/schemas.py	`99.40% <100.00%> (ø)`
... and 3 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 8c25f2f...9bb937e. Read the comment docs.

geido · 2021-12-02T17:25:32Z

superset-frontend/cypress-base/cypress/integration/dashboard/edit_properties.test.ts

-    cy.wait('@dashboardGet').then(() => {
-      selectColorScheme('d3Category20b');
-    });
-


This was here because the color scheme was considered to be mandatory. This is not the case any longer

superset-frontend/spec/javascripts/dashboard/components/PropertiesModal_spec.jsx

superset-frontend/src/dashboard/components/PropertiesModal/index.tsx

kgabryje

Great work! I left a few cosmetic comments.

Since this PR is very large, it would be great if more people took a look at the code

…dashboard_properties_onapply

kgabryje

Thanks for the fixes! LGTM, but like I said, it would be great to have 1 more review since it's a big PR

rusackas · 2021-12-08T19:44:48Z

/testenv up

github-actions · 2021-12-08T19:49:12Z

@rusackas Ephemeral environment spinning up at http://34.217.207.248:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

rusackas · 2021-12-09T05:22:03Z

superset-frontend/src/dashboard/components/PropertiesModal/index.tsx

+    }
+
+    Modal.error({
+      title: 'Error',


Maybe a t() here - not worth holding up the PR over this though. Looks like there are a few more of these floating around in the codebase to sweep up anyway

rusackas · 2021-12-09T16:57:56Z

superset-frontend/src/dashboard/components/PropertiesModal/index.tsx

+        content: t('A valid color scheme is required'),
+        okButtonProps: { danger: true, className: 'btn-danger' },
+      });
+      throw new Error('A valid color scheme is required');


Suggested change

throw new Error('A valid color scheme is required');

throw new Error(t('A valid color scheme is required'));

Could use a t() here. Not blocking the PR for this!

rusackas

LGTM! Thanks for all this hard work!

github-actions · 2021-12-09T17:03:33Z

Ephemeral environment shutdown and build artifacts deleted.

suddjian · 2021-12-10T04:55:34Z

Thanks for this fix! Embedded work has been keeping me busy so thanks to the others for picking up the review. I managed to finally take a bit of a look today and it LGTM too. The original code here was one of my first Superset contributions 😁

* Refactor PropertiesModal * Update json_metadata fully * Clean up * Verify values * Catch changed to metadata * Always updated dashboard info on update * Avoid unnecessary fetches * Formt * Fix copy dashboards * Fixes onUpdate onCopy handlers * Pylint * Update tests * Clean up * Handle data on show * Change Save to Apply * Update Cypress save test * Update Cypress edit prop test * Update PropertiesModal test * Fix duplicate request with cross filters * Improve code style * Fix typo * Lint

geido added 2 commits November 26, 2021 14:23

Refactor PropertiesModal

5c4b6f9

Update json_metadata fully

5dd323b

superset-github-bot bot added the Flexiana label Nov 29, 2021

pull-request-size bot added the size/XXL label Nov 29, 2021

geido added 6 commits November 29, 2021 09:31

Clean up

020c679

Merge

8c02f1d

Verify values

2e5ca66

Merge branch 'master' of https://github.com/apache/superset into fix/…

0bc536a

…dashboard_properties_onapply

Catch changed to metadata

f44f484

Always updated dashboard info on update

d8dd0b2

geido mentioned this pull request Dec 1, 2021

fix: Revert "fix(Dashboard): Only apply changes when editing properties" #17618

Merged

Avoid unnecessary fetches

f814d63

geido marked this pull request as ready for review December 1, 2021 19:49

geido added 2 commits December 1, 2021 19:52

Merge

2388d37

Formt

730f30c

rusackas requested review from betodealmeida and suddjian December 1, 2021 20:08

geido added 5 commits December 1, 2021 20:24

Fix copy dashboards

f3a049c

Fixes onUpdate onCopy handlers

12932d6

Pylint

03d3c23

Update tests

42ade6d

Clean up

bc9b461

Handle data on show

a1b6922

geido commented Dec 2, 2021

View reviewed changes

superset-frontend/spec/javascripts/dashboard/components/PropertiesModal_spec.jsx Show resolved Hide resolved

geido added 4 commits December 3, 2021 14:29

Change Save to Apply

a5c3bd1

Update Cypress save test

c94d684

Update Cypress edit prop test

bc908a8

Update PropertiesModal test

df8fe9f

kgabryje reviewed Dec 7, 2021

View reviewed changes

superset-frontend/src/dashboard/components/PropertiesModal/index.tsx Outdated Show resolved Hide resolved

kgabryje reviewed Dec 7, 2021

View reviewed changes

superset-frontend/src/dashboard/components/PropertiesModal/index.tsx Outdated Show resolved Hide resolved

kgabryje reviewed Dec 7, 2021

View reviewed changes

superset-frontend/src/dashboard/components/PropertiesModal/index.tsx Outdated Show resolved Hide resolved

kgabryje reviewed Dec 7, 2021

View reviewed changes

superset-frontend/src/dashboard/components/PropertiesModal/index.tsx Outdated Show resolved Hide resolved

kgabryje requested changes Dec 7, 2021

View reviewed changes

geido added 4 commits December 8, 2021 14:52

Merge branch 'master' of https://github.com/apache/superset into fix/…

9ea377d

…dashboard_properties_onapply

Improve code style

6fd698f

Fix typo

d63fc65

Lint

9bb937e

kgabryje approved these changes Dec 8, 2021

View reviewed changes

eschutho added the v1.4 label Dec 8, 2021

rusackas reviewed Dec 9, 2021

View reviewed changes

rusackas approved these changes Dec 9, 2021

View reviewed changes

rusackas merged commit 12bd1fc into apache:master Dec 9, 2021

geido mentioned this pull request Dec 9, 2021

fix(Dashboard): Copy dashboard with duplicating charts 500 error #17707

Merged

9 tasks

serenajiang mentioned this pull request Dec 15, 2021

revert(#17570): fix dashboard editing for dashboards with non-ascii characters #17753

Closed

9 tasks

geido mentioned this pull request Dec 15, 2021

fix: Remove positions from json_metadata #17766

Merged

9 tasks

serenajiang mentioned this pull request Dec 16, 2021

fix(dashboard): commit update once #17781

Merged

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 🍒 1.4.0 🍒 1.4.1 🍒 1.4.2 labels Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Save properties after applying changes in Dashboard #17570

fix: Save properties after applying changes in Dashboard #17570

geido commented Nov 29, 2021 •

edited

Loading

codecov bot commented Dec 2, 2021 •

edited

Loading

geido Dec 2, 2021 •

edited

Loading

kgabryje left a comment

kgabryje left a comment

rusackas commented Dec 8, 2021

github-actions bot commented Dec 8, 2021

rusackas Dec 9, 2021

rusackas Dec 9, 2021 •

edited

Loading

rusackas left a comment

github-actions bot commented Dec 9, 2021

suddjian commented Dec 10, 2021

	throw new Error('A valid color scheme is required');
	throw new Error(t('A valid color scheme is required'));

fix: Save properties after applying changes in Dashboard #17570

fix: Save properties after applying changes in Dashboard #17570

Conversation

geido commented Nov 29, 2021 • edited Loading

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Dec 2, 2021 • edited Loading

Codecov Report

geido Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

kgabryje left a comment

Choose a reason for hiding this comment

kgabryje left a comment

Choose a reason for hiding this comment

rusackas commented Dec 8, 2021

github-actions bot commented Dec 8, 2021

rusackas Dec 9, 2021

Choose a reason for hiding this comment

rusackas Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

rusackas left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 9, 2021

suddjian commented Dec 10, 2021

geido commented Nov 29, 2021 •

edited

Loading

codecov bot commented Dec 2, 2021 •

edited

Loading

geido Dec 2, 2021 •

edited

Loading

rusackas Dec 9, 2021 •

edited

Loading