feat: Better return messages in SQL Editor #14381

AAfghahi · 2021-04-27T20:58:12Z

SUMMARY

We wanted more robust messaging in Sql Lab that tells user what, if anything, is limiting the results that they get when they run a SQL query. Currently this file will also include a database migration, we are going to wait for this migration to resolve before we continue with this PR.

Because of the varied heights of the alert component, this PR will also fix some of the code in SQL Editor and SouthPane that uses static height.

This has custom messages that are based on:

DISPLAY_MAX_ROW causing the results to be limited (admin and non-admin messaging)
The query itself limiting the options
the dropdown limiting results
the default dropdown of 1000 being the cause for the limit

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

display max row for admin

non-admin

Query Limit

Dropdown limit:

Default Dropdown limit of 1000

TEST PLAN

Visual Testing

ADDITIONAL INFORMATION

Has associated issue:
[ x] Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

Steejay · 2021-04-27T21:08:05Z

@AAfghahi the non admin messsage should also have a header that displays the row returned number.

AAfghahi · 2021-04-27T21:10:48Z

@AAfghahi the non admin messsage should also have a header that displays the row returned number.

Ah, they do. My picture was of the wrong thing. TY!

codecov · 2021-04-27T21:24:22Z

Codecov Report

Merging #14381 (af14960) into master (6871ad1) will increase coverage by 0.28%.
The diff coverage is 59.52%.

@@            Coverage Diff             @@
##           master   #14381      +/-   ##
==========================================
+ Coverage   77.09%   77.37%   +0.28%     
==========================================
  Files         959      958       -1     
  Lines       48309    48562     +253     
  Branches     5661     5714      +53     
==========================================
+ Hits        37243    37577     +334     
+ Misses      10866    10783      -83     
- Partials      200      202       +2

Flag	Coverage Δ
javascript	`72.48% <60.97%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...-frontend/src/SqlLab/components/SouthPane/state.ts	`100.00% <ø> (ø)`
...erset-frontend/src/SqlLab/components/SqlEditor.jsx	`54.62% <ø> (ø)`
...src/components/FilterableTable/FilterableTable.tsx	`82.26% <ø> (ø)`
superset/views/core.py	`75.48% <0.00%> (-0.04%)`	⬇️
...erset-frontend/src/SqlLab/components/ResultSet.tsx	`67.31% <57.89%> (-2.69%)`	⬇️
...rset-frontend/src/SqlLab/components/QueryTable.jsx	`67.85% <100.00%> (+1.19%)`	⬆️
...tend/src/SqlLab/components/SouthPane/SouthPane.tsx	`79.54% <100.00%> (ø)`
...uperset-frontend/src/components/Form/FormLabel.tsx	`68.75% <0.00%> (-31.25%)`	⬇️
superset-frontend/src/profile/components/App.tsx	`85.71% <0.00%> (-14.29%)`	⬇️
...ontend/src/common/hooks/apiResources/dashboards.ts	`40.00% <0.00%> (-10.00%)`	⬇️
... and 85 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6871ad1...af14960. Read the comment docs.

eschutho · 2021-04-28T00:17:16Z

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

+    let limitMessage;
+    const appContainer = document.getElementById('app');
+    const bootstrapData = JSON.parse(
+      appContainer?.getAttribute('data-bootstrap') || '{}',


this is in redux already.. can you get it from there?

Of course! I did it this way because this is how user was accessed earlier in the file:
https://github.com/apache/superset/blob/master/superset-frontend/src/SqlLab/components/ResultSet.tsx#L321

should I amend both?

oh, I see, yes that would be great if you could update both.. thanks!

eschutho · 2021-04-28T15:42:48Z

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

+      appContainer?.getAttribute('data-bootstrap') || '{}',
+    );
+    const isAdmin = bootstrapData.user?.roles?.hasOwnProperty('Admin');
+    const defaultDropwdown =


oops! embarrassing. TY for the catch.

eschutho · 2021-04-28T15:43:01Z

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

+    );
+    const isAdmin = bootstrapData.user?.roles?.hasOwnProperty('Admin');
+    const defaultDropwdown =
+      limitingFactor === 'DROPDOWN' && queryLimit === 1000;


can we put 1000 into a const as well?

github-actions · 2021-04-29T21:38:45Z

⚠️ @AAfghahi Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

github-actions · 2021-05-03T12:34:26Z

⚠️ @AAfghahi Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

hughhhh · 2021-05-03T16:58:01Z

/testenv up

github-actions · 2021-05-03T16:59:51Z

@hughhhh Ephemeral environment spinning up at http://54.212.228.251:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

eschutho · 2021-05-04T16:24:06Z

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

-      .shallow();
+    const mockStore = configureStore([thunk]);
+    const store = mockStore({});
+    const spyOnUseSelector = jest.spyOn(redux, 'useSelector');


instead of using a spy for this, can you push the data that you need into the store when you load it with the provider?

Ok, I added it to the store, and added user as a fixture.

eschutho · 2021-05-04T16:26:12Z

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

-      actions={actions}
-      displayLimit={displayLimit}
-    />
+    <div className="scrollable">


I'd suggest what Evan was demoing the other day and use emotion's css property here instead: https://codesandbox.io/s/emotion-playground-forked-7z07l?file=/src/index.js
That way you can reuse a scrollable style instead of a class.

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

eschutho · 2021-05-04T16:27:51Z

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

@@ -64,6 +70,8 @@ const QueryTable = props => {
    [props.columns],
  );

+  const user = useSelector(({ sqlLab: { user } }) => user);


nice destructuring there.

eschutho · 2021-05-04T16:31:10Z

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

            buttonSize="small"
            buttonStyle="link"
            onClick={() => openQuery(q.queryId)}
          >
            <i className="fa fa-external-link m-r-3" />
            {t('Edit')}
-          </Button>
+          </StyledButton>


it feels a little weird to make all of these buttons static. Can we put them in a wrapper that is static instead?

I realized after pulling this down and looking at it that there isn't nothing necessarily significant with the static style, it just doesn't seem to like the current relative one. The changes that you made look good.

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

eschutho · 2021-05-04T16:32:35Z

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

+          )}
+        </span>
+      );
+    } else if (limitingFactor === 'QUERY_AND_DROPDOWN') {


can you put these constants into an enum?

rusackas · 2021-05-05T18:43:52Z

Fixes #14455 as bycatch

betodealmeida · 2021-05-12T18:50:28Z

/testenv up

github-actions · 2021-05-12T20:12:44Z

@betodealmeida Ephemeral environment spinning up at http://54.202.234.102:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

betodealmeida · 2021-05-12T22:03:02Z

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

+}
+
+function defaultDropdown(queryLimit: number, limitingFactor: string) {
+  return queryLimit === 1000 && limitingFactor === LIMITING_FACTOR.DROPDOWN;


I don't remember, is the 1000 default query limit configurable? If so, we should read the configured value. Even if not, we should move this to a constant.

yeah! I just found it, and am now piping it into result set for this reason.

betodealmeida · 2021-05-12T22:03:30Z

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

+function limitReached(displayLimitReached: boolean) {
+  return displayLimitReached;
+}


This function doesn't do anything, we should remove it.

eschutho · 2021-05-12T22:43:16Z

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

@@ -205,6 +227,17 @@ export default class ResultSet extends React.PureComponent<
    ) {
      this.fetchResults(nextProps.query);
    }
+    if (


is there any way to close the alert and then have it update props without running a new query?

eschutho · 2021-05-12T22:45:55Z

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

-    );
-
-    if (bootstrapData.user && bootstrapData.user.userId) {
+    if (this.props.user?.userId) {


since you use this value twice here, do you want to destructure just the userId out?

eschutho · 2021-05-12T22:49:06Z

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

-    const limitReached = results?.displayLimitReached;
+    const { results, rows, queryLimit, limitingFactor } = this.props.query;
+    let limitMessage;
+    const isAdmin = this.props.user?.roles.hasOwnProperty('Admin');


why hasOwnProperty here? are you looking for a boolean? !!this.props.user?roles.Admin

or you could also Object.keys(this.props.user?.roles).includes("Admin")

it's not that hasOwnProperty doesn't work, it's just that it was mainly used to make sure that the object isn't inheriting the property from something up the prototype chain, and so I was trying to think about why that was an issue. I think the other methods are more common nowadays.

eschutho · 2021-05-12T22:56:23Z

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

+    const isAdmin = this.props.user?.roles.hasOwnProperty('Admin');
+    const adminWarning = isAdmin
+      ? ' by the configuration DISPLAY_MAX_ROWS'
+      : null;


if you only have one option, it's better to use && `const adminWarning = isAdmin && ' by the....'. it will be falsy otherwise, so setting it to null is redundant, unless you explicitly need null.

Yeah I explicitly need null here otherwise it returns undefined in the Alert

description={t( `The number of results displayed is limited to %s%s. Please add additional limits/filters or download to csv to see more rows up to the %s limit. %s`, rows, adminWarning, queryLimit, )}

My original method was two separate Alerts one for admin and one for non-admin but playing around with it, I tried this way and it worked and was less lines. Happy to go back to the other method though if the construction with && is DRY-er

oh, ok.. then can you do an empty string?

yeah that works, ty.

eschutho · 2021-05-12T22:58:36Z

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

+  return displayLimitReached;
+}
+
+function defaultDropdown(queryLimit: number, limitingFactor: string) {


can we make the name more indicative of what this function does? Does it return a boolean, then maybe shouldShowDefaultDropdown? or shouldUse... whatever fits better.

eschutho · 2021-05-12T23:00:33Z

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

+            </span>
+          )}
+        {!limitReached(results?.displayLimitReached) &&
+          defaultDropdown(queryLimit, limitingFactor) && (


do you want to DRY this up and get the value above and assign it to a const to reuse?

eschutho · 2021-05-12T23:01:06Z

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

+              )}
+            />
+          )}
+        {limitReached(results?.displayLimitReached) && (


same thing here.. you can dry up this value

eschutho · 2021-05-12T23:04:02Z

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

+            description={t(
+              `The number of results displayed is limited to %s%s. Please add
+              additional limits/filters or download to csv to see more rows up to
+              the %s limit. %s`,


do I see four string placeholders above, but three values below? AdminWarning isn't going to get translated either, btw.

eschutho

Just minor nits for me.. looks good!

eschutho · 2021-05-17T16:01:55Z

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

+        `The number of results displayed is limited to %(rows)d.
+        Please add additional limits/filters, download to csv, or contact an admin
+        to see more rows up to the the %(queryLimit)d limit.`,
+        { rows, queryLimit },


one other thing I found is that multiline strings don't work well with the translations. I'd recommend breaking this into 3-4 strings.

oh interesting, ok I will

eschutho · 2021-05-17T16:03:31Z

@yousoph do you want to take another pass at a visual review or are you good on this? I think it's ready to go, with one small change left.

github-actions · 2021-05-18T23:26:23Z

Ephemeral environment shutdown and build artifacts deleted.

* 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>

AAfghahi requested a review from a team as a code owner April 27, 2021 20:58

pull-request-size bot added the size/L label Apr 27, 2021

AAfghahi mentioned this pull request Apr 27, 2021

chore: SQLLab row limit warning using Alert component #14298

Merged

8 tasks

AAfghahi force-pushed the ch5675_returnMessages branch from 5c159bd to bceeb54 Compare April 27, 2021 21:01

AAfghahi force-pushed the ch5675_returnMessages branch from bceeb54 to 3e35242 Compare April 27, 2021 21:08

AAfghahi force-pushed the ch5675_returnMessages branch from 3e35242 to d455991 Compare April 27, 2021 22:05

eschutho reviewed Apr 28, 2021

View reviewed changes

AAfghahi force-pushed the ch5675_returnMessages branch 2 times, most recently from 22eca93 to f346554 Compare April 29, 2021 18:10

AAfghahi force-pushed the ch5675_returnMessages branch 3 times, most recently from 378e720 to 66cf78c Compare May 3, 2021 15:59

eschutho reviewed May 4, 2021

View reviewed changes

superset-frontend/src/SqlLab/components/QueryTable.jsx Show resolved Hide resolved

eschutho reviewed May 4, 2021

View reviewed changes

superset-frontend/src/SqlLab/components/ResultSet.tsx Outdated Show resolved Hide resolved

eschutho reviewed May 4, 2021

View reviewed changes

AAfghahi mentioned this pull request May 5, 2021

Extra scrollbars in SQL lab #14455

Closed

betodealmeida reviewed May 12, 2021

View reviewed changes

eschutho reviewed May 12, 2021

View reviewed changes

AAfghahi force-pushed the ch5675_returnMessages branch 2 times, most recently from 3900eed to b834013 Compare May 14, 2021 21:01

aaron helped me fix this

867acf0

AAfghahi force-pushed the ch5675_returnMessages branch from b834013 to 867acf0 Compare May 14, 2021 21:31

better alert messages

41950fc

eschutho reviewed May 17, 2021

View reviewed changes

fixed result set test

af14960

AAfghahi force-pushed the ch5675_returnMessages branch from 4378548 to af14960 Compare May 17, 2021 16:27

betodealmeida approved these changes May 18, 2021

View reviewed changes

betodealmeida merged commit a7a011c into apache:master May 18, 2021

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 2024

feat: Better return messages in SQL Editor #14381

feat: Better return messages in SQL Editor #14381

Conversation

AAfghahi commented Apr 27, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

Steejay commented Apr 27, 2021

AAfghahi commented Apr 27, 2021

codecov bot commented Apr 27, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 29, 2021

github-actions bot commented May 3, 2021

hughhhh commented May 3, 2021

github-actions bot commented May 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rusackas commented May 5, 2021

betodealmeida commented May 12, 2021

github-actions bot commented May 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho May 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AAfghahi May 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho commented May 17, 2021

github-actions bot commented May 18, 2021

AAfghahi commented Apr 27, 2021 •

edited

Loading

codecov bot commented Apr 27, 2021 •

edited

Loading

eschutho May 4, 2021 •

edited

Loading

eschutho May 12, 2021 •

edited

Loading

AAfghahi May 13, 2021 •

edited

Loading