Audit questions BugBash fixes (#16545)

* add units to new columns, add hover state for column headers

* fixup! add units to new columns, add hover state for column headers

* hide loader when filtering or sorting audit table

* fix tests

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

@@ -95,9 +95,9 @@
                [:user_name       {:display_name "Created By",      :base_type :type/Text,    :remapped_from :user_id}]
                [:public_link     {:display_name "Public Link",     :base_type :type/URL}]
                [:cache_ttl       {:display_name "Cache Duration",  :base_type :type/Number}]
-               [:avg_exec_time   {:display_name "Average Runtime", :base_type :type/Integer}]
-               [:total_runtime   {:display_name "Total Runtime",   :base_type :type/Number}]
-               [:query_runs      {:display_name "Query Runs",      :base_type :type/Integer}]
+               [:avg_exec_time   {:display_name "Average Runtime (ms)", :base_type :type/Integer}]
+               [:total_runtime   {:display_name "Total Runtime (ms)",   :base_type :type/Number}]
+               [:query_runs      {:display_name "Query Runs (ms)",      :base_type :type/Integer}]
                ]
     :results  (common/reducible-query
                 (->

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

@@ -55,6 +55,7 @@ function AuditTable({
   return (
     <div>
       <QuestionLoadAndDisplay
+        keepPreviousWhileLoading
         className="mt3"
         question={question}
         metadata={metadata}

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

@@ -68,6 +68,7 @@ export default class AuditTableVisualization extends React.Component {
       visualizationIsClickable,
       onVisualizationClick,
       settings,
+      isSortable,
     } = this.props;
 
     const columnIndexes = settings["table.columns"]
@@ -99,6 +100,7 @@ export default class AuditTableVisualization extends React.Component {
                   className={cx("text-nowrap", {
                     "text-right": isColumnRightAligned(column),
                     "text-brand": isSortedByColumn,
+                    "cursor-pointer text-brand-hover": isSortable,
                   })}
                 >
                   {formatColumn(cols[colIndex])}

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

@@ -5,13 +5,29 @@ import QuestionResultLoader from "metabase/containers/QuestionResultLoader";
 import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper";
 import Visualization from "metabase/visualizations/components/Visualization";
 
-const QuestionLoadAndDisplay = ({ question, onLoad, ...props }) => (
-  <QuestionResultLoader question={question} onLoad={onLoad}>
-    {({ loading, error, ...resultProps }) => (
-      <LoadingAndErrorWrapper loading={loading} error={error} noWrapper>
-        {() => <Visualization {...props} {...resultProps} />}
-      </LoadingAndErrorWrapper>
-    )}
+const QuestionLoadAndDisplay = ({
+  question,
+  onLoad,
+  keepPreviousWhileLoading,
+  ...props
+}) => (
+  <QuestionResultLoader
+    question={question}
+    onLoad={onLoad}
+    keepPreviousWhileLoading={keepPreviousWhileLoading}
+  >
+    {({ loading, error, ...resultProps }) => {
+      const shouldShowLoader = loading && resultProps.results == null;
+      return (
+        <LoadingAndErrorWrapper
+          loading={shouldShowLoader}
+          error={error}
+          noWrapper
+        >
+          <Visualization {...props} {...resultProps} />
+        </LoadingAndErrorWrapper>
+      );
+    }}
   </QuestionResultLoader>
 );
 

frontend/src/metabase/containers/QuestionResultLoader.jsx

 import React from "react";
+import PropTypes from "prop-types";
 import { defer } from "metabase/lib/promise";
 
 import type { Dataset } from "metabase-types/types/Dataset";
@@ -30,6 +31,13 @@ type State = {
   error: ?any,
 };
 
+const propTypes = {
+  question: PropTypes.object,
+  children: PropTypes.func,
+  onLoad: PropTypes.func,
+  keepPreviousWhileLoading: PropTypes.bool,
+};
+
 /*
  * Question result loader
  *
@@ -58,24 +66,22 @@ export class QuestionResultLoader extends React.Component {
 
   _cancelDeferred: ?() => void;
 
-  UNSAFE_componentWillMount() {
-    this._loadResult(this.props.question, this.props.onLoad);
-  }
+  UNSAFE_componentWillMount = () => {
+    this._reload();
+  };
 
-  UNSAFE_componentWillReceiveProps(nextProps: Props) {
+  UNSAFE_componentWillReceiveProps(nextProps) {
+    const { question, onLoad, keepPreviousWhileLoading } = nextProps;
     // if the question is different, we need to do a fresh load
-    if (
-      nextProps.question &&
-      !nextProps.question.isEqual(this.props.question)
-    ) {
-      this._loadResult(nextProps.question, nextProps.onLoad);
+    if (question && !question.isEqual(this.props.question)) {
+      this._loadResult(question, onLoad, keepPreviousWhileLoading);
     }
   }
 
   /*
    * load the result by calling question.apiGetResults
    */
-  async _loadResult(question: ?Question, onLoad: ?OnLoadCallback) {
+  async _loadResult(question, onLoad, keepPreviousWhileLoading) {
     // we need to have a question for anything to happen
     if (question) {
       try {
@@ -83,7 +89,11 @@ export class QuestionResultLoader extends React.Component {
         this._cancelDeferred = defer();
 
         // begin the request, set cancel in state so the query can be canceled
-        this.setState({ loading: true, results: null, error: null });
+        this.setState(prev => ({
+          loading: true,
+          results: keepPreviousWhileLoading ? prev.results : null,
+          error: null,
+        }));
 
         // call apiGetResults and pass our cancel to allow for cancelation
         const results: Dataset[] = await question.apiGetResults({
@@ -112,7 +122,8 @@ export class QuestionResultLoader extends React.Component {
    * load again
    */
   _reload = () => {
-    this._loadResult(this.props.question, this.props.onLoad);
+    const { question, onLoad, keepPreviousWhileLoading } = this.props;
+    this._loadResult(question, onLoad, keepPreviousWhileLoading);
   };
 
   /*
@@ -153,4 +164,10 @@ export class QuestionResultLoader extends React.Component {
   }
 }
 
+QuestionResultLoader.defaultProps = {
+  keepPreviousWhileLoading: false,
+};
+
+QuestionResultLoader.propTypes = propTypes;
+
 export default QuestionResultLoader;

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

@@ -18,6 +18,6 @@ describe("QuestionResultLoader", () => {
       </QuestionResultLoader>,
     );
 
-    expect(loadSpy).toHaveBeenCalledWith(question, undefined);
+    expect(loadSpy).toHaveBeenCalledWith(question, undefined, false);
   });
 });

frontend/test/metabase/scenarios/admin/audit/questions-audit.cy.spec.js

@@ -33,11 +33,11 @@ describeWithToken("audit > auditing > questions", () => {
 
       assertRowsOrder(QUERY_RUNS_DESC_ORDER);
 
-      cy.findByText("Query Runs").click();
+      cy.findByText("Query Runs (ms)").click();
 
       assertRowsOrder(QUERY_RUNS_ASC_ORDER);
 
-      cy.findByText("Query Runs").click();
+      cy.findByText("Query Runs (ms)").click();
 
       assertRowsOrder(QUERY_RUNS_DESC_ORDER);
     });