Skip fetching collection permissions graph on update (#45561)

* updates the fe to skip refetching the graph when updating the collection graph, adds tests

* Moves the skip_graph flag to be part of the body.

- the FE tooling makes it awkward for PUTs to include query params

* fixes failing unit tests

* pr feedback

---------

Co-authored-by: Bryan Maass <bryan.maass@gmail.com>

e2e/test/scenarios/native/snippets.cy.spec.js

@@ -408,12 +408,12 @@ describeEE("scenarios > question > snippets (EE)", () => {
       });
 
       // API check
-      cy.get("@updatePermissions").then(intercept => {
-        const { groups } = intercept.response.body;
-        const allUsers = groups[ALL_USERS_GROUP];
-
-        expect(allUsers.root).to.equal("write");
-      });
+      cy.request("GET", "/api/collection/graph?namespace=snippets").then(
+        ({ body }) => {
+          const allUsers = body.groups[ALL_USERS_GROUP];
+          expect(allUsers.root).to.equal("write");
+        },
+      );
     });
   });
 });

e2e/test/scenarios/permissions/admin-permissions.cy.spec.js

@@ -642,34 +642,90 @@ describe("scenarios > admin > permissions", () => {
     cy.signInAsAdmin();
   });
 
-  it("partial data permission updates should not remove permissions from other unmodified groups", () => {
-    // check the we have an expected initial state
-    cy.visit(`admin/permissions/data/group/${DATA_GROUP}`);
-    assertPermissionTable([["Sample Database", "Query builder and native"]]);
-
-    // make a change to the permissions of another group
-    selectSidebarItem("nosql");
-    assertPermissionTable([["Sample Database", "Query builder only"]]);
-    modifyPermission("Sample Database", NATIVE_QUERIES_PERMISSION_INDEX, "No");
-
-    // observe the save change request and assert that we don't get back
-    // values for groups we did not modify
-    cy.intercept("PUT", "/api/permissions/graph").as("updateGraph");
-
-    // save changes
-    cy.button("Save changes").click();
-    modal().within(() => {
-      cy.button("Yes").click();
-    });
+  context("partial updates", () => {
+    it("partial data permission updates should not remove permissions from other unmodified groups", () => {
+      // check the we have an expected initial state
+      cy.visit(`admin/permissions/data/group/${DATA_GROUP}`);
+      assertPermissionTable([["Sample Database", "Query builder and native"]]);
+
+      // make a change to the permissions of another group
+      selectSidebarItem("nosql");
+      assertPermissionTable([["Sample Database", "Query builder only"]]);
+      modifyPermission(
+        "Sample Database",
+        NATIVE_QUERIES_PERMISSION_INDEX,
+        "No",
+      );
+
+      // observe the save change request and assert that we don't get back
+      // values for groups we did not modify
+      cy.intercept("PUT", "/api/permissions/graph").as("updateGraph");
+
+      // save changes
+      cy.button("Save changes").click();
+      modal().within(() => {
+        cy.button("Yes").click();
+      });
+
+      cy.wait("@updateGraph").then(interception => {
+        const requestGroupIds = Object.keys(interception.request.body.groups);
+        const responseGroupIds = Object.keys(interception.response.body.groups);
+        expect(requestGroupIds).to.deep.equal(responseGroupIds);
+      });
 
-    cy.wait("@updateGraph").then(interception => {
-      const requestGroupIds = Object.keys(interception.request.body.groups);
-      const responseGroupIds = Object.keys(interception.response.body.groups);
-      expect(requestGroupIds).to.deep.equal(responseGroupIds);
+      // make sure that our other group's permission data did not get changed
+      selectSidebarItem("data");
+      assertPermissionTable([["Sample Database", "Query builder and native"]]);
     });
 
-    // make sure that our other group's permission data did not get changed
-    selectSidebarItem("data");
-    assertPermissionTable([["Sample Database", "Query builder and native"]]);
+    it("partial collection permission updates should not prevent user from making further changes", () => {
+      cy.visit("/admin/permissions/collections");
+
+      selectSidebarItem("First collection");
+
+      modifyPermission(
+        "All Users",
+        COLLECTION_ACCESS_PERMISSION_INDEX,
+        "View",
+        true,
+      );
+
+      cy.intercept("PUT", "/api/collection/graph").as("updateGraph");
+
+      cy.button("Save changes").click();
+      modal().within(() => {
+        cy.button("Yes").click();
+      });
+
+      cy.wait("@updateGraph").then(interception => {
+        cy.log("should skip graph in request and response");
+        expect(interception.request.body.skip_graph).to.equal(true);
+        expect(interception.response.body).to.not.haveOwnProperty("groups");
+      });
+
+      selectSidebarItem("First collection");
+
+      modifyPermission(
+        "nosql",
+        COLLECTION_ACCESS_PERMISSION_INDEX,
+        "Curate",
+        true,
+      );
+
+      cy.button("Save changes").click();
+      modal().within(() => {
+        cy.button("Yes").click();
+      });
+
+      cy.wait("@updateGraph").then(interception => {
+        cy.log("should not send previously saved edits");
+        expect(interception.request.body.groups).to.not.haveOwnProperty(
+          USER_GROUPS.ALL_USERS_GROUP,
+        );
+
+        cy.log("should not fail when making multiple rounds of edits");
+        expect(interception.response.statusCode).to.equal(200);
+      });
+    });
   });
 });

frontend/src/metabase-types/api/permissions.ts

@@ -102,7 +102,7 @@ export type CollectionPermissionsGraph = {
 };
 
 export type CollectionPermissions = {
-  [key: GroupId]: Partial<Record<CollectionId, CollectionPermission>>;
+  [key: GroupId | string]: Partial<Record<CollectionId, CollectionPermission>>;
 };
 
 export type CollectionPermission = "write" | "read" | "none";

frontend/src/metabase/admin/permissions/pages/CollectionPermissionsPage/tests/common.unit.spec.tsx

@@ -99,12 +99,12 @@ describe("Admin > CollectionPermissionsPage", () => {
       expect(lastRequest).toEqual({
         ...defaultPermissionsGraph,
         groups: {
-          ...defaultPermissionsGraph.groups,
           3: {
             ...defaultPermissionsGraph.groups[3],
             3: "read",
           },
         },
+        skip_graph: true,
       });
     });
 
@@ -155,13 +155,13 @@ describe("Admin > CollectionPermissionsPage", () => {
       expect(lastRequest).toEqual({
         ...defaultPermissionsGraph,
         groups: {
-          ...defaultPermissionsGraph.groups,
           3: {
             ...defaultPermissionsGraph.groups[3],
             1: "write",
             3: "write",
           },
         },
+        skip_graph: true,
       });
     });
 

frontend/src/metabase/admin/permissions/pages/CollectionPermissionsPage/tests/enterprise.unit.spec.tsx

@@ -110,12 +110,12 @@ describe("Admin > CollectionPermissionsPage (enterprise)", () => {
       expect(lastRequest).toEqual({
         ...iaPermissionsGraph,
         groups: {
-          ...iaPermissionsGraph.groups,
           1: {
             ...iaPermissionsGraph.groups[1],
             13371337: "none",
           },
         },
+        skip_graph: true,
       });
     });
   });

frontend/src/metabase/admin/permissions/permissions.js

@@ -33,6 +33,7 @@ import { DataPermissionType, DataPermission } from "./types";
 import { isDatabaseEntityId } from "./utils/data-entity-id";
 import {
   getModifiedGroupsPermissionsGraphParts,
+  getModifiedCollectionPermissionsGraphParts,
   mergeGroupsPermissionsUpdates,
 } from "./utils/graph/partial-updates";
 
@@ -239,14 +240,29 @@ export const saveCollectionPermissions = createThunkAction(
   SAVE_COLLECTION_PERMISSIONS,
   namespace => async (_dispatch, getState) => {
     MetabaseAnalytics.trackStructEvent("Permissions", "save");
-    const { collectionPermissions, collectionPermissionsRevision } =
-      getState().admin.permissions;
+
+    const {
+      originalCollectionPermissions,
+      collectionPermissions,
+      collectionPermissionsRevision,
+    } = getState().admin.permissions;
+
+    const modifiedPermissions = getModifiedCollectionPermissionsGraphParts(
+      originalCollectionPermissions,
+      collectionPermissions,
+    );
+
     const result = await CollectionsApi.updateGraph({
       namespace,
       revision: collectionPermissionsRevision,
-      groups: collectionPermissions,
+      groups: modifiedPermissions,
+      skip_graph: true,
     });
-    return result;
+
+    return {
+      ...result,
+      groups: collectionPermissions,
+    };
   },
 );
 

frontend/src/metabase/admin/permissions/utils/graph/partial-updates.ts

 import _ from "underscore";
 
-import type { GroupsPermissions } from "metabase-types/api";
+import type {
+  CollectionPermissions,
+  GroupsPermissions,
+} from "metabase-types/api";
 
 // utils for dealing with partial graph updates
 
@@ -49,3 +52,16 @@ export function mergeGroupsPermissionsUpdates(
 
   return Object.fromEntries(latestPermissionsEntries);
 }
+
+export function getModifiedCollectionPermissionsGraphParts(
+  originalCollectionPermissions: CollectionPermissions,
+  collectionPermissions: CollectionPermissions,
+) {
+  const groupIds = Object.keys(collectionPermissions);
+  const modifiedGroupIds = groupIds.filter(groupId => {
+    const originalPerms = originalCollectionPermissions[groupId];
+    const currPerms = collectionPermissions[groupId];
+    return !_.isEqual(currPerms, originalPerms);
+  });
+  return _.pick(collectionPermissions, modifiedGroupIds);
+}

frontend/src/metabase/admin/permissions/utils/graph/partial-updates.unit.spec.ts

@@ -6,6 +6,7 @@ import { DataPermission, DataPermissionValue } from "../../types";
 
 import {
   getModifiedGroupsPermissionsGraphParts,
+  getModifiedCollectionPermissionsGraphParts,
   mergeGroupsPermissionsUpdates,
 } from "./partial-updates";
 
@@ -106,3 +107,25 @@ describe("mergeGroupsPermissionsUpdates", () => {
     );
   });
 });
+
+describe("getModifiedCollectionPermissionsGraphParts", () => {
+  it("should only include groups that have had data permission updated", async () => {
+    const simpleUpdate = getModifiedCollectionPermissionsGraphParts(
+      {
+        "1": { "1": "write", "2": "write" },
+        "2": { "1": "write", "2": "write" },
+        "3": { "1": "write", "2": "write" },
+      },
+      {
+        "1": { "1": "write", "2": "write" },
+        "2": { "1": "read", "2": "read" },
+        "3": { "1": "write", "2": "write" },
+      },
+    );
+
+    // should not contain groups that have not been modified
+    expect(simpleUpdate).not.toHaveProperty("1");
+    expect(simpleUpdate).not.toHaveProperty("3");
+    expect(simpleUpdate).toEqual({ "2": { "1": "read", "2": "read" } });
+  });
+});

src/metabase/api/collection.clj

@@ -1260,8 +1260,7 @@
   Will overwrite parts of the graph that are present in the request, and leave the rest unchanged.
 
   If the `skip_graph` query parameter is true, it will only return the current revision"
-  [:as {{:keys [namespace revision groups]} :body
-        {:strs [skip_graph]} :query-params}]
+  [:as {{:keys [namespace revision groups skip_graph]} :body}]
   {namespace [:maybe ms/NonBlankString]
    revision  ms/Int
    groups :map

test/metabase/api/collection_test.clj

@@ -2273,6 +2273,8 @@
   (is (malli= [:map {:closed true} [:revision :int]]
               (mt/user-http-request :crowberto
                                     :put 200
-                                    "collection/graph?skip_graph=true"
-                                    {:revision (c-perm-revision/latest-id) :groups {}}))
+                                    "collection/graph"
+                                    {:revision (c-perm-revision/latest-id)
+                                     :groups {}
+                                     :skip_graph true}))
       "PUTs with skip_graph should not return the coll permission graph."))