Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 42 commits into from
Feb 24, 2022
Merged

Conversation

hughhhh
Copy link
Member

@hughhhh hughhhh commented Feb 14, 2022

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

@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

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

Impacted file tree graph

@@            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.

@hughhhh hughhhh changed the title [WIP] follow for connecting DB globally feat: DBC-UI Globally available across the app 🌎 Feb 16, 2022
@hughhhh hughhhh force-pushed the hugh-add-db-modal branch 3 times, most recently from c0dc697 to d161dea Compare February 18, 2022 19:26
@hughhhh hughhhh force-pushed the hugh-add-db-modal branch 2 times, most recently from c560ec0 to 969b96e Compare February 22, 2022 06:10
Copy link
Member

@lyndsiWilliams lyndsiWilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

const handleMenuSelection = (itemChose: any) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<a href={item.url}> {item.label} </a>
</Menu.Item>
<>
{idx === 2 && <Menu.Divider />}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something we can discuss later.

@@ -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])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@hughhhh hughhhh force-pushed the hugh-add-db-modal branch 2 times, most recently from 9ae0edb to dbae5cf Compare February 24, 2022 00:35
{idx === 2 && <Menu.Divider />}
<Menu.Item key={item.name}>
{item.url && <a href={item.url}> {item.label} </a>}
{item.url === undefined && item.label}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@eschutho
Copy link
Member

@AAfghahi how does this look to you?

@hughhhh 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
hughhhh added a commit to preset-io/superset that referenced this pull request Mar 7, 2022
@mistercrunch 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 mistercrunch deleted the hugh-add-db-modal branch March 26, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants