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: postgres connection form #14456

Closed

Conversation

eschutho
Copy link
Member

@eschutho eschutho commented May 4, 2021

SUMMARY

This PR adds the functionality of calling databases/available api to get a list of databases that have drivers loaded and available to use. The form is built dynamically based on that information. This is currently behind a local flag, and there are no visual changes, but the piece in development is as shown below. More work is needed to hook the form up to the api on connect/save, but I'm merging often to reduce any rebasing issues and also keep PRs small.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

This is behind a local flag useSqlAlchemyForm which is set to 'false'
_DEV__Superset

TEST PLAN

visual changes only right now, so all existing tests should pass

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

@@ -241,6 +253,8 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
}
if (show) {
setTabKey(DEFAULT_TAB_KEY);
getAvailableDbs();
setSelectedDb('postgresql');
Copy link
Member Author

Choose a reason for hiding this comment

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

this is hardcoded for now, and doesn't do anything unless useSqlAlchemyForm is true. Eventually the db will be chosen by the user on a different step.

@codecov
Copy link

codecov bot commented May 4, 2021

Codecov Report

Merging #14456 (6e508bd) into master (6d786d4) will decrease coverage by 0.03%.
The diff coverage is 44.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14456      +/-   ##
==========================================
- Coverage   77.40%   77.37%   -0.04%     
==========================================
  Files         958      959       +1     
  Lines       48329    48388      +59     
  Branches     5679     5689      +10     
==========================================
+ Hits        37410    37438      +28     
- Misses      10719    10750      +31     
  Partials      200      200              
Flag Coverage Δ
javascript 72.40% <44.77%> (-0.07%) ⬇️

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

Impacted Files Coverage Δ
...RUD/data/database/DatabaseModal/SqlAlchemyForm.tsx 100.00% <ø> (ø)
...c/views/CRUD/data/database/DatabaseModal/styles.ts 69.86% <13.04%> (-26.14%) ⬇️
...tend/src/views/CRUD/data/database/DatabaseList.tsx 80.37% <25.00%> (-0.76%) ⬇️
...c/views/CRUD/data/database/DatabaseModal/index.tsx 56.77% <57.14%> (+0.17%) ⬆️
.../database/DatabaseModal/DatabaseConnectionForm.tsx 64.28% <64.28%> (ø)
superset-frontend/src/views/CRUD/hooks.ts 46.97% <100.00%> (+1.78%) ⬆️
superset-frontend/src/views/store.ts 100.00% <0.00%> (ø)
superset-frontend/src/dashboard/actions/hydrate.js 1.75% <0.00%> (ø)

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 6d786d4...6e508bd. Read the comment docs.

@eschutho
Copy link
Member Author

eschutho commented May 5, 2021

This pr is dependent on #14470

@eschutho eschutho force-pushed the elizabeth/postgres-connection-form branch from cf2107d to 7adc550 Compare May 6, 2021 01:30
@eschutho
Copy link
Member Author

Rerunning tests.

@eschutho eschutho force-pushed the elizabeth/postgres-connection-form branch from 7adc550 to 6e508bd Compare May 12, 2021 15:25
id={field}
bsSize="sm"
autoComplete="off"
onChange={() => {}}
Copy link
Member Author

Choose a reason for hiding this comment

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

the functionality is set up in the next PR that has the save db: #14583

@eschutho
Copy link
Member Author

A significant enough portion of this PR has changed now that would warrant me closing it and leaving just the save PR for review.

@eschutho eschutho closed this May 13, 2021
@eschutho eschutho deleted the elizabeth/postgres-connection-form branch May 24, 2021 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants