team 関連のAPIの実装 #20
team 関連のAPIの実装 #20
Conversation
There was a problem hiding this comment.
Copilot reviewed 8 out of 20 changed files in this pull request and generated no comments.
Files not reviewed (12)
- go.mod: Language not supported
- server/domain/team.go: Evaluated as low risk
- server/handler/handler.go: Evaluated as low risk
- server/handler/middleware.go: Evaluated as low risk
- server/handler/middleware_test.go: Evaluated as low risk
- server/usecase/error.go: Evaluated as low risk
- server/domain/user.go: Evaluated as low risk
- server/repository/repository.go: Evaluated as low risk
- server/handler/response.go: Evaluated as low risk
- server/handler/openapi/oas_parameters_gen.go: Evaluated as low risk
- server/repository/db/user.go: Evaluated as low risk
- server/repository/mock/repository.go: Evaluated as low risk
Comments suppressed due to low confidence (9)
server/usecase/team_test.go:27
- The test does not verify the expected behavior of the CreateTeam method. Consider adding assertions to check if the method is called with the correct parameters.
mockRepo.EXPECT().CreateTeam(gomock.Any(), gomock.Any()).AnyTimes()
server/usecase/team_test.go:95
- The test does not verify the expected behavior of the UpdateTeam method. Consider adding assertions to check if the method is called with the correct parameters.
mockRepo.EXPECT().UpdateTeam(gomock.Any(), gomock.Any()).AnyTimes()
server/usecase/team_test.go:34
- [nitpick] The field name 'wantErr' is ambiguous. It should be renamed to 'expectError' to indicate that an error is expected.
wantErr bool
server/usecase/team.go:41
- [nitpick] The field name CreatorID should be checked for consistency with the rest of the codebase. If camelCase is the convention, it should be renamed to creatorID.
CreatorID string
server/usecase/team.go:44
- The CreateTeam function should have corresponding unit tests to ensure the behavior is covered.
func (u *TeamUseCase) CreateTeam(ctx context.Context, input CreateTeamInput) (domain.Team, error) {
server/usecase/team.go:83
- The UpdateTeam function should have corresponding unit tests to ensure the behavior is covered.
func (u *TeamUseCase) UpdateTeam(ctx context.Context, input UpdateTeamInput) (domain.Team, error) {
server/handler/team.go:92
- Potential nil pointer dereference. Add a nil check for req.Name before accessing its Value property.
Name: string(req.Name.Value),
server/handler/openapi/oas_validators_gen.go:416
- The error message 'invalid value' is not very helpful. It should include the invalid value for better context.
return errors.Errorf("invalid value: %v", s)
server/handler/openapi/oas_validators_gen.go:485
- The error message 'array' is not descriptive. It should specify that the array length is not within the expected range.
return errors.Wrap(err, "array")
|
|
||
| import "errors" | ||
|
|
||
| type ErrBadRequest struct { |
There was a problem hiding this comment.
BadRequestはhttpサーバーとしての機能な気がするので、usecaseで処理するよりはhandler側で扱いたいです。
There was a problem hiding this comment.
handlerで行いたいvalidationとusecaseで行いたいvalidationがあるので分けてます。リクエストのフォーマットなどはhandlerで見て、ロジックに関する部分はusecase層で見ています。
BadRequestという名前がよくない気はするんですが、internal server error とそうでないもので分けるために設けてます。
There was a problem hiding this comment.
理由は分かりました:okk:
traQなどのように、ErrTooManyMembersのようなエラーの変数を定義する方が僕はいいと思います。理由としては、呼び出し側から見てどんな理由のエラーが来るのか分かって処理が追いやすいというのがあります。
team, err := h.teamUseCase.CreateTeam(...)
if err != nil {
if usecase.IsErrBadRequest(err) {
return badRequestResponse(c, err.Error())
}
return internalServerErrorResponse(c, err)
}よりも
team, err := h.teamUseCase.CreateTeam(...)
if err != nil {
if errors.Is(err, ErrCreatorNotInNewTeam) || errors.Is(err, ErrTooManyMembers) {
return badRequestResponse(c, err.Error())
}
return internalServerErrorResponse(c, err)
}の方が見通しがいい気がしませんか？
There was a problem hiding this comment.
もとの実装であれば、BadRequestじゃなくてUseCaseErrorはどうでしょう
| @@ -0,0 +1,112 @@ | |||
| package handler | |||
| c := echo.New().NewContext(req, rec) | ||
| c.SetParamNames("teamID") | ||
| c.SetParamValues("test-team-id") | ||
| c.Set("userID", userID) |
There was a problem hiding this comment.
こういうのを使ってテストのときだけexportすることもできる。やらなくてもいいとも思う
https://engineering.mercari.com/blog/entry/2018-08-08-080000/
server/repository/db/team_test.go
Outdated
| CreatedAt: time.Now(), | ||
| } | ||
| for _, member := range members { | ||
| if err := repo.CreateUser(context.Background(), member); err != nil { |
There was a problem hiding this comment.
ここがむずいんだけど、テスト対象じゃない関数を呼ぶのはどうなのかというのがある。例えばCreateUserが壊れた時にTestCreateTeamも落ちるようになるので、壊れた部分の特定が難しい。
There was a problem hiding this comment.
難しいですね... bobの関数をそのまま使ったmustCreateUserみたいなのを定義するのがよさそうですか？
やったこと
タイトル通り
handlerに全部書くとユニットテストが書きづらすぎたので、usecase層を作りました
やってないこと
github id の扱いについて、teamごとに設定できるようにしますが、管理は別でしようと今のところ考えているので、このprには含めていません
メモ
そろそろrepositoryが大きくなってきたので、分割してもいいかもです