docs/openapi/v2.yamlにbotを弾くクエリパラメータの記述を追加#1103
Conversation
PR Reviewer Guide 🔍(Review updated until commit 809d0b0)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 809d0b0
Previous suggestionsSuggestions up to commit edb7deb
|
ikura-hamu
left a comment
ikura-hamu
left a comment
There was a problem hiding this comment.
okです。task generateを実行するとコード生成が実行されるので、それをやってからこのブランチで開発を進めてください
|
users.goにクエリパラメータの取得部分を書きました。 |
|
v1/service/src/user.goにbotを弾くフィルタリングを実装しました |
ikura-hamu
left a comment
ikura-hamu
left a comment
There was a problem hiding this comment.
ありがとうございます。処理は問題ないと思います。1か所だけ抜けてそうなとこがあったので、そこは修正お願いします。
コンパイルエラーの部分はいろんな関数やinterfaceが書き換わった影響だと思うので、頑張って地道に直してください。interfaceを書き換えてるので、テストで使ってるモックを生成しなおすために task generate をするといいと思います。
| users = filteredUsers | ||
| return users, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
この下の userAuth.GetAllActiveUsers の結果も、botかどうかのフィルターをかけて返すようにしてください。キャッシュにセットする値はbotも含めていいと思います。
ikura-hamu
left a comment
ikura-hamu
left a comment
There was a problem hiding this comment.
ありがとうございます！大まかな流れは正しく実装できています。テストコードでできるだけ多くのケースを抜け漏れなく試せるよう意識してみてください。VSCodeであれば、コマンドパレットのGo: Toggle Test Ccoverage In Current Package でどれだけテストがカバーしているか可視化できて便利なので試してみてください
src/handler/v2/users.go
Outdated
| if botParam != "" { | ||
| includeBot, err = strconv.ParseBool(botParam) | ||
| if err != nil { | ||
| log.Printf("error: invalid query parameter 'bot': %v\n", err) |
There was a problem hiding this comment.
正常な処理とエラーの区別がつきにくくなるので、 ログを残すのはInternalServerErrorのときのみにしてほしいです。
src/handler/v2/users_test.go
Outdated
| req := httptest.NewRequest(http.MethodPost, "/users/me", nil) | ||
| rec := httptest.NewRecorder() | ||
| c := e.NewContext(req, rec) | ||
| bot := true |
There was a problem hiding this comment.
ここでbotを指定してしまうとbot=falseのときのテストができないと思うので、上の方のtest構造体にフィールドを足して、bot=trueとbot=falseの両方で正しく動くことを確認するようなテストケースを追加してください
src/service/v1/user.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get user info: %w", err) | ||
| } | ||
| if err == nil { |
There was a problem hiding this comment.
ここは上でerrがnilではないことが分かってるので、ifに入れなくても良いです。（Goではネストが深くなることを嫌う文化があります）
src/service/v1/user.go
Outdated
| // cacheから取り出した場合はそれを返す | ||
| if err == nil { | ||
| filteredUsers := make([]*service.UserInfo, 0, len(users)) | ||
| for _, user := range users { |
There was a problem hiding this comment.
ここと下のやつとでfilterする処理が同じなので、適当な関数に切り出しちゃってください
src/service/v1/user_test.go
Outdated
| values.NewOIDCAccessToken("access token"), | ||
| time.Now(), | ||
| ) | ||
| includeBot := true |
There was a problem hiding this comment.
ここでもテストケースでbotを含むパターンと含まないパターンの両方の動作を確認するようにしてください
src/service/v1/user.go
Outdated
| filteredUsers = append(filteredUsers, user) | ||
| } | ||
| users = filteredUsers | ||
| return users, nil |
There was a problem hiding this comment.
ここでreturnしちゃうとキャッシュへのセットが行われないので、テストが落ちています
|
春休み忙しくてとても遅くなってすみません:pray: |
ikura-hamu
left a comment
ikura-hamu
left a comment
There was a problem hiding this comment.
ありがとうございます、リファクタリングはいい感じです。まだバグがあるっぽいので、そこの修正をお願いします
src/service/v1/user.go
Outdated
| } | ||
| } | ||
|
|
||
| func FilteringUsers(users []*service.UserInfo, includeBot bool){ |
There was a problem hiding this comment.
この関数はパッケージの外部で使われてほしくないので、private(最初を小文字)にしてください。
あと、たぶんこの関数だとfilterがかかってないので、新しいスライスを返す形の関数にしてください。
https://go.dev/play/p/WumURkHqC5k
Goのスライスを引数に渡した時の挙動としては、参照が渡されるけど代入されると書き換わるぽいです
| values.TrapMemberStatusActive, | ||
| false, | ||
| ), | ||
| } |
There was a problem hiding this comment.
今はbot以外のユーザーしか想定したテストケースになっていないので、キャッシュから得られるユーザーやtraQから得られるユーザーにbotが含まれているパターンもあるとよいと思います。
|
関数がフィルタリング後の結果をreturnで返すようにしたのと、テストの追加をやりました |
|
最新のmainブランチをこのブランチにマージして、コード生成をもう一回かけるといけると思います |
|
Persistent review updated to latest commit 809d0b0 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1103 +/- ##
==========================================
+ Coverage 49.74% 49.78% +0.04%
==========================================
Files 123 123
Lines 11118 11157 +39
==========================================
+ Hits 5531 5555 +24
- Misses 5290 5304 +14
- Partials 297 298 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
User description
タイトルの通りです。
他の部分を参考に見よう見まねで書きました。
PR Type
Description
ボット除外パラメータを追加
ユーザフィルタリングロジック更新
テストケースとモック更新対応
Goバージョン/Docker依存修正
Changes walkthrough 📝
5 files
GetUsersハンドラにbotクエリパラメータ実装GetAllActiveUser関数にincludeBot引数追加UserInfoコンストラクタにbotフラグ導入Goバージョン/Dockerイメージを1.23系に調整モックパッケージimportをgithub.com/golang/mockへ更新1 files
OpenAPI定義にbotパラメータ記述＆operationId修正1 files
各テストケースにbotフラグfalseを設定96 files