Conversation
… unique or not while registering
db/db.go
Outdated
|
|
||
| type Storer interface { | ||
| ListUsers(context.Context) ([]User, error) | ||
| CreateNewUser(context.Context, User) (User, error) |
db/user.go
Outdated
|
|
||
| // CreateNewUser = creates a new user in database | ||
| func (s *pgStore) CreateNewUser(ctx context.Context, u User) (newUser User, err error) { | ||
| tx, err := s.db.Beginx() |
There was a problem hiding this comment.
no need to use transaction. we can use s.db to run the query
There was a problem hiding this comment.
Removed transaction and instead used Exec
db/db.go
Outdated
| type Storer interface { | ||
| ListUsers(context.Context) ([]User, error) | ||
| CreateNewUser(context.Context, User) (User, error) | ||
| CheckUserByEmail(context.Context, string) (bool, User, error) |
There was a problem hiding this comment.
i think CheckUserByEmail(context.Context, string) (bool, User, error) is quite ambiguous.
if error occurred then ideally bool value should be true &
if error not occurred then bool value should be false
In CheckUserByEmail method we are using bool & err variable to denote if user is present or not, which i think is confusing
So, instead
Rename the method
From:- CheckUserByEmail(context.Context, string) (bool, User, error)
To :- GetUserByEmail(context.Context, string) (User, error)
so in GetUserByEmail method
if err == sql.NoRows then user is not present &
if err != nil && err != sql.NoRows then db error
| func (s *pgStore) CheckUserByEmail(ctx context.Context, email string) (check bool, user User, err error) { | ||
| err = s.db.Get(&user, getUserByEmailQuery, email) | ||
| if err != nil { | ||
| if err == sql.ErrNoRows { |
There was a problem hiding this comment.
no need to add
if err == sql.ErrNoRows condition in this func
caller of the func should handle it
There was a problem hiding this comment.
I have used if err == sql.ErrNoRows condition to avoid logging no row found errors and to log only database related errors
service/user_http_test.go
Outdated
| } | ||
|
|
||
| func (suite *UsersHandlerTestSuite) TestRegisterUserSuccess() { | ||
| suite.dbMock.On("CreateNewUser", mock.Anything, mock.Anything).Return(db.User{}, nil) |
There was a problem hiding this comment.
avoid using mock.Anything instead use actual User object
for context you can use mock.Anything
There was a problem hiding this comment.
If we don't use mock.Anything for user then we have to manually fill the user object
db/db.go
Outdated
| type Storer interface { | ||
| ListUsers(context.Context) ([]User, error) | ||
| CreateUser(context.Context, User) (User, error) | ||
| // CheckUserByEmail(context.Context, string) (bool, User, error) |
service/user_http.go
Outdated
| // For checking if user already registered | ||
| _, err = deps.Store.GetUserByEmail(req.Context(), user.Email) | ||
|
|
||
| // If check true then user is already registered |
service/user_http.go
Outdated
| _, err = deps.Store.GetUserByEmail(req.Context(), user.Email) | ||
|
|
||
| // If check true then user is already registered | ||
| if err != sql.ErrNoRows && err == nil { |
service/user_http_test.go
Outdated
| } | ||
|
|
||
| func (suite *UsersHandlerTestSuite) TestRegisterUserFailure() { | ||
| suite.dbMock.On("CreateUser", mock.Anything, mock.Anything).Return(db.User{}, nil) |
There was a problem hiding this comment.
avoid using mock.Anything.
you can you mock.Anything for context.Context
| logger.WithField("err", err.Error()).Error("Error while preparing user insert query") | ||
| return | ||
| } | ||
| _, err = stmt.Exec(u) |
2 participants
No description provided.