From 591df92265b683b36b0350cc65befde93b812ae5 Mon Sep 17 00:00:00 2001 From: idafurjes <36131195+idafurjes@users.noreply.github.com> Date: Thu, 22 Sep 2022 19:02:55 +0200 Subject: [PATCH] Chore: Copy methods from sqlstore to org store (#55615) * Chore: Copy methods from sqlstore to org store * Rename method, add test * Add comments of tests --- pkg/services/org/model.go | 31 ++++++-- pkg/services/org/org.go | 3 +- pkg/services/org/orgimpl/org.go | 90 ++-------------------- pkg/services/org/orgimpl/org_test.go | 22 +++++- pkg/services/org/orgimpl/store.go | 102 +++++++++++++++++++++++++ pkg/services/org/orgimpl/store_test.go | 91 +++++++++++++++++++++- pkg/services/org/orgtest/fake.go | 5 +- 7 files changed, 246 insertions(+), 98 deletions(-) diff --git a/pkg/services/org/model.go b/pkg/services/org/model.go index 6ffecfa6cc2..ce73e8fd5e8 100644 --- a/pkg/services/org/model.go +++ b/pkg/services/org/model.go @@ -50,7 +50,7 @@ type CreateOrgCommand struct { Name string `json:"name" binding:"Required"` // initial admin user for account - UserID int64 `json:"-"` + UserID int64 `json:"-" xorm:"user_id"` } type GetOrgIDForNewUserCommand struct { @@ -62,11 +62,11 @@ type GetOrgIDForNewUserCommand struct { } type GetUserOrgListQuery struct { - UserID int64 + UserID int64 `xorm:"user_id"` } type UserOrgDTO struct { - OrgID int64 `json:"orgId"` + OrgID int64 `json:"orgId" xorm:"org_id"` Name string `json:"name"` Role RoleType `json:"role"` } @@ -81,11 +81,11 @@ type SearchOrgsQuery struct { Name string Limit int Page int - IDs []int64 + IDs []int64 `xorm:"ids"` } type OrgDTO struct { - ID int64 `json:"id"` + ID int64 `json:"id" xorm:"id"` Name string `json:"name"` } @@ -168,3 +168,24 @@ func (r *RoleType) UnmarshalText(data []byte) error { return nil } + +type ByOrgName []*UserOrgDTO + +// Len returns the length of an array of organisations. +func (o ByOrgName) Len() int { + return len(o) +} + +// Swap swaps two indices of an array of organizations. +func (o ByOrgName) Swap(i, j int) { + o[i], o[j] = o[j], o[i] +} + +// Less returns whether element i of an array of organizations is less than element j. +func (o ByOrgName) Less(i, j int) bool { + if strings.ToLower(o[i].Name) < strings.ToLower(o[j].Name) { + return true + } + + return o[i].Name < o[j].Name +} diff --git a/pkg/services/org/org.go b/pkg/services/org/org.go index f899b588d5f..af4299f610f 100644 --- a/pkg/services/org/org.go +++ b/pkg/services/org/org.go @@ -14,8 +14,7 @@ type Service interface { GetByID(context.Context, *GetOrgByIdQuery) (*Org, error) GetByNameHandler(context.Context, *GetOrgByNameQuery) (*Org, error) GetByName(string) (*Org, error) - CreateWithMember(string, int64) (*Org, error) - Create(context.Context, *CreateOrgCommand) (*Org, error) + CreateWithMember(context.Context, *CreateOrgCommand) (*Org, error) UpdateAddress(context.Context, *UpdateOrgAddressCommand) error Delete(context.Context, *DeleteOrgCommand) error GetOrCreate(context.Context, string) (int64, error) diff --git a/pkg/services/org/orgimpl/org.go b/pkg/services/org/orgimpl/org.go index 2b6baa06803..da5d658a58a 100644 --- a/pkg/services/org/orgimpl/org.go +++ b/pkg/services/org/orgimpl/org.go @@ -85,52 +85,19 @@ func (s *Service) DeleteUserFromAll(ctx context.Context, userID int64) error { return s.store.DeleteUserFromAll(ctx, userID) } -// TODO: remove wrapper around sqlstore +// TODO: refactor service to call store CRUD method func (s *Service) GetUserOrgList(ctx context.Context, query *org.GetUserOrgListQuery) ([]*org.UserOrgDTO, error) { - q := &models.GetUserOrgListQuery{ - UserId: query.UserID, - } - err := s.sqlStore.GetUserOrgList(ctx, q) - if err != nil { - return nil, err - } - var result []*org.UserOrgDTO - for _, orga := range q.Result { - result = append(result, &org.UserOrgDTO{ - OrgID: orga.OrgId, - Name: orga.Name, - Role: orga.Role, - }) - } - return result, nil + return s.store.GetUserOrgList(ctx, query) } +// TODO: refactor service to call store CRUD method func (s *Service) UpdateOrg(ctx context.Context, cmd *org.UpdateOrgCommand) error { return s.store.Update(ctx, cmd) } -// TODO: remove wrapper around sqlstore +// TODO: refactor service to call store CRUD method func (s *Service) Search(ctx context.Context, query *org.SearchOrgsQuery) ([]*org.OrgDTO, error) { - var res []*org.OrgDTO - q := &models.SearchOrgsQuery{ - Query: query.Query, - Name: query.Name, - Limit: query.Limit, - Page: query.Page, - Ids: query.IDs, - } - err := s.sqlStore.SearchOrgs(ctx, q) - if err != nil { - return nil, err - } - - for _, r := range q.Result { - res = append(res, &org.OrgDTO{ - ID: r.Id, - Name: r.Name, - }) - } - return res, nil + return s.store.Search(ctx, query) } // TODO: remove wrapper around sqlstore @@ -198,50 +165,9 @@ func (s *Service) GetByName(name string) (*org.Org, error) { }, nil } -// TODO: remove wrapper around sqlstore -func (s *Service) CreateWithMember(name string, userID int64) (*org.Org, error) { - orga, err := s.sqlStore.CreateOrgWithMember(name, userID) - if err != nil { - return nil, err - } - return &org.Org{ - ID: orga.Id, - Version: orga.Version, - Name: orga.Name, - Address1: orga.Address1, - Address2: orga.Address2, - City: orga.City, - ZipCode: orga.ZipCode, - State: orga.State, - Country: orga.Country, - Created: orga.Created, - Updated: orga.Updated, - }, nil -} - -// TODO: remove wrapper around sqlstore -func (s *Service) Create(ctx context.Context, cmd *org.CreateOrgCommand) (*org.Org, error) { - q := &models.CreateOrgCommand{ - Name: cmd.Name, - UserId: cmd.UserID, - } - err := s.sqlStore.CreateOrg(ctx, q) - if err != nil { - return nil, err - } - return &org.Org{ - ID: q.Result.Id, - Version: q.Result.Version, - Name: q.Result.Name, - Address1: q.Result.Address1, - Address2: q.Result.Address2, - City: q.Result.City, - ZipCode: q.Result.ZipCode, - State: q.Result.State, - Country: q.Result.Country, - Created: q.Result.Created, - Updated: q.Result.Updated, - }, nil +// TODO: refactor service to call store CRUD method +func (s *Service) CreateWithMember(ctx context.Context, cmd *org.CreateOrgCommand) (*org.Org, error) { + return s.store.CreateWithMember(ctx, cmd) } // TODO: refactor service to call store CRUD method diff --git a/pkg/services/org/orgimpl/org_test.go b/pkg/services/org/orgimpl/org_test.go index e7add921e96..51963f31b81 100644 --- a/pkg/services/org/orgimpl/org_test.go +++ b/pkg/services/org/orgimpl/org_test.go @@ -54,10 +54,12 @@ func TestOrgService(t *testing.T) { } type FakeOrgStore struct { - ExpectedOrg *org.Org - ExpectedOrgID int64 - ExpectedUserID int64 - ExpectedError error + ExpectedOrg *org.Org + ExpectedOrgID int64 + ExpectedUserID int64 + ExpectedError error + ExpectedUserOrgs []*org.UserOrgDTO + ExpectedOrgs []*org.OrgDTO } func newOrgStoreFake() *FakeOrgStore { @@ -91,3 +93,15 @@ func (f *FakeOrgStore) UpdateAddress(ctx context.Context, cmd *org.UpdateOrgAddr func (f *FakeOrgStore) Delete(ctx context.Context, cmd *org.DeleteOrgCommand) error { return f.ExpectedError } + +func (f *FakeOrgStore) GetUserOrgList(ctx context.Context, query *org.GetUserOrgListQuery) ([]*org.UserOrgDTO, error) { + return f.ExpectedUserOrgs, f.ExpectedError +} + +func (f *FakeOrgStore) Search(ctx context.Context, query *org.SearchOrgsQuery) ([]*org.OrgDTO, error) { + return f.ExpectedOrgs, f.ExpectedError +} + +func (f *FakeOrgStore) CreateWithMember(ctx context.Context, cmd *org.CreateOrgCommand) (*org.Org, error) { + return f.ExpectedOrg, f.ExpectedError +} diff --git a/pkg/services/org/orgimpl/store.go b/pkg/services/org/orgimpl/store.go index ae6f8c0936d..affe48cba56 100644 --- a/pkg/services/org/orgimpl/store.go +++ b/pkg/services/org/orgimpl/store.go @@ -2,6 +2,8 @@ package orgimpl import ( "context" + "fmt" + "sort" "time" "github.com/grafana/grafana/pkg/events" @@ -24,6 +26,9 @@ type store interface { // TO BE REFACTORED - move logic to service methods and leave CRUD methods for store UpdateAddress(context.Context, *org.UpdateOrgAddressCommand) error Delete(context.Context, *org.DeleteOrgCommand) error + GetUserOrgList(context.Context, *org.GetUserOrgListQuery) ([]*org.UserOrgDTO, error) + Search(context.Context, *org.SearchOrgsQuery) ([]*org.OrgDTO, error) + CreateWithMember(context.Context, *org.CreateOrgCommand) (*org.Org, error) } type sqlStore struct { @@ -217,3 +222,100 @@ func (ss *sqlStore) Delete(ctx context.Context, cmd *org.DeleteOrgCommand) error return nil }) } + +// TODO: refactor move logic to service method +func (ss *sqlStore) GetUserOrgList(ctx context.Context, query *org.GetUserOrgListQuery) ([]*org.UserOrgDTO, error) { + result := make([]*org.UserOrgDTO, 0) + err := ss.db.WithDbSession(ctx, func(dbSess *sqlstore.DBSession) error { + sess := dbSess.Table("org_user") + sess.Join("INNER", "org", "org_user.org_id=org.id") + sess.Join("INNER", ss.dialect.Quote("user"), fmt.Sprintf("org_user.user_id=%s.id", ss.dialect.Quote("user"))) + sess.Where("org_user.user_id=?", query.UserID) + sess.Where(ss.notServiceAccountFilter()) + sess.Cols("org.name", "org_user.role", "org_user.org_id") + sess.OrderBy("org.name") + err := sess.Find(&result) + sort.Sort(org.ByOrgName(result)) + return err + }) + if err != nil { + return nil, err + } + return result, nil +} + +func (ss *sqlStore) notServiceAccountFilter() string { + return fmt.Sprintf("%s.is_service_account = %s", + ss.dialect.Quote("user"), + ss.dialect.BooleanStr(false)) +} + +func (ss *sqlStore) Search(ctx context.Context, query *org.SearchOrgsQuery) ([]*org.OrgDTO, error) { + result := make([]*org.OrgDTO, 0) + err := ss.db.WithDbSession(ctx, func(dbSession *sqlstore.DBSession) error { + sess := dbSession.Table("org") + if query.Query != "" { + sess.Where("name LIKE ?", query.Query+"%") + } + if query.Name != "" { + sess.Where("name=?", query.Name) + } + + if len(query.IDs) > 0 { + sess.In("id", query.IDs) + } + + if query.Limit > 0 { + sess.Limit(query.Limit, query.Limit*query.Page) + } + + sess.Cols("id", "name") + err := sess.Find(&result) + return err + }) + if err != nil { + return nil, err + } + return result, nil +} + +// CreateWithMember creates an organization with a certain name and a certain user as member. +func (ss *sqlStore) CreateWithMember(ctx context.Context, cmd *org.CreateOrgCommand) (*org.Org, error) { + orga := org.Org{ + Name: cmd.Name, + Created: time.Now(), + Updated: time.Now(), + } + if err := ss.db.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { + if isNameTaken, err := isOrgNameTaken(cmd.Name, 0, sess); err != nil { + return err + } else if isNameTaken { + return models.ErrOrgNameTaken + } + + if _, err := sess.Insert(&orga); err != nil { + return err + } + + user := org.OrgUser{ + OrgID: orga.ID, + UserID: cmd.UserID, + Role: org.RoleAdmin, + Created: time.Now(), + Updated: time.Now(), + } + + _, err := sess.Insert(&user) + + sess.PublishAfterCommit(&events.OrgCreated{ + Timestamp: orga.Created, + Id: orga.ID, + Name: orga.Name, + }) + + return err + }); err != nil { + return &orga, err + } + return &orga, nil +} diff --git a/pkg/services/org/orgimpl/store_test.go b/pkg/services/org/orgimpl/store_test.go index 6842824f2b7..e82141704e0 100644 --- a/pkg/services/org/orgimpl/store_test.go +++ b/pkg/services/org/orgimpl/store_test.go @@ -2,6 +2,7 @@ package orgimpl import ( "context" + "fmt" "testing" "time" @@ -94,6 +95,58 @@ func TestIntegrationOrgDataAccess(t *testing.T) { // err = orgStore.GetSignedInUser(context.Background(), &models.GetSignedInUserQuery{UserId: ac2.ID}) // require.Equal(t, err, user.ErrUserNotFound) }) + + t.Run("Given we have organizations, we can query them by IDs", func(t *testing.T) { + var err error + var cmd *org.CreateOrgCommand + ids := []int64{} + + for i := 1; i < 4; i++ { + cmd = &org.CreateOrgCommand{Name: fmt.Sprint("Org #", i)} + result, err := orgStore.CreateWithMember(context.Background(), cmd) + require.NoError(t, err) + + ids = append(ids, result.ID) + } + + query := &org.SearchOrgsQuery{IDs: ids} + result, err := orgStore.Search(context.Background(), query) + require.NoError(t, err) + require.Equal(t, len(result), 3) + }) + + t.Run("Given we have organizations, we can limit and paginate search", func(t *testing.T) { + ss = sqlstore.InitTestDB(t) + for i := 1; i < 4; i++ { + cmd := &org.CreateOrgCommand{Name: fmt.Sprint("Orga #", i)} + _, err := orgStore.CreateWithMember(context.Background(), cmd) + require.NoError(t, err) + } + + t.Run("Should be able to search with defaults", func(t *testing.T) { + query := &org.SearchOrgsQuery{} + result, err := orgStore.Search(context.Background(), query) + + require.NoError(t, err) + require.Equal(t, len(result), 3) + }) + + t.Run("Should be able to limit search", func(t *testing.T) { + query := &org.SearchOrgsQuery{Limit: 1} + result, err := orgStore.Search(context.Background(), query) + + require.NoError(t, err) + require.Equal(t, len(result), 1) + }) + + t.Run("Should be able to limit and paginate search", func(t *testing.T) { + query := &org.SearchOrgsQuery{Limit: 2, Page: 1} + result, err := orgStore.Search(context.Background(), query) + + require.NoError(t, err) + require.Equal(t, len(result), 1) + }) + }) } func TestIntegrationOrgUserDataAccess(t *testing.T) { @@ -103,7 +156,8 @@ func TestIntegrationOrgUserDataAccess(t *testing.T) { ss := sqlstore.InitTestDB(t) orgUserStore := sqlStore{ - db: ss, + db: ss, + dialect: ss.GetDialect(), } t.Run("org user inserted", func(t *testing.T) { @@ -121,4 +175,39 @@ func TestIntegrationOrgUserDataAccess(t *testing.T) { err := orgUserStore.DeleteUserFromAll(context.Background(), 1) require.NoError(t, err) }) + + // TODO: these test will be added when store will be CRUD + // t.Run("Given single org mode", func(t *testing.T) { + // sqlStore.Cfg.AutoAssignOrg = true + // sqlStore.Cfg.AutoAssignOrgId = 1 + // sqlStore.Cfg.AutoAssignOrgRole = "Viewer" + + // t.Run("Users should be added to default organization", func(t *testing.T) { + // ac1cmd := user.CreateUserCommand{Login: "ac1", Email: "ac1@test.com", Name: "ac1 name"} + // ac2cmd := user.CreateUserCommand{Login: "ac2", Email: "ac2@test.com", Name: "ac2 name"} + + // ac1, err := sqlStore.CreateUser(context.Background(), ac1cmd) + // require.NoError(t, err) + // ac2, err := sqlStore.CreateUser(context.Background(), ac2cmd) + // require.NoError(t, err) + + // q1 := models.GetUserOrgListQuery{UserId: ac1.ID} + // q2 := models.GetUserOrgListQuery{UserId: ac2.ID} + // err = sqlStore.GetUserOrgList(context.Background(), &q1) + // require.NoError(t, err) + // err = sqlStore.GetUserOrgList(context.Background(), &q2) + // require.NoError(t, err) + + // require.Equal(t, q1.Result[0].OrgId, q2.Result[0].OrgId) + // require.Equal(t, string(q1.Result[0].Role), "Viewer") + // }) + // }) + + // t.Run("Can get user organizations", func(t *testing.T) { + // query := models.GetUserOrgListQuery{UserId: ac2.ID} + // err := sqlStore.GetUserOrgList(context.Background(), &query) + + // require.NoError(t, err) + // require.Equal(t, len(query.Result), 2) + // }) } diff --git a/pkg/services/org/orgtest/fake.go b/pkg/services/org/orgtest/fake.go index 08f61420e0f..fe910095b7f 100644 --- a/pkg/services/org/orgtest/fake.go +++ b/pkg/services/org/orgtest/fake.go @@ -58,10 +58,7 @@ func (f *FakeOrgService) GetByName(name string) (*org.Org, error) { return f.ExpectedOrg, f.ExpectedError } -func (f *FakeOrgService) CreateWithMember(name string, userID int64) (*org.Org, error) { - return f.ExpectedOrg, f.ExpectedError -} -func (f *FakeOrgService) Create(ctx context.Context, cmd *org.CreateOrgCommand) (*org.Org, error) { +func (f *FakeOrgService) CreateWithMember(ctx context.Context, cmd *org.CreateOrgCommand) (*org.Org, error) { return f.ExpectedOrg, f.ExpectedError }