rusackas commented on code in PR #38033:
URL: https://github.com/apache/superset/pull/38033#discussion_r3627546156
##########
superset-frontend/src/pages/Login/Login.test.tsx:
##########
@@ -31,27 +31,26 @@ const defaultBootstrapData = {
},
};
-const mockApplicationRoot = jest.fn<string, []>(() => '');
+jest.mock('src/utils/getBootstrapData', () => ({
+ __esModule: true,
+ default: jest.fn(() => defaultBootstrapData),
+}));
-jest.mock('src/utils/getBootstrapData', () => {
- const actual = jest.requireActual<
- typeof import('src/utils/getBootstrapData')
- >('src/utils/getBootstrapData');
- return {
- __esModule: true,
- ...actual,
- default: jest.fn(() => defaultBootstrapData),
- applicationRoot: () => mockApplicationRoot(),
- };
-});
+const mockEnsureAppRoot = jest.fn((path: string) => path);
+jest.mock('src/utils/pathUtils', () => ({
Review Comment:
This works — `navigationUtils` re-exports `ensureAppRoot` straight from
`pathUtils`, so mocking `pathUtils` intercepts the call the component actually
makes.
##########
superset-frontend/src/pages/Login/index.tsx:
##########
@@ -96,15 +96,19 @@ export default function Login() {
}, []);
const loginEndpoint = useMemo(
- () => (nextUrl ? `/login/?next=${encodeURIComponent(nextUrl)}` :
'/login/'),
+ () =>
+ ensureAppRoot(
+ nextUrl ? `/login/?next=${encodeURIComponent(nextUrl)}` : '/login/',
+ ),
[nextUrl],
);
const buildProviderLoginUrl = (providerName: string) => {
- const base = `/login/${encodeURIComponent(providerName)}`;
- return ensureAppRoot(
- nextUrl ? `${base}?next=${encodeURIComponent(nextUrl)}` : base,
- );
+ const base = `/login/${providerName}`;
Review Comment:
Good catch — restored `encodeURIComponent` on the provider name in
`buildProviderLoginUrl`.
##########
superset/app.py:
##########
@@ -81,6 +81,12 @@ def create_app(
# value of app_root so things work out of the box
if not app.config["STATIC_ASSETS_PREFIX"]:
app.config["STATIC_ASSETS_PREFIX"] = app_root
+ # Prefix APP_ICON path with subdirectory root for subdirectory
deployments
+ if (
+ app.config.get("APP_ICON", "").startswith("/static/")
+ and app_root != "/"
+ ):
+ app.config["APP_ICON"] = f"{app_root}{app.config['APP_ICON']}"
Review Comment:
Switched to `STATIC_ASSETS_PREFIX` (which defaults to `app_root` here) so a
custom CDN/static prefix is honoured.
##########
superset-frontend/src/pages/Register/Register.test.tsx:
##########
@@ -17,32 +17,63 @@
* under the License.
*/
import { render, screen } from 'spec/helpers/testing-library';
-import { MemoryRouter } from 'react-router-dom';
+import { MemoryRouter, Route } from 'react-router-dom';
+import getBootstrapData from 'src/utils/getBootstrapData';
import Register from './index';
jest.mock('src/utils/getBootstrapData', () => ({
__esModule: true,
- default: () => ({
+ default: jest.fn(() => ({
common: {
conf: {
RECAPTCHA_PUBLIC_KEY: '',
},
},
- }),
+ })),
+}));
+
+const mockMakeUrl = jest.fn((path: string) => path);
+jest.mock('src/utils/pathUtils', () => ({
+ makeUrl: (...args: string[]) => mockMakeUrl(...args),
}));
Review Comment:
Fixed — the mock now stubs `ensureAppRoot`, matching what the component
imports.
##########
superset-frontend/src/pages/Register/Register.test.tsx:
##########
@@ -80,3 +111,71 @@ test('should render input placeholders', () => {
expect(screen.getByPlaceholderText('Password')).toBeInTheDocument();
expect(screen.getByPlaceholderText('Confirm password')).toBeInTheDocument();
});
+
+// --- Recaptcha tests ---
+
+test('should not render captcha when RECAPTCHA_PUBLIC_KEY is empty', () => {
+ renderRegister();
+ expect(screen.queryByTestId('captcha-input')).not.toBeInTheDocument();
+});
+
+test('should render captcha when RECAPTCHA_PUBLIC_KEY is set', () => {
+ mockGetBootstrapData.mockReturnValue({
+ common: {
+ conf: {
+ RECAPTCHA_PUBLIC_KEY: 'test-key-123',
+ },
+ },
+ } as ReturnType<typeof getBootstrapData>);
+ renderRegister();
+ expect(screen.getByTestId('captcha-input')).toBeInTheDocument();
+});
+
+// --- Activation success page tests ---
+
+test('should render activation success page when activationHash is present',
() => {
+ renderActivated();
+ expect(screen.getByText('Registration successful')).toBeInTheDocument();
+ expect(
+ screen.getByText(
+ 'Your account is activated. You can log in with your credentials.',
+ ),
+ ).toBeInTheDocument();
+ expect(screen.getByTestId('login-button')).toBeInTheDocument();
+});
+
+test('should not render registration form on activation page', () => {
+ renderActivated();
+ expect(screen.queryByTestId('username-input')).not.toBeInTheDocument();
+ expect(screen.queryByTestId('register-button')).not.toBeInTheDocument();
+});
+
+test('should call makeUrl for login link on activation page', () => {
+ renderActivated();
+ expect(mockMakeUrl).toHaveBeenCalledWith('/login/');
+});
Review Comment:
Fixed — the assertion now checks `ensureAppRoot`.
--
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]
