Fix support for invoking bulk actions with flattened params

airhorns · airhorns · commit 5c7342f480e0 · 2023-09-28T19:37:13.000-04:00
Support for invoking bulk actions with flattened params in react was broken, and #267 accidentally broke it further. It was broken since inception because the tests were asserting incorrectly that the produced `input` params weren't nested -- they should have been, oops! Updated the tests and stopped using snapshot tests to describe this so no one is tempted to break it. Then, to fix the issue and properly disambiguate the params, I had to change the behaviour of the disambiguator a little bit. To preserve the behaviour for the change in #267, we need to respect the idea that if an action does not accept model input, we shouldn't do any nesting of variables. This does this by giving the new `action.acceptsModelInput` value precedence over `action.hasCreateOrUpdateEffect`, which was the ancient way of asking this question. That old way keeps support for old API clients, but the new way is more trustworthy. For things like the signUp action, `action.acceptsModelInput` is false, but `action.hasCreateOrUpdateEffect` is true. So, we stop `||`-ing them, and instead give the new thing precedence. This way, for actions with hardcoded param sets like `user.signUp`, we never nest. This fixes the over-eager nesting issue for bulk actions as well.
diff --git a/packages/react/spec/useAction.spec.tsx b/packages/react/spec/useAction.spec.tsx
@@ -457,12 +457,10 @@ describe("useAction", () => {
       expect(promiseResult.error).toBeFalsy();
     });
 
-    expect(variables).toMatchInlineSnapshot(`
-      {
-        "email": "bob@test.com",
-        "password": "password123!",
-      }
-    `);
+    expect(variables).toEqual({
+      email: "bob@test.com",
+      password: "password123!",
+    });
   });
 
   test("should throw if called without a model api identifier and there is an ambiguous field", async () => {
diff --git a/packages/react/spec/useBulkAction.spec.ts b/packages/react/spec/useBulkAction.spec.ts
@@ -102,31 +102,7 @@ describe("useBulkAction", () => {
     expect(result.current[0].error).toBeFalsy();
   });
 
-  test("can execute a bulk create with params", async () => {
-    const mockBulkCreate = {
-      type: "action",
-      operationName: "bulkCreateWidgets",
-      namespace: null,
-      modelApiIdentifier: "widget",
-      modelSelectionField: "widgets",
-      isBulk: true,
-      defaultSelection: {
-        id: true,
-        name: true,
-      },
-      selectionType: {},
-      optionsType: {},
-      schemaType: null,
-      variablesType: void 0,
-      variables: {
-        inputs: {
-          required: true,
-          type: "[BulkCreateWidgetsInput!]",
-        },
-      },
-      hasReturnType: false,
-    } as any;
-
+  test("can execute a bulk create with flattened params", async () => {
     // eslint-disable-next-line @typescript-eslint/ban-ts-comment
     // @ts-ignore waiting for bulk params to be released gadget side
     const { result } = renderHook(() => useBulkAction<any, any, any, any>(mockBulkCreate), { wrapper: MockClientWrapper(bulkExampleApi) });
@@ -141,18 +117,20 @@ describe("useBulkAction", () => {
     expect(result.current[0].error).toBeFalsy();
 
     expect(mockUrqlClient.executeMutation).toBeCalledTimes(1);
-    expect(mockUrqlClient.executeMutation.mock.calls[0][0].variables).toMatchInlineSnapshot(`
-      {
-        "inputs": [
-          {
-            "name": "foo",
+    expect(mockUrqlClient.executeMutation.mock.calls[0][0].variables).toEqual({
+      inputs: [
+        {
+          widget: {
+            name: "foo",
           },
-          {
-            "name": "bar",
+        },
+        {
+          widget: {
+            name: "bar",
           },
-        ],
-      }
-    `);
+        },
+      ],
+    });
 
     mockUrqlClient.executeMutation.pushResponse("bulkCreateWidgets", {
       data: {
@@ -183,30 +161,6 @@ describe("useBulkAction", () => {
   });
 
   test("can execute a bulk create with fully qualified params", async () => {
-    const mockBulkCreate = {
-      type: "action",
-      operationName: "bulkCreateWidgets",
-      namespace: null,
-      modelApiIdentifier: "widget",
-      modelSelectionField: "widgets",
-      isBulk: true,
-      defaultSelection: {
-        id: true,
-        name: true,
-      },
-      selectionType: {},
-      optionsType: {},
-      schemaType: null,
-      variablesType: void 0,
-      variables: {
-        inputs: {
-          required: true,
-          type: "[BulkCreateWidgetsInput!]",
-        },
-      },
-      hasReturnType: false,
-    } as any;
-
     // eslint-disable-next-line @typescript-eslint/ban-ts-comment
     // @ts-ignore waiting for bulk params to be released gadget side
     const { result } = renderHook(() => useBulkAction<any, any, any, any>(mockBulkCreate), { wrapper: MockClientWrapper(bulkExampleApi) });
@@ -221,22 +175,20 @@ describe("useBulkAction", () => {
     expect(result.current[0].error).toBeFalsy();
 
     expect(mockUrqlClient.executeMutation).toBeCalledTimes(1);
-    expect(mockUrqlClient.executeMutation.mock.calls[0][0].variables).toMatchInlineSnapshot(`
-      {
-        "inputs": [
-          {
-            "widget": {
-              "name": "foo",
-            },
+    expect(mockUrqlClient.executeMutation.mock.calls[0][0].variables).toEqual({
+      inputs: [
+        {
+          widget: {
+            name: "foo",
           },
-          {
-            "widget": {
-              "name": "bar",
-            },
+        },
+        {
+          widget: {
+            name: "bar",
           },
-        ],
-      }
-    `);
+        },
+      ],
+    });
 
     mockUrqlClient.executeMutation.pushResponse("bulkCreateWidgets", {
       data: {
@@ -266,40 +218,79 @@ describe("useBulkAction", () => {
     expect(result.current[0].error).toBeFalsy();
   });
 
-  test("can execute a bulk update with params", async () => {
-    const mockBulkUpdate = {
-      type: "action",
-      operationName: "bulkUpdateWidgets",
-      namespace: null,
-      modelApiIdentifier: "widget",
-      modelSelectionField: "widgets",
-      isBulk: true,
-      defaultSelection: {
-        id: true,
-        name: true,
-      },
-      selectionType: {},
-      optionsType: {},
-      schemaType: null,
-      variablesType: void 0,
-      variables: {
-        inputs: {
-          required: true,
-          type: "[BulkUpdateWidgetsInput!]",
+  test("can execute a bulk update with flattened params", async () => {
+    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    // @ts-ignore waiting for bulk params to be released gadget side
+    const { result } = renderHook(() => useBulkAction<any, any, any, any>(mockBulkUpdate), { wrapper: MockClientWrapper(bulkExampleApi) });
+
+    let mutationPromise: any;
+    act(() => {
+      mutationPromise = result.current[1]([
+        { id: "123", name: "foo" },
+        { id: "124", name: "bar" },
+      ]);
+    });
+
+    expect(result.current[0].data).toBeFalsy();
+    expect(result.current[0].fetching).toBe(true);
+    expect(result.current[0].error).toBeFalsy();
+
+    expect(mockUrqlClient.executeMutation).toBeCalledTimes(1);
+    expect(mockUrqlClient.executeMutation.mock.calls[0][0].variables).toEqual({
+      inputs: [
+        {
+          id: "123",
+          widget: {
+            name: "foo",
+          },
+        },
+        {
+          id: "124",
+          widget: {
+            name: "bar",
+          },
+        },
+      ],
+    });
+
+    mockUrqlClient.executeMutation.pushResponse("bulkUpdateWidgets", {
+      data: {
+        bulkUpdateWidgets: {
+          success: true,
+          widgets: [
+            { id: "123", name: "foo" },
+            { id: "124", name: "bar" },
+          ],
         },
       },
-      hasReturnType: false,
-    } as any;
+      stale: false,
+      hasNext: false,
+    });
 
+    await act(async () => {
+      const promiseResult = await mutationPromise;
+      expect(promiseResult.data!.length).toEqual(2);
+      expect(promiseResult.data![0].id).toEqual("123");
+      expect(promiseResult.data![1].id).toEqual("124");
+    });
+
+    expect(result.current[0].data!.length).toEqual(2);
+    expect(result.current[0].data![0].id).toEqual("123");
+    expect(result.current[0].data![1].id).toEqual("124");
+    expect(result.current[0].fetching).toBe(false);
+    expect(result.current[0].error).toBeFalsy();
+  });
+
+  test("can execute a bulk update with fully qualified params", async () => {
     // eslint-disable-next-line @typescript-eslint/ban-ts-comment
     // @ts-ignore waiting for bulk params to be released gadget side
     const { result } = renderHook(() => useBulkAction<any, any, any, any>(mockBulkUpdate), { wrapper: MockClientWrapper(bulkExampleApi) });
 
     let mutationPromise: any;
     act(() => {
       mutationPromise = result.current[1]([
-        { id: "123", name: "foo" },
-        { id: "124", name: "bar" },
+        { id: "123", widget: { name: "foo" } },
+        { id: "124", widget: { name: "bar" } },
       ]);
     });
 
@@ -308,20 +299,22 @@ describe("useBulkAction", () => {
     expect(result.current[0].error).toBeFalsy();
 
     expect(mockUrqlClient.executeMutation).toBeCalledTimes(1);
-    expect(mockUrqlClient.executeMutation.mock.calls[0][0].variables).toMatchInlineSnapshot(`
-      {
-        "inputs": [
-          {
-            "id": "123",
-            "name": "foo",
+    expect(mockUrqlClient.executeMutation.mock.calls[0][0].variables).toEqual({
+      inputs: [
+        {
+          id: "123",
+          widget: {
+            name: "foo",
           },
-          {
-            "id": "124",
-            "name": "bar",
+        },
+        {
+          id: "124",
+          widget: {
+            name: "bar",
           },
-        ],
-      }
-    `);
+        },
+      ],
+    });
 
     mockUrqlClient.executeMutation.pushResponse("bulkUpdateWidgets", {
       data: {
@@ -440,3 +433,53 @@ describe("useBulkAction", () => {
     expect(result.current[0]).toBe(beforeObject);
   });
 });
+
+const mockBulkCreate = {
+  type: "action",
+  operationName: "bulkCreateWidgets",
+  namespace: null,
+  modelApiIdentifier: "widget",
+  modelSelectionField: "widgets",
+  isBulk: true,
+  defaultSelection: {
+    id: true,
+    name: true,
+  },
+  selectionType: {},
+  optionsType: {},
+  schemaType: null,
+  variablesType: void 0,
+  variables: {
+    inputs: {
+      required: true,
+      type: "[BulkCreateWidgetsInput!]",
+    },
+  },
+  acceptsModelInput: true,
+  hasReturnType: false,
+} as any;
+
+const mockBulkUpdate = {
+  type: "action",
+  operationName: "bulkUpdateWidgets",
+  namespace: null,
+  modelApiIdentifier: "widget",
+  modelSelectionField: "widgets",
+  isBulk: true,
+  defaultSelection: {
+    id: true,
+    name: true,
+  },
+  selectionType: {},
+  optionsType: {},
+  schemaType: null,
+  variablesType: void 0,
+  variables: {
+    inputs: {
+      required: true,
+      type: "[BulkUpdateWidgetsInput!]",
+    },
+  },
+  acceptsModelInput: true,
+  hasReturnType: false,
+} as any;
diff --git a/packages/react/src/utils.ts b/packages/react/src/utils.ts
@@ -323,14 +323,12 @@ export const disambiguateActionVariables = (
   }
 
   let newVariables: Record<string, any>;
-  const idVariable = Object.entries(action.variables).find(([key, value]) => key === "id" && value.type === "GadgetID");
 
-  if (action.acceptsModelInput || action.hasCreateOrUpdateEffect) {
+  if (action.acceptsModelInput ?? action.hasCreateOrUpdateEffect) {
     if (
-      (action.modelApiIdentifier in variables &&
-        typeof variables[action.modelApiIdentifier] === "object" &&
-        variables[action.modelApiIdentifier] !== null) ||
-      !action.variables[action.modelApiIdentifier]
+      action.modelApiIdentifier in variables &&
+      typeof variables[action.modelApiIdentifier] === "object" &&
+      variables[action.modelApiIdentifier] != null
     ) {
       newVariables = variables;
     } else {
@@ -341,7 +339,7 @@ export const disambiguateActionVariables = (
         if (action.paramOnlyVariables?.includes(key)) {
           newVariables[key] = value;
         } else {
-          if (idVariable && key === idVariable[0]) {
+          if (key == "id") {
             newVariables.id = value;
           } else {
             newVariables[action.modelApiIdentifier][key] = value;
