From 4c8fa7782feb014775af82d3a49db742e5d8fbe4 Mon Sep 17 00:00:00 2001
From: "Mahatthana (Kelvin) Nomsawadi" <mahatthana.n@gmail.com>
Date: Mon, 11 Jul 2022 17:33:22 +0700
Subject: [PATCH] Add toast after moving questions and models (#23756)
* Add toast message after moving questions or models
* Add test
* Address feedback: extract component into its own file
* Remove undo action on question move because it doesn't work in all cases
* Fix failed test
nodata user can't turn a question into a model
* Remove commented code that could confuses more people
---
frontend/src/metabase/entities/questions.js | 7 ++
.../SidebarItems/SidebarCollectionLink.tsx | 1 +
.../query_builder/components/QueryModals.jsx | 33 +++++--
.../QuestionMoveToast.styled.tsx | 13 +++
.../QuestionMoveToast/QuestionMoveToast.tsx | 40 ++++++++
.../components/QuestionMoveToast/index.ts | 1 +
.../question/question-management.cy.spec.js | 98 ++++++++++++++++---
7 files changed, 176 insertions(+), 17 deletions(-)
create mode 100644 frontend/src/metabase/query_builder/components/QuestionMoveToast/QuestionMoveToast.styled.tsx
create mode 100644 frontend/src/metabase/query_builder/components/QuestionMoveToast/QuestionMoveToast.tsx
create mode 100644 frontend/src/metabase/query_builder/components/QuestionMoveToast/index.ts
diff --git a/frontend/src/metabase/entities/questions.js b/frontend/src/metabase/entities/questions.js
index f85544d3183..65b108e0f82 100644
--- a/frontend/src/metabase/entities/questions.js
+++ b/frontend/src/metabase/entities/questions.js
@@ -2,6 +2,8 @@ import { createEntity, undo } from "metabase/lib/entities";
import * as Urls from "metabase/lib/urls";
import { color } from "metabase/lib/colors";
+import { API_UPDATE_QUESTION } from "metabase/query_builder/actions";
+
import Collections, {
getCollectionType,
normalizedCollection,
@@ -42,6 +44,11 @@ const Questions = createEntity({
Collections.actions.fetchList({ tree: true }, { reload: true }),
);
+ const card = result?.payload?.question;
+ if (card) {
+ dispatch.action(API_UPDATE_QUESTION, card);
+ }
+
return result;
};
},
diff --git a/frontend/src/metabase/nav/containers/MainNavbar/SidebarItems/SidebarCollectionLink.tsx b/frontend/src/metabase/nav/containers/MainNavbar/SidebarItems/SidebarCollectionLink.tsx
index b37f3298058..bdb2d1ea752 100644
--- a/frontend/src/metabase/nav/containers/MainNavbar/SidebarItems/SidebarCollectionLink.tsx
+++ b/frontend/src/metabase/nav/containers/MainNavbar/SidebarItems/SidebarCollectionLink.tsx
@@ -92,6 +92,7 @@ const SidebarCollectionLink = React.forwardRef<HTMLLIElement, Props>(
<CollectionNodeRoot
role="treeitem"
depth={depth}
+ aria-selected={isSelected}
isSelected={isSelected}
hovered={isHovered}
onClick={onToggleExpand}
diff --git a/frontend/src/metabase/query_builder/components/QueryModals.jsx b/frontend/src/metabase/query_builder/components/QueryModals.jsx
index 07051a2e737..212f24d0be5 100644
--- a/frontend/src/metabase/query_builder/components/QueryModals.jsx
+++ b/frontend/src/metabase/query_builder/components/QueryModals.jsx
@@ -1,9 +1,13 @@
/* eslint-disable react/prop-types */
import React from "react";
+import { connect } from "react-redux";
import { t } from "ttag";
import _ from "underscore";
+import Questions from "metabase/entities/questions";
+import { ROOT_COLLECTION } from "metabase/entities/collections";
+
import { MODAL_TYPES } from "metabase/query_builder/constants";
import Modal from "metabase/components/Modal";
@@ -25,8 +29,13 @@ import BulkFilterModal from "metabase/query_builder/components/filters/modals/Bu
import NewEventModal from "metabase/timelines/questions/containers/NewEventModal";
import EditEventModal from "metabase/timelines/questions/containers/EditEventModal";
import MoveEventModal from "metabase/timelines/questions/containers/MoveEventModal";
+import QuestionMoveToast from "./QuestionMoveToast";
+
+const mapDispatchToProps = {
+ setQuestionCollection: Questions.actions.setCollection,
+};
-export default class QueryModals extends React.Component {
+class QueryModals extends React.Component {
showAlertsAfterQuestionSaved = () => {
const { questionAlerts, user, onCloseModal, onOpenModal } = this.props;
@@ -171,11 +180,21 @@ export default class QueryModals extends React.Component {
initialCollectionId={question.collectionId()}
onClose={onCloseModal}
onMove={collection => {
- const card = question
- .setCollectionId(collection && collection.id)
- .card();
-
- this.props.onSave(card);
+ this.props.setQuestionCollection(
+ { id: question.id() },
+ collection,
+ {
+ notify: {
+ message: (
+ <QuestionMoveToast
+ isModel={question.isDataset()}
+ collectionId={collection.id || ROOT_COLLECTION.id}
+ />
+ ),
+ undo: false,
+ },
+ },
+ );
onCloseModal();
}}
/>
@@ -237,3 +256,5 @@ export default class QueryModals extends React.Component {
) : null;
}
}
+
+export default connect(null, mapDispatchToProps)(QueryModals);
diff --git a/frontend/src/metabase/query_builder/components/QuestionMoveToast/QuestionMoveToast.styled.tsx b/frontend/src/metabase/query_builder/components/QuestionMoveToast/QuestionMoveToast.styled.tsx
new file mode 100644
index 00000000000..dd547f8d0a9
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/QuestionMoveToast/QuestionMoveToast.styled.tsx
@@ -0,0 +1,13 @@
+import styled from "@emotion/styled";
+import { space } from "metabase/styled-components/theme";
+
+import Icon from "metabase/components/Icon";
+
+export const ToastRoot = styled.div`
+ display: flex;
+ align-items: center;
+`;
+
+export const StyledIcon = styled(Icon)`
+ margin-right: ${space(1)};
+`;
diff --git a/frontend/src/metabase/query_builder/components/QuestionMoveToast/QuestionMoveToast.tsx b/frontend/src/metabase/query_builder/components/QuestionMoveToast/QuestionMoveToast.tsx
new file mode 100644
index 00000000000..8faf35fe53e
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/QuestionMoveToast/QuestionMoveToast.tsx
@@ -0,0 +1,40 @@
+import React from "react";
+import { jt } from "ttag";
+
+import Collections from "metabase/entities/collections";
+import { color } from "metabase/lib/colors";
+
+import { StyledIcon, ToastRoot } from "./QuestionMoveToast.styled";
+
+interface QuestionMoveToastProps {
+ isModel: boolean;
+ collectionId: number;
+}
+
+export default function QuestionMoveToast({
+ isModel,
+ collectionId,
+}: QuestionMoveToastProps) {
+ return (
+ <ToastRoot>
+ <StyledIcon name="all" color="white" />
+ {isModel
+ ? jt`Model moved to ${(
+ <Collections.Link
+ key="collection-link"
+ id={collectionId}
+ ml={1}
+ color={color("brand")}
+ />
+ )}`
+ : jt`Question moved to ${(
+ <Collections.Link
+ key="collection-link"
+ id={collectionId}
+ ml={1}
+ color={color("brand")}
+ />
+ )}`}
+ </ToastRoot>
+ );
+}
diff --git a/frontend/src/metabase/query_builder/components/QuestionMoveToast/index.ts b/frontend/src/metabase/query_builder/components/QuestionMoveToast/index.ts
new file mode 100644
index 00000000000..f3008df3e7f
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/QuestionMoveToast/index.ts
@@ -0,0 +1 @@
+export { default } from "./QuestionMoveToast";
diff --git a/frontend/test/metabase/scenarios/question/question-management.cy.spec.js b/frontend/test/metabase/scenarios/question/question-management.cy.spec.js
index db5a2018fb4..fd40894a72a 100644
--- a/frontend/test/metabase/scenarios/question/question-management.cy.spec.js
+++ b/frontend/test/metabase/scenarios/question/question-management.cy.spec.js
@@ -3,6 +3,8 @@ import {
visitQuestion,
saveDashboard,
popover,
+ openNavigationSidebar,
+ navigationSidebar,
openQuestionActions,
questionInfoButton,
} from "__support__/e2e/helpers";
@@ -35,7 +37,6 @@ describe("managing question from the question's details sidebar", () => {
});
it("should be able to edit question details (metabase#11719-1)", () => {
- // cy.skipOn(user === "nodata");
cy.findByTestId("saved-question-header-title")
.click()
.type("1")
@@ -45,8 +46,6 @@ describe("managing question from the question's details sidebar", () => {
});
it("should be able to edit a question's description", () => {
- // cy.skipOn(user === "nodata");
-
questionInfoButton().click();
cy.findByPlaceholderText("Add description")
@@ -58,14 +57,79 @@ describe("managing question from the question's details sidebar", () => {
cy.findByText("foo");
});
- it("should be able to move the question (metabase#11719-2)", () => {
- // cy.skipOn(user === "nodata");
- openQuestionActions();
- cy.findByTestId("move-button").click();
- cy.findByText("My personal collection").click();
- clickButton("Move");
- assertOnRequest("updateQuestion");
- cy.contains("37.65");
+ describe("move", () => {
+ it("should be able to move the question (metabase#11719-2)", () => {
+ openNavigationSidebar();
+ navigationSidebar().within(() => {
+ // Highlight "Our analytics"
+ cy.findByText("Our analytics")
+ .parents("li")
+ .should("have.attr", "aria-selected", "true");
+ cy.findByText("Your personal collection")
+ .parents("li")
+ .should("have.attr", "aria-selected", "false");
+ });
+
+ openQuestionActions();
+ cy.findByTestId("move-button").click();
+ cy.findByText("My personal collection").click();
+ clickButton("Move");
+ assertOnRequest("updateQuestion");
+ cy.contains("37.65");
+
+ cy.contains(
+ `Question moved to ${getPersonalCollectionName(USERS[user])}`,
+ );
+
+ navigationSidebar().within(() => {
+ // Highlight "Your personal collection" after move
+ cy.findByText("Our analytics")
+ .parents("li")
+ .should("have.attr", "aria-selected", "false");
+ cy.findByText("Your personal collection")
+ .parents("li")
+ .should("have.attr", "aria-selected", "true");
+ });
+ });
+
+ it("should be able to move models", () => {
+ // TODO: Currently nodata users can't turn a question into a model
+ cy.skipOn(user === "nodata");
+
+ turnIntoModel();
+
+ openNavigationSidebar();
+ navigationSidebar().within(() => {
+ // Highlight "Our analytics"
+ cy.findByText("Our analytics")
+ .parents("li")
+ .should("have.attr", "aria-selected", "true");
+ cy.findByText("Your personal collection")
+ .parents("li")
+ .should("have.attr", "aria-selected", "false");
+ });
+
+ openQuestionActions();
+ cy.findByTestId("move-button").click();
+ cy.findByText("My personal collection").click();
+ clickButton("Move");
+ assertOnRequest("updateQuestion");
+ cy.contains("37.65");
+
+ cy.contains(
+ `Model moved to ${getPersonalCollectionName(USERS[user])}`,
+ );
+
+ navigationSidebar().within(() => {
+ // Highlight "Your personal collection" after move
+ cy.findByText("Our analytics")
+ .parents("li")
+ .should("have.attr", "aria-selected", "false");
+ cy.findByText("Your personal collection")
+ .parents("li")
+ .should("have.attr", "aria-selected", "true");
+ });
+ });
});
it("should be able to archive the question (metabase#11719-3, metabase#16512, metabase#20133)", () => {
@@ -182,3 +246,15 @@ function assertOnRequest(xhr_alias) {
"not.exist",
);
}
+
+function getPersonalCollectionName(user) {
+ const name = [user.first_name, user.last_name].join(" ");
+
+ return `${name}'s Personal Collection`;
+}
+
+function turnIntoModel() {
+ openQuestionActions();
+ cy.findByText("Turn into a model").click();
+ cy.findByText("Turn this into a model").click();
+}
--
GitLab