sadpandajoe commented on code in PR #45142:
URL: https://github.com/apache/superset/pull/45142#discussion_r4258666302
##########
superset-frontend/src/features/alerts/AlertReportModal.test.tsx:
##########
@@ -2221,6 +2226,532 @@ test('filter reappears in dropdown after clearing with
X icon', async () => {
);
}, 45000);
+test.each(['older', 'latest'] as const)(
+ 'overlapping filter requests preserve the latest selection when %s finishes
first',
+ async firstToFinish => {
+ fetchMock.removeRoute(tabsEndpoint);
+ fetchMock.get(tabsEndpoint, tabsWithFilters, { name: tabsEndpoint });
+
+ type Response = {
+ json: { result: { data: { country: string }[] }[] };
+ };
+ let resolveOlder!: (response: Response) => void;
+ let resolveLatest!: (response: Response) => void;
+ const olderRequest = new Promise<Response>(resolve => {
+ resolveOlder = resolve;
+ });
+ const latestRequest = new Promise<Response>(resolve => {
+ resolveLatest = resolve;
+ });
+ mockGetChartDataRequest
+ .mockReturnValueOnce(olderRequest)
+ .mockReturnValueOnce(latestRequest);
+
+ render(<AlertReportModal {...generateMockedProps(true, true)} />, {
+ useRedux: true,
+ });
+ await userEvent.click(screen.getByTestId('contents-panel'));
+ await screen.findByText(/test dashboard/i);
+
+ const filterSelect = screen.getByRole('combobox', {
+ name: /select filter/i,
+ });
+ const valueSelect = screen.getByRole('combobox', {
+ name: /select value/i,
+ });
+ const selectCountry = async () => {
+ fireEvent.mouseDown(filterSelect);
+ await userEvent.click(await screen.findByText('Country Filter'));
+ };
+ await waitFor(() => expect(filterSelect).toBeEnabled());
+ await selectCountry();
+ await waitFor(() =>
+ expect(mockGetChartDataRequest).toHaveBeenCalledTimes(1),
+ );
+ expect(valueSelect).toBeDisabled();
+
+ const clearIcon = filterSelect
+ .closest('.ant-select')!
+ .querySelector('.ant-select-clear [aria-label="close-circle"]')!;
+ await userEvent.click(clearIcon);
+ await selectCountry();
+ await waitFor(() =>
+ expect(mockGetChartDataRequest).toHaveBeenCalledTimes(2),
+ );
+
+ const olderResponse: Response = {
+ json: { result: [{ data: [{ country: 'Stale country' }] }] },
+ };
+ const latestResponse: Response = {
+ json: { result: [{ data: [{ country: 'Latest country' }] }] },
+ };
+ if (firstToFinish === 'older') {
+ await act(async () => {
+ resolveOlder(olderResponse);
+ await olderRequest;
+ });
+ expect(valueSelect).toBeDisabled();
+ expect(
+
valueSelect.closest('.ant-select')!.querySelector('[aria-busy="true"]'),
+ ).toBeInTheDocument();
+ }
+
+ await act(async () => {
+ resolveLatest(latestResponse);
+ await latestRequest;
+ });
+ await waitFor(() => expect(valueSelect).toBeEnabled());
+
+ if (firstToFinish === 'latest') {
+ await act(async () => {
+ resolveOlder(olderResponse);
+ await olderRequest;
+ });
+ }
+
+ fireEvent.mouseDown(valueSelect);
+ expect(await screen.findByText('Latest country')).toBeInTheDocument();
+ expect(screen.queryByText('Stale country')).not.toBeInTheDocument();
+ expect(
+ valueSelect.closest('.ant-select')!.querySelector('[aria-busy="true"]'),
+ ).not.toBeInTheDocument();
+ },
+);
+
+test('an older filter request rejection does not replace the latest options
with an error', async () => {
+ fetchMock.removeRoute(tabsEndpoint);
+ fetchMock.get(tabsEndpoint, tabsWithFilters, { name: tabsEndpoint });
+
+ type Response = {
+ json: { result: { data: { country: string }[] }[] };
+ };
+ let rejectOlder!: (error: Error) => void;
+ let resolveLatest!: (response: Response) => void;
+ const olderRequest = new Promise<Response>((_, reject) => {
+ rejectOlder = reject;
+ });
+ const latestRequest = new Promise<Response>(resolve => {
+ resolveLatest = resolve;
+ });
+ mockGetChartDataRequest
+ .mockReturnValueOnce(olderRequest)
+ .mockReturnValueOnce(latestRequest);
+
+ render(<AlertReportModal {...generateMockedProps(true, true)} />, {
+ useRedux: true,
+ });
+ await userEvent.click(screen.getByTestId('contents-panel'));
+ await screen.findByText(/test dashboard/i);
+
+ const filterSelect = screen.getByRole('combobox', {
+ name: /select filter/i,
+ });
+ const valueSelect = screen.getByRole('combobox', {
+ name: /select value/i,
+ });
+ const selectCountry = async () => {
+ fireEvent.mouseDown(filterSelect);
+ await userEvent.click(await screen.findByText('Country Filter'));
+ };
+ await waitFor(() => expect(filterSelect).toBeEnabled());
+ await selectCountry();
+ await waitFor(() =>
expect(mockGetChartDataRequest).toHaveBeenCalledTimes(1));
+
+ const clearIcon = filterSelect
+ .closest('.ant-select')!
+ .querySelector('.ant-select-clear [aria-label="close-circle"]')!;
+ await userEvent.click(clearIcon);
+ await selectCountry();
+ await waitFor(() =>
expect(mockGetChartDataRequest).toHaveBeenCalledTimes(2));
+
+ await act(async () => {
+ resolveLatest({
+ json: { result: [{ data: [{ country: 'Latest country' }] }] },
+ });
+ await latestRequest;
+ });
+ await waitFor(() => expect(valueSelect).toBeEnabled());
+
+ await act(async () => {
+ rejectOlder(new Error('Older request failed'));
Review Comment:
This only covers an older rejection that arrives after the latest request
has already succeeded, and it uses a plain `Error`, which
`getClientErrorObject` normalizes with no real delay. Moving the
`isLatestRequest()` check in `handleError` ahead of its `await` would still
pass here. The failing case is an older request that rejects while still
current, with its error body still being parsed, followed by clear/reselect and
a successful latest request. Could a test reject the older request with a
`Response` whose body read is deferred, finish the latest request, then release
the older body and assert the latest options stay selectable with no error
shown?
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -1924,6 +1979,53 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
</div>
);
}
+ const filterError = nativeFilterErrors[filter.nativeFilterId ?? ''];
+ if (filterError) {
+ const { error, isNetworkError } = filterError;
+ return (
+ <ErrorMessageWithStackTrace
Review Comment:
If the options request fails with an `OAUTH2_REDIRECT` error, this renders
the registered OAuth2 message with its "provide authorization" link. That
component only retries after the authorization tab reports back when it gets an
`errorMitigationFunction` or a known `source`, and neither is passed here. So
the user authorizes successfully and the value picker stays replaced by the
error until they clear and reselect the filter. Could this pass a callback that
re-runs the options request for this filter, so authorization recovers in place?
--
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]
