feat: bulk save enhancements

- Warn when an item was saved but is not reloaded by the save response - Don't perform ref fixup on models that have a PK and aren't dirty - Make tests more strict by using exact array equality assertions on the payload
IntelliTect · Jun 19, 2024 · 6c10133 · 6c10133
1 parent fb977ab
commit 6c10133
Show file tree

Hide file tree

Showing 2 changed files with 101 additions and 26 deletions.
diff --git a/src/coalesce-vue/src/viewmodel.ts b/src/coalesce-vue/src/viewmodel.ts
@@ -681,7 +681,12 @@ export abstract class ViewModel<
 
                 nextModels.push(principal);
 
-                if (principal && !principal._isRemoved && !model._isRemoved) {
+                if (
+                  principal &&
+                  !principal._isRemoved &&
+                  !model._isRemoved &&
+                  (model.$isDirty || !model._existsOnServer)
+                ) {
                   if (
                     // The principal is not yet saved, but will be saved.
                     (!principal._existsOnServer &&
@@ -822,10 +827,32 @@ export abstract class ViewModel<
 
           const result = ret.data.object;
           if (result) {
+            const age = performance.now();
+
             // `purgeUnsaved = true` here (arg3) since the bulk save should have covered the entire object graph.
             // Wiping out unsaved items will help developers discover bugs where their root model's data source
             // does not correctly include the whole object graph that they're saving (which is expected for bulk saves)
-            this.$loadFromModel(result, performance.now(), true);
+            this.$loadFromModel(result, age, true);
+
+            const unloadedTypes = new Set(
+              dataToSend
+                .filter(
+                  (d) =>
+                    d.action == "save" &&
+                    d.model._dataAge != age &&
+                    // We currently don't have a good way to reload additionalRoots items.
+                    !options?.additionalRoots?.includes(d.model)
+                )
+                .map((d) => d.model.$metadata.name)
+            );
+
+            if (unloadedTypes.size > 0) {
+              console.warn(
+                `One or more ${[...unloadedTypes.values()].join(
+                  " and "
+                )} items were saved by a bulk save, but were not returned by the response. The Data Source of the bulk save target may not be returning all entities.`
+              );
+            }
           }
         }
 

diff --git a/src/coalesce-vue/test/viewmodel.spec.ts b/src/coalesce-vue/test/viewmodel.spec.ts
@@ -678,7 +678,7 @@ describe("ViewModel", () => {
       await student.$bulkSave();
 
       expect(JSON.parse(endpoint.mock.calls[0][0].data)).toMatchObject({
-        items: expect.arrayContaining([
+        items: [
           {
             action: "save",
             type: "Student",
@@ -689,6 +689,12 @@ describe("ViewModel", () => {
             },
             root: true,
           },
+          {
+            action: "save",
+            type: "Advisor",
+            data: { advisorId: null, name: "bob" },
+            refs: { advisorId: originalAdvisor.$stableId },
+          },
           {
             action: "save",
             type: "Course",
@@ -698,12 +704,6 @@ describe("ViewModel", () => {
               studentId: student.$stableId,
             },
           },
-          {
-            action: "save",
-            type: "Advisor",
-            data: { advisorId: null, name: "bob" },
-            refs: { advisorId: originalAdvisor.$stableId },
-          },
           {
             action: "save",
             type: "Student",
@@ -713,7 +713,7 @@ describe("ViewModel", () => {
               studentAdvisorId: originalAdvisor.$stableId,
             },
           },
-        ]),
+        ],
       });
 
       // Preserves non-circular instances:
@@ -773,7 +773,7 @@ describe("ViewModel", () => {
       });
 
       expect(JSON.parse(endpoint.mock.calls[0][0].data)).toMatchObject({
-        items: expect.arrayContaining([
+        items: [
           {
             action: "save",
             type: "Student",
@@ -785,7 +785,7 @@ describe("ViewModel", () => {
             },
             root: true,
           },
-        ]),
+        ],
       });
 
       // Preserves non-circular instances:
@@ -841,7 +841,7 @@ describe("ViewModel", () => {
       expect(student.$removedItems?.length).toBeFalsy();
 
       expect(JSON.parse(bulkSaveEndpoint.mock.calls[0][0].data)).toMatchObject({
-        items: expect.arrayContaining([
+        items: [
           {
             action: "none",
             type: "Student",
@@ -861,7 +861,7 @@ describe("ViewModel", () => {
             data: { advisorId: 3 },
             refs: { advisorId: advisor.$stableId },
           },
-        ]),
+        ],
       });
 
       loadEndpoint.destroy();
@@ -890,7 +890,7 @@ describe("ViewModel", () => {
       await student.$bulkSave();
 
       expect(JSON.parse(bulkSaveEndpoint.mock.calls[0][0].data)).toMatchObject({
-        items: expect.arrayContaining([
+        items: [
           {
             action: "save",
             type: "Student",
@@ -913,7 +913,7 @@ describe("ViewModel", () => {
             },
             refs: { advisorId: advisor.$stableId },
           },
-        ]),
+        ],
       });
 
       bulkSaveEndpoint.destroy();
@@ -964,15 +964,15 @@ describe("ViewModel", () => {
       await student.$bulkSave();
 
       expect(JSON.parse(bulkSaveEndpoint.mock.calls[0][0].data)).toMatchObject({
-        items: expect.arrayContaining([
+        items: [
           {
             action: "save",
             type: "Student",
             data: { studentId: null },
             refs: { studentId: student.$stableId },
             root: true,
           },
-        ]),
+        ],
       });
 
       bulkSaveEndpoint.destroy();
@@ -1023,7 +1023,7 @@ describe("ViewModel", () => {
       await student.$bulkSave();
 
       expect(JSON.parse(endpoint.mock.calls[0][0].data)).toMatchObject({
-        items: expect.arrayContaining([
+        items: [
           {
             action: "none",
             type: "Student",
@@ -1041,7 +1041,7 @@ describe("ViewModel", () => {
               studentId: originalSteve.$stableId,
             },
           },
-        ]),
+        ],
       });
 
       expect(student.$bulkSave.wasSuccessful).toBeTruthy();
@@ -1070,7 +1070,7 @@ describe("ViewModel", () => {
 
       // Assert
       expect(JSON.parse(endpoint.mock.calls[0][0].data)).toMatchObject({
-        items: expect.arrayContaining([
+        items: [
           {
             action: "none",
             type: "Company",
@@ -1092,7 +1092,55 @@ describe("ViewModel", () => {
               companyId: parent.$stableId,
             },
           },
-        ]),
+        ],
+      });
+
+      endpoint.destroy();
+    });
+
+    test("existing nondirty child of existing parent without nav/fk on child does not save", async () => {
+      // SCENARIO: A new child object gets saved by virtue of existing in a child collection.
+      // However, this child object isn't reloaded by the response of the save.
+
+      const endpoint = mockEndpoint(
+        "/Company/bulkSave",
+        vitest.fn((req) => ({
+          wasSuccessful: true,
+        }))
+      );
+
+      const parent = new CompanyViewModel();
+      parent.$loadCleanData({
+        companyId: 1,
+        name: "existing parent",
+        employees: [{ personId: 1, firstName: "bob" }],
+      });
+
+      // Sanity check: The employee lacks a reference nav or FK to its parent
+      // but is not dirty and does have a PK.
+      parent.employees![0].company = null;
+      parent.employees![0].companyId = null;
+      parent.employees![0].$isDirty = false;
+
+      // Act
+      await parent.$bulkSave();
+
+      // Assert
+      expect(JSON.parse(endpoint.mock.calls[0][0].data)).toMatchObject({
+        items: [
+          {
+            action: "none",
+            type: "Company",
+            data: { companyId: parent.companyId },
+            refs: {
+              companyId: parent.$stableId,
+            },
+            root: true,
+          },
+          // There should NOT be a "Person" model in the payload,
+          // since despite missing a foreign key to its parent,
+          // it isn't dirty and does exist on the server.
+        ],
       });
 
       endpoint.destroy();
@@ -1116,7 +1164,7 @@ describe("ViewModel", () => {
 
       // Assert
       expect(JSON.parse(endpoint.mock.calls[0][0].data)).toMatchObject({
-        items: expect.arrayContaining([
+        items: [
           {
             action: "save",
             type: "Company",
@@ -1134,7 +1182,7 @@ describe("ViewModel", () => {
               companyId: company2.$stableId,
             },
           },
-        ]),
+        ],
       });
 
       endpoint.destroy();
@@ -1164,7 +1212,7 @@ describe("ViewModel", () => {
       await student.$bulkSave();
 
       expect(JSON.parse(bulkSaveEndpoint.mock.calls[0][0].data)).toMatchObject({
-        items: expect.arrayContaining([
+        items: [
           {
             action: "save",
             type: "Student",
@@ -1179,7 +1227,7 @@ describe("ViewModel", () => {
             },
             root: true,
           },
-        ]),
+        ],
       });
 
       bulkSaveEndpoint.destroy();