gabotorresruiz commented on code in PR #44031:
URL: https://github.com/apache/superset/pull/44031#discussion_r3982193412
##########
superset-frontend/src/utils/downloadAsPivotExcel.ts:
##########
@@ -64,12 +65,20 @@ function restoreUnambiguousNumbers(sheet: WorkSheet): void {
export default function exportPivotExcel(
tableSelector: string,
fileName: string,
+ // Bound via `useToasts()`/`bindActionCreators`, not the raw action
+ // creator from `actions.ts`: this module has no dispatch of its own, so an
+ // unbound creator would only build a Redux action object and never render
+ // a toast.
+ addWarningToast?: (text: string) => void,
) {
const table = document.querySelector(tableSelector);
if (!table) {
logging.error(
`[exportPivotExcel] No element found for selector: "${tableSelector}"`,
);
+ addWarningToast?.(
Review Comment:
Not a blocker, and if anything it strengthens the case for the approach you
landed here: I went to check the convention in the sibling utilities and found
that their warning toasts never actually render. `downloadAsPdf.ts` imports the
raw action creator and returns the action object from its event handler (line
59), which every caller discards, and `downloadAsImage.tsx` calls the raw
creator and drops the result in four places (391, 436, 536, 617). The creators
in `MessageToasts/actions.ts` are plain object factories, so without a dispatch
nothing reaches the toast reducer, which is exactly what the first round of
this PR hit.
So this file is now the only one of the three whose missing-element toast
will actually show. Worth a follow-up applying the same bound-callback
treatment to `downloadAsPdf`/`downloadAsImage`, with the same kind of test you
added here? Happy to file it or take it if you'd rather keep this PR scoped.
##########
superset-frontend/src/utils/downloadAsPivotExcel.test.ts:
##########
@@ -124,12 +128,27 @@ test('leaves date-shaped strings as text rather than
reinterpreting them as date
expect(sheet.C1).toMatchObject({ t: 's', v: 'not-a-date' });
});
-test('should log an error and return early when table element is not found',
() => {
+test('logs an error, warns the user via the bound toast callback, and returns
early when table element is not found', () => {
jest.spyOn(document, 'querySelector').mockReturnValue(null);
+ const addWarningToast = jest.fn();
- exportPivotExcel('.non-existent-selector', 'test-file');
+ exportPivotExcel('.non-existent-selector', 'test-file', addWarningToast);
expect(logging.error as jest.Mock).toHaveBeenCalledWith(
'[exportPivotExcel] No element found for selector:
".non-existent-selector"',
);
+ // Passed in already bound to dispatch (e.g. via `useToasts()`), so calling
+ // it directly is what actually renders the toast -- unlike the raw action
+ // creator, which only builds a Redux action object.
+ expect(addWarningToast).toHaveBeenCalledWith(
Review Comment:
Small heads-up on the open bot suggestion to add
`expect(mockWriteFile).not.toHaveBeenCalled()` here: I tried it verbatim on
this branch and it fails, because `mockWriteFile` still holds the calls from
the four table-export tests above (the file's `afterEach` only restores spies,
and there's no global `clearMocks`). If you want the early-return assertion, it
needs a `mockWriteFile.mockClear()` at the top of this test first. Fine to skip
it entirely too, the `not.toThrow` test below plus this one already pin the
branch.
--
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]
