-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
chore(dashboard): Disable 'Set filter mapping' when appropriate #23261
chore(dashboard): Disable 'Set filter mapping' when appropriate #23261
Conversation
6fa443c
to
1faa5b4
Compare
1faa5b4
to
0382922
Compare
0382922
to
be702f3
Compare
expect(screen.getByText('Edit properties')).toBeInTheDocument(); | ||
expect(screen.getByText('Edit CSS')).toBeInTheDocument(); | ||
}); | ||
|
||
test('should render filter mapping in edit mode if DASHBOARD_NATIVE_FILTERS disabled and mapping undefined', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this test is failing
137d984
to
37fd75c
Compare
2ec74b4
to
8f547b2
Compare
Codecov Report
@@ Coverage Diff @@
## master #23261 +/- ##
==========================================
+ Coverage 67.51% 67.58% +0.07%
==========================================
Files 1907 1907
Lines 73436 73531 +95
Branches 7977 7981 +4
==========================================
+ Hits 49581 49698 +117
+ Misses 21803 21785 -18
+ Partials 2052 2048 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
{editMode && | ||
!( | ||
isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS) && | ||
isEmpty(dashboardInfo?.metadata?.filter_scopes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment in the PR description. I think it's fine to look at the filter_scopes
field (since this is what setting the filter mapping configures) rather than checking for the non-presence of filter-box charts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
}); | ||
|
||
it('should render filter mapping in edit mode if explicit filter scopes defined', async () => { | ||
const mockedProps = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Can you extract mockedProps
to a sharable constant similar to editModeOnProps
and reuse it in the tests? It could be something like propsWithFilterScopes
8f547b2
to
07709a9
Compare
SUMMARY
This PR addresses recommendation #5 in #14383.
The "set filter mapping" dashboard menu option (see screenshot) is used to configure the filter scopes for legacy (filter-box) filters. By defaults filter scopes are implicitly defined, i.e., charts have no immunity to filter-boxes, however users can explicitly specify them (by defining their scope and inclusion/exclusion logic).
Though both legacy and native filters can coexist the behavior is not ideal from a UX perspective, i.e., when the
DASHBOARD_NATIVE_FILTERS
feature is enabled users should strive to solely use the native filters functionality/modal to define filtering whilst simultaneously removing legacy filter-box charts.This PR ensures that if the
DASHBOARD_NATIVE_FILTERS
feature is enabled and no legacy filter mapping is explicitly defined (filter-box charts can be present*) then the menu option to set the mapping should not be present, i.e., we should prevent users from having the ability to define legacy filter scopes. The TL;DR is we want to prevent users from creating new legacy filter scopes when the feature is enabled.* If filter-box charts are present then (per my previous comment) implicit filter scope logic is still adhered to even if there's no explicitly defined legacy filter scopes. The TL;DR with this change users wont be able to configure the filter-box charts' scopes/immunity, however users should be encouraged to simply remove the filter-box charts from the dashboard and replace them with native filters.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Added unit tests.
ADDITIONAL INFORMATION