codeant-ai-for-open-source[bot] commented on code in PR #40817: URL: https://github.com/apache/superset/pull/40817#discussion_r3383320941
########## superset-frontend/playwright/tests/chart/handlebars-format-date.spec.ts: ########## @@ -0,0 +1,90 @@ +/** + * 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. + */ + +/** + * Regression for #32960: the `formatDate` Handlebars helper renders a real + * formatted date instead of a "... is not a function" helper error. CI green + * closes #32960; CI red means the fix is still needed in the Handlebars plugin + * (superset-frontend/plugins/plugin-chart-handlebars). + */ +import { testWithAssets, expect } from '../../helpers/fixtures'; +import { apiPost } from '../../helpers/api/requests'; +import { getDatasetByName } from '../../helpers/api/dataset'; +import { TIMEOUT } from '../../utils/constants'; + +const DATASET_NAME = 'birth_names'; + +testWithAssets( + 'Handlebars formatDate helper renders a formatted date (#32960)', + async ({ page, testAssets }) => { + testWithAssets.setTimeout(TIMEOUT.SLOW_TEST); + + const dataset = await getDatasetByName(page, DATASET_NAME); + if (!dataset) { + throw new Error(`Dataset ${DATASET_NAME} not found`); + } + const datasetId = dataset.id; + + const params = { + datasource: `${datasetId}__table`, + viz_type: 'handlebars', + query_mode: 'aggregate', + groupby: ['ds'], + metrics: ['count'], + adhoc_filters: [], + row_limit: 5, + handlebarsTemplate: + "<ul class='data-list'>{{#each data}}" + + "<li class='hb-item'>{{formatDate 'DD.MM.YYYY' ds}}</li>" + + '{{/each}}</ul>', + styleTemplate: '', + }; + + const chartResp = await apiPost(page, 'api/v1/chart/', { + slice_name: `handlebars_format_date_${Date.now()}`, + viz_type: 'handlebars', + datasource_id: datasetId, + datasource_type: 'table', + params: JSON.stringify(params), + }); + expect(chartResp.ok()).toBe(true); + const chartId: number = (await chartResp.json()).id; Review Comment: **Suggestion:** This assumes chart creation always returns a top-level `id`, but existing Playwright helpers in this codebase already handle both `{ id }` and `{ result: { id } }` response shapes. If the API returns the wrapped shape, `chartId` becomes `undefined`, causing navigation to `slice_id=undefined` and cleanup tracking to break. Parse both response shapes (and fail explicitly if neither exists) before using the chart ID. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Handlebars formatDate E2E test may navigate to invalid chart. - ⚠️ Test may fail for navigation issues, masking formatDate bug. - ⚠️ Chart cleanup in testAssets may target id "undefined" and leak. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the Playwright spec `Handlebars formatDate helper renders a formatted date (#32960)` defined in `superset-frontend/playwright/tests/chart/handlebars-format-date.spec.ts:33-89` (for example via `npx playwright test playwright/tests/chart/handlebars-format-date.spec.ts`). 2. Within this test, a chart is created by calling `apiPost(page, 'api/v1/chart/', {...})` at `handlebars-format-date.spec.ts:59-65`, and the response body is read using `const chartId: number = (await chartResp.json()).id;` at `handlebars-format-date.spec.ts:67`, which assumes the API returns a top-level `{ id }`. 3. The same chart creation endpoint `api/v1/chart/` is used elsewhere with helpers that explicitly handle both `{ id }` and `{ result: { id } }` shapes – e.g. `createTestChart` in `superset-frontend/playwright/tests/chart/chart-test-helpers.ts:64-78` and the generic `extractIdFromResponse` in `superset-frontend/playwright/helpers/api/assertions.ts:11-18`, both resolving `body.result?.id ?? body.id`, demonstrating that a `{ result: { id } }` response is a realistic and supported shape for this API. 4. When `api/v1/chart/` returns the nested `{ result: { id } }` shape, `(await chartResp.json()).id` at `handlebars-format-date.spec.ts:67` evaluates to `undefined`, so `chartId` is `undefined`; the test then calls `testAssets.trackChart(chartId)` at line 68 (adding `undefined` into the cleanup Set used by `trackChart` in `helpers/fixtures/testAssets.ts:43-59`) and navigates to `explore/?slice_id=${chartId}` at line 70, producing an Explore URL with `slice_id=undefined`. This causes the E2E test to exercise the wrong Explore state (or fail outright) and also breaks resource cleanup (attempting to delete chart `undefined` in `testAssets` at `helpers/fixtures/testAssets.ts:102-115` while leaking the real chart), meaning the regression for `formatDate` is not reliably validated. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a0e8e87548f943d8bd02161e410f8fb1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a0e8e87548f943d8bd02161e410f8fb1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/tests/chart/handlebars-format-date.spec.ts **Line:** 67:67 **Comment:** *Api Mismatch: This assumes chart creation always returns a top-level `id`, but existing Playwright helpers in this codebase already handle both `{ id }` and `{ result: { id } }` response shapes. If the API returns the wrapped shape, `chartId` becomes `undefined`, causing navigation to `slice_id=undefined` and cleanup tracking to break. Parse both response shapes (and fail explicitly if neither exists) before using the chart ID. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40817&comment_hash=73ee78cb7a0faf73180d2e7fdb4270a86bbe75661966046c601bee96c9af28fd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40817&comment_hash=73ee78cb7a0faf73180d2e7fdb4270a86bbe75661966046c601bee96c9af28fd&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]
