-
Notifications
You must be signed in to change notification settings - Fork 1
同じ人が1つの合宿の複数の部屋に所属できないようにする #298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
akimon658
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
動作確認はしていませんが、良い感じだと思います!
追加で
- テストを追加してほしいです
repository/gormrepository/room_test.goに、以下を確認するケースを追加- 他の部屋に所属している人を新たに部屋に所属させようとしたとき、期待通りにエラーが出ること
- 他の合宿で部屋に所属していてもエラーにはならないこと
router/rooms_test.goに、repositoryがErrUserAlreadyAssignedを返したとき期待通りのエラーレスポンスが返ってくることを確かめるケースを追加
UpdateRoomは部屋の更新をするメソッドですが、部屋の作成時にもチェックが必要だと思います- 作成は
RoomGroupを作るときにまとめてできる関係でちょっと難しそうなので、重かったらこのPRではやらなくても良いです
- 作成は
大変だとは思うんだけど、テストの方は必ずやってほしいです 🙏 (手作業での動作確認には限界があるので、なるべく自動化したい)
ちなみにちなみに、コミットメッセージにissueの番号を含める必要はないです 👀
マージするときにPR内のコミットを1つにまとめるようにしていて、PRのタイトルが最終的なmainブランチのコミットメッセージになります。なのでコミットメッセージは適当でよくて、それよりもプルリクのタイトルをちゃんと書いてもらえるとありがたいです 🙇
|
知見の塊すぎるレビューありがとうございます....! |
akimon658
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
テストがError 1040: Too many connectionsというエラーで落ちてしまっているんだけど、これは既存のテストが悪そうなので気にしなくて良いです 🙇♂️
akimon658
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTMです 👍 お疲れさまでした!
No description provided.