codeant-ai-for-open-source[bot] commented on code in PR #37790: URL: https://github.com/apache/superset/pull/37790#discussion_r2782886057
########## superset-frontend/playwright/tests/experimental/localization/filter-name-localization.spec.ts: ########## @@ -0,0 +1,209 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { test, expect } from '@playwright/test'; +import { + apiPutDashboard, + apiDeleteDashboard, + createTestDashboard, +} from '../../../helpers/api/dashboard'; +import { + createTestVirtualDataset, + apiDeleteDataset, +} from '../../../helpers/api/dataset'; +import { + createTestDatabase, + apiDeleteDatabase, +} from '../../../helpers/api/database'; +import { DashboardPage } from '../../../pages/DashboardPage'; +import { TIMEOUT } from '../../../utils/constants'; + +/** + * Native Filter Name Localization E2E Tests + * + * Verifies that native filter names on a dashboard are displayed in the + * user's session locale when translations exist in the filter configuration. + * + * Prerequisites: + * - Superset running with ENABLE_CONTENT_LOCALIZATION = True + * - Multiple LANGUAGES configured in superset_config.py + * - Admin user authenticated (via global-setup) + */ + +const ORIGINAL_FILTER_NAME = 'Year Filter'; +const GERMAN_FILTER_NAME = 'Jahresfilter'; + +// File-scope state (reset in beforeEach) +let testResources: { + dashboardIds: number[]; + datasetIds: number[]; + databaseIds: number[]; +}; + +test.beforeEach(async () => { + testResources = { dashboardIds: [], datasetIds: [], databaseIds: [] }; +}); + +test.afterEach(async ({ page }) => { + // Reset locale to English + await page.request.get('lang/en', { maxRedirects: 0 }).catch(() => {}); + + // Sequential per group to respect FK constraints + await Promise.all( + testResources.dashboardIds.map(id => + apiDeleteDashboard(page, id, { failOnStatusCode: false }).catch(e => + console.warn(`[Cleanup] dashboard ${id}:`, String(e)), + ), + ), + ); + await Promise.all( + testResources.datasetIds.map(id => + apiDeleteDataset(page, id, { failOnStatusCode: false }).catch(e => + console.warn(`[Cleanup] dataset ${id}:`, String(e)), + ), + ), + ); + await Promise.all( + testResources.databaseIds.map(id => + apiDeleteDatabase(page, id, { failOnStatusCode: false }).catch(e => + console.warn(`[Cleanup] database ${id}:`, String(e)), + ), + ), + ); +}); + +/** + * Build json_metadata with a native filter that has translations. + */ +function buildFilterMetadata( + datasetId: number, + filterName: string, + translations: Record<string, string>, +): string { + return JSON.stringify({ + native_filter_configuration: [ + { + id: 'NATIVE_FILTER-test1', + name: filterName, + filterType: 'filter_select', + targets: [{ datasetId, column: { name: 'id' } }], + defaultDataMask: { filterState: {} }, + controlValues: { + enableEmptyFilter: false, + multiSelect: true, + inverseSelection: false, + }, + cascadeParentIds: [], + scope: { rootPath: ['ROOT_ID'], excluded: [] }, + translations: { name: translations }, + }, + ], + show_native_filters: true, + }); +} + +test('should display localized filter name on dashboard when session locale has translation', async ({ + page, +}) => { + const suffix = Date.now(); + + // 1. Create test database and virtual dataset (filter needs a valid datasource) + const dbId = await createTestDatabase(page, `test_filter_loc_db_${suffix}`); + expect(dbId).not.toBeNull(); + testResources.databaseIds.push(dbId!); + + const datasetId = await createTestVirtualDataset( + page, + `test_filter_loc_ds_${suffix}`, + dbId!, + ); + expect(datasetId).not.toBeNull(); + testResources.datasetIds.push(datasetId!); + + // 2. Create dashboard with native filter + German translation + const dashboardTitle = `test_dash_filter_loc_${suffix}`; + const dashboardId = await createTestDashboard(page, dashboardTitle); + expect(dashboardId).not.toBeNull(); + testResources.dashboardIds.push(dashboardId!); + + const putRes = await apiPutDashboard(page, dashboardId!, { + json_metadata: buildFilterMetadata(datasetId!, ORIGINAL_FILTER_NAME, { + de: GERMAN_FILTER_NAME, + }), + }); + expect(putRes.ok()).toBe(true); + + // 3. Switch session locale to German + await page.request.get('lang/de', { maxRedirects: 0 }); + + // 4. Navigate to dashboard + const dashboardPage = new DashboardPage(page); + await dashboardPage.gotoById(dashboardId!); + await dashboardPage.waitForLoad({ timeout: TIMEOUT.PAGE_LOAD }); + + // 5. Verify filter name is localized + const filterName = page.locator('[data-test="filter-control-name"]').first(); + await expect(filterName).toContainText(GERMAN_FILTER_NAME, { + timeout: TIMEOUT.API_RESPONSE, + }); +}); + +test('should display original filter name when session locale has no translation', async ({ + page, +}) => { + const suffix = Date.now(); + + // 1. Create test database and virtual dataset + const dbId = await createTestDatabase(page, `test_filter_orig_db_${suffix}`); + expect(dbId).not.toBeNull(); + testResources.databaseIds.push(dbId!); + + const datasetId = await createTestVirtualDataset( + page, + `test_filter_orig_ds_${suffix}`, + dbId!, + ); + expect(datasetId).not.toBeNull(); + testResources.datasetIds.push(datasetId!); + + // 2. Create dashboard with native filter + only German translation + const dashboardTitle = `test_dash_filter_orig_${suffix}`; + const dashboardId = await createTestDashboard(page, dashboardTitle); + expect(dashboardId).not.toBeNull(); + testResources.dashboardIds.push(dashboardId!); + + await apiPutDashboard(page, dashboardId!, { + json_metadata: buildFilterMetadata(datasetId!, ORIGINAL_FILTER_NAME, { + de: GERMAN_FILTER_NAME, + }), + }); Review Comment: **Suggestion:** In the "no translation" filter name test, the PUT that updates the dashboard's `json_metadata` is not validated, so if the request fails (e.g., bad filter config) the test will only fail later when asserting the UI, making it harder to detect and diagnose the real error. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Filter-name fallback E2E test can hide dashboard API failures. - ⚠️ Debugging localization regressions becomes harder due to opaque errors. ``` </details> ```suggestion const putRes = await apiPutDashboard(page, dashboardId!, { json_metadata: buildFilterMetadata(datasetId!, ORIGINAL_FILTER_NAME, { de: GERMAN_FILTER_NAME, }), }); expect(putRes.ok()).toBe(true); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Execute the Playwright test `should display original filter name when session locale has no translation` in `superset-frontend/playwright/tests/experimental/localization/filter-name-localization.spec.ts` (lines 167–209). 2. Within this test, a dashboard is created (lines 185–189), then `apiPutDashboard` is called at lines 191–195 to update `json_metadata` with a native filter and German translation, but the returned `APIResponse` is ignored. 3. If the underlying dashboard update endpoint used by `apiPutDashboard` (implemented in `superset-frontend/playwright/helpers/api/dashboard.ts`, see surrounding helpers like `apiDeleteDashboard` at lines 104–110) starts returning a non-OK status for this payload (for example due to a regression in json_metadata validation), the test will still proceed to switch locale and open the dashboard (lines 197–203). 4. The final assertion at lines 205–208 uses `page.locator('[data-test="filter-control-name"]').first()` and `expect(...).toContainText(ORIGINAL_FILTER_NAME, ...)`; this will fail with a generic locator timeout or text mismatch instead of clearly surfacing the failed PUT, making the root cause (API failure) harder to diagnose. Adding `expect(putRes.ok()).toBe(true)` immediately after the PUT would fail fast with an explicit API error. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/tests/experimental/localization/filter-name-localization.spec.ts **Line:** 191:195 **Comment:** *Possible Bug: In the "no translation" filter name test, the PUT that updates the dashboard's `json_metadata` is not validated, so if the request fails (e.g., bad filter config) the test will only fail later when asserting the UI, making it harder to detect and diagnose the real error. 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%2F37790&comment_hash=43c45ca542254c078c49863040d70507c374a1db2fec342ede42adb44614691d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=43c45ca542254c078c49863040d70507c374a1db2fec342ede42adb44614691d&reaction=dislike'>👎</a> ########## superset-frontend/playwright/helpers/api/chart.ts: ########## @@ -0,0 +1,153 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Page, APIResponse } from '@playwright/test'; +import { + apiGet, + apiPost, + apiPut, + apiDelete, + ApiRequestOptions, +} from './requests'; + +export const ENDPOINTS = { + CHART: 'api/v1/chart/', +} as const; + +/** Payload for POST /api/v1/chart/ */ +export interface ChartCreatePayload { + slice_name: string; + datasource_id: number; + datasource_type: string; + viz_type?: string; + owners?: number[]; + dashboards?: number[]; + params?: string; +} + +/** Payload for PUT /api/v1/chart/{id} */ +export interface ChartUpdatePayload { + slice_name?: string; + owners?: number[]; + dashboards?: number[]; + translations?: Record<string, Record<string, string>>; +} + +/** Chart data shape from API responses */ +export interface ChartResult { + id: number; + uuid: string; + slice_name: string; + viz_type?: string; + translations?: Record<string, Record<string, string>>; + available_locales?: string[]; +} + +/** + * POST /api/v1/chart/ + * Creates a new chart. + */ +export async function apiPostChart( + page: Page, + payload: ChartCreatePayload, +): Promise<APIResponse> { + return apiPost(page, ENDPOINTS.CHART, payload); +} + +/** + * GET /api/v1/chart/{id} + * Fetches chart details. + */ +export async function apiGetChart( + page: Page, + chartId: number, + options?: ApiRequestOptions, +): Promise<APIResponse> { + return apiGet(page, `${ENDPOINTS.CHART}${chartId}`, options); +} + +/** + * PUT /api/v1/chart/{id} + * Updates chart fields including translations. + */ +export async function apiPutChart( + page: Page, + chartId: number, + data: ChartUpdatePayload, + options?: ApiRequestOptions, +): Promise<APIResponse> { + return apiPut(page, `${ENDPOINTS.CHART}${chartId}`, data, options); +} + +/** + * DELETE /api/v1/chart/{id} + * Removes a chart. + */ +export async function apiDeleteChart( + page: Page, + chartId: number, + options?: ApiRequestOptions, +): Promise<APIResponse> { + return apiDelete(page, `${ENDPOINTS.CHART}${chartId}`, options); +} + +/** + * Creates a minimal chart for testing. + * Requires a datasource (use createTestVirtualDataset first). + * Fetches UUID via GET after creation (not in POST response). + * @returns Chart ID and UUID, or null on failure. + */ +export async function createTestChart( + page: Page, + sliceName: string, + datasourceId: number, +): Promise<{ id: number; uuid: string } | null> { + const response = await apiPostChart(page, { + slice_name: sliceName, + datasource_id: datasourceId, + datasource_type: 'table', + viz_type: 'table', + owners: [], + }); Review Comment: **Suggestion:** The helper `createTestChart` is documented to return `null` on failure and checks `response.ok()`, but it calls `apiPostChart`/`apiPost` with the default `failOnStatusCode: true`, so any non-2xx status will cause Playwright to throw before reaching the `ok()` check, resulting in an unhandled exception instead of a graceful `null` return. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Tests using createTestChart crash on non-2xx responses. - ⚠️ Helper contract "null on failure" not actually respected. ``` </details> ```suggestion const response = await apiPost(page, ENDPOINTS.CHART, { slice_name: sliceName, datasource_id: datasourceId, datasource_type: 'table', viz_type: 'table', owners: [], }, { failOnStatusCode: false }); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In a Playwright test, call `createTestChart(page, sliceName, datasourceId)` from `superset-frontend/playwright/helpers/api/chart.ts:116` with a `datasourceId` or payload that causes the backend `/api/v1/chart/` endpoint to return a 4xx/5xx response (e.g., nonexistent datasource). 2. `createTestChart` calls `apiPostChart` at `superset-frontend/playwright/helpers/api/chart.ts:66`, which in turn calls `apiPost(page, ENDPOINTS.CHART, payload)` from `superset-frontend/playwright/helpers/api/requests.ts:121-135` without passing any options. 3. Inside `apiPost`, the request is sent via `page.request.post(url, { data, headers, params, failOnStatusCode: options?.failOnStatusCode ?? true })` (see `requests.ts:127-135`), so `failOnStatusCode` is `true` for this call. 4. When the backend responds with a non-2xx status, Playwright throws from `page.request.post` due to `failOnStatusCode: true`, so execution never reaches the `if (!response.ok()) { ... return null; }` block in `createTestChart`, and the caller observes an unhandled exception instead of the documented `null` return on failure. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/helpers/api/chart.ts **Line:** 121:127 **Comment:** *Possible Bug: The helper `createTestChart` is documented to return `null` on failure and checks `response.ok()`, but it calls `apiPostChart`/`apiPost` with the default `failOnStatusCode: true`, so any non-2xx status will cause Playwright to throw before reaching the `ok()` check, resulting in an unhandled exception instead of a graceful `null` return. 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%2F37790&comment_hash=77082fbd22a8c3460745c7393e517b82332a628146ead483df76a099ddfc2923&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=77082fbd22a8c3460745c7393e517b82332a628146ead483df76a099ddfc2923&reaction=dislike'>👎</a> ########## superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx: ########## @@ -569,6 +585,109 @@ const FiltersConfigForm = ( [filterId, form, formChanged], ); + const localizationEnabled = isFeatureEnabled( + FeatureFlag.EnableContentLocalization, + ); + + const currentTranslations: Translations = + formFilter?.translations ?? filterToEdit?.translations ?? {}; + + useEffect(() => { + if (filterToEdit?.translations) { + setNativeFilterFieldValues(form, filterId, { + translations: filterToEdit.translations, + }); + } + }, [filterId, filterToEdit?.translations, form]); + + useEffect(() => { + if (localizationEnabled) { + SupersetClient.get({ + endpoint: '/api/v1/localization/available_locales', + }).then( + response => { + const { locales, default_locale } = response.json.result; + setAllLocales(locales); + setDefaultLocale(default_locale); + }, + err => logging.error('Failed to fetch available locales', err), + ); + } + }, [localizationEnabled]); + + // Refs for values read during initialization but not triggering re-runs. + // Without refs, editing a translation would reset the active locale + // because currentTranslations changes on every keystroke. + const currentTranslationsRef = useRef(currentTranslations); + currentTranslationsRef.current = currentTranslations; + const userLocaleRef = useRef(userLocale); + userLocaleRef.current = userLocale; + + // Initialize nameActiveLocale when switching between filters. + useEffect(() => { + if ( + localizationEnabled && + currentTranslationsRef.current.name?.[userLocaleRef.current] + ) { + setNameActiveLocale(userLocaleRef.current); + } else { + setNameActiveLocale(DEFAULT_LOCALE_KEY); + } + }, [localizationEnabled, filterId]); + + const handleNameTranslationChange = useCallback( + (value: string) => { + const updated: Translations = { + ...currentTranslationsRef.current, + name: { ...currentTranslationsRef.current.name, [nameActiveLocale]: value }, Review Comment: **Suggestion:** When creating the updated translations object, spreading `currentTranslationsRef.current.name` directly will throw a runtime TypeError if `name` is undefined (i.e., when there are no existing name translations), because object spread cannot operate on `undefined` or `null`; you should default it to an empty object before spreading. [type error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Editing filter name translations crashes native filters configuration. - ⚠️ Localization feature unstable for dashboard native filter editors. ``` </details> ```suggestion name: { ...(currentTranslationsRef.current.name ?? {}), [nameActiveLocale]: value, }, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Enable the `ENABLE_CONTENT_LOCALIZATION` feature flag so `isFeatureEnabled(FeatureFlag.EnableContentLocalization)` returns true in `FiltersConfigForm.tsx` (around lines 210–260), making `localizationEnabled` true. 2. In the Superset UI, open any dashboard and launch the Native Filters configuration modal, which renders the `FiltersConfigForm` component (`superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx`). 3. Create a new native filter or edit an existing one that has no `translations` saved yet; on first render, `formFilter?.translations ?? filterToEdit?.translations ?? {}` (lines ~585–595) yields `{}`, so `currentTranslations.name` is `undefined`. 4. In the filter configuration tab, use the inline `LocaleSwitcher` rendered in the name field suffix (JSX block around lines ~720–760) to select a non-default locale, then type in the translation input (rendered when `isEditingFilterTranslation` is true); this triggers `handleNameTranslationChange` (lines 638–650), which executes `name: { ...currentTranslationsRef.current.name, [nameActiveLocale]: value }` with `currentTranslationsRef.current.name === undefined`, causing a runtime `TypeError: Cannot convert undefined or null to object` and breaking the modal. ``` </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/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx **Line:** 642:642 **Comment:** *Type Error: When creating the updated translations object, spreading `currentTranslationsRef.current.name` directly will throw a runtime TypeError if `name` is undefined (i.e., when there are no existing name translations), because object spread cannot operate on `undefined` or `null`; you should default it to an empty object before spreading. 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%2F37790&comment_hash=39541a69e56192a41d3b49cb8804b12bc817b1a56e823183aca7679d0bf93259&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=39541a69e56192a41d3b49cb8804b12bc817b1a56e823183aca7679d0bf93259&reaction=dislike'>👎</a> ########## superset-frontend/playwright/tests/experimental/localization/chart-title-localization.spec.ts: ########## @@ -0,0 +1,271 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { test, expect } from '@playwright/test'; +import { + apiPutDashboard, + apiDeleteDashboard, + createTestDashboard, +} from '../../../helpers/api/dashboard'; +import { + apiPutChart, + apiDeleteChart, + createTestChart, +} from '../../../helpers/api/chart'; +import { + createTestVirtualDataset, + apiDeleteDataset, +} from '../../../helpers/api/dataset'; +import { + createTestDatabase, + apiDeleteDatabase, +} from '../../../helpers/api/database'; +import { DashboardPage } from '../../../pages/DashboardPage'; +import { TIMEOUT } from '../../../utils/constants'; + +/** + * Chart Title Localization E2E Tests + * + * Verifies that chart titles on a dashboard are displayed in the + * user's session locale when translations exist on the chart. + * + * Prerequisites: + * - Superset running with ENABLE_CONTENT_LOCALIZATION = True + * - Multiple LANGUAGES configured in superset_config.py + * - Admin user authenticated (via global-setup) + */ + +const GERMAN_CHART_TITLE = 'Deutscher Diagrammtitel'; + +// File-scope state (reset in beforeEach) +let testResources: { + dashboardIds: number[]; + chartIds: number[]; + datasetIds: number[]; + databaseIds: number[]; +}; + +test.beforeEach(async () => { + testResources = { + dashboardIds: [], + chartIds: [], + datasetIds: [], + databaseIds: [], + }; +}); + +test.afterEach(async ({ page }) => { + // Reset locale to English + await page.request.get('lang/en', { maxRedirects: 0 }).catch(() => {}); + + // Cleanup in reverse creation order: charts → dashboards → datasets → databases + // Sequential per group to respect FK constraints + await Promise.all( + testResources.chartIds.map(id => + apiDeleteChart(page, id, { failOnStatusCode: false }).catch(e => + console.warn(`[Cleanup] chart ${id}:`, String(e)), + ), + ), + ); + await Promise.all( + testResources.dashboardIds.map(id => + apiDeleteDashboard(page, id, { failOnStatusCode: false }).catch(e => + console.warn(`[Cleanup] dashboard ${id}:`, String(e)), + ), + ), + ); + await Promise.all( + testResources.datasetIds.map(id => + apiDeleteDataset(page, id, { failOnStatusCode: false }).catch(e => + console.warn(`[Cleanup] dataset ${id}:`, String(e)), + ), + ), + ); + await Promise.all( + testResources.databaseIds.map(id => + apiDeleteDatabase(page, id, { failOnStatusCode: false }).catch(e => + console.warn(`[Cleanup] database ${id}:`, String(e)), + ), + ), + ); +}); + +/** + * Build minimal position_json with a single chart component. + */ +function buildPositionJson( + chartId: number, + chartUuid: string, + sliceName: string, + dashboardTitle: string, +): string { + return JSON.stringify({ + DASHBOARD_VERSION_KEY: 'v2', + ROOT_ID: { type: 'ROOT', id: 'ROOT_ID', children: ['GRID_ID'] }, + GRID_ID: { + type: 'GRID', + id: 'GRID_ID', + children: ['ROW-test1'], + }, + HEADER_ID: { + type: 'HEADER', + id: 'HEADER_ID', + meta: { text: dashboardTitle }, + }, + 'ROW-test1': { + type: 'ROW', + id: 'ROW-test1', + children: ['CHART-test1'], + meta: { background: 'BACKGROUND_TRANSPARENT' }, + }, + 'CHART-test1': { + type: 'CHART', + id: 'CHART-test1', + children: [], + meta: { + chartId, + sliceName, + uuid: chartUuid, + width: 12, + height: 50, + }, + }, + }); +} + +test('should display localized chart title on dashboard when session locale has translation', async ({ + page, +}) => { + const suffix = Date.now(); + + // 1. Create test database and virtual dataset + const dbId = await createTestDatabase(page, `test_chart_loc_db_${suffix}`); + expect(dbId).not.toBeNull(); + testResources.databaseIds.push(dbId!); + + const datasetId = await createTestVirtualDataset( + page, + `test_chart_loc_ds_${suffix}`, + dbId!, + ); + expect(datasetId).not.toBeNull(); + testResources.datasetIds.push(datasetId!); + + // 2. Create chart and dashboard + const originalTitle = `test_chart_loc_${suffix}`; + const chart = await createTestChart(page, originalTitle, datasetId!); + expect(chart).not.toBeNull(); + testResources.chartIds.push(chart!.id); + + const dashboardTitle = `test_dash_chart_loc_${suffix}`; + const dashboardId = await createTestDashboard(page, dashboardTitle); + expect(dashboardId).not.toBeNull(); + testResources.dashboardIds.push(dashboardId!); + + // 3. Associate chart with dashboard and set German translation + const putChartRes = await apiPutChart(page, chart!.id, { + dashboards: [dashboardId!], + translations: { slice_name: { de: GERMAN_CHART_TITLE } }, + }); + expect(putChartRes.ok()).toBe(true); + + // 4. Set dashboard layout with the chart + const putDashRes = await apiPutDashboard(page, dashboardId!, { + position_json: buildPositionJson( + chart!.id, + chart!.uuid, + originalTitle, + dashboardTitle, + ), + }); + expect(putDashRes.ok()).toBe(true); + + // 5. Switch session locale to German + await page.request.get('lang/de', { maxRedirects: 0 }); + + // 6. Navigate to dashboard and verify localized chart title + const dashboardPage = new DashboardPage(page); + await dashboardPage.gotoById(dashboardId!); + await dashboardPage.waitForLoad({ timeout: TIMEOUT.PAGE_LOAD }); + + // Chart header should show German title + const chartHeader = page.locator('[data-test="slice-header"]').first(); + await expect(chartHeader).toContainText(GERMAN_CHART_TITLE, { + timeout: TIMEOUT.API_RESPONSE, + }); +}); + +test('should display original chart title when session locale has no translation', async ({ + page, +}) => { + const suffix = Date.now(); + + // 1. Create test database and virtual dataset + const dbId = await createTestDatabase(page, `test_chart_orig_db_${suffix}`); + expect(dbId).not.toBeNull(); + testResources.databaseIds.push(dbId!); + + const datasetId = await createTestVirtualDataset( + page, + `test_chart_orig_ds_${suffix}`, + dbId!, + ); + expect(datasetId).not.toBeNull(); + testResources.datasetIds.push(datasetId!); + + // 2. Create chart and dashboard + const originalTitle = `test_chart_orig_${suffix}`; + const chart = await createTestChart(page, originalTitle, datasetId!); + expect(chart).not.toBeNull(); + testResources.chartIds.push(chart!.id); + + const dashboardTitle = `test_dash_chart_orig_${suffix}`; + const dashboardId = await createTestDashboard(page, dashboardTitle); + expect(dashboardId).not.toBeNull(); + testResources.dashboardIds.push(dashboardId!); + + // 3. Associate chart with dashboard and set German translation (no French) + await apiPutChart(page, chart!.id, { + dashboards: [dashboardId!], + translations: { slice_name: { de: GERMAN_CHART_TITLE } }, + }); + + // Set dashboard layout + await apiPutDashboard(page, dashboardId!, { + position_json: buildPositionJson( + chart!.id, + chart!.uuid, + originalTitle, + dashboardTitle, + ), + }); Review Comment: **Suggestion:** In the "no translation" chart title test, the PUT requests that associate the chart with the dashboard and update the dashboard layout are not checked for success, so if either call fails the test will later fail with a confusing UI assertion error instead of clearly failing at the API step. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Localization E2E test can fail with misleading UI error. - ⚠️ Backend regressions in chart/dashboard APIs may go unnoticed. ``` </details> ```suggestion const putChartRes = await apiPutChart(page, chart!.id, { dashboards: [dashboardId!], translations: { slice_name: { de: GERMAN_CHART_TITLE } }, }); expect(putChartRes.ok()).toBe(true); // Set dashboard layout const putDashRes = await apiPutDashboard(page, dashboardId!, { position_json: buildPositionJson( chart!.id, chart!.uuid, originalTitle, dashboardTitle, ), }); expect(putDashRes.ok()).toBe(true); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the Playwright test file `superset-frontend/playwright/tests/experimental/localization/chart-title-localization.spec.ts`, focusing on the second test `test('should display original chart title when session locale has no translation', ...)` defined near lines 214–271. 2. During this test, observe the API setup section at lines 243–257 where `apiPutChart` and `apiPutDashboard` are awaited without checking their responses: - `await apiPutChart(page, chart!.id, { ... })` at lines 244–247. - `await apiPutDashboard(page, dashboardId!, { ... })` at lines 250–257. 3. If the Superset backend returns a non-2xx status for either call (for example due to a temporary backend error or misconfigured `ENABLE_CONTENT_LOCALIZATION` feature flag), these failures are silently ignored because the `Response` objects are not inspected or asserted. 4. The test then proceeds to navigate to the dashboard and assert the chart title in the UI at lines 259–270; because the chart–dashboard association or layout might not have been persisted, the final `expect(chartHeader).toContainText(originalTitle, ...)` assertion fails with a generic UI mismatch, obscuring the original API failure that actually caused the problem. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/tests/experimental/localization/chart-title-localization.spec.ts **Line:** 244:257 **Comment:** *Possible Bug: In the "no translation" chart title test, the PUT requests that associate the chart with the dashboard and update the dashboard layout are not checked for success, so if either call fails the test will later fail with a confusing UI assertion error instead of clearly failing at the API step. 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%2F37790&comment_hash=590ff6d660901df5e7fa56497d2093f8621dec4963795d05c326da5091c43758&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=590ff6d660901df5e7fa56497d2093f8621dec4963795d05c326da5091c43758&reaction=dislike'>👎</a> ########## superset-frontend/src/explore/components/PropertiesModal/index.tsx: ########## @@ -258,6 +292,18 @@ function PropertiesModal({ // get the owners of this slice useEffect(() => { fetchChartProperties(); + if (isFeatureEnabled(FeatureFlag.EnableContentLocalization)) { + SupersetClient.get({ + endpoint: '/api/v1/localization/available_locales', + }).then( + response => { + const { locales, default_locale } = response.json.result; + setAllLocales(locales); + setDefaultLocale(default_locale); + }, + err => logging.error('Failed to fetch available locales', err), Review Comment: **Suggestion:** The error handler for fetching available locales now only logs the failure and no longer calls the existing user-facing `showError` helper, so when this request fails the UI silently proceeds with empty locale data and gives no feedback to the user, which is inconsistent with other error paths and can leave localization controls misconfigured without explanation. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Locale list load failures are silent in chart properties modal. - ⚠️ Translation controls may appear empty without user-facing explanation. - ⚠️ Inconsistent error handling vs `fetchChartProperties` failures. ``` </details> ```suggestion err => { logging.error('Failed to fetch available locales', err); getClientErrorObject(err).then(showError); }, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Enable the content localization feature flag so that `FeatureFlag.EnableContentLocalization` is true on the frontend (this controls the localization branch in `PropertiesModal` at `superset-frontend/src/explore/components/PropertiesModal/index.tsx:289-307`). 2. In the running Superset instance, open the Explore view for any existing chart and open the "Chart properties" modal, which renders the `PropertiesModal` component defined in `superset-frontend/src/explore/components/PropertiesModal/index.tsx`. 3. While the modal is open, the `useEffect` hook at `index.tsx:289-307` runs, calling `SupersetClient.get({ endpoint: '/api/v1/localization/available_locales' })` and attaching the `.then(…, err => logging.error('Failed to fetch available locales', err))` handler at `index.tsx:296-305`. 4. Cause the `/api/v1/localization/available_locales` request to fail (for example by stopping the backend, misconfiguring the endpoint, or injecting a 500 in a proxy); observe in the browser devtools console that an error is logged, but no toast is shown (unlike other errors that go through `showError` at `index.tsx:153-163`), and the locale-related state (`allLocales`, `defaultLocale`) remains at their initial empty values without any user-facing explanation. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/explore/components/PropertiesModal/index.tsx **Line:** 304:304 **Comment:** *Logic Error: The error handler for fetching available locales now only logs the failure and no longer calls the existing user-facing `showError` helper, so when this request fails the UI silently proceeds with empty locale data and gives no feedback to the user, which is inconsistent with other error paths and can leave localization controls misconfigured without explanation. 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%2F37790&comment_hash=45d83989696a377b7381546725a109e3300326624a7ef1fc3a22fc652c265d1a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=45d83989696a377b7381546725a109e3300326624a7ef1fc3a22fc652c265d1a&reaction=dislike'>👎</a> ########## superset-frontend/src/dashboard/components/PropertiesModal/index.tsx: ########## @@ -448,6 +481,19 @@ const PropertiesModal = ({ t('An error occurred while fetching available themes'), ); }); + + if (isFeatureEnabled(FeatureFlag.EnableContentLocalization)) { + SupersetClient.get({ + endpoint: '/api/v1/localization/available_locales', + }).then( + response => { + const { locales, default_locale } = response.json.result; + setAllLocales(locales); + setDefaultLocale(default_locale); + }, + err => logging.error('Failed to fetch available locales', err), Review Comment: **Suggestion:** The error handler for the available locales request only logs to the console and does not surface any user-visible feedback, so when this API fails the locale switcher silently breaks with no locales loaded and no explanation to the user; reusing the existing error handler will restore consistent error reporting behavior. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Dashboard properties modal locale list fails silently on errors. - ⚠️ Users see non-working locale switcher without any explanation. - ⚠️ Inconsistent error handling versus other SupersetClient.get calls. ``` </details> ```suggestion handleErrorResponse, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Enable the `EnableContentLocalization` feature flag so the localization-specific code path is active (checked in `PropertiesModal` at `superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:485`). 2. Open the Dashboard "Properties" modal in the UI, which renders the `PropertiesModal` component exported from `superset-frontend/src/dashboard/components/PropertiesModal/index.tsx` and triggers the `useEffect` starting around line 451 when `show` is true. 3. While the modal is open, cause the `/api/v1/localization/available_locales` request to fail (for example, by using browser devtools to go offline or by having the backend return a 500), so that the `SupersetClient.get` call at lines 485–494 rejects. 4. Observe that the rejection handler `err => logging.error('Failed to fetch available locales', err)` at line 494 only logs the error (via `logging.error`) and does not invoke `handleErrorResponse`, meaning no `addDangerToast` message is shown to the user even though the locale list did not load and the locale switcher cannot function correctly. ``` </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/components/PropertiesModal/index.tsx **Line:** 494:494 **Comment:** *Logic Error: The error handler for the available locales request only logs to the console and does not surface any user-visible feedback, so when this API fails the locale switcher silently breaks with no locales loaded and no explanation to the user; reusing the existing error handler will restore consistent error reporting behavior. 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%2F37790&comment_hash=7932963596ca91c38db47014f59c25ea3f620ae4d1f7b59b389495771a0acd0d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=7932963596ca91c38db47014f59c25ea3f620ae4d1f7b59b389495771a0acd0d&reaction=dislike'>👎</a> ########## superset/dashboards/schemas.py: ########## @@ -257,8 +273,121 @@ def post_dump(self, serialized: dict[str, Any], **kwargs: Any) -> dict[str, Any] del serialized["owners"] del serialized["changed_by_name"] del serialized["changed_by"] + + # Apply content localization when feature is enabled + if is_feature_enabled("ENABLE_CONTENT_LOCALIZATION"): + self._apply_localization(serialized, obj) + return serialized + def _apply_localization(self, serialized: dict[str, Any], obj: Any) -> None: Review Comment: **Suggestion:** The translations validator treats the mere presence of the `translations` key as requiring structural validation, so when the field is explicitly set to null (which is allowed by the schema), it will pass `None` into `validate_translations`, likely causing a runtime error instead of cleanly accepting or ignoring the null value; this should short-circuit when the value is None. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Dashboard update fails when translations explicitly sent as null. - ⚠️ Breaks API clients clearing translations using null payload. ``` </details> ```suggestion translations = data.get("translations") if translations is None: return if not is_feature_enabled("ENABLE_CONTENT_LOCALIZATION"): raise ValidationError( "Content localization is not enabled. " "Set ENABLE_CONTENT_LOCALIZATION=True to use translations.", field_name="translations", ) validate_translations(translations) ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Enable the `ENABLE_CONTENT_LOCALIZATION` feature flag in `superset/config.py`, then start the Superset web server. 2. Call the dashboard update endpoint `PUT /api/v1/dashboard/<dashboard_id>` implemented in `superset/dashboards/api.py` (DashboardRestApi), sending JSON that includes `"translations": null` along with other valid dashboard fields. 3. The request payload is validated using `DashboardPutSchema` in `superset/dashboards/schemas.py`, which invokes `validate_translations_data` during schema validation. 4. Because the `translations` key is present with value `None`, `data["translations"]` is `None`; with the feature flag enabled, `validate_translations(data["translations"])` receives `None` instead of a dict, causing it to treat the value as invalid (or raise a type error) even though the field is declared with `allow_none=True`, so clients that send `null` cannot successfully update dashboards. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/dashboards/schemas.py **Line:** 274:283 **Comment:** *Logic Error: The translations validator treats the mere presence of the `translations` key as requiring structural validation, so when the field is explicitly set to null (which is allowed by the schema), it will pass `None` into `validate_translations`, likely causing a runtime error instead of cleanly accepting or ignoring the null value; this should short-circuit when the value is None. 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%2F37790&comment_hash=9c280d70c6558e038b7bee7b2cbf815035a4bad2b2975d16c4884182c85d4a65&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=9c280d70c6558e038b7bee7b2cbf815035a4bad2b2975d16c4884182c85d4a65&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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
