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(explore): Don't allow to add a duplicated saved metric to controls #12657

Merged
merged 1 commit into from Jan 21, 2021

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Jan 21, 2021

SUMMARY

When the same saved metric is added twice to controls, the copies are indistinguishable - when the user tries to edit one saved metric, both are getting edited. This PR disables the possibility of adding the same saved metric more than once by removing options from metrics popover

Fixes #12611
Closes #12611

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Nagranie.z.ekranu.2021-01-21.o.13.48.19.mov

TEST PLAN

ADDITIONAL INFORMATION

CC: @junlincc @villebro @john-bodley @ktmud @adam-stasiak
This PR replaces #12619

@kgabryje kgabryje changed the title Don't allow to add a duplicated saved metric to controls fix: Don't allow to add a duplicated saved metric to controls Jan 21, 2021
@codecov-io
Copy link

codecov-io commented Jan 21, 2021

Codecov Report

Merging #12657 (f4cb911) into master (0de61df) will decrease coverage by 9.76%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12657      +/-   ##
==========================================
- Coverage   66.85%   57.09%   -9.77%     
==========================================
  Files        1018      961      -57     
  Lines       49776    47001    -2775     
  Branches     4869     4383     -486     
==========================================
- Hits        33280    26834    -6446     
- Misses      16373    20167    +3794     
+ Partials      123        0     -123     
Flag Coverage Δ
cypress 45.13% <84.61%> (-5.83%) ⬇️
javascript ?
python 63.88% <ø> (-0.14%) ⬇️

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

Impacted Files Coverage Δ
...nents/controls/MetricControl/AdhocMetricOption.jsx 100.00% <ø> (ø)
...ntrols/MetricControl/AdhocMetricPopoverTrigger.tsx 93.10% <ø> (+1.43%) ⬆️
...s/controls/MetricControl/MetricDefinitionValue.jsx 88.23% <ø> (-5.89%) ⬇️
.../controls/MetricControl/AdhocMetricEditPopover.jsx 76.80% <83.33%> (-2.57%) ⬇️
...mponents/controls/MetricControl/MetricsControl.jsx 55.88% <85.71%> (-33.76%) ⬇️
...uperset-frontend/src/dashboard/util/dnd-reorder.js 0.00% <0.00%> (-100.00%) ⬇️
...erset-frontend/src/common/hooks/useChangeEffect.ts 0.00% <0.00%> (-100.00%) ⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js 0.00% <0.00%> (-100.00%) ⬇️
...t-frontend/src/dashboard/containers/SliceAdder.jsx 0.00% <0.00%> (-100.00%) ⬇️
...frontend/src/dashboard/util/dropOverflowsParent.js 0.00% <0.00%> (-100.00%) ⬇️
... and 418 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 0de61df...f4cb911. Read the comment docs.

@junlincc junlincc added the explore:metrics Related to metrics of Explore label Jan 21, 2021
@junlincc junlincc changed the title fix: Don't allow to add a duplicated saved metric to controls fix(explore): Don't allow to add a duplicated saved metric to controls Jan 21, 2021
@adam-stasiak
Copy link
Contributor

tested manually - looks fine. 🟢
One question - Should all chosen metrics be removed when I delete from Edit Dataset menu one of them?
The same behavior is outside this PR.
Scenario:

  1. add metrics to Query section
  2. Go to edit dataset
  3. remove one of chosen metrics

Observed: All chosen Metrics are not set anymore

Copy link
Member

@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

@ktmud ktmud merged commit 6280b34 into apache:master Jan 21, 2021
john-bodley pushed a commit to airbnb/superset-fork that referenced this pull request Jan 22, 2021
@mistercrunch mistercrunch added 🍒 1.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 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 explore:metrics Related to metrics of Explore size/M v1.0.1 🍒 1.0.1 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[explore]Non-metric uniqueness in Explore
7 participants