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(dashboard): Fix FilterWithDataMask typing and add null check #22260

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

codyml
Copy link
Member

@codyml codyml commented Nov 29, 2022

SUMMARY

This PR fixes a typing error that enabled an undefined key access error: dataMask isn't guaranteed to exist on type FilterWithDataMask because the source of that key, DataMaskStateWithId, isn't guaranteed to have a value for each filter ID.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Example of error:
204566163-6539cf34-efa5-44d4-93bd-3fa1a90919a8

TESTING INSTRUCTIONS

I had trouble reproducing this issue locally, but try various combinations of filled and unfilled filters and ensure that it doesn't happen again.

ADDITIONAL INFORMATION

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

@codyml codyml changed the title Fix dataMask typing and add null check. fix(dashboard): Fix FilterWithDataMask typing Nov 29, 2022
@codyml codyml changed the title fix(dashboard): Fix FilterWithDataMask typing fix(dashboard): Fix FilterWithDataMask typing and add null check Nov 29, 2022
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.

LGTM

@codyml
Copy link
Member Author

codyml commented Nov 29, 2022

Thanks for the reviews, @villebro or @kgabryje feel free to merge!

@villebro villebro merged commit a642d12 into apache:master Nov 29, 2022
@@ -128,7 +128,7 @@ const FilterControls: FC<FilterControlsProps> = ({
const activeOverflowedFiltersInScope = useMemo(
() =>
overflowedFiltersInScope.filter(
filter => isNativeFilter(filter) && filter.dataMask.filterState?.value,
filter => isNativeFilter(filter) && filter.dataMask?.filterState?.value,
Copy link
Member

Choose a reason for hiding this comment

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

Currently, there are isNativeFilter and isFilterDivider as the type guard, so I think we should introduce a new type guard for this use case, for example, isFilterWithDataMask.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #22307!

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 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 size/XS 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants