You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "john-bodley (via GitHub)" <gi...@apache.org> on 2023/03/01 23:43:07 UTC
[GitHub] [superset] john-bodley opened a new pull request, #23261: fix(dashboard): Disable 'Set filter mapping' when appropriate
john-bodley opened a new pull request, #23261:
URL: https://github.com/apache/superset/pull/23261
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
Though both legacy and native filters can coexist the behavior is not ideal. This PR ensures that if the `DASHBOARD_NATIVE_FILTERS` feature is enabled and no legacy filter mapping is defined then the option to display said mapping should be disabled.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
Added unit tests.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #23261: chore(dashboard): Disable 'Set filter mapping' when appropriate
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #23261:
URL: https://github.com/apache/superset/pull/23261#discussion_r1138633019
##########
superset-frontend/src/dashboard/components/Header/HeaderActionsDropdown/HeaderActionsDropdown.test.tsx:
##########
@@ -112,11 +116,84 @@ test('should render the menu items in edit mode', async () => {
setup(editModeOnProps);
expect(screen.getAllByRole('menuitem')).toHaveLength(4);
expect(screen.getByText('Set auto-refresh interval')).toBeInTheDocument();
- expect(screen.getByText('Set filter mapping')).toBeInTheDocument();
expect(screen.getByText('Edit properties')).toBeInTheDocument();
expect(screen.getByText('Edit CSS')).toBeInTheDocument();
});
+describe('with native filters feature flag disabled', () => {
+ beforeAll(() => {
+ isFeatureEnabledMock = jest
+ .spyOn(featureFlags, 'isFeatureEnabled')
+ .mockImplementation(
+ (featureFlag: FeatureFlag) =>
+ featureFlag !== FeatureFlag.DASHBOARD_NATIVE_FILTERS,
+ );
+ });
+
+ afterAll(() => {
+ // @ts-ignore
+ isFeatureEnabledMock.restore();
+ });
+
+ it('should render filter mapping in edit mode if explicit filter scopes undefined', async () => {
+ setup(editModeOnProps);
+ expect(screen.getByText('Set filter mapping')).toBeInTheDocument();
+ });
+
+ it('should render filter mapping in edit mode if explicit filter scopes defined', async () => {
+ const mockedProps = {
Review 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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #23261: chore(dashboard): Disable 'Set filter mapping' when appropriate
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #23261:
URL: https://github.com/apache/superset/pull/23261#discussion_r1138633019
##########
superset-frontend/src/dashboard/components/Header/HeaderActionsDropdown/HeaderActionsDropdown.test.tsx:
##########
@@ -112,11 +116,84 @@ test('should render the menu items in edit mode', async () => {
setup(editModeOnProps);
expect(screen.getAllByRole('menuitem')).toHaveLength(4);
expect(screen.getByText('Set auto-refresh interval')).toBeInTheDocument();
- expect(screen.getByText('Set filter mapping')).toBeInTheDocument();
expect(screen.getByText('Edit properties')).toBeInTheDocument();
expect(screen.getByText('Edit CSS')).toBeInTheDocument();
});
+describe('with native filters feature flag disabled', () => {
+ beforeAll(() => {
+ isFeatureEnabledMock = jest
+ .spyOn(featureFlags, 'isFeatureEnabled')
+ .mockImplementation(
+ (featureFlag: FeatureFlag) =>
+ featureFlag !== FeatureFlag.DASHBOARD_NATIVE_FILTERS,
+ );
+ });
+
+ afterAll(() => {
+ // @ts-ignore
+ isFeatureEnabledMock.restore();
+ });
+
+ it('should render filter mapping in edit mode if explicit filter scopes undefined', async () => {
+ setup(editModeOnProps);
+ expect(screen.getByText('Set filter mapping')).toBeInTheDocument();
+ });
+
+ it('should render filter mapping in edit mode if explicit filter scopes defined', async () => {
+ const mockedProps = {
Review Comment:
Nit: Can you extract `mockedProps` to a sharable constant similar to `editModeOnProps`? It could be something like `propsWithFilterScopes`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley merged pull request #23261: chore(dashboard): Disable 'Set filter mapping' when appropriate
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley merged PR #23261:
URL: https://github.com/apache/superset/pull/23261
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #23261: chore(dashboard): Disable 'Set filter mapping' when appropriate
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #23261:
URL: https://github.com/apache/superset/pull/23261#discussion_r1129814390
##########
superset-frontend/src/dashboard/components/Header/HeaderActionsDropdown/index.jsx:
##########
@@ -370,14 +372,18 @@ class HeaderActionsDropdown extends React.PureComponent {
</Menu>
)
) : null}
- {editMode && (
- <Menu.Item key={MENU_KEYS.SET_FILTER_MAPPING}>
- <FilterScopeModal
- className="m-r-5"
- triggerNode={t('Set filter mapping')}
- />
- </Menu.Item>
- )}
+ {editMode &&
+ !(
+ isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS) &&
+ isEmpty(dashboardInfo?.metadata?.filter_scopes)
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #23261: chore(dashboard): Disable 'Set filter mapping' when appropriate
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23261:
URL: https://github.com/apache/superset/pull/23261#issuecomment-1459073862
# [Codecov](https://codecov.io/gh/apache/superset/pull/23261?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#23261](https://codecov.io/gh/apache/superset/pull/23261?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d18e17f) into [master](https://codecov.io/gh/apache/superset/commit/7d8383cbd5f75e59d996d5c67c2576e7a333bb45?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7d8383c) will **decrease** coverage by `0.01%`.
> The diff coverage is `77.17%`.
> :exclamation: Current head d18e17f differs from pull request most recent head 8f547b2. Consider uploading reports for the commit 8f547b2 to get more accurate results
```diff
@@ Coverage Diff @@
## master #23261 +/- ##
==========================================
- Coverage 67.51% 67.51% -0.01%
==========================================
Files 1907 1909 +2
Lines 73436 73489 +53
Branches 7977 7988 +11
==========================================
+ Hits 49581 49615 +34
- Misses 21803 21824 +21
+ Partials 2052 2050 -2
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `53.78% <48.71%> (+0.01%)` | :arrow_up: |
| mysql | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/23261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...et-ui-chart-controls/src/operators/sortOperator.ts](https://codecov.io/gh/apache/superset/pull/23261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL29wZXJhdG9ycy9zb3J0T3BlcmF0b3IudHM=) | `100.00% <ø> (ø)` | |
| [...rt-controls/src/shared-controls/customControls.tsx](https://codecov.io/gh/apache/superset/pull/23261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9jdXN0b21Db250cm9scy50c3g=) | `17.39% <0.00%> (-7.61%)` | :arrow_down: |
| [...s/plugin-chart-pivot-table/src/PivotTableChart.tsx](https://codecov.io/gh/apache/superset/pull/23261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtcGl2b3QtdGFibGUvc3JjL1Bpdm90VGFibGVDaGFydC50c3g=) | `0.00% <0.00%> (ø)` | |
| [...tend/plugins/plugin-chart-table/src/TableChart.tsx](https://codecov.io/gh/apache/superset/pull/23261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtdGFibGUvc3JjL1RhYmxlQ2hhcnQudHN4) | `45.16% <0.00%> (-0.25%)` | :arrow_down: |
| [superset/common/query\_context\_processor.py](https://codecov.io/gh/apache/superset/pull/23261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X2NvbnRleHRfcHJvY2Vzc29yLnB5) | `88.47% <ø> (ø)` | |
| [superset/models/helpers.py](https://codecov.io/gh/apache/superset/pull/23261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL2hlbHBlcnMucHk=) | `38.16% <66.66%> (+0.13%)` | :arrow_up: |
| [.../plugin-chart-echarts/src/Timeseries/buildQuery.ts](https://codecov.io/gh/apache/superset/pull/23261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy9idWlsZFF1ZXJ5LnRz) | `70.00% <75.00%> (-1.43%)` | :arrow_down: |
| [...t-ui-chart-controls/src/operators/pivotOperator.ts](https://codecov.io/gh/apache/superset/pull/23261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL29wZXJhdG9ycy9waXZvdE9wZXJhdG9yLnRz) | `100.00% <100.00%> (ø)` | |
| [...ontrols/src/operators/utils/extractExtraMetrics.ts](https://codecov.io/gh/apache/superset/pull/23261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL29wZXJhdG9ycy91dGlscy9leHRyYWN0RXh0cmFNZXRyaWNzLnRz) | `100.00% <100.00%> (ø)` | |
| [...ages/superset-ui-core/src/utils/getSelectedText.ts](https://codecov.io/gh/apache/superset/pull/23261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdXRpbHMvZ2V0U2VsZWN0ZWRUZXh0LnRz) | `100.00% <100.00%> (ø)` | |
| ... and [19 more](https://codecov.io/gh/apache/superset/pull/23261?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #23261: chore(dashboard): Disable 'Set filter mapping' when appropriate
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #23261:
URL: https://github.com/apache/superset/pull/23261#discussion_r1123557971
##########
superset-frontend/src/dashboard/components/Header/HeaderActionsDropdown/HeaderActionsDropdown.test.tsx:
##########
@@ -112,11 +113,70 @@ test('should render the menu items in edit mode', async () => {
setup(editModeOnProps);
expect(screen.getAllByRole('menuitem')).toHaveLength(4);
expect(screen.getByText('Set auto-refresh interval')).toBeInTheDocument();
- expect(screen.getByText('Set filter mapping')).toBeInTheDocument();
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 () => {
Review Comment:
Seems like this test is failing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org