mikebridge commented on code in PR #41550: URL: https://github.com/apache/superset/pull/41550#discussion_r3691303028
########## 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: Fixed before this review landed, as it happens — head `44764616b6` renamed the constant to `target_absent` (13 chars) after the PG/MySQL lanes failed on exactly the mechanism you describe (SQLite passing because it does not enforce VARCHAR length). The model now carries a comment recording the `String(16)` bound and the dialect trap so the next status value cannot repeat this. Your read of the blast radius (batch commit rolling back the legitimately-`failed` records as collateral, and the REST path burning the same doomed transaction via `reconcile_pending()`) matches what CI showed. ########## 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: Real, and fixed in `0ce35d8224`. Went with your third option, generalized: the type-change handler clears `sortColumn`/`sortOrder`/`pageIndex` from the URL before switching, so the new type starts from its own defaults (`deleted_at` desc) — that also closes the `pageIndex` carry-over you flagged in the same move, and any future per-collection axis ListView grows. `?filters=` keeps surviving the switch via the existing `urlDisplay` mechanism. Test is the exact shape you suggested (sort by Name on charts, switch to Dashboard, assert no `order_column:slice_name` in any dashboard call) and fails against the pre-fix source. ########## 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: Fixed in `0ce35d8224`: `superset-e2e.yml` gains a dedicated "Soft-delete Tests" step running `playwright-run "<app_root>" recently-archived/` with `SUPERSET_FEATURE_SOFT_DELETE: "true"` scoped to the step — each `playwright-run` boots its own gunicorn with the step env, so the Required Tests server keeps master's Flask configuration. That is the same isolation pattern the Embedded step in `superset-playwright.yml` established (and it answers this helper's own docstring objection to whole-run enablement). The Required run still collects-and-skips these specs, which is now expected and documented in the spec header. ########## 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: Corrected in `0ce35d8224` — the header now states the flag is off by default everywhere including the docker dev stack, and points at the dedicated CI step as the place where these specs actually execute. -- 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]
