yousoph commented on code in PR #41181:
URL: https://github.com/apache/superset/pull/41181#discussion_r3434589130
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -615,6 +615,170 @@ test('onTabChange correctly updates selectedTab via
forceUpdate', () => {
});
});
+const ownerUser = {
+ userId: 1,
+ username: 'testuser',
+ firstName: 'Test',
+ lastName: 'User',
+ isActive: true,
+ isAnonymous: false,
+ permissions: {},
+ roles: { Alpha: [['can_write', 'Dashboard']] as [string, string][] },
+ groups: [],
+};
+
+const makeMetadataDashboard = (id: number, title: string) => ({
+ id,
+ dashboard_title: title,
+ owners: [{ id: 1, first_name: 'Test', last_name: 'User' }],
+ extra_owners: [],
+ roles: [],
+ url: `/superset/dashboard/${id}/`,
+ slug: null,
+ thumbnail_url: null,
+ published: true,
+ changed_by_name: 'Test User',
+ changed_by: { id: 1, first_name: 'Test', last_name: 'User' },
+ changed_on: '2024-01-01',
+ charts: [],
+});
+
+test('pre-populates dashboard from metadata.dashboards when dashboardId prop
is absent', async () => {
+ const dashboardId = 5;
+ const dashboardTitle = 'Chart Dashboard';
+
+ const myProps = {
+ ...defaultProps,
+ dashboardId: null,
+ metadata: {
+ dashboards: [{ id: dashboardId, dashboard_title: dashboardTitle }],
+ owners: ['Test User'],
+ created_on_humanized: '2 days ago',
+ changed_on_humanized: '1 day ago',
+ },
+ user: ownerUser,
+ slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] },
+ dispatch: jest.fn(),
+ addDangerToast: jest.fn(),
+ };
+
+ const component = new TestSaveModal(myProps);
+ const mockFull = makeMetadataDashboard(dashboardId, dashboardTitle);
+
+ component.loadDashboard = jest.fn().mockResolvedValue(mockFull);
+ component.loadTabs = jest.fn().mockResolvedValue([]);
+
+ const stateUpdates: any[] = [];
Review Comment:
Declining — line 602 in this same file already uses the identical `setState
= function (this: any, stateUpdate: any)` pattern. Adding specific types only
to the new test helper would be inconsistent.
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -615,6 +615,170 @@ test('onTabChange correctly updates selectedTab via
forceUpdate', () => {
});
});
+const ownerUser = {
+ userId: 1,
+ username: 'testuser',
+ firstName: 'Test',
+ lastName: 'User',
+ isActive: true,
+ isAnonymous: false,
+ permissions: {},
+ roles: { Alpha: [['can_write', 'Dashboard']] as [string, string][] },
+ groups: [],
+};
+
+const makeMetadataDashboard = (id: number, title: string) => ({
+ id,
+ dashboard_title: title,
+ owners: [{ id: 1, first_name: 'Test', last_name: 'User' }],
+ extra_owners: [],
+ roles: [],
+ url: `/superset/dashboard/${id}/`,
+ slug: null,
+ thumbnail_url: null,
+ published: true,
+ changed_by_name: 'Test User',
+ changed_by: { id: 1, first_name: 'Test', last_name: 'User' },
+ changed_on: '2024-01-01',
+ charts: [],
+});
+
+test('pre-populates dashboard from metadata.dashboards when dashboardId prop
is absent', async () => {
+ const dashboardId = 5;
+ const dashboardTitle = 'Chart Dashboard';
+
+ const myProps = {
+ ...defaultProps,
+ dashboardId: null,
+ metadata: {
+ dashboards: [{ id: dashboardId, dashboard_title: dashboardTitle }],
+ owners: ['Test User'],
+ created_on_humanized: '2 days ago',
+ changed_on_humanized: '1 day ago',
+ },
+ user: ownerUser,
+ slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] },
+ dispatch: jest.fn(),
+ addDangerToast: jest.fn(),
+ };
+
+ const component = new TestSaveModal(myProps);
+ const mockFull = makeMetadataDashboard(dashboardId, dashboardTitle);
+
+ component.loadDashboard = jest.fn().mockResolvedValue(mockFull);
+ component.loadTabs = jest.fn().mockResolvedValue([]);
+
+ const stateUpdates: any[] = [];
+ component.setState = jest.fn((update: any) => {
Review Comment:
Declining — same reason as the adjacent comment: line 602 uses the identical
`any` pattern for `setState` mocking. Consistent with existing style.
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -162,6 +163,34 @@ class SaveModal extends Component<SaveModalProps,
SaveModalState> {
t('An error occurred while loading dashboard information.'),
);
}
+ } else {
+ const metadataDashboards = this.props.metadata?.dashboards;
+ if (metadataDashboards?.length) {
+ // Fallback: the chart is already on one or more dashboards (from
Explore API
+ // metadata). Pre-populate with the first dashboard the user can edit
so the
+ // "Save & go to dashboard" button works out of the box.
+ try {
+ const loaded = await Promise.all(
+ metadataDashboards.map(({ id }) =>
+ this.loadDashboard(id).catch(() => null),
+ ),
+ );
Review Comment:
Valid concern — fixing in a follow-up commit. Will switch to a sequential
early-exit loop so we stop fetching once the first editable dashboard is found,
instead of firing N concurrent requests.
--
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]
