-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
fix: Save properties after applying changes in Dashboard #17570
fix: Save properties after applying changes in Dashboard #17570
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
cy.wait('@dashboardGet').then(() => { | ||
selectColorScheme('d3Category20b'); | ||
}); | ||
|
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.
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
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
Outdated
Show resolved
Hide resolved
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.
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
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.
Thanks for the fixes! LGTM, but like I said, it would be great to have 1 more review since it's a big PR
/testenv up |
@rusackas Ephemeral environment spinning up at http://34.217.207.248:8080. Credentials are |
} | ||
|
||
Modal.error({ | ||
title: 'Error', |
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.
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
content: t('A valid color scheme is required'), | ||
okButtonProps: { danger: true, className: 'btn-danger' }, | ||
}); | ||
throw new Error('A valid color scheme is required'); |
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.
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!
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.
LGTM! Thanks for all this hard work!
Ephemeral environment shutdown and build artifacts deleted. |
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
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 theonlyApply
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:PropertiesModal
to use typescriptform
component to use the functionalities provided by Antdesignjson_metadata
forcolor_namespace
,expanded_slices
,timed_refresh_immune_slices
were not savedsave_dash
) with the standard PUT endpoint. Also, it adds theset_dash_metadata
function to theupdate
command in the backend. Theset_dash_metadata
function was only used by thesave_dash
endpoint before, causing potential discrepancies in the way thejson_metatada
was handled by the standard PUT endpoint.TESTING INSTRUCTIONS
ADDITIONAL INFORMATION