From 0f2e87933973327c8077921a3fa6c16a064aab5e Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Wed, 13 Jun 2018 18:01:50 +0200 Subject: [PATCH 1/8] set current org when adding/removing user to org To not get into a situation where a user has a current organization assign which he is not a member of we try to always make sure that a user has a valid current organization assigned. (cherry picked from commit 6d48d0a80c8ce59b9dc782a623dab4e3fcefcbb4) --- pkg/services/sqlstore/org_test.go | 16 ++++- pkg/services/sqlstore/org_users.go | 64 ++++++++++++++++++- pkg/services/sqlstore/user.go | 18 ++++-- pkg/services/sqlstore/user_test.go | 14 ++-- .../features/admin/admin_edit_user_ctrl.ts | 2 + 5 files changed, 96 insertions(+), 18 deletions(-) diff --git a/pkg/services/sqlstore/org_test.go b/pkg/services/sqlstore/org_test.go index 63b20aa6e86..dcf45032198 100644 --- a/pkg/services/sqlstore/org_test.go +++ b/pkg/services/sqlstore/org_test.go @@ -150,7 +150,7 @@ func TestAccountDataAccess(t *testing.T) { }) Convey("Can set using org", func() { - cmd := m.SetUsingOrgCommand{UserId: ac2.Id, OrgId: ac1.Id} + cmd := m.SetUsingOrgCommand{UserId: ac2.Id, OrgId: ac1.OrgId} err := SetUsingOrg(&cmd) So(err, ShouldBeNil) @@ -159,13 +159,25 @@ func TestAccountDataAccess(t *testing.T) { err := GetSignedInUser(&query) So(err, ShouldBeNil) - So(query.Result.OrgId, ShouldEqual, ac1.Id) + So(query.Result.OrgId, ShouldEqual, ac1.OrgId) So(query.Result.Email, ShouldEqual, "ac2@test.com") So(query.Result.Name, ShouldEqual, "ac2 name") So(query.Result.Login, ShouldEqual, "ac2") So(query.Result.OrgName, ShouldEqual, "ac1@test.com") So(query.Result.OrgRole, ShouldEqual, "Viewer") }) + + Convey("Should set last org as current when removing user from current", func() { + remCmd := m.RemoveOrgUserCommand{OrgId: ac1.OrgId, UserId: ac2.Id} + err := RemoveOrgUser(&remCmd) + So(err, ShouldBeNil) + + query := m.GetSignedInUserQuery{UserId: ac2.Id} + err = GetSignedInUser(&query) + + So(err, ShouldBeNil) + So(query.Result.OrgId, ShouldEqual, ac2.OrgId) + }) }) Convey("Cannot delete last admin org user", func() { diff --git a/pkg/services/sqlstore/org_users.go b/pkg/services/sqlstore/org_users.go index 0b991c73c55..aad72cdacb4 100644 --- a/pkg/services/sqlstore/org_users.go +++ b/pkg/services/sqlstore/org_users.go @@ -20,7 +20,14 @@ func init() { func AddOrgUser(cmd *m.AddOrgUserCommand) error { return inTransaction(func(sess *DBSession) error { // check if user exists - if res, err := sess.Query("SELECT 1 from org_user WHERE org_id=? and user_id=?", cmd.OrgId, cmd.UserId); err != nil { + var user m.User + if exists, err := sess.Id(cmd.UserId).Get(&user); err != nil { + return err + } else if !exists { + return m.ErrUserNotFound + } + + if res, err := sess.Query("SELECT 1 from org_user WHERE org_id=? and user_id=?", cmd.OrgId, user.Id); err != nil { return err } else if len(res) == 1 { return m.ErrOrgUserAlreadyAdded @@ -41,7 +48,26 @@ func AddOrgUser(cmd *m.AddOrgUserCommand) error { } _, err := sess.Insert(&entity) - return err + if err != nil { + return err + } + + var userOrgs []*m.UserOrgDTO + sess.Table("org_user") + sess.Join("INNER", "org", "org_user.org_id=org.id") + sess.Where("org_user.user_id=? AND org_user.org_id=?", user.Id, user.OrgId) + sess.Cols("org.name", "org_user.role", "org_user.org_id") + err = sess.Find(&userOrgs) + + if err != nil { + return err + } + + if len(userOrgs) == 0 { + return setUsingOrgInTransaction(sess, user.Id, cmd.OrgId) + } + + return nil }) } @@ -110,6 +136,14 @@ func GetOrgUsers(query *m.GetOrgUsersQuery) error { func RemoveOrgUser(cmd *m.RemoveOrgUserCommand) error { return inTransaction(func(sess *DBSession) error { + // check if user exists + var user m.User + if exists, err := sess.Id(cmd.UserId).Get(&user); err != nil { + return err + } else if !exists { + return m.ErrUserNotFound + } + deletes := []string{ "DELETE FROM org_user WHERE org_id=? and user_id=?", "DELETE FROM dashboard_acl WHERE org_id=? and user_id = ?", @@ -123,6 +157,32 @@ func RemoveOrgUser(cmd *m.RemoveOrgUserCommand) error { } } + var userOrgs []*m.UserOrgDTO + sess.Table("org_user") + sess.Join("INNER", "org", "org_user.org_id=org.id") + sess.Where("org_user.user_id=?", user.Id) + sess.Cols("org.name", "org_user.role", "org_user.org_id") + err := sess.Find(&userOrgs) + + if err != nil { + return err + } + + hasCurrentOrgSet := false + for _, userOrg := range userOrgs { + if user.OrgId == userOrg.OrgId { + hasCurrentOrgSet = true + break + } + } + + if !hasCurrentOrgSet && len(userOrgs) > 0 { + err = setUsingOrgInTransaction(sess, user.Id, userOrgs[0].OrgId) + if err != nil { + return err + } + } + return validateOneAdminLeftInOrg(cmd.OrgId, sess) }) } diff --git a/pkg/services/sqlstore/user.go b/pkg/services/sqlstore/user.go index e7aa8da837a..ad86323c0d8 100644 --- a/pkg/services/sqlstore/user.go +++ b/pkg/services/sqlstore/user.go @@ -290,16 +290,20 @@ func SetUsingOrg(cmd *m.SetUsingOrgCommand) error { } return inTransaction(func(sess *DBSession) error { - user := m.User{ - Id: cmd.UserId, - OrgId: cmd.OrgId, - } - - _, err := sess.Id(cmd.UserId).Update(&user) - return err + return setUsingOrgInTransaction(sess, cmd.UserId, cmd.OrgId) }) } +func setUsingOrgInTransaction(sess *DBSession, userID int64, orgID int64) error { + user := m.User{ + Id: userID, + OrgId: orgID, + } + + _, err := sess.Id(userID).Update(&user) + return err +} + func GetUserProfile(query *m.GetUserProfileQuery) error { var user m.User has, err := x.Id(query.UserId).Get(&user) diff --git a/pkg/services/sqlstore/user_test.go b/pkg/services/sqlstore/user_test.go index 2830733c96a..076e88c2bb3 100644 --- a/pkg/services/sqlstore/user_test.go +++ b/pkg/services/sqlstore/user_test.go @@ -96,33 +96,33 @@ func TestUserDataAccess(t *testing.T) { }) Convey("when a user is an org member and has been assigned permissions", func() { - err = AddOrgUser(&m.AddOrgUserCommand{LoginOrEmail: users[0].Login, Role: m.ROLE_VIEWER, OrgId: users[0].OrgId}) + err = AddOrgUser(&m.AddOrgUserCommand{LoginOrEmail: users[1].Login, Role: m.ROLE_VIEWER, OrgId: users[0].OrgId, UserId: users[1].Id}) So(err, ShouldBeNil) - testHelperUpdateDashboardAcl(1, m.DashboardAcl{DashboardId: 1, OrgId: users[0].OrgId, UserId: users[0].Id, Permission: m.PERMISSION_EDIT}) + testHelperUpdateDashboardAcl(1, m.DashboardAcl{DashboardId: 1, OrgId: users[0].OrgId, UserId: users[1].Id, Permission: m.PERMISSION_EDIT}) So(err, ShouldBeNil) - err = SavePreferences(&m.SavePreferencesCommand{UserId: users[0].Id, OrgId: users[0].OrgId, HomeDashboardId: 1, Theme: "dark"}) + err = SavePreferences(&m.SavePreferencesCommand{UserId: users[1].Id, OrgId: users[0].OrgId, HomeDashboardId: 1, Theme: "dark"}) So(err, ShouldBeNil) Convey("when the user is deleted", func() { - err = DeleteUser(&m.DeleteUserCommand{UserId: users[0].Id}) + err = DeleteUser(&m.DeleteUserCommand{UserId: users[1].Id}) So(err, ShouldBeNil) Convey("Should delete connected org users and permissions", func() { - query := &m.GetOrgUsersQuery{OrgId: 1} + query := &m.GetOrgUsersQuery{OrgId: users[0].OrgId} err = GetOrgUsersForTest(query) So(err, ShouldBeNil) So(len(query.Result), ShouldEqual, 1) - permQuery := &m.GetDashboardAclInfoListQuery{DashboardId: 1, OrgId: 1} + permQuery := &m.GetDashboardAclInfoListQuery{DashboardId: 1, OrgId: users[0].OrgId} err = GetDashboardAclInfoList(permQuery) So(err, ShouldBeNil) So(len(permQuery.Result), ShouldEqual, 0) - prefsQuery := &m.GetPreferencesQuery{OrgId: users[0].OrgId, UserId: users[0].Id} + prefsQuery := &m.GetPreferencesQuery{OrgId: users[0].OrgId, UserId: users[1].Id} err = GetPreferences(prefsQuery) So(err, ShouldBeNil) diff --git a/public/app/features/admin/admin_edit_user_ctrl.ts b/public/app/features/admin/admin_edit_user_ctrl.ts index 8203c7399c1..1d4fb9cf19a 100644 --- a/public/app/features/admin/admin_edit_user_ctrl.ts +++ b/public/app/features/admin/admin_edit_user_ctrl.ts @@ -75,6 +75,7 @@ export class AdminEditUserCtrl { $scope.removeOrgUser = function(orgUser) { backendSrv.delete('/api/orgs/' + orgUser.orgId + '/users/' + $scope.user_id).then(function() { + $scope.getUser($scope.user_id); $scope.getUserOrgs($scope.user_id); }); }; @@ -108,6 +109,7 @@ export class AdminEditUserCtrl { $scope.newOrg.loginOrEmail = $scope.user.login; backendSrv.post('/api/orgs/' + orgInfo.id + '/users/', $scope.newOrg).then(function() { + $scope.getUser($scope.user_id); $scope.getUserOrgs($scope.user_id); }); }; From 8d635efda00a9a907edfea1cec38ad34fad1e333 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Mon, 18 Jun 2018 11:04:16 +0200 Subject: [PATCH 2/8] snapshot: copy correct props when creating a snapshot (cherry picked from commit a7383479574a73cf4c0d87658e36ae0fccf3ac9c) --- public/app/features/dashboard/share_snapshot_ctrl.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/public/app/features/dashboard/share_snapshot_ctrl.ts b/public/app/features/dashboard/share_snapshot_ctrl.ts index aa146dcad63..7d5bd112dfd 100644 --- a/public/app/features/dashboard/share_snapshot_ctrl.ts +++ b/public/app/features/dashboard/share_snapshot_ctrl.ts @@ -123,6 +123,9 @@ export class ShareSnapshotCtrl { enable: annotation.enable, iconColor: annotation.iconColor, snapshotData: annotation.snapshotData, + type: annotation.type, + builtIn: annotation.builtIn, + hide: annotation.hide, }; }) .value(); From f453fbe8efccf36564034a24e7497ec93f68a9b0 Mon Sep 17 00:00:00 2001 From: Martin Molnar Date: Tue, 20 Feb 2018 11:15:31 +0100 Subject: [PATCH 3/8] feat(ldap): Allow use of DN in user attribute filter (#3132) (cherry picked from commit be2fa54459bd6aa9305ec6ee83be8599370f9ffe) --- pkg/login/ldap.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/pkg/login/ldap.go b/pkg/login/ldap.go index 3c5001df3a4..2f25b453a17 100644 --- a/pkg/login/ldap.go +++ b/pkg/login/ldap.go @@ -308,6 +308,10 @@ func (a *ldapAuther) searchForUser(username string) (*LdapUserInfo, error) { } else { filter_replace = getLdapAttr(a.server.GroupSearchFilterUserAttribute, searchResult) } + if a.server.GroupSearchFilterUserAttribute == "dn" { + filter_replace = searchResult.Entries[0].DN + } + filter := strings.Replace(a.server.GroupSearchFilter, "%s", ldap.EscapeFilter(filter_replace), -1) a.log.Info("Searching for user's groups", "filter", filter) @@ -330,7 +334,11 @@ func (a *ldapAuther) searchForUser(username string) (*LdapUserInfo, error) { if len(groupSearchResult.Entries) > 0 { for i := range groupSearchResult.Entries { - memberOf = append(memberOf, getLdapAttrN(a.server.Attr.MemberOf, groupSearchResult, i)) + if a.server.Attr.MemberOf == "dn" { + memberOf = append(memberOf, groupSearchResult.Entries[i].DN) + } else { + memberOf = append(memberOf, getLdapAttrN(a.server.Attr.MemberOf, groupSearchResult, i)) + } } break } From af42e0836a181085ce10a6ff6baf6901ec199e02 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Wed, 20 Jun 2018 00:08:52 +0200 Subject: [PATCH 4/8] fix regressions after save modal changes of not storing time and variables per default Fix problem with adhoc variable filters not handled. Fix problem with saving variables and time per default when saving a dashboard as/first time. Fix updating dashboard model after save with saving time/variables enabled so that next time you save you won't get checkboxes for save time/variables unless any values changed. Tests validating correctness if time/variable values has changed. (cherry picked from commit 41ac8d4cd56a9986b258e0c4600b7761d55c800c) --- .../app/features/dashboard/dashboard_model.ts | 69 +++++-- public/app/features/dashboard/save_modal.ts | 52 ++--- .../dashboard/specs/dashboard_model.jest.ts | 192 +++++++++++++++--- .../dashboard/specs/save_modal.jest.ts | 165 +++++---------- 4 files changed, 286 insertions(+), 192 deletions(-) diff --git a/public/app/features/dashboard/dashboard_model.ts b/public/app/features/dashboard/dashboard_model.ts index a37e753bd89..976e4213920 100644 --- a/public/app/features/dashboard/dashboard_model.ts +++ b/public/app/features/dashboard/dashboard_model.ts @@ -22,10 +22,10 @@ export class DashboardModel { editable: any; graphTooltip: any; time: any; - originalTime: any; + private originalTime: any; timepicker: any; templating: any; - originalTemplating: any; + private originalTemplating: any; annotations: any; refresh: any; snapshot: any; @@ -50,6 +50,8 @@ export class DashboardModel { meta: true, panels: true, // needs special handling templating: true, // needs special handling + originalTime: true, + originalTemplating: true, }; constructor(data, meta?) { @@ -70,12 +72,8 @@ export class DashboardModel { this.editable = data.editable !== false; this.graphTooltip = data.graphTooltip || 0; this.time = data.time || { from: 'now-6h', to: 'now' }; - this.originalTime = _.cloneDeep(this.time); this.timepicker = data.timepicker || {}; this.templating = this.ensureListExist(data.templating); - this.originalTemplating = _.map(this.templating.list, variable => { - return { name: variable.name, current: _.clone(variable.current) }; - }); this.annotations = this.ensureListExist(data.annotations); this.refresh = data.refresh; this.snapshot = data.snapshot; @@ -85,6 +83,9 @@ export class DashboardModel { this.gnetId = data.gnetId || null; this.panels = _.map(data.panels || [], panelData => new PanelModel(panelData)); + this.resetOriginalVariables(); + this.resetOriginalTime(); + this.initMeta(meta); this.updateSchema(data); @@ -138,8 +139,8 @@ export class DashboardModel { // cleans meta data and other non persistent state getSaveModelClone(options?) { let defaults = _.defaults(options || {}, { - saveVariables: false, - saveTimerange: false, + saveVariables: true, + saveTimerange: true, }); // make clone @@ -153,15 +154,23 @@ export class DashboardModel { } // get variable save models - //console.log(this.templating.list); copy.templating = { list: _.map(this.templating.list, variable => (variable.getSaveModel ? variable.getSaveModel() : variable)), }; - if (!defaults.saveVariables && copy.templating.list.length === this.originalTemplating.length) { + if (!defaults.saveVariables) { for (let i = 0; i < copy.templating.list.length; i++) { - if (copy.templating.list[i].name === this.originalTemplating[i].name) { - copy.templating.list[i].current = this.originalTemplating[i].current; + let current = copy.templating.list[i]; + let original = _.find(this.originalTemplating, { name: current.name, type: current.type }); + + if (!original) { + continue; + } + + if (current.type === 'adhoc') { + copy.templating.list[i].filters = original.filters; + } else { + copy.templating.list[i].current = original.current; } } } @@ -785,4 +794,40 @@ export class DashboardModel { let migrator = new DashboardMigrator(this); migrator.updateSchema(old); } + + resetOriginalTime() { + this.originalTime = _.cloneDeep(this.time); + } + + hasTimeChanged() { + return !_.isEqual(this.time, this.originalTime); + } + + resetOriginalVariables() { + this.originalTemplating = _.map(this.templating.list, variable => { + return { + name: variable.name, + type: variable.type, + current: _.cloneDeep(variable.current), + filters: _.cloneDeep(variable.filters), + }; + }); + } + + hasVariableValuesChanged() { + if (this.templating.list.length !== this.originalTemplating.length) { + return false; + } + + const updated = _.map(this.templating.list, variable => { + return { + name: variable.name, + type: variable.type, + current: _.cloneDeep(variable.current), + filters: _.cloneDeep(variable.filters), + }; + }); + + return !_.isEqual(updated, this.originalTemplating); + } } diff --git a/public/app/features/dashboard/save_modal.ts b/public/app/features/dashboard/save_modal.ts index 525d2d1cbc3..3afcbab707c 100644 --- a/public/app/features/dashboard/save_modal.ts +++ b/public/app/features/dashboard/save_modal.ts @@ -1,5 +1,4 @@ import coreModule from 'app/core/core_module'; -import _ from 'lodash'; const template = `
@@ -70,7 +69,6 @@ export class SaveDashboardModalCtrl { message: string; saveVariables = false; saveTimerange = false; - templating: any; time: any; originalTime: any; current = []; @@ -87,40 +85,8 @@ export class SaveDashboardModalCtrl { this.message = ''; this.max = 64; this.isSaving = false; - this.templating = dashboardSrv.dash.templating.list; - - this.compareTemplating(); - this.compareTime(); - } - - compareTime() { - if (_.isEqual(this.dashboardSrv.dash.time, this.dashboardSrv.dash.originalTime)) { - this.timeChange = false; - } else { - this.timeChange = true; - } - } - - compareTemplating() { - //checks if variables has been added or removed, if so variables will be saved automatically - if (this.dashboardSrv.dash.originalTemplating.length !== this.dashboardSrv.dash.templating.list.length) { - return (this.variableValueChange = false); - } - - //checks if variable value has changed - if (this.dashboardSrv.dash.templating.list.length > 0) { - for (let i = 0; i < this.dashboardSrv.dash.templating.list.length; i++) { - if ( - this.dashboardSrv.dash.templating.list[i].current.text !== - this.dashboardSrv.dash.originalTemplating[i].current.text - ) { - return (this.variableValueChange = true); - } - } - return (this.variableValueChange = false); - } else { - return (this.variableValueChange = false); - } + this.timeChange = this.dashboardSrv.getCurrent().hasTimeChanged(); + this.variableValueChange = this.dashboardSrv.getCurrent().hasVariableValuesChanged(); } save() { @@ -139,7 +105,19 @@ export class SaveDashboardModalCtrl { this.isSaving = true; - return this.dashboardSrv.save(saveModel, options).then(this.dismiss); + return this.dashboardSrv.save(saveModel, options).then(this.postSave.bind(this, options)); + } + + postSave(options) { + if (options.saveVariables) { + this.dashboardSrv.getCurrent().resetOriginalVariables(); + } + + if (options.saveTimerange) { + this.dashboardSrv.getCurrent().resetOriginalTime(); + } + + this.dismiss(); } } diff --git a/public/app/features/dashboard/specs/dashboard_model.jest.ts b/public/app/features/dashboard/specs/dashboard_model.jest.ts index 3fb5b3c3b73..6ac642cd58e 100644 --- a/public/app/features/dashboard/specs/dashboard_model.jest.ts +++ b/public/app/features/dashboard/specs/dashboard_model.jest.ts @@ -435,8 +435,67 @@ describe('DashboardModel', function() { }); }); - describe('save variables and timeline', () => { - let model; + describe('Given model with time', () => { + let model: DashboardModel; + + beforeEach(() => { + model = new DashboardModel({ + time: { + from: 'now-6h', + to: 'now', + }, + }); + expect(model.hasTimeChanged()).toBeFalsy(); + model.time = { + from: 'now-3h', + to: 'now-1h', + }; + }); + + it('hasTimeChanged should be true', () => { + expect(model.hasTimeChanged()).toBeTruthy(); + }); + + it('getSaveModelClone should return original time when saveTimerange=false', () => { + let options = { saveTimerange: false }; + let saveModel = model.getSaveModelClone(options); + + expect(saveModel.time.from).toBe('now-6h'); + expect(saveModel.time.to).toBe('now'); + }); + + it('getSaveModelClone should return updated time when saveTimerange=true', () => { + let options = { saveTimerange: true }; + let saveModel = model.getSaveModelClone(options); + + expect(saveModel.time.from).toBe('now-3h'); + expect(saveModel.time.to).toBe('now-1h'); + }); + + it('hasTimeChanged should be false when reset original time', () => { + model.resetOriginalTime(); + expect(model.hasTimeChanged()).toBeFalsy(); + }); + + it('getSaveModelClone should return original time when saveTimerange=false', () => { + let options = { saveTimerange: false }; + let saveModel = model.getSaveModelClone(options); + + expect(saveModel.time.from).toBe('now-6h'); + expect(saveModel.time.to).toBe('now'); + }); + + it('getSaveModelClone should return updated time when saveTimerange=true', () => { + let options = { saveTimerange: true }; + let saveModel = model.getSaveModelClone(options); + + expect(saveModel.time.from).toBe('now-3h'); + expect(saveModel.time.to).toBe('now-1h'); + }); + }); + + describe('Given model with template variable of type query', () => { + let model: DashboardModel; beforeEach(() => { model = new DashboardModel({ @@ -444,6 +503,7 @@ describe('DashboardModel', function() { list: [ { name: 'Server', + type: 'query', current: { selected: true, text: 'server_001', @@ -452,45 +512,127 @@ describe('DashboardModel', function() { }, ], }, - time: { - from: 'now-6h', - to: 'now', - }, }); - model.templating.list[0] = { - name: 'Server', + expect(model.hasVariableValuesChanged()).toBeFalsy(); + }); + + it('hasVariableValuesChanged should be false when adding a template variable', () => { + model.templating.list.push({ + name: 'Server2', + type: 'query', current: { selected: true, text: 'server_002', value: 'server_002', }, - }; - model.time = { - from: 'now-3h', - to: 'now', - }; + }); + expect(model.hasVariableValuesChanged()).toBeFalsy(); }); - it('should not save variables and timeline', () => { - let options = { - saveVariables: false, - saveTimerange: false, - }; + it('hasVariableValuesChanged should be false when removing existing template variable', () => { + model.templating.list = []; + expect(model.hasVariableValuesChanged()).toBeFalsy(); + }); + + it('hasVariableValuesChanged should be true when changing value of template variable', () => { + model.templating.list[0].current.text = 'server_002'; + expect(model.hasVariableValuesChanged()).toBeTruthy(); + }); + + it('getSaveModelClone should return original variable when saveVariables=false', () => { + model.templating.list[0].current.text = 'server_002'; + + let options = { saveVariables: false }; let saveModel = model.getSaveModelClone(options); expect(saveModel.templating.list[0].current.text).toBe('server_001'); - expect(saveModel.time.from).toBe('now-6h'); }); - it('should save variables and timeline', () => { - let options = { - saveVariables: true, - saveTimerange: true, - }; + it('getSaveModelClone should return updated variable when saveVariables=true', () => { + model.templating.list[0].current.text = 'server_002'; + + let options = { saveVariables: true }; let saveModel = model.getSaveModelClone(options); expect(saveModel.templating.list[0].current.text).toBe('server_002'); - expect(saveModel.time.from).toBe('now-3h'); + }); + }); + + describe('Given model with template variable of type adhoc', () => { + let model: DashboardModel; + + beforeEach(() => { + model = new DashboardModel({ + templating: { + list: [ + { + name: 'Filter', + type: 'adhoc', + filters: [ + { + key: '@hostname', + operator: '=', + value: 'server 20', + }, + ], + }, + ], + }, + }); + expect(model.hasVariableValuesChanged()).toBeFalsy(); + }); + + it('hasVariableValuesChanged should be false when adding a template variable', () => { + model.templating.list.push({ + name: 'Filter', + type: 'adhoc', + filters: [ + { + key: '@hostname', + operator: '=', + value: 'server 1', + }, + ], + }); + expect(model.hasVariableValuesChanged()).toBeFalsy(); + }); + + it('hasVariableValuesChanged should be false when removing existing template variable', () => { + model.templating.list = []; + expect(model.hasVariableValuesChanged()).toBeFalsy(); + }); + + it('hasVariableValuesChanged should be true when changing value of filter', () => { + model.templating.list[0].filters[0].value = 'server 1'; + expect(model.hasVariableValuesChanged()).toBeTruthy(); + }); + + it('hasVariableValuesChanged should be true when adding an additional condition', () => { + model.templating.list[0].filters[0].condition = 'AND'; + model.templating.list[0].filters[1] = { + key: '@metric', + operator: '=', + value: 'logins.count', + }; + expect(model.hasVariableValuesChanged()).toBeTruthy(); + }); + + it('getSaveModelClone should return original variable when saveVariables=false', () => { + model.templating.list[0].filters[0].value = 'server 1'; + + let options = { saveVariables: false }; + let saveModel = model.getSaveModelClone(options); + + expect(saveModel.templating.list[0].filters[0].value).toBe('server 20'); + }); + + it('getSaveModelClone should return updated variable when saveVariables=true', () => { + model.templating.list[0].filters[0].value = 'server 1'; + + let options = { saveVariables: true }; + let saveModel = model.getSaveModelClone(options); + + expect(saveModel.templating.list[0].filters[0].value).toBe('server 1'); }); }); }); diff --git a/public/app/features/dashboard/specs/save_modal.jest.ts b/public/app/features/dashboard/specs/save_modal.jest.ts index ebae6f0f5bd..669ae43a0ff 100644 --- a/public/app/features/dashboard/specs/save_modal.jest.ts +++ b/public/app/features/dashboard/specs/save_modal.jest.ts @@ -1,128 +1,57 @@ import { SaveDashboardModalCtrl } from '../save_modal'; -jest.mock('app/core/services/context_srv', () => ({})); +const setup = (timeChanged, variableValuesChanged, cb) => { + const dash = { + hasTimeChanged: jest.fn().mockReturnValue(timeChanged), + hasVariableValuesChanged: jest.fn().mockReturnValue(variableValuesChanged), + resetOriginalTime: jest.fn(), + resetOriginalVariables: jest.fn(), + getSaveModelClone: jest.fn().mockReturnValue({}), + }; + const dashboardSrvMock = { + getCurrent: jest.fn().mockReturnValue(dash), + save: jest.fn().mockReturnValue(Promise.resolve()), + }; + const ctrl = new SaveDashboardModalCtrl(dashboardSrvMock); + ctrl.saveForm = { + $valid: true, + }; + ctrl.dismiss = () => Promise.resolve(); + cb(dash, ctrl, dashboardSrvMock); +}; describe('SaveDashboardModal', () => { - describe('save modal checkboxes', () => { - it('should show checkboxes', () => { - let fakeDashboardSrv = { - dash: { - templating: { - list: [ - { - current: { - selected: true, - tags: Array(0), - text: 'server_001', - value: 'server_001', - }, - name: 'Server', - }, - ], - }, - originalTemplating: [ - { - current: { - selected: true, - text: 'server_002', - value: 'server_002', - }, - name: 'Server', - }, - ], - time: { - from: 'now-3h', - to: 'now', - }, - originalTime: { - from: 'now-6h', - to: 'now', - }, - }, - }; - let modal = new SaveDashboardModalCtrl(fakeDashboardSrv); - - expect(modal.timeChange).toBe(true); - expect(modal.variableValueChange).toBe(true); + describe('Given time and template variable values have not changed', () => { + setup(false, false, (dash, ctrl: SaveDashboardModalCtrl) => { + it('When creating ctrl should set time and template variable values changed', () => { + expect(ctrl.timeChange).toBeFalsy(); + expect(ctrl.variableValueChange).toBeFalsy(); + }); }); + }); - it('should hide checkboxes', () => { - let fakeDashboardSrv = { - dash: { - templating: { - list: [ - { - current: { - selected: true, - text: 'server_002', - value: 'server_002', - }, - name: 'Server', - }, - ], - }, - originalTemplating: [ - { - current: { - selected: true, - text: 'server_002', - value: 'server_002', - }, - name: 'Server', - }, - ], - time: { - from: 'now-3h', - to: 'now', - }, - originalTime: { - from: 'now-3h', - to: 'now', - }, - }, - }; - let modal = new SaveDashboardModalCtrl(fakeDashboardSrv); - expect(modal.timeChange).toBe(false); - expect(modal.variableValueChange).toBe(false); - }); + describe('Given time and template variable values have changed', () => { + setup(true, true, (dash, ctrl: SaveDashboardModalCtrl) => { + it('When creating ctrl should set time and template variable values changed', () => { + expect(ctrl.timeChange).toBeTruthy(); + expect(ctrl.variableValueChange).toBeTruthy(); + }); - it('should hide variable checkboxes', () => { - let fakeDashboardSrv = { - dash: { - templating: { - list: [ - { - current: { - selected: true, - text: 'server_002', - value: 'server_002', - }, - name: 'Server', - }, - { - current: { - selected: true, - text: 'web_002', - value: 'web_002', - }, - name: 'Web', - }, - ], - }, - originalTemplating: [ - { - current: { - selected: true, - text: 'server_002', - value: 'server_002', - }, - name: 'Server', - }, - ], - }, - }; - let modal = new SaveDashboardModalCtrl(fakeDashboardSrv); - expect(modal.variableValueChange).toBe(false); + it('When save time and variable value changes disabled and saving should reset original time and template variable values', async () => { + ctrl.saveTimerange = false; + ctrl.saveVariables = false; + await ctrl.save(); + expect(dash.resetOriginalTime).toHaveBeenCalledTimes(0); + expect(dash.resetOriginalVariables).toHaveBeenCalledTimes(0); + }); + + it('When save time and variable value changes enabled and saving should reset original time and template variable values', async () => { + ctrl.saveTimerange = true; + ctrl.saveVariables = true; + await ctrl.save(); + expect(dash.resetOriginalTime).toHaveBeenCalledTimes(1); + expect(dash.resetOriginalVariables).toHaveBeenCalledTimes(1); + }); }); }); }); From 74d6b5fc1c7bb63f1cb2b37b6c208461851ccaec Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Tue, 19 Jun 2018 16:12:02 +0200 Subject: [PATCH 5/8] dashboard: fix drop down links (cherry picked from commit 4ef4a4d3a7afb66dd19dc6a2e99343212188c73b) --- public/app/features/dashlinks/module.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/public/app/features/dashlinks/module.ts b/public/app/features/dashlinks/module.ts index 148d32f4399..f9482049c40 100644 --- a/public/app/features/dashlinks/module.ts +++ b/public/app/features/dashlinks/module.ts @@ -48,9 +48,11 @@ function dashLink($compile, $sanitize, linkSrv) { function update() { var linkInfo = linkSrv.getAnchorInfo(link); span.text(linkInfo.title); - anchor.attr('href', linkInfo.href); - sanitizeAnchor(); - + if (!link.asDropdown) { + anchor.attr('href', linkInfo.href); + sanitizeAnchor(); + } + elem.find('a').attr('data-placement', 'bottom'); // tooltip elem.find('a').tooltip({ title: $sanitize(scope.link.tooltip), From 7c94d5cd1a05eb8e28315621da85f859da0faf0f Mon Sep 17 00:00:00 2001 From: Alexander Zobnin Date: Thu, 31 May 2018 22:18:59 +0300 Subject: [PATCH 6/8] graph: fix legend decimals precision calculation (cherry picked from commit 24f6d34abd38eef8bafe3767c632d276159269c2) --- public/app/core/specs/ticks.jest.ts | 25 +++++++++++ public/app/core/specs/time_series.jest.ts | 52 +++++++++++++++++++++++ public/app/core/time_series2.ts | 14 +++--- public/app/core/utils/ticks.ts | 18 +++----- public/app/plugins/panel/graph/graph.ts | 3 +- 5 files changed, 94 insertions(+), 18 deletions(-) create mode 100644 public/app/core/specs/ticks.jest.ts diff --git a/public/app/core/specs/ticks.jest.ts b/public/app/core/specs/ticks.jest.ts new file mode 100644 index 00000000000..8b7e0cd73b5 --- /dev/null +++ b/public/app/core/specs/ticks.jest.ts @@ -0,0 +1,25 @@ +import * as ticks from '../utils/ticks'; + +describe('ticks', () => { + describe('getFlotTickDecimals()', () => { + let ctx: any = {}; + + beforeEach(() => { + ctx.axis = {}; + }); + + it('should calculate decimals precision based on graph height', () => { + let dec = ticks.getFlotTickDecimals(0, 10, ctx.axis, 200); + expect(dec.tickDecimals).toBe(1); + expect(dec.scaledDecimals).toBe(1); + + dec = ticks.getFlotTickDecimals(0, 100, ctx.axis, 200); + expect(dec.tickDecimals).toBe(0); + expect(dec.scaledDecimals).toBe(-1); + + dec = ticks.getFlotTickDecimals(0, 1, ctx.axis, 200); + expect(dec.tickDecimals).toBe(2); + expect(dec.scaledDecimals).toBe(3); + }); + }); +}); diff --git a/public/app/core/specs/time_series.jest.ts b/public/app/core/specs/time_series.jest.ts index 6214c687add..f5245476218 100644 --- a/public/app/core/specs/time_series.jest.ts +++ b/public/app/core/specs/time_series.jest.ts @@ -1,4 +1,5 @@ import TimeSeries from 'app/core/time_series2'; +import { updateLegendValues } from 'app/core/time_series2'; describe('TimeSeries', function() { var points, series; @@ -311,4 +312,55 @@ describe('TimeSeries', function() { expect(series.formatValue(-Infinity)).toBe(''); }); }); + + describe('legend decimals', function() { + let series, panel; + let height = 200; + beforeEach(function() { + testData = { + alias: 'test', + datapoints: [[1, 2], [0, 3], [10, 4], [8, 5]], + }; + series = new TimeSeries(testData); + series.getFlotPairs(); + panel = { + decimals: null, + yaxes: [ + { + decimals: null, + }, + ], + }; + }); + + it('should set decimals based on Y axis (expect calculated decimals = 1)', function() { + let data = [series]; + // Expect ticks with this data will have decimals = 1 + updateLegendValues(data, panel, height); + expect(data[0].decimals).toBe(2); + }); + + it('should set decimals based on Y axis to 0 if calculated decimals = 0)', function() { + testData.datapoints = [[10, 2], [0, 3], [100, 4], [80, 5]]; + series = new TimeSeries(testData); + series.getFlotPairs(); + let data = [series]; + updateLegendValues(data, panel, height); + expect(data[0].decimals).toBe(0); + }); + + it('should set decimals to Y axis decimals + 1', function() { + panel.yaxes[0].decimals = 2; + let data = [series]; + updateLegendValues(data, panel, height); + expect(data[0].decimals).toBe(3); + }); + + it('should set decimals to legend decimals value if it was set explicitly', function() { + panel.decimals = 3; + let data = [series]; + updateLegendValues(data, panel, height); + expect(data[0].decimals).toBe(3); + }); + }); }); diff --git a/public/app/core/time_series2.ts b/public/app/core/time_series2.ts index 4da64850e59..59729ebc312 100644 --- a/public/app/core/time_series2.ts +++ b/public/app/core/time_series2.ts @@ -23,23 +23,27 @@ function translateFillOption(fill) { * Calculate decimals for legend and update values for each series. * @param data series data * @param panel + * @param height */ -export function updateLegendValues(data: TimeSeries[], panel) { +export function updateLegendValues(data: TimeSeries[], panel, height) { for (let i = 0; i < data.length; i++) { let series = data[i]; - let yaxes = panel.yaxes; + const yaxes = panel.yaxes; const seriesYAxis = series.yaxis || 1; - let axis = yaxes[seriesYAxis - 1]; - let { tickDecimals, scaledDecimals } = getFlotTickDecimals(data, axis); - let formater = kbn.valueFormats[panel.yaxes[seriesYAxis - 1].format]; + const axis = yaxes[seriesYAxis - 1]; + let formater = kbn.valueFormats[axis.format]; // decimal override if (_.isNumber(panel.decimals)) { series.updateLegendValues(formater, panel.decimals, null); + } else if (_.isNumber(axis.decimals)) { + series.updateLegendValues(formater, axis.decimals + 1, null); } else { // auto decimals // legend and tooltip gets one more decimal precision // than graph legend ticks + const { datamin, datamax } = getDataMinMax(data); + let { tickDecimals, scaledDecimals } = getFlotTickDecimals(datamin, datamax, axis, height); tickDecimals = (tickDecimals || -1) + 1; series.updateLegendValues(formater, tickDecimals, scaledDecimals + 2); } diff --git a/public/app/core/utils/ticks.ts b/public/app/core/utils/ticks.ts index db65104cfc0..66e6a7ce4fc 100644 --- a/public/app/core/utils/ticks.ts +++ b/public/app/core/utils/ticks.ts @@ -1,5 +1,3 @@ -import { getDataMinMax } from 'app/core/time_series2'; - /** * Calculate tick step. * Implementation from d3-array (ticks.js) @@ -121,12 +119,10 @@ export function getFlotRange(panelMin, panelMax, datamin, datamax) { * Calculate tick decimals. * Implementation from Flot. */ -export function getFlotTickDecimals(data, axis) { - let { datamin, datamax } = getDataMinMax(data); - let { min, max } = getFlotRange(axis.min, axis.max, datamin, datamax); - let noTicks = 3; - let tickDecimals, maxDec; - let delta = (max - min) / noTicks; +export function getFlotTickDecimals(datamin, datamax, axis, height) { + const { min, max } = getFlotRange(axis.min, axis.max, datamin, datamax); + const noTicks = 0.3 * Math.sqrt(height); + const delta = (max - min) / noTicks; let dec = -Math.floor(Math.log(delta) / Math.LN10); let magn = Math.pow(10, -dec); @@ -139,19 +135,17 @@ export function getFlotTickDecimals(data, axis) { } else if (norm < 3) { size = 2; // special case for 2.5, requires an extra decimal - if (norm > 2.25 && (maxDec == null || dec + 1 <= maxDec)) { + if (norm > 2.25) { size = 2.5; - ++dec; } } else if (norm < 7.5) { size = 5; } else { size = 10; } - size *= magn; - tickDecimals = Math.max(0, maxDec != null ? maxDec : dec); + const tickDecimals = Math.max(0, -Math.floor(Math.log(delta) / Math.LN10) + 1); // grafana addition const scaledDecimals = tickDecimals - Math.floor(Math.log(size) / Math.LN10); return { tickDecimals, scaledDecimals }; diff --git a/public/app/plugins/panel/graph/graph.ts b/public/app/plugins/panel/graph/graph.ts index 9e4fb42952e..9f216c12288 100755 --- a/public/app/plugins/panel/graph/graph.ts +++ b/public/app/plugins/panel/graph/graph.ts @@ -64,7 +64,8 @@ function graphDirective(timeSrv, popoverSrv, contextSrv) { } annotations = ctrl.annotations || []; buildFlotPairs(data); - updateLegendValues(data, panel); + const graphHeight = elem.height(); + updateLegendValues(data, panel, graphHeight); ctrl.events.emit('render-legend'); }); From b4c1df11f65c3fcd0242f422a7e09dc694b7ed09 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Tue, 19 Jun 2018 16:56:14 +0200 Subject: [PATCH 7/8] make sure to process panels in collapsed rows when exporting dashboard (cherry picked from commit a2e08dc4e84df43aa1e7702f9e5eeac4b1991d02) --- .../app/features/dashboard/export/exporter.ts | 15 +++++- .../features/dashboard/specs/exporter.jest.ts | 54 ++++++++++++++++++- 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/public/app/features/dashboard/export/exporter.ts b/public/app/features/dashboard/export/exporter.ts index 8b93c12bf50..fc24de76fcc 100644 --- a/public/app/features/dashboard/export/exporter.ts +++ b/public/app/features/dashboard/export/exporter.ts @@ -63,8 +63,7 @@ export class DashboardExporter { ); }; - // check up panel data sources - for (let panel of saveModel.panels) { + const processPanel = panel => { if (panel.datasource !== undefined) { templateizeDatasourceUsage(panel); } @@ -86,6 +85,18 @@ export class DashboardExporter { version: panelDef.info.version, }; } + }; + + // check up panel data sources + for (let panel of saveModel.panels) { + processPanel(panel); + + // handle collapsed rows + if (panel.collapsed !== undefined && panel.collapsed === true && panel.panels) { + for (let rowPanel of panel.panels) { + processPanel(rowPanel); + } + } } // templatize template vars diff --git a/public/app/features/dashboard/specs/exporter.jest.ts b/public/app/features/dashboard/specs/exporter.jest.ts index f94d1368aa5..aa574a4b85a 100644 --- a/public/app/features/dashboard/specs/exporter.jest.ts +++ b/public/app/features/dashboard/specs/exporter.jest.ts @@ -62,6 +62,27 @@ describe('given dashboard with repeated panels', () => { type: 'graph', }, { id: 3, repeat: null, repeatPanelId: 2 }, + { + id: 4, + collapsed: true, + panels: [ + { id: 10, datasource: 'gfdb', type: 'table' }, + { id: 11 }, + { + id: 12, + datasource: '-- Mixed --', + targets: [{ datasource: 'other' }], + }, + { id: 13, datasource: '$ds' }, + { + id: 14, + repeat: 'apps', + datasource: 'gfdb', + type: 'heatmap', + }, + { id: 15, repeat: null, repeatPanelId: 14 }, + ], + }, ], }; @@ -78,6 +99,18 @@ describe('given dashboard with repeated panels', () => { info: { version: '1.1.0' }, }; + config.panels['table'] = { + id: 'table', + name: 'Table', + info: { version: '1.1.1' }, + }; + + config.panels['heatmap'] = { + id: 'heatmap', + name: 'Heatmap', + info: { version: '1.1.2' }, + }; + dash = new DashboardModel(dash, {}); var exporter = new DashboardExporter(datasourceSrvStub); exporter.makeExportable(dash).then(clean => { @@ -91,6 +124,11 @@ describe('given dashboard with repeated panels', () => { expect(panel.datasource).toBe('${DS_GFDB}'); }); + it('should replace datasource refs in collapsed row', () => { + var panel = exported.panels[5].panels[0]; + expect(panel.datasource).toBe('${DS_GFDB}'); + }); + it('should replace datasource in variable query', () => { expect(exported.templating.list[0].datasource).toBe('${DS_GFDB}'); expect(exported.templating.list[0].options.length).toBe(0); @@ -126,13 +164,27 @@ describe('given dashboard with repeated panels', () => { expect(require).not.toBe(undefined); }); - it('should add panel to required', () => { + it('should add graph panel to required', () => { var require = _.find(exported.__requires, { name: 'Graph' }); expect(require.name).toBe('Graph'); expect(require.id).toBe('graph'); expect(require.version).toBe('1.1.0'); }); + it('should add table panel to required', () => { + var require = _.find(exported.__requires, { name: 'Table' }); + expect(require.name).toBe('Table'); + expect(require.id).toBe('table'); + expect(require.version).toBe('1.1.1'); + }); + + it('should add heatmap panel to required', () => { + var require = _.find(exported.__requires, { name: 'Heatmap' }); + expect(require.name).toBe('Heatmap'); + expect(require.id).toBe('heatmap'); + expect(require.version).toBe('1.1.2'); + }); + it('should add grafana version', () => { var require = _.find(exported.__requires, { name: 'Grafana' }); expect(require.type).toBe('grafana'); From cdae9126edb35303bc3071f4079eb5a9d2699f91 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Wed, 20 Jun 2018 14:58:30 +0200 Subject: [PATCH 8/8] release v5.2.0-beta2 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 599364573c8..b2832fef90a 100644 --- a/package.json +++ b/package.json @@ -4,7 +4,7 @@ "company": "Grafana Labs" }, "name": "grafana", - "version": "5.2.0-beta1", + "version": "5.2.0-beta2", "repository": { "type": "git", "url": "http://github.com/grafana/grafana.git"