feat: SQL preview modal for Query History #11634

nytai · 2020-11-10T03:35:04Z

SUMMARY

Building off of #11574 this PR adds a preview/inspect modal to the Query History list view with a toggle for User inputted SQL and db executed SQL.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before:

After:

Also, added copy query button to Saved Query preview modal

TEST PLAN

unit tested
manually tested:
- empty User SQL
- empty Executed SQL
- previous button
- next button
- open in sql lab button
- copy query button
- list view row highlight

ADDITIONAL INFORMATION

Has associated issue:
Changes UI (behind feature flag SIP_34_QUERY_SEARCH_UI)
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

codecov-io · 2020-11-10T20:36:07Z

Codecov Report

Merging #11634 (01106e0) into master (68693c7) will increase coverage by 0.28%.
The diff coverage is 67.83%.

@@            Coverage Diff             @@
##           master   #11634      +/-   ##
==========================================
+ Coverage   62.86%   63.14%   +0.28%     
==========================================
  Files         889      900      +11     
  Lines       43055    43563     +508     
  Branches     4017     4199     +182     
==========================================
+ Hits        27065    27509     +444     
- Misses      15811    15874      +63     
- Partials      179      180       +1

Flag	Coverage Δ
javascript	`62.84% <67.83%> (+0.02%)`	⬆️
python	`63.32% <ø> (+0.44%)`	⬆️

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

Impacted Files	Coverage Δ
superset-frontend/src/common/components/index.tsx	`100.00% <ø> (ø)`
...rontend/src/messageToasts/enhancers/withToasts.tsx	`100.00% <ø> (ø)`
.../src/views/CRUD/data/savedquery/SavedQueryList.tsx	`61.36% <0.00%> (ø)`
superset/queries/api.py	`100.00% <ø> (ø)`
...UD/data/components/SyntaxHighlighterCopy/index.tsx	`35.55% <35.55%> (ø)`
...t-frontend/src/views/CRUD/data/query/QueryList.tsx	`72.38% <66.66%> (-3.18%)`	⬇️
superset-frontend/src/views/CRUD/data/hooks.ts	`71.42% <71.42%> (ø)`
...nd/src/views/CRUD/data/query/QueryPreviewModal.tsx	`97.67% <97.67%> (ø)`
...ws/CRUD/data/savedquery/SavedQueryPreviewModal.tsx	`100.00% <100.00%> (+14.54%)`	⬆️
superset/utils/cache.py	`57.57% <0.00%> (-22.43%)`	⬇️
... and 53 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 68693c7...01106e0. Read the comment docs.

eschutho · 2020-11-18T19:07:16Z

superset-frontend/src/views/CRUD/data/components/SyntaxHighlighterCopy/index.tsx

+    top: 40px;
+    left: 512px;
+    visibility: hidden;
+    margin: -4px;


small nit, but do you need the negative margin if you have the top and left relative positions?

That negative margin gets rid of the space that would be occupied by the element if it were not relative positioned. If we were using absolute positioning we wouldn't need it.

eschutho · 2020-11-18T19:14:09Z

superset-frontend/src/views/CRUD/data/components/SyntaxHighlighterCopy/index.tsx

+  }
+  const syntaxHighlighterProps = { ...props };
+  delete syntaxHighlighterProps.addDangerToast;
+  delete syntaxHighlighterProps.addSuccessToast;


would this work for you?
const { addDangerToast, addSuccessToast, ...syntaxHighlighterProps } = props;

do you need to remove 'props.children' from the syntaxHighlighterProps as well?

👏 why didn't I think of that!

eschutho · 2020-11-18T19:17:34Z

superset-frontend/src/views/CRUD/data/hooks.ts

+
+  function checkIndex() {
+    if (currentIndex === 0) {
+      setDisbalePrevious(true);


eschutho · 2020-11-18T19:19:57Z

superset-frontend/src/views/CRUD/data/hooks.ts

+    if (currentIndex === 0) {
+      setDisbalePrevious(true);
+    } else {
+      setDisbalePrevious(false);


could this be shortened to checkDisablePrevious(currentIndex === 0)

eschutho · 2020-11-18T19:20:42Z

superset-frontend/src/views/CRUD/data/hooks.ts

+    }
+
+    if (currentIndex === queries.length - 1) {
+      setDisbaleNext(true);


setDisableNext(currentIndex === queries.length - 1)?

riahk

👍 just a couple questions!

riahk · 2020-11-19T23:24:48Z

superset-frontend/src/views/CRUD/data/components/SyntaxHighlighterCopy/index.tsx

+import { ToastProps } from 'src/messageToasts/enhancers/withToasts';
+import Icon from 'src/components/Icon';
+
+SyntaxHighlighter.registerLanguage('sql', sql);


Would it make sense to add a language prop so we can use this component with json/css/etc.?

I added 'sql' | 'markdown' | 'html' | 'json' which is all I could find in the code base. Adding more, as needed, should be pretty simple.

riahk · 2020-11-19T23:32:45Z

superset-frontend/src/views/CRUD/data/query/QueryList.tsx

+              data-test={`open-sql-preview-${id}`}
+              onClick={() => setQueryCurrentlyPreviewing(original)}
+            >
+              <StyledSyntaxHighlighter language="sql" style={github}>


What's the difference between setting the language prop here and calling SyntaxHighlighter.registerLanguage above? Does the latter just load in the highlighting logic for while former explicitly sets it?

I think the SyntaxHighlighter ships with a few languages, and probably an interface to create your own. I think the registerLanguage step is to slim down the component and not include a bunch of unnecessary code that won't be used. I'll scan the codebase for other languages we use with SyntaxHighlighter and include those too.

riahk · 2020-11-19T23:35:09Z

superset-frontend/src/views/CRUD/data/hooks.ts

+type BaseQueryObject = {
+  id: number;
+};
+export function useQueryPreviewState<D extends BaseQueryObject = any>({


ktmud · 2020-11-21T01:50:12Z

superset-frontend/jest.config.js

+  setupFilesAfterEnv: [
+    '<rootDir>/node_modules/jest-enzyme/lib/index.js',
+    '<rootDir>/spec/helpers/shim.ts',
+  ],


@nytai any reason for this change?

I was having issues with shim.ts not loading when I was running tests in vscode -- mainly type errors. I was digging through some docs and most of them were using absolute paths. Is it causing issues?

superset-github-bot bot added the preset-io label Nov 10, 2020

pull-request-size bot added the size/XXL label Nov 10, 2020

nytai force-pushed the tai/query-history-preview branch 3 times, most recently from c031e49 to 1b67dfa Compare November 10, 2020 20:25

nytai force-pushed the tai/query-history-preview branch 4 times, most recently from f0a19a5 to 83b1561 Compare November 12, 2020 21:59

pull-request-size bot added size/XL and removed size/XXL labels Nov 12, 2020

nytai marked this pull request as ready for review November 13, 2020 02:26

nytai mentioned this pull request Nov 14, 2020

feat: list view filters for Query History #11702

Merged

6 tasks

nytai added 6 commits November 13, 2020 15:25

feat: SQL preview modal for Query History

0404b82

add spec

e5b0af1

move type

f1712a6

add spec for QueryPreviewModal

03ebe2c

fix type

d802158

fix react warning

1eaa405

nytai force-pushed the tai/query-history-preview branch from 43a6bf7 to 1eaa405 Compare November 14, 2020 01:26

eschutho reviewed Nov 18, 2020

View reviewed changes

address review feedback

1d42dad

riahk approved these changes Nov 19, 2020

View reviewed changes

add more syntax options for SyntaxHighlighterCopy

01106e0

willbarrett approved these changes Nov 20, 2020

View reviewed changes

nytai merged commit fbe4a66 into apache:master Nov 21, 2020

nytai deleted the tai/query-history-preview branch November 21, 2020 00:01

ktmud reviewed Nov 21, 2020

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: SQL preview modal for Query History #11634

feat: SQL preview modal for Query History #11634

nytai commented Nov 10, 2020 •

edited

Loading

codecov-io commented Nov 10, 2020 •

edited

Loading

eschutho Nov 18, 2020

nytai Nov 18, 2020

eschutho Nov 18, 2020

nytai Nov 18, 2020

eschutho Nov 18, 2020

eschutho Nov 18, 2020

eschutho Nov 18, 2020

riahk left a comment

riahk Nov 19, 2020

nytai Nov 20, 2020

riahk Nov 19, 2020

nytai Nov 20, 2020

riahk Nov 19, 2020

ktmud Nov 21, 2020

nytai Nov 21, 2020 •

edited

Loading

feat: SQL preview modal for Query History #11634

feat: SQL preview modal for Query History #11634

Conversation

nytai commented Nov 10, 2020 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov-io commented Nov 10, 2020 • 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

riahk left a 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

Choose a reason for hiding this comment

nytai Nov 21, 2020 • edited Loading

Choose a reason for hiding this comment

nytai commented Nov 10, 2020 •

edited

Loading

codecov-io commented Nov 10, 2020 •

edited

Loading

nytai Nov 21, 2020 •

edited

Loading