Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughユーザーが参加する通常イベント一覧を追加。コントローラ、ビュー（タブナビ含む）、ルーティング、ヘルパー修正、およびシステムテストの追加・更新を行っています。（50語以内） Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant Router as Router
participant Controller as Users::RegularEventsController
participant DB as DB
participant View as ViewRenderer
Browser->>Router: GET /users/:user_id/regular_events
Router->>Controller: dispatch index
Controller->>DB: User.find(params[:user_id])
Controller->>DB: user.participate_regular_events.includes(:comments, :users, :user).order(start_at: :desc).page(params[:page])
DB-->>Controller: regular events
Controller->>View: render index with `@user` and `@events`
View-->>Browser: HTML (tabs + list + pagination)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
y-kawahara-gs
force-pushed
the
feature/display-regular-events-on-user-page
branch
3 times, most recently
from
ba94dd1 to
85d09bb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
app/views/users/events/_nav.html.slim (1)
4-10: イベント種別ナビは共通partial化を推奨します。
app/views/users/regular_events/_nav.html.slimと実装が重複しているため、1つに寄せると保守性が上がります。💡 方向性（例）
- = render 'nav' + = render 'users/events/event_type_nav', user: `@user`, active_id: 'events'- = render 'nav' + = render 'users/events/event_type_nav', user: `@user`, active_id: 'regular_events'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/users/events/_nav.html.slim` around lines 4 - 10, Extract the duplicated event-nav markup into a single shared partial (e.g., _events_nav.html.slim) and replace both app/views/users/events/_nav.html.slim and app/views/users/regular_events/_nav.html.slim with a render of that partial; move the navs array and the li + link_to logic into the partial, make it parameterized via locals (pass in user as `@user` and compute active id or pass current_path_id) so the conditional using request.path.split('/').last still works, and update callers to render with render 'events_nav', user: `@user`, current_id: 'events' (or compute in caller) so counts (user.participate_events.length, user.participate_regular_events.length) and paths (user_events_path(user), user_regular_events_path(user)) are supplied by the caller or assembled inside the partial using the passed user.test/system/user/events_test.rb (1)
18-18: テスト名の英語文法を整えると読みやすくなります。Line 18 は
does not show ...の形にすると意図が明確です。💡 修正案
- test 'does not shows events the user is not participating in' do + test 'does not show events the user is not participating in' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system/user/events_test.rb` at line 18, Rename the failing test description string in the test method starting with test 'does not shows events the user is not participating in' to use correct English: change "does not shows events the user is not participating in" to "does not show events the user is not participating in" (update the test declaration in tests under the same method in events_test.rb and any matching test name occurrences such as in assertions or test output expectations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/users/regular_events_controller.rb`:
- Around line 6-8: The query assigning `@regular_events` via
`@user.participate_regular_events` currently only includes(:comments, :users)
causing N+1 when the view calls event.user and event.organizers; update the
includes on the `@user.participate_regular_events` call to also preload :user and
:organizers (or the exact association name for organizers) so event.user and
event.organizers are eager-loaded and eliminate the extra queries.
In `@app/helpers/page_tabs/users_helper.rb`:
- Line 16: Replace uses of .length on ActiveRecord associations in this helper
with .count to avoid loading full record sets; specifically change expressions
like user.participate_events.length and user.participate_regular_events.length
(used in the tabs construction where user_events_path(user) is added) to use
.count, and update all other .length occurrences in this method (the ones noted
on lines 6, 11, 13, 14, 15, 16, 20) so every association-based length call uses
.count instead.
In `@app/views/users/regular_events/index.html.slim`:
- Around line 57-59: Remove the empty .a-meta element to avoid rendering an
unnecessary DOM node: either delete the bare .a-meta line or move its intended
content into it and render it conditionally (e.g., inside the existing if
event.comments.size.positive? block); specifically update the template around
the .card-list-item-meta__item and the .a-meta lines so .a-meta is only present
when it contains content (refer to the .a-meta element and the if
event.comments.size.positive? conditional).
In `@test/system/user/regular_events_test.rb`:
- Around line 18-22: Rename the failing test title string in the test method
inside test/system/user/regular_events_test.rb: change "does not shows regular
events the user is not participating in" to the grammatically correct "does not
show regular events the user is not participating in" (update the test '...'
declaration in the test block that visits
"/users/#{users(:kimura).id}/regular_events" and asserts presence/absence of
event texts).
---
Nitpick comments:
In `@app/views/users/events/_nav.html.slim`:
- Around line 4-10: Extract the duplicated event-nav markup into a single shared
partial (e.g., _events_nav.html.slim) and replace both
app/views/users/events/_nav.html.slim and
app/views/users/regular_events/_nav.html.slim with a render of that partial;
move the navs array and the li + link_to logic into the partial, make it
parameterized via locals (pass in user as `@user` and compute active id or pass
current_path_id) so the conditional using request.path.split('/').last still
works, and update callers to render with render 'events_nav', user: `@user`,
current_id: 'events' (or compute in caller) so counts
(user.participate_events.length, user.participate_regular_events.length) and
paths (user_events_path(user), user_regular_events_path(user)) are supplied by
the caller or assembled inside the partial using the passed user.
In `@test/system/user/events_test.rb`:
- Line 18: Rename the failing test description string in the test method
starting with test 'does not shows events the user is not participating in' to
use correct English: change "does not shows events the user is not participating
in" to "does not show events the user is not participating in" (update the test
declaration in tests under the same method in events_test.rb and any matching
test name occurrences such as in assertions or test output expectations).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e5e2f2e-2659-4688-9e17-f5acc41a5c63
📒 Files selected for processing (9)
app/controllers/users/regular_events_controller.rbapp/helpers/page_tabs/users_helper.rbapp/views/users/events/_nav.html.slimapp/views/users/events/index.html.slimapp/views/users/regular_events/_nav.html.slimapp/views/users/regular_events/index.html.slimconfig/routes/users.rbtest/system/user/events_test.rbtest/system/user/regular_events_test.rb
y-kawahara-gs
force-pushed
the
feature/display-regular-events-on-user-page
branch
from
220ec87 to
20efd69
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/system/user/events_test.rb`:
- Line 18: Rename the test name string in the test case from "does not shows
events the user is not participating in" to the grammatically correct "does not
show events the user is not participating in" by updating the test declaration
in test 'does not shows events the user is not participating in' to test 'does
not show events the user is not participating in' so the test description reads
correctly.
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1ec716e2-7f70-40c4-9a3d-e978afd15b78
📒 Files selected for processing (9)
app/controllers/users/regular_events_controller.rbapp/helpers/page_tabs/users_helper.rbapp/views/users/events/_nav.html.slimapp/views/users/events/index.html.slimapp/views/users/regular_events/_nav.html.slimapp/views/users/regular_events/index.html.slimconfig/routes/users.rbtest/system/user/events_test.rbtest/system/user/regular_events_test.rb
🚧 Files skipped from review as they are similar to previous changes (6)
- app/views/users/regular_events/_nav.html.slim
- app/views/users/events/index.html.slim
- test/system/user/regular_events_test.rb
- app/helpers/page_tabs/users_helper.rb
- app/views/users/events/_nav.html.slim
- app/views/users/regular_events/index.html.slim
y-kawahara-gs
force-pushed
the
feature/display-regular-events-on-user-page
branch
from
20efd69 to
55c9fad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/system/user/events_test.rb (1)
18-18:⚠️ Potential issue | 🟡 Minorテスト名の文法を修正してください。
does not showsは不自然なのでdoes not showにしてください。✏️ 修正案
- test 'does not shows events the user is not participating in' do + test 'does not show events the user is not participating in' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system/user/events_test.rb` at line 18, テスト名の文法を修正してください: テスト定義の文字列 "does not shows events the user is not participating in" を "does not show events the user is not participating in" に変更してください（該当のテスト宣言はテストメソッドの定義行 test 'does not shows events the user is not participating in' を探して修正してください）。
🧹 Nitpick comments (1)
test/system/user/regular_events_test.rb (1)
6-6: テスト名末尾の不要スペースは削除しておくとよいです。✏️ 修正案
- test 'shows the regular event listing for a user ' do + test 'shows the regular event listing for a user' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system/user/regular_events_test.rb` at line 6, The test declaration "test 'shows the regular event listing for a user ' do" contains an extraneous trailing space in the test name; remove the trailing space inside the string so the test reads "test 'shows the regular event listing for a user' do" to keep test names clean and avoid accidental mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/views/users/regular_events/index.html.slim`:
- Around line 44-54: The template is passing an Organizer object into the
users/icon partial causing display mismatch; change the render call to pass a
User instead (e.g. inside the loop over event.organizers, pass organizer.user to
users/icon) or iterate event.users and pass each user to the partial; update the
render invocation (currently in the loop that calls render 'users/icon', user:
organizer, link_class: ..., image_class: ...) so the `user:` argument is a User
instance (organizer.user or item from event.users) and keep the same link_class
and image_class props.
---
Duplicate comments:
In `@test/system/user/events_test.rb`:
- Line 18: テスト名の文法を修正してください: テスト定義の文字列 "does not shows events the user is not
participating in" を "does not show events the user is not participating in"
に変更してください（該当のテスト宣言はテストメソッドの定義行 test 'does not shows events the user is not
participating in' を探して修正してください）。
---
Nitpick comments:
In `@test/system/user/regular_events_test.rb`:
- Line 6: The test declaration "test 'shows the regular event listing for a user
' do" contains an extraneous trailing space in the test name; remove the
trailing space inside the string so the test reads "test 'shows the regular
event listing for a user' do" to keep test names clean and avoid accidental
mismatches.
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a308cc5-663a-4ecb-9188-fe6ee4290e2d
📒 Files selected for processing (9)
app/controllers/users/regular_events_controller.rbapp/helpers/page_tabs/users_helper.rbapp/views/users/events/_nav.html.slimapp/views/users/events/index.html.slimapp/views/users/regular_events/_nav.html.slimapp/views/users/regular_events/index.html.slimconfig/routes/users.rbtest/system/user/events_test.rbtest/system/user/regular_events_test.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- app/views/users/regular_events/_nav.html.slim
- config/routes/users.rb
- app/views/users/events/_nav.html.slim
y-kawahara-gs
force-pushed
the
feature/display-regular-events-on-user-page
branch
from
55c9fad to
dbd4313
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/system/user/regular_events_test.rb (1)
6-6: テスト名末尾の不要な空白は削除しておくと読みやすいです。機能影響はありませんが、将来の検索性のために整えておくのがよいです。
💡 修正案
- test 'shows the regular event listing for a user ' do + test 'shows the regular event listing for a user' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system/user/regular_events_test.rb` at line 6, Remove the trailing space at the end of the test name string in the test definition in regular_events_test.rb (the line starting with test 'shows the regular event listing for a user '). Edit the test declaration so the human-readable name has no trailing whitespace (i.e., "shows the regular event listing for a user") to improve readability and future searchability.app/views/users/events/_nav.html.slim (1)
5-6: 件数表示でlengthを使うと全件ロードが発生します。Line 5 と Line 6 は件数表示用途なので、
lengthではなくcountを使ってください。
（同じ実装がapp/views/users/regular_events/_nav.html.slimにもあります）💡 修正案
- { id: 'events', name: Event.model_name.human, count: `@user.participate_events.length`, path: user_events_path(`@user`) }, \ - { id: 'regular_events', name: RegularEvent.model_name.human, count: `@user.participate_regular_events.length`, path: user_regular_events_path(`@user`) } \ + { id: 'events', name: Event.model_name.human, count: `@user.participate_events.count`, path: user_events_path(`@user`) }, \ + { id: 'regular_events', name: RegularEvent.model_name.human, count: `@user.participate_regular_events.count`, path: user_regular_events_path(`@user`) } \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/users/events/_nav.html.slim` around lines 5 - 6, 件数表示で `@user.participate_events.length` と `@user.participate_regular_events.length` が呼ばれているため全件がメモリにロードされます。ビューの該当箇所（参照されている symbols: Event.model_name.human, `@user.participate_events`, user_events_path, RegularEvent.model_name.human, `@user.participate_regular_events`, user_regular_events_path）で .length を .count に置き換え、DBの COUNT を使うようにしてください（同様の修正を app/views/users/regular_events/_nav.html.slim にも適用）。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/views/users/regular_events/index.html.slim`:
- Around line 50-54: The organizer avatar N+1 is caused by rendering users/icon
in the view which triggers user.avatar_url lazy loads; update
Users::RegularEventsController#index to eager load avatars by preloading users
with avatar_attachment and blob (e.g. include users: [avatar_attachment: :blob])
so organizer_user.avatar_url does not issue per-user DB/ActiveStorage queries.
---
Nitpick comments:
In `@app/views/users/events/_nav.html.slim`:
- Around line 5-6: 件数表示で `@user.participate_events.length` と
`@user.participate_regular_events.length` が呼ばれているため全件がメモリにロードされます。ビューの該当箇所（参照されている
symbols: Event.model_name.human, `@user.participate_events`, user_events_path,
RegularEvent.model_name.human, `@user.participate_regular_events`,
user_regular_events_path）で .length を .count に置き換え、DBの COUNT を使うようにしてください（同様の修正を
app/views/users/regular_events/_nav.html.slim にも適用）。
In `@test/system/user/regular_events_test.rb`:
- Line 6: Remove the trailing space at the end of the test name string in the
test definition in regular_events_test.rb (the line starting with test 'shows
the regular event listing for a user '). Edit the test declaration so the
human-readable name has no trailing whitespace (i.e., "shows the regular event
listing for a user") to improve readability and future searchability.
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b961031-a441-441b-841f-60e81b04fefe
📒 Files selected for processing (9)
app/controllers/users/regular_events_controller.rbapp/helpers/page_tabs/users_helper.rbapp/views/users/events/_nav.html.slimapp/views/users/events/index.html.slimapp/views/users/regular_events/_nav.html.slimapp/views/users/regular_events/index.html.slimconfig/routes/users.rbtest/system/user/events_test.rbtest/system/user/regular_events_test.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- app/controllers/users/regular_events_controller.rb
- app/views/users/events/index.html.slim
- app/helpers/page_tabs/users_helper.rb
y-kawahara-gs
force-pushed
the
feature/display-regular-events-on-user-page
branch
from
dbd4313 to
4ffef9a
Compare
|
@yokomaru さん |
|
@y-kawahara-gs 承知いたしました！ |
yokomaru
left a comment
yokomaru
left a comment
There was a problem hiding this comment.
@y-kawahara-gs
お待たせいたしました！
レビューいたしましたのでご確認よろしくお願いいたします🙏
| - navs = [\ | ||
| { id: 'events', name: Event.model_name.human, count: @user.participate_events.length, path: user_events_path(@user) }, \ | ||
| { id: 'regular_events', name: RegularEvent.model_name.human, count: @user.participate_regular_events.length, path: user_regular_events_path(@user) } \ | ||
| ] |
There was a problem hiding this comment.
navsの組み立てがviewに入っているので、このあたりはhelperに切り出すと見通しがよくなりそうかなと思いました💡
There was a problem hiding this comment.
helperに切り出した方がDRYにかけますね！修正しました
コミット：533e19c
| ] | ||
| - navs.each do |nav| | ||
| li.tab-nav__item | ||
| = link_to "#{nav[:name]}(#{nav[:count]})", nav[:path], class: (request.path.split('/').last == nav[:id] ? ['is-active'] : []) << 'tab-nav__item-link' |
There was a problem hiding this comment.
is_activeの判定はcurrent_page?(nav[:path])を使うとシンプルに書けそう && navsのidが不要になりそうです！
参考: https://railsguides.jp/action_view_helpers.html#current-page-questionmark
また、1行に判定ロジックと描画が混ざると何をしているのかパッと何をしているのか見て分かりづらくなってしまいがちなので、クラスの組み立てを分離してもいいかもと思いました！💡
(クラスの組み立て自体をhelperに切り出しても良いかもしれませんがお任せします！)
| = link_to "#{nav[:name]}(#{nav[:count]})", nav[:path], class: (request.path.split('/').last == nav[:id] ? ['is-active'] : []) << 'tab-nav__item-link' | |
| - navs.each do |nav| | |
| li.tab-nav__item | |
| - class = ['tab-nav__item-link'] | |
| - class += 'is-active' if current_page?(nav[:path]) | |
| = link_to "#{nav[:name]}(#{nav[:count]})", nav[:path], class: class |
There was a problem hiding this comment.
current_page?助かります🙇♂️
判定ロジックと描画など、１つ１つ処理は細かく分けるべきでした！
修正しております
コミット：1feb017
| nav.tab-nav | ||
| .container | ||
| ul.tab-nav__items | ||
| - navs = [\ | ||
| { id: 'events', name: Event.model_name.human, count: @user.participate_events.length, path: user_events_path(@user) }, \ | ||
| { id: 'regular_events', name: RegularEvent.model_name.human, count: @user.participate_regular_events.length, path: user_regular_events_path(@user) } \ | ||
| ] | ||
| - navs.each do |nav| | ||
| li.tab-nav__item | ||
| = link_to "#{nav[:name]}(#{nav[:count]})", nav[:path], class: (request.path.split('/').last == nav[:id] ? ['is-active'] : []) << 'tab-nav__item-link' |
There was a problem hiding this comment.
app/views/users/events/_nav.html.slimと同じ構成に見えたので、users/event_navのように共通化しておくと今後のメンテナンスもしやすくなるかなと思いました！
| @regular_events = @user.participate_regular_events | ||
| .includes(:comments, :user, | ||
| { users: { avatar_attachment: :blob } }) | ||
| .order(start_at: :desc) |
There was a problem hiding this comment.
定期イベントページはstart_atの降順で並べていると思うのですが、
start_atはイベントの開始時間なので、「開始時間が遅い順に並べたい」という意図であれば、
想定通りの並びになっていないかもしれないと思いました👀(本来であれば先頭が21:00 になるかなと思いました！)
bootcamp(dev):025> User.find("991528156").participate_regular_events.order(start_at: :desc).pluck(:id, :title,:start_at)
User Load (0.8ms) SELECT "users".* FROM "users" WHERE "users"."id" = 991528156 LIMIT 1 /*application='Bootcamp'*/
RegularEvent Pluck (4.3ms) SELECT "regular_events"."id", "regular_events"."title", "regular_events"."start_at" FROM "regular_events" INNER JOIN "regular_event_participations" ON "regular_events"."id" = "regular_event_participations"."regular_event_id" WHERE "regular_event_participations"."user_id" = 991528156 ORDER BY "regular_events"."start_at" DESC /*application='Bootcamp'*/
=>
[[1004488907, "test 朝開催", 2000-01-01 07:00:00.000000000 JST +09:00],
[1004488908, "test 朝開催 終了済み", 2000-01-01 07:00:00.000000000 JST +09:00],
[582552593, "定期イベント8", 2000-01-01 21:00:00.000000000 JST +09:00],
[364788892, "定期イベント9", 2000-01-01 21:00:00.000000000 JST +09:00],
[1004488906, "Everyday Rails輪読会", 2000-01-01 17:00:00.000000000 JST +09:00],
[40690962, "質問・雑談タイム", 2000-01-01 16:00:00.000000000 JST +09:00]]おそらくですが、DBとアプリ側でタイムゾーンが異なっているためDB上でソートした結果と表示が不整合になっている可能性が高いかなと思いました。
参考: https://techracho.bpsinc.jp/yusiro/2024_12_18/147469
DB側でのorderはせずにrails側でsortしなおしたりすれば解消するかなと思うのですが、
そもそも開始時間順で問題ないか(終了済みなどが混合する形になって問題ないか)
machidaさんに確認の上ご対応いただければと思います！🙏
There was a problem hiding this comment.
machidaさん、komagataさんと仕様のすり合わせを行い、開催しているイベントと終了を分け、create_atで並び替えて新しく作成されたものが上に来るような順番にしました！
コミット：a2c48ba
|
|
||
| test 'shows only events the user is participating in' do | ||
| test 'shows correct count of user participating events' do | ||
| visit_with_auth "/users/#{users(:kimura).id}/events", 'kimura' |
There was a problem hiding this comment.
[NITS]
他のところと合わせていただいていると思うのでこのままで問題ないのですが、パスヘルパーを使用しても書くことが可能です💡
visit_with_auth user_events_path(users(:kimura)), 'kimura'There was a problem hiding this comment.
ですね、ここはパスヘルパーを使えるのですが、他の部分に合わせてこのままでいきたいと思ってます！
| image_class: 'card-list-item__user-icon' | ||
| .card-list-item-meta__item | ||
| time.a-meta(datetime="#{event.start_at}") | ||
| | 開催日時:#{event.holding_cycles} #{event.start_at.strftime('%H:%M')} ～ #{event.end_at.strftime('%H:%M')} |
There was a problem hiding this comment.
%H:%Mのフォーマットはi18n側ですでに定義されているので、以下のように指定するとDRYに書けると思いました💡
| | 開催日時:#{event.holding_cycles} #{event.start_at.strftime('%H:%M')} ～ #{event.end_at.strftime('%H:%M')} | |
| | 開催日時:#{event.holding_cycles} #{l event.start_at, format: :time_only)} ～ #{l event.end_at, format: :time_only} |
There was a problem hiding this comment.
ありがとうございます！ config/ja.ymlにて確認できました！
コミット：7550dc9
| @user = User.find(params[:user_id]) | ||
| @regular_events = @user.participate_regular_events | ||
| .includes(:comments, :user, | ||
| { users: { avatar_attachment: :blob } }) |
There was a problem hiding this comment.
controller側でavatar_attachment: :blobまで含めてincludesする書き方を知らなかったので、勉強になりました！🙌
| def index | ||
| @user = User.find(params[:user_id]) | ||
| @regular_events = @user.participate_regular_events | ||
| .includes(:comments, :user, |
There was a problem hiding this comment.
view側で呼んでいるヘルパーメソッドevent.holding_cycles内でregular_event_repeat_rulesをeachしているため、N+1 が起きていそうでした。includesに追加いただくと解消できるかと思います！
また、includesまわりについては、他の方のPRのこちらのコメントがとても参考になると思うので、必要に応じて目を通していただけるとよさそうです💡
ちなみに、bootcampアプリにはN+1を検知できるBulletというgemも入っているので、必要であれば設定をオンにして確認してみるのもよいかもしれません(ご存知でしたらすみません🙏)。
リポジトリ、使い方
There was a problem hiding this comment.
Bullet使ったことなかったので助かりました🙇♂️
N+１見落としておりました。
regular_event_repeat_rulesをincludesに入れまとめて取得することで、毎回クエリ発行をしない処理に修正してます。
コミット：ad211da
| .card-list-item__row | ||
| .card-list-item-meta | ||
| .card-list-item-meta__items | ||
| - if event.users.count.positive? |
There was a problem hiding this comment.
個人的に、ここはsizeを使う方が良いのかなと思いました！
countは都度SQLを発行して件数を取得しているためcontroller側でincludes(:users)していてもeachの中で使用するとイベントの数だけクエリが発行されて N+1になってしまう可能性があるというのが理由です💡
（length、size、count は状況によって使い分けると良いようなので、確認の上で count の方が適していそうでしたらそのままでも問題ないと思います🙏）
参考:
https://dev.to/yutakusuno/rails-count-vs-length-vs-size-with-samples-and-a-diagram-37be
There was a problem hiding this comment.
どれも数を数えるメソッドですが、違いをイマイチ意識できてませんでした。
count：毎回クエリ実行
length：ロードされている場合、クエリを作成せずに数を返す。ロードされていない場合は、全てロードしてから数を返す。
size：ロードされている場合、オブジェクトの数を返す。ロードされていない場合、毎回クエリを実行する。
この場合、sizeの方がcountとlengthの利点を活かせるのでいいと思ったため、修正してます。
コミット：d11d29b
test/system/user/events_test.rb
Outdated
| test 'shows only events the user is participating in' do | ||
| test 'shows correct count of user participating events' do | ||
| visit_with_auth "/users/#{users(:kimura).id}/events", 'kimura' | ||
| assert_text "特別イベント(#{users(:kimura).participate_events.count})" |
There was a problem hiding this comment.
[NITS]
assert_textだとページ全体から検索するので、仮に特別イベント(2)などのテストデータができた場合テストが通ってしまう可能性があるため、セレクターまで指定してあげたほうがテストとして堅牢かな？と思いました💡
※ 他のテストでも assert_text が使われているようなので、今回はこちらのままでも問題ないかな？と思っています🙆
| assert_text "特別イベント(#{users(:kimura).participate_events.count})" | |
| assert_selector '.tab-nav__item-link', text: "特別イベント(#{users(:kimura).participate_events.count})" |
There was a problem hiding this comment.
特別イベント(2)などのテストデータができた場合テストが通ってしまう可能性があるため
ですね、ここはテキストではなく、セレクターを指定してアサーションを確かめた方が堅牢ですね！修正してます。
コミット：80d3773
y-kawahara-gs
force-pushed
the
feature/display-regular-events-on-user-page
branch
5 times, most recently
from
a850a5e to
9e8df37
Compare
|
@yokomaru |
y-kawahara-gs
force-pushed
the
feature/display-regular-events-on-user-page
branch
from
9e8df37 to
80d3773
Compare
y-kawahara-gs
left a comment
y-kawahara-gs
left a comment
There was a problem hiding this comment.
@yokomaru
お待たせしました！
レビュー対応完了したので確認お願いします🙇♂️
| .card-list-item__row | ||
| .card-list-item-meta | ||
| .card-list-item-meta__items | ||
| - if event.users.count.positive? |
There was a problem hiding this comment.
どれも数を数えるメソッドですが、違いをイマイチ意識できてませんでした。
count：毎回クエリ実行
length：ロードされている場合、クエリを作成せずに数を返す。ロードされていない場合は、全てロードしてから数を返す。
size：ロードされている場合、オブジェクトの数を返す。ロードされていない場合、毎回クエリを実行する。
この場合、sizeの方がcountとlengthの利点を活かせるのでいいと思ったため、修正してます。
コミット：d11d29b
| - navs = [\ | ||
| { id: 'events', name: Event.model_name.human, count: @user.participate_events.length, path: user_events_path(@user) }, \ | ||
| { id: 'regular_events', name: RegularEvent.model_name.human, count: @user.participate_regular_events.length, path: user_regular_events_path(@user) } \ | ||
| ] |
There was a problem hiding this comment.
helperに切り出した方がDRYにかけますね！修正しました
コミット：533e19c
| ] | ||
| - navs.each do |nav| | ||
| li.tab-nav__item | ||
| = link_to "#{nav[:name]}(#{nav[:count]})", nav[:path], class: (request.path.split('/').last == nav[:id] ? ['is-active'] : []) << 'tab-nav__item-link' |
There was a problem hiding this comment.
current_page?助かります🙇♂️
判定ロジックと描画など、１つ１つ処理は細かく分けるべきでした！
修正しております
コミット：1feb017
| nav.tab-nav | ||
| .container | ||
| ul.tab-nav__items | ||
| - navs = [\ | ||
| { id: 'events', name: Event.model_name.human, count: @user.participate_events.length, path: user_events_path(@user) }, \ | ||
| { id: 'regular_events', name: RegularEvent.model_name.human, count: @user.participate_regular_events.length, path: user_regular_events_path(@user) } \ | ||
| ] | ||
| - navs.each do |nav| | ||
| li.tab-nav__item | ||
| = link_to "#{nav[:name]}(#{nav[:count]})", nav[:path], class: (request.path.split('/').last == nav[:id] ? ['is-active'] : []) << 'tab-nav__item-link' |
| def index | ||
| @user = User.find(params[:user_id]) | ||
| @regular_events = @user.participate_regular_events | ||
| .includes(:comments, :user, |
There was a problem hiding this comment.
Bullet使ったことなかったので助かりました🙇♂️
N+１見落としておりました。
regular_event_repeat_rulesをincludesに入れまとめて取得することで、毎回クエリ発行をしない処理に修正してます。
コミット：ad211da
| image_class: 'card-list-item__user-icon' | ||
| .card-list-item-meta__item | ||
| time.a-meta(datetime="#{event.start_at}") | ||
| | 開催日時:#{event.holding_cycles} #{event.start_at.strftime('%H:%M')} ～ #{event.end_at.strftime('%H:%M')} |
There was a problem hiding this comment.
ありがとうございます！ config/ja.ymlにて確認できました！
コミット：7550dc9
test/system/user/events_test.rb
Outdated
| test 'shows only events the user is participating in' do | ||
| test 'shows correct count of user participating events' do | ||
| visit_with_auth "/users/#{users(:kimura).id}/events", 'kimura' | ||
| assert_text "特別イベント(#{users(:kimura).participate_events.count})" |
There was a problem hiding this comment.
特別イベント(2)などのテストデータができた場合テストが通ってしまう可能性があるため
ですね、ここはテキストではなく、セレクターを指定してアサーションを確かめた方が堅牢ですね！修正してます。
コミット：80d3773
|
|
||
| test 'shows only events the user is participating in' do | ||
| test 'shows correct count of user participating events' do | ||
| visit_with_auth "/users/#{users(:kimura).id}/events", 'kimura' |
There was a problem hiding this comment.
ですね、ここはパスヘルパーを使えるのですが、他の部分に合わせてこのままでいきたいと思ってます！
| @regular_events = @user.participate_regular_events | ||
| .includes(:comments, :user, | ||
| { users: { avatar_attachment: :blob } }) | ||
| .order(start_at: :desc) |
There was a problem hiding this comment.
machidaさん、komagataさんと仕様のすり合わせを行い、開催しているイベントと終了を分け、create_atで並び替えて新しく作成されたものが上に来るような順番にしました！
コミット：a2c48ba
There was a problem hiding this comment.
@y-kawahara-gs
すみません、GitHub上でのメンションに気付けず確認遅れてしまいました💦
修正いただきありがとうございます、こちらApproveいたします！
また、今回の実装の範囲内ではないのものの触っていていくつか気づいた箇所があったので、Issue化＋こちらにメモとして残しておきます！
|
@okuramasafumi |
2 participants
Issue
概要
変更確認方法
ここで以下の点を確認する
Screenshot
変更前
変更後
特別イベントの表示
定期イベントの表示
Summary by CodeRabbit
新機能
改善
テスト