From 3cd3bb00ec0d356dfcc1a8f695a53d1935543e67 Mon Sep 17 00:00:00 2001 From: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> Date: Thu, 30 Mar 2023 17:31:53 +0300 Subject: [PATCH] API: Fix "Updated by" Column in dashboard versions table (#65351) * API: Fix dashboard versions created by field * Add tests * Update OpenAPI specs * Apply suggestion from code review --- pkg/api/dashboard.go | 43 ++++++-- pkg/api/dashboard_test.go | 136 ++++++++++++++++++++++++- pkg/services/dashboardversion/model.go | 1 - public/api-merged.json | 44 +------- public/openapi3.json | 44 +------- 5 files changed, 172 insertions(+), 96 deletions(-) diff --git a/pkg/api/dashboard.go b/pkg/api/dashboard.go index e10aeeaceb1..b019c18fca6 100644 --- a/pkg/api/dashboard.go +++ b/pkg/api/dashboard.go @@ -660,25 +660,52 @@ func (hs *HTTPServer) GetDashboardVersions(c *contextmodel.ReqContext) response. Start: c.QueryInt("start"), } - res, err := hs.dashboardVersionService.List(c.Req.Context(), &query) + versions, err := hs.dashboardVersionService.List(c.Req.Context(), &query) if err != nil { return response.Error(404, fmt.Sprintf("No versions found for dashboardId %d", dash.ID), err) } - for _, version := range res { + loginMem := make(map[int64]string, len(versions)) + res := make([]dashver.DashboardVersionMeta, 0, len(versions)) + for _, version := range versions { + msg := version.Message if version.RestoredFrom == version.Version { - version.Message = "Initial save (created by migration)" - continue + msg = "Initial save (created by migration)" } if version.RestoredFrom > 0 { - version.Message = fmt.Sprintf("Restored from version %d", version.RestoredFrom) - continue + msg = fmt.Sprintf("Restored from version %d", version.RestoredFrom) } if version.ParentVersion == 0 { - version.Message = "Initial save" + msg = "Initial save" } + + creator := anonString + if version.CreatedBy > 0 { + login, found := loginMem[version.CreatedBy] + if found { + creator = login + } else { + creator = hs.getUserLogin(c.Req.Context(), version.CreatedBy) + if creator != anonString { + loginMem[version.CreatedBy] = creator + } + } + } + + res = append(res, dashver.DashboardVersionMeta{ + ID: version.ID, + DashboardID: version.DashboardID, + DashboardUID: dash.UID, + Data: version.Data, + ParentVersion: version.ParentVersion, + RestoredFrom: version.RestoredFrom, + Version: version.Version, + Created: version.Created, + Message: msg, + CreatedBy: creator, + }) } return response.JSON(http.StatusOK, res) @@ -1267,7 +1294,7 @@ type GetHomeDashboardResponseBody struct { // swagger:response dashboardVersionsResponse type DashboardVersionsResponse struct { // in: body - Body []*dashver.DashboardVersionDTO `json:"body"` + Body []dashver.DashboardVersionMeta `json:"body"` } // swagger:response dashboardVersionResponse diff --git a/pkg/api/dashboard_test.go b/pkg/api/dashboard_test.go index a5931e67793..bd84bb0c8d7 100644 --- a/pkg/api/dashboard_test.go +++ b/pkg/api/dashboard_test.go @@ -8,6 +8,7 @@ import ( "net/http" "os" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -15,6 +16,7 @@ import ( "github.com/grafana/grafana/pkg/services/publicdashboards" "github.com/grafana/grafana/pkg/services/publicdashboards/api" + "github.com/grafana/grafana/pkg/services/user/usertest" "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" @@ -23,6 +25,8 @@ import ( "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/db/dbtest" + "github.com/grafana/grafana/pkg/infra/localcache" + "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/infra/usagestats" "github.com/grafana/grafana/pkg/plugins" @@ -140,7 +144,7 @@ func TestDashboardAPIEndpoint(t *testing.T) { fakeDash.FolderID = 1 fakeDash.HasACL = false fakeDashboardVersionService := dashvertest.NewDashboardVersionServiceFake() - fakeDashboardVersionService.ExpectedDashboardVersion = &dashver.DashboardVersionDTO{} + fakeDashboardVersionService.ExpectedDashboardVersion = &dashver.DashboardVersionDTO{CreatedBy: 1} teamService := &teamtest.FakeService{} dashboardService := dashboards.NewFakeDashboardService(t) dashboardService.On("GetDashboard", mock.Anything, mock.AnythingOfType("*dashboards.GetDashboardQuery")).Return(fakeDash, nil) @@ -156,6 +160,9 @@ func TestDashboardAPIEndpoint(t *testing.T) { dashboardVersionService: fakeDashboardVersionService, Kinds: corekind.NewBase(nil), QuotaService: quotatest.New(false, nil), + userService: &usertest.FakeUserService{ + ExpectedUser: &user.User{ID: 1, Login: "test-user"}, + }, } setUp := func() { @@ -225,6 +232,10 @@ func TestDashboardAPIEndpoint(t *testing.T) { hs.callGetDashboardVersion(sc) assert.Equal(t, 200, sc.resp.Code) + var version *dashver.DashboardVersionMeta + err := json.NewDecoder(sc.resp.Body).Decode(&version) + require.NoError(t, err) + assert.NotEqual(t, anonString, version.CreatedBy) }, mockSQLStore) loggedInUserScenarioWithRole(t, "When calling GET on", "GET", "/api/dashboards/id/2/versions", @@ -935,6 +946,129 @@ func TestDashboardAPIEndpoint(t *testing.T) { }) } +func TestDashboardVersionsAPIEndpoint(t *testing.T) { + fakeDash := dashboards.NewDashboard("Child dash") + fakeDash.ID = 1 + fakeDash.FolderID = 1 + fakeDash.HasACL = false + + fakeDashboardVersionService := dashvertest.NewDashboardVersionServiceFake() + dashboardService := dashboards.NewFakeDashboardService(t) + dashboardService.On("GetDashboard", mock.Anything, mock.AnythingOfType("*dashboards.GetDashboardQuery")).Return(fakeDash, nil) + + teamService := &teamtest.FakeService{} + + mockSQLStore := dbtest.NewFakeDB() + + cfg := setting.NewCfg() + + getHS := func(userSvc *usertest.FakeUserService) *HTTPServer { + return &HTTPServer{ + Cfg: cfg, + pluginStore: &plugins.FakePluginStore{}, + SQLStore: mockSQLStore, + AccessControl: accesscontrolmock.New(), + Features: featuremgmt.WithFeatures(), + DashboardService: dashboardService, + dashboardVersionService: fakeDashboardVersionService, + Kinds: corekind.NewBase(nil), + QuotaService: quotatest.New(false, nil), + userService: userSvc, + CacheService: localcache.New(5*time.Minute, 10*time.Minute), + log: log.New(), + } + } + + setUp := func() { + viewerRole := org.RoleViewer + editorRole := org.RoleEditor + qResult := []*dashboards.DashboardACLInfoDTO{ + {Role: &viewerRole, Permission: dashboards.PERMISSION_VIEW}, + {Role: &editorRole, Permission: dashboards.PERMISSION_EDIT}, + } + dashboardService.On("GetDashboardACLInfoList", mock.Anything, mock.AnythingOfType("*dashboards.GetDashboardACLInfoListQuery")).Return(qResult, nil) + guardian.InitLegacyGuardian(cfg, mockSQLStore, dashboardService, teamService) + } + + loggedInUserScenarioWithRole(t, "When user exists and calling GET on", "GET", "/api/dashboards/id/2/versions", + "/api/dashboards/id/:dashboardId/versions", org.RoleEditor, func(sc *scenarioContext) { + setUp() + fakeDashboardVersionService.ExpectedListDashboarVersions = []*dashver.DashboardVersionDTO{ + { + Version: 1, + CreatedBy: 1, + }, + { + Version: 2, + CreatedBy: 1, + }, + } + getHS(&usertest.FakeUserService{ + ExpectedUser: &user.User{ID: 1, Login: "test-user"}, + }).callGetDashboardVersions(sc) + + assert.Equal(t, 200, sc.resp.Code) + var versions []dashver.DashboardVersionMeta + err := json.NewDecoder(sc.resp.Body).Decode(&versions) + require.NoError(t, err) + for _, v := range versions { + assert.Equal(t, "test-user", v.CreatedBy) + } + }, mockSQLStore) + + loggedInUserScenarioWithRole(t, "When user does not exist and calling GET on", "GET", "/api/dashboards/id/2/versions", + "/api/dashboards/id/:dashboardId/versions", org.RoleEditor, func(sc *scenarioContext) { + setUp() + fakeDashboardVersionService.ExpectedListDashboarVersions = []*dashver.DashboardVersionDTO{ + { + Version: 1, + CreatedBy: 1, + }, + { + Version: 2, + CreatedBy: 1, + }, + } + getHS(&usertest.FakeUserService{ + ExpectedError: user.ErrUserNotFound, + }).callGetDashboardVersions(sc) + + assert.Equal(t, 200, sc.resp.Code) + var versions []dashver.DashboardVersionMeta + err := json.NewDecoder(sc.resp.Body).Decode(&versions) + require.NoError(t, err) + for _, v := range versions { + assert.Equal(t, anonString, v.CreatedBy) + } + }, mockSQLStore) + + loggedInUserScenarioWithRole(t, "When failing to get user and calling GET on", "GET", "/api/dashboards/id/2/versions", + "/api/dashboards/id/:dashboardId/versions", org.RoleEditor, func(sc *scenarioContext) { + setUp() + fakeDashboardVersionService.ExpectedListDashboarVersions = []*dashver.DashboardVersionDTO{ + { + Version: 1, + CreatedBy: 1, + }, + { + Version: 2, + CreatedBy: 1, + }, + } + getHS(&usertest.FakeUserService{ + ExpectedError: fmt.Errorf("some error"), + }).callGetDashboardVersions(sc) + + assert.Equal(t, 200, sc.resp.Code) + var versions []dashver.DashboardVersionMeta + err := json.NewDecoder(sc.resp.Body).Decode(&versions) + require.NoError(t, err) + for _, v := range versions { + assert.Equal(t, anonString, v.CreatedBy) + } + }, mockSQLStore) +} + func getDashboardShouldReturn200WithConfig(t *testing.T, sc *scenarioContext, provisioningService provisioning.ProvisioningService, dashboardStore dashboards.Store, dashboardService dashboards.DashboardService, folderStore folder.FolderStore) dtos.DashboardFullWithMeta { t.Helper() diff --git a/pkg/services/dashboardversion/model.go b/pkg/services/dashboardversion/model.go index 35cb8facc48..435fc794311 100644 --- a/pkg/services/dashboardversion/model.go +++ b/pkg/services/dashboardversion/model.go @@ -66,7 +66,6 @@ type ListDashboardVersionsQuery struct { Limit int Start int } - type DashboardVersionDTO struct { ID int64 `json:"id"` DashboardID int64 `json:"dashboardId"` diff --git a/public/api-merged.json b/public/api-merged.json index 78f01f1a17c..b2859bf29fc 100644 --- a/public/api-merged.json +++ b/public/api-merged.json @@ -12708,48 +12708,6 @@ } } }, - "DashboardVersionDTO": { - "type": "object", - "properties": { - "created": { - "type": "string", - "format": "date-time" - }, - "createdBy": { - "type": "integer", - "format": "int64" - }, - "dashboardId": { - "type": "integer", - "format": "int64" - }, - "dashboardUid": { - "type": "string" - }, - "data": { - "$ref": "#/definitions/Json" - }, - "id": { - "type": "integer", - "format": "int64" - }, - "message": { - "type": "string" - }, - "parentVersion": { - "type": "integer", - "format": "int64" - }, - "restoredFrom": { - "type": "integer", - "format": "int64" - }, - "version": { - "type": "integer", - "format": "int64" - } - } - }, "DashboardVersionMeta": { "description": "DashboardVersionMeta extends the DashboardVersionDTO with the names\nassociated with the UserIds, overriding the field with the same name from\nthe DashboardVersionDTO model.", "type": "object", @@ -19881,7 +19839,7 @@ "schema": { "type": "array", "items": { - "$ref": "#/definitions/DashboardVersionDTO" + "$ref": "#/definitions/DashboardVersionMeta" } } }, diff --git a/public/openapi3.json b/public/openapi3.json index 2dd00ca9996..02f4b97b5c0 100644 --- a/public/openapi3.json +++ b/public/openapi3.json @@ -332,7 +332,7 @@ "application/json": { "schema": { "items": { - "$ref": "#/components/schemas/DashboardVersionDTO" + "$ref": "#/components/schemas/DashboardVersionMeta" }, "type": "array" } @@ -3833,48 +3833,6 @@ }, "type": "object" }, - "DashboardVersionDTO": { - "properties": { - "created": { - "format": "date-time", - "type": "string" - }, - "createdBy": { - "format": "int64", - "type": "integer" - }, - "dashboardId": { - "format": "int64", - "type": "integer" - }, - "dashboardUid": { - "type": "string" - }, - "data": { - "$ref": "#/components/schemas/Json" - }, - "id": { - "format": "int64", - "type": "integer" - }, - "message": { - "type": "string" - }, - "parentVersion": { - "format": "int64", - "type": "integer" - }, - "restoredFrom": { - "format": "int64", - "type": "integer" - }, - "version": { - "format": "int64", - "type": "integer" - } - }, - "type": "object" - }, "DashboardVersionMeta": { "description": "DashboardVersionMeta extends the DashboardVersionDTO with the names\nassociated with the UserIds, overriding the field with the same name from\nthe DashboardVersionDTO model.", "properties": {