sadpandajoe commented on code in PR #45508:
URL: https://github.com/apache/superset/pull/45508#discussion_r4378399916
##########
superset-frontend/src/features/alerts/AlertReportModal.test.tsx:
##########
@@ -4662,3 +4665,429 @@ test('global attachment policy hides both the alert
toggle and attachment contro
);
}
});
+
+test('Changing the first Teams method preserves the following destination',
async () => {
+ mockNotificationMethods = ['Email', 'Teams'];
+ fetchMock.get('glob:*/api/v1/report/92', {
+ result: {
+ ...generateMockPayload(true),
+ id: 92,
+ report_format: 'NONE',
+ recipients: [
+ { id: 1, type: 'Teams', recipient_config_json: '{"target":"ops"}' },
+ { id: 2, type: 'Teams', recipient_config_json: '{"target":"finance"}'
},
+ ],
+ },
+ });
+ fetchMock.get(
+ 'glob:*/api/v1/report/teams_destinations/',
+ {
+ result: [
+ { key: 'ops', label: 'Operations', available: true },
+ { key: 'finance', label: 'Finance', available: false },
+ ],
+ },
+ { name: 'teams-destinations' },
+ );
+ fetchMock.put(
+ 'glob:*/api/v1/report/92',
+ { id: 92, result: {} },
+ { name: 'save-teams' },
+ );
+ try {
+ render(
+ <AlertReportModal
+ {...generateMockedProps(false, true)}
+ alert={{ ...validAlert, id: 92 }}
+ />,
+ {
+ useRedux: true,
+ initialState: { user: adminUser },
+ },
+ );
+ await waitFor(() =>
+ expect(screen.getByRole('button', { name: /save/i })).toBeEnabled(),
+ );
+ await userEvent.click(screen.getByTestId('notification-method-panel'));
+ expect(
+ await screen.findByText('Finance (Bot installation required)'),
+ ).toBeInTheDocument();
+ expect(
+ screen.getAllByRole('combobox', { name: 'Teams destination' }),
+ ).toHaveLength(2);
+ expect(
+ screen.getByText(/Add another notification method/i),
+ ).toBeInTheDocument();
+ await userEvent.click(screen.getByTestId('contents-panel'));
+ const attachmentToggle = screen.getByRole('switch', {
+ name: 'Include attachment',
+ });
+ await userEvent.click(attachmentToggle);
+ await waitFor(() =>
+ expect(screen.getByRole('button', { name: /save/i })).toBeDisabled(),
+ );
+ await userEvent.click(attachmentToggle);
+ await waitFor(() =>
+ expect(screen.getByRole('button', { name: /save/i })).toBeEnabled(),
+ );
+ await userEvent.click(screen.getByTestId('notification-method-panel'));
+ expect(
+ screen.getByText('Finance (Bot installation required)'),
+ ).toBeInTheDocument();
+ await userEvent.click(
+ screen.getAllByRole('combobox', { name: /^Delivery method/ })[0],
+ );
+ await userEvent.click(await screen.findByText('Email'));
+ await addAsyncSelectValue(
+ /email recipients/i,
+ '[email protected]',
+ reportUsersEndpoint,
+ );
+ await userEvent.click(screen.getByRole('button', { name: /save/i }));
+ await waitFor(() =>
+ expect(fetchMock.callHistory.calls('save-teams')).toHaveLength(1),
+ );
+ const [call] = fetchMock.callHistory.calls('save-teams');
+ expect(JSON.parse(call.options.body as string)).toMatchObject({
+ report_format: 'NONE',
+ recipients: [
+ {
+ type: 'Email',
+ recipient_config_json: {
+ target: '[email protected]',
+ ccTarget: '',
+ bccTarget: '',
+ },
+ },
+ { type: 'Teams', recipient_config_json: { target: 'finance' } },
+ ],
+ });
+ expect(JSON.parse(call.options.body as string).recipients).toEqual([
+ {
+ type: 'Email',
+ recipient_config_json: {
+ target: '[email protected]',
+ ccTarget: '',
+ bccTarget: '',
+ },
+ },
+ { type: 'Teams', recipient_config_json: { target: 'finance' } },
+ ]);
+ } finally {
+ fetchMock.removeRoute('teams-destinations');
+ fetchMock.removeRoute('save-teams');
+ }
+});
+
+test.each([
+ ['an active alert', true, false],
+ ['an inactive alert', false, false],
+ ['pausing an alert', true, true],
+])('Teams unavailable integration allows %s', async (_, active, pause) => {
+ const id = pause ? 193 : active ? 194 : 195;
+ mockNotificationMethods = ['Email', 'Slack'];
+ fetchMock.get(`glob:*/api/v1/report/${id}`, {
+ result: {
+ ...generateMockPayload(true),
+ id,
+ active,
+ report_format: 'NONE',
+ recipients: [
+ {
+ id: 1,
+ type: 'Teams',
+ recipient_config_json: '{"target":"removed-key"}',
+ },
+ { id: 2, type: 'Slack', recipient_config_json: { target: 'C123' } },
+ ],
+ },
+ });
+ fetchMock.get(
+ 'glob:*/api/v1/report/teams_destinations/',
+ { result: [] },
+ { name: 'teams-unavailable' },
+ );
+ fetchMock.put(
+ `glob:*/api/v1/report/${id}`,
+ { id, result: {} },
+ {
+ name: 'teams-unavailable-save',
+ },
+ );
+ try {
+ render(
+ <AlertReportModal
+ {...generateMockedProps(false, true)}
+ alert={{ ...validAlert, id }}
+ />,
+ { useRedux: true, initialState: { user: adminUser } },
+ );
+ await userEvent.click(screen.getByTestId('notification-method-panel'));
+ expect(
+ await screen.findByText('removed-key (Unavailable)'),
+ ).toBeInTheDocument();
+ expect(
+ await screen.findByText(/Teams delivery is unavailable/),
+ ).toBeInTheDocument();
+ await userEvent.click(screen.getByTestId('general-information-panel'));
+ const description = screen.getByPlaceholderText(
+ /include description to be sent with alert/i,
+ );
+ await userEvent.clear(description);
+ await userEvent.type(description, 'Updated description');
+ if (pause) await userEvent.click(screen.getByRole('switch'));
+ const save = screen.getByRole('button', { name: /save/i });
+ await waitFor(() => expect(save).toBeEnabled());
+ await userEvent.click(screen.getByTestId('notification-method-panel'));
+ const slackTarget = screen.getByTestId('recipients');
+ fireEvent.change(slackTarget, { target: { value: 'C456' } });
+ await waitFor(() => expect(save).toBeDisabled());
+ fireEvent.change(slackTarget, { target: { value: 'C123' } });
+ await waitFor(() => expect(save).toBeEnabled());
+ await userEvent.click(save);
+ await waitFor(() =>
+ expect(
+ fetchMock.callHistory.calls('teams-unavailable-save'),
+ ).toHaveLength(1),
+ );
+ const [call] = fetchMock.callHistory.calls('teams-unavailable-save');
+ expect(JSON.parse(call.options.body as string)).toMatchObject({
+ active: pause ? false : active,
+ description: 'Updated description',
+ report_format: 'NONE',
+ recipients: [
+ { type: 'Teams', recipient_config_json: { target: 'removed-key' } },
+ {
+ type: 'Slack',
+ recipient_config_json: {
+ target: 'C123',
+ ccTarget: '',
+ bccTarget: '',
+ },
+ },
+ ],
+ });
+ } finally {
+ fetchMock.removeRoute(`glob:*/api/v1/report/${id}`);
+ fetchMock.removeRoute('teams-unavailable');
+ fetchMock.removeRoute('teams-unavailable-save');
+ }
+});
+
+test('Teams unavailable saved keys can be replaced without losing inactive
state', async () => {
+ mockNotificationMethods = ['Email', 'Teams'];
+ fetchMock.get('glob:*/api/v1/report/94', {
+ result: {
+ ...generateMockPayload(true),
+ id: 94,
+ active: false,
+ report_format: 'NONE',
+ recipients: [
+ {
+ id: 1,
+ type: 'Teams',
+ recipient_config_json: { target: 'removed-key' },
+ },
+ ],
+ },
+ });
+ fetchMock.get(
+ 'glob:*/api/v1/report/teams_destinations/',
+ {
+ result: [{ key: 'ops', label: 'Operations', available: false }],
+ },
+ { name: 'teams-replacement' },
+ );
+ fetchMock.put(
+ 'glob:*/api/v1/report/94',
+ { id: 94, result: {} },
+ { name: 'teams-replacement-save' },
+ );
+ try {
+ render(
+ <AlertReportModal
+ {...generateMockedProps(false, true)}
+ alert={{ ...validAlert, id: 94 }}
+ />,
+ {
+ useRedux: true,
+ initialState: { user: adminUser },
+ },
+ );
+ await userEvent.click(screen.getByTestId('notification-method-panel'));
+ expect(
+ await screen.findByText('removed-key (Unavailable)'),
+ ).toBeInTheDocument();
+ await userEvent.click(
+ screen.getByRole('combobox', { name: 'Teams destination' }),
+ );
+ await userEvent.click(
+ await screen.findByText('Operations (Bot installation required)'),
+ );
+ expect(
+ screen.queryByText(/This saved Teams destination is unavailable/),
+ ).not.toBeInTheDocument();
+ await userEvent.click(screen.getByRole('button', { name: /save/i }));
+ await waitFor(() =>
+ expect(
+ fetchMock.callHistory.calls('teams-replacement-save'),
+ ).toHaveLength(1),
+ );
+ const [call] = fetchMock.callHistory.calls('teams-replacement-save');
+ const payload = JSON.parse(call.options.body as string);
+ expect(payload.active).toBe(false);
+ expect(payload.report_format).toBe('NONE');
+ expect(payload.recipients).toEqual([
+ { type: 'Teams', recipient_config_json: { target: 'ops' } },
+ ]);
+ } finally {
+ fetchMock.removeRoute('teams-replacement');
+ fetchMock.removeRoute('teams-replacement-save');
+ }
+});
+
+test('Teams saved recipients stay visible and replaceable when the optional
SDK is unavailable', async () => {
+ mockNotificationMethods = ['Email'];
+ fetchMock.get('glob:*/api/v1/report/95', {
+ result: {
+ ...generateMockPayload(true),
+ id: 95,
+ report_format: 'NONE',
+ recipients: [
+ { id: 1, type: 'Teams', recipient_config_json: '{"target":"ops"}' },
+ ],
+ },
+ });
+ fetchMock.get(
+ 'glob:*/api/v1/report/teams_destinations/',
+ { result: [] },
+ { name: 'teams-sdk-missing' },
+ );
+ try {
+ render(
+ <AlertReportModal
+ {...generateMockedProps(false, true)}
+ alert={{ ...validAlert, id: 95 }}
+ />,
+ {
+ useRedux: true,
+ initialState: { user: adminUser },
+ },
+ );
+ await userEvent.click(screen.getByTestId('notification-method-panel'));
+ expect(await screen.findByText('ops (Unavailable)')).toBeInTheDocument();
+ expect(
+ screen.getByText(
+ /Teams delivery is unavailable.*optional Teams dependency/,
+ ),
+ ).toBeInTheDocument();
+ expect(
+ screen.getByRole('combobox', { name: 'Teams destination' }),
+ ).toBeDisabled();
+ const save = screen.getByRole('button', { name: /save/i });
+ await waitFor(() => expect(save).toBeEnabled());
+ await userEvent.click(screen.getByTestId('general-information-panel'));
+ await userEvent.click(screen.getByRole('switch'));
+ await waitFor(() => expect(save).toBeDisabled());
+ await userEvent.click(screen.getByRole('switch'));
+ await waitFor(() => expect(save).toBeEnabled());
+ await userEvent.click(screen.getByTestId('notification-method-panel'));
+ await userEvent.click(
+ screen.getByRole('combobox', { name: /^Delivery method/ }),
+ );
+ await userEvent.click(await screen.findByText('Email'));
+ expect(
+ screen.queryByRole('combobox', { name: 'Teams destination' }),
+ ).not.toBeInTheDocument();
+ expect(screen.queryByText('ops (Unavailable)')).not.toBeInTheDocument();
+ } finally {
+ fetchMock.removeRoute('teams-sdk-missing');
+ }
+});
+
+test.each(['PNG', 'CSV'])(
+ 'Teams edits normalize saved %s format when attachments are globally
disabled',
+ async reportFormat => {
+ mockNotificationMethods = ['Email', 'Teams'];
+ fetchMock.removeRoute(configurationEndpoint);
+ fetchMock.get(
+ configurationEndpoint,
+ {
+ result: { ...mockReportConfiguration, alerts_attach_reports: false },
+ },
+ { name: configurationEndpoint },
+ );
+ fetchMock.get('glob:*/api/v1/report/96', {
Review Comment:
Both parameterized cases register `GET glob:*/api/v1/report/96` without a
name, and nothing removes it: the `finally` only removes the Teams routes and
the `afterEach` list doesn't include it. fetch-mock serves the first matching
route, so the CSV case loads the PNG payload from the PNG case, and a
regression that normalizes a saved PNG format but leaves CSV unchanged would
still pass both. Could each case use a distinct report id or a named route
removed in `finally`, so the CSV case really loads `report_format: 'CSV'`?
##########
.github/workflows/superset-python-unittest.yml:
##########
@@ -65,6 +65,10 @@ jobs:
uses: $/.github/actions/setup-backend/
with:
python-version: ${{ matrix.python-version }}
+ - name: Install optional Teams SDK for authentication and delivery tests
+ run: |
+ uv pip install --system -e '.[teams]'
Review Comment:
This installs `.[teams]` without the `requirements/development.txt`
constraint, and `teams` isn't in `requirements/development.in`.
`microsoft-teams-apps==2.1.1` requires `opentelemetry-api>=1.41.0` and
`pydantic-settings>=2.11.0`, while the lock pins `1.39.1` and `2.10.1`, and
`fastapi`/`msal` aren't locked at all. So this step upgrades locked packages
and resolves new ones at whatever is latest before the whole unit suite runs,
which means CI passes or fails against an environment no locked install uses,
and `check-python-deps` can't see the drift. Would adding `teams` to
`development.in` and regenerating the lock (instead of this ad hoc install)
work here?
--
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]
