Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

fix(plugin-chart-table): refine ordering logic #930

Merged

Conversation

villebro
Copy link
Contributor

@villebro villebro commented Feb 3, 2021

🐛 Bug Fix
Previously table chart would by default order by the first metric if no sort order was specified. This replicates the logic from the old viz.py implementation and also fixes a few minor issues in the current code:

  • Infer queryMode from formData contents; if it's undefined and metrics is not empty, assume QueryMode.aggregate. This is the case in some old metadata, e.g. the World Bank example dashboard.
  • timeseries_limit_metric is [] when the control is unset, and QueryFormMetric when set. The type is updated and a util is added for normalizing to QueryFormMetric[] + relevant tests.
  • Added a missing license header.

AFTER

image

BEFORE

image

@villebro villebro requested a review from a team as a code owner February 3, 2021 15:05
@vercel
Copy link

vercel bot commented Feb 3, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/k57lswnpy
✅ Preview: https://superset-ui-git-fork-preset-io-villebro-fix-table-orderby.superset.now.sh

Comment on lines -18 to +38
if (timeseriesLimitMetric && orderDesc != null) {
orderby = [[timeseriesLimitMetric, !orderDesc]];
if (timeseriesLimitMetric.length > 0 && orderDesc != null) {
orderby = [[timeseriesLimitMetric[0], !orderDesc]];
Copy link
Contributor Author

@villebro villebro Feb 3, 2021

Choose a reason for hiding this comment

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

In practice timeseriesLimitMetric is always truthy, as the control always returns either an empty array (nothing selected) or a string/object (=an orderby is set). See the updated type in types.ts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, didn't notice the control returns an empty array when nothing is selected. That sounds wrong.

@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #930 (068d6cf) into master (84f8e45) will increase coverage by 0.04%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #930      +/-   ##
==========================================
+ Coverage   27.32%   27.37%   +0.04%     
==========================================
  Files         411      412       +1     
  Lines        8281     8292      +11     
  Branches     1134     1139       +5     
==========================================
+ Hits         2263     2270       +7     
- Misses       5886     5888       +2     
- Partials      132      134       +2     
Impacted Files Coverage Δ
plugins/plugin-chart-table/src/buildQuery.ts 68.42% <33.33%> (-16.20%) ⬇️
...ins/plugin-chart-table/src/utils/extractOrderby.ts 100.00% <100.00%> (ø)

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 84f8e45...068d6cf. Read the comment docs.

Copy link
Contributor

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix!

@villebro villebro merged commit f0d1bd5 into apache-superset:master Feb 3, 2021
@villebro villebro deleted the villebro/fix-table-orderby branch February 3, 2021 19:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants