Copilot commented on code in PR #37497:
URL: https://github.com/apache/superset/pull/37497#discussion_r2737931766
##########
superset-frontend/src/dashboard/actions/dashboardState.test.js:
##########
@@ -234,4 +236,61 @@ describe('dashboardState actions', () => {
);
});
});
+
+ // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
+ describe('fetchFaveStar', () => {
+ test('dispatches TOGGLE_FAVE_STAR on successful fetch', async () => {
+ const dashboardId = 123;
+ const dispatch = sinon.stub();
+
+ getStub.restore();
+ getStub = sinon.stub(SupersetClient, 'get').resolves({
+ json: {
+ result: [{ id: dashboardId, value: true }],
+ },
+ });
+
+ const thunk = fetchFaveStar(dashboardId);
+ await thunk(dispatch);
+
+ await waitFor(() => expect(dispatch.callCount).toBe(1));
+ expect(dispatch.getCall(0).args[0].type).toBe(TOGGLE_FAVE_STAR);
+ expect(dispatch.getCall(0).args[0].isStarred).toBe(true);
+ });
+
+ test('does not dispatch error toast on 404 response', async () => {
+ const dashboardId = 999;
+ const dispatch = sinon.stub();
+
+ getStub.restore();
+ const error404 = new Error('Not found');
+ error404.status = 404;
+ getStub = sinon.stub(SupersetClient, 'get').rejects(error404);
+
+ const thunk = fetchFaveStar(dashboardId);
+ await thunk(dispatch);
+
+ // Should not dispatch any action (no toast, no toggle)
+ expect(dispatch.callCount).toBe(0);
+ });
+
+ test('dispatches error toast on non-404 errors', async () => {
+ const dashboardId = 123;
+ const dispatch = sinon.stub();
+
+ getStub.restore();
+ const error500 = new Error('Internal server error');
+ error500.status = 500;
+ getStub = sinon.stub(SupersetClient, 'get').rejects(error500);
Review Comment:
Same mocking issue as the 404 test: rejecting with a plain `Error` (even
with `status = 500`) won’t reliably propagate `status` through
`getClientErrorObject`. To ensure this test is validating the intended non-404
branch, reject with a Response-like object that includes `status`/`statusText`
(or mock `getClientErrorObject`).
##########
superset-frontend/src/dashboard/components/Header/index.jsx:
##########
@@ -592,7 +592,12 @@ const Header = () => {
const faveStarProps = useMemo(
() => ({
itemId: dashboardInfo.id,
- fetchFaveStar: boundActionCreators.fetchFaveStar,
+ // Only fetch favorite status for valid, existing dashboards
+ // Skip fetch entirely if dashboard ID is falsy to prevent
+ // 404 errors when navigating after dashboard deletion
Review Comment:
The inline comment suggests a falsy dashboard ID would lead to 404s, but a
404 implies a *truthy* ID that no longer exists. Consider rewording this
comment to reflect the actual scenario (skip fetch when there is no ID /
component is unmounted), and rely on the 404 handling in `fetchFaveStar` for
deleted/stale IDs.
```suggestion
// Only attempt to fetch favorite status when there's a valid
dashboard ID.
// When the ID is falsy (e.g., dashboard not yet created or component
unmounted),
// skip the fetch and rely on fetchFaveStar to handle 404s for
deleted/stale IDs.
```
##########
superset-frontend/src/dashboard/actions/dashboardState.test.js:
##########
@@ -234,4 +236,61 @@ describe('dashboardState actions', () => {
);
});
});
+
+ // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
+ describe('fetchFaveStar', () => {
+ test('dispatches TOGGLE_FAVE_STAR on successful fetch', async () => {
+ const dashboardId = 123;
+ const dispatch = sinon.stub();
+
+ getStub.restore();
+ getStub = sinon.stub(SupersetClient, 'get').resolves({
+ json: {
+ result: [{ id: dashboardId, value: true }],
+ },
+ });
+
+ const thunk = fetchFaveStar(dashboardId);
+ await thunk(dispatch);
+
+ await waitFor(() => expect(dispatch.callCount).toBe(1));
+ expect(dispatch.getCall(0).args[0].type).toBe(TOGGLE_FAVE_STAR);
+ expect(dispatch.getCall(0).args[0].isStarred).toBe(true);
+ });
+
+ test('does not dispatch error toast on 404 response', async () => {
+ const dashboardId = 999;
+ const dispatch = sinon.stub();
+
+ getStub.restore();
+ const error404 = new Error('Not found');
+ error404.status = 404;
+ getStub = sinon.stub(SupersetClient, 'get').rejects(error404);
Review Comment:
The mocked 404 error here (`new Error()` with a `status` field) will not be
interpreted as a 404 by `getClientErrorObject` (it only carries `status`
through when the rejection is a Response-like object, e.g. `{ response: {
status, statusText, bodyUsed } }`). As a result, this test won’t actually
exercise the 404-suppression path and will likely dispatch the danger toast
instead. Update the mock rejection to match the shape
SupersetClient/getClientErrorObject expects (or mock `getClientErrorObject`
directly).
```suggestion
getStub = sinon
.stub(SupersetClient, 'get')
.rejects({
response: {
status: 404,
statusText: 'Not Found',
bodyUsed: false,
},
});
```
--
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]
