pierrejeambrun commented on code in PR #26457: URL: https://github.com/apache/airflow/pull/26457#discussion_r1017015631
########## airflow/www/static/js/api/useSetDagRunNotes.ts: ########## @@ -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 axios from 'axios'; +import { useMutation, useQueryClient } from 'react-query'; + +import { getMetaValue } from 'src/utils'; +import type { API } from 'src/types'; +import useErrorToast from 'src/utils/useErrorToast'; + +import { emptyGridData } from './useGridData'; +import type { GridData } from './useGridData'; + +const setDagRunNotesURI = getMetaValue('set_dag_run_notes'); + +interface Props { + dagId: string; + runId: string; +} + +export default function useSetDagRunNotes({ + dagId, runId, +}: Props) { + const queryClient = useQueryClient(); + const errorToast = useErrorToast(); + const setDagRunNotes = setDagRunNotesURI.replace('_DAG_RUN_ID_', runId); + + return useMutation( + ['setDagRunNotes', dagId, runId], + (notes: string | null) => axios.patch(setDagRunNotes, { notes }), Review Comment: ```suggestion (notes: string | null) => axios.patch<AxiosResponse, API.DagRun>(setDagRunNotes, { notes }), ``` ########## airflow/www/static/js/api/useSetDagRunNotes.ts: ########## @@ -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 axios from 'axios'; +import { useMutation, useQueryClient } from 'react-query'; + +import { getMetaValue } from 'src/utils'; +import type { API } from 'src/types'; +import useErrorToast from 'src/utils/useErrorToast'; + +import { emptyGridData } from './useGridData'; +import type { GridData } from './useGridData'; + +const setDagRunNotesURI = getMetaValue('set_dag_run_notes'); + +interface Props { + dagId: string; + runId: string; +} + +export default function useSetDagRunNotes({ + dagId, runId, +}: Props) { + const queryClient = useQueryClient(); + const errorToast = useErrorToast(); + const setDagRunNotes = setDagRunNotesURI.replace('_DAG_RUN_ID_', runId); + + return useMutation( + ['setDagRunNotes', dagId, runId], + (notes: string | null) => axios.patch(setDagRunNotes, { notes }), + { + onSuccess: async (data) => { + const notes = (data as API.DAGRun).notes ?? null; Review Comment: So you can avoid casting here ```suggestion const notes = data.notes ?? null; ``` ########## airflow/www/static/js/utils/colors.json: ########## @@ -0,0 +1,842 @@ +[ Review Comment: I don't like having to add an 800 json file to do color mapping just for the note linear-gradient. I wish we could do without it. Or implement a solution that leverage ChakraUI themes and color palette. (We could for instance only map default colors to chakra color palette and then play with the numbers to get brighter colors -> red.400, red.200 etc.) ########## airflow/www/static/js/api/useSetDagRunNotes.ts: ########## @@ -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 axios from 'axios'; Review Comment: ```suggestion import axios, { AxiosResponse } from 'axios'; ``` ########## airflow/www/static/js/api/useSetDagRunNotes.ts: ########## @@ -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 axios from 'axios'; +import { useMutation, useQueryClient } from 'react-query'; + +import { getMetaValue } from 'src/utils'; +import type { API } from 'src/types'; +import useErrorToast from 'src/utils/useErrorToast'; + +import { emptyGridData } from './useGridData'; +import type { GridData } from './useGridData'; + +const setDagRunNotesURI = getMetaValue('set_dag_run_notes'); + +interface Props { + dagId: string; + runId: string; +} Review Comment: For late: Too bad we cannot use `SetDagRunNotesVariables` to type this. I think we should separate the type generation for `parameters/path & query` and `requetsBody`. This way we could easily type such mutation, keeping type enforced by the OpenAPI spec. What do you think @bbovenzi ########## airflow/www/static/js/api/useSetTaskInstanceNotes.ts: ########## @@ -0,0 +1,114 @@ +/*! + * 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 axios from 'axios'; +import { useMutation, useQueryClient } from 'react-query'; + +import { getMetaValue } from 'src/utils'; +import useErrorToast from 'src/utils/useErrorToast'; + +import type { API } from 'src/types'; + +const setTaskInstancesNotesURI = getMetaValue('set_task_instance_notes'); +const setMappedTaskInstancesNotesURI = getMetaValue('set_mapped_task_instance_notes'); + +interface Props { + dagId: string; + runId: string; + taskId: string; + mapIndex?: number; +} + +export default function useSetTaskInstanceNotes({ + dagId, runId, taskId, mapIndex = -1, +}: Props) { + const queryClient = useQueryClient(); + const errorToast = useErrorToast(); + // Note: Werkzeug does not like the META URL on dag.html with an integer. It can not put + // _MAP_INDEX_ there as it interprets that as the integer. Hence, we pass 0 as the integer. + // To avoid we replace other stuff, we add the surrounding strings to the replacement query. + const url = (mapIndex >= 0 ? setMappedTaskInstancesNotesURI : setTaskInstancesNotesURI) + .replace('_DAG_RUN_ID_', runId) + .replace('_TASK_ID_/0/setNote', `_TASK_ID_/${mapIndex}/setNote`) + .replace('_TASK_ID_', taskId); + + return useMutation( + ['setTaskInstanceNotes', dagId, runId, taskId, mapIndex], + (notes: string | null) => axios.patch(url, { notes }), + { + onSuccess: async (data) => { + const notes = (data as API.TaskInstance).notes ?? null; Review Comment: Same here to avoid the `as` ########## airflow/www/static/js/api/useSetTaskInstanceNotes.ts: ########## @@ -0,0 +1,114 @@ +/*! + * 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 axios from 'axios'; +import { useMutation, useQueryClient } from 'react-query'; + +import { getMetaValue } from 'src/utils'; +import useErrorToast from 'src/utils/useErrorToast'; + +import type { API } from 'src/types'; + +const setTaskInstancesNotesURI = getMetaValue('set_task_instance_notes'); +const setMappedTaskInstancesNotesURI = getMetaValue('set_mapped_task_instance_notes'); + +interface Props { + dagId: string; + runId: string; + taskId: string; + mapIndex?: number; +} + +export default function useSetTaskInstanceNotes({ + dagId, runId, taskId, mapIndex = -1, +}: Props) { + const queryClient = useQueryClient(); + const errorToast = useErrorToast(); + // Note: Werkzeug does not like the META URL on dag.html with an integer. It can not put + // _MAP_INDEX_ there as it interprets that as the integer. Hence, we pass 0 as the integer. + // To avoid we replace other stuff, we add the surrounding strings to the replacement query. + const url = (mapIndex >= 0 ? setMappedTaskInstancesNotesURI : setTaskInstancesNotesURI) + .replace('_DAG_RUN_ID_', runId) + .replace('_TASK_ID_/0/setNote', `_TASK_ID_/${mapIndex}/setNote`) + .replace('_TASK_ID_', taskId); + + return useMutation( + ['setTaskInstanceNotes', dagId, runId, taskId, mapIndex], + (notes: string | null) => axios.patch(url, { notes }), + { + onSuccess: async (data) => { + const notes = (data as API.TaskInstance).notes ?? null; + + const updateMappedInstancesResult = (oldMappedInstances?: API.TaskInstanceCollection) => { + if (!oldMappedInstances) { + return { + taskInstances: [], + totalEntries: 0, + }; + } + if (mapIndex === undefined || mapIndex < 0) return oldMappedInstances; + return { + ...oldMappedInstances, + taskInstances: oldMappedInstances.taskInstances?.map((ti) => ( + ti.dagRunId === runId && ti.taskId === taskId && ti.mapIndex === mapIndex + ? { ...ti, notes } + : ti + )), + }; + }; + + const updateTaskInstanceResult = (oldTaskInstance?: API.TaskInstance) => { + if (!oldTaskInstance) throw new Error('Unknown value...'); + if ( + oldTaskInstance.dagRunId === runId + && oldTaskInstance.taskId === taskId + && ( + (oldTaskInstance.mapIndex == null && mapIndex < 0) + || oldTaskInstance.mapIndex === mapIndex + ) + ) { + return { + ...oldTaskInstance, + notes, + }; + } + return oldTaskInstance; + }; + /* + This will force a refetch of gridData. + Mutating the nested object is quite complicated, + we should simplify the gridData API object first + */ + await queryClient.invalidateQueries('gridData'); + + if (mapIndex >= 0) { + await queryClient.cancelQueries('mappedInstances'); + queryClient.setQueriesData('mappedInstances', updateMappedInstancesResult); + } + + await queryClient.cancelQueries('taskInstance'); + queryClient.setQueriesData( + ['taskInstance', dagId, runId, taskId, mapIndex], + updateTaskInstanceResult, + ); + }, + onError: (error: Error) => errorToast({ error }), Review Comment: Shouldn't we rollback the optimistic update in case there is an error ? ########## airflow/www/static/js/api/useSetDagRunNotes.ts: ########## @@ -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 axios from 'axios'; +import { useMutation, useQueryClient } from 'react-query'; + +import { getMetaValue } from 'src/utils'; +import type { API } from 'src/types'; +import useErrorToast from 'src/utils/useErrorToast'; + +import { emptyGridData } from './useGridData'; +import type { GridData } from './useGridData'; + +const setDagRunNotesURI = getMetaValue('set_dag_run_notes'); + +interface Props { + dagId: string; + runId: string; +} Review Comment: For instance the `notes` type would raise a type error I believe, it can be `undefined | string` but not `null | string` I believe. -- 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]
