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

Avoid fetch fav dashboard stat not logged in #8527

Merged

Conversation

aspedrosa
Copy link
Contributor

CATEGORY

Choose one

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

SUMMARY

As explained in #8501, if you allow non logged in users to see dashboards, the favorite status of that dashboard will still be fetched, causing an error toast to appear.
To fix it I check if the userId present on the dashboardInfo prop of the Header component is valid, and only if it is, the span related to the fav-star is rendered.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

fav-start-showing
fav-start-not-showing

TEST PLAN

There's already a test that verifies if the fav-start is displayed and works correctly

ADDITIONAL INFORMATION

REVIEWERS

@aspedrosa aspedrosa changed the title Avoid fetch fav dashboard stat not logged in [WIP] Avoid fetch fav dashboard stat not logged in Nov 7, 2019
@aspedrosa aspedrosa changed the title [WIP] Avoid fetch fav dashboard stat not logged in Avoid fetch fav dashboard stat not logged in Nov 7, 2019
@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 7, 2019
@codecov-io
Copy link

codecov-io commented Nov 9, 2019

Codecov Report

Merging #8527 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8527      +/-   ##
==========================================
- Coverage   66.68%   66.67%   -0.01%     
==========================================
  Files         449      449              
  Lines       22687    22698      +11     
  Branches     2366     2368       +2     
==========================================
+ Hits        15128    15135       +7     
- Misses       7421     7425       +4     
  Partials      138      138
Impacted Files Coverage Δ
...uperset/assets/src/dashboard/components/Header.jsx 43.24% <100%> (+1.04%) ⬆️
superset/db_engine_specs/postgres.py 55.55% <0%> (-27.78%) ⬇️
superset/db_engine_specs/base.py 80.33% <0%> (-0.26%) ⬇️
superset/views/database/forms.py 91.11% <0%> (ø) ⬆️
superset/assets/src/setup/setupColors.js 0% <0%> (ø) ⬆️
superset/db_engine_specs/hive.py 28.18% <0%> (+0.32%) ⬆️
superset/views/database/views.py 92.15% <0%> (+8.64%) ⬆️

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 ec476fc...68390e7. Read the comment docs.

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, thanks for adding tests too!

Copy link
Member

@nytai nytai left a comment

Choose a reason for hiding this comment

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

👍 🎉

@stale
Copy link

stale bot commented Jan 26, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Jan 26, 2020
@willbarrett
Copy link
Member

nudge to @dpgaspar, @mistercrunch, and @craig-rueda that this appears ready for merge.

@stale stale bot removed the inactive Inactive for >= 30 days label Jan 29, 2020
@craig-rueda craig-rueda merged commit 5f499b9 into apache:master Jan 29, 2020
@aspedrosa aspedrosa deleted the fix-fetching-fav-dashs-on-log-out branch July 8, 2021 11:03
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 19, 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/M 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants