🐛 Fix custom target order handling

Resolves: https://issues.redhat.com/browse/MTA-6287
Resolves: #2708

All targets will new be displayed, even if they are not in the
target order setting.  The target query hook now will return the
server ordering of the targets, and ordering by the "ui.target.order"
setting.  All targets are included in both lists.  Any unordered
targets is added to the end of the ordered list.  By putting this
logic in the target query hook, target order can be used the same
way everywhere it is needed.

On the migration-targets page, the targets are now displayed in
the order specified by the "ui.target.order" setting.  Any targets
not in the setting are added to the end of the list.

On the custom-target-form, updated the onSubmit handler to
both create a new target and add it to the target order.  This
is done awaiting async mutations to help ensure the operations
finish before the create modal is closed.

Signed-off-by: Scott J Dickerson <[email protected]>

Author: sjd78

client/src/app/pages/applications/analysis-wizard/set-targets.tsx

@@ -37,6 +37,7 @@ interface SetTargetsProps {
 const useEnhancedTargets = (applications: Application[]) => {
   const {
     targets,
+    targetsInOrder,
     isFetching: isTargetsLoading,
     fetchError: isTargetsError,
   } = useFetchTargets();
@@ -69,31 +70,14 @@ const useEnhancedTargets = (applications: Application[]) => {
     [applications, languageTags, languageProviders]
   );
 
-  // 1. missing target order setting is not a blocker (only lowers user experience)
-  // 2. targets without assigned position (if any) are put at the end
-  const targetsWithOrder = useMemo(
-    () =>
-      targets
-        .map((target) => {
-          const index = targetOrder.findIndex((id) => id === target.id);
-          return {
-            target,
-            order: index === -1 ? targets.length : index,
-          };
-        })
-        .sort((a, b) => a.order - b.order)
-        .map(({ target }) => target),
-    [targets, targetOrder]
-  );
-
   return {
     // true if some queries are still fetching data for the first time (initial load)
     // note that the default re-try count (3) is used
     isLoading:
       isTagCategoriesLoading || isTargetsLoading || isTargetOrderLoading,
     // missing targets are the only blocker
     isError: !!isTargetsError,
-    targets: targetsWithOrder,
+    targets: targetsInOrder,
     applicationProviders,
     languageProviders,
   };

client/src/app/pages/migration-targets/components/custom-target-form.tsx

@@ -27,6 +27,7 @@ import { NotificationsContext } from "@app/components/NotificationsContext";
 import { OptionWithValue, SimpleSelect } from "@app/components/SimpleSelect";
 import { UploadFileSchema } from "@app/pages/applications/analysis-wizard/schema";
 import { useFetchIdentities } from "@app/queries/identities";
+import { useSettingMutation } from "@app/queries/settings";
 import {
   useCreateTargetMutation,
   useFetchTargets,
@@ -47,6 +48,7 @@ import defaultImage from "./default.png";
 
 export interface CustomTargetFormProps {
   target?: Target | null;
+  targetOrder: number[];
   onSaved: (response: AxiosResponse<Target>) => void;
   onCancel: () => void;
 }
@@ -77,12 +79,58 @@ const targetRulesetsToReadFiles = (target?: Target) =>
     };
   }) || [];
 
+const useTargetQueryHooks = (
+  onSaved: (response: AxiosResponse<Target>) => void,
+  reset: () => void
+) => {
+  const { pushNotification } = useContext(NotificationsContext);
+
+  const { mutateAsync: createImageFileAsync } = useCreateFileMutation();
+
+  const { mutateAsync: createTargetAsync } = useCreateTargetMutation(
+    (response: AxiosResponse<Target>) => {
+      // TODO: Consider any removed files and delete them from hub if necessary
+      onSaved(response);
+      reset();
+    },
+    (error: AxiosError) => {
+      pushNotification({
+        title: getAxiosErrorMessage(error),
+        variant: "danger",
+      });
+    }
+  );
+
+  const onUpdateTargetSuccess = (response: AxiosResponse<Target>) => {
+    // TODO: Consider any removed files and delete them from hub if necessary
+    onSaved(response);
+    reset();
+  };
+
+  const onUpdateTargetFailure = (_error: AxiosError) => {};
+
+  const { mutate: updateTarget } = useUpdateTargetMutation(
+    onUpdateTargetSuccess,
+    onUpdateTargetFailure
+  );
+
+  const { mutateAsync: targetOrderMutateAsync } =
+    useSettingMutation("ui.target.order");
+
+  return {
+    createTargetAsync,
+    updateTarget,
+    targetOrderMutateAsync,
+    createImageFileAsync,
+  };
+};
+
 export const CustomTargetForm: React.FC<CustomTargetFormProps> = ({
   target: initialTarget,
+  targetOrder,
   onSaved,
   onCancel,
 }) => {
-  const { pushNotification } = useContext(NotificationsContext);
   const baseProviderList = useMigrationProviderList();
   const providerList = useMemo(
     () =>
@@ -261,6 +309,13 @@ export const CustomTargetForm: React.FC<CustomTargetFormProps> = ({
 
   const values = getValues();
 
+  const {
+    createTargetAsync,
+    updateTarget,
+    targetOrderMutateAsync,
+    createImageFileAsync,
+  } = useTargetQueryHooks(onSaved, reset);
+
   const onSubmit = async (formValues: CustomTargetFormValues) => {
     const rules: Rule[] = formValues.customRulesFiles
       .map((file) => {
@@ -339,7 +394,22 @@ export const CustomTargetForm: React.FC<CustomTargetFormProps> = ({
     if (target) {
       updateTarget({ id: target.id, ...payload });
     } else {
-      createTarget(payload);
+      let newTargetId: number | undefined;
+      try {
+        const response = await createTargetAsync(payload);
+        newTargetId = response.data.id;
+      } catch {
+        /* ignore */
+      }
+
+      // Add the new target to the target order and wait for it to complete before continuing.
+      if (newTargetId !== undefined) {
+        try {
+          await targetOrderMutateAsync([...targetOrder, newTargetId]);
+        } catch {
+          /* ignore */
+        }
+      }
     }
   };
 
@@ -348,39 +418,6 @@ export const CustomTargetForm: React.FC<CustomTargetFormProps> = ({
     onCancel();
   };
 
-  const { mutateAsync: createImageFileAsync } = useCreateFileMutation();
-
-  const onCreateTargetSuccess = (response: AxiosResponse<Target>) => {
-    // TODO: Consider any removed files and delete them from hub if necessary
-    onSaved(response);
-    reset();
-  };
-
-  const onCreateTargetFailure = (error: AxiosError) => {
-    pushNotification({
-      title: getAxiosErrorMessage(error),
-      variant: "danger",
-    });
-  };
-
-  const { mutate: createTarget } = useCreateTargetMutation(
-    onCreateTargetSuccess,
-    onCreateTargetFailure
-  );
-
-  const onUpdateTargetSuccess = (response: AxiosResponse<Target>) => {
-    // TODO: Consider any removed files and delete them from hub if necessary
-    onSaved(response);
-    reset();
-  };
-
-  const onUpdateTargetFailure = (_error: AxiosError) => {};
-
-  const { mutate: updateTarget } = useUpdateTargetMutation(
-    onUpdateTargetSuccess,
-    onUpdateTargetFailure
-  );
-
   const handleImageFileUpload = async (file: File) => {
     setImageFilename(file.name);
     return createImageFileAsync({ file });

client/src/app/pages/migration-targets/migration-targets.tsx

@@ -50,7 +50,7 @@ export const MigrationTargets: React.FC = () => {
   const { t } = useTranslation();
   const { pushNotification } = React.useContext(NotificationsContext);
 
-  const { targets, refetch: refetchTargets } = useFetchTargets();
+  const { targetsInOrder } = useFetchTargets();
 
   const targetOrderSetting = useSetting("ui.target.order");
   const targetOrderSettingMutation = useSettingMutation("ui.target.order");
@@ -94,7 +94,7 @@ export const MigrationTargets: React.FC = () => {
     onDeleteTargetError
   );
 
-  const onCustomTargetModalSaved = (response: AxiosResponse<Target>) => {
+  const onCustomTargetModalSaved = async (response: AxiosResponse<Target>) => {
     if (targetToUpdate) {
       pushNotification({
         title: t("toastr.success.saveWhat", {
@@ -121,18 +121,6 @@ export const MigrationTargets: React.FC = () => {
         ),
       });
 
-      // update target order
-      if (
-        targetOrderSetting.isSuccess &&
-        response.data.id &&
-        targetOrderSetting.data
-      ) {
-        targetOrderSettingMutation.mutate([
-          ...targetOrderSetting.data,
-          response.data.id,
-        ]);
-      }
-
       // Make sure the new target's provider is part of the providers filter so it can be seen
       if (filterState.filterValues["provider"]) {
         const targetProvider = response.data.provider;
@@ -149,8 +137,8 @@ export const MigrationTargets: React.FC = () => {
         });
       }
     }
+
     setCreateUpdateModalState(null);
-    refetchTargets();
   };
 
   const sensors = useSensors(useSensor(MouseSensor), useSensor(TouchSensor));
@@ -180,13 +168,15 @@ export const MigrationTargets: React.FC = () => {
 
   const handleDragStart = ({ active }: DragStartEvent) => {
     const activeId = active.id as number;
-    const activeTarget = targets.find((target) => target.id === activeId);
+    const activeTarget = targetsInOrder.find(
+      (target) => target.id === activeId
+    );
     setActiveTarget(activeTarget ?? null);
   };
 
   const tableControls = useLocalTableControls({
     tableName: "target-cards",
-    items: targets,
+    items: targetsInOrder,
     idProperty: "name",
     initialFilterValues: { provider: [DEFAULT_PROVIDER] },
     columnNames: {
@@ -200,7 +190,7 @@ export const MigrationTargets: React.FC = () => {
     filterCategories: [
       {
         selectOptions: unique(
-          targets.map((target) => target.provider).filter(Boolean)
+          targetsInOrder.map((target) => target.provider).filter(Boolean)
         ).map((target) => ({ value: target })),
         placeholderText: "Filter by language...",
         categoryKey: "provider",
@@ -230,9 +220,19 @@ export const MigrationTargets: React.FC = () => {
       return [];
     }
 
-    return targetOrderSetting.data
+    // Get targets in the order specified by the setting
+    const orderedTargets = targetOrderSetting.data
       .map((id) => filteredTargets.find((target) => target.id === id))
       .filter(Boolean);
+
+    // Find targets that are in filteredTargets but not in the ordered list
+    const orderedTargetIds = new Set(orderedTargets.map((target) => target.id));
+    const unorderedTargets = filteredTargets.filter(
+      (target) => !orderedTargetIds.has(target.id)
+    );
+
+    // Combine ordered targets with unordered ones at the end
+    return [...orderedTargets, ...unorderedTargets];
   }, [filteredTargets, targetOrderSetting.data, targetOrderSetting.isSuccess]);
 
   return (
@@ -315,6 +315,7 @@ export const MigrationTargets: React.FC = () => {
       >
         <CustomTargetForm
           target={targetToUpdate}
+          targetOrder={targetOrderSetting.data ?? []}
           onSaved={onCustomTargetModalSaved}
           onCancel={() => setCreateUpdateModalState(null)}
         />

client/src/app/queries/settings.ts

@@ -23,10 +23,10 @@ export const useSettingMutation = <K extends keyof SettingTypes>(key: K) => {
   return useMutation({
     mutationFn: (value: SettingTypes[K]) => updateSetting({ key, value }),
     onSuccess: () => {
-      queryClient.invalidateQueries({ queryKey: [SettingQueryKey] });
+      queryClient.invalidateQueries({ queryKey: [SettingQueryKey, key] });
     },
     onError: () => {
-      queryClient.invalidateQueries({ queryKey: [SettingQueryKey] });
+      queryClient.invalidateQueries({ queryKey: [SettingQueryKey, key] });
     },
   });
 };

client/src/app/queries/targets.ts

@@ -1,3 +1,4 @@
+import { useMemo } from "react";
 import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query";
 import { AxiosError, AxiosResponse } from "axios";
 
@@ -11,20 +12,53 @@ import {
   updateTarget,
 } from "@app/api/rest";
 
+import { useSetting } from "./settings";
+
 export const TargetsQueryKey = "targets";
 
 export const useFetchTargets = (
   refetchInterval: number | false = DEFAULT_REFETCH_INTERVAL
 ) => {
-  const { data, isLoading, isSuccess, error, refetch } = useQuery<Target[]>({
+  const {
+    data: targets,
+    isLoading,
+    isSuccess,
+    error,
+    refetch,
+  } = useQuery<Target[]>({
     queryKey: [TargetsQueryKey],
     queryFn: async () => await getTargets(),
     onError: (err) => console.log(err),
     refetchInterval,
   });
 
+  const { data: targetOrder = [] } = useSetting("ui.target.order");
+
+  const targetsInOrder = useMemo(
+    () =>
+      !targets
+        ? []
+        : targets
+            .map((target, targetIndex) => {
+              const index = targetOrder.findIndex((id) => id === target.id);
+              return {
+                target,
+                order: index === -1 ? targets.length + targetIndex : index,
+              };
+            })
+            .sort((a, b) => a.order - b.order)
+            .map(({ target }) => target),
+    [targets, targetOrder]
+  );
+
   return {
-    targets: data || [],
+    /** Targets in their original order */
+    targets: targets || [],
+    /**
+     * Targets in the order specified by the setting "ui.target.order" including targets
+     * that are not in the setting at the end of the list.
+     */
+    targetsInOrder,
     isFetching: isLoading,
     isSuccess,
     fetchError: error,
@@ -83,7 +117,7 @@ export const useCreateTargetMutation = (
   onError: (err: AxiosError) => void
 ) => {
   const queryClient = useQueryClient();
-  const { isPending, mutate, error } = useMutation({
+  const { isPending, mutate, mutateAsync, error } = useMutation({
     mutationFn: createTarget,
     onSuccess: (res) => {
       onSuccess(res);
@@ -95,6 +129,7 @@ export const useCreateTargetMutation = (
   });
   return {
     mutate,
+    mutateAsync,
     isPending,
     error,
   };