bbovenzi commented on code in PR #45696: URL: https://github.com/apache/airflow/pull/45696#discussion_r1918745899
########## airflow/auth/managers/simple/ui/src/login/Controller.tsx: ########## @@ -0,0 +1,65 @@ +/*! + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* global document */ + +import React from "react"; +import {Alert, Container, Heading, Text} from "@chakra-ui/react"; + +import {useCreateToken} from "src/queries/useCreateToken"; +import {LoginForm} from "src/login/Form"; +import type {ApiError} from "openapi-gen/requests/core/ApiError"; +import type {LoginResponse, HTTPExceptionResponse, HTTPValidationError} from "openapi-gen/requests/types.gen"; + +export type LoginBody = { + username: string; password: string; +}; + +type ExpandedApiError = { + body: HTTPExceptionResponse | HTTPValidationError; +} & ApiError; + +export const Login = () => { + const onSuccess = (data: LoginResponse) => { + // Redirect to index page with the token + globalThis.location.replace(`/webapp/?token=${data.jwt_token}`); Review Comment: We'll need to remember to change this whenever we drop the `/webapp`. Or better, have one source of truth on the UI base route ########## airflow/auth/managers/simple/ui/src/login/Controller.tsx: ########## @@ -0,0 +1,65 @@ +/*! + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* global document */ Review Comment: ```suggestion ``` We don't need this. ########## airflow/auth/managers/simple/ui/src/login/Form.tsx: ########## @@ -0,0 +1,67 @@ +/*! + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import React from "react"; +import {Button, Field, Input, Stack} from "@chakra-ui/react"; +import {Controller, useForm} from "react-hook-form"; +import {LoginBody} from "./Controller"; + +type LoginFormProps = { + readonly onLogin: (loginBody: LoginBody) => string; + readonly isPending: boolean; +}; + +export const LoginForm = ({ onLogin, isPending }: LoginFormProps) => { + const { + control, handleSubmit, + formState: { isValid }, + } = useForm<LoginBody>({ + defaultValues: { + username: "", password: "", + }, + }); + + return <Stack spacing={4}> + <Controller + name="username" + control={control} + render={({field, fieldState}) => ( + <Field.Root invalid={Boolean(fieldState.error)} required> + <Field.Label>Username</Field.Label> + <Input {...field} /> + </Field.Root>)} + rules={{required: true}} + /> + + <Controller + name="password" + control={control} + render={({field, fieldState}) => ( + <Field.Root invalid={Boolean(fieldState.error)} required> + <Field.Label>Username</Field.Label> Review Comment: ```suggestion <Field.Label>Password</Field.Label> ``` ########## airflow/auth/managers/simple/ui/src/login/index.test.tsx: ########## @@ -0,0 +1,43 @@ +/*! + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* global describe */ Review Comment: ```suggestion ``` ########## airflow/auth/managers/simple/ui/src/login/Form.tsx: ########## @@ -0,0 +1,67 @@ +/*! + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import React from "react"; +import {Button, Field, Input, Stack} from "@chakra-ui/react"; +import {Controller, useForm} from "react-hook-form"; +import {LoginBody} from "./Controller"; + +type LoginFormProps = { + readonly onLogin: (loginBody: LoginBody) => string; + readonly isPending: boolean; +}; + +export const LoginForm = ({ onLogin, isPending }: LoginFormProps) => { + const { + control, handleSubmit, + formState: { isValid }, + } = useForm<LoginBody>({ + defaultValues: { + username: "", password: "", + }, + }); + + return <Stack spacing={4}> + <Controller + name="username" + control={control} + render={({field, fieldState}) => ( + <Field.Root invalid={Boolean(fieldState.error)} required> + <Field.Label>Username</Field.Label> + <Input {...field} /> + </Field.Root>)} + rules={{required: true}} + /> + + <Controller + name="password" + control={control} + render={({field, fieldState}) => ( + <Field.Root invalid={Boolean(fieldState.error)} required> + <Field.Label>Username</Field.Label> + <Input {...field} type="password"/> + </Field.Root>)} + rules={{required: true}} + /> + + <Button disabled={!isValid || isPending} colorPalette="blue" onClick={() => void handleSubmit(onLogin)()}> Review Comment: Let's add a `type="submit"` here and wrap the <Stack> component with `<form></form>`. Then one can hit enter/return without having to focus on the Sign In button itself ########## airflow/auth/managers/simple/ui/src/test-utils.tsx: ########## @@ -0,0 +1,36 @@ +/*! + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import React, { PropsWithChildren } from "react"; +import { ChakraProvider } from "@chakra-ui/react"; +import { MemoryRouter, MemoryRouterProps } from "react-router-dom"; + +interface WrapperProps extends PropsWithChildren { + initialEntries?: MemoryRouterProps["initialEntries"]; +} + +export const Wrapper = ({ initialEntries, children }: WrapperProps) => { + return ( + <ChakraProvider> Review Comment: ```suggestion <ChakraProvider value={defaultSystem}> <QueryClientProvider client={queryClient}> ``` We need to pass the defaultSystem and also add a queryClient to our wrapper ########## airflow/auth/managers/simple/ui/src/queryClient.ts: ########## @@ -0,0 +1,36 @@ +/*! + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { QueryClient } from "@tanstack/react-query"; + +export const queryClient = new QueryClient({ + defaultOptions: { + mutations: { + retry: 1, + retryDelay: 500, + }, + queries: { + initialDataUpdatedAt: new Date().setMinutes(-6), // make sure initial data is already expired + refetchOnMount: true, // Refetches stale queries, not "always" + refetchOnWindowFocus: false, + retry: 1, + retryDelay: 500, + staleTime: 5 * 60 * 1000, // 5 minutes + }, + }, Review Comment: We don't have any queries and only one mutation, log in. So I don't think this is necessary. ########## airflow/auth/managers/simple/ui/src/login/Controller.tsx: ########## Review Comment: Let's have the file name match the component name `Login.tsx` ########## airflow/auth/managers/simple/ui/src/login/index.test.tsx: ########## Review Comment: `Login.test.tsx` ########## airflow/auth/managers/simple/ui/src/login/Form.tsx: ########## Review Comment: `LoginForm.tsx` -- 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]
