Fix the stale collection information when the source in the data selector changes (#40063) (#40167)

* Bridge out of sync collection information between the card and the state

* Account for models

* Use existing helpers

* Create virtual schema if it doesn't exist yet

* Add E2E reproduction for #39812

* Create virtual schema if it doesn't exist yet

* Create virtual schema if it doesn't exist yet

* Create virtual schema if it doesn't exist yet

* Expand the reproduction to include the nested question scenario

* Improve assertions

* Convert E2E repro to TS

* Fix tests

---------

Co-authored-by: Nemanja Glumac <31325167+nemanjaglumac@users.noreply.github.com>
Co-authored-by: Alexander Polyankin <alexander.polyankin@metabase.com>

e2e/test/scenarios/question/reproductions/39812-data-selector-virtual-table-collection-change.cy.spec.ts

+import {
+  ORDERS_MODEL_ID,
+  ORDERS_COUNT_QUESTION_ID,
+} from "e2e/support/cypress_sample_instance_data";
+import {
+  restore,
+  visitModel,
+  openNotebook,
+  openQuestionActions,
+  popover,
+  visitQuestion,
+} from "e2e/support/helpers";
+
+describe("issue 39812", () => {
+  beforeEach(() => {
+    restore();
+    cy.signInAsAdmin();
+  });
+
+  it("moving the model to another collection should immediately be reflected in the data selector (metabase#39812-1)", () => {
+    visitModel(ORDERS_MODEL_ID);
+    openNotebook();
+
+    openDataSelector();
+    assertSourceCollection("Our analytics");
+    assertDataSource("Orders Model");
+
+    moveToCollection("First collection");
+
+    openDataSelector();
+    assertSourceCollection("First collection");
+    assertDataSource("Orders Model");
+  });
+
+  it("moving the source question should immediately reflect in the data selector for the nested question that depends on it (metabase#39812-2)", () => {
+    const SOURCE_QUESTION_ID = ORDERS_COUNT_QUESTION_ID;
+    // Rename the source question to make assertions more explicit
+    const sourceQuestionName = "Source Question";
+    cy.request("PUT", `/api/card/${ORDERS_COUNT_QUESTION_ID}`, {
+      name: sourceQuestionName,
+    });
+
+    const nestedQuestionDetails = {
+      name: "Nested Question",
+      query: { "source-table": `card__${SOURCE_QUESTION_ID}` },
+    };
+
+    cy.createQuestion(nestedQuestionDetails, {
+      wrapId: true,
+      idAlias: "nestedQuestionId",
+    });
+
+    visitQuestion("@nestedQuestionId");
+    openNotebook();
+
+    openDataSelector();
+    assertSourceCollection("Our analytics");
+    assertDataSource(sourceQuestionName);
+
+    cy.log("Move the source question to another collection");
+    visitQuestion(SOURCE_QUESTION_ID);
+    openNotebook();
+    moveToCollection("First collection");
+
+    cy.log("Make sure the source change is reflected in a nested question");
+    visitQuestion("@nestedQuestionId");
+    openNotebook();
+
+    openDataSelector();
+    assertSourceCollection("First collection");
+    assertDataSource(sourceQuestionName);
+  });
+});
+
+function moveToCollection(collection: string) {
+  openQuestionActions();
+  popover().findByTextEnsureVisible("Move").click();
+  cy.findByRole("dialog").within(() => {
+    cy.intercept("GET", "/api/collection/tree**").as("updateCollectionTree");
+    cy.findAllByTestId("item-picker-item")
+      .filter(`:contains(${collection})`)
+      .click();
+
+    cy.button("Move").click();
+    cy.wait("@updateCollectionTree");
+  });
+}
+
+function openDataSelector() {
+  cy.findByTestId("data-step-cell").click();
+}
+
+function assertItemSelected(item: string) {
+  cy.findByLabelText(item).should("have.attr", "aria-selected", "true");
+}
+
+function assertSourceCollection(collection: string) {
+  return assertItemSelected(collection);
+}
+
+function assertDataSource(questionOrModel: string) {
+  return assertItemSelected(questionOrModel);
+}

frontend/src/metabase/entities/schemas.js

-import { updateIn } from "icepick";
+import { assocIn, updateIn } from "icepick";
 
 import Questions from "metabase/entities/questions";
 import { createEntity } from "metabase/lib/entities";
@@ -7,7 +7,9 @@ import { getMetadata } from "metabase/selectors/metadata";
 import { MetabaseApi } from "metabase/services";
 import {
   getCollectionVirtualSchemaId,
+  getCollectionVirtualSchemaName,
   getQuestionVirtualTableId,
+  SAVED_QUESTIONS_VIRTUAL_DB_ID,
 } from "metabase-lib/metadata/utils/saved-questions";
 import {
   generateSchemaId,
@@ -84,16 +86,16 @@ export default createEntity({
     }
 
     if (type === Questions.actionTypes.UPDATE && !error) {
-      const { question } = payload;
-      const schemaId = getCollectionVirtualSchemaId(question.collection, {
-        isDatasets: question.dataset,
+      const { question: card } = payload;
+      const virtualSchemaId = getCollectionVirtualSchemaId(card.collection, {
+        isDatasets: card.type === "model",
       });
-
-      const virtualQuestionId = getQuestionVirtualTableId(question.id);
+      const virtualSchemaName = getCollectionVirtualSchemaName(card.collection);
+      const virtualQuestionId = getQuestionVirtualTableId(card.id);
       const previousSchemaContainingTheQuestion =
         getPreviousSchemaContainingTheQuestion(
           state,
-          schemaId,
+          virtualSchemaId,
           virtualQuestionId,
         );
 
@@ -105,16 +107,20 @@ export default createEntity({
         );
       }
 
-      if (!state[schemaId]) {
-        return state;
+      if (!state[virtualSchemaId]) {
+        state = assocIn(state, [virtualSchemaId], {
+          id: virtualSchemaId,
+          name: virtualSchemaName,
+          database: SAVED_QUESTIONS_VIRTUAL_DB_ID,
+        });
       }
 
-      return updateIn(state, [schemaId, "tables"], tables => {
+      return updateIn(state, [virtualSchemaId, "tables"], tables => {
         if (!tables) {
           return tables;
         }
 
-        if (question.archived) {
+        if (card.archived) {
           return tables.filter(id => id !== virtualQuestionId);
         }
         return addTableAvoidingDuplicates(tables, virtualQuestionId);

frontend/src/metabase/entities/schemas.unit.spec.js

@@ -3,7 +3,10 @@ import fetchMock from "fetch-mock";
 import { getStore } from "__support__/entities-store";
 import Questions from "metabase/entities/questions";
 import Schemas from "metabase/entities/schemas";
-import { ROOT_COLLECTION_VIRTUAL_SCHEMA } from "metabase-lib/metadata/utils/saved-questions";
+import {
+  ROOT_COLLECTION_VIRTUAL_SCHEMA,
+  SAVED_QUESTIONS_VIRTUAL_DB_ID,
+} from "metabase-lib/metadata/utils/saved-questions";
 
 describe("schema entity", () => {
   let store;
@@ -251,7 +254,7 @@ describe("schema entity", () => {
       });
     });
 
-    it("should not add question ID when it is unarchived if collection schema is not present in store", () => {
+    it("should add question ID when it is unarchived if collection schema is not present in store", () => {
       const nextState = Schemas.reducer(
         {
           [ROOT_COLLECTION_VIRTUAL_SCHEMA]: {
@@ -265,6 +268,11 @@ describe("schema entity", () => {
         [ROOT_COLLECTION_VIRTUAL_SCHEMA]: {
           tables: ["card__123"],
         },
+        [`${SAVED_QUESTIONS_VIRTUAL_DB_ID}:foo`]: {
+          id: `${SAVED_QUESTIONS_VIRTUAL_DB_ID}:foo`,
+          name: "foo",
+          database: SAVED_QUESTIONS_VIRTUAL_DB_ID,
+        },
       });
     });
   });

frontend/src/metabase/entities/tables.js

@@ -26,6 +26,8 @@ import { MetabaseApi } from "metabase/services";
 import {
   convertSavedQuestionToVirtualTable,
   getQuestionVirtualTableId,
+  getCollectionVirtualSchemaId,
+  getCollectionVirtualSchemaName,
 } from "metabase-lib/metadata/utils/saved-questions";
 
 const listTablesForDatabase = async (...args) =>
@@ -161,25 +163,36 @@ const Tables = createEntity({
 
     if (type === Questions.actionTypes.UPDATE && !error) {
       const card = payload.question;
-      const virtualQuestionId = getQuestionVirtualTableId(card.id);
+      const virtualTableId = getQuestionVirtualTableId(card.id);
 
-      if (card.archived && state[virtualQuestionId]) {
-        delete state[virtualQuestionId];
+      if (card.archived && state[virtualTableId]) {
+        delete state[virtualTableId];
         return state;
       }
 
-      if (state[virtualQuestionId]) {
-        const virtualQuestion = state[virtualQuestionId];
+      if (state[virtualTableId]) {
+        const virtualTable = state[virtualTableId];
+        const virtualSchemaId = getCollectionVirtualSchemaId(card.collection, {
+          isDatasets: card.type === "model",
+        });
+        const virtualSchemaName = getCollectionVirtualSchemaName(
+          card.collection,
+        );
+
         if (
-          virtualQuestion.display_name !== card.name ||
-          virtualQuestion.moderated_status !== card.moderated_status ||
-          virtualQuestion.description !== card.description
+          virtualTable.display_name !== card.name ||
+          virtualTable.moderated_status !== card.moderated_status ||
+          virtualTable.description !== card.description ||
+          virtualTable.schema !== virtualSchemaId ||
+          virtualTable.schema_name !== virtualSchemaName
         ) {
-          state = updateIn(state, [virtualQuestionId], table => ({
+          state = updateIn(state, [virtualTableId], table => ({
             ...table,
             display_name: card.name,
             moderated_status: card.moderated_status,
             description: card.description,
+            schema: virtualSchemaId,
+            schema_name: virtualSchemaName,
           }));
         }
 
@@ -188,7 +201,7 @@ const Tables = createEntity({
 
       return {
         ...state,
-        [virtualQuestionId]: convertSavedQuestionToVirtualTable(card),
+        [virtualTableId]: convertSavedQuestionToVirtualTable(card),
       };
     }