You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "richardfogaca (via GitHub)" <gi...@apache.org> on 2026/04/14 22:18:51 UTC
[PR] fix(dashboard): apply dynamic groupby display controls to scoped charts [superset]
richardfogaca opened a new pull request, #39356:
URL: https://github.com/apache/superset/pull/39356
### SUMMARY
This is the focused extraction of the core fix from #39354.
It keeps only the changes required for the reported dashboard bug where a Dynamic Group By display control either has no visible effect on scoped charts or appears to keep the previous grouping.
Included in this PR:
- Fix dynamic group-by query generation so selected columns are applied as chart dimensions instead of being injected as spurious row filters.
- Allow valid group-by selections even when the selected column already exists in the chart's base `groupby`.
- Normalize single-select values so scoped charts receive the expected `groupby` array shape.
- Migrate legacy chart customization items before scope calculation so in-scope charts are actually updated.
- Add direct regression coverage for `getFormDataWithExtraFilters` and `DashboardContainer`.
Intentionally excluded from this focused PR:
- persisted multi-select canonicalization and clear-all cleanup
- config modal save/edit persistence changes
- indicator/selectors follow-up cleanup
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Skip.
### TESTING INSTRUCTIONS
1. Create an aggregate table chart with a `groupby` field and add it to a dashboard.
2. Add a Dynamic Group By display control scoped to that chart.
3. Select a column and click Apply Filters.
4. Expected: the chart re-queries with the selected column in `groupby` and without a spurious `filters` clause containing the selected column name as a value.
5. Also verify legacy dashboard metadata still scopes the customization correctly.
Automated:
- `npm run test -- src/dashboard/util/getFormDataWithExtraFilters.test.ts`
- `npm run test -- src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsx`
Validation note:
- `pre-commit run --all-files` was executed in the isolated worktree, but the frontend type-check step hits existing baseline declaration issues unrelated to this patch (`dom-to-image-more` / `dom-to-pdf` resolution in `src/utils/downloadAsImage.tsx` and `src/utils/downloadAsPdf.ts`).
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] 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
Re: [PR] fix(dashboard): apply dynamic groupby display controls to scoped charts [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on PR #39356:
URL: https://github.com/apache/superset/pull/39356#issuecomment-4253772016
hey @aminghadersohi , thanks for reviewing this PR :)
The chord-specific branch is leftover from an earlier attempt to support chord in the generic dynamic groupby flow, but chord was later excluded after local testing exposed regressions. So in the current PR state that branch is dead code.
I don’t think we should re-enable chord in this PR just to make that path reachable. Chord is a legacy special case: it still models source/target through groupby + columns, and the backend builds its query from those fields directly, so it’s riskier than the other charts covered here. The safer approach is to keep chord excluded in this PR and remove the unreachable branch so the intent is explicit.
I also checked the existingGroupBy concern. For the chart types still participating in this flow, I don’t see an obvious duplication issue from that change, but I agree it was worth validating.
The jest.restoreAllMocks() cleanup suggestion is reasonable too, and I can fold that in
--
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
Re: [PR] fix(dashboard): apply dynamic groupby display controls to scoped charts [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39356:
URL: https://github.com/apache/superset/pull/39356#discussion_r3082830914
##########
superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts:
##########
@@ -22,189 +22,436 @@ import getFormDataWithExtraFilters, {
} from 'src/dashboard/util/charts/getFormDataWithExtraFilters';
import { sliceId as chartId } from 'spec/fixtures/mockChartQueries';
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
-describe('getFormDataWithExtraFilters', () => {
- const filterId = 'native-filter-1';
- const mockChart = {
- id: chartId,
- chartAlert: null,
- chartStatus: null,
- chartUpdateEndTime: null,
- chartUpdateStartTime: 1,
- lastRendered: 1,
- latestQueryFormData: {},
- sliceFormData: null,
- queryController: null,
- queriesResponse: null,
- triggerQuery: false,
- form_data: {
- viz_type: 'filter_select',
- filters: [
- {
- col: 'country_name',
- op: 'IN',
- val: ['United States'],
- },
- ],
- datasource: '123',
- url_params: {},
- },
- };
- const mockArgs: GetFormDataWithExtraFiltersArguments = {
- chartConfiguration: {},
- chart: mockChart,
- filters: {
- region: ['Spain'],
- color: ['pink', 'purple'],
- },
- sliceId: chartId,
- nativeFilters: {},
- dataMask: {
- [filterId]: {
- id: filterId,
- extraFormData: {},
- filterState: {},
- ownState: {},
+const expectGroupBy = (
+ result: CachedFormDataWithExtraControls,
+ expected: unknown,
+) => {
+ expect('groupby' in result).toBe(true);
+ if (!('groupby' in result)) {
+ throw new Error('Expected groupby to be present in form data');
+ }
+ expect(result.groupby).toEqual(expected);
+};
+
+const expectGroupByLength = (
+ result: CachedFormDataWithExtraControls,
+ length: number,
+) => {
+ expect('groupby' in result).toBe(true);
+ if (!('groupby' in result)) {
+ throw new Error('Expected groupby to be present in form data');
+ }
+ expect(result.groupby).toHaveLength(length);
+};
+
+const getResultFilters = (result: CachedFormDataWithExtraControls) => {
+ if (!('filters' in result) || !Array.isArray(result.filters)) {
+ return [];
+ }
+
+ return result.filters.filter(
+ (
+ filter,
+ ): filter is {
+ col: string;
+ val: unknown[];
+ } =>
+ typeof filter === 'object' &&
+ filter !== null &&
+ 'col' in filter &&
+ 'val' in filter &&
+ typeof filter.col === 'string' &&
+ Array.isArray(filter.val),
+ );
+};
+
+const filterId = 'native-filter-1';
+const mockChart = {
+ id: chartId,
+ chartAlert: null,
+ chartStatus: null,
+ chartUpdateEndTime: null,
+ chartUpdateStartTime: 1,
+ lastRendered: 1,
+ latestQueryFormData: {},
+ sliceFormData: null,
+ queryController: null,
+ queriesResponse: null,
+ triggerQuery: false,
+ form_data: {
+ viz_type: 'filter_select',
+ filters: [
+ {
+ col: 'country_name',
+ op: 'IN',
+ val: ['United States'],
},
+ ],
+ datasource: '123',
+ url_params: {},
+ },
+};
+const mockArgs: GetFormDataWithExtraFiltersArguments = {
+ chartConfiguration: {},
+ chart: mockChart,
+ filters: {
+ region: ['Spain'],
+ color: ['pink', 'purple'],
+ },
+ sliceId: chartId,
+ nativeFilters: {},
+ dataMask: {
+ [filterId]: {
+ id: filterId,
+ extraFormData: {},
+ filterState: {},
+ ownState: {},
},
- extraControls: {
- stack: 'Stacked',
- },
- allSliceIds: [chartId],
- };
+ },
+ extraControls: {
+ stack: 'Stacked',
+ },
+ allSliceIds: [chartId],
+};
- test('should include filters from the passed filters', () => {
- const result = getFormDataWithExtraFilters(mockArgs);
- expect(result.extra_filters).toHaveLength(2);
- expect(result.extra_filters[0]).toEqual({
- col: 'region',
- op: 'IN',
- val: ['Spain'],
- });
- expect(result.extra_filters[1]).toEqual({
- col: 'color',
- op: 'IN',
- val: ['pink', 'purple'],
- });
+test('should include filters from the passed filters', () => {
+ const result = getFormDataWithExtraFilters(mockArgs);
+ expect(result.extra_filters).toHaveLength(2);
+ expect(result.extra_filters[0]).toEqual({
+ col: 'region',
+ op: 'IN',
+ val: ['Spain'],
});
-
- test('should compose extra control', () => {
- const result: CachedFormDataWithExtraControls =
- getFormDataWithExtraFilters(mockArgs);
- expect(result.stack).toEqual('Stacked');
+ expect(result.extra_filters[1]).toEqual({
+ col: 'color',
+ op: 'IN',
+ val: ['pink', 'purple'],
});
+});
- test('should merge extraFormData from chart customizations', () => {
- const customizationId = 'CHART_CUSTOMIZATION-1';
- const argsWithCustomization: GetFormDataWithExtraFiltersArguments = {
- ...mockArgs,
- dataMask: {
- [customizationId]: {
- id: customizationId,
- extraFormData: {
- time_grain_sqla: 'PT1H',
- },
- filterState: {
- value: ['category1', 'category2'],
- },
- ownState: {},
+test('should compose extra control', () => {
+ const result: CachedFormDataWithExtraControls =
+ getFormDataWithExtraFilters(mockArgs);
+ expect(result.stack).toEqual('Stacked');
+});
+
+test('should merge extraFormData from chart customizations', () => {
+ const customizationId = 'CHART_CUSTOMIZATION-1';
+ const argsWithCustomization: GetFormDataWithExtraFiltersArguments = {
+ ...mockArgs,
+ dataMask: {
+ [customizationId]: {
+ id: customizationId,
+ extraFormData: {
+ time_grain_sqla: 'PT1H',
},
+ filterState: {
+ value: ['category1', 'category2'],
+ },
+ ownState: {},
},
- chartCustomizationItems: [
- {
- id: customizationId,
- type: 'CHART_CUSTOMIZATION' as any,
- name: 'Time Grain Customization',
- filterType: 'chart_customization_time_grain',
- targets: [
- {
- datasetId: 123,
- column: { name: 'time_column' },
- },
- ],
- scope: {
- rootPath: [],
- excluded: [],
+ },
+ chartCustomizationItems: [
+ {
+ id: customizationId,
+ type: 'CHART_CUSTOMIZATION' as any,
+ name: 'Time Grain Customization',
+ filterType: 'chart_customization_time_grain',
+ targets: [
+ {
+ datasetId: 123,
+ column: { name: 'time_column' },
},
- chartsInScope: [chartId],
- defaultDataMask: {},
- controlValues: {},
+ ],
+ scope: {
+ rootPath: [],
+ excluded: [],
},
- ],
- };
+ chartsInScope: [chartId],
+ defaultDataMask: {},
+ controlValues: {},
+ },
+ ],
+ };
- const result = getFormDataWithExtraFilters(argsWithCustomization);
- expect((result as any).time_grain_sqla).toEqual('PT1H');
- });
+ const result = getFormDataWithExtraFilters(argsWithCustomization);
+ expect((result as any).time_grain_sqla).toEqual('PT1H');
+});
- test('should merge both filters and customization extraFormData', () => {
- const customizationId = 'CHART_CUSTOMIZATION-1';
- const argsWithBoth: GetFormDataWithExtraFiltersArguments = {
- ...mockArgs,
- chartConfiguration: {
- [filterId]: {
- id: filterId,
- targets: [
+test('should merge both filters and customization extraFormData', () => {
+ const customizationId = 'CHART_CUSTOMIZATION-1';
+ const argsWithBoth: GetFormDataWithExtraFiltersArguments = {
+ ...mockArgs,
+ chartConfiguration: {
+ [filterId]: {
+ id: filterId,
+ targets: [
+ {
+ datasetId: 123,
+ column: { name: 'country_name' },
+ },
+ ],
+ scope: { rootPath: ['ROOT_ID'], excluded: [] },
+ cascadeParentIds: [],
+ },
+ } as any,
Review Comment:
**Suggestion:** Replace the `any` cast on `chartConfiguration` with the concrete argument field type to keep strict typing in test setup. [custom_rule]
**Severity Level:** Minor ⚠️
```suggestion
} as GetFormDataWithExtraFiltersArguments['chartConfiguration'],
```
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The existing code does use `as any` on `chartConfiguration`, so the typing rule violation is real. The proposed replacement is syntactically correct, and the imported `GetFormDataWithExtraFiltersArguments` type is already in scope, so the indexed access type is available here.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts
**Line:** 197:197
**Comment:**
*Custom Rule: Replace the `any` cast on `chartConfiguration` with the concrete argument field type to keep strict typing in test setup.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39356&comment_hash=7021be9e402b6d2f502dc2192666221e79089bbc42fedc4d0999391f16d0c259&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39356&comment_hash=7021be9e402b6d2f502dc2192666221e79089bbc42fedc4d0999391f16d0c259&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts:
##########
@@ -22,189 +22,436 @@ import getFormDataWithExtraFilters, {
} from 'src/dashboard/util/charts/getFormDataWithExtraFilters';
import { sliceId as chartId } from 'spec/fixtures/mockChartQueries';
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
-describe('getFormDataWithExtraFilters', () => {
- const filterId = 'native-filter-1';
- const mockChart = {
- id: chartId,
- chartAlert: null,
- chartStatus: null,
- chartUpdateEndTime: null,
- chartUpdateStartTime: 1,
- lastRendered: 1,
- latestQueryFormData: {},
- sliceFormData: null,
- queryController: null,
- queriesResponse: null,
- triggerQuery: false,
- form_data: {
- viz_type: 'filter_select',
- filters: [
- {
- col: 'country_name',
- op: 'IN',
- val: ['United States'],
- },
- ],
- datasource: '123',
- url_params: {},
- },
- };
- const mockArgs: GetFormDataWithExtraFiltersArguments = {
- chartConfiguration: {},
- chart: mockChart,
- filters: {
- region: ['Spain'],
- color: ['pink', 'purple'],
- },
- sliceId: chartId,
- nativeFilters: {},
- dataMask: {
- [filterId]: {
- id: filterId,
- extraFormData: {},
- filterState: {},
- ownState: {},
+const expectGroupBy = (
+ result: CachedFormDataWithExtraControls,
+ expected: unknown,
+) => {
+ expect('groupby' in result).toBe(true);
+ if (!('groupby' in result)) {
+ throw new Error('Expected groupby to be present in form data');
+ }
+ expect(result.groupby).toEqual(expected);
+};
+
+const expectGroupByLength = (
+ result: CachedFormDataWithExtraControls,
+ length: number,
+) => {
+ expect('groupby' in result).toBe(true);
+ if (!('groupby' in result)) {
+ throw new Error('Expected groupby to be present in form data');
+ }
+ expect(result.groupby).toHaveLength(length);
+};
+
+const getResultFilters = (result: CachedFormDataWithExtraControls) => {
+ if (!('filters' in result) || !Array.isArray(result.filters)) {
+ return [];
+ }
+
+ return result.filters.filter(
+ (
+ filter,
+ ): filter is {
+ col: string;
+ val: unknown[];
+ } =>
+ typeof filter === 'object' &&
+ filter !== null &&
+ 'col' in filter &&
+ 'val' in filter &&
+ typeof filter.col === 'string' &&
+ Array.isArray(filter.val),
+ );
+};
+
+const filterId = 'native-filter-1';
+const mockChart = {
+ id: chartId,
+ chartAlert: null,
+ chartStatus: null,
+ chartUpdateEndTime: null,
+ chartUpdateStartTime: 1,
+ lastRendered: 1,
+ latestQueryFormData: {},
+ sliceFormData: null,
+ queryController: null,
+ queriesResponse: null,
+ triggerQuery: false,
+ form_data: {
+ viz_type: 'filter_select',
+ filters: [
+ {
+ col: 'country_name',
+ op: 'IN',
+ val: ['United States'],
},
+ ],
+ datasource: '123',
+ url_params: {},
+ },
+};
+const mockArgs: GetFormDataWithExtraFiltersArguments = {
+ chartConfiguration: {},
+ chart: mockChart,
+ filters: {
+ region: ['Spain'],
+ color: ['pink', 'purple'],
+ },
+ sliceId: chartId,
+ nativeFilters: {},
+ dataMask: {
+ [filterId]: {
+ id: filterId,
+ extraFormData: {},
+ filterState: {},
+ ownState: {},
},
- extraControls: {
- stack: 'Stacked',
- },
- allSliceIds: [chartId],
- };
+ },
+ extraControls: {
+ stack: 'Stacked',
+ },
+ allSliceIds: [chartId],
+};
- test('should include filters from the passed filters', () => {
- const result = getFormDataWithExtraFilters(mockArgs);
- expect(result.extra_filters).toHaveLength(2);
- expect(result.extra_filters[0]).toEqual({
- col: 'region',
- op: 'IN',
- val: ['Spain'],
- });
- expect(result.extra_filters[1]).toEqual({
- col: 'color',
- op: 'IN',
- val: ['pink', 'purple'],
- });
+test('should include filters from the passed filters', () => {
+ const result = getFormDataWithExtraFilters(mockArgs);
+ expect(result.extra_filters).toHaveLength(2);
+ expect(result.extra_filters[0]).toEqual({
+ col: 'region',
+ op: 'IN',
+ val: ['Spain'],
});
-
- test('should compose extra control', () => {
- const result: CachedFormDataWithExtraControls =
- getFormDataWithExtraFilters(mockArgs);
- expect(result.stack).toEqual('Stacked');
+ expect(result.extra_filters[1]).toEqual({
+ col: 'color',
+ op: 'IN',
+ val: ['pink', 'purple'],
});
+});
- test('should merge extraFormData from chart customizations', () => {
- const customizationId = 'CHART_CUSTOMIZATION-1';
- const argsWithCustomization: GetFormDataWithExtraFiltersArguments = {
- ...mockArgs,
- dataMask: {
- [customizationId]: {
- id: customizationId,
- extraFormData: {
- time_grain_sqla: 'PT1H',
- },
- filterState: {
- value: ['category1', 'category2'],
- },
- ownState: {},
+test('should compose extra control', () => {
+ const result: CachedFormDataWithExtraControls =
+ getFormDataWithExtraFilters(mockArgs);
+ expect(result.stack).toEqual('Stacked');
+});
+
+test('should merge extraFormData from chart customizations', () => {
+ const customizationId = 'CHART_CUSTOMIZATION-1';
+ const argsWithCustomization: GetFormDataWithExtraFiltersArguments = {
+ ...mockArgs,
+ dataMask: {
+ [customizationId]: {
+ id: customizationId,
+ extraFormData: {
+ time_grain_sqla: 'PT1H',
},
+ filterState: {
+ value: ['category1', 'category2'],
+ },
+ ownState: {},
},
- chartCustomizationItems: [
- {
- id: customizationId,
- type: 'CHART_CUSTOMIZATION' as any,
- name: 'Time Grain Customization',
- filterType: 'chart_customization_time_grain',
- targets: [
- {
- datasetId: 123,
- column: { name: 'time_column' },
- },
- ],
- scope: {
- rootPath: [],
- excluded: [],
+ },
+ chartCustomizationItems: [
+ {
+ id: customizationId,
+ type: 'CHART_CUSTOMIZATION' as any,
Review Comment:
**Suggestion:** Remove the `any` cast on the customization type and use the string literal directly so the object is type-checked. [custom_rule]
**Severity Level:** Minor ⚠️
```suggestion
type: 'CHART_CUSTOMIZATION',
```
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The file still contains this `as any` assertion in the chart customization fixture, so the rule violation is real. Replacing it with the string literal keeps the value the same while removing the unnecessary `any` cast, and the literal is valid in this test setup.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts
**Line:** 157:157
**Comment:**
*Custom Rule: Remove the `any` cast on the customization type and use the string literal directly so the object is type-checked.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39356&comment_hash=06f50f96a6d018a1bd189b18af88395e766c01bea090118667c6f9e9771480bb&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39356&comment_hash=06f50f96a6d018a1bd189b18af88395e766c01bea090118667c6f9e9771480bb&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts:
##########
@@ -22,189 +22,436 @@ import getFormDataWithExtraFilters, {
} from 'src/dashboard/util/charts/getFormDataWithExtraFilters';
import { sliceId as chartId } from 'spec/fixtures/mockChartQueries';
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
-describe('getFormDataWithExtraFilters', () => {
- const filterId = 'native-filter-1';
- const mockChart = {
- id: chartId,
- chartAlert: null,
- chartStatus: null,
- chartUpdateEndTime: null,
- chartUpdateStartTime: 1,
- lastRendered: 1,
- latestQueryFormData: {},
- sliceFormData: null,
- queryController: null,
- queriesResponse: null,
- triggerQuery: false,
- form_data: {
- viz_type: 'filter_select',
- filters: [
- {
- col: 'country_name',
- op: 'IN',
- val: ['United States'],
- },
- ],
- datasource: '123',
- url_params: {},
- },
- };
- const mockArgs: GetFormDataWithExtraFiltersArguments = {
- chartConfiguration: {},
- chart: mockChart,
- filters: {
- region: ['Spain'],
- color: ['pink', 'purple'],
- },
- sliceId: chartId,
- nativeFilters: {},
- dataMask: {
- [filterId]: {
- id: filterId,
- extraFormData: {},
- filterState: {},
- ownState: {},
+const expectGroupBy = (
+ result: CachedFormDataWithExtraControls,
+ expected: unknown,
+) => {
+ expect('groupby' in result).toBe(true);
+ if (!('groupby' in result)) {
+ throw new Error('Expected groupby to be present in form data');
+ }
+ expect(result.groupby).toEqual(expected);
+};
+
+const expectGroupByLength = (
+ result: CachedFormDataWithExtraControls,
+ length: number,
+) => {
+ expect('groupby' in result).toBe(true);
+ if (!('groupby' in result)) {
+ throw new Error('Expected groupby to be present in form data');
+ }
+ expect(result.groupby).toHaveLength(length);
+};
+
+const getResultFilters = (result: CachedFormDataWithExtraControls) => {
+ if (!('filters' in result) || !Array.isArray(result.filters)) {
+ return [];
+ }
+
+ return result.filters.filter(
+ (
+ filter,
+ ): filter is {
+ col: string;
+ val: unknown[];
+ } =>
+ typeof filter === 'object' &&
+ filter !== null &&
+ 'col' in filter &&
+ 'val' in filter &&
+ typeof filter.col === 'string' &&
+ Array.isArray(filter.val),
+ );
+};
+
+const filterId = 'native-filter-1';
+const mockChart = {
+ id: chartId,
+ chartAlert: null,
+ chartStatus: null,
+ chartUpdateEndTime: null,
+ chartUpdateStartTime: 1,
+ lastRendered: 1,
+ latestQueryFormData: {},
+ sliceFormData: null,
+ queryController: null,
+ queriesResponse: null,
+ triggerQuery: false,
+ form_data: {
+ viz_type: 'filter_select',
+ filters: [
+ {
+ col: 'country_name',
+ op: 'IN',
+ val: ['United States'],
},
+ ],
+ datasource: '123',
+ url_params: {},
+ },
+};
+const mockArgs: GetFormDataWithExtraFiltersArguments = {
+ chartConfiguration: {},
+ chart: mockChart,
+ filters: {
+ region: ['Spain'],
+ color: ['pink', 'purple'],
+ },
+ sliceId: chartId,
+ nativeFilters: {},
+ dataMask: {
+ [filterId]: {
+ id: filterId,
+ extraFormData: {},
+ filterState: {},
+ ownState: {},
},
- extraControls: {
- stack: 'Stacked',
- },
- allSliceIds: [chartId],
- };
+ },
+ extraControls: {
+ stack: 'Stacked',
+ },
+ allSliceIds: [chartId],
+};
- test('should include filters from the passed filters', () => {
- const result = getFormDataWithExtraFilters(mockArgs);
- expect(result.extra_filters).toHaveLength(2);
- expect(result.extra_filters[0]).toEqual({
- col: 'region',
- op: 'IN',
- val: ['Spain'],
- });
- expect(result.extra_filters[1]).toEqual({
- col: 'color',
- op: 'IN',
- val: ['pink', 'purple'],
- });
+test('should include filters from the passed filters', () => {
+ const result = getFormDataWithExtraFilters(mockArgs);
+ expect(result.extra_filters).toHaveLength(2);
+ expect(result.extra_filters[0]).toEqual({
+ col: 'region',
+ op: 'IN',
+ val: ['Spain'],
});
-
- test('should compose extra control', () => {
- const result: CachedFormDataWithExtraControls =
- getFormDataWithExtraFilters(mockArgs);
- expect(result.stack).toEqual('Stacked');
+ expect(result.extra_filters[1]).toEqual({
+ col: 'color',
+ op: 'IN',
+ val: ['pink', 'purple'],
});
+});
- test('should merge extraFormData from chart customizations', () => {
- const customizationId = 'CHART_CUSTOMIZATION-1';
- const argsWithCustomization: GetFormDataWithExtraFiltersArguments = {
- ...mockArgs,
- dataMask: {
- [customizationId]: {
- id: customizationId,
- extraFormData: {
- time_grain_sqla: 'PT1H',
- },
- filterState: {
- value: ['category1', 'category2'],
- },
- ownState: {},
+test('should compose extra control', () => {
+ const result: CachedFormDataWithExtraControls =
+ getFormDataWithExtraFilters(mockArgs);
+ expect(result.stack).toEqual('Stacked');
+});
+
+test('should merge extraFormData from chart customizations', () => {
+ const customizationId = 'CHART_CUSTOMIZATION-1';
+ const argsWithCustomization: GetFormDataWithExtraFiltersArguments = {
+ ...mockArgs,
+ dataMask: {
+ [customizationId]: {
+ id: customizationId,
+ extraFormData: {
+ time_grain_sqla: 'PT1H',
},
+ filterState: {
+ value: ['category1', 'category2'],
+ },
+ ownState: {},
},
- chartCustomizationItems: [
- {
- id: customizationId,
- type: 'CHART_CUSTOMIZATION' as any,
- name: 'Time Grain Customization',
- filterType: 'chart_customization_time_grain',
- targets: [
- {
- datasetId: 123,
- column: { name: 'time_column' },
- },
- ],
- scope: {
- rootPath: [],
- excluded: [],
+ },
+ chartCustomizationItems: [
+ {
+ id: customizationId,
+ type: 'CHART_CUSTOMIZATION' as any,
+ name: 'Time Grain Customization',
+ filterType: 'chart_customization_time_grain',
+ targets: [
+ {
+ datasetId: 123,
+ column: { name: 'time_column' },
},
- chartsInScope: [chartId],
- defaultDataMask: {},
- controlValues: {},
+ ],
+ scope: {
+ rootPath: [],
+ excluded: [],
},
- ],
- };
+ chartsInScope: [chartId],
+ defaultDataMask: {},
+ controlValues: {},
+ },
+ ],
+ };
- const result = getFormDataWithExtraFilters(argsWithCustomization);
- expect((result as any).time_grain_sqla).toEqual('PT1H');
- });
+ const result = getFormDataWithExtraFilters(argsWithCustomization);
+ expect((result as any).time_grain_sqla).toEqual('PT1H');
+});
- test('should merge both filters and customization extraFormData', () => {
- const customizationId = 'CHART_CUSTOMIZATION-1';
- const argsWithBoth: GetFormDataWithExtraFiltersArguments = {
- ...mockArgs,
- chartConfiguration: {
- [filterId]: {
- id: filterId,
- targets: [
+test('should merge both filters and customization extraFormData', () => {
+ const customizationId = 'CHART_CUSTOMIZATION-1';
+ const argsWithBoth: GetFormDataWithExtraFiltersArguments = {
+ ...mockArgs,
+ chartConfiguration: {
+ [filterId]: {
+ id: filterId,
+ targets: [
+ {
+ datasetId: 123,
+ column: { name: 'country_name' },
+ },
+ ],
+ scope: { rootPath: ['ROOT_ID'], excluded: [] },
+ cascadeParentIds: [],
+ },
+ } as any,
+ dataMask: {
+ [filterId]: {
+ id: filterId,
+ extraFormData: {
+ filters: [
{
- datasetId: 123,
- column: { name: 'country_name' },
+ col: 'country_name',
+ op: 'IN',
+ val: ['United States'],
},
],
- scope: { rootPath: ['ROOT_ID'], excluded: [] },
- cascadeParentIds: [],
},
- } as any,
- dataMask: {
- [filterId]: {
- id: filterId,
- extraFormData: {
- filters: [
- {
- col: 'country_name',
- op: 'IN',
- val: ['United States'],
- },
- ],
- },
- filterState: {},
- ownState: {},
+ filterState: {},
+ ownState: {},
+ },
+ [customizationId]: {
+ id: customizationId,
+ extraFormData: {
+ time_grain_sqla: 'PT1H',
},
- [customizationId]: {
- id: customizationId,
- extraFormData: {
- time_grain_sqla: 'PT1H',
- },
- filterState: {
- value: ['category1'],
- },
- ownState: {},
+ filterState: {
+ value: ['category1'],
},
+ ownState: {},
},
- chartCustomizationItems: [
- {
- id: customizationId,
- type: 'CHART_CUSTOMIZATION' as any,
- name: 'Time Grain Customization',
- filterType: 'chart_customization_time_grain',
- targets: [
- {
- datasetId: 123,
- column: { name: 'time_column' },
- },
- ],
- scope: {
- rootPath: [],
- excluded: [],
+ },
+ chartCustomizationItems: [
+ {
+ id: customizationId,
+ type: 'CHART_CUSTOMIZATION' as any,
+ name: 'Time Grain Customization',
+ filterType: 'chart_customization_time_grain',
+ targets: [
+ {
+ datasetId: 123,
+ column: { name: 'time_column' },
},
- chartsInScope: [chartId],
- defaultDataMask: {},
- controlValues: {},
+ ],
+ scope: {
+ rootPath: [],
+ excluded: [],
},
- ],
- };
+ chartsInScope: [chartId],
+ defaultDataMask: {},
+ controlValues: {},
+ },
+ ],
+ };
+
+ const result = getFormDataWithExtraFilters(argsWithBoth);
+ expect((result as any).time_grain_sqla).toEqual('PT1H');
+ expect(result.extra_form_data).toBeDefined();
+});
+
+const makeGroupByArgs = (
+ selectedValue: string | string[],
+ baseGroupby: string[] = [],
+): GetFormDataWithExtraFiltersArguments => {
+ const customizationId = 'CHART_CUSTOMIZATION-groupby-1';
+ return {
+ ...mockArgs,
+ chart: {
+ ...mockChart,
+ form_data: {
+ ...mockChart.form_data,
+ viz_type: 'table',
+ datasource: '3__table',
+ groupby: baseGroupby,
+ },
+ },
+ dataMask: {
+ [customizationId]: {
+ id: customizationId,
+ extraFormData: {},
+ filterState: { value: selectedValue },
+ ownState: {},
+ },
+ },
+ chartCustomizationItems: [
+ {
+ id: customizationId,
+ type: 'CHART_CUSTOMIZATION' as any,
Review Comment:
**Suggestion:** Remove the `any` assertion from the dynamic group-by customization type and keep the value strongly typed. [custom_rule]
**Severity Level:** Minor ⚠️
```suggestion
type: 'CHART_CUSTOMIZATION',
```
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is the same real `as any` pattern, but in another fixture. The fix is safe and syntactically correct because it only removes an unnecessary assertion without changing the underlying literal value.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts
**Line:** 279:279
**Comment:**
*Custom Rule: Remove the `any` assertion from the dynamic group-by customization type and keep the value strongly typed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39356&comment_hash=43ed0a0a877f85321fcb91597e8ade903fe81989eb369fb75bde2291db836367&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39356&comment_hash=43ed0a0a877f85321fcb91597e8ade903fe81989eb369fb75bde2291db836367&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts:
##########
@@ -22,189 +22,436 @@ import getFormDataWithExtraFilters, {
} from 'src/dashboard/util/charts/getFormDataWithExtraFilters';
import { sliceId as chartId } from 'spec/fixtures/mockChartQueries';
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
-describe('getFormDataWithExtraFilters', () => {
- const filterId = 'native-filter-1';
- const mockChart = {
- id: chartId,
- chartAlert: null,
- chartStatus: null,
- chartUpdateEndTime: null,
- chartUpdateStartTime: 1,
- lastRendered: 1,
- latestQueryFormData: {},
- sliceFormData: null,
- queryController: null,
- queriesResponse: null,
- triggerQuery: false,
- form_data: {
- viz_type: 'filter_select',
- filters: [
- {
- col: 'country_name',
- op: 'IN',
- val: ['United States'],
- },
- ],
- datasource: '123',
- url_params: {},
- },
- };
- const mockArgs: GetFormDataWithExtraFiltersArguments = {
- chartConfiguration: {},
- chart: mockChart,
- filters: {
- region: ['Spain'],
- color: ['pink', 'purple'],
- },
- sliceId: chartId,
- nativeFilters: {},
- dataMask: {
- [filterId]: {
- id: filterId,
- extraFormData: {},
- filterState: {},
- ownState: {},
+const expectGroupBy = (
+ result: CachedFormDataWithExtraControls,
+ expected: unknown,
+) => {
+ expect('groupby' in result).toBe(true);
+ if (!('groupby' in result)) {
+ throw new Error('Expected groupby to be present in form data');
+ }
+ expect(result.groupby).toEqual(expected);
+};
+
+const expectGroupByLength = (
+ result: CachedFormDataWithExtraControls,
+ length: number,
+) => {
+ expect('groupby' in result).toBe(true);
+ if (!('groupby' in result)) {
+ throw new Error('Expected groupby to be present in form data');
+ }
+ expect(result.groupby).toHaveLength(length);
+};
+
+const getResultFilters = (result: CachedFormDataWithExtraControls) => {
+ if (!('filters' in result) || !Array.isArray(result.filters)) {
+ return [];
+ }
+
+ return result.filters.filter(
+ (
+ filter,
+ ): filter is {
+ col: string;
+ val: unknown[];
+ } =>
+ typeof filter === 'object' &&
+ filter !== null &&
+ 'col' in filter &&
+ 'val' in filter &&
+ typeof filter.col === 'string' &&
+ Array.isArray(filter.val),
+ );
+};
+
+const filterId = 'native-filter-1';
+const mockChart = {
+ id: chartId,
+ chartAlert: null,
+ chartStatus: null,
+ chartUpdateEndTime: null,
+ chartUpdateStartTime: 1,
+ lastRendered: 1,
+ latestQueryFormData: {},
+ sliceFormData: null,
+ queryController: null,
+ queriesResponse: null,
+ triggerQuery: false,
+ form_data: {
+ viz_type: 'filter_select',
+ filters: [
+ {
+ col: 'country_name',
+ op: 'IN',
+ val: ['United States'],
},
+ ],
+ datasource: '123',
+ url_params: {},
+ },
+};
+const mockArgs: GetFormDataWithExtraFiltersArguments = {
+ chartConfiguration: {},
+ chart: mockChart,
+ filters: {
+ region: ['Spain'],
+ color: ['pink', 'purple'],
+ },
+ sliceId: chartId,
+ nativeFilters: {},
+ dataMask: {
+ [filterId]: {
+ id: filterId,
+ extraFormData: {},
+ filterState: {},
+ ownState: {},
},
- extraControls: {
- stack: 'Stacked',
- },
- allSliceIds: [chartId],
- };
+ },
+ extraControls: {
+ stack: 'Stacked',
+ },
+ allSliceIds: [chartId],
+};
- test('should include filters from the passed filters', () => {
- const result = getFormDataWithExtraFilters(mockArgs);
- expect(result.extra_filters).toHaveLength(2);
- expect(result.extra_filters[0]).toEqual({
- col: 'region',
- op: 'IN',
- val: ['Spain'],
- });
- expect(result.extra_filters[1]).toEqual({
- col: 'color',
- op: 'IN',
- val: ['pink', 'purple'],
- });
+test('should include filters from the passed filters', () => {
+ const result = getFormDataWithExtraFilters(mockArgs);
+ expect(result.extra_filters).toHaveLength(2);
+ expect(result.extra_filters[0]).toEqual({
+ col: 'region',
+ op: 'IN',
+ val: ['Spain'],
});
-
- test('should compose extra control', () => {
- const result: CachedFormDataWithExtraControls =
- getFormDataWithExtraFilters(mockArgs);
- expect(result.stack).toEqual('Stacked');
+ expect(result.extra_filters[1]).toEqual({
+ col: 'color',
+ op: 'IN',
+ val: ['pink', 'purple'],
});
+});
- test('should merge extraFormData from chart customizations', () => {
- const customizationId = 'CHART_CUSTOMIZATION-1';
- const argsWithCustomization: GetFormDataWithExtraFiltersArguments = {
- ...mockArgs,
- dataMask: {
- [customizationId]: {
- id: customizationId,
- extraFormData: {
- time_grain_sqla: 'PT1H',
- },
- filterState: {
- value: ['category1', 'category2'],
- },
- ownState: {},
+test('should compose extra control', () => {
+ const result: CachedFormDataWithExtraControls =
+ getFormDataWithExtraFilters(mockArgs);
+ expect(result.stack).toEqual('Stacked');
+});
+
+test('should merge extraFormData from chart customizations', () => {
+ const customizationId = 'CHART_CUSTOMIZATION-1';
+ const argsWithCustomization: GetFormDataWithExtraFiltersArguments = {
+ ...mockArgs,
+ dataMask: {
+ [customizationId]: {
+ id: customizationId,
+ extraFormData: {
+ time_grain_sqla: 'PT1H',
},
+ filterState: {
+ value: ['category1', 'category2'],
+ },
+ ownState: {},
},
- chartCustomizationItems: [
- {
- id: customizationId,
- type: 'CHART_CUSTOMIZATION' as any,
- name: 'Time Grain Customization',
- filterType: 'chart_customization_time_grain',
- targets: [
- {
- datasetId: 123,
- column: { name: 'time_column' },
- },
- ],
- scope: {
- rootPath: [],
- excluded: [],
+ },
+ chartCustomizationItems: [
+ {
+ id: customizationId,
+ type: 'CHART_CUSTOMIZATION' as any,
+ name: 'Time Grain Customization',
+ filterType: 'chart_customization_time_grain',
+ targets: [
+ {
+ datasetId: 123,
+ column: { name: 'time_column' },
},
- chartsInScope: [chartId],
- defaultDataMask: {},
- controlValues: {},
+ ],
+ scope: {
+ rootPath: [],
+ excluded: [],
},
- ],
- };
+ chartsInScope: [chartId],
+ defaultDataMask: {},
+ controlValues: {},
+ },
+ ],
+ };
- const result = getFormDataWithExtraFilters(argsWithCustomization);
- expect((result as any).time_grain_sqla).toEqual('PT1H');
- });
+ const result = getFormDataWithExtraFilters(argsWithCustomization);
+ expect((result as any).time_grain_sqla).toEqual('PT1H');
+});
- test('should merge both filters and customization extraFormData', () => {
- const customizationId = 'CHART_CUSTOMIZATION-1';
- const argsWithBoth: GetFormDataWithExtraFiltersArguments = {
- ...mockArgs,
- chartConfiguration: {
- [filterId]: {
- id: filterId,
- targets: [
+test('should merge both filters and customization extraFormData', () => {
+ const customizationId = 'CHART_CUSTOMIZATION-1';
+ const argsWithBoth: GetFormDataWithExtraFiltersArguments = {
+ ...mockArgs,
+ chartConfiguration: {
+ [filterId]: {
+ id: filterId,
+ targets: [
+ {
+ datasetId: 123,
+ column: { name: 'country_name' },
+ },
+ ],
+ scope: { rootPath: ['ROOT_ID'], excluded: [] },
+ cascadeParentIds: [],
+ },
+ } as any,
+ dataMask: {
+ [filterId]: {
+ id: filterId,
+ extraFormData: {
+ filters: [
{
- datasetId: 123,
- column: { name: 'country_name' },
+ col: 'country_name',
+ op: 'IN',
+ val: ['United States'],
},
],
- scope: { rootPath: ['ROOT_ID'], excluded: [] },
- cascadeParentIds: [],
},
- } as any,
- dataMask: {
- [filterId]: {
- id: filterId,
- extraFormData: {
- filters: [
- {
- col: 'country_name',
- op: 'IN',
- val: ['United States'],
- },
- ],
- },
- filterState: {},
- ownState: {},
+ filterState: {},
+ ownState: {},
+ },
+ [customizationId]: {
+ id: customizationId,
+ extraFormData: {
+ time_grain_sqla: 'PT1H',
},
- [customizationId]: {
- id: customizationId,
- extraFormData: {
- time_grain_sqla: 'PT1H',
- },
- filterState: {
- value: ['category1'],
- },
- ownState: {},
+ filterState: {
+ value: ['category1'],
},
+ ownState: {},
},
- chartCustomizationItems: [
- {
- id: customizationId,
- type: 'CHART_CUSTOMIZATION' as any,
- name: 'Time Grain Customization',
- filterType: 'chart_customization_time_grain',
- targets: [
- {
- datasetId: 123,
- column: { name: 'time_column' },
- },
- ],
- scope: {
- rootPath: [],
- excluded: [],
+ },
+ chartCustomizationItems: [
+ {
+ id: customizationId,
+ type: 'CHART_CUSTOMIZATION' as any,
+ name: 'Time Grain Customization',
+ filterType: 'chart_customization_time_grain',
+ targets: [
+ {
+ datasetId: 123,
+ column: { name: 'time_column' },
},
- chartsInScope: [chartId],
- defaultDataMask: {},
- controlValues: {},
+ ],
+ scope: {
+ rootPath: [],
+ excluded: [],
},
- ],
- };
+ chartsInScope: [chartId],
+ defaultDataMask: {},
+ controlValues: {},
+ },
+ ],
+ };
+
+ const result = getFormDataWithExtraFilters(argsWithBoth);
+ expect((result as any).time_grain_sqla).toEqual('PT1H');
+ expect(result.extra_form_data).toBeDefined();
+});
+
+const makeGroupByArgs = (
+ selectedValue: string | string[],
+ baseGroupby: string[] = [],
+): GetFormDataWithExtraFiltersArguments => {
+ const customizationId = 'CHART_CUSTOMIZATION-groupby-1';
+ return {
+ ...mockArgs,
+ chart: {
+ ...mockChart,
+ form_data: {
+ ...mockChart.form_data,
+ viz_type: 'table',
+ datasource: '3__table',
+ groupby: baseGroupby,
+ },
+ },
+ dataMask: {
+ [customizationId]: {
+ id: customizationId,
+ extraFormData: {},
+ filterState: { value: selectedValue },
+ ownState: {},
+ },
+ },
+ chartCustomizationItems: [
+ {
+ id: customizationId,
+ type: 'CHART_CUSTOMIZATION' as any,
+ name: 'Dynamic Group By',
+ filterType: 'chart_customization_dynamic_groupby',
+ targets: [{ datasetId: 3, column: { name: 'status' } }],
+ scope: { rootPath: [], excluded: [] },
+ chartsInScope: [chartId],
+ defaultDataMask: {},
+ controlValues: {},
+ },
+ ],
+ };
+};
+
+test('dynamic group by does not inject a filter using the selected column name as a value', () => {
+ const result = getFormDataWithExtraFilters(makeGroupByArgs(['status']));
+ const spuriousFilter = getResultFilters(result).find(
+ filter => filter.col === 'status' && filter.val.includes('status'),
+ );
+ expect(spuriousFilter).toBeUndefined();
+ expectGroupBy(result, ['status']);
+});
- const result = getFormDataWithExtraFilters(argsWithBoth);
- expect((result as any).time_grain_sqla).toEqual('PT1H');
- expect(result.extra_form_data).toBeDefined();
+test('dynamic group by still applies when the selected column is already in the base groupby', () => {
+ const result = getFormDataWithExtraFilters(
+ makeGroupByArgs(['status'], ['status']),
+ );
+ expectGroupBy(result, ['status']);
+});
+
+test('chord chart does not duplicate a selected column that already exists in the base groupby', () => {
+ const result = getFormDataWithExtraFilters({
+ ...makeGroupByArgs(['status'], ['status']),
+ chart: {
+ ...mockChart,
+ form_data: {
+ ...mockChart.form_data,
+ viz_type: 'chord',
+ datasource: '3__table',
+ groupby: ['status'],
+ },
+ },
});
+
+ expectGroupBy(result, ['status']);
+});
+
+test('dynamic group by normalizes a single-select string value into a one-item groupby array', () => {
+ const result = getFormDataWithExtraFilters(makeGroupByArgs('status'));
+ expectGroupBy(result, ['status']);
+});
+
+test('structural conflict: metric column blocks groupby override (nonConflictingColumns guard)', () => {
+ const customizationId = 'CHART_CUSTOMIZATION-groupby-conflict';
+ const argsWithMetricConflict: GetFormDataWithExtraFiltersArguments = {
+ ...mockArgs,
+ chart: {
+ ...mockChart,
+ form_data: {
+ ...mockChart.form_data,
+ viz_type: 'table',
+ datasource: '3__table',
+ groupby: ['original_column'],
+ metrics: ['revenue'],
+ },
+ },
+ dataMask: {
+ [customizationId]: {
+ id: customizationId,
+ extraFormData: {},
+ filterState: { value: ['revenue'] },
+ ownState: {},
+ },
+ },
+ chartCustomizationItems: [
+ {
+ id: customizationId,
+ type: 'CHART_CUSTOMIZATION' as any,
+ name: 'Dynamic Group By',
+ filterType: 'chart_customization_dynamic_groupby',
+ targets: [{ datasetId: 3, column: { name: 'revenue' } }],
+ scope: { rootPath: [], excluded: [] },
+ chartsInScope: [chartId],
+ defaultDataMask: {},
+ controlValues: {},
+ },
+ ],
+ };
+
+ const result = getFormDataWithExtraFilters(argsWithMetricConflict);
+ expectGroupBy(result, ['original_column']);
+});
+
+test('multi-column selection: all selected columns appear in result groupby', () => {
+ const result = getFormDataWithExtraFilters(
+ makeGroupByArgs(['status', 'product_line']),
+ );
+ expectGroupBy(result, expect.arrayContaining(['status', 'product_line']));
+ expectGroupByLength(result, 2);
+});
+
+test('dataset mismatch: display control for a different dataset does not affect the chart', () => {
+ const customizationId = 'CHART_CUSTOMIZATION-wrong-dataset';
+ const argsWrongDataset: GetFormDataWithExtraFiltersArguments = {
+ ...mockArgs,
+ chart: {
+ ...mockChart,
+ form_data: {
+ ...mockChart.form_data,
+ viz_type: 'table',
+ datasource: '3__table',
+ groupby: ['original_column'],
+ },
+ },
+ dataMask: {
+ [customizationId]: {
+ id: customizationId,
+ extraFormData: {},
+ filterState: { value: ['status'] },
+ ownState: {},
+ },
+ },
+ chartCustomizationItems: [
+ {
+ id: customizationId,
+ type: 'CHART_CUSTOMIZATION' as any,
+ name: 'Dynamic Group By',
+ filterType: 'chart_customization_dynamic_groupby',
+ targets: [{ datasetId: 999, column: { name: 'status' } }],
+ scope: { rootPath: [], excluded: [] },
+ chartsInScope: [chartId],
+ defaultDataMask: {},
+ controlValues: {},
+ },
+ ],
+ };
+
+ const result = getFormDataWithExtraFilters(argsWrongDataset);
+ expectGroupBy(result, ['original_column']);
+});
+
+test('Scope boundary: display control with chartsInScope:[] does not affect the chart', () => {
+ const customizationId = 'CHART_CUSTOMIZATION-groupby-out-of-scope';
+ const argsOutOfScope: GetFormDataWithExtraFiltersArguments = {
+ ...mockArgs,
+ chart: {
+ ...mockChart,
+ form_data: {
+ ...mockChart.form_data,
+ viz_type: 'table',
+ datasource: '3__table',
+ groupby: ['original_column'],
+ },
+ },
+ dataMask: {
+ [customizationId]: {
+ id: customizationId,
+ extraFormData: {},
+ filterState: { value: ['replacement_column'] },
+ ownState: {},
+ },
+ },
+ chartCustomizationItems: [
+ {
+ id: customizationId,
+ type: 'CHART_CUSTOMIZATION' as any,
Review Comment:
**Suggestion:** Remove the `any` cast on the out-of-scope customization type and use the typed literal value. [custom_rule]
**Severity Level:** Minor ⚠️
```suggestion
type: 'CHART_CUSTOMIZATION',
```
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The file still contains this `as any` assertion in the out-of-scope customization fixture, so the suggestion addresses a real rule violation. The improved code is valid TypeScript and preserves the same runtime value.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts
**Line:** 443:443
**Comment:**
*Custom Rule: Remove the `any` cast on the out-of-scope customization type and use the typed literal value.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39356&comment_hash=3dbcca7edbefc7f5d12a8eea9b995b08ecbe0cb74a53117dff4cb76b4447893e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39356&comment_hash=3dbcca7edbefc7f5d12a8eea9b995b08ecbe0cb74a53117dff4cb76b4447893e&reaction=dislike'>👎</a>
--
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
Re: [PR] fix(dashboard): apply dynamic groupby display controls to scoped charts [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #39356:
URL: https://github.com/apache/superset/pull/39356#issuecomment-4247635388
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/39356?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `77.77778%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 64.53%. Comparing base ([`3e25f02`](https://app.codecov.io/gh/apache/superset/commit/3e25f02da981bd6d8e14d024bf77b80c7df071f6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`aaffada`](https://app.codecov.io/gh/apache/superset/commit/aaffadad09154a6175d9ae90d181acb6b6413906?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/39356?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...shboard/util/charts/getFormDataWithExtraFilters.ts](https://app.codecov.io/gh/apache/superset/pull/39356?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fdashboard%2Futil%2Fcharts%2FgetFormDataWithExtraFilters.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2NoYXJ0cy9nZXRGb3JtRGF0YVdpdGhFeHRyYUZpbHRlcnMudHM=) | 60.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39356?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
:x: Your project check has failed because the head coverage (99.81%) is below the target coverage (100.00%). You can increase the head coverage or adjust the [target](https://docs.codecov.com/docs/commit-status#target) coverage.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #39356 +/- ##
==========================================
+ Coverage 64.45% 64.53% +0.08%
==========================================
Files 2555 2555
Lines 132721 132720 -1
Branches 30802 30805 +3
==========================================
+ Hits 85539 85654 +115
+ Misses 45696 45580 -116
Partials 1486 1486
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/39356/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/39356/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.32% <77.77%> (+0.15%)` | :arrow_up: |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/39356?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] fix(dashboard): apply dynamic groupby display controls to scoped charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39356:
URL: https://github.com/apache/superset/pull/39356#issuecomment-4248133866
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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
Re: [PR] fix(dashboard): apply dynamic groupby display controls to scoped charts [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39356:
URL: https://github.com/apache/superset/pull/39356#discussion_r3082844833
##########
superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts:
##########
@@ -251,7 +247,9 @@ function applyChartSpecificGroupBy(
groupByFormData.target = limitedColumns[1];
}
} else if (['chord'].includes(chartType)) {
- groupByFormData.groupby = [...existingGroupBy, ...groupByColumns];
+ groupByFormData.groupby = [
+ ...new Set([...existingGroupBy, ...groupByColumns]),
+ ];
Review Comment:
**Suggestion:** The chord branch merges `existingGroupBy` with the newly selected dynamic group-by columns, which keeps stale base dimensions and prevents true override behavior when users pick a different column. This causes chord charts to keep previous grouping unexpectedly. Use only the selected dynamic columns (deduplicated) for the override. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dynamic group by cannot fully override chord source dimension.
- ⚠️ Dashboard chord charts may show stale grouping after selection.
- ⚠️ Users get confusing results when changing chord groupby display.
```
</details>
```suggestion
groupByFormData.groupby = [...new Set(groupByColumns)];
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard containing a chord chart (viz_type 'chord'), whose base form_data has
a single groupby column (the "Source" field) as configured in
`plugins/legacy-plugin-chart-chord/src/controlPanel.ts:28-38` where `groupby` is
single-select and labeled Source.
2. Add a Dynamic Group By display control (`filterType:
'chart_customization_dynamic_groupby'`) scoped to that chord chart's dataset and chart, so
that when the user selects a column, the selection is stored in
`dataMask[customizationId].filterState.value` and passed into
`getFormDataWithExtraFilters` via the Chart component at
`src/dashboard/components/gridComponents/Chart/Chart.tsx:439-40`.
3. In the dashboard UI, choose a dynamic group-by column that is different from the
chart's base groupby (e.g., base groupby = 'status', dynamic selection = 'product_line');
this causes `getFormDataWithExtraFilters` to build `groupByState` from `dataMask` at
`getFormDataWithExtraFilters.ts:113-141`, then `processGroupByCustomizations` at
`getFormDataWithExtraFilters.ts:274-381` computes `groupByColumns = ['product_line']` and
`existingGroupBy = ['status']`.
4. When `processGroupByCustomizations` calls `applyChartSpecificGroupBy` at
`getFormDataWithExtraFilters.ts:382-387` with `chartType === 'chord'`, execution enters
the chord branch at `getFormDataWithExtraFilters.ts:249-252`, where
`groupByFormData.groupby = [...new Set([...existingGroupBy, ...groupByColumns])]` produces
`['status', 'product_line']`; because the chord control panel (`controlPanel.ts:28-37`)
defines a single Source field and the original base groupby remains in the array, the
chord chart's "source" grouping does not fully switch to the newly selected dynamic
column, so the previous grouping dimension persists alongside the new one instead of being
cleanly overridden.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts
**Line:** 250:252
**Comment:**
*Logic Error: The chord branch merges `existingGroupBy` with the newly selected dynamic group-by columns, which keeps stale base dimensions and prevents true override behavior when users pick a different column. This causes chord charts to keep previous grouping unexpectedly. Use only the selected dynamic columns (deduplicated) for the override.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39356&comment_hash=ac2a420e8dce8f1cfddbb8514f61fcf84adc19708daa4796116edff4c0c98f09&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39356&comment_hash=ac2a420e8dce8f1cfddbb8514f61fcf84adc19708daa4796116edff4c0c98f09&reaction=dislike'>👎</a>
--
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
Re: [PR] fix(dashboard): apply dynamic groupby display controls to scoped charts [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #39356:
URL: https://github.com/apache/superset/pull/39356#discussion_r3082842675
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsx:
##########
@@ -438,3 +449,146 @@ test('calculates tabsInScope for filters with tab-scoped charts', async () => {
]),
);
});
+
+// Chart customization scope tests.
+
+test('calculates chartsInScope correctly for new-format chart customizations', async () => {
+ const customizationId = 'CHART_CUSTOMIZATION-1';
+ const originalFn = chartCustomizationActions.setInScopeStatusOfCustomizations;
+ const spy = jest.spyOn(
+ chartCustomizationActions,
+ 'setInScopeStatusOfCustomizations',
+ );
+ spy.mockImplementation(args => originalFn(args));
+
+ try {
+ const state = {
+ dashboardInfo: {
+ ...mockState.dashboardInfo,
+ metadata: {
+ ...mockState.dashboardInfo.metadata,
+ native_filter_configuration: [],
+ chart_customization_config: [
+ {
+ id: customizationId,
+ type: 'CHART_CUSTOMIZATION',
+ name: 'Dynamic Group By',
+ filterType: 'chart_customization_dynamic_groupby',
+ targets: [{ datasetId: 1, column: { name: 'status' } }],
+ scope: { rootPath: ['ROOT_ID'], excluded: [] },
+ chartsInScope: [],
+ defaultDataMask: {},
+ controlValues: {},
+ },
+ ],
+ },
+ },
+ nativeFilters: {
+ filters: {
+ [customizationId]: {
+ id: customizationId,
+ type: 'CHART_CUSTOMIZATION',
+ chartsInScope: [],
+ },
+ },
+ },
+ };
+ setup(state);
+
+ await waitFor(() => {
+ expect(spy).toHaveBeenCalledWith(
+ expect.arrayContaining([
+ expect.objectContaining({
+ customizationId,
+ chartsInScope: [sliceId],
+ }),
+ ]),
+ );
+ });
+ } finally {
+ spy.mockRestore();
+ }
+});
+
+test('migrates legacy-format customizations before scope calculation for scope-less items', async () => {
+ const legacyCustomizationId = 'CHART_CUSTOMIZATION-legacy-1';
+ const originalFn = chartCustomizationActions.setInScopeStatusOfCustomizations;
+ const spy = jest.spyOn(
+ chartCustomizationActions,
+ 'setInScopeStatusOfCustomizations',
+ );
+ spy.mockImplementation(args => originalFn(args));
+
+ try {
+ const state = {
+ dashboardInfo: {
+ ...mockState.dashboardInfo,
+ metadata: {
+ ...mockState.dashboardInfo.metadata,
+ native_filter_configuration: [],
+ chart_customization_config: [
+ {
+ id: legacyCustomizationId,
+ customization: {
+ dataset: 1,
+ column: 'status',
+ filterType: 'chart_customization_dynamic_groupby',
+ name: 'Legacy Group By',
+ },
+ },
+ ],
+ },
+ },
+ nativeFilters: {
+ filters: {
+ [legacyCustomizationId]: {
+ id: legacyCustomizationId,
+ chartsInScope: [],
+ },
+ },
+ },
+ };
+ setup(state);
+
+ await waitFor(() => {
+ expect(spy).toHaveBeenCalledWith(
+ expect.arrayContaining([
+ expect.objectContaining({
+ customizationId: legacyCustomizationId,
+ chartsInScope: [sliceId],
+ }),
+ ]),
+ );
+ });
+ } finally {
+ spy.mockRestore();
+ }
+});
Review Comment:
The legacy migration scope test currently uses a dashboard with a single chart, so it won’t catch cases where a legacy customization includes a `chartId` and should remain scoped to only that chart. Given `DashboardContainer` now migrates legacy items and then uses `calculateScopes` (which ignores `chartsInScope`), adding a regression test with 2+ charts and a legacy item containing `chartId` would help prevent accidentally expanding its scope to all 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
Re: [PR] fix(dashboard): apply dynamic groupby display controls to scoped charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39356:
URL: https://github.com/apache/superset/pull/39356#issuecomment-4247540849
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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
Re: [PR] fix(dashboard): apply dynamic groupby display controls to scoped charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39356:
URL: https://github.com/apache/superset/pull/39356#issuecomment-4248500224
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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
Re: [PR] fix(dashboard): apply dynamic groupby display controls to scoped charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39356:
URL: https://github.com/apache/superset/pull/39356#issuecomment-4254587301
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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
Re: [PR] fix(dashboard): apply dynamic groupby display controls to scoped charts [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca merged PR #39356:
URL: https://github.com/apache/superset/pull/39356
--
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