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.

Sign up for GitHub

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

Jump to bottom

fix(import): Add the error alert on failed database import #19673

Merged
merged 3 commits into from
Apr 18, 2022
Merged

fix(import): Add the error alert on failed database import #19673

merged 3 commits into from
Apr 18, 2022

Conversation

prosdev0107
Copy link
Contributor

@prosdev0107 prosdev0107 commented Apr 12, 2022 •
edited

SUMMARY

Add error message when user tries to import without having db driver installed

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:
Not Error Alert Message

AFTER:
image

TESTING INSTRUCTIONS

How to reproduce issues
Repro steps:

  1. Import a file (dashboard, chart, dataset, database) that contains a database where the user doesn't have the database driver installed

Current behavior: Import hangs
Expected behavior: User sees an error message when they don't have the db driver installed

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 Apr 12, 2022 •
edited

Codecov Report

Merging #19673 (738e932) into master (59dda1f) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master   #19673      +/-   ##
==========================================
- Coverage   66.46%   66.46%   -0.01%     
==========================================
  Files        1681     1684       +3     
  Lines       64449    64506      +57     
  Branches     6607     6632      +25     
==========================================
+ Hits        42838    42874      +36     
- Misses      19917    19932      +15     
- Partials     1694     1700       +6
Flag Coverage Δ
javascript 51.14% <50.00%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
...frontend/src/components/ImportModal/ErrorAlert.tsx 40.00% <40.00%> (ø)
...rset-frontend/src/components/ImportModal/index.tsx 73.17% <50.00%> (+0.67%) ⬆️
...rset-frontend/src/components/ImportModal/styles.ts 100.00% <100.00%> (ø)
...d/src/SqlLab/components/SqlEditorLeftBar/index.tsx 44.89% <0.00%> (-11.86%) ⬇️
...rset-frontend/src/dashboard/util/findPermission.ts 92.30% <0.00%> (-7.70%) ⬇️
...perset-frontend/src/views/components/MenuRight.tsx 60.63% <0.00%> (-7.18%) ⬇️
superset-frontend/src/components/Icons/Icon.tsx 95.23% <0.00%> (-4.77%) ⬇️
...end/src/dashboard/components/SliceHeader/index.tsx 87.23% <0.00%> (-2.51%) ⬇️
superset-frontend/src/components/Chart/Chart.jsx 50.00% <0.00%> (-2.46%) ⬇️
...frontend/src/SqlLab/components/ResultSet/index.tsx 50.00% <0.00%> (-0.50%) ⬇️
... and 20 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 59dda1f...738e932. Read the comment docs.

diegomedina248
diegomedina248 reviewed Apr 13, 2022
View reviewed changes
superset-frontend/src/components/ImportModal/index.tsx Outdated
@@ -142,7 +154,7 @@ const ImportModelsModal: FunctionComponent<ImportModelsModalProps> = ({

const handleErrorMsg = (msg: string) => {
clearModal();
addDangerToast(msg);
setErrMsg(msg);
Copy link
Contributor

Choose a reason for hiding this comment

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

we should clear it when attempting a new upload right?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, as far as I can see, the modal now is not closed, so if we can make a new upload with the same modal, we should clear previous error there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay agree.

superset-frontend/src/components/ImportModal/index.tsx Outdated Show resolved Hide resolved
superset-frontend/src/components/ImportModal/index.tsx Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/L and removed size/M labels Apr 14, 2022
@rusackas rusackas changed the title fix(import): make to add the error alert fix(import): Add the error alert on failed database import Apr 18, 2022
@rusackas rusackas merged commit a2d34ec into apache:master Apr 18, 2022
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
@prosdev0107 @philipher29
fix(import): Add the error alert on failed database import (apache#19673
4d951f3 
)

* fix(import): make to add the error alert

* fix(import): make to add licence

* fix(import): make to create ErrorAlert component and use errorMessage spelling
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rusackas rusackas rusackas approved these changes

@diegomedina248 diegomedina248 diegomedina248 approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels Preset-Patch size/L 🚢 2.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@prosdev0107 @rusackas @diegomedina248 @mistercrunch