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

fix bug where error at import dashboard fails to show toast in "welcome" app #9714

Merged
merged 13 commits into from
May 8, 2020

Conversation

pkdotson
Copy link
Member

@pkdotson pkdotson commented May 2, 2020

CATEGORY

Choose one

  • [ x] Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

The toast messages weren't working when there was an error redirect from the import template to the react dashboard component. To fix this common data had to be passed into the components the app level to show toast or not.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

To test out go to import and try to upload a non dashboard. You will be redirected to page to dashboardlist with the correct toast.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

@pkdotson pkdotson changed the title fix bug where error at import dashboard fails fix bug where error at import dashboard fails to show toast in "welcome" app May 2, 2020
Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

some comments, but a higher level question too: it seems like we'd want to show these flash messages (if they exist) on any page. Is there a way we can raise this logic up to the App.jsx level instead so that it applies everywhere instead of only on the DashboardList?

superset-frontend/src/welcome/App.jsx Outdated Show resolved Hide resolved
superset-frontend/src/welcome/App.jsx Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented May 4, 2020

Codecov Report

Merging #9714 into master will decrease coverage by 0.24%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9714      +/-   ##
==========================================
- Coverage   70.78%   70.54%   -0.25%     
==========================================
  Files         586      587       +1     
  Lines       30433    30449      +16     
  Branches     3117     3119       +2     
==========================================
- Hits        21543    21481      -62     
- Misses       8776     8847      +71     
- Partials      114      121       +7     
Flag Coverage Δ
#cypress 52.89% <ø> (-0.75%) ⬇️
#javascript 58.95% <0.00%> (-0.08%) ⬇️
#python 70.93% <ø> (ø)
Impacted Files Coverage Δ
superset-frontend/src/components/FlashProvider.tsx 0.00% <0.00%> (ø)
...frontend/src/views/dashboardList/DashboardList.tsx 63.70% <ø> (ø)
superset-frontend/src/welcome/App.jsx 0.00% <0.00%> (ø)
...et-frontend/src/SqlLab/reducers/getInitialState.js 33.33% <0.00%> (-16.67%) ⬇️
superset-frontend/src/reduxUtils.js 76.47% <0.00%> (-10.30%) ⬇️
...rontend/src/SqlLab/components/TabbedSqlEditors.jsx 75.52% <0.00%> (-6.30%) ⬇️
superset-frontend/src/SqlLab/actions/sqlLab.js 61.13% <0.00%> (-5.68%) ⬇️
...rontend/src/SqlLab/components/SqlEditorLeftBar.jsx 44.00% <0.00%> (-4.00%) ⬇️
superset-frontend/src/SqlLab/reducers/sqlLab.js 37.44% <0.00%> (-3.30%) ⬇️
...end/src/SqlLab/components/TemplateParamsEditor.jsx 88.57% <0.00%> (-2.86%) ⬇️
... and 5 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 b93bf05...7d42c3d. Read the comment docs.

@pkdotson pkdotson force-pushed the phillip/SO-384-flash-messages branch from 02cc383 to fbc4496 Compare May 4, 2020 23:42
@pull-request-size pull-request-size bot added size/L and removed size/S labels May 4, 2020
Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

lgtm with the exception of a couple remaining nits. Thanks for the contribution!

superset-frontend/src/components/FlashProvider.tsx Outdated Show resolved Hide resolved
superset-frontend/src/components/FlashProvider.tsx Outdated Show resolved Hide resolved
superset-frontend/src/components/FlashProvider.tsx Outdated Show resolved Hide resolved
superset-frontend/src/components/FlashProvider.tsx Outdated Show resolved Hide resolved
superset-frontend/src/components/FlashProvider.tsx Outdated Show resolved Hide resolved
Phillip Kelley-Dotson added 2 commits May 7, 2020 10:48
@etr2460 etr2460 merged commit b6df5da into apache:master May 8, 2020
@rusackas rusackas deleted the phillip/SO-384-flash-messages branch May 8, 2020 18:35
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Feb 28, 2024
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 🚢 0.37.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants