From 5c07df9f4b52b399eb2d8592663af96d69998a60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joan=20L=C3=B3pez=20de=20la=20Franca=20Beltran?= Date: Fri, 19 Mar 2021 09:14:14 +0100 Subject: [PATCH] Chore: Replace Command dispatches by explicit calls (#32131) * Clean up GetProvisionedDashboardDataByIdQuery * Clean up SaveProvisionedDashboardCommand * Clean up & fix AddTeamMemberCommand usages * Clean up & fix UpdateUserPermissionsCommand usages * Lint imports --- pkg/api/admin_users.go | 18 +++++---- pkg/api/admin_users_test.go | 18 +++++++-- pkg/api/api.go | 2 +- pkg/api/team.go | 8 ---- pkg/api/team_members.go | 12 +++++- pkg/models/dashboards.go | 12 ------ pkg/models/user.go | 5 --- .../dashboards/dashboard_service_test.go | 38 ------------------- .../dashboards/folder_service_test.go | 10 ----- 9 files changed, 38 insertions(+), 85 deletions(-) diff --git a/pkg/api/admin_users.go b/pkg/api/admin_users.go index 01c7f7042f6..b4b87b06b06 100644 --- a/pkg/api/admin_users.go +++ b/pkg/api/admin_users.go @@ -9,6 +9,7 @@ import ( "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/infra/metrics" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/util" ) @@ -86,15 +87,11 @@ func AdminUpdateUserPassword(c *models.ReqContext, form dtos.AdminUpdateUserPass } // PUT /api/admin/users/:id/permissions -func AdminUpdateUserPermissions(c *models.ReqContext, form dtos.AdminUpdateUserPermissionsForm) response.Response { +func (hs *HTTPServer) AdminUpdateUserPermissions(c *models.ReqContext, form dtos.AdminUpdateUserPermissionsForm) response.Response { userID := c.ParamsInt64(":id") - cmd := models.UpdateUserPermissionsCommand{ - UserId: userID, - IsGrafanaAdmin: form.IsGrafanaAdmin, - } - - if err := bus.Dispatch(&cmd); err != nil { + err := updateUserPermissions(hs.SQLStore, userID, form.IsGrafanaAdmin) + if err != nil { if errors.Is(err, models.ErrLastGrafanaAdmin) { return response.Error(400, models.ErrLastGrafanaAdmin.Error(), nil) } @@ -189,3 +186,10 @@ func (hs *HTTPServer) AdminRevokeUserAuthToken(c *models.ReqContext, cmd models. userID := c.ParamsInt64(":id") return hs.revokeUserAuthTokenInternal(c, userID, cmd) } + +// updateUserPermissions updates the user's permissions. +// +// Stubbable by tests. +var updateUserPermissions = func(sqlStore *sqlstore.SQLStore, userID int64, isAdmin bool) error { + return sqlStore.UpdateUserPermissions(userID, isAdmin) +} diff --git a/pkg/api/admin_users_test.go b/pkg/api/admin_users_test.go index 2822d8ab272..75ef59a3bd7 100644 --- a/pkg/api/admin_users_test.go +++ b/pkg/api/admin_users_test.go @@ -13,6 +13,8 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/login" + "github.com/grafana/grafana/pkg/services/sqlstore" + "github.com/grafana/grafana/pkg/setting" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -34,10 +36,16 @@ func TestAdminAPIEndpoint(t *testing.T) { putAdminScenario(t, "When calling PUT on", "/api/admin/users/1/permissions", "/api/admin/users/:id/permissions", role, updateCmd, func(sc *scenarioContext) { - bus.AddHandler("test", func(cmd *models.UpdateUserPermissionsCommand) error { - return models.ErrLastGrafanaAdmin + // TODO: Use a fake SQLStore when it's represented by an interface + origUpdateUserPermissions := updateUserPermissions + t.Cleanup(func() { + updateUserPermissions = origUpdateUserPermissions }) + updateUserPermissions = func(sqlStore *sqlstore.SQLStore, userID int64, isAdmin bool) error { + return models.ErrLastGrafanaAdmin + } + sc.fakeReqWithParams("PUT", sc.url, map[string]string{}).exec() assert.Equal(t, 400, sc.resp.Code) }) @@ -300,6 +308,10 @@ func putAdminScenario(t *testing.T, desc string, url string, routePattern string t.Run(fmt.Sprintf("%s %s", desc, url), func(t *testing.T) { t.Cleanup(bus.ClearBusHandlers) + hs := &HTTPServer{ + Cfg: setting.NewCfg(), + } + sc := setupScenarioContext(t, url) sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response { sc.context = c @@ -307,7 +319,7 @@ func putAdminScenario(t *testing.T, desc string, url string, routePattern string sc.context.OrgId = testOrgID sc.context.OrgRole = role - return AdminUpdateUserPermissions(c, cmd) + return hs.AdminUpdateUserPermissions(c, cmd) }) sc.m.Put(routePattern, sc.defaultHandler) diff --git a/pkg/api/api.go b/pkg/api/api.go index b97b814241c..bb463db97ff 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -406,7 +406,7 @@ func (hs *HTTPServer) registerRoutes() { adminRoute.Get("/settings", routing.Wrap(AdminGetSettings)) adminRoute.Post("/users", bind(dtos.AdminCreateUserForm{}), routing.Wrap(hs.AdminCreateUser)) adminRoute.Put("/users/:id/password", bind(dtos.AdminUpdateUserPasswordForm{}), routing.Wrap(AdminUpdateUserPassword)) - adminRoute.Put("/users/:id/permissions", bind(dtos.AdminUpdateUserPermissionsForm{}), routing.Wrap(AdminUpdateUserPermissions)) + adminRoute.Put("/users/:id/permissions", bind(dtos.AdminUpdateUserPermissionsForm{}), routing.Wrap(hs.AdminUpdateUserPermissions)) adminRoute.Delete("/users/:id", routing.Wrap(AdminDeleteUser)) adminRoute.Post("/users/:id/disable", routing.Wrap(hs.AdminDisableUser)) adminRoute.Post("/users/:id/enable", routing.Wrap(AdminEnableUser)) diff --git a/pkg/api/team.go b/pkg/api/team.go index 258a84797c3..f452c6f94bc 100644 --- a/pkg/api/team.go +++ b/pkg/api/team.go @@ -176,11 +176,3 @@ func (hs *HTTPServer) UpdateTeamPreferences(c *models.ReqContext, dtoCmd dtos.Up var createTeam = func(sqlStore *sqlstore.SQLStore, name, email string, orgID int64) (models.Team, error) { return sqlStore.CreateTeam(name, email, orgID) } - -// addTeamMember adds a team member. -// -// Stubbable by tests. -var addTeamMember = func(sqlStore *sqlstore.SQLStore, userID, orgID, teamID int64, isExternal bool, - permission models.PermissionType) error { - return sqlStore.AddTeamMember(userID, orgID, teamID, isExternal, permission) -} diff --git a/pkg/api/team_members.go b/pkg/api/team_members.go index 64bad505c93..5c84a73342e 100644 --- a/pkg/api/team_members.go +++ b/pkg/api/team_members.go @@ -7,6 +7,7 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/teamguardian" "github.com/grafana/grafana/pkg/util" ) @@ -48,7 +49,8 @@ func (hs *HTTPServer) AddTeamMember(c *models.ReqContext, cmd models.AddTeamMemb return response.Error(403, "Not allowed to add team member", err) } - if err := hs.Bus.Dispatch(&cmd); err != nil { + err := addTeamMember(hs.SQLStore, cmd.UserId, cmd.OrgId, cmd.TeamId, cmd.External, cmd.Permission) + if err != nil { if errors.Is(err, models.ErrTeamNotFound) { return response.Error(404, "Team not found", nil) } @@ -119,3 +121,11 @@ func (hs *HTTPServer) RemoveTeamMember(c *models.ReqContext) response.Response { } return response.Success("Team Member removed") } + +// addTeamMember adds a team member. +// +// Stubbable by tests. +var addTeamMember = func(sqlStore *sqlstore.SQLStore, userID, orgID, teamID int64, isExternal bool, + permission models.PermissionType) error { + return sqlStore.AddTeamMember(userID, orgID, teamID, isExternal, permission) +} diff --git a/pkg/models/dashboards.go b/pkg/models/dashboards.go index c3281897743..aac35cf6a03 100644 --- a/pkg/models/dashboards.go +++ b/pkg/models/dashboards.go @@ -355,13 +355,6 @@ type DashboardProvisioning struct { Updated int64 } -type SaveProvisionedDashboardCommand struct { - DashboardCmd *SaveDashboardCommand - DashboardProvisioning *DashboardProvisioning - - Result *Dashboard -} - type DeleteDashboardCommand struct { Id int64 OrgId int64 @@ -418,11 +411,6 @@ type GetDashboardSlugByIdQuery struct { Result string } -type GetProvisionedDashboardDataByIdQuery struct { - DashboardId int64 - Result *DashboardProvisioning -} - type GetDashboardsBySlugQuery struct { OrgId int64 Slug string diff --git a/pkg/models/user.go b/pkg/models/user.go index 2a549a44813..65cdb668c99 100644 --- a/pkg/models/user.go +++ b/pkg/models/user.go @@ -87,11 +87,6 @@ type ChangeUserPasswordCommand struct { UserId int64 `json:"-"` } -type UpdateUserPermissionsCommand struct { - IsGrafanaAdmin bool - UserId int64 `json:"-"` -} - type DisableUserCommand struct { UserId int64 IsDisabled bool diff --git a/pkg/services/dashboards/dashboard_service_test.go b/pkg/services/dashboards/dashboard_service_test.go index 033d3d32b3c..3ad6115b9f9 100644 --- a/pkg/services/dashboards/dashboard_service_test.go +++ b/pkg/services/dashboards/dashboard_service_test.go @@ -107,13 +107,6 @@ func TestDashboardService(t *testing.T) { }) Convey("Should not return validation error if dashboard is provisioned but UI updates allowed", func() { - provisioningValidated := false - bus.AddHandler("test", func(cmd *models.GetProvisionedDashboardDataByIdQuery) error { - provisioningValidated = true - cmd.Result = &models.DashboardProvisioning{} - return nil - }) - origValidateAlerts := validateAlerts t.Cleanup(func() { validateAlerts = origValidateAlerts @@ -126,16 +119,10 @@ func TestDashboardService(t *testing.T) { dto.Dashboard.SetId(3) dto.User = &models.SignedInUser{UserId: 1} _, err := service.SaveDashboard(dto, true) - So(provisioningValidated, ShouldBeFalse) So(err, ShouldBeNil) }) Convey("Should return validation error if alert data is invalid", func() { - bus.AddHandler("test", func(cmd *models.GetProvisionedDashboardDataByIdQuery) error { - cmd.Result = nil - return nil - }) - origValidateAlerts := validateAlerts t.Cleanup(func() { validateAlerts = origValidateAlerts @@ -154,13 +141,6 @@ func TestDashboardService(t *testing.T) { dto := &SaveDashboardDTO{} Convey("Should not return validation error if dashboard is provisioned", func() { - provisioningValidated := false - bus.AddHandler("test", func(cmd *models.GetProvisionedDashboardDataByIdQuery) error { - provisioningValidated = true - cmd.Result = &models.DashboardProvisioning{} - return nil - }) - origUpdateAlerting := UpdateAlerting t.Cleanup(func() { UpdateAlerting = origUpdateAlerting @@ -178,16 +158,11 @@ func TestDashboardService(t *testing.T) { return nil } - bus.AddHandler("test", func(cmd *models.SaveProvisionedDashboardCommand) error { - return nil - }) - dto.Dashboard = models.NewDashboard("Dash") dto.Dashboard.SetId(3) dto.User = &models.SignedInUser{UserId: 1} _, err := service.SaveProvisionedDashboard(dto, nil) So(err, ShouldBeNil) - So(provisioningValidated, ShouldBeFalse) }) Convey("Should override invalid refresh interval if dashboard is provisioned", func() { @@ -195,11 +170,6 @@ func TestDashboardService(t *testing.T) { setting.MinRefreshInterval = "5m" defer func() { setting.MinRefreshInterval = oldRefreshInterval }() - bus.AddHandler("test", func(cmd *models.GetProvisionedDashboardDataByIdQuery) error { - cmd.Result = &models.DashboardProvisioning{} - return nil - }) - origValidateAlerts := validateAlerts t.Cleanup(func() { validateAlerts = origValidateAlerts @@ -208,10 +178,6 @@ func TestDashboardService(t *testing.T) { return nil } - bus.AddHandler("test", func(cmd *models.SaveProvisionedDashboardCommand) error { - return nil - }) - origUpdateAlerting := UpdateAlerting t.Cleanup(func() { UpdateAlerting = origUpdateAlerting @@ -248,10 +214,6 @@ func TestDashboardService(t *testing.T) { return nil } - bus.AddHandler("test", func(cmd *models.SaveProvisionedDashboardCommand) error { - return nil - }) - origUpdateAlerting := UpdateAlerting t.Cleanup(func() { UpdateAlerting = origUpdateAlerting diff --git a/pkg/services/dashboards/folder_service_test.go b/pkg/services/dashboards/folder_service_test.go index 6eb414a352f..9853a2665b0 100644 --- a/pkg/services/dashboards/folder_service_test.go +++ b/pkg/services/dashboards/folder_service_test.go @@ -102,18 +102,9 @@ func TestFolderService(t *testing.T) { return nil }) - provisioningValidated := false - - bus.AddHandler("test", func(query *models.GetProvisionedDashboardDataByIdQuery) error { - provisioningValidated = true - query.Result = nil - return nil - }) - Convey("When creating folder should not return access denied error", func() { _, err := service.CreateFolder("Folder", "") So(err, ShouldBeNil) - So(provisioningValidated, ShouldBeFalse) }) Convey("When updating folder should not return access denied error", func() { @@ -122,7 +113,6 @@ func TestFolderService(t *testing.T) { Title: "Folder", }) So(err, ShouldBeNil) - So(provisioningValidated, ShouldBeFalse) }) Convey("When deleting folder by uid should not return access denied error", func() {