feat: Better return messages in SQL Editor (#14381)

* Sqllab limit

* Add migration script

* Set default values

* initial push

* revisions

* moving migration to separate PR

* revisions

* Fix apply_limit_to_sql

* all but tests

* added unit tests

* result set

* first draft

* revisions

* made user required prop, added it to all places ResultSet is imported

* changed QueryTable test to allow for useSelector

* Query Table working

* working with heights

* fixed scrolling

* got rid of animated

* fixed tests, revisions

* revisions

* revisions

* heights

* fun with heights

* alert state

* aaron helped me fix this

* better alert messages

* fixed result set test

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

superset-frontend/spec/javascripts/sqllab/QuerySearch_spec.jsx

@@ -26,9 +26,12 @@ import { supersetTheme, ThemeProvider } from '@superset-ui/core';
 import { fireEvent, render, screen, act } from '@testing-library/react';
 import '@testing-library/jest-dom/extend-expect';
 import userEvent from '@testing-library/user-event';
+import { user } from './fixtures';
 
 const mockStore = configureStore([thunk]);
-const store = mockStore({});
+const store = mockStore({
+  sqlLab: user,
+});
 
 const SEARCH_ENDPOINT = 'glob:*/superset/search_queries?*';
 const USER_ENDPOINT = 'glob:*/api/v1/query/related/user';

superset-frontend/spec/javascripts/sqllab/QueryTable_spec.jsx

@@ -17,11 +17,14 @@
  * under the License.
  */
 import React from 'react';
-import { shallow } from 'enzyme';
+import thunk from 'redux-thunk';
+import configureStore from 'redux-mock-store';
+import { styledMount as mount } from 'spec/helpers/theming';
 import QueryTable from 'src/SqlLab/components/QueryTable';
 import TableView from 'src/components/TableView';
 import { TableCollection } from 'src/components/dataViewCommon';
-import { queries } from './fixtures';
+import { Provider } from 'react-redux';
+import { queries, user } from './fixtures';
 
 describe('QueryTable', () => {
   const mockedProps = {
@@ -35,12 +38,18 @@ describe('QueryTable', () => {
     expect(React.isValidElement(<QueryTable {...mockedProps} />)).toBe(true);
   });
   it('renders a proper table', () => {
-    const wrapper = shallow(<QueryTable {...mockedProps} />);
-    const tableWrapper = wrapper
-      .find(TableView)
-      .shallow()
-      .find(TableCollection)
-      .shallow();
+    const mockStore = configureStore([thunk]);
+    const store = mockStore({
+      sqlLab: user,
+    });
+
+    const wrapper = mount(
+      <Provider store={store}>
+        <QueryTable {...mockedProps} />
+      </Provider>,
+    );
+    const tableWrapper = wrapper.find(TableView).find(TableCollection);
+
     expect(wrapper.find(TableView)).toExist();
     expect(tableWrapper.find('table')).toExist();
     expect(tableWrapper.find('table').find('thead').find('tr')).toHaveLength(1);

superset-frontend/spec/javascripts/sqllab/ResultSet_spec.jsx

@@ -37,6 +37,7 @@ import {
   runningQuery,
   stoppedQuery,
   initialState,
+  user,
 } from './fixtures';
 
 const mockStore = configureStore([thunk]);
@@ -54,8 +55,10 @@ describe('ResultSet', () => {
     },
     cache: true,
     query: queries[0],
-    height: 0,
+    height: 140,
     database: { allows_virtual_table_explore: true },
+    user,
+    defaultQueryLimit: 1000,
   };
   const stoppedQueryProps = { ...mockedProps, query: stoppedQuery };
   const runningQueryProps = { ...mockedProps, query: runningQuery };

superset-frontend/spec/javascripts/sqllab/fixtures.ts

@@ -208,6 +208,7 @@ export const queries = [
     executedSql: null,
     changed_on: '2016-10-19T20:56:06',
     rows: 42,
+    queryLimit: 100,
     endDttm: 1476910566798,
     limit_reached: false,
     schema: 'test_schema',
@@ -461,6 +462,18 @@ export const runningQuery = {
 };
 export const cachedQuery = { ...queries[0], cached: true };
 
+export const user = {
+  createdOn: '2021-04-27T18:12:38.952304',
+  email: 'admin',
+  firstName: 'admin',
+  isActive: true,
+  lastName: 'admin',
+  permissions: {},
+  roles: { Admin: Array(173) },
+  userId: 1,
+  username: 'admin',
+};
+
 export const initialState = {
   sqlLab: {
     offline: false,
@@ -473,6 +486,7 @@ export const initialState = {
     workspaceQueries: [],
     queriesLastUpdate: 0,
     activeSouthPaneTab: 'Results',
+    user: { user },
   },
   messageToasts: [],
   common: {

superset-frontend/src/SqlLab/components/QueryTable.jsx

@@ -22,8 +22,8 @@ import moment from 'moment';
 import Card from 'src/components/Card';
 import ProgressBar from 'src/components/ProgressBar';
 import Label from 'src/components/Label';
-import { t } from '@superset-ui/core';
-
+import { t, css } from '@superset-ui/core';
+import { useSelector } from 'react-redux';
 import TableView from 'src/components/TableView';
 import Button from 'src/components/Button';
 import { fDuration } from 'src/modules/dates';
@@ -53,6 +53,10 @@ const openQuery = id => {
   window.open(url);
 };
 
+const StaticPosition = css`
+  position: static;
+`;
+
 const QueryTable = props => {
   const columns = useMemo(
     () =>
@@ -64,6 +68,8 @@ const QueryTable = props => {
     [props.columns],
   );
 
+  const user = useSelector(({ sqlLab: { user } }) => user);
+
   const data = useMemo(() => {
     const restoreSql = query => {
       props.actions.queryEditorSetSql({ id: query.sqlEditorId }, query.sql);
@@ -129,7 +135,7 @@ const QueryTable = props => {
           </Button>
         );
         q.sql = (
-          <Card>
+          <Card css={[StaticPosition]}>
             <HighlightedSql
               sql={q.sql}
               rawSql={q.executedSql}
@@ -153,6 +159,7 @@ const QueryTable = props => {
               modalBody={
                 <ResultSet
                   showSql
+                  user={user}
                   query={query}
                   actions={props.actions}
                   height={400}

superset-frontend/src/SqlLab/components/ResultSet.tsx

@@ -30,6 +30,7 @@ import { debounce } from 'lodash';
 import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace';
 import { SaveDatasetModal } from 'src/SqlLab/components/SaveDatasetModal';
 import { put as updateDatset } from 'src/api/dataset';
+import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
 import Loading from '../../components/Loading';
 import ExploreCtasResultsButton from './ExploreCtasResultsButton';
 import ExploreResultsButton from './ExploreResultsButton';
@@ -42,8 +43,6 @@ import { exploreChart } from '../../explore/exploreUtils';
 import { CtasEnum } from '../actions/sqlLab';
 import { Query } from '../types';
 
-const SEARCH_HEIGHT = 46;
-
 enum DatasetRadioState {
   SAVE_NEW = 1,
   OVERWRITE_DATASET = 2,
@@ -56,6 +55,13 @@ const EXPLORE_CHART_DEFAULT = {
   viz_type: 'table',
 };
 
+enum LIMITING_FACTOR {
+  QUERY = 'QUERY',
+  QUERY_AND_DROPDOWN = 'QUERY_AND_DROPDOWN',
+  DROPDOWN = 'DROPDOWN',
+  NOT_LIMITED = 'NOT_LIMITED',
+}
+
 const LOADING_STYLES: CSSProperties = { position: 'relative', minHeight: 100 };
 
 interface DatasetOptionAutocomplete {
@@ -75,6 +81,8 @@ interface ResultSetProps {
   search?: boolean;
   showSql?: boolean;
   visualize?: boolean;
+  user: UserWithPermissionsAndRoles;
+  defaultQueryLimit: number;
 }
 
 interface ResultSetState {
@@ -88,6 +96,7 @@ interface ResultSetState {
   datasetToOverwrite: Record<string, any>;
   saveModalAutocompleteValue: string;
   userDatasetOptions: DatasetOptionAutocomplete[];
+  alertIsOpen: boolean;
 }
 
 // Making text render line breaks/tabs as is as monospace,
@@ -103,6 +112,10 @@ const MonospaceDiv = styled.div`
 const ReturnedRows = styled.div`
   font-size: 13px;
   line-height: 24px;
+  .limitMessage {
+    color: ${({ theme }) => theme.colors.secondary.light1};
+    margin-left: ${({ theme }) => theme.gridUnit * 2}px;
+  }
 `;
 const ResultSetControls = styled.div`
   display: flex;
@@ -146,8 +159,8 @@ export default class ResultSet extends React.PureComponent<
       datasetToOverwrite: {},
       saveModalAutocompleteValue: '',
       userDatasetOptions: [],
+      alertIsOpen: false,
     };
-
     this.changeSearch = this.changeSearch.bind(this);
     this.fetchResults = this.fetchResults.bind(this);
     this.popSelectStar = this.popSelectStar.bind(this);
@@ -207,6 +220,14 @@ export default class ResultSet extends React.PureComponent<
     }
   }
 
+  calculateAlertRefHeight = (alertElement: HTMLElement | null) => {
+    if (alertElement) {
+      this.setState({ alertIsOpen: true });
+    } else {
+      this.setState({ alertIsOpen: false });
+    }
+  };
+
   getDefaultDatasetName = () =>
     `${this.props.query.tab} ${moment().format('MM/DD/YYYY HH:mm:ss')}`;
 
@@ -321,12 +342,8 @@ export default class ResultSet extends React.PureComponent<
   getUserDatasets = async (searchText = '') => {
     // Making sure that autocomplete input has a value before rendering the dropdown
     // Transforming the userDatasetsOwned data for SaveModalComponent)
-    const appContainer = document.getElementById('app');
-    const bootstrapData = JSON.parse(
-      appContainer?.getAttribute('data-bootstrap') || '{}',
-    );
-
-    if (bootstrapData.user && bootstrapData.user.userId) {
+    const { userId } = this.props.user;
+    if (userId) {
       const queryParams = rison.encode({
         filters: [
           {
@@ -337,7 +354,7 @@ export default class ResultSet extends React.PureComponent<
           {
             col: 'owners',
             opr: 'rel_m_m',
-            value: bootstrapData.user.userId,
+            value: userId,
           },
         ],
         order_column: 'changed_on_delta_humanized',
@@ -501,25 +518,105 @@ export default class ResultSet extends React.PureComponent<
     return <div />;
   }
 
+  onAlertClose = () => {
+    this.setState({ alertIsOpen: false });
+  };
+
   renderRowsReturned() {
-    const { results, rows, queryLimit } = this.props.query;
+    const { results, rows, queryLimit, limitingFactor } = this.props.query;
+    let limitMessage;
     const limitReached = results?.displayLimitReached;
+    const isAdmin = !!this.props.user?.roles.Admin;
+    const displayMaxRowsReachedMessage = {
+      withAdmin: t(
+        `The number of results displayed is limited to %(rows)d by the configuration DISPLAY_MAX_ROWS. `,
+        { rows },
+      ).concat(
+        t(
+          `Please add additional limits/filters or download to csv to see more rows up to the`,
+        ),
+        t(`the %(queryLimit)d limit.`, { queryLimit }),
+      ),
+      withoutAdmin: t(
+        `The number of results displayed is limited to %(rows)d. `,
+        { rows },
+      ).concat(
+        t(
+          `Please add additional limits/filters, download to csv, or contact an admin`,
+        ),
+        t(`to see more rows up to the the %(queryLimit)d limit.`, {
+          queryLimit,
+        }),
+      ),
+    };
+    const shouldUseDefaultDropdownAlert =
+      queryLimit === this.props.defaultQueryLimit &&
+      limitingFactor === LIMITING_FACTOR.DROPDOWN;
+
+    if (limitingFactor === LIMITING_FACTOR.QUERY && this.props.csv) {
+      limitMessage = (
+        <span className="limitMessage">
+          {t(
+            `The number of rows displayed is limited to %(rows)d by the query`,
+            { rows },
+          )}
+        </span>
+      );
+    } else if (
+      limitingFactor === LIMITING_FACTOR.DROPDOWN &&
+      !shouldUseDefaultDropdownAlert
+    ) {
+      limitMessage = (
+        <span className="limitMessage">
+          {t(
+            `The number of rows displayed is limited to %(rows)d by the limit dropdown.`,
+            { rows },
+          )}
+        </span>
+      );
+    } else if (limitingFactor === LIMITING_FACTOR.QUERY_AND_DROPDOWN) {
+      limitMessage = (
+        <span className="limitMessage">
+          {t(
+            `The number of rows displayed is limited to %(rows)d by the query and limit dropdown.`,
+            { rows },
+          )}
+        </span>
+      );
+    }
     return (
       <ReturnedRows>
-        {!limitReached && (
-          <Alert type="warning" message={t(`%s rows returned`, rows)} />
+        {!limitReached && !shouldUseDefaultDropdownAlert && (
+          <span>
+            {t(`%(rows)d rows returned`, { rows })} {limitMessage}
+          </span>
+        )}
+        {!limitReached && shouldUseDefaultDropdownAlert && (
+          <div ref={this.calculateAlertRefHeight}>
+            <Alert
+              type="warning"
+              message={t(`%(rows)d rows returned`, { rows })}
+              onClose={this.onAlertClose}
+              description={t(
+                `The number of rows displayed is limited to %s by the dropdown.`,
+                rows,
+              )}
+            />
+          </div>
         )}
         {limitReached && (
-          <Alert
-            type="warning"
-            message={t(
-              `The number of results displayed is limited to %s. Please add
-            additional limits/filters or download to csv to see more rows up to
-            the %s limit.`,
-              rows,
-              queryLimit,
-            )}
-          />
+          <div ref={this.calculateAlertRefHeight}>
+            <Alert
+              type="warning"
+              onClose={this.onAlertClose}
+              message={t(`%(rows)d rows returned`, { rows })}
+              description={
+                isAdmin
+                  ? displayMaxRowsReachedMessage.withAdmin
+                  : displayMaxRowsReachedMessage.withoutAdmin
+              }
+            />
+          </div>
         )}
       </ReturnedRows>
     );
@@ -527,10 +624,6 @@ export default class ResultSet extends React.PureComponent<
 
   render() {
     const { query } = this.props;
-    const height = Math.max(
-      0,
-      this.props.search ? this.props.height - SEARCH_HEIGHT : this.props.height,
-    );
     let sql;
     let exploreDBId = query.dbId;
     if (this.props.database && this.props.database.explore_database_id) {
@@ -601,6 +694,9 @@ export default class ResultSet extends React.PureComponent<
     }
     if (query.state === 'success' && query.results) {
       const { results } = query;
+      const height = this.state.alertIsOpen
+        ? this.props.height - 70
+        : this.props.height;
       let data;
       if (this.props.cache && query.cached) {
         ({ data } = this.state);

superset-frontend/src/SqlLab/components/SouthPane/SouthPane.tsx

@@ -25,6 +25,7 @@ import { t, styled } from '@superset-ui/core';
 import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
 
 import Label from 'src/components/Label';
+import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
 import QueryHistory from '../QueryHistory';
 import ResultSet from '../ResultSet';
 import {
@@ -33,7 +34,7 @@ import {
   LOCALSTORAGE_MAX_QUERY_AGE_MS,
 } from '../../constants';
 
-const TAB_HEIGHT = 90;
+const TAB_HEIGHT = 140;
 
 /*
     editorQueries are queries executed by users passed from SqlEditor component
@@ -49,6 +50,8 @@ interface SouthPanePropTypes {
   databases: Record<string, any>;
   offline?: boolean;
   displayLimit: number;
+  user: UserWithPermissionsAndRoles;
+  defaultQueryLimit: number;
 }
 
 const StyledPane = styled.div`
@@ -61,6 +64,16 @@ const StyledPane = styled.div`
     height: 100%;
     display: flex;
     flex-direction: column;
+    .scrollable {
+      overflow-y: auto;
+    }
+  }
+  .ant-tabs-tabpane {
+    display: flex;
+    flex-direction: column;
+    .scrollable {
+      overflow-y: auto;
+    }
   }
   .tab-content {
     .alert {
@@ -83,13 +96,14 @@ export default function SouthPane({
   databases,
   offline = false,
   displayLimit,
+  user,
+  defaultQueryLimit,
 }: SouthPanePropTypes) {
   const innerTabContentHeight = height - TAB_HEIGHT;
   const southPaneRef = createRef<HTMLDivElement>();
   const switchTab = (id: string) => {
     actions.setActiveSouthPaneTab(id);
   };
-
   const renderOfflineStatus = () => (
     <Label className="m-r-3" type={STATE_TYPE_MAP[STATUS_OPTIONS.offline]}>
       {STATUS_OPTIONS.offline}
@@ -127,9 +141,11 @@ export default function SouthPane({
             search
             query={latestQuery}
             actions={actions}
+            user={user}
             height={innerTabContentHeight}
             database={databases[latestQuery.dbId]}
             displayLimit={displayLimit}
+            defaultQueryLimit={defaultQueryLimit}
           />
         );
       }
@@ -153,8 +169,10 @@ export default function SouthPane({
           csv={false}
           actions={actions}
           cache
+          user={user}
           height={innerTabContentHeight}
           displayLimit={displayLimit}
+          defaultQueryLimit={defaultQueryLimit}
         />
       </Tabs.TabPane>
     ));

superset-frontend/src/SqlLab/components/SouthPane/state.ts

@@ -26,6 +26,7 @@ function mapStateToProps({ sqlLab }: Record<string, any>) {
     activeSouthPaneTab: sqlLab.activeSouthPaneTab,
     databases: sqlLab.databases,
     offline: sqlLab.offline,
+    user: sqlLab.user,
   };
 }
 

superset-frontend/src/SqlLab/components/SqlEditor.jsx

@@ -483,6 +483,7 @@ class SqlEditor extends React.PureComponent {
           actions={this.props.actions}
           height={southPaneHeight}
           displayLimit={this.props.displayLimit}
+          defaultQueryLimit={this.props.defaultQueryLimit}
         />
       </Split>
     );

superset-frontend/src/SqlLab/main.less

@@ -294,7 +294,7 @@ div.Workspace {
   .queryPane {
     flex: 1 1 auto;
     padding-left: 10px;
-    overflow-y: visible;
+    overflow-y: none;
     overflow-x: scroll;
   }
 
@@ -436,6 +436,10 @@ div.tablePopover {
   display: inline-block;
 }
 
+.QueryTable .ant-btn {
+  position: static;
+}
+
 .ResultsModal .ant-modal-body {
   min-height: 560px;
 }

superset-frontend/src/components/FilterableTable/FilterableTable.tsx

@@ -83,8 +83,10 @@ const JSON_TREE_THEME = {
 };
 
 const StyledFilterableTable = styled.div`
+  height: 100%;
   overflow-x: auto;
   margin-top: ${({ theme }) => theme.gridUnit * 2}px;
+  overflow-y: hidden;
 `;
 
 // when more than MAX_COLUMNS_FOR_TABLE are returned, switch from table to grid view
@@ -466,7 +468,6 @@ export default class FilterableTable extends PureComponent<
         <ScrollSync>
           {({ onScroll, scrollTop }) => (
             <div
-              style={{ height }}
               className="filterable-table-container Table"
               data-test="filterable-table-container"
               ref={this.container}
@@ -559,7 +560,6 @@ export default class FilterableTable extends PureComponent<
       this.getDatum(sortedAndFilteredList, index);
     return (
       <StyledFilterableTable
-        style={{ height }}
         className="filterable-table-container"
         ref={this.container}
       >

superset/views/core.py

@@ -2905,7 +2905,7 @@ class Superset(BaseSupersetView):  # pylint: disable=too-many-public-methods
             except json.JSONDecodeError:
                 pass
 
-        payload["user"] = bootstrap_user_data(g.user)
+        payload["user"] = bootstrap_user_data(g.user, include_perms=True)
         bootstrap_data = json.dumps(
             payload, default=utils.pessimistic_json_iso_dttm_ser
         )