Fix inability to change field schema (#18385)

* test

* Fix inability to change field schema

* include db name for diff

* unskip repro

frontend/src/metabase/admin/permissions/components/CollectionPermissionsModal/CollectionPermissionsModal.jsx

@@ -19,7 +19,6 @@ import Groups from "metabase/entities/groups";
 import { PermissionsTable } from "../PermissionsTable";
 import { permissionEditorPropTypes } from "../PermissionsEditor";
 import {
-  getDiff,
   getIsDirty,
   getCollectionsPermissionEditor,
 } from "../../selectors/collection-permissions";
@@ -52,7 +51,6 @@ const mapStateToProps = (state, props) => {
     collectionsList: Collections.selectors.getList(state, {
       entityQuery: { tree: true },
     }),
-    diff: getDiff(state, props),
     isDirty: getIsDirty(state, props),
   };
 };

frontend/src/metabase/admin/permissions/components/PermissionsPageLayout/PermissionsEditBar.jsx

@@ -28,7 +28,7 @@ export function PermissionsEditBar({
     <Confirm
       title={t`Save permissions?`}
       action={onSave}
-      content={<PermissionsConfirm diff={diff} />}
+      content={diff ? <PermissionsConfirm diff={diff} /> : null}
       triggerClasses={cx({ disabled: !isDirty })}
       key="save"
     >

frontend/src/metabase/admin/permissions/pages/CollectionPermissionsPage/CollectionPermissionsPage.jsx

@@ -25,7 +25,6 @@ import {
   getCollectionsPermissionEditor,
   getCollectionEntity,
   getIsDirty,
-  getDiff,
 } from "../../selectors/collection-permissions";
 import {
   PermissionsSidebar,
@@ -45,7 +44,6 @@ const mapStateToProps = (state, props) => {
     sidebar: getCollectionsSidebar(state, props),
     permissionEditor: getCollectionsPermissionEditor(state, props),
     isDirty: getIsDirty(state, props),
-    diff: getDiff(state, props),
     collection: getCollectionEntity(state, props),
   };
 };
@@ -61,7 +59,6 @@ const propTypes = {
   navigateToItem: PropTypes.func.isRequired,
   updateCollectionPermission: PropTypes.func.isRequired,
   isDirty: PropTypes.bool,
-  diff: PropTypes.object,
   savePermissions: PropTypes.func.isRequired,
   loadPermissions: PropTypes.func.isRequired,
   initialize: PropTypes.func.isRequired,
@@ -73,7 +70,6 @@ function CollectionsPermissionsPage({
   permissionEditor,
   collection,
   isDirty,
-  diff,
   savePermissions,
   loadPermissions,
   updateCollectionPermission,
@@ -100,7 +96,6 @@ function CollectionsPermissionsPage({
   return (
     <PermissionsPageLayout
       tab="collections"
-      diff={diff}
       isDirty={isDirty}
       route={route}
       onSave={savePermissions}

frontend/src/metabase/admin/permissions/selectors/collection-permissions.js

@@ -4,7 +4,6 @@ import { getIn } from "icepick";
 import _ from "underscore";
 
 import Group from "metabase/entities/groups";
-import { diffPermissions } from "metabase/lib/permissions";
 import Collections, {
   getCollectionIcon,
   ROOT_COLLECTION,
@@ -24,14 +23,6 @@ export const getIsDirty = createSelector(
     JSON.stringify(permissions) !== JSON.stringify(originalPermissions),
 );
 
-export const getDiff = createSelector(
-  Group.selectors.getList,
-  state => state.admin.permissions.collectionPermissions,
-  state => state.admin.permissions.originalCollectionPermissions,
-  (groups, permissions, originalPermissions) =>
-    diffPermissions(permissions, originalPermissions, groups),
-);
-
 export const getCurrentCollectionId = (_state, props) =>
   props.params.collectionId === ROOT_COLLECTION.id
     ? ROOT_COLLECTION.id

frontend/src/metabase/admin/permissions/selectors/data-permissions.js

@@ -19,7 +19,7 @@ import {
   getNativePermission,
   getSchemasPermission,
   getTablesPermission,
-  diffPermissions,
+  diffDataPermissions,
   isRestrictivePermission,
 } from "metabase/lib/permissions";
 import {
@@ -50,6 +50,30 @@ import {
 } from "../utils/urls";
 import { limitDatabasePermission } from "../permissions";
 
+export const getDatabasesWithTables = createSelector(
+  state => state.entities.databases,
+  state => state.entities.tables,
+  (databases, tables) => {
+    if (!databases || !tables) {
+      return [];
+    }
+    const databasesList = Object.values(databases);
+    const tablesList = Object.values(tables);
+
+    return databasesList.map(database => {
+      const databaseTables = tablesList.filter(
+        table => table.db_id === database.id,
+      );
+
+      return {
+        id: database.id,
+        name: database.name,
+        tables: databaseTables,
+      };
+    });
+  },
+);
+
 const getGroupsWithoutMetabot = createSelector(
   Group.selectors.getList,
   groups => groups?.filter(group => !isMetaBotGroup(group)) ?? [],
@@ -63,12 +87,12 @@ export const getIsDirty = createSelector(
 );
 
 export const getDiff = createSelector(
-  getMetadataWithHiddenTables,
+  getDatabasesWithTables,
   getGroupsWithoutMetabot,
   state => state.admin.permissions.dataPermissions,
   state => state.admin.permissions.originalDataPermissions,
-  (metadata, groups, permissions, originalPermissions) =>
-    diffPermissions(permissions, originalPermissions, groups, metadata),
+  (databases, groups, permissions, originalPermissions) =>
+    diffDataPermissions(permissions, originalPermissions, groups, databases),
 );
 
 export const getIsLoadingDatabaseTables = (state, props) => {

frontend/src/metabase/lib/permissions.js

@@ -476,10 +476,10 @@ function diffGroupPermissions(
   newPerms: GroupsPermissions,
   oldPerms: GroupsPermissions,
   groupId: GroupId,
-  metadata: Metadata,
+  databases: Array<Object>,
 ): GroupPermissionsDiff {
   const groupDiff: GroupPermissionsDiff = { databases: {} };
-  for (const database of metadata.databasesList()) {
+  for (const database of databases) {
     groupDiff.databases[database.id] = diffDatabasePermissions(
       newPerms,
       oldPerms,
@@ -495,20 +495,20 @@ function diffGroupPermissions(
   return groupDiff;
 }
 
-export function diffPermissions(
+export function diffDataPermissions(
   newPerms: GroupsPermissions,
   oldPerms: GroupsPermissions,
   groups: Array<Group>,
-  metadata: Metadata,
+  databases: Array<Object>,
 ): PermissionsDiff {
   const permissionsDiff: PermissionsDiff = { groups: {} };
-  if (newPerms && oldPerms && metadata) {
+  if (newPerms && oldPerms && databases) {
     for (const group of groups) {
       permissionsDiff.groups[group.id] = diffGroupPermissions(
         newPerms,
         oldPerms,
         group.id,
-        metadata,
+        databases,
       );
       deleteIfEmpty(permissionsDiff.groups, group.id);
       if (permissionsDiff.groups[group.id]) {

frontend/src/metabase/selectors/metadata.js

@@ -99,6 +99,8 @@ export const getMetadata = createSelector(
     meta.segments = copyObjects(meta, segments, instantiateSegment);
     meta.metrics = copyObjects(meta, metrics, instantiateMetric);
 
+    // database
+    hydrateList(meta.databases, "tables", meta.tables);
     // schema
     hydrate(meta.schemas, "database", s => meta.database(s.database));
     // table
@@ -108,16 +110,6 @@ export const getMetadata = createSelector(
     hydrate(meta.tables, "db", t => meta.database(t.db_id || t.db));
     hydrate(meta.tables, "schema", t => meta.schema(t.schema));
 
-    hydrate(meta.databases, "tables", database => {
-      if (database.tables?.length > 0) {
-        return database.tables.map(tableId => meta.table(tableId));
-      }
-
-      return Object.values(meta.tables).filter(
-        table => table.schema && table.db_id === database.id,
-      );
-    });
-
     // NOTE: special handling for schemas
     // This is pretty hacky
     // hydrateList(meta.databases, "schemas", meta.schemas);

frontend/test/metabase/scenarios/admin/datamodel/reproductions/18384-field-settings-breaks-ui.cy.spec.js

@@ -3,7 +3,7 @@ import { SAMPLE_DATASET } from "__support__/e2e/cypress_sample_dataset";
 
 const { PEOPLE_ID, PEOPLE, REVIEWS_ID } = SAMPLE_DATASET;
 
-describe.skip("issue 18384", () => {
+describe("issue 18384", () => {
   beforeEach(() => {
     restore();
     cy.signInAsAdmin();