fix(explore): Metrics disappearing after removing metric from dataset

[kgabryje]

SUMMARY

Before, when a custom metric that was currently selected got removed from dataset, all selected metrics where cleared and user had to rebuild their chart.
This PR fixes that behaviour by removing from metrics picker only those custom metrics that were removed from dataset. Also, if user removes a column from dataset, the adhoc metrics that use that column will also be removed.
Also, it fixes another bug that's been around for long time - now adhoc metric labels display column's verbose name as a default label rather than it's name from database.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before: see linked issue
After:

Screen.Recording.2021-10-22.at.18.50.35.mov

TESTING INSTRUCTIONS

Follow the reproduction steps from linked issue, make sure that only the metric that got removed from dataset is removed from metrics picker.

ADDITIONAL INFORMATION

• Has associated issue: fixes [control][metrics] missing one custom metric will cause whole chart broken #16719, fixes [explore]changed label is not being picked up in metric popover #16384
• 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

CC @graceguo-supercat @rusackas @junlincc

[codecov]

Codecov Report

Merging #17201 (27449c3) into master (37909aa) will increase coverage by 0.04%.
The diff coverage is 76.00%.

❗ Current head 27449c3 differs from pull request most recent head 5353aa2. Consider uploading reports for the commit 5353aa2 to get more accurate results

@@            Coverage Diff             @@
##           master   #17201      +/-   ##
==========================================
+ Coverage   77.02%   77.07%   +0.04%     
==========================================
  Files        1037     1037              
  Lines       55629    55642      +13     
  Branches     7594     7599       +5     
==========================================
+ Hits        42850    42887      +37     
+ Misses      12529    12505      -24     
  Partials      250      250              

Flag	Coverage Δ
javascript	71.25% <76.00%> (+0.11%)	⬆️

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

Impacted Files	Coverage Δ
...mponents/controls/MetricControl/MetricsControl.jsx	38.18% <29.41%> (-3.88%)	⬇️
...ontrols/DndColumnSelectControl/DndMetricSelect.tsx	61.30% <100.00%> (+19.43%)	⬆️
...e/components/controls/MetricControl/AdhocMetric.js	97.33% <100.00%> (+0.07%)	⬆️
superset/db_engine_specs/presto.py	89.95% <0.00%> (-0.42%)	⬇️
...eFilters/FiltersConfigModal/FiltersConfigModal.tsx	87.71% <0.00%> (+0.58%)	⬆️
...s/controls/MetricControl/MetricDefinitionValue.jsx	100.00% <0.00%> (+20.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 37909aa...5353aa2. Read the comment docs.

[graceguo-supercat]

@kgabryje thanks so much for the fix! but in airbnb we didn't enable explore view's drag-n-drop. this fix didn't apply to us 😢

[villebro]

Looks great, especially the new test cases! Would be really nice to have type guards to distinguish saved from adhoc metrics, but those need to be addressed after monorepo is finished. A todo might be nice, but not mandatory IMO.

[kgabryje]

@graceguo-supercat Thanks for pointing that out! I updated the PR with fix for non-dnd controls. Would you mind checking it out?

[graceguo-supercat]

Hi @kgabryje thanks for the work! with the latest fix, non-dnd controls work good.

But here i have another question for this solution. In the current workflow, when front-end found any invalid metrics parameter, front-end will not display it, but the bad data is still in the Slice entity:

in the above example, max is invalid metrics. Front-end knows how to handle it, but should this bad data be buried quietly and stay in Slices table forever?

Ideally, Superset should let user know which piece of data is not valid anymore, and ask user to take action to clean up this bad data. right? cc @villebro @john-bodley @junlincc for suggestions

[graceguo-supercat]

Hi @kgabryje I think this PR is good so far, at least user can see the good part of the chart.
i created feature request: #17238. The extra work, to show error message and ask user to clean up bad data, we can do it in different PR. Thanks!

[kgabryje]

Thanks for reviewing Grace! I feel like your proposed change might require some PM and/or designer attention - let's continue the discussion in the issue you created