I3eka commented on code in PR #44456:
URL: https://github.com/apache/superset/pull/44456#discussion_r4079630849
##########
superset-frontend/src/pages/Home/Home.test.tsx:
##########
@@ -165,11 +171,35 @@ const renderWelcome = (props = mockedProps) =>
afterEach(() => {
fetchMock.clearHistory();
+ jest.mocked(redirect).mockClear();
});
+test.each([
+ ['anonymous', { roles: { Public: [] }, permissions: {}, groups: [] }],
+ ['guest', { ...mockedProps.user, userId: undefined }],
+ ['missing', undefined],
+])(
+ 'Redirects the %s user through the server without fetching Home data',
+ (_, user) => {
+ render(<Welcome user={user} />, { useRedux: true, useRouter: true });
+
+ expect(redirect).toHaveBeenCalledWith('/welcome/');
+ [
+ chartsEndpoint,
+ dashboardsEndpoint,
+ recentActivityEndpoint,
+ savedQueryEndpoint,
+ ].forEach(endpoint => {
+ expect(fetchMock.callHistory.calls(endpoint)).toHaveLength(0);
Review Comment:
The assertion now explicitly awaits the redirect with waitFor, then checks
that no Home requests or content were produced (319ba7886f). I could not
reproduce a scheduling flake: this repository's RTL render is already
act-wrapped, so the previous synchronous useEffect assertion was not evidence
of one. The change makes the expected effect boundary explicit; 220 tests
across 14 Home/navigation suites pass.
##########
superset-frontend/src/pages/Home/index.tsx:
##########
@@ -448,4 +450,20 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
);
}
-export default withToasts(Welcome);
+function WelcomePage({
+ user,
+ ...props
+}: Omit<WelcomeProps, 'user'> & { user?: UserWithPermissionsAndRoles }) {
+ const hasUserId = Boolean(user?.userId);
Review Comment:
Confirmed with a regression: a typed user with userId: 0 was redirected by
the truthiness check, while the normal ID and missing-ID controls behaved as
expected. Fixed in 319ba7886f with user?.userId != null. The zero-ID test fails
before and passes after; no assumption that production-generated IDs normally
start at zero is needed.
##########
superset-frontend/src/pages/Home/Home.test.tsx:
##########
@@ -165,11 +171,35 @@ const renderWelcome = (props = mockedProps) =>
afterEach(() => {
fetchMock.clearHistory();
+ jest.mocked(redirect).mockClear();
});
+test.each([
+ ['anonymous', { roles: { Public: [] }, permissions: {}, groups: [] }],
+ ['guest', { ...mockedProps.user, userId: undefined }],
+ ['missing', undefined],
+])(
+ 'Redirects the %s user through the server without fetching Home data',
+ (_, user) => {
+ render(<Welcome user={user} />, { useRedux: true, useRouter: true });
+
+ expect(redirect).toHaveBeenCalledWith('/welcome/');
Review Comment:
Changed the Home assertion to RoutePaths.HOME in 319ba7886f. I also checked
the concern behind retaining a literal in my earlier reply: routes.test.tsx
independently asserts /welcome/, and navigationUtils tests cover
application-root prefixing, so that contract is still tested separately. All 14
Home/navigation suites pass.
##########
superset-frontend/src/pages/Home/index.tsx:
##########
@@ -448,4 +450,20 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
);
}
-export default withToasts(Welcome);
+function WelcomePage({
+ user,
+ ...props
+}: Omit<WelcomeProps, 'user'> & { user?: UserWithPermissionsAndRoles }) {
+ const hasUserId = Boolean(user?.userId);
Review Comment:
Both the redirect effect and render now use the same explicit hasUserId
predicate (319ba7886f). TypeScript 5.4.5 correctly narrows the optional user
through this const condition; the full PR-file type/lint hooks pass without
assertions or suppressions. Zero-ID and missing-ID regressions cover both
branches.
##########
superset-frontend/src/pages/Home/index.tsx:
##########
@@ -448,4 +450,20 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
);
}
-export default withToasts(Welcome);
+function WelcomePage({
+ user,
+ ...props
+}: Omit<WelcomeProps, 'user'> & { user?: UserWithPermissionsAndRoles }) {
+ const hasUserId = Boolean(user?.userId);
+
+ useEffect(() => {
+ if (!hasUserId) {
+ // SPA navigation can bypass the welcome view's server-side login check.
+ redirect(RoutePaths.HOME);
+ }
+ }, [hasUserId]);
+
+ return user?.userId ? <Welcome {...props} user={user} /> : null;
Review Comment:
Addressed with the same shared predicate in 319ba7886f; there is no second
truthiness check in the render path. The zero-ID regression and three
missing-ID cases pass, along with the targeted TypeScript check.
--
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]
