From 4f7b562c909d287a045ac775ca4b3098c1be4e26 Mon Sep 17 00:00:00 2001
From: Nemanja Glumac <31325167+nemanjaglumac@users.noreply.github.com>
Date: Fri, 15 Mar 2024 13:40:21 +0100
Subject: [PATCH] Fix the stale collection information when the source in the
data selector changes (#40063)
* 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: Alexander Polyankin <alexander.polyankin@metabase.com>
---
...virtual-table-collection-change.cy.spec.ts | 103 ++++++++++++++++++
frontend/src/metabase/entities/schemas.js | 28 +++--
.../metabase/entities/schemas.unit.spec.js | 12 +-
frontend/src/metabase/entities/tables.js | 33 ++++--
4 files changed, 153 insertions(+), 23 deletions(-)
create mode 100644 e2e/test/scenarios/question/reproductions/39812-data-selector-virtual-table-collection-change.cy.spec.ts
diff --git a/e2e/test/scenarios/question/reproductions/39812-data-selector-virtual-table-collection-change.cy.spec.ts b/e2e/test/scenarios/question/reproductions/39812-data-selector-virtual-table-collection-change.cy.spec.ts
new file mode 100644
index 00000000000..8df21cdaa0f
--- /dev/null
+++ b/e2e/test/scenarios/question/reproductions/39812-data-selector-virtual-table-collection-change.cy.spec.ts
@@ -0,0 +1,103 @@
+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);
+}
diff --git a/frontend/src/metabase/entities/schemas.js b/frontend/src/metabase/entities/schemas.js
index 2f27ef88b17..59d711efac2 100644
--- a/frontend/src/metabase/entities/schemas.js
+++ b/frontend/src/metabase/entities/schemas.js
@@ -1,4 +1,4 @@
-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/v1/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.type === "model",
+ 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);
diff --git a/frontend/src/metabase/entities/schemas.unit.spec.js b/frontend/src/metabase/entities/schemas.unit.spec.js
index 8a885388dd9..58bc34f975c 100644
--- a/frontend/src/metabase/entities/schemas.unit.spec.js
+++ b/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/v1/metadata/utils/saved-questions";
+import {
+ ROOT_COLLECTION_VIRTUAL_SCHEMA,
+ SAVED_QUESTIONS_VIRTUAL_DB_ID,
+} from "metabase-lib/v1/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,
+ },
});
});
});
diff --git a/frontend/src/metabase/entities/tables.js b/frontend/src/metabase/entities/tables.js
index 27b9f8546b5..2bc7fa6e82b 100644
--- a/frontend/src/metabase/entities/tables.js
+++ b/frontend/src/metabase/entities/tables.js
@@ -27,6 +27,8 @@ import { MetabaseApi } from "metabase/services";
import {
convertSavedQuestionToVirtualTable,
getQuestionVirtualTableId,
+ getCollectionVirtualSchemaId,
+ getCollectionVirtualSchemaName,
} from "metabase-lib/v1/metadata/utils/saved-questions";
const listTablesForDatabase = async (...args) =>
@@ -169,25 +171,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,
}));
}
@@ -196,7 +209,7 @@ const Tables = createEntity({
return {
...state,
- [virtualQuestionId]: convertSavedQuestionToVirtualTable(card),
+ [virtualTableId]: convertSavedQuestionToVirtualTable(card),
};
}
--
GitLab