codeant-ai-for-open-source[bot] commented on code in PR #41550:
URL: https://github.com/apache/superset/pull/41550#discussion_r3692123582
##########
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:
+ try:
+ days = int(value)
+ except (TypeError, ValueError):
+ # Filter values arrive from the URL; refusing loudly would turn a
+ # mangled query string into a 500. An unfiltered list is the same
+ # answer every other malformed FAB filter value produces.
+ return query
+ if days <= 0:
+ return query
+ cutoff = datetime.now() - timedelta(days=days)
+ return query.filter(self.model.deleted_at > cutoff)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not make recency filters imply the soft-delete visibility bypass;
require composition with the deleted_state filter so restore-audience scoping
is preserved and standalone queries fail closed.
**Applied to:**
- `superset/views/filters.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
superset-frontend/src/pages/ArchivedList/index.tsx:
##########
@@ -0,0 +1,558 @@
+/**
+ * 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 { useHistory } from 'react-router-dom';
+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)}
+ >
+ {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();
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> In ArchivedList, treat a successful restore/purge mutation independently
from refresh failures; use awaiting refreshData only to preserve UI ordering
and do not flag the completed mutation as failed when the refetch reports an
error.
**Applied to:**
- `superset-frontend/src/pages/ArchivedList/index.tsx`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
superset/commands/deletion_retention/purge_cascade.py:
##########
@@ -239,6 +273,28 @@ def cascade_hard_delete(
entity_uuid=uuid,
blocked_reason=str(ex),
)
+ except IntegrityError as ex:
+ # Not a policy decision: a restrictive FK the cascade did not handle.
+ # Two audiences, two messages. The curated reason goes to the caller
+ # (and from there into a user toast), because raw driver text carries
+ # the failing SQL and bind parameters. The constraint detail goes to
+ # the log at WARNING, because an entity permanently unpurgeable via an
+ # unknown FK is a cascade-coverage bug someone has to be able to
+ # diagnose -- reported at INFO as a policy block, it read as intended
+ # behaviour.
+ logger.warning(
+ "deletion_retention: %s id=%s purge failed on a restrictive "
+ "foreign key the cascade does not handle: %s",
+ entity_type,
+ entity_id,
+ ex,
+ )
+ return CascadeResult(
+ purged=False,
+ entity_type=entity_type,
+ entity_uuid=uuid,
+ blocked_reason="blocked by database references",
+ )
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not expose raw database or SQL error details in user-facing purge
responses; keep the full IntegrityError only in server logs.
**Applied to:**
- `superset/commands/deletion_retention/purge_cascade.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
superset/views/base.py:
##########
@@ -510,6 +544,8 @@ def cached_common_bootstrap_data( # pylint:
disable=unused-argument
# should not expose API TOKEN to frontend
frontend_config = {k: _get_frontend_config_value(k) for k in
FRONTEND_CONF_KEYS}
+ frontend_config.update(_soft_delete_conf())
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag the 60-second staleness of values returned by
`cached_common_bootstrap_data`; the shared cache duration is intentional, and
per-key invalidation is not worth the added coupling.
**Applied to:**
- `superset/views/base.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
--
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]
