kgabryje commented on code in PR #41550: URL: https://github.com/apache/superset/pull/41550#discussion_r3690823342
########## superset-frontend/playwright/tests/recently-archived/recently-archived.spec.ts: ########## @@ -0,0 +1,216 @@ +/** + * 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. + */ + +/** + * End-to-end coverage for the Archive (Recently-Archived) view. + * + * Requires the running instance to have the SOFT_DELETE feature flag enabled Review Comment: Small but worth correcting: the docker dev stack does **not** enable this flag. `docker/pythonpath_dev/superset_config.py` says so in as many words — *"Gated on the SOFT_DELETE feature flag, which is off by default"*. Combined with the flag also being unset in CI (see my comment on `playwright/helpers/featureFlags.ts`), these specs currently run in neither environment, so this comment will send the next person looking for a real failure down the wrong path. ########## superset-frontend/src/pages/ArchivedList/index.tsx: ########## @@ -0,0 +1,526 @@ +/** + * 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 { useCallback, useMemo, useRef, useState } from 'react'; +import { useAppSelector } from 'src/views/store'; +import { getClientErrorObject, SupersetClient } from '@superset-ui/core'; +import { t } from '@apache-superset/core/translation'; +import { styled } from '@apache-superset/core/theme'; +import { + ActionButton, + ConfirmStatusChange, + Select, + Tooltip, +} from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { useListViewResource } from 'src/views/CRUD/hooks'; +import { + ListView, + ListViewFilterOperator as FilterOperator, + type ListViewProps, + type ListViewFilters, +} from 'src/components'; +import SubMenu from 'src/features/home/SubMenu'; +import withToasts from 'src/components/MessageToasts/withToasts'; +import { recoveredToast } from 'src/utils/softDeleteCopy'; +import { findPermission } from 'src/utils/findPermission'; +import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; +import { + ARCHIVED_TYPES, + ARCHIVED_TYPE_CONFIG, + type ArchivedItem, + type ArchivedType, +} from './types'; + +const PAGE_SIZE = 25; + +const TypeSelectRow = styled.div` + ${({ theme }) => ` + padding: ${theme.sizeUnit * 2}px ${theme.sizeUnit * 4}px; + width: 240px; + `} +`; + +const StyledActions = styled.div` + ${({ theme }) => ` + color: ${theme.colorIcon}; + + /* TableCollection hides .actions with opacity and reveals them on row + hover. Without a focus companion, tabbing lands on fully transparent + controls — and on this page recovering and permanently deleting are + the only actions there are. Scoped here rather than in the shared + component, which has the same gap on every list view. */ + &:focus-within { + opacity: 1; + } + `} +`; + +const EmptyStateRow = styled.div` + ${({ theme }) => ` + padding: ${theme.sizeUnit * 6}px; + color: ${theme.colorTextSecondary}; + `} +`; + +const TYPE_LABELS: Record<ArchivedType, string> = { + chart: t('Chart'), + dashboard: t('Dashboard'), + dataset: t('Dataset'), +}; + +interface ToastProps { + addDangerToast: (msg: string) => void; + addSuccessToast: (msg: string, options?: { allowHtml?: boolean }) => void; +} + +/** The per-row Recover + Delete-permanently actions. */ +function ArchivedRowActions({ + item, + name, + onRestore, + onPurge, + busy = false, +}: { + item: ArchivedItem; + name: string; + onRestore: (item: ArchivedItem) => void; + onPurge: (item: ArchivedItem) => void; + /** A request for this row is in flight; both actions stand down. */ + busy?: boolean; +}) { + return ( + <StyledActions className="actions"> + <ActionButton + label={t('Recover')} + tooltip={t('Recover this item')} + placement="bottom" + icon={<Icons.RollbackOutlined iconSize="l" />} + dataTest="archived-row-restore" + disabled={busy} + onClick={() => onRestore(item)} + /> + <ConfirmStatusChange + title={t('Delete permanently %(name)s?', { name })} + description={t( + "If you delete this item, you won't be able to recover it.", + )} + onConfirm={() => onPurge(item)} + requireConfirmationText={false} + > + {confirmDelete => ( + <ActionButton + label={t('Delete permanently')} + tooltip={t('Delete permanently')} + placement="bottom" + icon={<Icons.DeleteOutlined iconSize="l" />} + dataTest="archived-row-purge" + disabled={busy} + onClick={confirmDelete} + /> + )} + </ConfirmStatusChange> + </StyledActions> + ); +} + +/** + * The per-type table body. Mounted with `key={type}` by the parent so the + * `useListViewResource` state and derived columns reset cleanly on a type + * switch. Sourced from the selected type's existing list endpoint with the + * soft-delete `<type>_deleted_state:only` baseline filter. + */ +function ArchivedListBody({ + type, + addDangerToast, + addSuccessToast, +}: ToastProps & { type: ArchivedType }) { + const config = ARCHIVED_TYPE_CONFIG[type]; + + const baseFilters = useMemo( + () => [{ id: 'id', operator: config.deletedStateOperator, value: 'only' }], + [config.deletedStateOperator], + ); + + const { + state: { loading, resourceCount, resourceCollection }, + fetchData, + refreshData, + } = useListViewResource<ArchivedItem>( + config.resource, + TYPE_LABELS[type], + addDangerToast, + true, + [], + baseFilters, + ); + + // Restore is immediate (no confirm dialog). On success, refetch the full page + // so the server-side count/pagination stays consistent and the row drops out; + // on any error surface a danger toast and leave the row in place. The list + // read is already owner-scoped, so every visible row is restorable. + // A second activation while a request is in flight races the first: by the + // time the retry lands the row is already restored (or purged), so the + // server answers 404 and the user is shown a failure after a success. The + // ref is the guard rather than the state, because state updates are async + // and two quick clicks could both pass a state check; the state mirrors it + // so the buttons can render disabled meanwhile. + const inFlightRef = useRef<Set<string>>(new Set()); + const [inFlight, setInFlight] = useState<readonly string[]>([]); + + const beginAction = useCallback((uuid: string): boolean => { + if (inFlightRef.current.has(uuid)) { + return false; + } + inFlightRef.current.add(uuid); + setInFlight([...inFlightRef.current]); + return true; + }, []); + + const endAction = useCallback((uuid: string) => { + inFlightRef.current.delete(uuid); + setInFlight([...inFlightRef.current]); + }, []); + + const handleRestore = useCallback( + async (item: ArchivedItem) => { + const name = String(item[config.nameField] ?? ''); + if (!beginAction(item.uuid)) { + return; + } + try { + await SupersetClient.post({ + endpoint: `/api/v1/${config.resource}/${item.uuid}/restore`, + }); + const { text, options } = recoveredToast( + name, + TYPE_LABELS[type], + item.url ?? item.explore_url, + ); + addSuccessToast(text, options); + // Awaited so the finally's endAction does not re-enable this row's + // buttons while the stale, already-restored row is still rendered -- + // a keyboard user could re-activate it and get a 404 after success. + await refreshData(); + } catch (error) { + const { error: errMsg } = await getClientErrorObject(error); + addDangerToast( + errMsg + ? t('Failed to restore %(name)s: %(errMsg)s', { name, errMsg }) + : t('Failed to restore %(name)s', { name }), + ); + } finally { + endAction(item.uuid); + } + }, + [ + config.resource, + config.nameField, + type, + addSuccessToast, + addDangerToast, + refreshData, + beginAction, + endAction, + ], + ); + + // Permanent delete (force-purge) of an archived item — irreversible. Owner/ + // admin-gated server-side (mirrors restore). The confirmation is a plain + // danger modal (no type-to-confirm), per the "delete forever" design. + const handlePurge = useCallback( + async (item: ArchivedItem) => { + const name = String(item[config.nameField] ?? ''); + if (!beginAction(item.uuid)) { + return; + } + try { + await SupersetClient.post({ + endpoint: `/api/v1/${config.resource}/${item.uuid}/purge`, + }); + addSuccessToast(t('%(name)s deleted successfully', { name })); + // Awaited for the same reason as the restore path: the in-flight + // guard must outlive the stale row. + await refreshData(); + } catch (error) { + // A blocked purge answers 422 carrying the reason -- an alert or + // report still referencing the object. The docs promise that reason + // is shown, and it is the only thing telling the user what to remove + // before retrying. + const { error: errMsg } = await getClientErrorObject(error); + addDangerToast( + errMsg + ? t('Failed to delete %(name)s: %(errMsg)s', { name, errMsg }) + : t('Failed to delete %(name)s', { name }), + ); + } finally { + endAction(item.uuid); + } + }, + [ + config.resource, + config.nameField, + addSuccessToast, + addDangerToast, + refreshData, + beginAction, + endAction, + ], + ); + + const columns = useMemo<ListViewProps['columns']>( + () => [ + { + Cell: ({ row: { original } }: { row: { original: ArchivedItem } }) => { + const name = String(original[config.nameField] ?? ''); + // Archived objects are not viewable in place. Verified against a + // running instance: an archived dashboard's page 404s, and an + // archived chart's explore page answers 200 with no chart and no + // error — the reader is shown what looks like an empty new chart + // rather than told anything. Neither is a preview, and the silent + // one is the worse of the two, so no row links out until the object + // is recovered. + return ( + <Tooltip title={t('Recover this item to open it')}> + <span>{name}</span> + </Tooltip> + ); + }, + accessor: config.nameField, + Header: t('Name'), + id: config.nameField, Review Comment: **Likely blocking:** this makes the Name column sortable under a **per-type** id (`slice_name` / `dashboard_title` / `table_name`), and that id leaks across a Type switch into a request that the API rejects. The chain: 1. No `disableSortBy` here, so the column is sortable and its `id` is whichever `config.nameField` the current type uses. 2. `ListView` persists the active sort into the shared URL — `queryParams.sortColumn = sortBy[0].id` in `src/components/ListView/utils.ts` — and reads it back on every mount. 3. Switching Type is pure React state and remounts the body via `key={type}` **without touching the URL**, so `sortColumn=slice_name` survives into the dashboard fetch. 4. `slice_name` is not in `order_columns` for dashboards or datasets, and Flask-AppBuilder *rejects* rather than ignores an unknown order column: `raise InvalidOrderByColumnFABException(...)`, returned as `response_400`. Superset doesn't intercept it. Net effect: sort by Name on Chart → switch to Dashboard → `GET /api/v1/dashboard/?q=(order_column:slice_name,...)` → 400, empty table, generic "An error occurred while fetching Dashboards" toast. The bad `sortColumn` is now in the URL, so every later type switch **and every reload** stays broken until the user re-sorts or edits the address bar. This is the same failure mode the `urlDisplay: 'name'` fix solved on the filter axis (the comment above it describes it exactly); the sort axis has no `urlDisplay` equivalent and was left exposed. **Fix options:** give Name a type-stable `id` and map it to the API column in `fetchData`; or `disableSortBy: true` on Name (the page's natural sort is `deleted_at`, which *is* orderable on all three); or clear `sortColumn`/`sortOrder` from the query params when `setType` fires. `pageIndex` has the same carry-over problem, more mildly — switching from page 4 of charts to a single-page dataset list shows "no data" while the pager claims rows exist. **To prove it:** in `ArchivedList.test.tsx`, click the "Name" header, switch Type to Dashboard, then assert the last `/dashboard/?q` call does not contain `order_column:slice_name`. It currently does. *(One caveat on the FAB behaviour: I verified the `raise` against flask-appbuilder 5.2.1, one patch below this repo's `>=5.2.2` floor. Worth a second look if you think 5.2.2 changed it.)* ########## superset-frontend/src/pages/ArchivedList/index.tsx: ########## @@ -0,0 +1,526 @@ +/** + * 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 { useCallback, useMemo, useRef, useState } from 'react'; +import { useAppSelector } from 'src/views/store'; +import { getClientErrorObject, SupersetClient } from '@superset-ui/core'; +import { t } from '@apache-superset/core/translation'; +import { styled } from '@apache-superset/core/theme'; +import { + ActionButton, + ConfirmStatusChange, + Select, + Tooltip, +} from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { useListViewResource } from 'src/views/CRUD/hooks'; +import { + ListView, + ListViewFilterOperator as FilterOperator, + type ListViewProps, + type ListViewFilters, +} from 'src/components'; +import SubMenu from 'src/features/home/SubMenu'; +import withToasts from 'src/components/MessageToasts/withToasts'; +import { recoveredToast } from 'src/utils/softDeleteCopy'; +import { findPermission } from 'src/utils/findPermission'; +import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; +import { + ARCHIVED_TYPES, + ARCHIVED_TYPE_CONFIG, + type ArchivedItem, + type ArchivedType, +} from './types'; + +const PAGE_SIZE = 25; + +const TypeSelectRow = styled.div` + ${({ theme }) => ` + padding: ${theme.sizeUnit * 2}px ${theme.sizeUnit * 4}px; + width: 240px; + `} +`; + +const StyledActions = styled.div` + ${({ theme }) => ` + color: ${theme.colorIcon}; + + /* TableCollection hides .actions with opacity and reveals them on row + hover. Without a focus companion, tabbing lands on fully transparent + controls — and on this page recovering and permanently deleting are + the only actions there are. Scoped here rather than in the shared + component, which has the same gap on every list view. */ + &:focus-within { + opacity: 1; + } + `} +`; + +const EmptyStateRow = styled.div` + ${({ theme }) => ` + padding: ${theme.sizeUnit * 6}px; + color: ${theme.colorTextSecondary}; + `} +`; + +const TYPE_LABELS: Record<ArchivedType, string> = { + chart: t('Chart'), + dashboard: t('Dashboard'), + dataset: t('Dataset'), +}; + +interface ToastProps { + addDangerToast: (msg: string) => void; + addSuccessToast: (msg: string, options?: { allowHtml?: boolean }) => void; +} + +/** The per-row Recover + Delete-permanently actions. */ +function ArchivedRowActions({ + item, + name, + onRestore, + onPurge, + busy = false, +}: { + item: ArchivedItem; + name: string; + onRestore: (item: ArchivedItem) => void; + onPurge: (item: ArchivedItem) => void; + /** A request for this row is in flight; both actions stand down. */ + busy?: boolean; +}) { + return ( + <StyledActions className="actions"> + <ActionButton + label={t('Recover')} + tooltip={t('Recover this item')} + placement="bottom" + icon={<Icons.RollbackOutlined iconSize="l" />} + dataTest="archived-row-restore" + disabled={busy} + onClick={() => onRestore(item)} + /> + <ConfirmStatusChange + title={t('Delete permanently %(name)s?', { name })} + description={t( + "If you delete this item, you won't be able to recover it.", + )} + onConfirm={() => onPurge(item)} + requireConfirmationText={false} Review Comment: Worth reconsidering: `requireConfirmationText={false}` means `DeleteModal` drops the confirmation input (`showConfirmationInput = !recoverable && requireConfirmationText`), so the single most destructive action in the product — an irreversible cascade purge — is two clicks, while an ordinary hard delete elsewhere in Superset still makes you type `DELETE`. That inverts the platform's own friction convention precisely where the stakes are highest. Echoing the object name in the title helps, but it isn't the same gate. The recoverable *archive* path is the one that earned reduced friction; I'd keep the type-to-confirm here and let `recoverable` carry the softening. ########## superset-frontend/src/pages/DashboardList/index.tsx: ########## @@ -336,11 +340,23 @@ function DashboardList(props: DashboardListProps) { }).then( ({ json = {} }) => { refreshData(); - addSuccessToast(json.message); + addSuccessToast( + softDelete + ? t('Archived %s item(s)', dashboardsToDelete.length) + : json.message, + ); }, createErrorHandler(errMsg => addDangerToast( - t('There was an issue deleting the selected dashboards: ', errMsg), + softDelete + ? t( + 'There was an issue archiving the selected dashboards: %s', + errMsg, + ) + : t( + 'There was an issue deleting the selected dashboards: %s', Review Comment: Heads-up rather than an objection: this changes an existing translated msgid on the flag-**off** path, so the PR's "with the flag off, behaviour is byte-for-byte the pre-existing hard-delete flow" claim isn't strictly true. Before, it was `t('There was an issue deleting the selected dashboards: ', errMsg)` — no `%s`, so `errMsg` was silently discarded. Fixing that is clearly right, but the msgid change orphans every existing `.po` entry for the string, and it happens with the flag disabled. Worth a line in the PR description so translators and anyone diffing the flag-off path know it's intentional. ########## superset/models/purge_audit_log.py: ########## @@ -34,6 +34,11 @@ STATUS_CONFIRMED = "confirmed" STATUS_FAILED = "failed" STATUS_BLOCKED = "blocked" +#: Reconciliation found the target durably gone but cannot prove THIS attempt +#: removed it -- a concurrent purge or an unrelated deletion is equally +#: consistent with the evidence. Deliberately distinct from ``confirmed`` so +#: the compliance record never attributes a success it did not witness. +STATUS_RECONCILED_ABSENT = "reconciled_absent" Review Comment: **Likely blocking:** this value is 17 characters, but the column is `VARCHAR(16)`. ```python status = Column(String(16), nullable=False, default=STATUS_PENDING) # :55 ``` and the migration agrees — `sa.Column("status", sa.String(length=16), nullable=False)` in `2026-07-28_09-00_e7d93a524ff6_add_purge_audit_log.py`. `git diff` over `superset/migrations/` on this branch is empty, so nothing widens it. The write is `record.status = STATUS_RECONCILED_ABSENT` in `commands/deletion_retention/audit.py`, inside a loop followed by a single `session.commit()` wrapped in `except Exception: session.rollback()`. On PostgreSQL (`value too long for type character varying(16)`) and MySQL in strict mode that commit raises, so: - stale `pending` audit rows — the compliance record — are never finalized on either production backend; - because it's one commit for the whole batch, the records that *should* have become `failed` (which fits in 16) are rolled back as collateral; - the failure is swallowed to a WARNING, so it's silent. It also isn't confined to the scheduled task: `force_purge.py` calls `audit.reconcile_pending()` at the top of `run()`, so every REST purge burns a doomed transaction. This passes CI because SQLite doesn't enforce VARCHAR length, and `deletion_retention/audit_tests.py` asserts `row.status == audit.STATUS_RECONCILED_ABSENT` — so the assertion is correct and still can't catch it. **Fix:** either shorten the constant to ≤16 chars (e.g. `"absent"`), or add a migration widening `purge_audit_log.status` and update the model. **To prove it:** run `tests/integration_tests/deletion_retention/audit_tests.py` against Postgres. ########## superset-frontend/playwright/helpers/featureFlags.ts: ########## @@ -0,0 +1,83 @@ +/** + * 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, test } from '@playwright/test'; + +/** + * Read a feature flag the way the application itself does. + * + * The server injects the flag map into the page bootstrap and the frontend + * consults `window.featureFlags` via `isFeatureEnabled`, so this is the same + * signal that decides whether flag-gated UI renders. A Superset page must + * already be loaded. + */ +export async function isFeatureEnabled( + page: Page, + flag: string, +): Promise<boolean> { + // The bootstrap runs as part of the bundle, so the map can be absent for a + // moment after the load event. Wait for it rather than racing: reading too + // early would report every flag as off, which for a skip guard means + // silently standing down on an instance where the feature is in fact on. + await page.waitForFunction( + () => + Boolean( + (window as unknown as { featureFlags?: Record<string, boolean> }) + .featureFlags, + ), + undefined, + { timeout: 30000 }, + ); + return page.evaluate( + name => + Boolean( + (window as unknown as { featureFlags?: Record<string, boolean> }) + .featureFlags?.[name], + ), + flag, + ); +} + +/** + * Stand down when `flag` is off on the instance under test. + * + * Features that ship dark behind a release toggle are not reachable in a + * default CI run, so their end-to-end coverage cannot pass there. Skipping + * says that plainly rather than failing on an element that was never meant to + * render, and the specs still run for real against any instance with the flag + * on — including once the toggle is flipped. + * + * Enabling such a flag for the whole Playwright run is not an alternative: + * a flag that changes application behaviour also changes it for every other + * spec sharing that server. + * + * Only a flag that is present and off causes a skip. If the flag map never + * appears the probe throws instead, so a genuinely broken page fails loudly + * rather than disguising itself as an empty run. + */ +export async function skipUnlessFeatureEnabled( + page: Page, + flag: string, + probeUrl = 'chart/list/', +): Promise<void> { + await page.goto(probeUrl); + test.skip( Review Comment: **Likely blocking:** this guard is correct in isolation, but it means none of the new E2E coverage runs anywhere. All three new specs call this unconditionally in `beforeEach` (`menu-link.spec.ts`, `delete-modal.spec.ts`, `recently-archived.spec.ts`). And: - `superset/config.py` has `"SOFT_DELETE": False`; - `grep -rn SOFT_DELETE .github/` returns **0 hits** — the Playwright job sets `SUPERSET_ENV`, `SUPERSET_CONFIG`, the DB URI, `PYTHONPATH`, `REDIS_PORT`, `GITHUB_TOKEN`, and no feature flags; - `tests/integration_tests/superset_test_config.py` (the config that job loads) overrides `ENABLE_TEMPLATE_PROCESSING`, `ALERT_REPORTS`, `DRILL_TO_DETAIL`, `DRILL_BY`, `GLOBAL_TASK_FRAMEWORK` — not `SOFT_DELETE`. So all 8 tests are collected, skipped, and the job goes green: ~314 lines of E2E ship with zero executed coverage. The skip is effectively invisible too — the `github` reporter annotates failures rather than skips, and the HTML/JSON reports that carry the skip reason are uploaded only `if: failure()`. **Fix:** add `SUPERSET_FEATURE_SOFT_DELETE: "true"` to a dedicated Playwright project or job — the env-override mechanism already exists in `superset/config.py`. Failing that, at minimum make the skip loud enough that an all-skipped run is distinguishable from a real pass. ########## superset/views/filters.py: ########## @@ -292,6 +314,37 @@ def _mark_response_for_deleted_at_augmentation() -> None: setattr(g, AUGMENT_RESPONSE_WITH_DELETED_AT, True) +class BaseDeletedRecencyFilter(BaseFilter): # pylint: disable=too-few-public-methods + """Keep rows archived within the last *value* days, by the server's clock. + + The archive UI's time-range presets used to send an absolute cutoff + computed client-side in UTC. ``deleted_at`` is stamped with the server's + naive-local ``datetime.now()``, so on any non-UTC deployment those + cutoffs were shifted by the server offset -- and because the cutoff was + frozen when the page mounted, a long-lived tab drifted further. Taking a + day count and resolving it here, on the clock that stamped the column, + removes both failure modes and lets the client keep stable, shareable + filter values. + + Subclasses set ``arg_name`` (e.g. ``"chart_deleted_recency"``). + """ + + name = lazy_gettext("Archived within") + + def apply(self, query: Query, value: Any) -> Query: Review Comment: Minor: this catches `TypeError`/`ValueError`, but `int()` happily parses arbitrarily large integers, and `timedelta` then raises `OverflowError`, which isn't caught. ```python if days <= 0: return query cutoff = datetime.now() - timedelta(days=days) ``` `timedelta(days=999999999999999999999)` → `OverflowError: Python int too large to convert to C int`. So a crafted rison filter such as `chart_deleted_recency=999999999999999999999` produces a 500 — which is the exact outcome the comment just above says it's avoiding ("refusing loudly would turn a mangled query string into a 500"). **Fix:** validate against a bounded positive range before constructing the `timedelta`, or at minimum add `OverflowError` to the caught tuple. ########## superset-frontend/src/pages/ArchivedList/index.tsx: ########## @@ -0,0 +1,526 @@ +/** + * 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 { useCallback, useMemo, useRef, useState } from 'react'; +import { useAppSelector } from 'src/views/store'; +import { getClientErrorObject, SupersetClient } from '@superset-ui/core'; +import { t } from '@apache-superset/core/translation'; +import { styled } from '@apache-superset/core/theme'; +import { + ActionButton, + ConfirmStatusChange, + Select, + Tooltip, +} from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { useListViewResource } from 'src/views/CRUD/hooks'; +import { + ListView, + ListViewFilterOperator as FilterOperator, + type ListViewProps, + type ListViewFilters, +} from 'src/components'; +import SubMenu from 'src/features/home/SubMenu'; +import withToasts from 'src/components/MessageToasts/withToasts'; +import { recoveredToast } from 'src/utils/softDeleteCopy'; +import { findPermission } from 'src/utils/findPermission'; +import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; +import { + ARCHIVED_TYPES, + ARCHIVED_TYPE_CONFIG, + type ArchivedItem, + type ArchivedType, +} from './types'; + +const PAGE_SIZE = 25; + +const TypeSelectRow = styled.div` + ${({ theme }) => ` + padding: ${theme.sizeUnit * 2}px ${theme.sizeUnit * 4}px; + width: 240px; + `} +`; + +const StyledActions = styled.div` + ${({ theme }) => ` + color: ${theme.colorIcon}; + + /* TableCollection hides .actions with opacity and reveals them on row + hover. Without a focus companion, tabbing lands on fully transparent + controls — and on this page recovering and permanently deleting are + the only actions there are. Scoped here rather than in the shared + component, which has the same gap on every list view. */ + &:focus-within { + opacity: 1; + } + `} +`; + +const EmptyStateRow = styled.div` + ${({ theme }) => ` + padding: ${theme.sizeUnit * 6}px; + color: ${theme.colorTextSecondary}; + `} +`; + +const TYPE_LABELS: Record<ArchivedType, string> = { + chart: t('Chart'), + dashboard: t('Dashboard'), + dataset: t('Dataset'), +}; + +interface ToastProps { + addDangerToast: (msg: string) => void; + addSuccessToast: (msg: string, options?: { allowHtml?: boolean }) => void; +} + +/** The per-row Recover + Delete-permanently actions. */ +function ArchivedRowActions({ + item, + name, + onRestore, + onPurge, + busy = false, +}: { + item: ArchivedItem; + name: string; + onRestore: (item: ArchivedItem) => void; + onPurge: (item: ArchivedItem) => void; + /** A request for this row is in flight; both actions stand down. */ + busy?: boolean; +}) { + return ( + <StyledActions className="actions"> + <ActionButton + label={t('Recover')} + tooltip={t('Recover this item')} + placement="bottom" + icon={<Icons.RollbackOutlined iconSize="l" />} + dataTest="archived-row-restore" + disabled={busy} + onClick={() => onRestore(item)} + /> + <ConfirmStatusChange + title={t('Delete permanently %(name)s?', { name })} + description={t( + "If you delete this item, you won't be able to recover it.", + )} + onConfirm={() => onPurge(item)} + requireConfirmationText={false} + > + {confirmDelete => ( + <ActionButton + label={t('Delete permanently')} + tooltip={t('Delete permanently')} + placement="bottom" + icon={<Icons.DeleteOutlined iconSize="l" />} + dataTest="archived-row-purge" + disabled={busy} + onClick={confirmDelete} + /> + )} + </ConfirmStatusChange> + </StyledActions> + ); +} + +/** + * The per-type table body. Mounted with `key={type}` by the parent so the + * `useListViewResource` state and derived columns reset cleanly on a type + * switch. Sourced from the selected type's existing list endpoint with the + * soft-delete `<type>_deleted_state:only` baseline filter. + */ +function ArchivedListBody({ + type, + addDangerToast, + addSuccessToast, +}: ToastProps & { type: ArchivedType }) { + const config = ARCHIVED_TYPE_CONFIG[type]; + + const baseFilters = useMemo( + () => [{ id: 'id', operator: config.deletedStateOperator, value: 'only' }], + [config.deletedStateOperator], + ); + + const { + state: { loading, resourceCount, resourceCollection }, + fetchData, + refreshData, + } = useListViewResource<ArchivedItem>( + config.resource, + TYPE_LABELS[type], + addDangerToast, + true, + [], + baseFilters, + ); + + // Restore is immediate (no confirm dialog). On success, refetch the full page + // so the server-side count/pagination stays consistent and the row drops out; + // on any error surface a danger toast and leave the row in place. The list + // read is already owner-scoped, so every visible row is restorable. + // A second activation while a request is in flight races the first: by the + // time the retry lands the row is already restored (or purged), so the + // server answers 404 and the user is shown a failure after a success. The + // ref is the guard rather than the state, because state updates are async + // and two quick clicks could both pass a state check; the state mirrors it + // so the buttons can render disabled meanwhile. + const inFlightRef = useRef<Set<string>>(new Set()); + const [inFlight, setInFlight] = useState<readonly string[]>([]); + + const beginAction = useCallback((uuid: string): boolean => { + if (inFlightRef.current.has(uuid)) { + return false; + } + inFlightRef.current.add(uuid); + setInFlight([...inFlightRef.current]); + return true; + }, []); + + const endAction = useCallback((uuid: string) => { + inFlightRef.current.delete(uuid); + setInFlight([...inFlightRef.current]); + }, []); + + const handleRestore = useCallback( + async (item: ArchivedItem) => { + const name = String(item[config.nameField] ?? ''); + if (!beginAction(item.uuid)) { + return; + } + try { + await SupersetClient.post({ + endpoint: `/api/v1/${config.resource}/${item.uuid}/restore`, + }); + const { text, options } = recoveredToast( + name, + TYPE_LABELS[type], + item.url ?? item.explore_url, + ); + addSuccessToast(text, options); + // Awaited so the finally's endAction does not re-enable this row's + // buttons while the stale, already-restored row is still rendered -- + // a keyboard user could re-activate it and get a 404 after success. + await refreshData(); + } catch (error) { + const { error: errMsg } = await getClientErrorObject(error); + addDangerToast( + errMsg + ? t('Failed to restore %(name)s: %(errMsg)s', { name, errMsg }) + : t('Failed to restore %(name)s', { name }), + ); + } finally { + endAction(item.uuid); + } + }, + [ + config.resource, + config.nameField, + type, + addSuccessToast, + addDangerToast, + refreshData, + beginAction, + endAction, + ], + ); + + // Permanent delete (force-purge) of an archived item — irreversible. Owner/ + // admin-gated server-side (mirrors restore). The confirmation is a plain + // danger modal (no type-to-confirm), per the "delete forever" design. + const handlePurge = useCallback( + async (item: ArchivedItem) => { + const name = String(item[config.nameField] ?? ''); + if (!beginAction(item.uuid)) { + return; + } + try { + await SupersetClient.post({ + endpoint: `/api/v1/${config.resource}/${item.uuid}/purge`, + }); + addSuccessToast(t('%(name)s deleted successfully', { name })); + // Awaited for the same reason as the restore path: the in-flight + // guard must outlive the stale row. + await refreshData(); + } catch (error) { + // A blocked purge answers 422 carrying the reason -- an alert or + // report still referencing the object. The docs promise that reason + // is shown, and it is the only thing telling the user what to remove + // before retrying. + const { error: errMsg } = await getClientErrorObject(error); + addDangerToast( + errMsg + ? t('Failed to delete %(name)s: %(errMsg)s', { name, errMsg }) + : t('Failed to delete %(name)s', { name }), + ); + } finally { + endAction(item.uuid); + } + }, + [ + config.resource, + config.nameField, + addSuccessToast, + addDangerToast, + refreshData, + beginAction, + endAction, + ], + ); + + const columns = useMemo<ListViewProps['columns']>( + () => [ + { + Cell: ({ row: { original } }: { row: { original: ArchivedItem } }) => { + const name = String(original[config.nameField] ?? ''); + // Archived objects are not viewable in place. Verified against a + // running instance: an archived dashboard's page 404s, and an + // archived chart's explore page answers 200 with no chart and no + // error — the reader is shown what looks like an empty new chart + // rather than told anything. Neither is a preview, and the silent + // one is the worse of the two, so no row links out until the object + // is recovered. + return ( + <Tooltip title={t('Recover this item to open it')}> + <span>{name}</span> + </Tooltip> + ); + }, + accessor: config.nameField, + Header: t('Name'), + id: config.nameField, + }, + { + Cell: () => TYPE_LABELS[type], + Header: t('Type'), + id: 'type', + disableSortBy: true, + }, + { + // Relative archive time, humanized by the SERVER (like + // changed_on_delta_humanized on the sibling pages). deleted_at is + // stamped with the server's naive-local clock, so parsing it here + // means guessing the server's timezone -- this page used to guess + // UTC, shifting every age by the server offset on non-UTC + // deployments. Sortable: id stays deleted_at, which is in + // order_columns on all three list APIs. + Cell: ({ row: { original } }: { row: { original: ArchivedItem } }) => + String(original.deleted_at_delta_humanized ?? ''), + Header: t('Archived'), + id: 'deleted_at', + }, + { + // Archiving user, from changed_by. Non-sortable — there is no backend + // deleted-by ordering. + Cell: ({ row: { original } }: { row: { original: ArchivedItem } }) => { + const by = [ + original.changed_by?.first_name, + original.changed_by?.last_name, + ] + .filter(Boolean) + .join(' '); + return by || t('Unknown'); + }, + Header: t('Archived by'), + id: 'archived_by', + disableSortBy: true, + }, + { + Cell: ({ row: { original } }: { row: { original: ArchivedItem } }) => ( + <ArchivedRowActions + item={original} + name={String(original[config.nameField] ?? '')} + onRestore={handleRestore} + onPurge={handlePurge} + busy={inFlight.includes(original.uuid)} + /> + ), + Header: t('Actions'), + id: 'actions', + disableSortBy: true, + size: 'sm', + }, + ], + [config.nameField, type, handleRestore, handlePurge, inFlight], + ); + + // Default to most-recently-archived first. `deleted_at` is orderable on all + // three list endpoints, so it's the natural sort. + const initialSort = useMemo(() => [{ id: 'deleted_at', desc: true }], []); + + // Time-range presets send a day count; the server resolves the cutoff with + // the same clock that stamped deleted_at. An absolute cutoff computed here + // was wrong three ways: it was client-UTC against a server-local column + // (shifted by the server offset), it was frozen at mount (a long-lived tab + // drifted a day per day), and it was persisted into ?filters= as a + // timestamp no regenerated option could ever match. A day count has none + // of those failure modes. "All time" is the unfiltered default. + const timeRangeOptions = useMemo( + () => [ + { label: t('Last 7 days'), value: 7 }, + { label: t('Last 30 days'), value: 30 }, + { label: t('Last 90 days'), value: 90 }, + ], + [], + ); + + const filters: ListViewFilters = useMemo( + () => [ + { + Header: t('Name'), + key: 'search', + id: config.nameField, + // The API column differs per type (slice_name / dashboard_title / + // table_name) but ListView persists applied filters in the shared + // ?filters= param keyed by this id. Switching Type remounts the body + // without touching the URL, so a per-type key would come back as a + // stale entry that the new type's filter list cannot claim -- it + // reaches fetchData with operator undefined and rison refuses to + // encode it, leaving the list permanently empty. A stable URL key is + // claimed by whichever type is mounted, which then rewrites the id + // back to its own column. + urlDisplay: 'name', + input: 'search', + // Charts expose an all-text search on slice_name (chart_all_text) + // rather than a plain `ct`; dashboards/datasets accept `ct` on their + // name column. + operator: + type === 'chart' + ? FilterOperator.ChartAllText + : FilterOperator.Contains, + }, + { + Header: t('Archived'), + key: 'deleted_at', + id: 'deleted_at', + input: 'select', + operator: config.deletedRecencyOperator as FilterOperator, + unfilteredLabel: t('All time'), + selects: timeRangeOptions, + }, + ], + [config.nameField, type, timeRangeOptions], + ); + + return ( + <ListView<ArchivedItem> + className="archived-list-view" + columns={columns} + filters={filters} + data={resourceCollection} + count={resourceCount} + pageSize={PAGE_SIZE} + fetchData={fetchData} + refreshData={refreshData} + addSuccessToast={addSuccessToast} + addDangerToast={addDangerToast} + loading={loading} + initialSort={initialSort} + emptyState={{ + title: t('No archived items'), + image: 'empty.svg', + }} + /> + ); +} + +/** + * Archive (Recently-Archived) view (sc-111760): find and restore soft-deleted + * charts, dashboards, and datasets — one type at a time via the Type selector. + */ +function ArchivedList({ addDangerToast, addSuccessToast }: ToastProps) { + const roles = useAppSelector( + state => + (state.user as UserWithPermissionsAndRoles | undefined)?.roles ?? + undefined, + ); + + // Offer only the types this viewer can load. The page fronts three + // independently-gated list APIs, so a single all-or-nothing gate is the + // wrong shape in both directions: it can hide the whole archive from + // someone who owns archived datasets, and it can offer a type whose API + // will answer 403. This is presentation only — each API remains the + // enforcement point, so a hand-crafted request is still refused. + const availableTypes = useMemo(() => { + // Without roles we cannot say what is readable, so offer everything and + // let the APIs answer — the same behaviour as before this filter existed. + // Narrowing on missing information would hide the whole page instead. + if (!roles) { + return ARCHIVED_TYPES; + } + // A viewer whose roles resolve to NO readable type gets the empty state Review Comment: This branch looks unreachable, and the PR description describes it as user-visible behaviour. The description says *"a viewer with none of the three sees an explanatory empty state"*. But the server refuses before the SPA shell is ever served — `superset/views/archived_assets.py`: ```python if not any( security_manager.can_access("can_read", resource) for resource in _ADMITTING_RESOURCES ): abort(403) ``` and `tests/integration_tests/views/archived_assets_tests.py` asserts `rv.status_code == 403` for exactly that user. So a viewer with none of the three types gets a 403 error page, never this empty state. Not a functional bug — the 403 is arguably the better behaviour — but three things are out of sync: this code path, the comment describing it, and the PR description. Worth picking one. If 403 is intended, this branch and its comment can go; if the empty state is intended, the server check needs to admit the user and let the client explain. -- 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]
