sadpandajoe commented on code in PR #45589:
URL: https://github.com/apache/superset/pull/45589#discussion_r4365981426
##########
superset-frontend/src/features/home/ActivityTable.tsx:
##########
@@ -155,7 +157,7 @@ export default function ActivityTable({
isMounted = false;
setIsFetchingEditedCards(false);
};
- }, [activeChild, user.userId]);
+ }, [activeChild, user.userId, refreshKey]);
Review Comment:
`refreshKey` here is what clears a deleted item from the Edited tab, but
nothing exercises it: the `ActivityTable` tests only enter Edited, and
`Home.refresh.test.tsx` never selects it. Dropping `refreshKey` from this
dependency list (or the prop from Home) would leave the deleted dashboard
listed and all tests green. Could you add a Home-level test that selects
Edited, triggers the refresh with a changed edited-objects response, and
asserts the removed card disappears?
##########
superset-frontend/src/features/home/ChartTable.tsx:
##########
@@ -232,10 +242,10 @@ function ChartTable({
hasPerm={hasPerm}
showThumbnails={showThumbnails}
bulkSelectEnabled={bulkSelectEnabled}
- refreshData={refreshData}
+ refreshData={handleRefreshData}
Review Comment:
The Mine tab's delete goes through this `refreshData` wrapper, but the only
new `ChartTable` assertion runs on the All/Other tab, which takes the `getData`
path. If this prop were reverted to the raw `refreshData`, deleting from Mine
would stop refreshing Recents and every test would still pass. Could you add a
Mine-tab delete test that asserts `onActivityRefresh` is not called before the
DELETE resolves and is called once after?
##########
superset-frontend/src/pages/Home/Home.refresh.test.tsx:
##########
@@ -0,0 +1,178 @@
+/**
+ * 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.
+ */
+// Imported first so the antd mock is defined before anything requires 'antd'.
+import { mockAntdWithDesktopBreakpoint } from 'spec/helpers/mobileTestUtils';
+import fetchMock from 'fetch-mock';
+import { SupersetClient } from '@superset-ui/core';
+import {
+ render,
+ screen,
+ userEvent,
+ waitFor,
+} from 'spec/helpers/testing-library';
+import Welcome from 'src/pages/Home';
+import { LocalStorageKeys, setItem } from 'src/utils/localStorageHelpers';
+
+const chartsEndpoint = 'glob:*/api/v1/chart/?*';
+const dashboardsEndpoint = 'glob:*/api/v1/dashboard/?*';
+const savedQueryEndpoint = 'glob:*/api/v1/saved_query/?*';
+const recentActivityEndpoint = 'glob:*/api/v1/log/recent_activity/*';
+
+jest.mock('antd', () => mockAntdWithDesktopBreakpoint());
+
+// The delete flow lives in the tables; Home only receives `onActivityRefresh`.
+// Stub the tables with a button that simulates a completed delete.
+jest.mock('src/features/home/DashboardTable', () => ({
+ __esModule: true,
+ default: ({ onActivityRefresh }: { onActivityRefresh?: () => void }) => (
+ <button type="button" onClick={onActivityRefresh}>
+ simulate delete
+ </button>
+ ),
+}));
+jest.mock('src/features/home/ChartTable', () => ({
+ __esModule: true,
+ default: () => <div />,
+}));
+
+const user = {
+ username: 'alpha',
+ firstName: 'alpha',
+ lastName: 'alpha',
+ createdOn: '2016-11-11T12:34:17',
+ userId: 5,
+ email: '[email protected]',
+ isActive: true,
+ isAnonymous: false,
+ permissions: {},
+ roles: { sql_lab: [['can_read', 'SavedQuery']] },
+};
+
+const recentItem = (title: string, id: number) => ({
+ action: 'dashboard',
+ item_title: title,
+ item_type: 'dashboard',
+ item_url: `/superset/dashboard/${id}/`,
+ time: 1741644942130 + id,
+ time_delta_humanized: 'a day ago',
+});
+
+let recentResponder: () => unknown;
+
+beforeAll(() => {
+ SupersetClient.configure().init();
+});
+
+beforeEach(() => {
+ localStorage.clear();
+ // Start with the Recents and Dashboards panels expanded.
+ setItem(LocalStorageKeys.HomepageCollapseState, ['recents', 'dashboards']);
+ fetchMock.get(chartsEndpoint, { result: [] });
+ fetchMock.get(dashboardsEndpoint, { result: [] });
+ fetchMock.get(savedQueryEndpoint, { result: [] });
+ fetchMock.get(recentActivityEndpoint, () => recentResponder());
+});
+
+afterEach(() => {
+ fetchMock.removeRoutes();
+ fetchMock.clearHistory();
+});
+
+const renderHome = async () => {
+ render(<Welcome user={user} />, { useRedux: true, useRouter: true });
+ return screen.findByRole('button', { name: 'simulate delete' });
+};
+
+test('delete refetches recents without resetting the selected tab', async ()
=> {
+ recentResponder = () => ({ result: [recentItem('Viewed dash', 1)] });
+ const deleteButton = await renderHome();
+ expect(await screen.findByText('Viewed dash')).toBeInTheDocument();
+ expect(fetchMock.callHistory.calls(recentActivityEndpoint)).toHaveLength(1);
+ const dashboardCallsBefore =
+ fetchMock.callHistory.calls(dashboardsEndpoint).length;
+
+ // Switch away from the default (Viewed) tab; a refresh must not undo this.
+ await userEvent.click(screen.getByText('Created'));
+ await waitFor(() =>
+ expect(screen.queryByText('Viewed dash')).not.toBeInTheDocument(),
+ );
+
+ // Drop the persisted tab so only the in-memory selection survives; a
+ // refresh that re-ran the first-load tab restore would flip back to Viewed.
+ localStorage.removeItem(LocalStorageKeys.HomepageActivityFilter);
+
+ recentResponder = () => ({ result: [recentItem('Fresh dash', 2)] });
+ await userEvent.click(deleteButton);
+
+ await waitFor(() =>
+
expect(fetchMock.callHistory.calls(recentActivityEndpoint)).toHaveLength(2),
+ );
+ await waitFor(() =>
+ expect(
+ fetchMock.callHistory.calls(dashboardsEndpoint).length,
+ ).toBeGreaterThan(dashboardCallsBefore),
+ );
+ // Still on the Created tab: neither the old nor the refreshed Viewed items
+ // are shown, and the collapse panels were not reset.
Review Comment:
This can't detect a collapse reset: the panels start as `['recents',
'dashboards']`, which is exactly what a re-run of the first-load restore would
produce, and the assertion only checks that the stub button is still rendered.
Removing the `refreshKey === 0` guard around `setActiveState` would still pass.
Could the test collapse Recents (keeping Dashboards open), trigger the refresh,
and assert Recents is still collapsed afterwards?
--
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]
