Tool to fix broken questions, details page (#18022)

Details page for tools to fix broken questions. Also the PR ran long enough that nemanja found some bugs in the details page. Does not include any loading overlay or anything to indicate that reloads are happening, which is for 41.1 I guess

enterprise/backend/src/metabase_enterprise/audit_app/pages/common/cards.clj

 (ns metabase-enterprise.audit-app.pages.common.cards
   (:require [metabase-enterprise.audit-app.pages.common :as common]
+            [metabase.db.connection :as mdb.connection]
             [metabase.util.honeysql-extensions :as hx]))
 
 (def avg-exec-time
@@ -55,6 +56,15 @@
                :from [:report_dashboardcard]
                :group-by [:card_id]}])
 
+(def dashboards-ids
+  "HoneySQL for a CTE to enumerate the dashboards for a Card. We get the actual ID's"
+  [:dash_card {:select [:card_id [(common/group-concat (hx/cast
+                                                         (if (= (mdb.connection/db-type) :mysql) :char :text)
+                                                         :report_dashboard.name) "|") :name_str]]
+               :from [:report_dashboardcard]
+               :join [:report_dashboard [:= :report_dashboardcard.dashboard_id :report_dashboard.id]]
+               :group-by [:card_id]}])
+
 (def views
   "HoneySQL for a CTE to include the total view count for each Card."
   [:card_views {:select   [[:model_id :card_id]

enterprise/backend/src/metabase_enterprise/audit_app/pages/queries.clj

@@ -3,6 +3,7 @@
             [metabase-enterprise.audit-app.interface :as audit.i]
             [metabase-enterprise.audit-app.pages.common :as common]
             [metabase-enterprise.audit-app.pages.common.cards :as cards]
+            [metabase.db.connection :as mdb.connection]
             [metabase.util.honeysql-extensions :as hx]))
 
 ;; DEPRECATED Query that returns data for a two-series timeseries chart with number of queries ran and average query
@@ -70,6 +71,7 @@
               [:collection_name {:display_name "Collection",         :base_type :type/Text    :remapped_from :collection_id}]
               [:database_id     {:display_name "Database ID",        :base_type :type/Integer :remapped_to   :database_name}]
               [:database_name   {:display_name "Database",           :base_type :type/Text    :remapped_from :database_id}]
+              [:schema_name     {:display_name "Schema",             :base_type :type/Text}]
               [:table_id        {:display_name "Table ID",           :base_type :type/Integer :remapped_to   :table_name}]
               [:table_name      {:display_name "Table",              :base_type :type/Text    :remapped_from :table_id}]
               [:last_run_at     {:display_name "Last run at",        :base_type :type/DateTime}]
@@ -86,11 +88,19 @@
                              cards/dashboards-count]
                  :select    [[:card.id :card_id]
                              [:card.name :card_name]
-                             [(hsql/call :concat (hsql/call :substring :latest_qe.error 0 60) "...") :error_substr]
+                             [(hsql/call :concat
+                                         (hsql/call
+                                           :substring
+                                           :latest_qe.error
+                                           (if (= (mdb.connection/db-type) :mysql) 1 0)
+                                           60)
+                                         "...")
+                              :error_substr]
                              :collection_id
                              [:coll.name :collection_name]
                              :card.database_id
                              [:db.name :database_name]
+                             [:t.schema :schema_name]
                              :card.table_id
                              [:t.name :table_name]
                              [:latest_qe.started_at :last_run_at]

enterprise/backend/src/metabase_enterprise/audit_app/pages/query_detail.clj

@@ -3,19 +3,69 @@
   (:require [cheshire.core :as json]
             [metabase-enterprise.audit-app.interface :as audit.i]
             [metabase-enterprise.audit-app.pages.common :as common]
+            [metabase-enterprise.audit-app.pages.common.cards :as cards]
             [metabase.util.schema :as su]
             [ring.util.codec :as codec]
             [schema.core :as s]))
 
+(defmethod audit.i/internal-query ::bad-card
+  [_ card-id]
+  {:metadata [[:card_id         {:display_name "Question ID",        :base_type :type/Integer :remapped_from :card_name}]
+              [:card_name       {:display_name "Question",           :base_type :type/Text    :remapped_from :card_id}]
+              [:error_str       {:display_name "Error",              :base_type :type/Text    :code          true}]
+              [:collection_id   {:display_name "Collection ID",      :base_type :type/Integer :remapped_to   :collection_name}]
+              [:collection_name {:display_name "Collection",         :base_type :type/Text    :remapped_from :collection_id}]
+              [:database_id     {:display_name "Database ID",        :base_type :type/Integer :remapped_to   :database_name}]
+              [:database_name   {:display_name "Database",           :base_type :type/Text    :remapped_from :database_id}]
+              [:schema_name     {:display_name "Schema",             :base_type :type/Text}]
+              [:table_id        {:display_name "Table ID",           :base_type :type/Integer :remapped_to   :table_name}]
+              [:table_name      {:display_name "Table",              :base_type :type/Text    :remapped_from :table_id}]
+              [:last_run_at     {:display_name "Last run at",        :base_type :type/DateTime}]
+              [:total_runs      {:display_name "Total runs",         :base_type :type/Integer}]
+              ;; Denormalize by string_agg in order to avoid having to deal with complicated left join
+              [:dash_name_str   {:display_name "Dashboards it's in", :base_type :type/Text}]
+              [:user_id         {:display_name "Created By ID",      :base_type :type/Integer :remapped_to   :user_name}]
+              [:user_name       {:display_name "Created By",         :base_type :type/Text    :remapped_from :user_id}]
+              [:updated_at      {:display_name "Updated At",         :base_type :type/DateTime}]]
+   :results (common/reducible-query
+              {:with      [cards/query-runs
+                           cards/latest-qe
+                           cards/dashboards-ids]
+               :select    [[:card.id :card_id]
+                           [:card.name :card_name]
+                           [:latest_qe.error :error_str]
+                           :collection_id
+                           [:coll.name :collection_name]
+                           :card.database_id
+                           [:db.name :database_name]
+                           [:t.schema :schema_name]
+                           :card.table_id
+                           [:t.name :table_name]
+                           [:latest_qe.started_at :last_run_at]
+                           [:query_runs.count :total_runs]
+                           [:dash_card.name_str :dash_name_str]
+                           [:card.creator_id :user_id]
+                           [(common/user-full-name :u) :user_name]
+                           [:card.updated_at :updated_at]]
+               :from      [[:report_card :card]]
+               :left-join [[:collection :coll]                [:= :card.collection_id :coll.id]
+                           [:metabase_database :db]           [:= :card.database_id :db.id]
+                           [:metabase_table :t]               [:= :card.table_id :t.id]
+                           [:core_user :u]                    [:= :card.creator_id :u.id]
+                           :latest_qe                         [:= :card.id :latest_qe.card_id]
+                           :query_runs                        [:= :card.id :query_runs.card_id]
+                           :dash_card                         [:= :card.id :dash_card.card_id]]
+               :where     [:= :card.id card-id] })})
+
 ;; Details about a specific query (currently just average execution time).
 (s/defmethod audit.i/internal-query ::details
   [_ query-hash :- su/NonBlankString]
   {:metadata [[:query                  {:display_name "Query",                :base_type :type/Dictionary}]
               [:average_execution_time {:display_name "Avg. Exec. Time (ms)", :base_type :type/Number}]]
    :results  (common/reducible-query
-              {:select [:query
-                        :average_execution_time]
-               :from   [:query]
-               :where  [:= :query_hash (codec/base64-decode query-hash)]
-               :limit  1})
+               {:select [:query
+                         :average_execution_time]
+                :from   [:query]
+                :where  [:= :query_hash (codec/base64-decode query-hash)]
+                :limit  1})
    :xform (map #(update (vec %) 0 json/parse-string))})

enterprise/frontend/src/metabase-enterprise/audit_app/components/AuditParameters.jsx

@@ -23,6 +23,7 @@ const propTypes = {
     }),
   ),
   children: PropTypes.func,
+  hasResults: PropTypes.bool,
 };
 
 export default class AuditParameters extends React.Component {
@@ -48,8 +49,14 @@ export default class AuditParameters extends React.Component {
   }, DEBOUNCE_PERIOD);
 
   render() {
-    const { parameters, children, buttons } = this.props;
+    const { parameters, children, buttons, hasResults } = this.props;
     const { inputValues, committedValues } = this.state;
+
+    const disabled =
+      hasResults === false &&
+      inputValues &&
+      Object.values(inputValues).every(v => v === "");
+
     return (
       <div>
         <div className="pt4">
@@ -59,6 +66,7 @@ export default class AuditParameters extends React.Component {
               type="text"
               value={inputValues[key] || ""}
               placeholder={placeholder}
+              disabled={disabled}
               onChange={value => {
                 this.changeValue(key, value);
               }}
@@ -66,7 +74,13 @@ export default class AuditParameters extends React.Component {
             />
           ))}
           {buttons?.map(({ key, onClick, label }) => (
-            <Button primary key={key} onClick={onClick} className="ml2">
+            <Button
+              primary
+              key={key}
+              onClick={onClick}
+              disabled={disabled}
+              className="ml2"
+            >
               {label}
             </Button>
           ))}

enterprise/frontend/src/metabase-enterprise/audit_app/containers/AuditTable.jsx

@@ -30,19 +30,31 @@ AuditTable.propTypes = {
   reload: PropTypes.bool,
   children: PropTypes.node,
   dispatch: PropTypes.func.isRequired,
+  onLoad: PropTypes.func,
+  mode: PropTypes.shape({
+    name: PropTypes.string.isRequired,
+    drills: PropTypes.func.isRequired,
+  }),
 };
 
 function AuditTable({
   metadata,
   table,
   pageSize = DEFAULT_PAGE_SIZE,
+  mode = AuditMode,
   children,
   dispatch,
+  onLoad,
   ...rest
 }) {
   const [loadedCount, setLoadedCount] = useState(0);
   const { handleNextPage, handlePreviousPage, page } = usePagination();
 
+  const handleOnLoad = results => {
+    setLoadedCount(results[0].row_count);
+    onLoad(results);
+  };
+
   const card = chain(table.card)
     .assoc("display", "audit-table")
     .assocIn(["dataset_query", "limit"], pageSize)
@@ -60,10 +72,10 @@ function AuditTable({
         className="mt3"
         question={question}
         metadata={metadata}
-        mode={AuditMode}
+        mode={mode}
         onChangeLocation={handleChangeLocation}
         onChangeCardAndRun={() => {}}
-        onLoad={results => setLoadedCount(results[0].row_count)}
+        onLoad={handleOnLoad}
         dispatch={dispatch}
         {...rest}
       />

enterprise/frontend/src/metabase-enterprise/audit_app/containers/QuestionLoadAndDisplay.jsx

-import React, { useEffect, useRef } from "react";
+import React, { useEffect, useRef, useImperativeHandle } from "react";
 import PropTypes from "prop-types";
 import QuestionResultLoader from "metabase/containers/QuestionResultLoader";
 import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper";
@@ -9,6 +9,7 @@ const propTypes = {
   keepPreviousWhileLoading: PropTypes.bool,
   reload: PropTypes.bool,
   onLoad: PropTypes.func,
+  reloadRef: PropTypes.shape({ current: PropTypes.func }),
 };
 
 const QuestionLoadAndDisplay = ({
@@ -16,12 +17,15 @@ const QuestionLoadAndDisplay = ({
   keepPreviousWhileLoading,
   reload,
   onLoad,
+  reloadRef,
   ...props
 }) => {
-  const reloadRef = useRef();
+  const reloadFnRef = useRef();
+
+  useImperativeHandle(reloadRef, () => () => reloadFnRef.current?.());
 
   useEffect(() => {
-    reload && reloadRef.current();
+    reload && reloadFnRef.current();
   }, [reload]);
 
   return (
@@ -32,7 +36,7 @@ const QuestionLoadAndDisplay = ({
     >
       {({ loading, error, reload, ...resultProps }) => {
         const shouldShowLoader = loading && resultProps.results == null;
-        reloadRef.current = reload;
+        reloadFnRef.current = reload;
 
         return (
           <LoadingAndErrorWrapper

enterprise/frontend/src/metabase-enterprise/audit_app/lib/cards/queries.js

@@ -78,7 +78,7 @@ export const bad_table = (
         { name: "error_substr", enabled: true },
         { name: "collection_id", enabled: true },
         { name: "database_id", enabled: true },
-        { name: "schema_id", enabled: true },
+        { name: "schema", enabled: true },
         { name: "table_id", enabled: true },
         { name: "last_run_at", enabled: true },
         { name: "total_runs", enabled: true },

enterprise/frontend/src/metabase-enterprise/audit_app/lib/mode.js

@@ -12,7 +12,9 @@ export const getRowValuesByColumns = (row, cols) =>
     };
   }, {});
 
-const columnNameToUrl = {
+export const columnNameToUrl = {
+  // No admin page for collections but still want to link to it
+  collection_id: value => `/collection/${value}`,
   user_id: value => `/admin/audit/member/${value}`,
   creator_id: value => `/admin/audit/member/${value}`,
   viewed_by_id: value => `/admin/audit/member/${value}`,

enterprise/frontend/src/metabase-enterprise/tools/containers/ErrorDetail.jsx

+import React from "react";
+
+import { connect } from "react-redux";
+import { push } from "react-router-redux";
+import { getMetadata } from "metabase/selectors/metadata";
+
+import { t } from "ttag";
+import PropTypes from "prop-types";
+import { getIn } from "icepick";
+
+import { formatColumn, formatValue } from "metabase/lib/formatting";
+import { CardApi } from "metabase/services";
+import Button from "metabase/components/Button";
+import Link from "metabase/components/Link";
+import Question from "metabase-lib/lib/Question";
+import { QuestionResultLoader } from "metabase/containers/QuestionResultLoader";
+import { columnNameToUrl } from "../../audit_app/lib/mode";
+
+function idxToUrl(resRow, resCols, nameToResCol, colName) {
+  const idVal = resRow[nameToResCol[colName]];
+  const urlVal = colName && idVal ? columnNameToUrl[colName](idVal) : "";
+  const linkClass = urlVal === "" ? "" : "text-brand";
+  return [urlVal, linkClass];
+}
+
+function ErrorDetailDisplay(props) {
+  const { result } = props;
+  const resRow = getIn(result, ["data", "rows", 0]);
+  const resCols = getIn(result, ["data", "cols"]);
+  if (resRow && resCols) {
+    const nameToResCol = resCols.reduce(
+      (obj, x, idx) => Object.assign(obj, { [x.name]: idx }),
+      {},
+    );
+
+    const linkColumns = [
+      null,
+      "collection_id",
+      "database_id",
+      null,
+      "table_id",
+      null,
+      "user_id",
+      null,
+    ];
+
+    const ordinaryRows = [
+      "last_run_at",
+      "collection_name",
+      "database_name",
+      "schema_name",
+      "table_name",
+      "total_runs",
+      "user_name",
+      "updated_at",
+    ].map((x, idx) => {
+      const [urlVal, linkClass] = idxToUrl(
+        resRow,
+        resCols,
+        nameToResCol,
+        linkColumns[idx],
+      );
+      const formattedVal = formatValue(resRow[nameToResCol[x]], {
+        column: resCols[nameToResCol[x]],
+        jsx: true,
+        rich: true,
+        type: "cell",
+        local: true,
+      });
+      return (
+        <tr key={x}>
+          <td align="right" className="m0 mt1 text-medium">
+            {formatColumn(resCols[nameToResCol[x]])}
+          </td>
+          <td>
+            {
+              <Link to={urlVal} className={linkClass}>
+                {formattedVal}
+              </Link>
+            }
+          </td>
+        </tr>
+      );
+    });
+
+    const dashIdRows = resRow[nameToResCol.dash_name_str]
+      ?.split("|")
+      ?.map((x, idx) => (
+        <tr key={x}>
+          <td align="right" className="m0 mt1 text-medium">
+            {idx === 0 && formatColumn(resCols[nameToResCol.dash_name_str])}
+          </td>
+          <td className="text-bold">
+            {formatValue(x, { column: resCols[nameToResCol.dash_name_str] })}
+          </td>
+        </tr>
+      ));
+
+    const [cardUrlVal, cardLinkClass] = idxToUrl(
+      resRow,
+      resCols,
+      nameToResCol,
+      "card_id",
+    );
+
+    return [
+      <h2 className="PageTitle p1" key="card_name">
+        {
+          <Link to={cardUrlVal} className={cardLinkClass}>
+            {resRow[nameToResCol.card_name]}
+          </Link>
+        }
+      </h2>,
+      <div key="error_str" className="p1 text-code">
+        {resRow[nameToResCol.error_str]}
+      </div>,
+      <table key="table" className="ContentTable">
+        <tbody>{[ordinaryRows, dashIdRows]}</tbody>
+      </table>,
+    ];
+  } else {
+    return null;
+  }
+}
+
+function ErrorDetail(props) {
+  const { params, errorRetry } = props;
+  const cardId = parseInt(params.cardId);
+
+  // below card is not the card in question, but
+  // the card we're creating to query for the error details
+  const card = {
+    name: "Card Errors",
+    dataset_query: {
+      type: "internal",
+      fn: "metabase-enterprise.audit-app.pages.query-detail/bad-card",
+      args: [cardId],
+    },
+  };
+  const question = new Question(card, null);
+
+  return (
+    <div>
+      <QuestionResultLoader question={question}>
+        {({ rawSeries, result }) => <ErrorDetailDisplay result={result} />}
+      </QuestionResultLoader>
+      <Button className="float-right" onClick={() => errorRetry(cardId)}>
+        {t`Rerun Question`}
+      </Button>
+    </div>
+  );
+}
+
+const mapStateToProps = (state, props) => ({
+  metadata: getMetadata(state),
+});
+
+const mapDispatchToProps = {
+  errorRetry: async cardId => {
+    await CardApi.query({ cardId: cardId });
+    // we're imagining that we successfully reran, in which case we want to go back to overall table
+    return push("/admin/tools/errors/");
+  },
+};
+
+export default connect(
+  mapStateToProps,
+  mapDispatchToProps,
+)(ErrorDetail);
+
+ErrorDetail.propTypes = {
+  params: PropTypes.object,
+  errorRetry: PropTypes.func,
+};
+ErrorDetailDisplay.propTypes = {
+  result: PropTypes.object,
+};

enterprise/frontend/src/metabase-enterprise/tools/containers/ErrorOverview.jsx

-import React, { useState } from "react";
+import React, { useState, useRef } from "react";
 import { t } from "ttag";
 
 import _ from "underscore";
@@ -8,15 +8,21 @@ import { CardApi } from "metabase/services";
 import * as Queries from "../../audit_app/lib/cards/queries";
 import AuditTable from "../../audit_app/containers/AuditTable";
 import AuditParameters from "../../audit_app/components/AuditParameters";
+import { ErrorMode } from "../mode";
 
 const getSortOrder = isAscending => (isAscending ? "asc" : "desc");
 
 const CARD_ID_COL = 0;
 
 export default function ErrorOverview(props) {
+  const reloadRef = useRef(null);
+  // TODO: use isReloading to display a loading overlay
+  // eslint-disable-next-line no-unused-vars
+  const [isReloading, setIsReloading] = useState(false);
+  const [hasResults, setHasResults] = useState(false);
   const [sorting, setSorting] = useState({
-    column: "card_name",
-    isAscending: true,
+    column: "last_run_at",
+    isAscending: false,
   });
 
   const [rowChecked, setRowChecked] = useState({});
@@ -29,7 +35,9 @@ export default function ErrorOverview(props) {
     setRowChecked(newRowChecked);
     setRowToCardId(newRowToCardId);
   };
+
   const handleReloadSelected = async () => {
+    setIsReloading(true);
     const checkedCardIds = Object.values(
       _.pick(rowToCardId, (member, key) => rowChecked[key]),
     );
@@ -38,10 +46,16 @@ export default function ErrorOverview(props) {
         async member => await CardApi.query({ cardId: member }),
       ),
     );
-    location.reload();
+    reloadRef.current?.reload();
   };
 
   const handleSortingChange = sorting => setSorting(sorting);
+
+  const handleLoad = result => {
+    setHasResults(result[0].row_count !== 0);
+    setIsReloading(false);
+  };
+
   return (
     <AuditParameters
       parameters={[
@@ -56,10 +70,12 @@ export default function ErrorOverview(props) {
           onClick: handleReloadSelected,
         },
       ]}
+      hasResults={hasResults}
     >
       {({ errorFilter, dbFilter, collectionFilter }) => (
         <AuditTable
           {...props}
+          reloadRef={reloadRef}
           pageSize={50}
           isSortable
           isSelectable
@@ -68,6 +84,8 @@ export default function ErrorOverview(props) {
           sorting={sorting}
           onSortingChange={handleSortingChange}
           onRowSelectClick={handleRowSelectClick}
+          onLoad={handleLoad}
+          mode={ErrorMode}
           table={Queries.bad_table(
             errorFilter,
             dbFilter,

enterprise/frontend/src/metabase-enterprise/tools/mode.js

+import { t } from "ttag";
+import { push } from "react-router-redux";
+
+const CARD_ID_ROW_IDX = 0;
+
+const ErrorDrill = ({ clicked }) => {
+  if (!clicked) {
+    return [];
+  }
+
+  const cardId = clicked.origin.row[CARD_ID_ROW_IDX];
+
+  return [
+    {
+      name: "detail",
+      title: t`View this`,
+      default: true,
+      action() {
+        return push(`/admin/tools/errors/${cardId}`);
+      },
+    },
+  ];
+};
+
+export const ErrorMode = {
+  name: "error",
+  drills: () => [ErrorDrill],
+};

enterprise/frontend/src/metabase-enterprise/tools/routes.jsx

@@ -5,6 +5,7 @@ import { IndexRedirect } from "react-router";
 import { t } from "ttag";
 import ToolsApp from "./containers/ToolsApp";
 import ErrorOverview from "./containers/ErrorOverview";
+import ErrorDetail from "./containers/ErrorDetail";
 
 const getRoutes = (store: any) => (
   <Route path="tools" title={t`Tools`} component={ToolsApp}>
@@ -14,6 +15,7 @@ const getRoutes = (store: any) => (
       title={t`Erroring Questions`}
       component={ErrorOverview}
     />
+    <Route path="errors/:cardId" component={ErrorDetail} />
   </Route>
 );