fix: Explore popovers issues #11428

kgabryje · 2020-10-26T15:53:40Z

SUMMARY

Fixes issues #11388 and #11404.
Changes in AdhocFilterOption might be in conflict with #11412 by @mistercrunch - unfortunately it seems that mutating a adhocFilter.isNew prop is necessary until we do a deeper refactor. Simply making that prop a part of the state leads to various issues regarding auto-opening the popup when new filter is added - particularly when there are multiple unsaved filters.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

As shown on the gif, the popup opens automatically and typing spaces and comas works properly.

TEST PLAN

ADDITIONAL INFORMATION

Has associated issue: Explore Query metrics value popup does not open automatically #11388, Backspaces and spaces do not work when editing simple filter value in explore chart #11404
Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

kgabryje · 2020-10-26T15:54:00Z

@mistercrunch Can you take a look?

superset-frontend/cypress-base/cypress/integration/explore/control.test.ts

superset-frontend/src/explore/components/AdhocFilterOption.jsx

rusackas

One nit, but not losing any sleep over it!

codecov-io · 2020-10-26T19:40:44Z

Codecov Report

Merging #11428 into master will decrease coverage by 9.63%.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##           master   #11428      +/-   ##
==========================================
- Coverage   66.59%   56.96%   -9.64%     
==========================================
  Files         863      408     -455     
  Lines       40986    13656   -27330     
  Branches     3694     3478     -216     
==========================================
- Hits        27295     7779   -19516     
+ Misses      13594     5715    -7879     
- Partials       97      162      +65

Flag	Coverage Δ
#cypress	`56.96% <93.54%> (+0.05%)`	⬆️
#javascript	`?`
#python	`?`

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

Impacted Files	Coverage Δ
...explore/components/controls/AdhocFilterControl.jsx	`68.93% <ø> (-8.74%)`	⬇️
...ntend/src/explore/components/AdhocFilterOption.jsx	`89.28% <88.23%> (+8.64%)`	⬆️
...uperset-frontend/src/common/components/Popover.tsx	`100.00% <100.00%> (ø)`
...ntend/src/explore/components/AdhocMetricOption.jsx	`93.75% <100.00%> (+3.12%)`	⬆️
...uperset-frontend/src/dashboard/util/dnd-reorder.js	`0.00% <0.00%> (-100.00%)`	⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js	`0.00% <0.00%> (-100.00%)`	⬇️
...et-frontend/src/components/Menu/LanguagePicker.tsx	`0.00% <0.00%> (-100.00%)`	⬇️
...dashboard/components/resizable/ResizableHandle.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js	`0.00% <0.00%> (-93.48%)`	⬇️
...uperset-frontend/src/utils/getClientErrorObject.ts	`0.00% <0.00%> (-89.19%)`	⬇️
... and 685 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 eecabf9...4ecf5ee. Read the comment docs.

ktmud · 2020-10-27T19:13:59Z

Not sure if it was the original behavior, but while testing this, I noticed that selecting saved metrics (such as count in the energy_usage example dataset) did not auto-open the popover. So I pushed a fix: 7ab7a53

ktmud · 2020-10-27T23:23:45Z

Some unrelated tests are failing. I've reran the tests two times.


    1) chart card view
         should edit correctly:
       AssertionError: Timed out retrying: Expected to find element: `[data-test="properties-name-input"]`, but never found it.
        at Context.eval (http://localhost:8081/__cypress/tests?p=cypress/integration/chart_list/card_view.test.ts:199:55)

@kgabryje @nytai could you take a look?

rusackas · 2020-10-27T23:57:23Z

Some unrelated tests are failing. I've reran the tests two times.


    1) chart card view
         should edit correctly:
       AssertionError: Timed out retrying: Expected to find element: `[data-test="properties-name-input"]`, but never found it.
        at Context.eval (http://localhost:8081/__cypress/tests?p=cypress/integration/chart_list/card_view.test.ts:199:55)

@kgabryje @nytai could you take a look?

Seeing that same test causing issues on another PR. We're going to skip it for now in another PR, and then circle back to fix it separately.

ktmud · 2020-10-28T04:59:56Z

I noticed that the popover for adhoc metrics are not fixed. So I pushed another fix.

ktmud · 2020-10-28T05:00:07Z

superset-frontend/src/common/components/Popover.tsx


-const SupersetPopover = styled(AntdPopover)``;
+const SupersetPopover = AntdPopover;


Let's add it back when we do need overrides.

rusackas · 2020-10-28T06:00:03Z

A rebase should fix that failing unit test now.

kgabryje · 2020-10-28T11:42:07Z

@ktmud Thank you for your fixes! openPopover method wasn't used anywhere so I removed it. I also fixed that failing E2E test.

* Fix spaces and comas not working in filter popover * Fix popup not opening automatically * Add e2e test * Remove only from test * Remove redundant test, add checking label content * Add comments to e2e test * Fix unit test * Use destructuring * Always open popup for functions and saved metrics, too * Fix popover for adhoc metrics too * Small refactor to consistency * Refactor for consistency * Remove redundant functions * Test fix Co-authored-by: Jesse Yang <jesse.yang@airbnb.com> (cherry picked from commit b2636f0)

* Fix spaces and comas not working in filter popover * Fix popup not opening automatically * Add e2e test * Remove only from test * Remove redundant test, add checking label content * Add comments to e2e test * Fix unit test * Use destructuring * Always open popup for functions and saved metrics, too * Fix popover for adhoc metrics too * Small refactor to consistency * Refactor for consistency * Remove redundant functions * Test fix Co-authored-by: Jesse Yang <jesse.yang@airbnb.com>

superset-github-bot bot added the polidea label Oct 26, 2020

pull-request-size bot added the size/M label Oct 26, 2020

kgabryje mentioned this pull request Oct 26, 2020

fix: multiple issues with FilterPopover #11412

Merged

ktmud reviewed Oct 26, 2020

View reviewed changes

superset-frontend/cypress-base/cypress/integration/explore/control.test.ts Outdated Show resolved Hide resolved

mistercrunch approved these changes Oct 26, 2020

View reviewed changes

rusackas reviewed Oct 26, 2020

View reviewed changes

superset-frontend/src/explore/components/AdhocFilterOption.jsx Show resolved Hide resolved

rusackas approved these changes Oct 26, 2020

View reviewed changes

mistercrunch closed this Oct 26, 2020

mistercrunch reopened this Oct 26, 2020

ktmud force-pushed the fix/explore-popovers branch 3 times, most recently from 2bd75da to 7ab7a53 Compare October 27, 2020 19:13

ktmud force-pushed the fix/explore-popovers branch from 7ab7a53 to f7c5c65 Compare October 27, 2020 20:45

pull-request-size bot added size/L and removed size/M labels Oct 27, 2020

ktmud reviewed Oct 28, 2020

View reviewed changes

ktmud force-pushed the fix/explore-popovers branch from 7d104da to 24c65b2 Compare October 28, 2020 05:56

ktmud force-pushed the fix/explore-popovers branch from 24c65b2 to 9dec0bf Compare October 28, 2020 06:06

kgabryje added 5 commits October 28, 2020 11:56

Fix spaces and comas not working in filter popover

2a1d9fc

Fix popup not opening automatically

626f22c

Add e2e test

4f093ee

Remove only from test

921ff28

Remove redundant test, add checking label content

4c4efd8

ktmud and others added 3 commits October 28, 2020 11:56

Refactor for consistency

6934650

Remove redundant functions

5953258

Test fix

4ecf5ee

kgabryje force-pushed the fix/explore-popovers branch from 9dec0bf to 4ecf5ee Compare October 28, 2020 11:37

ktmud approved these changes Oct 28, 2020

View reviewed changes

ktmud merged commit b2636f0 into apache:master Oct 28, 2020

mistercrunch mentioned this pull request Oct 31, 2020

fix: Explore popovers issues (#11428) preset-io/superset#35

Closed

kgabryje mentioned this pull request Nov 2, 2020

Backspaces and spaces do not work when editing simple filter value in explore chart #11404

Closed

3 tasks

laxmanraparthi mentioned this pull request Jun 28, 2023

[Snyk] Fix for 13 vulnerabilities ShareChat/superset#15

Open

This was referenced Nov 26, 2023

[Snyk] Fix for 10 vulnerabilities ShareChat/superset#56

Open

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 ShareChat/superset#57

Open

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 ShareChat/superset#61

Open

m-ajay mentioned this pull request Nov 28, 2023

[Snyk] Fix for 15 vulnerabilities m-ajay/superset#200

Open

This was referenced Dec 14, 2023

[Snyk] Fix for 15 vulnerabilities matrix-compute/superset#13

Open

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 matrix-compute/superset#21

Open

This was referenced Dec 20, 2023

[Snyk] Fix for 13 vulnerabilities ShareChat/superset#97

Open

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 ShareChat/superset#100

Open

matrix-compute mentioned this pull request Dec 26, 2023

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 matrix-compute/superset#28

Open

laxmanraparthi mentioned this pull request Jan 2, 2024

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 ShareChat/superset#107

Open

matrix-compute mentioned this pull request Feb 16, 2024

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 matrix-compute/superset#52

Open

matrix-compute mentioned this pull request Mar 11, 2024

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 matrix-compute/superset#68

Merged

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024

laxmanraparthi mentioned this pull request Mar 15, 2024

[Snyk] Fix for 22 vulnerabilities ShareChat/superset#156

Open

matrix-compute mentioned this pull request Mar 23, 2024

[Snyk] Fix for 14 vulnerabilities matrix-compute/superset#78

Open

This was referenced May 14, 2024

[Snyk] Fix for 3 vulnerabilities matrix-compute/superset#133

Open

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 matrix-compute/superset#136

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Explore popovers issues #11428

fix: Explore popovers issues #11428

kgabryje commented Oct 26, 2020 •

edited

Loading

kgabryje commented Oct 26, 2020

rusackas left a comment

codecov-io commented Oct 26, 2020 •

edited

Loading

ktmud commented Oct 27, 2020 •

edited

Loading

ktmud commented Oct 27, 2020

rusackas commented Oct 27, 2020

ktmud commented Oct 28, 2020

ktmud Oct 28, 2020

rusackas commented Oct 28, 2020

kgabryje commented Oct 28, 2020


		const SupersetPopover = styled(AntdPopover)``;
		const SupersetPopover = AntdPopover;

fix: Explore popovers issues #11428

fix: Explore popovers issues #11428

Conversation

kgabryje commented Oct 26, 2020 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

kgabryje commented Oct 26, 2020

rusackas left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 26, 2020 • edited Loading

Codecov Report

ktmud commented Oct 27, 2020 • edited Loading

ktmud commented Oct 27, 2020

rusackas commented Oct 27, 2020

ktmud commented Oct 28, 2020

ktmud Oct 28, 2020

Choose a reason for hiding this comment

rusackas commented Oct 28, 2020

kgabryje commented Oct 28, 2020

kgabryje commented Oct 26, 2020 •

edited

Loading

codecov-io commented Oct 26, 2020 •

edited

Loading

ktmud commented Oct 27, 2020 •

edited

Loading