From 4575994675cf59c3b23cb1d27aa13dc8c76d1908 Mon Sep 17 00:00:00 2001
From: Ryan Laurie <30528226+iethree@users.noreply.github.com>
Date: Thu, 29 Jun 2023 12:36:45 -0600
Subject: [PATCH] Require table primary key to enable basic actions (#31929)

* require table primary key to enable basic actions

* add unit tests

* update model detail page test metadata
---
 frontend/src/metabase-lib/Question.ts         | 11 ++-
 .../ModelDetailPage.unit.spec.tsx             |  1 +
 .../metabase-lib/lib/Question.unit.spec.js    | 71 +++++++++++++++++++
 3 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/frontend/src/metabase-lib/Question.ts b/frontend/src/metabase-lib/Question.ts
index dd219561050..0a8fd1d499c 100644
--- a/frontend/src/metabase-lib/Question.ts
+++ b/frontend/src/metabase-lib/Question.ts
@@ -486,7 +486,16 @@ class QuestionInner {
 
   supportsImplicitActions(): boolean {
     const query = this.query();
-    return query instanceof StructuredQuery && !query.hasAnyClauses();
+
+    // we want to check the metadata for the underlying table, not the model
+    const table = query.sourceTable();
+
+    const hasSinglePk =
+      table?.fields?.filter(field => field.isPK())?.length === 1;
+
+    return (
+      query instanceof StructuredQuery && !query.hasAnyClauses() && hasSinglePk
+    );
   }
 
   canAutoRun(): boolean {
diff --git a/frontend/src/metabase/models/containers/ModelDetailPage/ModelDetailPage.unit.spec.tsx b/frontend/src/metabase/models/containers/ModelDetailPage/ModelDetailPage.unit.spec.tsx
index 4997bac9d95..430bdbdd2a0 100644
--- a/frontend/src/metabase/models/containers/ModelDetailPage/ModelDetailPage.unit.spec.tsx
+++ b/frontend/src/metabase/models/containers/ModelDetailPage/ModelDetailPage.unit.spec.tsx
@@ -76,6 +76,7 @@ const TEST_TABLE_ID = 1;
 const TEST_FIELD = createMockField({
   id: 1,
   display_name: "Field 1",
+  semantic_type: TYPE.PK,
   table_id: TEST_TABLE_ID,
 });
 
diff --git a/frontend/test/metabase-lib/lib/Question.unit.spec.js b/frontend/test/metabase-lib/lib/Question.unit.spec.js
index 5434e205293..10d65595738 100644
--- a/frontend/test/metabase-lib/lib/Question.unit.spec.js
+++ b/frontend/test/metabase-lib/lib/Question.unit.spec.js
@@ -46,6 +46,31 @@ const metadata = createMockMetadata({
   ],
 });
 
+const metadata_without_order_pk = createMockMetadata({
+  databases: [
+    createSampleDatabase({
+      tables: [
+        createProductsTable(),
+        createPeopleTable(),
+        createReviewsTable(),
+        createOrdersTable({
+          fields: [
+            createOrdersIdField({ semantic_type: "type/Integer" }),
+            createOrdersUserIdField(),
+            createOrdersProductIdField(),
+            createOrdersSubtotalField(),
+            createOrdersTaxField(),
+            createOrdersTotalField(),
+            createOrdersDiscountField(),
+            createOrdersCreatedAtField(),
+            createOrdersQuantityField(),
+          ],
+        }),
+      ],
+    }),
+  ],
+});
+
 const card = {
   display: "table",
   visualization_settings: {},
@@ -75,6 +100,30 @@ const orders_raw_card = {
 };
 const orders_raw_question = new Question(orders_raw_card, metadata);
 
+const orders_card_without_pk = {
+  id: 1,
+  name: "Orders Model",
+  display: "table",
+  visualization_settings: {},
+  can_write: true,
+  dataset: true,
+  database_id: SAMPLE_DB_ID,
+  table_id: ORDERS_ID,
+  dataset_query: {
+    type: "query",
+    database: SAMPLE_DB_ID,
+    query: {
+      "source-table": ORDERS_ID,
+    },
+  },
+  result_metadata: [
+    createOrdersIdField({
+      semantic_type: "type/Integer",
+      field_ref: ["field", 11, null],
+    }),
+  ],
+};
+
 const orders_count_card = {
   id: 2,
   name: "# orders data",
@@ -257,6 +306,7 @@ const orders_count_by_id_card = {
     },
   },
 };
+
 const orders_count_by_id_question = new Question(
   orders_count_by_id_card,
   metadata,
@@ -1637,6 +1687,27 @@ describe("Question", () => {
       expect(question.supportsImplicitActions()).toBeFalsy();
     });
 
+    it("should allow to create implicit actions where the underlying table has a primary key but the model does not", () => {
+      const orders_question_without_pk = new Question(
+        orders_card_without_pk,
+        metadata,
+      );
+      expect(orders_question_without_pk.supportsImplicitActions()).toBeTruthy();
+    });
+
+    it("should not allow to create implicit actions where the underlying table has no primary key", () => {
+      const question = new Question(orders_raw_card, metadata_without_order_pk);
+      expect(question.supportsImplicitActions()).toBeFalsy();
+    });
+
+    it("should not allow to create implicit actions where the model has a primary key, but the underlying table does not", () => {
+      const question = new Question(
+        orders_card_without_pk,
+        metadata_without_order_pk,
+      );
+      expect(question.supportsImplicitActions()).toBeFalsy();
+    });
+
     it("should not allow to create implicit actions for a model with joins", () => {
       const question = new Question(orders_join_card, metadata);
       expect(question.supportsImplicitActions()).toBeFalsy();
-- 
GitLab