bbovenzi commented on code in PR #66554: URL: https://github.com/apache/airflow/pull/66554#discussion_r3250723491
########## airflow-core/src/airflow/ui/src/queries/useBulkDagRuns.ts: ########## @@ -0,0 +1,210 @@ +/*! + * 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 { useQueryClient } from "@tanstack/react-query"; +import { useState } from "react"; +import { useTranslation } from "react-i18next"; + +import { useDagRunServiceGetDagRunsKey, useTaskInstanceServiceGetTaskInstancesKey } from "openapi/queries"; +import { DagRunService } from "openapi/requests/services.gen"; +import type { + BulkActionResponse, + BulkResponse, + DAGRunPatchStates, + DAGRunResponse, +} from "openapi/requests/types.gen"; +import { toaster } from "src/components/ui"; + +type Props = { + readonly clearSelections: VoidFunction; + readonly onSuccessConfirm: VoidFunction; +}; + +export type BulkClearDagRunsOptions = { + note: string | null; + onlyFailed: boolean; + onlyNew: boolean; + runOnLatestVersion: boolean; +}; + +type BulkMarkOptions = { + note: string | null; + state: DAGRunPatchStates; +}; + +type ToasterKey = "toaster.bulkClear" | "toaster.bulkDelete" | "toaster.bulkUpdate"; + +const formatActionResult = (response: BulkActionResponse | null | undefined) => ({ + firstErrorDetail: + response?.errors && response.errors.length > 0 + ? ((response.errors[0] as { error?: string } | undefined)?.error ?? "Bulk request failed") + : null, Review Comment: In a bulk action, we will care about more than just the first error. After we move to use the useMutation hook. Let's make sure we handle multiple errors ########## airflow-core/src/airflow/ui/src/pages/DagRuns/BulkClearDagRunsButton.tsx: ########## Review Comment: We have one hook to handle every action, but three components? I feel like it should either be one hook <> one button+modal component. Or three hooks <> three button+modal components. ########## airflow-core/src/airflow/ui/src/queries/useBulkDagRuns.ts: ########## @@ -0,0 +1,210 @@ +/*! + * 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 { useQueryClient } from "@tanstack/react-query"; +import { useState } from "react"; +import { useTranslation } from "react-i18next"; + +import { useDagRunServiceGetDagRunsKey, useTaskInstanceServiceGetTaskInstancesKey } from "openapi/queries"; +import { DagRunService } from "openapi/requests/services.gen"; +import type { + BulkActionResponse, + BulkResponse, + DAGRunPatchStates, + DAGRunResponse, +} from "openapi/requests/types.gen"; +import { toaster } from "src/components/ui"; + +type Props = { + readonly clearSelections: VoidFunction; + readonly onSuccessConfirm: VoidFunction; +}; + +export type BulkClearDagRunsOptions = { + note: string | null; + onlyFailed: boolean; + onlyNew: boolean; + runOnLatestVersion: boolean; +}; + +type BulkMarkOptions = { + note: string | null; + state: DAGRunPatchStates; +}; + +type ToasterKey = "toaster.bulkClear" | "toaster.bulkDelete" | "toaster.bulkUpdate"; + +const formatActionResult = (response: BulkActionResponse | null | undefined) => ({ + firstErrorDetail: + response?.errors && response.errors.length > 0 + ? ((response.errors[0] as { error?: string } | undefined)?.error ?? "Bulk request failed") + : null, + successCount: response?.success?.length ?? 0, + successKeys: response?.success ?? [], +}); + +export const useBulkDagRuns = ({ clearSelections, onSuccessConfirm }: Props) => { + const queryClient = useQueryClient(); + const [error, setError] = useState<unknown>(undefined); + const [isPending, setIsPending] = useState(false); + const { t: translate } = useTranslation(["common", "dags"]); + + const invalidateQueries = async () => { + await Promise.all([ + queryClient.invalidateQueries({ queryKey: [useDagRunServiceGetDagRunsKey] }), + queryClient.invalidateQueries({ queryKey: [useTaskInstanceServiceGetTaskInstancesKey] }), + ]); + }; + + const handleResult = ( + actionResult: BulkActionResponse | null | undefined, + toasterKey: ToasterKey, + ): boolean => { + const { firstErrorDetail, successCount, successKeys } = formatActionResult(actionResult); + + if (successCount > 0) { + toaster.create({ + description: translate(`${toasterKey}.success.description`, { + count: successCount, + keys: successKeys.join(", "), + resourceName: translate("dagRun_other"), + }), + title: translate(`${toasterKey}.success.title`, { + resourceName: translate("dagRun_other"), + }), + type: "success", + }); + } + + if (firstErrorDetail !== null) { + setError({ body: { detail: firstErrorDetail } }); + + return false; + } + + setError(undefined); + + return true; + }; + + const bulkClear = async (dagRuns: Array<DAGRunResponse>, options: BulkClearDagRunsOptions) => { + setError(undefined); + setIsPending(true); + + try { + const response = await DagRunService.postClearDagRuns({ Review Comment: We should try to use the autogenerated tanstack/query hooks, `useDagRunServiceBulkDagRuns, useDagRunServicePostClearDagRuns`, and not call the DagRunService directly ########## airflow-core/src/airflow/ui/src/queries/useBulkDagRuns.ts: ########## @@ -0,0 +1,210 @@ +/*! + * 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 { useQueryClient } from "@tanstack/react-query"; +import { useState } from "react"; +import { useTranslation } from "react-i18next"; + +import { useDagRunServiceGetDagRunsKey, useTaskInstanceServiceGetTaskInstancesKey } from "openapi/queries"; +import { DagRunService } from "openapi/requests/services.gen"; +import type { + BulkActionResponse, + BulkResponse, + DAGRunPatchStates, + DAGRunResponse, +} from "openapi/requests/types.gen"; +import { toaster } from "src/components/ui"; + +type Props = { + readonly clearSelections: VoidFunction; + readonly onSuccessConfirm: VoidFunction; +}; + +export type BulkClearDagRunsOptions = { + note: string | null; + onlyFailed: boolean; + onlyNew: boolean; + runOnLatestVersion: boolean; +}; + +type BulkMarkOptions = { + note: string | null; + state: DAGRunPatchStates; +}; + +type ToasterKey = "toaster.bulkClear" | "toaster.bulkDelete" | "toaster.bulkUpdate"; + +const formatActionResult = (response: BulkActionResponse | null | undefined) => ({ + firstErrorDetail: + response?.errors && response.errors.length > 0 + ? ((response.errors[0] as { error?: string } | undefined)?.error ?? "Bulk request failed") + : null, + successCount: response?.success?.length ?? 0, + successKeys: response?.success ?? [], +}); + +export const useBulkDagRuns = ({ clearSelections, onSuccessConfirm }: Props) => { + const queryClient = useQueryClient(); + const [error, setError] = useState<unknown>(undefined); + const [isPending, setIsPending] = useState(false); + const { t: translate } = useTranslation(["common", "dags"]); + + const invalidateQueries = async () => { + await Promise.all([ + queryClient.invalidateQueries({ queryKey: [useDagRunServiceGetDagRunsKey] }), + queryClient.invalidateQueries({ queryKey: [useTaskInstanceServiceGetTaskInstancesKey] }), + ]); + }; + + const handleResult = ( + actionResult: BulkActionResponse | null | undefined, + toasterKey: ToasterKey, + ): boolean => { + const { firstErrorDetail, successCount, successKeys } = formatActionResult(actionResult); + + if (successCount > 0) { + toaster.create({ + description: translate(`${toasterKey}.success.description`, { + count: successCount, + keys: successKeys.join(", "), + resourceName: translate("dagRun_other"), + }), + title: translate(`${toasterKey}.success.title`, { + resourceName: translate("dagRun_other"), + }), + type: "success", + }); + } + + if (firstErrorDetail !== null) { + setError({ body: { detail: firstErrorDetail } }); + + return false; + } + + setError(undefined); + + return true; + }; + + const bulkClear = async (dagRuns: Array<DAGRunResponse>, options: BulkClearDagRunsOptions) => { + setError(undefined); + setIsPending(true); + + try { + const response = await DagRunService.postClearDagRuns({ + dagId: "~", + requestBody: { + dry_run: false, + note: options.note, + only_failed: options.onlyFailed, + only_new: options.onlyNew, + run_on_latest_version: options.runOnLatestVersion, + runs: dagRuns.map((dr) => ({ dag_id: dr.dag_id, dag_run_id: dr.dag_run_id })), + }, + }); + + await invalidateQueries(); + + if (handleResult(response, "toaster.bulkClear")) { + clearSelections(); + onSuccessConfirm(); Review Comment: We should invalidate the queries after success, not before. ########## airflow-core/src/airflow/ui/src/pages/DagRuns/BulkClearDagRunsButton.tsx: ########## @@ -0,0 +1,147 @@ +/*! + * 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 { Box, Button, Flex, Heading, Text, useDisclosure, VStack } from "@chakra-ui/react"; +import { useState } from "react"; +import { useTranslation } from "react-i18next"; +import { CgRedo } from "react-icons/cg"; + +import type { DAGRunResponse } from "openapi/requests/types.gen"; +import { ActionAccordion } from "src/components/ActionAccordion"; +import { ErrorAlert } from "src/components/ErrorAlert"; +import { Checkbox, Dialog } from "src/components/ui"; +import SegmentedControl from "src/components/ui/SegmentedControl"; +import { useBulkDagRuns } from "src/queries/useBulkDagRuns"; + +type Props = { + readonly clearSelections: VoidFunction; + readonly selectedDagRuns: Array<DAGRunResponse>; +}; + +const BulkClearDagRunsButton = ({ clearSelections, selectedDagRuns }: Props) => { + const { t: translate } = useTranslation(); + const { onClose, onOpen, open } = useDisclosure(); + const [selectedOptions, setSelectedOptions] = useState<Array<string>>(["existingTasks"]); + const [note, setNote] = useState<string | null>(null); + const [runOnLatestVersion, setRunOnLatestVersion] = useState(false); + const { bulkClear, error, isPending } = useBulkDagRuns({ + clearSelections, + onSuccessConfirm: onClose, + }); + + const handleClose = () => { Review Comment: Let's make sure we reset all states on modal close. Right now we're only resetting notes. ########## airflow-core/src/airflow/ui/src/queries/useBulkDagRuns.ts: ########## @@ -0,0 +1,210 @@ +/*! + * 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 { useQueryClient } from "@tanstack/react-query"; +import { useState } from "react"; +import { useTranslation } from "react-i18next"; + +import { useDagRunServiceGetDagRunsKey, useTaskInstanceServiceGetTaskInstancesKey } from "openapi/queries"; +import { DagRunService } from "openapi/requests/services.gen"; +import type { + BulkActionResponse, + BulkResponse, + DAGRunPatchStates, + DAGRunResponse, +} from "openapi/requests/types.gen"; +import { toaster } from "src/components/ui"; + +type Props = { + readonly clearSelections: VoidFunction; + readonly onSuccessConfirm: VoidFunction; +}; + +export type BulkClearDagRunsOptions = { + note: string | null; + onlyFailed: boolean; + onlyNew: boolean; + runOnLatestVersion: boolean; +}; + +type BulkMarkOptions = { + note: string | null; + state: DAGRunPatchStates; +}; + +type ToasterKey = "toaster.bulkClear" | "toaster.bulkDelete" | "toaster.bulkUpdate"; + +const formatActionResult = (response: BulkActionResponse | null | undefined) => ({ + firstErrorDetail: + response?.errors && response.errors.length > 0 + ? ((response.errors[0] as { error?: string } | undefined)?.error ?? "Bulk request failed") + : null, + successCount: response?.success?.length ?? 0, + successKeys: response?.success ?? [], +}); + +export const useBulkDagRuns = ({ clearSelections, onSuccessConfirm }: Props) => { + const queryClient = useQueryClient(); + const [error, setError] = useState<unknown>(undefined); Review Comment: This is why we want to use the tanstack/query mutation hooks. We shouldn't be manually handling pending and error. Especially with an unknown type. -- 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]
