You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/04/25 21:02:25 UTC
[GitHub] [superset] codemaster08240328 opened a new pull request, #19841: Fix table chart column config issue
codemaster08240328 opened a new pull request, #19841:
URL: https://github.com/apache/superset/pull/19841
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR resolved column config issue which happened when we update the chart.
### How To Check
1. Create a table chart with custom Metric.
2. Change the alignment in customize tab.
3. Update the custom Metric name.
4. Update the chart.
Previously changed alignment should be kept in the updated chart.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
[[DEV] Add new chart - 25 April 2022 - Watch Video](https://www.loom.com/share/ceb1977b5ad841918dc0c4f192ab6dc9)
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
[[DEV] Add new chart - 25 April 2022 - Watch Video](https://www.loom.com/share/af61215d8e404199b3b8d3492f589c05)
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Fix Table Chart Column Config Bug
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codemaster08240328 commented on a diff in pull request #19841: fix: Table chart column config issue
Posted by GitBox <gi...@apache.org>.
codemaster08240328 commented on code in PR #19841:
URL: https://github.com/apache/superset/pull/19841#discussion_r861166103
##########
superset-frontend/src/explore/reducers/exploreReducer.js:
##########
@@ -131,11 +131,30 @@ export default function exploreReducer(state = {}, action) {
},
[actions.SET_FIELD_VALUE]() {
const new_form_data = state.form_data;
+ const old_metrics_data = state.form_data.metrics;
+ const new_column_config = state.form_data.column_config;
const { controlName, value, validationErrors } = action;
new_form_data[controlName] = value;
const vizType = new_form_data.viz_type;
+ // if the controlName is metrics, and the metric column name is updated,
+ // need to update column config as well to keep the previou config.
+ if (controlName === 'metrics' && old_metrics_data && new_column_config) {
Review Comment:
same here.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #19841: Fix table chart column config issue
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #19841:
URL: https://github.com/apache/superset/pull/19841#issuecomment-1109079316
# [Codecov](https://codecov.io/gh/apache/superset/pull/19841?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19841](https://codecov.io/gh/apache/superset/pull/19841?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bfe83ab) into [master](https://codecov.io/gh/apache/superset/commit/ae384111c1887a4d18fbd78e66868ea4184243f1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ae38411) will **decrease** coverage by `0.00%`.
> The diff coverage is `44.44%`.
> :exclamation: Current head bfe83ab differs from pull request most recent head 49b3754. Consider uploading reports for the commit 49b3754 to get more accurate results
```diff
@@ Coverage Diff @@
## master #19841 +/- ##
==========================================
- Coverage 66.55% 66.55% -0.01%
==========================================
Files 1692 1692
Lines 64802 64811 +9
Branches 6657 6657
==========================================
+ Hits 43129 43133 +4
- Misses 19973 19978 +5
Partials 1700 1700
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.25% <44.44%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19841?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...et-frontend/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/superset/pull/19841/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZXhwbG9yZVJlZHVjZXIuanM=) | `27.84% <44.44%> (+2.13%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19841?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19841?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ae38411...49b3754](https://codecov.io/gh/apache/superset/pull/19841?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codemaster08240328 commented on a diff in pull request #19841: fix: Table chart column config issue
Posted by GitBox <gi...@apache.org>.
codemaster08240328 commented on code in PR #19841:
URL: https://github.com/apache/superset/pull/19841#discussion_r861165828
##########
superset-frontend/src/explore/reducers/exploreReducer.js:
##########
@@ -131,11 +131,30 @@ export default function exploreReducer(state = {}, action) {
},
[actions.SET_FIELD_VALUE]() {
const new_form_data = state.form_data;
+ const old_metrics_data = state.form_data.metrics;
+ const new_column_config = state.form_data.column_config;
const { controlName, value, validationErrors } = action;
new_form_data[controlName] = value;
const vizType = new_form_data.viz_type;
+ // if the controlName is metrics, and the metric column name is updated,
+ // need to update column config as well to keep the previou config.
+ if (controlName === 'metrics' && old_metrics_data && new_column_config) {
+ value.forEach((item, index) => {
+ if (
+ item.label !== old_metrics_data[index].label &&
+ !!new_column_config[old_metrics_data[index].label]
+ ) {
+ new_column_config[item.label] =
+ new_column_config[old_metrics_data[index].label];
+
+ delete new_column_config[old_metrics_data[index].label];
+ }
+ });
+ new_form_data.column_config = new_column_config;
Review Comment:
same here.
If you see line#137, we already used this assigning method into original state.
No need to do shallow copy of original state because we return the state as a result in Reducer, again.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas merged pull request #19841: fix: Table chart column config issue
Posted by GitBox <gi...@apache.org>.
rusackas merged PR #19841:
URL: https://github.com/apache/superset/pull/19841
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] diegomedina248 commented on a diff in pull request #19841: fix: Table chart column config issue
Posted by GitBox <gi...@apache.org>.
diegomedina248 commented on code in PR #19841:
URL: https://github.com/apache/superset/pull/19841#discussion_r859113134
##########
superset-frontend/src/explore/reducers/exploreReducer.js:
##########
@@ -131,11 +131,30 @@ export default function exploreReducer(state = {}, action) {
},
[actions.SET_FIELD_VALUE]() {
const new_form_data = state.form_data;
+ const old_metrics_data = state.form_data.metrics;
+ const new_column_config = state.form_data.column_config;
Review Comment:
since we're modifying these two below, shouldn't we copy these two? Unless shallowly, like `{...state.form_data.column_config}`.
##########
superset-frontend/src/explore/reducers/exploreReducer.js:
##########
@@ -148,9 +167,18 @@ export default function exploreReducer(state = {}, action) {
...getControlStateFromControlConfig(controlConfig, state, action.value),
};
+ const column_config = {
+ ...state.controls.column_config,
+ ...(new_column_config && { value: new_column_config }),
+ };
+
const newState = {
...state,
- controls: { ...state.controls, [action.controlName]: control },
+ controls: {
+ ...state.controls,
+ [action.controlName]: control,
Review Comment:
you can use `controlName` directly here
##########
superset-frontend/src/explore/reducers/exploreReducer.js:
##########
@@ -131,11 +131,30 @@ export default function exploreReducer(state = {}, action) {
},
[actions.SET_FIELD_VALUE]() {
const new_form_data = state.form_data;
+ const old_metrics_data = state.form_data.metrics;
+ const new_column_config = state.form_data.column_config;
const { controlName, value, validationErrors } = action;
new_form_data[controlName] = value;
const vizType = new_form_data.viz_type;
+ // if the controlName is metrics, and the metric column name is updated,
+ // need to update column config as well to keep the previou config.
+ if (controlName === 'metrics' && old_metrics_data && new_column_config) {
+ value.forEach((item, index) => {
+ if (
+ item.label !== old_metrics_data[index].label &&
+ !!new_column_config[old_metrics_data[index].label]
+ ) {
+ new_column_config[item.label] =
+ new_column_config[old_metrics_data[index].label];
+
+ delete new_column_config[old_metrics_data[index].label];
+ }
+ });
+ new_form_data.column_config = new_column_config;
Review Comment:
`new_form_data` is part of the original state, we shouldn't be mutating it
##########
superset-frontend/src/explore/reducers/exploreReducer.js:
##########
@@ -131,11 +131,30 @@ export default function exploreReducer(state = {}, action) {
},
[actions.SET_FIELD_VALUE]() {
const new_form_data = state.form_data;
+ const old_metrics_data = state.form_data.metrics;
+ const new_column_config = state.form_data.column_config;
const { controlName, value, validationErrors } = action;
new_form_data[controlName] = value;
const vizType = new_form_data.viz_type;
+ // if the controlName is metrics, and the metric column name is updated,
+ // need to update column config as well to keep the previou config.
+ if (controlName === 'metrics' && old_metrics_data && new_column_config) {
Review Comment:
consider making this pure, avoiding mutations.
This could result in unexpected bugs in other places of the app if not.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codemaster08240328 commented on a diff in pull request #19841: fix: Table chart column config issue
Posted by GitBox <gi...@apache.org>.
codemaster08240328 commented on code in PR #19841:
URL: https://github.com/apache/superset/pull/19841#discussion_r861167382
##########
superset-frontend/src/explore/reducers/exploreReducer.js:
##########
@@ -148,9 +167,18 @@ export default function exploreReducer(state = {}, action) {
...getControlStateFromControlConfig(controlConfig, state, action.value),
};
+ const column_config = {
+ ...state.controls.column_config,
+ ...(new_column_config && { value: new_column_config }),
+ };
+
const newState = {
...state,
- controls: { ...state.controls, [action.controlName]: control },
+ controls: {
+ ...state.controls,
+ [action.controlName]: control,
Review Comment:
Correct, we can use it.
Actually this change was made by vsCode auto lint, which means original code was like it. 🤣
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codemaster08240328 commented on a diff in pull request #19841: fix: Table chart column config issue
Posted by GitBox <gi...@apache.org>.
codemaster08240328 commented on code in PR #19841:
URL: https://github.com/apache/superset/pull/19841#discussion_r861164650
##########
superset-frontend/src/explore/reducers/exploreReducer.js:
##########
@@ -131,11 +131,30 @@ export default function exploreReducer(state = {}, action) {
},
[actions.SET_FIELD_VALUE]() {
const new_form_data = state.form_data;
+ const old_metrics_data = state.form_data.metrics;
+ const new_column_config = state.form_data.column_config;
Review Comment:
@diegomedina248 , no need, because this is reducer section, and in reducer we return the state. So mutating old data has no effect to setting state again. There is no need to do shallow copy in reducer.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org