mikebridge commented on code in PR #41550:
URL: https://github.com/apache/superset/pull/41550#discussion_r3691308962
##########
superset/commands/deletion_retention/audit.py:
##########
@@ -219,9 +228,8 @@ def reconcile_pending(stale_before: datetime | None = None)
-> dict[str, int]:
)
for record in records:
if _entity_exists(session, record) is False:
- record.status = STATUS_CONFIRMED
- record.confirmed_on = _utc_now()
- confirmed += 1
+ record.status = STATUS_RECONCILED_ABSENT
Review Comment:
Already fixed at head `44764616b6` (this review ran against the prior sha):
the constant is now `target_absent` (13 chars ≤ String(16)), renamed after the
PG/MySQL CI lanes failed on exactly this write.
##########
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:
Known trade-off of `cached_common_bootstrap_data`'s 60-second memoize,
shared by every value it carries (feature flags, menu, conf) — a runtime
retention change is stale for at most a minute, same as the rest of the
bootstrap payload. Invalidation hooks for one key inside the shared cache are
not worth the coupling; declining here.
##########
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
+ * (the docker dev stack does). Each test creates a disposable object via the
+ * authenticated REST API, soft-deletes it, then drives the real UI to restore
+ * it and asserts — via the API — that it is live again.
+ */
+import { test, expect, Page } from '@playwright/test';
+import { apiGet, apiPost } from '../../helpers/api/requests';
+import {
+ apiPostChart,
+ apiGetChart,
+ apiDeleteChart,
+} from '../../helpers/api/chart';
+import {
+ apiPostDashboard,
+ apiGetDashboard,
+ apiDeleteDashboard,
+} from '../../helpers/api/dashboard';
+import {
+ createTestVirtualDataset,
+ apiGetDataset,
+ apiDeleteDataset,
+} from '../../helpers/api/dataset';
+import { skipUnlessFeatureEnabled } from '../../helpers/featureFlags';
+
+test.beforeEach(async ({ page }) => {
+ await skipUnlessFeatureEnabled(page, 'SOFT_DELETE');
+});
+
+interface TypeConfig {
+ key: string;
+ label: string;
+ create: (page: Page, name: string) => Promise<number>;
+ softDelete: (page: Page, id: number) => Promise<{ ok: () => boolean }>;
+ status: (page: Page, id: number) => Promise<number>;
+}
+
+async function anyDatasetId(page: Page): Promise<number> {
+ const res = await apiGet(page, 'api/v1/dataset/?q=(page_size:1)');
+ const body = await res.json();
+ return body.result[0].id;
+}
+
+const TYPES: TypeConfig[] = [
+ {
+ key: 'dashboard',
+ label: 'Dashboard',
+ create: async (page, name) =>
+ (await (await apiPostDashboard(page, { dashboard_title: name })).json())
+ .id,
Review Comment:
Fixed in `0ce35d8224` — both the dashboard and chart creators now go through
the existing `extractIdFromResponse` helper, which handles the
`result.id`/top-level `id` shapes and asserts a numeric id before the test
proceeds.
##########
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
+ * (the docker dev stack does). Each test creates a disposable object via the
+ * authenticated REST API, soft-deletes it, then drives the real UI to restore
+ * it and asserts — via the API — that it is live again.
+ */
+import { test, expect, Page } from '@playwright/test';
+import { apiGet, apiPost } from '../../helpers/api/requests';
+import {
+ apiPostChart,
+ apiGetChart,
+ apiDeleteChart,
+} from '../../helpers/api/chart';
+import {
+ apiPostDashboard,
+ apiGetDashboard,
+ apiDeleteDashboard,
+} from '../../helpers/api/dashboard';
+import {
+ createTestVirtualDataset,
+ apiGetDataset,
+ apiDeleteDataset,
+} from '../../helpers/api/dataset';
+import { skipUnlessFeatureEnabled } from '../../helpers/featureFlags';
+
+test.beforeEach(async ({ page }) => {
+ await skipUnlessFeatureEnabled(page, 'SOFT_DELETE');
+});
+
+interface TypeConfig {
+ key: string;
+ label: string;
+ create: (page: Page, name: string) => Promise<number>;
+ softDelete: (page: Page, id: number) => Promise<{ ok: () => boolean }>;
+ status: (page: Page, id: number) => Promise<number>;
+}
+
+async function anyDatasetId(page: Page): Promise<number> {
+ const res = await apiGet(page, 'api/v1/dataset/?q=(page_size:1)');
+ const body = await res.json();
+ return body.result[0].id;
+}
+
+const TYPES: TypeConfig[] = [
+ {
+ key: 'dashboard',
+ label: 'Dashboard',
+ create: async (page, name) =>
+ (await (await apiPostDashboard(page, { dashboard_title: name })).json())
+ .id,
+ softDelete: (page, id) => apiDeleteDashboard(page, id),
+ status: async (page, id) => (await apiGetDashboard(page, id)).status(),
+ },
+ {
+ key: 'chart',
+ label: 'Chart',
+ create: async (page, name) => {
+ const datasourceId = await anyDatasetId(page);
+ const res = await apiPostChart(page, {
+ slice_name: name,
+ datasource_id: datasourceId,
+ datasource_type: 'table',
+ viz_type: 'table',
+ });
+ return (await res.json()).id;
Review Comment:
Fixed in `0ce35d8224` together with the dashboard case — see the reply
there; both creators use `extractIdFromResponse` now.
##########
tests/integration_tests/dashboards/soft_delete_tests.py:
##########
@@ -730,3 +745,154 @@ def test_restore_via_import_with_slug_rename(self) ->
None:
finally:
_hard_delete_dashboard(original_id)
_hard_delete_dashboard(claimant_id)
+
+
+class TestDashboardArchiveListing(SupersetTestCase):
Review Comment:
Fixed in `0ce35d8224`: the three listing/restore tests in this class (and
their siblings in the dataset file — and the chart file, which had the
identical uncalled gap) now run under `@with_feature_flags(SOFT_DELETE=True)`,
so the non-owner restore assertion can no longer be satisfied by the flag-off
404 alone. All 9 pass flag-on.
##########
tests/integration_tests/datasets/soft_delete_tests.py:
##########
@@ -603,3 +613,173 @@ def
test_create_blocked_by_soft_deleted_logical_duplicate(self) -> None:
)
row.restore()
db.session.commit()
+
+
+class TestDatasetArchiveListing(SupersetTestCase):
Review Comment:
Fixed in `0ce35d8224` — see the reply on the dashboards file; the same
decorator was applied here and in the chart suite.
--
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]
