feat: DBC-UI Globally available across the app 🌎 #18722

hughhhh · 2022-02-14T20:34:13Z

SUMMARY

We've now have added Connect Database + Connect Google Sheets to the new main nav. To allow users easier entry point for connecting their database or a gsheet

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Connecting DB via Postgres

saving_db.mov

Connecting GSheet

gsheets_global.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
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

Co-authored-by: Hugh A. Miles II <hughmil3s@gmail.com>

into chore-change-nav

…-add-db-modal

into hugh-add-db-modal

codecov · 2022-02-14T20:49:07Z

Codecov Report

Merging #18722 (192a7f4) into master (2421d17) will increase coverage by 0.35%.
The diff coverage is 55.00%.

@@            Coverage Diff             @@
##           master   #18722      +/-   ##
==========================================
+ Coverage   66.32%   66.67%   +0.35%     
==========================================
  Files        1620     1634      +14     
  Lines       63087    65054    +1967     
  Branches     6372     6616     +244     
==========================================
+ Hits        41840    43376    +1536     
- Misses      19590    19967     +377     
- Partials     1657     1711      +54

Flag	Coverage Δ
hive	`52.74% <100.00%> (+0.51%)`	⬆️
javascript	`51.13% <43.75%> (-0.13%)`	⬇️
mysql	`81.93% <100.00%> (+0.50%)`	⬆️
postgres	`81.97% <100.00%> (+0.49%)`	⬆️
presto	`52.55% <100.00%> (+0.48%)`	⬆️
python	`82.43% <100.00%> (+0.53%)`	⬆️
sqlite	`81.68% <100.00%> (+0.52%)`	⬆️

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

Impacted Files	Coverage Δ
superset-frontend/src/views/components/Menu.tsx	`56.41% <ø> (ø)`
...perset-frontend/src/views/components/MenuRight.tsx	`72.13% <43.75%> (-7.87%)`	⬇️
superset/views/base.py	`75.08% <100.00%> (+0.33%)`	⬆️
superset/reports/notifications/slack.py	`85.43% <0.00%> (-1.89%)`	⬇️
...ilters/FilterBar/FilterControls/FilterControls.tsx	`76.36% <0.00%> (-0.78%)`	⬇️
...board/components/nativeFilters/FilterBar/index.tsx	`69.54% <0.00%> (-0.29%)`	⬇️
superset/db_engine_specs/hive.py	`85.53% <0.00%> (-0.18%)`	⬇️
superset/sql_parse.py	`99.36% <0.00%> (-0.12%)`	⬇️
superset-frontend/src/dashboard/constants.ts	`100.00% <0.00%> (ø)`
superset-frontend/src/components/Button/index.tsx	`100.00% <0.00%> (ø)`
... and 39 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 2421d17...192a7f4. Read the comment docs.

lyndsiWilliams

A quick non-blocking question, otherwise this looks good to me!

lyndsiWilliams · 2022-02-22T19:05:50Z

superset-frontend/src/views/components/MenuRight.tsx

@@ -144,9 +151,25 @@ const RightMenu = ({
      {menu.label}
    </>
  );
+
+  const handleMenuSelection = (itemChose: any) => {


Is itemChose a complicated type or would this be able to be defined with more specific than any?

eschutho · 2022-02-23T19:02:43Z

superset-frontend/src/views/components/MenuRight.tsx

-                          <a href={item.url}> {item.label} </a>
-                        </Menu.Item>
+                        <>
+                          {idx === 2 && <Menu.Divider />}


maybe for future-proofing this, we can say idx > 1 instead of comparing it to 2 specifically?

if we go above idx > 1, this would render after every menu item after the first 2 items. We just want it to render it once right after the db connection buttons. Thats the one of the main reason i split out the db connection buttons from the config.

Ok, I see the complication. Again I would suggest separating the concerns between the data at the top and the rendering at the bottom. You could add the logic to the data with something like:

{ label: t('Connect Google Sheet'), name: GlobalMenuDataOptions.GOOGLE_SHEETS, perm: HAS_GSHEETS_INSTALLED, }, '---------', { label: t('Upload a CSV'), name: 'Upload a CSV', url: '/csvtodatabaseview/form', perm: CSV_EXTENSIONS, },

and then you can render the divider if typeof item === 'string'
or make it an object with a type of "divider"...

This is something we can discuss later.

eschutho · 2022-02-23T19:06:30Z

superset/views/base.py

@@ -366,6 +368,10 @@ def common_bootstrap_payload() -> Dict[str, Any]:
            ReportRecipientType.EMAIL,
        ]

+    # verify client has google sheets installed
+    available_specs = get_available_engine_specs()
+    frontend_config["SHOW_GLOBAL_GSHEETS"] = bool(available_specs[GSheetsEngineSpec])


bump of an earlier comment.. can we name this something more generic like HAS_GSHEETS_INSTALLED so that it's less tied to the client side functionality?

eschutho · 2022-02-23T19:10:15Z

superset-frontend/src/views/components/MenuRight.tsx

  // if user has any of these roles the dropdown will appear
  const configMap = {
    'Upload a CSV': CSV_EXTENSIONS,
    'Upload a Columnar file': COLUMNAR_EXTENSIONS,
    'Upload Excel': EXCEL_EXTENSIONS,
+    [GlobalMenuDataOptions.GOOGLE_SHEETS]: SHOW_GLOBAL_GSHEETS,
+    [GlobalMenuDataOptions.DB_CONNECTION]: true, // todo(hugh): add permission check here for database creation


instead of a separate map here, how about we move these values into the upper dropdownItems map with a key of something like has_perm? Then below instead of configMap[item.name] === true you can say item.has_perm

eschutho · 2022-02-24T01:30:34Z

superset-frontend/src/views/components/MenuRight.tsx

+                          {idx === 2 && <Menu.Divider />}
+                          <Menu.Item key={item.name}>
+                            {item.url && <a href={item.url}> {item.label} </a>}
+                            {item.url === undefined && item.label}


nit, but instead of two lines can you combine these by making it a ternary statement?

eschutho · 2022-02-24T18:21:00Z

@AAfghahi how does this look to you?

This reverts commit 209e3f4.

pkdotson and others added 15 commits February 8, 2022 18:44

more data nav menu

681c20e

fix lint and fix nav css

a4bbd5e

update test and remove icons

2ae8696

Update superset-frontend/src/views/components/Menu.test.tsx

237263a

Co-authored-by: Hugh A. Miles II <hughmil3s@gmail.com>

Apply suggestions from code review

04d1ff9

use backend app.link to show new nav changes

1c0f383

Merge branch 'chore-change-nav' of https://github.com/preset-io/superset

760d8c6

into chore-change-nav

fix lint

717c5bb

update test

2502a9a

usetheme and remove chaining

123eff6

add more suggestions

785f74b

fix lint

757d502

Merge branch 'master' of https://github.com/apache/superset into hugh…

2780d60

…-add-db-modal

Merge branch 'chore-change-nav' of https://github.com/preset-io/superset

8052f1f

into hugh-add-db-modal

working global db connection

b68aa95

pull-request-size bot added the size/L label Feb 14, 2022

pkdotson and others added 10 commits February 15, 2022 12:08

add allowed extensions to bootstrap and hard code links

a6c0bbe

remove backend links

0e9abe9

fix conflicts

70e7ba4

fix test

117261e

apply stashed gsheets

4fc95cd

fix check for google sheets

dca3be1

setup gsheets

b45184b

add extensions to frontend conf

c42491d

fix test and add be changes

9eac285

fix

a3cc781

hughhhh changed the title ~~[WIP] follow for connecting DB globally~~ feat: DBC-UI Globally available across the app 🌎 Feb 16, 2022

hughhhh and others added 2 commits February 15, 2022 19:13

remove package json changes

e64d6eb

test is python test passes

6e9eb0c

hughhhh force-pushed the hugh-add-db-modal branch 3 times, most recently from c0dc697 to d161dea Compare February 18, 2022 19:26

clean up test

58d7607

hughhhh force-pushed the hugh-add-db-modal branch 2 times, most recently from c560ec0 to 969b96e Compare February 22, 2022 06:10

address concerns

b1aeaa4

hughhhh force-pushed the hugh-add-db-modal branch from 969b96e to b1aeaa4 Compare February 22, 2022 18:32

lyndsiWilliams approved these changes Feb 22, 2022

View reviewed changes

hughhhh requested review from eschutho and AAfghahi February 22, 2022 21:12

eschutho reviewed Feb 23, 2022

View reviewed changes

hughhhh force-pushed the hugh-add-db-modal branch 2 times, most recently from 9ae0edb to dbae5cf Compare February 24, 2022 00:35

address concerns

e2b0570

hughhhh force-pushed the hugh-add-db-modal branch from dbae5cf to e2b0570 Compare February 24, 2022 01:17

eschutho reviewed Feb 24, 2022

View reviewed changes

change to tenarary

192a7f4

eschutho approved these changes Feb 24, 2022

View reviewed changes

AAfghahi approved these changes Feb 24, 2022

View reviewed changes

hughhhh merged commit 209e3f4 into master Feb 24, 2022

sadpandajoe added a commit to preset-io/superset that referenced this pull request Mar 7, 2022

Revert "feat: DBC-UI Globally available across the app 🌎 (apache#18722)"

8c74f1d

This reverts commit 209e3f4.

hughhhh added a commit to preset-io/superset that referenced this pull request Mar 7, 2022

Revert "feat: DBC-UI Globally available across the app 🌎 (apache#18722)"

8045c3e

This reverts commit 209e3f4.

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

mistercrunch deleted the hugh-add-db-modal branch March 26, 2024 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: DBC-UI Globally available across the app 🌎 #18722

feat: DBC-UI Globally available across the app 🌎 #18722

hughhhh commented Feb 14, 2022 •

edited

Loading

codecov bot commented Feb 14, 2022 •

edited

Loading

lyndsiWilliams left a comment

lyndsiWilliams Feb 22, 2022

eschutho Feb 23, 2022

hughhhh Feb 23, 2022

eschutho Feb 24, 2022

eschutho Feb 24, 2022

eschutho Feb 23, 2022

eschutho Feb 23, 2022

eschutho Feb 24, 2022

eschutho commented Feb 24, 2022

feat: DBC-UI Globally available across the app 🌎 #18722

feat: DBC-UI Globally available across the app 🌎 #18722

Conversation

hughhhh commented Feb 14, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Connecting DB via Postgres

Connecting GSheet

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Feb 14, 2022 • edited Loading

Codecov Report

lyndsiWilliams 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho commented Feb 24, 2022

hughhhh commented Feb 14, 2022 •

edited

Loading

codecov bot commented Feb 14, 2022 •

edited

Loading