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

PublicDashboards: Paused or deleted public dashboard screen #63970

Merged
merged 11 commits into from Mar 3, 2023

Conversation

juanicabanas
Copy link
Contributor

@juanicabanas juanicabanas commented Mar 1, 2023

What is this feature?

When an admin user pauses or deletes a public dashboard, then the viewer shows a new screen with the proper message.

Which issue(s) does this PR fix?:

Fixes #63953

Special notes for your reviewer:

new-pubdash-not-available.mov

@juanicabanas juanicabanas added this to the 9.5.0 milestone Mar 1, 2023
@juanicabanas juanicabanas self-assigned this Mar 1, 2023
@juanicabanas juanicabanas requested review from a team as code owners March 1, 2023 17:56
@juanicabanas juanicabanas requested review from dprokop, kaydelaney, joshhunt and yaelleC and removed request for a team March 1, 2023 17:56
@owensmallwood
Copy link
Contributor

might be helpful to keep this PR and the enterprise one named the same

@owensmallwood
Copy link
Contributor

The only thing I noticed while testing it was if I use the light theme on my Grafana instance, the paused page still uses the dark theme. Not sure if this matters. I think the request access page does the same thing.

Copy link
Contributor

@owensmallwood owensmallwood left a comment

Choose a reason for hiding this comment

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

Page looks good 👍

@@ -20,4 +20,6 @@ var (
ErrInvalidMaxDataPoints = errutil.NewBase(errutil.StatusBadRequest, "publicdashboards.maxDataPoints", errutil.WithPublicMessage("maxDataPoints should be greater than 0"))
ErrInvalidTimeRange = errutil.NewBase(errutil.StatusBadRequest, "publicdashboards.invalidTimeRange", errutil.WithPublicMessage("Invalid time range"))
ErrInvalidShareType = errutil.NewBase(errutil.StatusBadRequest, "publicdashboards.invalidShareType", errutil.WithPublicMessage("Invalid share type"))

ErrPublicDashboardPaused = errutil.NewBase(errutil.StatusForbidden, "publicdashboards.paused", errutil.WithPublicMessage("Public dashboard paused"))
Copy link
Contributor

Choose a reason for hiding this comment

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

With this change we are introducing a new concept 'paused' which is a friendly way to tell the user that a dashboard is disabled. I think we should not use this concept in the backend. I prefer to continue using 'disabled' or 'notEnabled' for consistency and let the frontend to choose how to communicate this state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. gonna make those changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, we still need to set the "Public dashboard paused" message, since the idea to that message is to be shown directly in the tooltip component, without any translation

@juanicabanas
Copy link
Contributor Author

The only thing I noticed while testing it was if I use the light theme on my Grafana instance, the paused page still uses the dark theme. Not sure if this matters. I think the request access page does the same thing.

@owensmallwood that's a good point. I'm checking and some attributes are null when pubdash is paused/deleted. Gonna make some research

@juanicabanas
Copy link
Contributor Author

The only thing I noticed while testing it was if I use the light theme on my Grafana instance, the paused page still uses the dark theme. Not sure if this matters. I think the request access page does the same thing.

@owensmallwood that's a good point. I'm checking and some attributes are null when pubdash is paused/deleted. Gonna make some research

After some research:

  • The theme is set based on the user preferences. Those preferences are queried based on the userId and the orgId. The problem here is the orgId
  • For pubdash, we have a GetOrgIdByAccessToken method, which gets the orgId for the respective public dashboard as long as it exists and is enabled. So, we can remove the is enabled restriction (which I would like to know why we have it). @evictorero any idea?
  • However, when the public dashboard is deleted, it's impossible to get the orgId, since there's no record anymore. Therefore, for the "deleted public dashboard" page, the orgId won't be returned and the theme will be the dark one.

@juanicabanas
Copy link
Contributor Author

The only thing I noticed while testing it was if I use the light theme on my Grafana instance, the paused page still uses the dark theme. Not sure if this matters. I think the request access page does the same thing.

@owensmallwood that's a good point. I'm checking and some attributes are null when pubdash is paused/deleted. Gonna make some research

After some research:

  • The theme is set based on the user preferences. Those preferences are queried based on the userId and the orgId. The problem here is the orgId
  • For pubdash, we have a GetOrgIdByAccessToken method, which gets the orgId for the respective public dashboard as long as it exists and is enabled. So, we can remove the is enabled restriction (which I would like to know why we have it). @evictorero any idea?
  • However, when the public dashboard is deleted, it's impossible to get the orgId, since there's no record anymore. Therefore, for the "deleted public dashboard" page, the orgId won't be returned and the theme will be the dark one.

Some updates here:

  • We have org theme and user theme configuration. If the pubdash is paused, we're not returning the orgId. Therefore, it takes de .ini theme config. Modifying the query I mentioned before, it lets the paused pubdash be rendered with the user theme. If there's no user, it takes the org theme, which is a good behavior.

Gonna make those changes @evictorero @owensmallwood

@juanicabanas juanicabanas merged commit c59682f into main Mar 3, 2023
@juanicabanas juanicabanas deleted the juanicabanas/publicDashboardNotAvailable branch March 3, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Task: Make paused and deleted public dashboard screens
4 participants