-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix: 複数idを指定するusers/show
が関係ないユーザを返すことがある問題を修正
#13765
Conversation
nullable arrayにするかは悩みましたがAPIの仕様がsetではなくarrayっぽいので複数のユーザをまとめてアクセスするのを最適化するのに使えるように設計されてたのでnullable arrayにしました |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #13765 +/- ##
===========================================
- Coverage 66.46% 65.15% -1.31%
===========================================
Files 988 985 -3
Lines 116719 112117 -4602
Branches 4511 4422 -89
===========================================
- Hits 77572 73054 -4518
+ Misses 39116 39032 -84
Partials 31 31 ☔ View full report in Codecov by Sentry. |
このPRによるapi.jsonの差分 差分はこちら |
nullが入るとあらゆるところで判定が必要になるからnon-nullで良いと思った |
削除されてる等のときに別の処理したいことないですかね |
まぁidでマッチするようにすればいいからレスポンスのさぃず変えるようにしますか |
non-nullにしました |
pushVisibleUser(users[i]); | ||
} | ||
} | ||
users.forEach(pushVisibleUser); |
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.
関数そのまま渡すと型は同じまま引数変わった時にバグるわね
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.
このファイル内で同様の処理が数多くあったのでそれに揃えたのですがそれらも同時にu => pushVisibleUserに治すということですかね
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.
ついでにやっちゃってもらえると助かります🙏
ごめんなさい。修正点あるの忘れてました。修正しました。マージターゲット修正したほうがいいですか |
conflict resolved |
🙏 |
What
#13717 (comment) の修正です。
Why
Additional info (optional)
Checklist