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

@@ -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)"
+			msg = "Initial save (created by migration)"
-			continue
 		}
 
 		if version.RestoredFrom > 0 {
-			version.Message = fmt.Sprintf("Restored from version %d", version.RestoredFrom)
+			msg = fmt.Sprintf("Restored from version %d", version.RestoredFrom)
-			continue
 		}
 
 		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

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()
 

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"`

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"
         }
       }
     },

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": {