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] Allow dashboard viewer auto refresh dashboard #8014

Merged

Conversation

graceguo-supercat
Copy link

@graceguo-supercat graceguo-supercat commented Aug 8, 2019

CATEGORY

Choose one

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

SUMMARY

before #7248, dashboard viewers and owners can auto refresh dashboard periodically, but this refresh frequency can not be saved into dashboard metadata. In #7248, dashboard owner can change refresh frequency and save it as part of dashboard metadata, so that next time viewer open dashboard, it will auto-refresh per owner's setting. But in this PR, viewers lost their options to change refresh frequency, even they doesn't want to change owner's setting. There are some cases where dashboard owner didn't set auto refresh, but viewers still want to auto refresh during current visit.

This PR added back this feature:

  • dashboard viewers can trigger auto refresh for this visit, but this change won't persist.
  • In edit mode, if dashboard owner changes auto refresh frequency, this change will be saved into dashboard metadata.

AFTER SCREENSHOTS OR ANIMATED GIF

Dashboard viewers will see menu Auto-refresh dashboard:
Screen Shot 2019-08-08 at 12 49 59 PM

In edit mode, dashboard owners will see Set auto-refresh interval:
Screen Shot 2019-08-08 at 12 50 17 PM

dwk0K7Y7YK

TEST PLAN

CI, and manual test.

REVIEWERS

@betodealmeida @khtruong
@michellethomas @etr2460

@kristw
Copy link
Contributor

kristw commented Aug 9, 2019

  • Dashboard viewers will see menu Auto-refresh dashboard:
    Can they click? What does it do?

  • Dashboard owners will see Set auto-refresh interval:
    Does this bring up another dialog or what happen? If brings up another dialog may be add ...
    Should the label be "Enable auto-refresh" instead of "Set auto-refresh interval" if it is just enable or not? The latter made it feel you have the ability to set the interval value.

@graceguo-supercat
Copy link
Author

graceguo-supercat commented Aug 9, 2019

  • Dashboard viewers will see menu Auto-refresh dashboard:
    Can they click? What does it do?

Yes.

  • Dashboard owners will see Set auto-refresh interval:
    Does this bring up another dialog or what happen? If brings up another dialog may be add ...
    Should the label be "Enable auto-refresh" instead of "Set auto-refresh interval" if it is just enable or not? The latter made it feel you have the ability to set the interval value.

Nothing extra to be enabled in edit mode. Edit mode can persist the change. After this fix, auto refreshing dashboard should be enabled for viewers and owners. But only owner has ability to save.

@betodealmeida
Copy link
Member

Thanks for fixing this, @graceguo-supercat!

@graceguo-supercat graceguo-supercat merged commit 613dcf5 into apache:master Aug 13, 2019
graceguo-supercat pushed a commit to graceguo-supercat/superset that referenced this pull request Aug 14, 2019
@graceguo-supercat graceguo-supercat deleted the gg-DashboardAutoRefresh branch September 21, 2019 08:09
@mistercrunch mistercrunch added 🍒 0.34.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.35.0 labels Feb 28, 2024
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 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 v0.34 🍒 0.34.1 🚢 0.35.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants