Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(Explore): Pivot table V2 sort by failure with D&D enabled #18835

Merged
merged 4 commits into from
Feb 28, 2022
Merged

fix(Explore): Pivot table V2 sort by failure with D&D enabled #18835

merged 4 commits into from
Feb 28, 2022

Conversation

geido
Copy link
Member

@geido geido commented Feb 21, 2022

SUMMARY

This PR fixes an issue for which the Pivot Table V2 would fail when the "Sort by" control was cleared with D&D enabled.
The reason for the failure was that when the "Sort by" was cleared, the series_limit_metric was passed as an empty array due to the default behavior of the Select control.
With the changes in this PR, the series_limit_metric type is strictly checked and converted to undefined when the selection is cleared up / an empty array is passed.

BEFORE

DEV.Video.Game.Sales.Pivot.Table.V2.mp4

AFTER

DEV.Video.Game.Sales.Pivot.Table.V2.1.mp4

TESTING INSTRUCTIONS

  1. Open a Pivot Table V2 with D&D enabled
  2. Add a "Sort by" option
  3. Clear the "Sort by"
  4. Make sure the data is loaded as expected

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags: ENABLE_EXPLORE_DRAG_AND_DROP ENABLE_DND_WITH_CLICK_UX
  • 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

@codecov
Copy link

codecov bot commented Feb 21, 2022

Codecov Report

Merging #18835 (6e72937) into master (c491829) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #18835   +/-   ##
=======================================
  Coverage   66.36%   66.37%           
=======================================
  Files        1620     1620           
  Lines       63041    63046    +5     
  Branches     6375     6376    +1     
=======================================
+ Hits        41839    41844    +5     
  Misses      19543    19543           
  Partials     1659     1659           
Flag Coverage Δ
javascript 51.26% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...ges/superset-ui-core/src/query/buildQueryObject.ts 100.00% <100.00%> (ø)
...ackages/superset-ui-core/src/query/types/Column.ts 100.00% <100.00%> (ø)
...mponents/controls/AnnotationLayerControl/index.jsx 10.16% <0.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 c491829...6e72937. Read the comment docs.

Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

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

Tested, works perfectly!

@geido geido merged commit eafe0cf into apache:master Feb 28, 2022
villebro pushed a commit that referenced this pull request Apr 3, 2022
* wip

* Add tests and clean up

* Clean up

* Remove unused import

(cherry picked from commit eafe0cf)
@mistercrunch mistercrunch added 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels lts-v1 size/M 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants