chore(dashboard): Disable 'Set filter mapping' when appropriate #23261
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
john-bodley
force-pushed
the
john-bodley--disable-check-filter-mapping
branch
from
6fa443c
to
1faa5b4
Compare
john-bodley
force-pushed
the
john-bodley--disable-check-filter-mapping
branch
from
1faa5b4
to
0382922
Compare
john-bodley
changed the title
john-bodley
changed the title
john-bodley
force-pushed
the
john-bodley--disable-check-filter-mapping
branch
from
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.
Seems like this test is failing
john-bodley
force-pushed
the
john-bodley--disable-check-filter-mapping
branch
6 times, most recently
from
137d984
to
37fd75c
Compare
john-bodley
force-pushed
the
john-bodley--disable-check-filter-mapping
branch
5 times, most recently
from
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 |
john-bodley
requested review from
Antonio-RiveroMartnez,
ktmud,
michael-s-molina and
rusackas
john-bodley
commented
Mar 8, 2023
| {editMode && | ||
| !( | ||
| isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS) && | ||
| isEmpty(dashboardInfo?.metadata?.filter_scopes) |
There was a problem hiding this comment.
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.
justinpark
approved these changes
Mar 14, 2023
michael-s-molina
approved these changes
Mar 16, 2023
| }); | ||
|
|
||
| it('should render filter mapping in edit mode if explicit filter scopes defined', async () => { | ||
| const mockedProps = { |
There was a problem hiding this comment.
Nit: Can you extract mockedProps to a sharable constant similar to editModeOnProps and reuse it in the tests? It could be something like propsWithFilterScopes
john-bodley
force-pushed
the
john-bodley--disable-check-filter-mapping
branch
from
8f547b2
to
07709a9
Compare
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_FILTERSfeature 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_FILTERSfeature 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