Pubdash: Email sharing handle dashboard deleted #64247
Conversation
owensmallwood
requested review from
dprokop,
polibb,
papagian,
zserge and
yangkb09
and removed request for
a team
| } | ||
|
|
||
| // ServiceWrapper these methods have different behavior between OSS and Enterprise. The latter would call the OSS service first | ||
| // | ||
| //go:generate mockery --name ServiceWrapper --structname FakePublicDashboardServiceWrapper --inpackage --filename public_dashboard_service_wrapper_mock.go | ||
| type ServiceWrapper interface { | ||
| FindByDashboardUid(ctx context.Context, orgId int64, dashboardUid string) (*PublicDashboard, error) | ||
| HandlePublicDashboardDeleted(ctx context.Context, pubdash *PublicDashboard) error |
There was a problem hiding this comment.
remember to change this naming as well
| @@ -342,6 +342,32 @@ func (pd *PublicDashboardServiceImpl) Delete(ctx context.Context, orgId int64, u | |||
| return nil | |||
| } | |||
|
|
|||
| func (pd *PublicDashboardServiceImpl) DeleteByDashboard(ctx context.Context, dashboard *dashboards.Dashboard) error { | |||
| if dashboard.IsFolder { | |||
There was a problem hiding this comment.
I think all of this should be in the same transaction. Thoughts? @evictorero
There was a problem hiding this comment.
What would the downside be if we only deleted some of the public dashboards and then returned an error?
There was a problem hiding this comment.
have done an inconsistent deletion. From my point of view, everything should be deleted or if there's an error in any deletion, anything should be deleted and an error should be thrown. Otherwise, we end up in an inconsistent state which we don't know how to manage. It's my thought. We should decide what to do but why not to make it as a transaction if we will throw the error anyways?
There was a problem hiding this comment.
If we are not blocking the dashboard deletion (which makes sense) I would say it's the same, maybe it is even better to delete a few ones since they will be orphans.
I'm wondering what we should do if the dashboard deletion fails after that.
There was a problem hiding this comment.
I think we should only use transactions when we want to avoid a weird state. For example, if we start deleting many pubdashes for a dashboard folder then fail halfway through, we'll return an error and there will still be some pubdashes remaining. I don't think this leaves us in a bad state - we just have less pubdashes than when we started, and we're still returning an error letting the caller know something went wrong. Win-win.
There was a problem hiding this comment.
I think this boils down to our definition of what getting left in a weird state is.
There was a problem hiding this comment.
@evictorero if the dashboard deletion fails, then it might be a confusing state for the user. We'd have to wrap the dashboard deletion itself in a transaction, which I don't think we can do right now. Might need to refactor some stuff in the dashboard api to do that.
There was a problem hiding this comment.
I was thinking of calling the public dashboard deletion after dashboard deletion success. Since we don't have FKs we can delete the father before the child. If there is no dashboard but we have public dashboard orphans it's ok, the other way we could delete a public dashboard but if the dashboard deletion fails some configuration will disappear (pubdash) and that is not consistent.
There was a problem hiding this comment.
currently we cant delete the pubdash after the dashboard bc we need to query the dashboard table when its a folder that got deleted
…PublicDashboard on the wrapper. Regenerates wrapper mock and fixes failing tests.
| var pubdashes []*PublicDashboard | ||
|
|
||
| err := d.sqlStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { | ||
| return sess.SQL("SELECT * from dashboard_public WHERE dashboard_uid IN (SELECT uid FROM dashboard WHERE folder_id = ?)", dashboard.ID).Find(&pubdashes) |
There was a problem hiding this comment.
The dashboard_uid is not enough for dashboard identification, you need to use the org_id as well.
There was a problem hiding this comment.
good call 👍 Will fix
| @@ -43,3 +42,16 @@ func (pd *PublicDashboardServiceWrapperImpl) FindByDashboardUid(ctx context.Cont | |||
|
|
|||
| return pubdash, nil | |||
| } | |||
|
|
|||
| func (pd *PublicDashboardServiceWrapperImpl) DeleteByPublicDashboard(ctx context.Context, pubdash *PublicDashboard) error { | |||
| affectedRows, err := pd.store.Delete(ctx, pubdash.OrgId, pubdash.Uid) | |||
There was a problem hiding this comment.
The orgId shouldn't be necessary to delete a pubdash, only the uid. This code is old I am making this comment just to be aware of it 😅
…shboard database layer, updates tests, regenerates mock
…eted during dashboard deletion
|
LGTM !!! 🤟🏻 |
owensmallwood
deleted the
owensmallwood/pubdash-email-sharing-handle-dashboard-deleted
branch
dashboard service calls pubdash service when dashboard deleted
Pubdash services handles when a dashboard is deleted (also handles case when dashboard is folder).
If enterprise, it will also delete all related email shares, magic links, and sessions for each pubdash that gets deleted.
Enterprise PR: https://github.com/grafana/grafana-enterprise/pull/4757
Fixes https://github.com/grafana/grafana-enterprise/issues/4394