Skip to content
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(chart & alert): make to show metrics properly #19939

Merged
merged 4 commits into from
May 16, 2022

Conversation

prosdev0107
Copy link
Contributor

SUMMARY

Altered Modal Doesn't Show Metrics Properly In Table Chart

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:
image

AFTER:
image

TESTING INSTRUCTIONS

How to reproduce bug

  1. Open a chart (table chart)
  2. Remove the column in the "Metric" field and add a different column
  3. Click the yellow "Altered" button at the top of the page

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

Copy link
Contributor

@diegomedina248 diegomedina248 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Anyway we could add a test in cypress? (we have a bunch of tests around charts, maybe we can add this one too).

@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #19939 (057e5d3) into master (e1f53f2) will decrease coverage by 0.00%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master   #19939      +/-   ##
==========================================
- Coverage   66.53%   66.52%   -0.01%     
==========================================
  Files        1714     1714              
  Lines       65044    65063      +19     
  Branches     6723     6721       -2     
==========================================
+ Hits        43278    43286       +8     
- Misses      20055    20069      +14     
+ Partials     1711     1708       -3     
Flag Coverage Δ
javascript 51.26% <60.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...-frontend/src/components/AlteredSliceTag/index.jsx 88.57% <60.00%> (+0.33%) ⬆️
...tend/src/visualizations/FilterBox/controlPanel.jsx 0.00% <0.00%> (-37.50%) ⬇️
superset-frontend/src/views/store.ts 80.00% <0.00%> (-10.00%) ⬇️
...ntend/src/explore/components/ExploreChartPanel.jsx 70.49% <0.00%> (-1.18%) ⬇️
superset-frontend/src/embedded/index.tsx 0.00% <0.00%> (ø)
superset-frontend/src/types/bootstrapTypes.ts 100.00% <0.00%> (ø)
...rset-frontend/src/dashboard/util/findPermission.ts 92.30% <0.00%> (ø)
...tend/plugins/plugin-chart-table/src/TableChart.tsx 38.57% <0.00%> (ø)
...nd/src/explore/components/DataTablesPane/index.tsx 71.55% <0.00%> (ø)
...es/superset-ui-core/src/chart/models/ChartProps.ts 100.00% <0.00%> (ø)
... and 6 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 e1f53f2...057e5d3. Read the comment docs.

@rusackas
Copy link
Member

rusackas commented May 3, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented May 3, 2022

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

@yousoph
Copy link
Member

yousoph commented May 4, 2022

@prosdev0107 I see metrics twice in the control panel now:
image

@ktmud
Copy link
Member

ktmud commented May 4, 2022

I don't think this is the right way to fix this issue. You'd probably want to update the "Chart changes" renderer instead of the customized metrics controls.

@@ -263,6 +263,7 @@ const config: ControlPanelConfig = {
},
],
['adhoc_filters'],
['metrics'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check instead what is the other metrics panel doing, it must be transforming the props or something

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me check

if (typeof value === 'boolean') {
return value ? 'true' : 'false';
}
if (value.constructor === Array) {
return value.length ? value.join(', ') : '[]';
const formattedValue = value.map(v => (v.label ? v.label : v));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could potentially be called for other control types aside from metrics.
Why did you move it from above? Was the code not entering? We're essentially switching from Array.isArray to value.constructor, which should have similar effect.
If that's the only reason, we could replace Array.isArray with value.constructor in the other block you removed, but keeping it locked to the metrics control.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diegomedina248
It does make sense. Resolved.

@pull-request-size pull-request-size bot added size/S and removed size/XS labels May 5, 2022
Copy link
Contributor

@diegomedina248 diegomedina248 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rusackas LGTM, please live test again.

@yousoph
Copy link
Member

yousoph commented May 13, 2022

/testenv up

@github-actions
Copy link
Contributor

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

@yousoph
Copy link
Member

yousoph commented May 13, 2022

a quick test on the ephemeral was looking good to me

@rusackas rusackas merged commit 55aef4d into apache:master May 16, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
* fix(chart & alert): make to show metrics properly

* fix(chart & alert): make to remove duplicate metrics

* fix(chart & alert): make to restore metrics control alert slice

* fix(chart & alert): make to fix lint issue
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels Preset-Patch size/S 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants