codeant-ai-for-open-source[bot] commented on code in PR #38069:
URL: https://github.com/apache/superset/pull/38069#discussion_r2822445128
##########
superset-frontend/src/pages/Login/Login.test.tsx:
##########
@@ -42,14 +54,100 @@ test('should render login form elements', () => {
});
test('should render username and password labels', () => {
- render(<Login />, { useRedux: true });
+ renderLogin();
expect(screen.getByText('Username:')).toBeInTheDocument();
expect(screen.getByText('Password:')).toBeInTheDocument();
});
test('should render form instruction text', () => {
- render(<Login />, { useRedux: true });
+ renderLogin();
expect(
screen.getByText('Enter your login and password below:'),
).toBeInTheDocument();
});
+
+test('should render registration button with correct app root URL when
authRegister=true', () => {
+ mockGetBootstrapData.mockReturnValue(defaultBootstrapData(true));
+ mockApplicationRoot.mockReturnValue('/superset');
+
+ renderLogin();
+
+ const registerButton = screen.getByTestId('register-button');
+ expect(registerButton).toHaveAttribute('href', '/superset/register/');
+});
+
+test.each([['', '/superset']])(
Review Comment:
**Suggestion:** The `test.each([['', '/superset']])` table passes two values
per row but the test callback only accepts one parameter, so only the first
value (`''`) is used and the `/superset` case is never exercised; split the
cases so each row provides a single `app_root` value. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ `/superset` app root OAuth URLs untested in Login tests.
- ⚠️ Regression on non-empty app_root may go unnoticed.
```
</details>
```suggestion
test.each([[''], ['/superset']])(
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset-frontend/src/pages/Login/Login.test.tsx` and locate the
OAuth providers
test starting at `test.each([['', '/superset']])(` (around line 79).
2. Note that the data table passed to `test.each` is `[['', '/superset']]`,
i.e., a single
row with two columns.
3. Observe that the test callback is declared as `(app_root: string) => {
... }`, which
means Jest will pass only the first column (`''`) as `app_root` and ignore
the second
(`'/superset'`), per Jest's `test.each` semantics.
4. Run `npm test -- Login.test.tsx` (or equivalent) and verify in the Jest
output that
there is only one instance of `'should render OAuth providers with app root
%s'` executed,
confirming that the `/superset` case is never exercised by this test.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/Login/Login.test.tsx
**Line:** 79:79
**Comment:**
*Logic Error: The `test.each([['', '/superset']])` table passes two
values per row but the test callback only accepts one parameter, so only the
first value (`''`) is used and the `/superset` case is never exercised; split
the cases so each row provides a single `app_root` value.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38069&comment_hash=1f14df4965612d0ff97d122cb880c73408ffacf61ed2e188e33073d7a31a9be9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38069&comment_hash=1f14df4965612d0ff97d122cb880c73408ffacf61ed2e188e33073d7a31a9be9&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts:
##########
@@ -696,7 +696,7 @@ describe('SupersetClientClass', () => {
client = new SupersetClientClass({ protocol, host, guestToken });
await client.init();
- await client.postForm(mockPostFormUrl, {});
+ await client.postForm({ endpoint: mockPostFormUrl, payload: {} });
Review Comment:
**Suggestion:** The guest-token postForm test now passes a full URL via the
`endpoint` property instead of the `url` property, which will cause `getUrl`
inside `postForm` to treat the full URL as a path segment and build an
incorrect action URL; this no longer matches the original behavior where a full
URL was passed directly, and the test no longer validates the correct URL
handling. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Guest-token postForm test misuses API, hides URL bugs.
- ⚠️ postForm guest-token flow not correctly validated in tests.
```
</details>
```suggestion
await client.postForm({ url: mockPostFormUrl, payload: {} });
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the Jest test suite for `SupersetClientClass` in
`superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts`.
2. In the `.postForm()` describe block, locate the test `"makes postForm
request with
guest token"` at lines 695–707, where `mockPostFormUrl` is defined earlier
as a full URL
(`const mockPostFormUrl = `${protocol}//${host}${mockPostFormEndpoint}`;`).
3. Observe that this test calls `await client.postForm({ endpoint:
mockPostFormUrl,
payload: {} });` (line 699), passing the full URL string via the `endpoint`
property
instead of the `url` property.
4. Compare this usage with the `.getUrl()` behavior tested earlier in the
same file
(describe('.getUrl()')) where `url` is used for full URLs (e.g.,
`client.getUrl({ url:
'myUrl', endpoint: 'blah', host: 'blah' })`) and `endpoint` is used for path
segments
only; this shows that passing a full URL as `endpoint` will cause `getUrl`
(and thus
`postForm`'s form `action`) to be constructed incorrectly, so the test no
longer validates
correct URL handling for the guest-token `postForm` case.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts
**Line:** 699:699
**Comment:**
*Logic Error: The guest-token postForm test now passes a full URL via
the `endpoint` property instead of the `url` property, which will cause
`getUrl` inside `postForm` to treat the full URL as a path segment and build an
incorrect action URL; this no longer matches the original behavior where a full
URL was passed directly, and the test no longer validates the correct URL
handling.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38069&comment_hash=38fddd3580b993b86f761c9ec3637acd072a11b4f92fbf7c1bb9a93b6a5e4e11&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38069&comment_hash=38fddd3580b993b86f761c9ec3637acd072a11b4f92fbf7c1bb9a93b6a5e4e11&reaction=dislike'>👎</a>
##########
superset-frontend/src/pages/Login/index.tsx:
##########
@@ -131,7 +132,13 @@ export default function Login() {
sessionStorage.setItem('login_attempted', 'true');
// Use standard form submission for Flask-AppBuilder compatibility
- SupersetClient.postForm(loginEndpoint, values, '');
+ SupersetClient.postForm({
+ endpoint: loginEndpoint,
+ payload: values,
+ target: '',
+ }).finally(() => {
+ setLoading(false);
Review Comment:
**Suggestion:** The `SupersetClient.postForm` helper is synchronous and does
not return a Promise, so calling `.finally` on its return value will throw a
runtime TypeError when submitting the login form; remove the `.finally` chain
and avoid treating it as a Promise. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Database/LDAP login form crashes on submit.
- ❌ Users cannot authenticate via username/password flow.
- ⚠️ Loading spinner remains, confusing user feedback.
```
</details>
```suggestion
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the Superset frontend and backend with this PR code and configure
AUTH_TYPE to
AuthDB or AuthLDAP so the username/password login form is rendered by
`superset-frontend/src/pages/Login/index.tsx`.
2. In a browser, navigate to the login page (served by this component) and
open dev tools
Console and Network tabs to observe runtime behavior.
3. Enter any username and password and click the "Sign in" button, which
triggers the
`onFinish` handler in `superset-frontend/src/pages/Login/index.tsx:128-142`.
4. The handler calls `SupersetClient.postForm({ ... })` and then attempts to
access
`.finally` on its return value at line 139; since `postForm` is a
synchronous helper that
returns `void`/`undefined` in `@superset-ui/core`, the browser throws
`TypeError: Cannot
read properties of undefined (reading 'finally')`, no network request is
sent, and the
login form never submits.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/Login/index.tsx
**Line:** 139:140
**Comment:**
*Type Error: The `SupersetClient.postForm` helper is synchronous and
does not return a Promise, so calling `.finally` on its return value will throw
a runtime TypeError when submitting the login form; remove the `.finally` chain
and avoid treating it as a Promise.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38069&comment_hash=a3d4864f14844472efa9d42a10f83dc0bb0270b69791cfe7eed3fe6c787e63c4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38069&comment_hash=a3d4864f14844472efa9d42a10f83dc0bb0270b69791cfe7eed3fe6c787e63c4&reaction=dislike'>👎</a>
##########
superset-frontend/src/pages/Login/Login.test.tsx:
##########
@@ -42,14 +54,100 @@ test('should render login form elements', () => {
});
test('should render username and password labels', () => {
- render(<Login />, { useRedux: true });
+ renderLogin();
expect(screen.getByText('Username:')).toBeInTheDocument();
expect(screen.getByText('Password:')).toBeInTheDocument();
});
test('should render form instruction text', () => {
- render(<Login />, { useRedux: true });
+ renderLogin();
expect(
screen.getByText('Enter your login and password below:'),
).toBeInTheDocument();
});
+
+test('should render registration button with correct app root URL when
authRegister=true', () => {
+ mockGetBootstrapData.mockReturnValue(defaultBootstrapData(true));
+ mockApplicationRoot.mockReturnValue('/superset');
+
+ renderLogin();
+
+ const registerButton = screen.getByTestId('register-button');
+ expect(registerButton).toHaveAttribute('href', '/superset/register/');
+});
+
+test.each([['', '/superset']])(
+ 'should render OAuth providers with app root %s',
+ (app_root: string) => {
+ mockGetBootstrapData.mockReturnValue({
+ common: {
+ conf: {
+ AUTH_TYPE: 4, // AuthType.AuthOauth
+ AUTH_PROVIDERS: [
+ { name: 'google', icon: 'google' },
+ { name: 'github', icon: 'github' },
+ ],
+ AUTH_USER_REGISTRATION: false,
+ },
+ },
+ });
+
+ mockApplicationRoot.mockReturnValue(app_root);
+
+ renderLogin();
+
+ const googleButton = screen.getByRole('link', {
+ name: /Sign in with Google/i,
+ });
+ const githubButton = screen.getByRole('link', {
+ name: /Sign in with Github/i,
+ });
+
+ expect(googleButton).toHaveAttribute('href', `${app_root}/login/google`);
+ expect(githubButton).toHaveAttribute('href', `${app_root}/login/github`);
+ },
+);
+
+test.each([[1, 2]])(
+ 'should call SupersetClient.postForm with correct endpoint for
AuthDB/AuthLDAP',
Review Comment:
**Suggestion:** The `test.each([[1, 2]])` table passes two values in a
single row but the test callback only uses one parameter, so the second auth
type is never tested; adjust the table to have one auth type per row so both
AuthDB and AuthLDAP are covered. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ AuthLDAP login path not covered by this test.
- ⚠️ Future regressions for authType 2 may slip through.
```
</details>
```suggestion
test.each([[1], [2]])(
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset-frontend/src/pages/Login/Login.test.tsx` and locate the
AuthDB/AuthLDAP
test starting at `test.each([[1, 2]])(` (around line 112).
2. Note that the data table for `test.each` is `[[1, 2]]`, i.e., a single
row with two
columns.
3. Observe that the test callback is declared as `async (authType: number)
=> { ... }`, so
Jest only passes the first column (`1`) as `authType`, and the second value
(`2`) is
ignored by the framework.
4. Run `npm test -- Login.test.tsx` and inspect the test output (or
temporarily log
`authType` inside the test) to confirm that the test only runs once with
`authType === 1`,
meaning the AuthLDAP type (`2`) is never actually tested.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/Login/Login.test.tsx
**Line:** 112:112
**Comment:**
*Logic Error: The `test.each([[1, 2]])` table passes two values in a
single row but the test callback only uses one parameter, so the second auth
type is never tested; adjust the table to have one auth type per row so both
AuthDB and AuthLDAP are covered.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38069&comment_hash=bd92b5549ff1b15ed9689f1d8fdd3c9d1c1550c8ae3ff443243a27634e14a148&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38069&comment_hash=bd92b5549ff1b15ed9689f1d8fdd3c9d1c1550c8ae3ff443243a27634e14a148&reaction=dislike'>👎</a>
--
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]
