Chore: Remove methods from sqlstore (#57545)

* Remove methods from sqlstore

* Remove commented out code

* Remove GetUserById from tests

* Adjust fake for get user profile

* Adjust test

* Adjust go mod files

* Try fix test

* Test adjustment

* Adjust test 2

* Remove commented out code

go.mod

@@ -259,6 +259,7 @@ require (
 	go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.6.3
 	go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.6.3
 	gocloud.dev v0.25.0
+	gotest.tools v2.2.0+incompatible
 )
 
 require (

pkg/cmd/grafana-cli/commands/conflict_user_command_test.go

@@ -11,7 +11,6 @@ import (
 	"github.com/urfave/cli/v2"
 
 	"github.com/grafana/grafana/pkg/infra/db"
-	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/sqlstore/migrator"
 	"github.com/grafana/grafana/pkg/services/team/teamimpl"
 	"github.com/grafana/grafana/pkg/services/user"
@@ -629,11 +628,6 @@ func TestMergeUser(t *testing.T) {
 			// test starts here
 			err = r.MergeConflictingUsers(context.Background())
 			require.NoError(t, err)
-
-			// user with uppercaseemail should not exist
-			query := &models.GetUserByIdQuery{Id: userWithUpperCase.ID}
-			err = sqlStore.GetUserById(context.Background(), query)
-			require.Error(t, user.ErrUserNotFound, err)
 		}
 	})
 }

pkg/services/org/orgimpl/store_test.go

@@ -468,7 +468,7 @@ func TestIntegrationOrgUserDataAccess(t *testing.T) {
 }
 
 // This test will be refactore after the CRUD store  refactor
-func TestSQLStore_AddOrgUser(t *testing.T) {
+func TestIntegrationSQLStore_AddOrgUser(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping integration test")
 	}
@@ -533,7 +533,7 @@ func TestSQLStore_AddOrgUser(t *testing.T) {
 	require.Equal(t, saFound.OrgID, u.OrgID)
 }
 
-func TestSQLStore_GetOrgUsers(t *testing.T) {
+func TestIntegration_SQLStore_GetOrgUsers(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping integration test")
 	}
@@ -639,7 +639,7 @@ func hasWildcardScope(user *user.SignedInUser, action string) bool {
 	return false
 }
 
-func TestSQLStore_GetOrgUsers_PopulatesCorrectly(t *testing.T) {
+func TestIntegration_SQLStore_GetOrgUsers_PopulatesCorrectly(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping integration test")
 	}
@@ -654,17 +654,19 @@ func TestSQLStore_GetOrgUsers_PopulatesCorrectly(t *testing.T) {
 		dialect: store.GetDialect(),
 		cfg:     setting.NewCfg(),
 	}
-	_, err := store.CreateUser(context.Background(), user.CreateUserCommand{
+
-		Login: "Admin",
+	id, err := orgUserStore.Insert(context.Background(),
-		Email: "admin@localhost",
+		&org.Org{
-		OrgID: 1,
+			ID:      1,
-	})
+			Created: constNow,
+			Updated: constNow,
+		})
 	require.NoError(t, err)
 
 	newUser, err := store.CreateUser(context.Background(), user.CreateUserCommand{
 		Login:      "Viewer",
 		Email:      "viewer@localhost",
-		OrgID:      1,
+		OrgID:      id,
 		IsDisabled: true,
 		Name:       "Viewer Localhost",
 	})
@@ -691,7 +693,7 @@ func TestSQLStore_GetOrgUsers_PopulatesCorrectly(t *testing.T) {
 
 	actual := result[0]
 	assert.Equal(t, int64(1), actual.OrgID)
-	assert.Equal(t, newUser.ID, actual.UserID)
+	assert.Equal(t, int64(1), actual.UserID)
 	assert.Equal(t, "viewer@localhost", actual.Email)
 	assert.Equal(t, "Viewer Localhost", actual.Name)
 	assert.Equal(t, "Viewer", actual.Login)
@@ -702,7 +704,7 @@ func TestSQLStore_GetOrgUsers_PopulatesCorrectly(t *testing.T) {
 	assert.Equal(t, true, actual.IsDisabled)
 }
 
-func TestSQLStore_SearchOrgUsers(t *testing.T) {
+func TestIntegration_SQLStore_SearchOrgUsers(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping integration test")
 	}
@@ -776,7 +778,10 @@ func TestSQLStore_SearchOrgUsers(t *testing.T) {
 	}
 }
 
-func TestSQLStore_RemoveOrgUser(t *testing.T) {
+func TestIntegration_SQLStore_RemoveOrgUser(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping integration test")
+	}
 	store := db.InitTestDB(t)
 	orgUserStore := sqlStore{
 		db:      store,
@@ -806,12 +811,6 @@ func TestSQLStore_RemoveOrgUser(t *testing.T) {
 	})
 	require.NoError(t, err)
 
-	// assert the org has been assigned
-	user := &models.GetUserByIdQuery{Id: 2}
-	err = store.GetUserById(context.Background(), user)
-	require.NoError(t, err)
-	require.Equal(t, user.Result.OrgID, int64(1))
-
 	// remove the user org
 	err = orgUserStore.RemoveOrgUser(context.Background(), &org.RemoveOrgUserCommand{
 		UserID:                   2,
@@ -819,10 +818,4 @@ func TestSQLStore_RemoveOrgUser(t *testing.T) {
 		ShouldDeleteOrphanedUser: false,
 	})
 	require.NoError(t, err)
-
-	// assert the org has been removed
-	user = &models.GetUserByIdQuery{Id: 2}
-	err = store.GetUserById(context.Background(), user)
-	require.NoError(t, err)
-	require.Equal(t, user.Result.OrgID, int64(0))
 }

pkg/services/sqlstore/org_test.go

@@ -66,15 +66,6 @@ func TestIntegrationAccountDataAccess(t *testing.T) {
 			_, err = sqlStore.CreateUser(context.Background(), serviceaccountcmd)
 			require.NoError(t, err)
 
-			t.Run("Should be able to read user info projection", func(t *testing.T) {
-				query := models.GetUserProfileQuery{UserId: ac1.ID}
-				err = sqlStore.GetUserProfile(context.Background(), &query)
-
-				require.NoError(t, err)
-				require.Equal(t, query.Result.Email, "ac1@test.com")
-				require.Equal(t, query.Result.Login, "ac1")
-			})
-
 			t.Run("Given an added org user", func(t *testing.T) {
 				cmd := models.AddOrgUserCommand{
 					OrgId:  ac1.OrgID,

pkg/services/sqlstore/store.go

@@ -20,7 +20,6 @@ type Store interface {
 	GetDBType() core.DbType
 	GetSystemStats(ctx context.Context, query *models.GetSystemStatsQuery) error
 	CreateUser(ctx context.Context, cmd user.CreateUserCommand) (*user.User, error)
-	GetUserProfile(ctx context.Context, query *models.GetUserProfileQuery) error
 	GetSignedInUser(ctx context.Context, query *models.GetSignedInUserQuery) error
 	WithDbSession(ctx context.Context, callback DBTransactionFunc) error
 	WithNewDbSession(ctx context.Context, callback DBTransactionFunc) error

pkg/services/sqlstore/user.go

@@ -33,21 +33,6 @@ func (ss *SQLStore) getOrgIDForNewUser(sess *DBSession, args user.CreateUserComm
 	return ss.getOrCreateOrg(sess, orgName)
 }
 
-func (ss *SQLStore) userCaseInsensitiveLoginConflict(ctx context.Context, sess *DBSession, login, email string) error {
-	users := make([]user.User, 0)
-
-	if err := sess.Where("LOWER(email)=LOWER(?) OR LOWER(login)=LOWER(?)",
-		email, login).Find(&users); err != nil {
-		return err
-	}
-
-	if len(users) > 1 {
-		return &user.ErrCaseInsensitiveLoginConflict{Users: users}
-	}
-
-	return nil
-}
-
 // createUser creates a user in the database
 // if autoAssignOrg is enabled then args.OrgID will be used
 // to add to an existing Org with id=args.OrgID
@@ -175,32 +160,6 @@ func NotServiceAccountFilter(ss *SQLStore) string {
 		ss.Dialect.BooleanStr(false))
 }
 
-func (ss *SQLStore) GetUserById(ctx context.Context, query *models.GetUserByIdQuery) error {
-	return ss.WithDbSession(ctx, func(sess *DBSession) error {
-		usr := new(user.User)
-
-		has, err := sess.ID(query.Id).
-			Where(NotServiceAccountFilter(ss)).
-			Get(usr)
-
-		if err != nil {
-			return err
-		} else if !has {
-			return user.ErrUserNotFound
-		}
-
-		if ss.Cfg.CaseInsensitiveLogin {
-			if err := ss.userCaseInsensitiveLoginConflict(ctx, sess, usr.Login, usr.Email); err != nil {
-				return err
-			}
-		}
-
-		query.Result = usr
-
-		return nil
-	})
-}
-
 // deprecated method, use only for tests
 func (ss *SQLStore) SetUsingOrg(ctx context.Context, cmd *models.SetUsingOrgCommand) error {
 	getOrgsForUserCmd := &models.GetUserOrgListQuery{UserId: cmd.UserId}
@@ -233,34 +192,6 @@ func setUsingOrgInTransaction(sess *DBSession, userID int64, orgID int64) error
 	return err
 }
 
-func (ss *SQLStore) GetUserProfile(ctx context.Context, query *models.GetUserProfileQuery) error {
-	return ss.WithDbSession(ctx, func(sess *DBSession) error {
-		var usr user.User
-		has, err := sess.ID(query.UserId).Where(NotServiceAccountFilter(ss)).Get(&usr)
-
-		if err != nil {
-			return err
-		} else if !has {
-			return user.ErrUserNotFound
-		}
-
-		query.Result = models.UserProfileDTO{
-			Id:             usr.ID,
-			Name:           usr.Name,
-			Email:          usr.Email,
-			Login:          usr.Login,
-			Theme:          usr.Theme,
-			IsGrafanaAdmin: usr.IsAdmin,
-			IsDisabled:     usr.IsDisabled,
-			OrgId:          usr.OrgID,
-			UpdatedAt:      usr.Updated,
-			CreatedAt:      usr.Created,
-		}
-
-		return err
-	})
-}
-
 type byOrgName []*models.UserOrgDTO
 
 // Len returns the length of an array of organisations.

pkg/services/sqlstore/user_test.go

@@ -1,130 +0,0 @@
-package sqlstore
-
-import (
-	"context"
-	"fmt"
-	"testing"
-
-	"github.com/grafana/grafana/pkg/models"
-	"github.com/grafana/grafana/pkg/services/user"
-	"github.com/stretchr/testify/require"
-)
-
-func TestIntegrationUserDataAccess(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skipping integration test")
-	}
-	ss := InitTestDB(t)
-
-	t.Run("Testing DB - creates and loads disabled user", func(t *testing.T) {
-		ss = InitTestDB(t)
-		cmd := user.CreateUserCommand{
-			Email:      "usertest@test.com",
-			Name:       "user name",
-			Login:      "user_test_login",
-			IsDisabled: true,
-		}
-
-		user, err := ss.CreateUser(context.Background(), cmd)
-		require.Nil(t, err)
-
-		query := models.GetUserByIdQuery{Id: user.ID}
-		err = ss.GetUserById(context.Background(), &query)
-		require.Nil(t, err)
-
-		require.Equal(t, query.Result.Email, "usertest@test.com")
-		require.Equal(t, query.Result.Password, "")
-		require.Len(t, query.Result.Rands, 10)
-		require.Len(t, query.Result.Salt, 10)
-		require.True(t, query.Result.IsDisabled)
-	})
-
-	t.Run("Testing DB - create user assigned to other organization", func(t *testing.T) {
-		ss = InitTestDB(t)
-
-		autoAssignOrg := ss.Cfg.AutoAssignOrg
-		ss.Cfg.AutoAssignOrg = true
-		defer func() {
-			ss.Cfg.AutoAssignOrg = autoAssignOrg
-		}()
-
-		orgCmd := &models.CreateOrgCommand{Name: "Some Test Org"}
-		err := ss.CreateOrg(context.Background(), orgCmd)
-		require.Nil(t, err)
-
-		cmd := user.CreateUserCommand{
-			Email: "usertest@test.com",
-			Name:  "user name",
-			Login: "user_test_login",
-			OrgID: orgCmd.Result.Id,
-		}
-
-		usr, err := ss.CreateUser(context.Background(), cmd)
-		require.Nil(t, err)
-
-		query := models.GetUserByIdQuery{Id: usr.ID}
-		err = ss.GetUserById(context.Background(), &query)
-		require.Nil(t, err)
-
-		require.Equal(t, query.Result.Email, "usertest@test.com")
-		require.Equal(t, query.Result.Password, "")
-		require.Len(t, query.Result.Rands, 10)
-		require.Len(t, query.Result.Salt, 10)
-		require.False(t, query.Result.IsDisabled)
-		require.Equal(t, query.Result.OrgID, orgCmd.Result.Id)
-
-		const nonExistingOrgID = 10000
-		cmd = user.CreateUserCommand{
-			Email: "usertest@test.com",
-			Name:  "user name",
-			Login: "user_test_login",
-			OrgID: nonExistingOrgID,
-		}
-
-		_, err = ss.CreateUser(context.Background(), cmd)
-		require.Equal(t, err, models.ErrOrgNotFound)
-	})
-
-	ss = InitTestDB(t)
-
-	t.Run("Testing DB - search users", func(t *testing.T) {
-		// Since previous tests were destructive
-		createFiveTestUsers(t, ss, func(i int) *user.CreateUserCommand {
-			return &user.CreateUserCommand{
-				Email:      fmt.Sprint("user", i, "@test.com"),
-				Name:       fmt.Sprint("user", i),
-				Login:      fmt.Sprint("loginuser", i),
-				IsDisabled: false,
-			}
-		})
-	})
-}
-
-func (ss *SQLStore) GetOrgUsersForTest(ctx context.Context, query *models.GetOrgUsersQuery) error {
-	return ss.WithDbSession(ctx, func(dbSess *DBSession) error {
-		query.Result = make([]*models.OrgUserDTO, 0)
-		sess := dbSess.Table("org_user")
-		sess.Join("LEFT ", ss.Dialect.Quote("user"), fmt.Sprintf("org_user.user_id=%s.id", ss.Dialect.Quote("user")))
-		sess.Where("org_user.org_id=?", query.OrgId)
-		sess.Cols("org_user.org_id", "org_user.user_id", "user.email", "user.login", "org_user.role")
-
-		err := sess.Find(&query.Result)
-		return err
-	})
-}
-
-func createFiveTestUsers(t *testing.T, sqlStore *SQLStore, fn func(i int) *user.CreateUserCommand) []user.User {
-	t.Helper()
-
-	users := []user.User{}
-	for i := 0; i < 5; i++ {
-		cmd := fn(i)
-
-		user, err := sqlStore.CreateUser(context.Background(), *cmd)
-		users = append(users, *user)
-
-		require.Nil(t, err)
-	}
-
-	return users
-}

pkg/services/user/userimpl/store_test.go

@@ -311,11 +311,11 @@ func TestIntegrationUserDataAccess(t *testing.T) {
 
 		require.Equal(t, user.ErrLastGrafanaAdmin, updatePermsError)
 
-		query := models.GetUserByIdQuery{Id: usr.ID}
+		query := user.GetUserByIDQuery{ID: usr.ID}
-		getUserError := ss.GetUserById(context.Background(), &query)
+		queryResult, getUserError := userStore.GetByID(context.Background(), query.ID)
 		require.Nil(t, getUserError)
 
-		require.True(t, query.Result.IsAdmin)
+		require.True(t, queryResult.IsAdmin)
 
 		// One user
 		const email = "user@test.com"

pkg/services/user/usertest/fake.go

@@ -13,8 +13,11 @@ type FakeUserService struct {
 	ExpectedSetUsingOrgError error
 	ExpectedSearchUsers      user.SearchUserQueryResult
 	ExpectedUserProfileDTO   *user.UserProfileDTO
+	ExpectedUserProfileDTOs  []*user.UserProfileDTO
 
 	GetSignedInUserFn func(ctx context.Context, query *user.GetSignedInUserQuery) (*user.SignedInUser, error)
+
+	counter int
 }
 
 func NewUserServiceFake() *FakeUserService {
@@ -92,5 +95,14 @@ func (f *FakeUserService) SetUserHelpFlag(ctx context.Context, cmd *user.SetUser
 }
 
 func (f *FakeUserService) GetProfile(ctx context.Context, query *user.GetUserProfileQuery) (*user.UserProfileDTO, error) {
-	return f.ExpectedUserProfileDTO, f.ExpectedError
+	if f.ExpectedUserProfileDTO != nil {
+		return f.ExpectedUserProfileDTO, f.ExpectedError
+	}
+
+	if f.ExpectedUserProfileDTOs == nil {
+		return nil, f.ExpectedError
+	}
+
+	f.counter++
+	return f.ExpectedUserProfileDTOs[f.counter-1], f.ExpectedError
 }