Allow official collections inside personal collections (#38823)

* initial stuff

* remove check for sub collections in personal collections

* remove test

* cleanup and adjusting tests

* import spacint

---------

Co-authored-by: Jerry Huang <jhuang37050@gmail.com>

e2e/test/scenarios/organization/official-collections.cy.spec.js

@@ -107,26 +107,35 @@ describeEE("official collections", () => {
       });
     });
 
-    it("should not be able to manage collection authority level for personal collections and their children", () => {
+    it("should be able to manage collection authority level for personal collections and their children (metabase#30236)", () => {
       cy.visit("/collection/root");
 
       openCollection("Your personal collection");
       getCollectionActions().within(() => {
-        cy.icon("ellipsis").should("not.exist");
+        cy.icon("ellipsis").should("exist");
+        cy.icon("ellipsis").click();
       });
 
+      popover().findByText("Make collection official").should("exist");
+
       openNewCollectionItemFlowFor("collection");
       modal().within(() => {
-        assertNoCollectionTypeInput();
+        assertHasCollectionTypeInput();
         cy.findByLabelText("Name").type("Personal collection child");
         cy.button("Create").click();
       });
 
       openCollection("Personal collection child");
 
+      getCollectionActions().within(() => {
+        cy.icon("ellipsis").should("exist");
+        cy.icon("ellipsis").click();
+      });
+      popover().findByText("Make collection official").should("exist");
+
       openNewCollectionItemFlowFor("collection");
       modal().within(() => {
-        assertNoCollectionTypeInput();
+        assertHasCollectionTypeInput();
         cy.icon("close").click();
       });
     });
@@ -263,6 +272,12 @@ function assertNoCollectionTypeInput() {
   cy.findByText("Official").should("not.exist");
 }
 
+function assertHasCollectionTypeInput() {
+  cy.findByText(/Collection type/i).should("exist");
+  cy.findByText("Regular").should("exist");
+  cy.findByText("Official").should("exist");
+}
+
 function assertNoCollectionTypeOption() {
   cy.findByText("Make collection official").should("not.exist");
   cy.findByText("Remove Official badge").should("not.exist");

enterprise/backend/test/metabase_enterprise/content_management/api/collection_test.clj

 (ns metabase-enterprise.content-management.api.collection-test
   (:require
    [clojure.test :refer :all]
-   [metabase.models.collection :as collection]
    [metabase.test :as mt]
    [metabase.util.malli.schema :as ms]
    [toucan2.core :as t2]
@@ -57,15 +56,9 @@
             [:model/Collection {id :id} {:authority_level nil}]
             (is (= "official"
                    (:authority_level (mt/user-http-request :crowberto :put 200 (format "collection/%d" id) {:authority_level "official"}))))
-            (is (= :official (t2/select-one-fn :authority_level :model/Collection id))))
+            (is (= :official (t2/select-one-fn :authority_level :model/Collection id)))))
 
-          (testing "but cannot update for personal collection"
-            (let [personal-coll (collection/user->personal-collection (mt/user->id :crowberto))]
-              (mt/user-http-request :crowberto :put 403 (str "collection/" (:id personal-coll))
-                                    {:authority_level "official"})
-              (is (nil? (t2/select-one-fn :authority_level :model/Collection :id (:id personal-coll)))))))
-
-        (testing "Non-adminds can patch without the :authority_level"
+        (testing "Non-admins can patch without the :authority_level"
           (t2.with-temp/with-temp [:model/Collection collection {:name "whatever" :authority_level "official"}]
             (is (= "official"
                    (-> (mt/user-http-request :rasta :put 200 (str "collection/" (:id collection))

frontend/src/metabase/collections/components/CollectionMenu/CollectionMenu.tsx

@@ -31,13 +31,7 @@ export const CollectionMenu = ({
     isInstanceAnalyticsCustomCollection(collection);
   const canWrite = collection.can_write;
 
-  if (
-    isAdmin &&
-    !isRoot &&
-    !isPersonal &&
-    !isPersonalCollectionChild &&
-    canWrite
-  ) {
+  if (isAdmin && !isRoot && canWrite) {
     items.push(
       ...PLUGIN_COLLECTIONS.getAuthorityLevelMenuItems(
         collection,

frontend/src/metabase/collections/components/CollectionMenu/tests/premium.unit.spec.tsx

 import userEvent from "@testing-library/user-event";
 
-import { getIcon, queryIcon, screen } from "__support__/ui";
+import { getIcon, screen } from "__support__/ui";
 import {
   createMockCollection,
   createMockTokenFeatures,
@@ -82,7 +82,7 @@ describe("CollectionMenu", () => {
     ).not.toBeInTheDocument();
   });
 
-  it("should not be able to make the collection official if it's a personal collection", () => {
+  it("should be able to make the collection official if it's a personal collection", async () => {
     const collection = createMockCollection({
       personal_owner_id: 1,
       can_write: true,
@@ -92,7 +92,10 @@ describe("CollectionMenu", () => {
       isAdmin: true,
     });
 
-    expect(queryIcon("ellipsis")).not.toBeInTheDocument();
+    userEvent.click(getIcon("ellipsis"));
+    expect(
+      await screen.findByText("Make collection official"),
+    ).toBeInTheDocument();
   });
 
   it("should not be able to make the collection official if it's a personal collection child", () => {

frontend/src/metabase/collections/containers/FormAuthorityLevelFieldContainer.tsx

-import { useMemo } from "react";
 import { connect } from "react-redux";
 import _ from "underscore";
 
-import { canManageCollectionAuthorityLevel } from "metabase/collections/utils";
 import Collections from "metabase/entities/collections";
 import { PLUGIN_COLLECTION_COMPONENTS } from "metabase/plugins";
 import { getUserIsAdmin } from "metabase/selectors/user";
 import type { Collection } from "metabase-types/api";
 import type { State } from "metabase-types/store";
 
-type CollectionsMap = Partial<Record<Collection["id"], Collection>>;
-
 interface OwnProps {
   collectionParentId: Collection["id"];
 }
 
 interface StateProps {
   isAdmin: boolean;
-  collectionsMap: CollectionsMap;
 }
 
 type FormAuthorityLevelFieldContainerProps = OwnProps & StateProps;
 
 function mapStateToProps(state: State): StateProps {
-  const { collections } = state.entities;
   return {
     isAdmin: getUserIsAdmin(state),
-    collectionsMap: collections || ({} as CollectionsMap),
   };
 }
 
 function FormAuthorityLevelFieldContainer({
-  collectionParentId,
-  collectionsMap,
   isAdmin,
 }: FormAuthorityLevelFieldContainerProps) {
-  const canManageAuthorityLevel = useMemo(
-    () =>
-      isAdmin &&
-      canManageCollectionAuthorityLevel(
-        { parent_id: collectionParentId },
-        collectionsMap,
-      ),
-    [collectionParentId, collectionsMap, isAdmin],
-  );
-
-  if (!canManageAuthorityLevel) {
+  if (!isAdmin) {
     return null;
   }
 

frontend/src/metabase/collections/utils.ts

 import { t } from "ttag";
 
-import { isNotNull } from "metabase/lib/types";
 import { PLUGIN_COLLECTIONS } from "metabase/plugins";
 import type {
   Collection,
@@ -209,32 +208,3 @@ export function isValidCollectionId(
   const id = canonicalCollectionId(collectionId);
   return id === null || typeof id === "number";
 }
-
-function isPersonalOrPersonalChild(
-  collection: Collection,
-  collections: Collection[],
-) {
-  if (!collection) {
-    return false;
-  }
-  return (
-    isRootPersonalCollection(collection) ||
-    isPersonalCollectionChild(collection, collections)
-  );
-}
-
-export function canManageCollectionAuthorityLevel(
-  collection: Partial<Collection>,
-  collectionMap: Partial<Record<CollectionId, Collection>>,
-) {
-  if (isRootPersonalCollection(collection)) {
-    return false;
-  }
-  const parentId = coerceCollectionId(collection.parent_id);
-  const parentCollection = collectionMap[parentId];
-  const collections = Object.values(collectionMap).filter(isNotNull);
-  return (
-    parentCollection &&
-    !isPersonalOrPersonalChild(parentCollection, collections)
-  );
-}

src/metabase/api/collection.clj

@@ -966,11 +966,7 @@
     (when (and (contains? collection-updates :authority_level)
                (not= (keyword authority_level) (:authority_level collection-before-update)))
       (premium-features/assert-has-feature :official-collections (tru "Official Collections"))
-      (api/check-403 (and api/*is-superuser?*
-                          ;; pre-update of model checks if the collection is a personal collection and rejects changes
-                          ;; to authority_level, but it doesn't check if it is a sub-collection of a personal one so we add that
-                          ;; here
-                          (not (collection/is-personal-collection-or-descendant-of-one? collection-before-update)))))
+      (api/check-403 api/*is-superuser?*))
     ;; ok, go ahead and update it! Only update keys that were specified in the `body`. But not `parent_id` since
     ;; that's not actually a property of Collection, and since we handle moving a Collection separately below.
     (let [updates (u/select-keys-when collection-updates :present [:name :description :archived :authority_level])]