-
-
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
feat(frontend): 今日誕生日のフォロー中のユーザーを一覧表示できるウィジェットを追加 #12450
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #12450 +/- ##
===========================================
- Coverage 78.72% 78.69% -0.04%
===========================================
Files 951 950 -1
Lines 103311 103208 -103
Branches 8301 8303 +2
===========================================
- Hits 81332 81217 -115
- Misses 21979 21991 +12 ☔ View full report in Codecov by Sentry. |
このPRによるapi.jsonの差分 差分はこちら--- base
+++ head
@@ -56888,6 +56888,10 @@
"type": "string",
"nullable": true,
"description": "The local host is represented with `null`."
+ },
+ "birthday": {
+ "type": "string",
+ "nullable": true
}
},
"anyOf": [
@@ -56950,6 +56954,15 @@
}
}
},
+ "BIRTHDAY_DATE_FORMAT_INVALID": {
+ "value": {
+ "error": {
+ "message": "Birthday date format is invalid.",
+ "code": "BIRTHDAY_DATE_FORMAT_INVALID",
+ "id": "a2b007b9-4782-4eba-abd3-93b05ed4130d"
+ }
+ }
+ },
"INVALID_PARAM": {
"value": {
"error": { |
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.
indexがないと重くなるらしいのでindexを立てたけどコレでいいのかわからん
const birthday = `____-${(d.getMonth() + 1).toString().padStart(2, '0')}-${d.getDate().toString().padStart(2, '0')}`; | ||
const birthdayUserQuery = this.userProfilesRepository.createQueryBuilder('user_profile'); | ||
birthdayUserQuery.select('user_profile.userId') | ||
.where(`user_profile.birthday LIKE '${birthday}'`); |
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.
怖め(typeormのパラメータがなぜか使えなかったので直接SQLに突っ込んだ)
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.
[ask]
why LIKE文? equal operator は =
特に理由がなければ完全一致のほうが望ましい
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.
データベースには誕生日(MM-DD)ではなく生年月日(YYYY-MM-DD)が入るから完全一致では検索できない
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.
あー、なるほど。
https://use-the-index-luke.com/ja/sql/where-clause/searching-for-ranges/like-performance-tuning
曰く、 LIKE分は前方にワイルドカードがくると一切インデックスが効かないらしい
ハックな方法では逆順に検索することで後方一致を前方一致に読み替えてインデックスを読み取る方法があるとか
https://blog.aleajactaest.org/article_7ehymh5nwnbkrg347nqmd33pfu.html
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.
YYYY-MM-DDではなくMM-DDを保存するカラム新たに作っても良さそうだけどマイグレが面倒そう
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.
ハックというか SUBSTR("birthday", 6, 5)
でインデックスすればいいのでは
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.
なおしましたどうでしょう
マイグレーションは作成されてるけどmodel定義の方にはインデックス設定が無いように見えるわね |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
useInterval(fetch, 1000 * 60, { | ||
immediate: true, | ||
afterMounted: true, | ||
}); |
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.
誕生日は1日に1回しか変わらないので60秒おきにfetchする理由はなさそう
タイムゾーンを考慮しても少なくとも1時間おきでよいのでは(半日とか1日おきでもよさそう。それか日付の変わり目に setTimeoutかな)
オンライン数分リクエストが飛ぶことを考えるとioでは7000リクエストぐらい一気にきそう
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.
これ日付変わるのを検知しないと実際にはfetchしないようになっています
やった |
👍👍 |
now.setHours(0, 0, 0, 0); | ||
|
||
if (now > lfAtD) { | ||
os.api('users/following', { | ||
limit: 18, | ||
birthday: now.toISOString(), |
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.
これタイムゾーン吹き飛ぶせいで UTC 以外(e.g. JST)だと一日ズレる
* (add) 今日誕生日のフォロイー一覧表示 * Update Changelog * Update Changelog * 実装漏れ * create index * (fix) index
What
users/following
で誕生日ごとにユーザーを絞り込めるようにWhy
Fix #12192
Additional info (optional)
セキュリティ面・パフォーマンス面が心配(とりあえず動く)
Checklist