fix: DB connection modal connect bug #21299

AAfghahi · 2022-09-01T18:26:06Z

SUMMARY

This was a bug that occurs because we have a default footer in our modal styles that let us connect when there is no database, which gave users a connection error. This hides the footer in the first stage of the DB modal, which is what it was according to its original design.

I also added some height to the modal so that the first stage doesn't need a scroll, happy to revert that.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-09-01.at.2.56.48.PM.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 · 2022-09-01T18:26:55Z

Codecov Report

Merging #21299 (df47f96) into master (fbe9807) will increase coverage by 0.00%.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##           master   #21299   +/-   ##
=======================================
  Coverage   66.45%   66.45%           
=======================================
  Files        1789     1789           
  Lines       68296    68297    +1     
  Branches     7275     7275           
=======================================
+ Hits        45387    45388    +1     
  Misses      21034    21034           
  Partials     1875     1875

Flag	Coverage Δ
hive	`53.02% <ø> (ø)`
javascript	`52.49% <66.66%> (+<0.01%)`	⬆️
mysql	`80.82% <ø> (ø)`
postgres	`80.88% <ø> (ø)`
presto	`52.92% <ø> (ø)`
python	`81.35% <ø> (ø)`
sqlite	`79.43% <ø> (ø)`
unit	`50.77% <ø> (ø)`

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

Impacted Files	Coverage Δ
...c/views/CRUD/data/database/DatabaseModal/index.tsx	`32.03% <50.00%> (ø)`
...c/views/CRUD/data/database/DatabaseModal/styles.ts	`76.92% <100.00%> (+0.22%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

eric-briscoe · 2022-09-01T18:32:40Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

@@ -1509,6 +1511,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
      show={show}
      title={<h4>{t('Connect a database')}</h4>}
      footer={renderModalFooter()}


@AAfghahi Do you want to be using the modalFooter variable here?

This is referencing line 1511? If so, no I don't think so becuase it comes after a ternary that checks for isEditMode

eric-briscoe · 2022-09-01T18:33:39Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

@@ -1509,6 +1511,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
      show={show}
      title={<h4>{t('Connect a database')}</h4>}
      footer={renderModalFooter()}
+      hideFooter={renderModalFooter() ? false : true}


You can use simpler truthy/falsey statement here with the existing modalFooter variable:
hideFooter={!modalFooter}

eric-briscoe · 2022-09-01T18:44:05Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/styles.ts

@@ -156,7 +156,7 @@ export const antDModalStyles = (theme: SupersetTheme) => css`
  }

  .ant-modal-body {
-    height: ${theme.gridUnit * 180.5}px;
+    height: ${theme.gridUnit * 188}px;


A good practice is to define numbers as a const with a comment providing its purpose, then use the const. Referred to commonly as a "magic number" because there is no context to what the value means. Having a const with a comment will help future developers understand what this number represents, and how changing it will affect the rendering of the component.

I this case you would define a const outside of the style rendering block like:

const DESCRIPTIVE_NAME:number = 188 // Description of what this is for...

Then in the styling block use it like:
height: ${theme.gridUnit * DESCRIPTIVE_NAME}px;

I know you did not define this originally, but this is a simple opportunity to make code more readable for future devs :)

I agree, and I changed it. Thank you!

eric-briscoe

Looks good, fixes issue

eschutho · 2022-09-01T19:46:41Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

@@ -901,7 +901,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
      );
    }

-    return [];
+    return <></>;


just a style nit, but most times that I've seen components render nothing, it would be a return null here.

not a blocker.

ok let me test to make sure that this still renders the expected behavior.

Ok no, that would render the default footer component because of the logic here:
https://github.com/apache/superset/blob/master/superset-frontend/src/components/Modal/Modal.tsx#L256

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

(cherry picked from commit 99a4f05)

sadpandajoe · 2022-09-02T18:06:30Z

🏷️ preset:2022.35

AAfghahi added 3 commits August 29, 2022 17:05

dataset exists error

1961582

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

d4255b4

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

dc65632

pull-request-size bot added the size/S label Sep 1, 2022

eric-briscoe suggested changes Sep 1, 2022

View reviewed changes

AAfghahi force-pushed the arash.afghahi/sc-56572/database-connect-db-page-show-error-if-user branch from b3e76cc to f22e694 Compare September 1, 2022 18:46

eric-briscoe approved these changes Sep 1, 2022

View reviewed changes

eschutho reviewed Sep 1, 2022

View reviewed changes

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx Show resolved Hide resolved

eschutho approved these changes Sep 1, 2022

View reviewed changes

AAfghahi force-pushed the arash.afghahi/sc-56572/database-connect-db-page-show-error-if-user branch from db1b89a to 3a84ad5 Compare September 1, 2022 21:07

AAfghahi added 4 commits September 2, 2022 09:36

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

e74b906

fix connect issue

ad649c5

new design

192f52d

added testing

df47f96

AAfghahi force-pushed the arash.afghahi/sc-56572/database-connect-db-page-show-error-if-user branch from 3a84ad5 to df47f96 Compare September 2, 2022 13:37

AAfghahi merged commit 99a4f05 into master Sep 2, 2022

AAfghahi deleted the arash.afghahi/sc-56572/database-connect-db-page-show-error-if-user branch September 2, 2022 16:14

sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Sep 2, 2022

fix: DB connection modal connect bug (apache#21299)

9af8a50

(cherry picked from commit 99a4f05)

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: DB connection modal connect bug #21299

fix: DB connection modal connect bug #21299

AAfghahi commented Sep 1, 2022 •

edited

Loading

codecov bot commented Sep 1, 2022 •

edited

Loading

eric-briscoe Sep 1, 2022

AAfghahi Sep 1, 2022

eric-briscoe Sep 1, 2022

eric-briscoe Sep 1, 2022

AAfghahi Sep 1, 2022

eric-briscoe left a comment

eschutho Sep 1, 2022

eschutho Sep 1, 2022

AAfghahi Sep 1, 2022

AAfghahi Sep 1, 2022

sadpandajoe commented Sep 2, 2022

fix: DB connection modal connect bug #21299

fix: DB connection modal connect bug #21299

Conversation

AAfghahi commented Sep 1, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Sep 1, 2022 • 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

eric-briscoe 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

sadpandajoe commented Sep 2, 2022

AAfghahi commented Sep 1, 2022 •

edited

Loading

codecov bot commented Sep 1, 2022 •

edited

Loading