fix sandboxing parameter field disabled (#29256)

frontend/src/metabase/containers/SavedQuestionLoader.jsx

-/* eslint-disable react/prop-types */
-import React from "react";
-import { connect } from "react-redux";
+import { useState, useEffect } from "react";
+import _ from "underscore";
+import { useAsync } from "react-use";
+import { useSelector, useDispatch } from "react-redux";
 
 import { loadMetadataForCard } from "metabase/questions/actions";
 import { getMetadata } from "metabase/selectors/metadata";
@@ -34,111 +35,51 @@ import Question from "metabase-lib/Question";
  * }
  *
  * @example
- *
- * The raw un-connected component is also exported so we can unit test it
- * without the redux store.
  */
-export class SavedQuestionLoader extends React.Component {
-  state = {
-    // this will store the loaded question
-    question: null,
-    card: null,
-    loading: false,
-    error: null,
-  };
-
-  UNSAFE_componentWillMount() {
-    // load the specified question when the component mounts
-    this._loadQuestion(this.props.questionId);
-  }
+const SavedQuestionLoader = ({ children, card, error, loading }) => {
+  const metadata = useSelector(getMetadata);
+  const dispatch = useDispatch();
+  const [question, setQuestion] = useState(null);
 
-  UNSAFE_componentWillReceiveProps(nextProps) {
-    // if the questionId changes (this will most likely be the result of a
-    // url change) then we need to load this new question
-    if (nextProps.questionId !== this.props.questionId) {
-      this._loadQuestion(nextProps.questionId);
+  const cardMetadataState = useAsync(async () => {
+    if (card?.id == null) {
+      return;
     }
 
-    // if the metadata changes for some reason we need to make sure we
-    // update the question with that metadata
-    if (nextProps.metadata !== this.props.metadata && this.state.card) {
-      this.setState({
-        question: new Question(this.state.card, nextProps.metadata),
-      });
-    }
-  }
+    await dispatch(loadMetadataForCard(card));
+  }, [card?.id]);
 
-  /*
-   * Load a saved question and any required metadata
-   *
-   * 1. Get the card from the api
-   * 2. Load any required metadata into the redux store
-   * 3. Create a new Question object to return to metabase-lib methods can
-   *    be used
-   * 4. Set the component state to the new Question
-   */
-  async _loadQuestion(questionId) {
-    if (questionId == null) {
-      this.setState({
-        loading: false,
-        error: null,
-        question: null,
-        card: null,
-      });
+  useEffect(() => {
+    if (card?.id == null) {
       return;
     }
-    try {
-      this.setState({ loading: true, error: null });
-      // get the saved question via the card API
-      const card = await this.props.fetchQuestion(questionId);
 
-      // pass the retrieved card to load any necessary metadata
-      // (tables, source db, segments, etc) into
-      // the redux store, the resulting metadata will be avaliable as metadata on the
-      // component props once it's avaliable
-      await this.props.loadMetadataForCard(card);
+    const hasCardMetadataLoaded =
+      !cardMetadataState.loading && cardMetadataState.error == null;
 
-      // instantiate a new question object using the metadata and saved question
-      // so we can use metabase-lib methods to retrieve information and modify
-      // the question
-      //
-      const question = new Question(card, this.props.metadata);
-
-      // finally, set state to store the Question object so it can be passed
-      // to the component using the loader, keep a reference to the card
-      // as well
-      this.setState({ loading: false, question, card });
-    } catch (error) {
-      this.setState({ loading: false, error });
+    if (!hasCardMetadataLoaded) {
+      setQuestion(null);
+      return;
     }
-  }
 
-  render() {
-    const { children } = this.props;
-    const { question, loading, error } = this.state;
-    // call the child function with our loaded question
-    return children && children({ question, loading, error });
-  }
-}
-
-// redux stuff
-function mapStateToProps(state) {
-  return {
-    metadata: getMetadata(state),
-  };
-}
+    if (!question) {
+      setQuestion(new Question(card, metadata));
+    }
+  }, [card, metadata, cardMetadataState, question]);
 
-const mapDispatchToProps = dispatch => {
-  return {
-    loadMetadataForCard: card => dispatch(loadMetadataForCard(card)),
-    fetchQuestion: async id => {
-      const action = await dispatch(Questions.actions.fetch({ id }));
-      return Questions.HACK_getObjectFromAction(action);
-    },
-  };
+  return (
+    children?.({
+      question,
+      loading: loading || cardMetadataState.loading,
+      error: error ?? cardMetadataState.error,
+    }) ?? null
+  );
 };
 
-export default connect(
-  mapStateToProps,
-  mapDispatchToProps,
+export default _.compose(
+  Questions.load({
+    id: (_state, props) => props.questionId,
+    loadingAndErrorWrapper: false,
+    entityAlias: "card",
+  }),
 )(SavedQuestionLoader);

frontend/test/__support__/server-mocks/database.ts

@@ -41,6 +41,13 @@ export const setupSchemaEndpoints = (db: Database) => {
   });
 };
 
+export const setupUnauthorizedSchemaEndpoints = (db: Database) => {
+  fetchMock.get(`path:/api/database/${db.id}/schemas`, {
+    status: 403,
+    body: PERMISSION_ERROR,
+  });
+};
+
 export function setupUnauthorizedDatabaseEndpoints(db: Database) {
   fetchMock.get(`path:/api/database/${db.id}`, {
     status: 403,

frontend/test/metabase/containers/SavedQuestionLoader.unit.spec.js

 import React from "react";
-import { render } from "@testing-library/react";
+import { screen } from "@testing-library/react";
 
-import { delay } from "metabase/lib/promise";
-
-// import the un-connected component so we can test its internal logic sans
-// redux
-import { SavedQuestionLoader } from "metabase/containers/SavedQuestionLoader";
+import SavedQuestionLoader from "metabase/containers/SavedQuestionLoader";
+import { renderWithProviders } from "__support__/ui";
+import {
+  setupCardEndpoints,
+  setupSchemaEndpoints,
+  setupUnauthorizedSchemaEndpoints,
+  setupUnauthorizedCardEndpoints,
+} from "__support__/server-mocks";
+import {
+  createMockCard,
+  createMockColumn,
+  createMockDatabase,
+} from "metabase-types/api/mocks";
 import Question from "metabase-lib/Question";
 
+const databaseMock = createMockDatabase({ id: 1 });
+
+const childrenRenderFn = ({ loading, question, error }) => {
+  if (error) {
+    return <div>error</div>;
+  }
+
+  if (loading) {
+    return <div>loading</div>;
+  }
+
+  return <div>{question.displayName()}</div>;
+};
+
+const setupQuestion = ({ id, name, hasAccess }) => {
+  const card = createMockCard({
+    id,
+    name,
+    result_metadata: [createMockColumn()],
+  });
+  const q = new Question(card, null);
+
+  if (hasAccess) {
+    setupCardEndpoints(q.card());
+  } else {
+    setupUnauthorizedCardEndpoints(q.card());
+  }
+
+  return card;
+};
+
+const setup = ({ questionId, hasAccess }) => {
+  if (hasAccess) {
+    setupSchemaEndpoints(databaseMock);
+  } else {
+    setupUnauthorizedSchemaEndpoints(databaseMock);
+  }
+
+  const card = setupQuestion({
+    id: questionId,
+    name: "Question 1",
+    hasAccess,
+  });
+
+  const { rerender } = renderWithProviders(
+    <SavedQuestionLoader questionId={questionId}>
+      {childrenRenderFn}
+    </SavedQuestionLoader>,
+  );
+
+  return { rerender, card };
+};
+
 describe("SavedQuestionLoader", () => {
-  let loadQuestionSpy, loadMetadataSpy, mockChild;
   beforeEach(() => {
-    // reset mocks between tests so we have fresh spies, etc
     jest.resetAllMocks();
-    mockChild = jest.fn().mockReturnValue(<div />);
-    loadMetadataSpy = jest.fn();
-    loadQuestionSpy = jest.spyOn(
-      SavedQuestionLoader.prototype,
-      "_loadQuestion",
-    );
   });
 
   it("should load a question given a questionId", async () => {
-    const questionId = 1;
-    const q = Question.create({ databaseId: 1, tableId: 2 });
-    const mockFetchQuestion = jest
-      .fn()
-      .mockResolvedValue(q._doNotCallSerializableCard());
-
-    render(
-      <SavedQuestionLoader
-        questionId={questionId}
-        loadMetadataForCard={loadMetadataSpy}
-        fetchQuestion={mockFetchQuestion}
-      >
-        {mockChild}
-      </SavedQuestionLoader>,
-    );
-    expect(mockChild.mock.calls[0][0].loading).toEqual(true);
-    expect(mockChild.mock.calls[0][0].error).toEqual(null);
+    setup({ questionId: 1, hasAccess: true });
 
-    // stuff happens asynchronously
-    await delay(0);
+    expect(screen.getByText("loading")).toBeInTheDocument();
+    expect(await screen.findByText("Question 1")).toBeInTheDocument();
+  });
 
-    expect(loadQuestionSpy).toHaveBeenCalledWith(questionId);
+  it("should handle errors", async () => {
+    setup({ questionId: 1, hasAccess: false });
 
-    const calls = mockChild.mock.calls;
-    const { question, loading, error } = calls[calls.length - 1][0];
-    expect(question.isEqual(q)).toBe(true);
-    expect(loading).toEqual(false);
-    expect(error).toEqual(null);
+    expect(screen.getByText("loading")).toBeInTheDocument();
+    expect(await screen.findByText("error")).toBeInTheDocument();
   });
 
-  it("should load a new question if the question ID changes", () => {
-    const originalQuestionId = 1;
-    const newQuestionId = 2;
+  it("should load a new question if the question ID changes", async () => {
+    const nextQuestionId = 2;
+    setupQuestion({
+      id: nextQuestionId,
+      name: "Question 2",
+      hasAccess: true,
+    });
 
-    const { rerender } = render(
-      <SavedQuestionLoader
-        questionId={originalQuestionId}
-        loadMetadataForCard={loadMetadataSpy}
-      >
-        {mockChild}
-      </SavedQuestionLoader>,
-    );
+    const { rerender } = setup({ questionId: 1, hasAccess: true });
 
-    expect(loadQuestionSpy).toHaveBeenCalledWith(originalQuestionId);
+    expect(await screen.findByText("Question 1")).toBeInTheDocument();
 
-    // update the question ID, a new question id param in the url would do this
     rerender(
-      <SavedQuestionLoader
-        questionId={newQuestionId}
-        loadMetadataForCard={loadMetadataSpy}
-      >
-        {mockChild}
+      <SavedQuestionLoader questionId={nextQuestionId}>
+        {childrenRenderFn}
       </SavedQuestionLoader>,
     );
 
-    // question loading should begin with the new ID
-    expect(loadQuestionSpy).toHaveBeenCalledWith(newQuestionId);
+    expect(screen.getByText("loading")).toBeInTheDocument();
+
+    expect(await screen.findByText("Question 2")).toBeInTheDocument();
   });
 });