Conversation
WalkthroughAPIとUsersのReportsコントローラにtarget=unchecked_reportsガードを追加し、未チェック日報はadmin_or_mentorのみ取得可能にした。フロントのReports.jsxはURL組み立てを一本化し、unchecked時にtargetパラメータを付与。ビューにタブナビと未チェックバッジを追加。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Mentor as 管理者/メンター
participant UI as Users Reports画面
participant FE as Reports.jsx
participant API as API::Reports#index
participant DB as Report Scopes
Mentor->>UI: /users/:id/reports?target=unchecked_reports を表示
UI->>FE: unchecked=true を渡す
FE->>API: GET /api/reports.json?user_id=:id&...&target=unchecked_reports
API->>API: current_user.admin_or_mentor? を検証
alt 権限あり
API->>DB: apply unchecked.not_wip + filters (practice/user/company/limit)
DB-->>API: records
API-->>FE: 200 OK (JSON)
FE-->>UI: 日報一覧表示
else 権限なし
API-->>FE: 403 Forbidden
FE-->>UI: エラー表示
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.76.1)app/controllers/api/reports_controller.rbrubocop-minitest extension supports plugin, specify app/controllers/users/reports_controller.rbrubocop-minitest extension supports plugin, specify Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
hirokiej
force-pushed
the
feature/add-unchecked-reports-tab-for-mentor-only
branch
3 times, most recently
from
7f8f9f2 to
f25d520
Compare
hirokiej
force-pushed
the
feature/add-unchecked-reports-tab-for-mentor-only
branch
from
f25d520 to
f223c54
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: 2
🧹 Nitpick comments (4)
app/controllers/users/reports_controller.rb (1)
46-48: targetパラメータは許可リストでガードすることを提案将来的なパラメータ乱用や想定外入力を避けるため、許可値のみを通す形にしておくと安全です。
- def set_target - @target = params[:target] || 'all_reports' - end + def set_target + allowed = %w[all_reports unchecked_reports] + @target = allowed.include?(params[:target]) ? params[:target] : 'all_reports' + endapp/views/users/reports/index.html.slim (2)
8-8: mentor_login? ブロック内での class 条件は冗長ですこの行は既に
- if mentor_login?ブロック内のため、三項演算子は常に'is-only-mentor'を返します。シンプルにできます。- nav.tab-nav(class="#{mentor_login? ? 'is-only-mentor' : ''}") + nav.tab-nav(class="is-only-mentor")
14-14: link_to の class 組み立てを簡潔にし、アクセシビリティ向上の aria-current を付与しましょう配列を
<<で破壊的に結合する形は読み取りがやや難しいです。配列とcompactを使うと簡潔で Rails 的です。あわせて、アクティブタブにはaria-current="page"を付与すると支援技術での認識性が上がります。- = link_to t("target.#{target}"), user_reports_path(@user, target: target), class: (@target == target ? ['is-active'] : []) << 'tab-nav__item-link' + = link_to t("target.#{target}"), + user_reports_path(@user, target: target), + class: ['tab-nav__item-link', ('is-active' if @target == target)].compact, + aria: { current: ('page' if @target == target) }app/javascript/components/Reports.jsx (1)
28-34: practice_id が二重に付与され得るため一本化を推奨
practiceIdとuserPracticeIdが同時に値を持つケースでpractice_idが2回クエリに付与され、受け側の解釈次第では意図しない絞り込みになる恐れがあります。どちらか優先（通常はユーザー操作のuserPracticeIdを優先）にしてください。- if (practiceId) reportsUrl += `&practice_id=${practiceId}` + // userPracticeId（ユーザー操作）を優先し、なければ props の practiceId を使う + const selectedPracticeId = userPracticeId || practiceId + if (selectedPracticeId) reportsUrl += `&practice_id=${selectedPracticeId}` if (companyId) reportsUrl += `&company_id=${companyId}` - if (userPracticeId) reportsUrl += `&practice_id=${userPracticeId}` if (unchecked) reportsUrl += `&target=unchecked_reports`
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
app/controllers/api/reports_controller.rb(1 hunks)app/controllers/users/reports_controller.rb(2 hunks)app/helpers/page_tabs/users_helper.rb(2 hunks)app/javascript/components/Reports.jsx(1 hunks)app/views/reports/_report_body.html.slim(1 hunks)app/views/users/reports/index.html.slim(1 hunks)config/locales/ja.yml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
⚙️ CodeRabbit Configuration File
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/controllers/api/reports_controller.rbapp/controllers/users/reports_controller.rbapp/helpers/page_tabs/users_helper.rb
🧬 Code Graph Analysis (3)
app/controllers/users/reports_controller.rb (2)
app/controllers/api/users_controller.rb (1)
before_action(3-87)app/controllers/reports_controller.rb (1)
set_report(106-108)
app/helpers/page_tabs/users_helper.rb (2)
app/models/user.rb (1)
admin_or_mentor?(662-664)app/models/cache.rb (1)
user_unchecked_report_count(15-19)
app/javascript/components/Reports.jsx (1)
app/javascript/user_mentor_memo.js (1)
userId(9-9)
🔇 Additional comments (7)
config/locales/ja.yml (1)
470-471: i18nキーの追加は妥当ですUIのタブ表示用に
target.all_reports/target.unchecked_reportsを用意した構成は既存の命名とも整合が取れており問題ありません。app/helpers/page_tabs/users_helper.rb (2)
10-10: 日報タブへのバッジ追加は意図どおりでOK
badge:でメンター専用の未チェック件数を表示する意図は明確で、count:と役割が分かれているのも良いです。
38-42: キャッシュの無効化タイミングの確認をお願いします
Cache.user_unchecked_report_count(user)を利用しているため、チェック作成/削除時やWIP状態の変更時に該当ユーザーのキャッシュキー（"#{user.id}-user_unchecked_report_count"）が適切に削除・更新されることをご確認ください。未対応の場合、バッジが古い件数を表示する恐れがあります。必要であれば、Checkの作成/削除やReportのWIP変更時にキャッシュ削除するコールバックの追加をご提案できます。app/views/reports/_report_body.html.slim (1)
17-17: メンター時の遷移先を未チェックタブに変更はOKメンター導線として「未チェック」タブに誘導する変更はPRの目的に合致しています。
target: 'unchecked_reports'の指定も適切です。app/controllers/users/reports_controller.rb (1)
34-39: サーバーレンダリング側のフィルタは意図どおりでOK
@target == 'unchecked_reports'の場合のみunchecked.not_wipを適用しており、WIPを除外した未チェック一覧になるため、要件に沿っています。API側の挙動も同様に揃えると一貫性が保てます（別コメント参照）。app/views/users/reports/index.html.slim (1)
7-15: メンター限定タブの導入は整合性が取れていて良いです
- target パラメータで「全て/未チェック」を切り替え、React 側に unchecked を渡す流れが明快です。
- 既存のリンク（ユーザー日報一覧へ）とも動作が揃っており、UX 的にも自然です。
app/javascript/components/Reports.jsx (1)
28-35: URL 構築ロジックの一本化は読みやすくて良いです分岐がなくなり、useSWR のキーも単純化されて保守性が上がっています。
| li.tab-nav__item | ||
| = link_to t("target.#{target}"), user_reports_path(@user, target: target), class: (@target == target ? ['is-active'] : []) << 'tab-nav__item-link' | ||
|
|
||
| = react_component('Reports', userId: @user.id, practices: @current_user_practice.sorted_practices, unchecked: (@target == 'unchecked_reports')) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
未チェック一覧のアクセス制御（メンター限定）を確認してください
URL 直叩きで一般ユーザーが target=unchecked_reports を指定した場合の挙動（無視 or 403 など）が API/コントローラ側で適切にガードされているか確認をお願いします。メンター UI は非表示でも、パラメータで閲覧できると情報露出になります。
以下スクリプトで関連箇所の有無とガード条件を確認できます（リポジトリルートで実行）。
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== unchecked_reports を扱うコントローラ周辺の定義確認 =='
rg -n -C5 -g 'app/controllers/**/*.rb' -P '(unchecked_reports|target\s*=>?\s*[\'"]unchecked_reports[\'"]|params\[:target\]\s*==\s*[\'"]unchecked_reports[\'"])'
echo
echo '== メンター/管理者ガードらしきメソッドの出現箇所確認 =='
rg -n -C3 -g 'app/controllers/**/*.rb' -P '(admin_or_mentor\?|mentor_login\?|mentor_mode)'
echo
echo '== users/reports_controller.rb と api/reports_controller.rb の set_target / フィルタ分岐の確認 =='
fd -t f reports_controller.rb app/controllers | xargs -I{} sh -c 'echo "--- {} ---"; sed -n "1,200p" {} | nl -ba | sed -n "1,200p" | rg -n -C3 -P "(before_action|set_target|unchecked|params\\[:target\\])" || true'Length of output: 220
シェルスクリプト内の正規表現のクオート部分でパースエラーが発生しているようです。以下の修正版をリポジトリルートで再度実行し、結果をご共有ください。
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== unchecked_reports を扱うコントローラ周辺の定義確認 =='
rg -n -C5 -g 'app/controllers/**/*.rb' -P "unchecked_reports|target\\s*=>?\\s*['\"]unchecked_reports['\"]|params\\[:target\\]\\s*==\\s*['\"]unchecked_reports['\"]"
echo
echo '== メンター/管理者ガードらしきメソッドの出現箇所確認 =='
rg -n -C3 -g 'app/controllers/**/*.rb' -P "admin_or_mentor\\?|mentor_login\\?|mentor_mode"
echo
echo '== users/reports_controller.rb と api/reports_controller.rb の set_target / フィルタ分岐の確認 =='
fd -t f reports_controller.rb app/controllers | while read file; do
echo "--- $file ---"
rg -n -C3 -e "before_action" -e "set_target" -e "params\\[:target\\]" -e "unchecked" "$file" || true
doneLength of output: 14405
以下のスクリプトで API 側と UI 側それぞれのベースコントローラにメンター制御（before_action 等）が仕込まれているか確認してください。
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== app/controllers/api/base_controller.rb (1-200行) =='
sed -n '1,200p' app/controllers/api/base_controller.rb | nl -ba
echo
echo '== API::BaseController の before_action 一覧 =='
rg -n --color=never 'before_action' -g 'app/controllers/api/base_controller.rb' || echo 'no matches'
echo
echo '== app/controllers/application_controller.rb (1-200行) =='
sed -n '1,200p' app/controllers/application_controller.rb | nl -ba
echo
echo '== ApplicationController の before_action 一覧 =='
rg -n --color=never 'before_action' -g 'app/controllers/application_controller.rb' || echo 'no matches'Length of output: 293
以下のスクリプトで API 側と UI 側それぞれのベースコントローラにメンター権限制御が設定されているか確認し、結果を共有してください。
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== API::BaseController (app/controllers/api/base_controller.rb lines 1-200) =='
if [[ -f app/controllers/api/base_controller.rb ]]; then
sed -n '1,200p' app/controllers/api/base_controller.rb
echo
echo '== before_action in API::BaseController =='
grep -n 'before_action' app/controllers/api/base_controller.rb || echo '== no before_action =='
echo
echo '== require_mentor_login_for_api in API::BaseController =='
grep -n 'require_mentor_login_for_api' app/controllers/api/base_controller.rb || echo '== no require_mentor_login_for_api =='
echo
echo '== require_admin_or_mentor_login in API::BaseController =='
grep -n 'require_admin_or_mentor_login' app/controllers/api/base_controller.rb || echo '== no require_admin_or_mentor_login =='
else
echo 'app/controllers/api/base_controller.rb not found'
fi
echo
echo '== ApplicationController (app/controllers/application_controller.rb lines 1-200) =='
if [[ -f app/controllers/application_controller.rb ]]; then
sed -n '1,200p' app/controllers/application_controller.rb
echo
echo '== before_action in ApplicationController =='
grep -n 'before_action' app/controllers/application_controller.rb || echo '== no before_action =='
echo
echo '== require_mentor_login in ApplicationController =='
grep -n 'require_mentor_login' app/controllers/application_controller.rb || echo '== no require_mentor_login =='
echo
echo '== require_admin_or_mentor_login in ApplicationController =='
grep -n 'require_admin_or_mentor_login' app/controllers/application_controller.rb || echo '== no require_admin_or_mentor_login =='
else
echo 'app/controllers/application_controller.rb not found'
fiLength of output: 4874
未チェック一覧へのアクセス制御を強化してください
現状、Users::ReportsController（UI側）と API::ReportsController（API側）いずれも target=unchecked_reports を指定すると誰でも未チェック一覧を取得できてしまいます。メンター以外のアクセスを禁止するため、以下の修正をお願いします。
-
app/controllers/users/reports_controller.rb
- クラス定義直後に
before_action :require_admin_or_mentor_login
を追加し、Web UI 側の/users/:user_id/reportsアクションをメンター限定にする
- クラス定義直後に
-
app/controllers/api/reports_controller.rb
- コントローラ先頭に
before_action :require_mentor_login_for_api, only: :index
を追加し、API 経由でtarget=unchecked_reportsを扱う際には JWT 認証＋メンター権限を必須化する - もしくは
index内の
if params[:target] == 'unchecked_reports'
の前後でhead :forbidden unless current_user.admin_or_mentor?のような明示的ガードを入れる
- コントローラ先頭に
これにより、パラメータ直接指定による情報漏洩を防止できます。
🤖 Prompt for AI Agents
In app/views/users/reports/index.html.slim around line 16: the view passes
target=unchecked_reports which currently allows anyone to fetch unchecked
reports; to enforce mentor-only access add controller guards: in
app/controllers/users/reports_controller.rb (class start) add before_action
:require_admin_or_mentor_login to restrict the web UI /users/:user_id/reports to
mentors/admins; in app/controllers/api/reports_controller.rb (top of controller)
add before_action :require_mentor_login_for_api, only: :index to require
JWT+mentor for API index, or alternatively inside index add an explicit guard
(e.g. head :forbidden unless current_user&.admin_or_mentor?) immediately before
handling if params[:target] == 'unchecked_reports' so parameter-based access to
unchecked reports is forbidden for non-mentors.
There was a problem hiding this comment.
それぞれ、head :forbiddenで対応。
There was a problem hiding this comment.
@hirokiej head :forbidden での対応で問題ありません。これにより一般ユーザーが target=unchecked_reports パラメータを直接指定してもアクセスが拒否され、メンター限定機能として適切に保護されます。
hirokiej
force-pushed
the
feature/add-unchecked-reports-tab-for-mentor-only
branch
from
f223c54 to
0d76628
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/controllers/api/reports_controller.rb (1)
11-16: API 側の未チェック絞り込みはスコープに寄せ、includes(:checks)は不要過去コメントのとおり（開発者も同意済み）、モデルスコープで統一すると読みやすく、WIP 除外も明示できます。
includes(:checks)はこの用途では不要です。- if params[:target] == 'unchecked_reports' - unless current_user.admin_or_mentor? - return head :forbidden - end - @reports = @reports.includes(:checks).unchecked.not_wip - end + if params[:target] == 'unchecked_reports' + return head :forbidden unless current_user.admin_or_mentor? + @reports = @reports.unchecked.not_wip + end
🧹 Nitpick comments (4)
app/helpers/page_tabs/users_helper.rb (2)
10-10: count にlengthを使うと関連を全件ロードしやすい—sizeかcountを推奨関連が未ロード時は
sizeはCOUNT(*)に、ロード済みならメモリ上の長さを返します。lengthは常にロードを誘発するため非効率です。- tabs << { name: '日報', link: user_reports_path(user), count: user.reports.length, badge: unchecked_report_badge(current_user:, user:) } + tabs << { name: '日報', link: user_reports_path(user), count: user.reports.size, badge: unchecked_report_badge(current_user:, user:) }
38-42: バッジは0件時に非表示にするか要件確認。ついでに nil セーフに現仕様だとメンター/管理者には 0 件でもバッジが出る可能性があります。UX的に 0 は非表示にするなら以下のようにします。
current_userが nil のケースにも安全です。- def unchecked_report_badge(current_user:, user:) - return nil unless current_user.admin_or_mentor? - - Cache.user_unchecked_report_count(user) - end + def unchecked_report_badge(current_user:, user:) + return nil unless current_user&.admin_or_mentor? + count = Cache.user_unchecked_report_count(user) + return nil if count.zero? + count + end補足: 「0 件時も表示する」仕様であればこの変更は不要です。どちらが正か教えてください。
app/javascript/components/Reports.jsx (2)
35-35: SWR のキーは OK。クエリ構築は URLSearchParams 化で可読性・安全性向上（任意）文字列連結でも動きますが、
URLSearchParamsを使うとエンコード漏れや二重付与の防止がしやすくなります。例:
const params = new URLSearchParams({ page: String(page) }) if (userId) params.set('user_id', userId) if (companyId) params.set('company_id', companyId) const pid = userPracticeId || practiceId if (pid) params.set('practice_id', pid) if (unchecked) params.set('target', 'unchecked_reports') const reportsUrl = `/api/reports.json?${params.toString()}` const { data, error } = useSWR(reportsUrl, fetcher)
28-35: おまけ: 自己代入の useEffect は削除可能
useEffect(() => { setUserPracticeId(userPracticeId) }, [userPracticeId])は実質ノーオペです。ノイズ削減のため削除を推奨します。以下は選択行範囲外のため参考コードです（実装は該当行を削除してください）:
// 削除して問題ありません // useEffect(() => { // setUserPracticeId(userPracticeId) // }, [userPracticeId])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
app/controllers/api/reports_controller.rb(1 hunks)app/controllers/users/reports_controller.rb(2 hunks)app/helpers/page_tabs/users_helper.rb(2 hunks)app/javascript/components/Reports.jsx(1 hunks)app/views/reports/_report_body.html.slim(1 hunks)app/views/users/reports/index.html.slim(1 hunks)config/locales/ja.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- config/locales/ja.yml
- app/views/reports/_report_body.html.slim
- app/controllers/users/reports_controller.rb
- app/views/users/reports/index.html.slim
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
⚙️ CodeRabbit Configuration File
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/controllers/api/reports_controller.rbapp/helpers/page_tabs/users_helper.rb
🧠 Learnings (1)
📚 Learning: 2025-07-23T20:31:13.856Z
Learnt from: jun-kondo
PR: fjordllc/bootcamp#8977
File: app/controllers/reports_controller.rb:63-63
Timestamp: 2025-07-23T20:31:13.856Z
Learning: fjordllc/bootcampプロジェクトの`app/controllers/reports_controller.rb`において、`create`と`update`アクションは両方とも`report.save_uniquely`を使用し、同じ`:report_save`イベントと`'report.save'`イベントを発行する。これは両方とも本質的に「レポートの保存」操作であり、作成と更新を区別する必要がないためである。
Applied to files:
app/controllers/api/reports_controller.rb
🧬 Code Graph Analysis (3)
app/controllers/api/reports_controller.rb (1)
app/models/user.rb (1)
admin_or_mentor?(662-664)
app/helpers/page_tabs/users_helper.rb (2)
app/models/user.rb (1)
admin_or_mentor?(662-664)app/models/cache.rb (1)
user_unchecked_report_count(15-19)
app/javascript/components/Reports.jsx (1)
app/javascript/user_mentor_memo.js (1)
userId(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (2)
app/helpers/page_tabs/users_helper.rb (1)
10-10: PageTabsComponent は badge に対応済みです
PageTabComponentの初期化子でbadge:を受け取り、badge&.positive?の判定でバッジを描画する実装が既に存在するため、該当のヘルパーから渡したbadge:は正しく表示されます。この指摘は無視して問題ありません。app/controllers/api/reports_controller.rb (1)
11-16: unchecked_reports はユーザー個別用途想定。user_id必須にするか要件確認現状、メンター/管理者であれば
user_idなしでも全体の未チェック一覧を取得可能です。仕様上「ユーザー個別配下の日報一覧」に限定するならparams[:user_id]の存在を必須にしておくと意図しない使われ方を防げます（UI は個別ユーザーでしか使用しない前提でも、API は汎用なので注意）。例（必要なら）:
if params[:target] == 'unchecked_reports' return head :forbidden unless current_user.admin_or_mentor? return head :bad_request if params[:user_id].blank? # 個別ページ限定にする場合 @reports = @reports.unchecked.not_wip end
hirokiej
force-pushed
the
feature/add-unchecked-reports-tab-for-mentor-only
branch
3 times, most recently
from
bda6473 to
10380eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/javascript/components/Reports.jsx (1)
27-28: practice_id の二重付与問題の解消、GJです
const pid = userPracticeId || practiceIdによってフィルタ優先度が明確になり、重複付与が解消されています。
🧹 Nitpick comments (2)
app/javascript/components/Reports.jsx (2)
24-29: クエリ文字列の構築は URLSearchParams で堅牢化を現在の文字列連結でも動作しますが、将来的なパラメータ追加や値のエンコードを考慮すると URLSearchParams を使うと安全・可読です（SWR のキー安定性も向上）。
以下の置き換えを提案します。- let reportsUrl = `/api/reports.json?page=${page}` - if (userId) reportsUrl += `&user_id=${userId}` - if (companyId) reportsUrl += `&company_id=${companyId}` - const pid = userPracticeId || practiceId - if (pid) reportsUrl += `&practice_id=${pid}` - if (unchecked) reportsUrl += `&target=unchecked_reports` + const params = new URLSearchParams({ page: String(page) }) + if (userId) params.set('user_id', String(userId)) + if (companyId) params.set('company_id', String(companyId)) + const pid = userPracticeId || practiceId + if (pid) params.set('practice_id', String(pid)) + if (unchecked) params.set('target', 'unchecked_reports') + const reportsUrl = `/api/reports.json?${params.toString()}`
24-29: フィルタ変更時にページ番号を 1 にリセットすると UX が向上しますpractice や unchecked の切り替え時に前の page を引きずると「空ページ」に飛ぶ可能性があります。
userPracticeId/userId/companyId/uncheckedが変わったらsetPage(1)するのが無難です（optional）。
実装例（別箇所に追加。useEffect の import も併せて復帰が必要）:// 例: setUserPracticeId の定義付近に useEffect(() => { setPage(1) }, [userPracticeId, userId, companyId, unchecked])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/javascript/components/Reports.jsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (3)
app/javascript/components/Reports.jsx (3)
1-1: 不要な useEffect を削除したのは妥当です副作用がなくなったため import の整理は正しいです。
31-31: SWR を単一キーに統一したのはシンプルで良いですURL 生成の一本化により分岐が減り、読みやすさ/保守性が向上しています。
29-29: 権限制御の確認完了
フロントからtarget=unchecked_reportsを付与した場合でも、以下のコントローラでcurrent_user.admin_or_mentor?による認可チェックが行われていることを確認しました。
- app/controllers/users/reports_controller.rb（set_reports 内）：36 行目
- app/controllers/api/reports_controller.rb：12 行目
これにより、メンター／管理者以外はアクセスできない仕様になっています。問題ありません。
|
@tyrrell-IH さん |
|
すいません、8/20〜8/26で帰省する予定でその間あまり稼働できません。 自身のPRの方も進めたいので、9/8(月)期限とかでよろしければレビューできますがいかがでしょうか？ もし難しければ、他の方にレビューをご依頼いただければと思います🙏 |
|
@tyrrell-IH さん @sjabcdefin さん |
|
@hirokiej |
|
@hirokiej |
hirokiej
force-pushed
the
feature/add-unchecked-reports-tab-for-mentor-only
branch
from
10380eb to
e2e1327
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/helpers/page_tabs/users_helper.rb (2)
10-10: count はlengthではなくsizeを推奨（無駄なレコードロードを回避）
lengthは関連をロード済みの場合に全件をメモリに展開する可能性がありコストが高いです。sizeなら未ロード時はCOUNT(*)にフォールバックします。- tabs << { name: '日報', link: user_reports_path(user), count: user.reports.length, badge: unchecked_report_badge(current_user:, user:) } + tabs << { name: '日報', link: user_reports_path(user), count: user.reports.size, badge: unchecked_report_badge(current_user:, user:) }
38-42:current_userが nil の場合に NoMethodError の可能性（nil セーフに）未ログイン状態でこのヘルパが呼ばれるコンテキストがある場合、
current_user.admin_or_mentor?が例外になります。nil セーフにしておくと安全です。- def unchecked_report_badge(current_user:, user:) - return nil unless current_user.admin_or_mentor? - - Cache.user_unchecked_report_count(user) - end + def unchecked_report_badge(current_user:, user:) + return nil unless current_user&.admin_or_mentor? + + Cache.user_unchecked_report_count(user) + end補足: PageTabsComponent 側で
badgeオプションの描画が実装済みであることも合わせてご確認ください（未実装でも本変更は無害ですが、表示されない可能性があります）。app/views/users/reports/index.html.slim (1)
7-16: クラス指定の簡素化と可読性向上（配列操作の<<は避けたい）
if mentor_login?ブロック内でさらにclass="#{mentor_login? ? ...}"を評価しているのは二重判定です。ここは静的に.is-only-mentorを付けてよいです。class: (@target == target ? ['is-active'] : []) << 'tab-nav__item-link'は配列へ副作用的に<<しており読み取りにくいです。['tab-nav__item-link', ('is-active' if 条件)]などの形がシンプルです。- 可能なら
aria-currentを付けてアクセシビリティを向上させると尚良いです。-- if mentor_login? - nav.tab-nav(class="#{mentor_login? ? 'is-only-mentor' : ''}") +- if mentor_login? + nav.tab-nav.is-only-mentor .container ul.tab-nav__items - targets = %w[all_reports unchecked_reports] - targets.each do |target| li.tab-nav__item - = link_to t("target.#{target}"), user_reports_path(@user, target: target), class: (@target == target ? ['is-active'] : []) << 'tab-nav__item-link' + = link_to t("target.#{target}"), + user_reports_path(@user, target: target), + class: ['tab-nav__item-link', ('is-active' if @target == target)], + aria: { current: (@target == target ? 'page' : nil) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
app/controllers/api/reports_controller.rb(1 hunks)app/controllers/users/reports_controller.rb(2 hunks)app/helpers/page_tabs/users_helper.rb(2 hunks)app/javascript/components/Reports.jsx(2 hunks)app/views/reports/_report_body.html.slim(1 hunks)app/views/users/reports/index.html.slim(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/controllers/api/reports_controller.rb
- app/javascript/components/Reports.jsx
- app/controllers/users/reports_controller.rb
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
⚙️ CodeRabbit Configuration File
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/helpers/page_tabs/users_helper.rb
🧬 Code Graph Analysis (1)
app/helpers/page_tabs/users_helper.rb (2)
app/models/user.rb (1)
admin_or_mentor?(662-664)app/models/cache.rb (1)
user_unchecked_report_count(15-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (1)
app/views/reports/_report_body.html.slim (1)
17-17: メンター向け導線として適切です（LGTM）
target: 'unchecked_reports'を付与して未チェック一覧へ遷移させる変更は意図通りに見えます。権限制御はコントローラ側でhead :forbidden等を入れている前提で問題ありません。
|
@sjabcdefin さん |
sjabcdefin
left a comment
sjabcdefin
left a comment
There was a problem hiding this comment.
お疲れさまです。😌
コメント残させていただきました！
ご確認お願いいたします。😌🙏
| render PageTabsComponent.new(tabs:, active_tab:) | ||
| end | ||
|
|
||
| def unchecked_report_badge(current_user:, user:) |
There was a problem hiding this comment.
unchecked_report_badgeメソッドについて、以下をuser_page_tabsメソッド内にまとめても良さそうに思いました！
unchecked_report_badge = current_user.admin_or_mentor? ? Cache.user_unchecked_report_count(user) : nil
There was a problem hiding this comment.
確かに、その方がわかりやすくて良いと思いました。
修正します！
| = user_page_tabs(@user, active_tab: '日報') | ||
|
|
||
| = react_component('Reports', userId: @user.id, practices: @current_user_practice.sorted_practices) | ||
| - if mentor_login? |
There was a problem hiding this comment.
admin ロールのみを持つユーザー（adminonly）でログインした際に、[全て] [未チェック]タブが表示されませんでした。
admin ユーザーでも [全て] [未チェック] タブは表示される仕様だと認識しています。
この部分が該当箇所かと思いますので、ご確認いただけると幸いです。😌
There was a problem hiding this comment.
勝手な思い込みでadminはroleとしてメンターも含まれていると勘違いしておりました。
ご確認ありがとうございます🙇
修正します！
There was a problem hiding this comment.
管理者ユーザ(adminonly)でログインした際に、[全て/未チェック]タブが表示されるようになりました！
ご対応ありがとうございます。😌🙏
すみません。もう１点ご確認いただけますでしょうか。 管理者ユーザ(adminonly)でメンターモードOFFのときに、[全て/未チェック]タブが表示されるままのようです。 メンターモードオフのときは、[全て/未チェック]タブが表示されない仕様だと認識しています。
ご確認いただけると幸いです。😌
| - targets = %w[all_reports unchecked_reports] | ||
| - targets.each do |target| | ||
| li.tab-nav__item | ||
| = link_to t("target.#{target}"), user_reports_path(@user, target: target), class: (@target == target ? ['is-active'] : []) << 'tab-nav__item-link' |
There was a problem hiding this comment.
user_reports_path(@user, target: target)について、user_reports_path(@user, target:)で問題ないと思います！！(target省略)
|
|
||
| def set_reports | ||
| @reports = user.reports.list.page(params[:page]) | ||
| @reports = case @target |
There was a problem hiding this comment.
分岐が2つしかないので、case ではなく if/else で十分ではないかと思いました。🤔
There was a problem hiding this comment.
caseからifを足したときにどうにかできないかな？と思ったのですが、==をを使えば、if/elseでいけましたね…！
修正します！
| @reports = user.reports.list.page(params[:page]) | ||
| @reports = case @target | ||
| when 'unchecked_reports' | ||
| head :forbidden unless current_user.admin_or_mentor? |
There was a problem hiding this comment.
WebUIからの操作だと、head :forbidden ではなく、「管理者・メンターとしてログインしてください」というメッセージを表示し、元のページにリダイレクトしたほうがユーザーフレンドリーではないかと思いました。🤔
There was a problem hiding this comment.
別のページではどうなっているかを怠っていました💦
ユーザーフレンドリーではないかと思いました。🤔
それとは別でユーザーフレンドリーという部分の考えもなかったので勉強になりました🙇
修正します！
hirokiej
force-pushed
the
feature/add-unchecked-reports-tab-for-mentor-only
branch
from
e2e1327 to
a476bef
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/helpers/page_tabs/users_helper.rb (1)
5-12: 過去指摘の「バッジ計算の集約」対応はOKです
user_page_tabs内でバッジ値を完結させた実装により、責務が明確になりました。app/views/users/reports/index.html.slim (1)
14-14: Shorthand引数（target:）の利用は問題なし
user_reports_path(@user, target:)の省略記法はRuby 3系で妥当です。可読性も良いです。
🧹 Nitpick comments (7)
app/controllers/users_controller.rb (1)
68-68: トークン生成はモデル側へ委譲（has_secure_token）し、Fat Controller を回避しましょうコントローラでトークンを生成すると責務が散らばります。User モデルに has_secure_token :unsubscribe_email_token を定義すれば、作成時に自動生成＋再生成 API（regenerate_...）が使え、重複にも強くなります。ガイドライン的にも Controller を痩せさせるのが吉です。
この差分のみの修正案（本ファイル内）:
- @user.unsubscribe_email_token = SecureRandom.urlsafe_base64 + # User.has_secure_token :unsubscribe_email_token により自動生成されるため不要サポートとなる変更（他ファイル例）:
# app/models/user.rb class User < ApplicationRecord has_secure_token :unsubscribe_email_token end任意の堅牢化（既存レコード対策・一意制約）:
- 既存ユーザーの nil 埋めを行う rake/migration（nil のみに token を付与）
- DB にユニークインデックスを付与（存在しない場合）
config/initializers/active_support_notifications.rb (1)
28-29: 購読登録を一度きりにするヘルパー化を検討してください説明
Rails.application.reloader.to_prepare 内でActiveSupport::Notifications.subscribeを呼び出すと、開発環境のコードリロード時に購読が重複登録され続けるリスクがあります。将来的なデバッグ容易性やメモリ効率を考慮し、subscribe_onceのような「一度だけ登録」するラッパーヘルパーの導入をおすすめします。対象箇所
- config/initializers/active_support_notifications.rb の 26〜29行
差分例
- ActiveSupport::Notifications.subscribe('regular_event.update', RegularEventUpdateNotifier.new) - ActiveSupport::Notifications.subscribe('student_or_trainee.create', TimesChannelCreator.new) + Notifications.subscribe_once('regular_event.update', RegularEventUpdateNotifier) + Notifications.subscribe_once('student_or_trainee.create', TimesChannelCreator)ヘルパー実装例
# config/initializers/notifications_helper.rb module Notifications mattr_accessor :handles, default: {} def self.subscribe_once(event, klass) # すでに購読ハンドルがあれば解除 ActiveSupport::Notifications.unsubscribe(handles[event]) if handles[event] # 新たに購読してハンドルを保持 handles[event] = ActiveSupport::Notifications.subscribe(event, klass.new) end endその他の
subscribe呼び出しも同様に見直し、一度きりの登録に置き換えることで、多重購読による意図しない副作用を防止してください。app/helpers/page_tabs/users_helper.rb (2)
7-7: nilセーフティと0件時の非表示（任意）未ログイン等で
current_userがnilの可能性を排除できない場合に備え、安全側に倒すのが無難です。ついでに0件時はバッジを非表示にする（nilにする）とUI的に自然です。- unchecked_report_badge = current_user.admin_or_mentor? ? Cache.user_unchecked_report_count(user) : nil + count = current_user&.admin_or_mentor? ? Cache.user_unchecked_report_count(user) : nil + unchecked_report_badge = count.to_i.positive? ? count : nilこの画面は常にログイン前提でしょうか？未ログインアクセスがあり得るなら上記変更をご検討ください。ゼロ件表示の是非も仕様確認をお願いします。
11-11:length→sizeで不要なレコードロードを回避
user.reports.lengthは未ロード時に全件ロードします。sizeはキャッシュ済なら配列長、未ロードならCOUNT(*)になり効率的です。- tabs << { name: '日報', link: user_reports_path(user), count: user.reports.length, badge: unchecked_report_badge } + tabs << { name: '日報', link: user_reports_path(user), count: user.reports.size, badge: unchecked_report_badge }app/views/users/reports/index.html.slim (1)
14-14: クラス属性の生成をclass_namesで簡潔に（任意）配列へ
<<で破壊的追加するより、Railsのclass_namesを使うと読みやすくなります（Rails 7+）。- = link_to t("target.#{target}"), user_reports_path(@user, target:), class: (@target == target ? ['is-active'] : []) << 'tab-nav__item-link' + = link_to t("target.#{target}"), + user_reports_path(@user, target:), + class: class_names('tab-nav__item-link', 'is-active': @target == target)app/models/user_course_practice.rb (1)
96-98: Query Objectへの委譲はGood複雑なクエリの切り出しでモデルの責務が軽くなりました。キャッシュ（メモ化）も維持されており問題ありません。
sorted_practicesの並び（courses→categories）とQuery側の並び（categories→courses）が逆順です。UI要件に合わせて揃える必要があれば、Query側のorderを調整してください。test/queries/user_unstarted_practices_query_test.rb (1)
55-57: 重複排除と並び順のテスト追加を提案（任意）
distinct追加の回帰を防ぐため、重複なし・期待順序の検証を1ケースずつ追加するのが安全です。@@ test 'should return different results for different users' do user1 = users(:komagata) user2 = users(:machida) result1 = UserUnstartedPracticesQuery.new(user: user1).call result2 = UserUnstartedPracticesQuery.new(user: user2).call assert_not_equal result1.to_a.sort, result2.to_a.sort end + + test 'should not include duplicate practices' do + user = users(:komagata) + result = UserUnstartedPracticesQuery.new(user:).call + assert_equal result.map(&:id).uniq, result.map(&:id) + end + + # 並び順が仕様に含まれる場合のみ有効化 + # test 'should be ordered by categories_practices and courses_categories position' do + # user = users(:komagata) + # result = UserUnstartedPracticesQuery.new(user:).call + # sorted_ids = result.sort_by { |p| + # [ + # p.categories_practices.first&.position || 0, + # p.categories.first&.courses_categories.first&.position || 0 + # ] + # }.map(&:id) + # assert_equal sorted_ids, result.map(&:id) + # end end並び順の仕様がUI依存であればコメントアウトのままでもOKです。仕様が固まっている場合は有効化をご検討ください。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
app/controllers/api/reports_controller.rb(1 hunks)app/controllers/users/reports_controller.rb(2 hunks)app/controllers/users_controller.rb(1 hunks)app/helpers/page_tabs/users_helper.rb(1 hunks)app/javascript/components/Reports.jsx(2 hunks)app/models/sign_up_notifier.rb(0 hunks)app/models/user_course_practice.rb(1 hunks)app/queries/user_unstarted_practices_query.rb(1 hunks)app/views/reports/_report_body.html.slim(1 hunks)app/views/users/reports/index.html.slim(1 hunks)config/initializers/active_support_notifications.rb(1 hunks)config/locales/ja.yml(1 hunks)test/queries/user_unstarted_practices_query_test.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/models/sign_up_notifier.rb
🚧 Files skipped from review as they are similar to previous changes (5)
- config/locales/ja.yml
- app/controllers/api/reports_controller.rb
- app/javascript/components/Reports.jsx
- app/views/reports/_report_body.html.slim
- app/controllers/users/reports_controller.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/models/user_course_practice.rbtest/queries/user_unstarted_practices_query_test.rbapp/controllers/users_controller.rbconfig/initializers/active_support_notifications.rbapp/queries/user_unstarted_practices_query.rbapp/helpers/page_tabs/users_helper.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/queries/user_unstarted_practices_query_test.rb
🧠 Learnings (1)
📚 Learning: 2025-07-23T20:31:13.856Z
Learnt from: jun-kondo
PR: fjordllc/bootcamp#8977
File: app/controllers/reports_controller.rb:63-63
Timestamp: 2025-07-23T20:31:13.856Z
Learning: fjordllc/bootcampプロジェクトの`app/controllers/reports_controller.rb`において、`create`と`update`アクションは両方とも`report.save_uniquely`を使用し、同じ`:report_save`イベントと`'report.save'`イベントを発行する。これは両方とも本質的に「レポートの保存」操作であり、作成と更新を区別する必要がないためである。
Applied to files:
app/views/users/reports/index.html.slim
🧬 Code graph analysis (2)
config/initializers/active_support_notifications.rb (2)
app/controllers/users_controller.rb (1)
new(52-57)app/controllers/regular_events_controller.rb (1)
new(17-24)
app/helpers/page_tabs/users_helper.rb (2)
app/models/user.rb (1)
admin_or_mentor?(662-664)app/models/cache.rb (1)
user_unchecked_report_count(15-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (5)
app/controllers/users_controller.rb (1)
60-75: 歓迎: 学生/研修生イベントへの一本化に沿った変更create フローの前段でトークンを付与し、その後の分岐（create_free_user! / create_user!）に影響しない位置に置かれている点は妥当です。Strong Parameters に unsubscribe_email_token を含めていないのも安全です。
app/views/users/reports/index.html.slim (2)
7-10: 提案された条件式の修正は不要です
admin_or_mentor_login?は管理者ユーザーに対してもtrueを返すよう、以下のように既に実装されています。
- app/controllers/concerns/authentication/login_helpers.rb（27行目）
def admin_or_mentor_login? logged_in? && current_user.admin_or_mentor? end- app/models/user.rb（662–664行目）
def admin_or_mentor? admin? || mentor? endしたがって、
- if admin_or_mentor_login? + if current_user&.admin? || mentor_login?への書き換えは既存ロジックと同等であり、根本的な解決にはなりません。
Likely an incorrect or invalid review comment.
16-16: Reportsコンポーネント呼び出しの確認結果
以下5箇所でreact_component('Reports', …)が使われていることをスクリプトで確認しました。
- app/views/practices/reports/index.html.slim:14
- app/views/users/reports/index.html.slim:16
- app/views/talks/show.html.slim:105
- app/views/current_user/reports/index.html.slim:23
- app/views/companies/reports/index.html.slim:21
また、
all:プロップの残存もスクリプトで検索しましたがヒットしませんでした。結論
新設のuncheckedはデフォルトでfalse（従来の全件表示相当）となっており、既存の呼び出しに手を加える必要はありません。app/queries/user_unstarted_practices_query.rb (1)
23-26: サブクエリの形は妥当（補足）
where.not(id: subquery)でSELECT practice_id FROM learnings...をそのまま渡す形はDB最適化されやすく、良い選択です。test/queries/user_unstarted_practices_query_test.rb (1)
5-57: 基本ケースの網羅は十分です未着手のみ抽出、status別の除外、ユーザー差分の検証が揃っていて回帰に強いです。
| def query | ||
| relation | ||
| .joins(categories_practices: { category: :courses_categories }) | ||
| .where(courses_categories: { course_id: @course_id }) | ||
| .where.not(id: learned_practice_ids) | ||
| .select('practices.*') | ||
| .order('categories_practices.position ASC, courses_categories.position ASC') | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
重複行の混入防止のためdistinctを付与してください
categories_practices/courses_categoriesへのJOINで同一Practiceが複数カテゴリに属するケースだと重複が返ります。UI/集計に影響が出るためDISTINCTを推奨します。
def query
relation
.joins(categories_practices: { category: :courses_categories })
.where(courses_categories: { course_id: @course_id })
.where.not(id: learned_practice_ids)
- .select('practices.*')
+ .select('practices.*')
+ .distinct
.order('categories_practices.position ASC, courses_categories.position ASC')
end本件対応後、結果件数/並び順が期待どおりか（重複が消え、順序は維持されるか）を実データ/テストでご確認ください。
🏁 Script executed:
#!/bin/bash
set -e
# Locate the query file
query_file=$(fd -t f 'user_unstarted_practices_query.rb')
echo "Found file: $query_file"
# Show the method implementation
echo "----- user_unstarted_practices_query.rb (first 200 lines) -----"
sed -n '1,200p' "$query_file"
# Search for specs referencing this query
echo "----- Specs referencing UserUnstartedPracticesQuery -----"
rg -n 'UserUnstartedPracticesQuery' -C3 --glob '*_spec.rb' || echo "No specs found for UserUnstartedPracticesQuery"Length of output: 1618
user_unstarted_practices_query.rb に .distinct を追加して重複行を排除
カテゴリ結合（joins(categories_practices: { category: :courses_categories })）を行うと、同一の Practice が複数のカテゴリに属するケースで重複レコードが返される恐れがあります。UI や集計への影響を避けるため、SQL レベルで DISTINCT を付与してください。
対象箇所:
app/queries/user_unstarted_practices_query.rbのdef queryメソッド内
修正例:
def query
relation
.joins(categories_practices: { category: :courses_categories })
.where(courses_categories: { course_id: @course_id })
.where.not(id: learned_practice_ids)
- .select('practices.*')
+ .select('practices.*')
+ .distinct
.order('categories_practices.position ASC, courses_categories.position ASC')
end追加で必要な対応:
.distinctを追加後、重複が排除されつつソート順が維持されるか、実データやテストで必ず検証してください- 現状、このクエリを直接参照するRSpec 等のテストが存在しないようです。重複チェックを含むテストケース（たとえば、同一 Practice が複数カテゴリに属した場合に 1 件しか返らないことを確認するテスト）を新規に実装してください
上記対応により、クエリ結果の正確性と並び順の維持が担保されます。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def query | |
| relation | |
| .joins(categories_practices: { category: :courses_categories }) | |
| .where(courses_categories: { course_id: @course_id }) | |
| .where.not(id: learned_practice_ids) | |
| .select('practices.*') | |
| .order('categories_practices.position ASC, courses_categories.position ASC') | |
| end | |
| def query | |
| relation | |
| .joins(categories_practices: { category: :courses_categories }) | |
| .where(courses_categories: { course_id: @course_id }) | |
| .where.not(id: learned_practice_ids) | |
| .select('practices.*') | |
| .distinct | |
| .order('categories_practices.position ASC, courses_categories.position ASC') | |
| end |
🤖 Prompt for AI Agents
In app/queries/user_unstarted_practices_query.rb around lines 14-21, the query
can return duplicate Practice rows when a practice belongs to multiple
categories; add .distinct to the ActiveRecord chain (keeping the existing
select/order) so duplicates are removed at SQL level, then run/adjust tests and
add a new RSpec that creates a Practice associated with multiple categories to
assert the query returns a single record and that ordering is preserved.
There was a problem hiding this comment.
mainとの差分が混ざってしまっていたので、上記は本PRとは関係ないと思います
There was a problem hiding this comment.
@hirokiej その通りです。app/queries/user_unstarted_practices_query.rb は今回のPR（未チェック日報一覧の追加）とは全く関係のないファイルですね。mainブランチとの差分が混ざってしまったことで、本来のPR変更分ではないファイルにコメントしてしまいました。
私の前回のコメントは無視していただいて構いません。本PRの実際の変更内容（日報一覧のタブ追加、API修正、フロントエンド更新など）に焦点を当てたレビューを行います。
ご指摘いただき、ありがとうございました。
(\ /)
( ._. )
o_(")(") *confused but learning*
hirokiej
force-pushed
the
feature/add-unchecked-reports-tab-for-mentor-only
branch
2 times, most recently
from
8c8a51b to
525bf87
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/controllers/users/reports_controller.rb (1)
35-40: 認可チェックの重複と UI 体験の改善（HTML はリダイレクト、API は 403 に）
- この行の
head :forbiddenは、直前のbefore_action :require_admin_or_mentor_loginと認可責務が重複しています。認可はフィルタに集約し、ここでは純粋に一覧取得のみとするのがシンプルです。- HTML アクセス時に 403 ヘッダのみだと不親切との既出コメントに同意です。フィルタ側でフラッシュメッセージ付きリダイレクト、API など非 HTML では 403 を返すのが良いです。
差分（重複認可の削除）:
- @reports = if @target == 'unchecked_reports' - head :forbidden unless current_user.admin_or_mentor? - @user.reports.unchecked.not_wip.list.page(params[:page]) - else - @user.reports.list.page(params[:page]) - end + @reports = if @target == 'unchecked_reports' + @user.reports.unchecked.not_wip.list.page(params[:page]) + else + @user.reports.list.page(params[:page]) + endフィルタ実装例（このコントローラに追加。既に共通実装があればそちらに寄せる）:
# private の中に追加 def require_admin_or_mentor_login return if current_user&.admin_or_mentor? respond_to do |format| format.html { redirect_to user_reports_path(@user, target: 'all_reports'), alert: '管理者・メンターとしてログインしてください' } format.json { head :forbidden } end end補足: グローバルでログイン必須なら
current_userの nil は起きませんが、そうでない場合は上記のようにセーフナビゲーションで防御しておくと安全です。#!/bin/bash # 直近の UI 要件に合うか、HTML/JSON のレスポンス分岐箇所をざっと確認 rg -n -C2 -g 'app/controllers/**' "respond_to\\s+do\\s+\\|format\\|" # user_reports_path の存在確認（パス名が異なる場合は適宜置換） rg -n -C2 -g 'config/routes*.rb' "resources\\s*:users"
🧹 Nitpick comments (2)
app/controllers/users/reports_controller.rb (2)
6-6: 条件は params ではなく @target を参照して一元化を
set_targetでの解釈結果に依存させると、将来params[:target]を正規化・別名対応した際も挙動がズレません。併せて、認可は before_action に集約し、業務ロジック（一覧取得）からは外すと責務が明確になります。適用差分:
- before_action :require_admin_or_mentor_login, if: -> { params[:target] == 'unchecked_reports' } + before_action :require_admin_or_mentor_login, if: -> { @target == 'unchecked_reports' }
47-49: target パラメータのホワイトリスト化（無効値は all_reports にフォールバック）予期しない値が来ても安全側に倒すようにしておくと将来の変更に強くなります。
適用差分:
- def set_target - @target = params[:target] || 'all_reports' - end + def set_target + allowed = %w[all_reports unchecked_reports] + @target = allowed.include?(params[:target]) ? params[:target] : 'all_reports' + end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
app/controllers/api/reports_controller.rb(1 hunks)app/controllers/users/reports_controller.rb(2 hunks)app/javascript/components/Reports.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/controllers/api/reports_controller.rb
- app/javascript/components/Reports.jsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/controllers/users/reports_controller.rb
🧬 Code graph analysis (1)
app/controllers/users/reports_controller.rb (2)
app/models/user.rb (1)
admin_or_mentor?(662-664)app/controllers/reports_controller.rb (1)
set_report(106-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (2)
app/controllers/users/reports_controller.rb (2)
5-5: LGTM: set_target を最初に実行する順序は適切です後続のフィルタや分岐が
@targetに依存するため、読みやすさ・保守性の観点でも良い並びです。
6-6: require_admin_or_mentor_login は既に定義・インクルード済みです
require_admin_or_mentor_loginは
•app/controllers/concerns/authentication/access_requirements.rb内のAuthentication::AccessRequirementsモジュールで定義されており
•app/controllers/concerns/authentication.rb（7行目）でAuthentication::AccessRequirementsをインクルード
•app/controllers/application_controller.rb（2行目）でAuthenticationモジュールをインクルードの流れで ApplicationController 配下にメソッドが組み込まれているため、
Users::ReportsControllerでも問題なく利用可能です。追加の実装作業は不要です。
unchecked_reportsのアクセス制御をhead :forbiddenからrequire_admin_or_mentor_loginに変更
hirokiej
force-pushed
the
feature/add-unchecked-reports-tab-for-mentor-only
branch
from
525bf87 to
4ead1d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/javascript/components/Reports.jsx (1)
27-29: practice_id の二重付与バグ解消を確認しました（userPracticeId 優先）
const pid = userPracticeId || practiceIdによる一本化で重複付与が解消され、レビュー指摘に対応できています。
🧹 Nitpick comments (4)
app/controllers/users/reports_controller.rb (3)
5-6: ターゲット判定は @target に統一し、メソッド化して可読性と保守性を向上させましょう
set_targetで正規化した値を使うほうが意図が明確です。条件はラムダではなく述語メソッド化するとテストもしやすくなります。適用例:
- before_action :require_admin_or_mentor_login, if: -> { params[:target] == 'unchecked_reports' } + before_action :require_admin_or_mentor_login, if: :unchecked_target?def set_target - @target = params[:target] || 'all_reports' + allowed = %w[all_reports unchecked_reports] + @target = allowed.include?(params[:target]) ? params[:target] : 'all_reports' end + + def unchecked_target? + @target == 'unchecked_reports' + end
35-39: 未チェック一覧の取得ロジックは妥当です（認可ガードと整合）
@user.reports.unchecked.not_wip.list.page(...)と通常一覧の分岐は明快で、上位のbefore_actionによる認可ガードとも整合しています。N+1 が気になる場合は必要に応じてincludes(...)を検討ください（画面で参照している関連に依存）。
46-48: target のホワイトリスト化で誤指定に強く任意文字列が来ても else 側に落ちるので致命傷ではないですが、明示的に許容値をホワイトリスト化しておくと将来の拡張や静的解析での検出がしやすくなります。上の提案と同じ差分で反映可能です。
app/javascript/components/Reports.jsx (1)
24-31: クエリの組み立ては URLSearchParams で安全・簡潔に文字列連結は可読性とエンコードの面で脆くなりがちです。
URLSearchParamsに寄せると安全・明瞭になります。- let reportsUrl = `/api/reports.json?page=${page}` - if (userId) reportsUrl += `&user_id=${userId}` - if (companyId) reportsUrl += `&company_id=${companyId}` - const pid = userPracticeId || practiceId - if (pid) reportsUrl += `&practice_id=${pid}` - if (unchecked) reportsUrl += `&target=unchecked_reports` + const params = new URLSearchParams({ page: String(page) }) + if (userId) params.set('user_id', String(userId)) + if (companyId) params.set('company_id', String(companyId)) + const pid = userPracticeId || practiceId + if (pid) params.set('practice_id', String(pid)) + if (unchecked) params.set('target', 'unchecked_reports') + const reportsUrl = `/api/reports.json?${params.toString()}`
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
app/controllers/api/reports_controller.rb(1 hunks)app/controllers/users/reports_controller.rb(2 hunks)app/javascript/components/Reports.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/api/reports_controller.rb
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/controllers/users/reports_controller.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (1)
app/javascript/components/Reports.jsx (1)
1-1: 不要な useEffect 削除に伴う import の整理、OKです
useStateのみの import にできており適切です。
hirokiej
left a comment
hirokiej
left a comment
There was a problem hiding this comment.
ご確認ありがとうございます🙇
修正しましたので、再度ご確認よろしくお願いいたします！
| render PageTabsComponent.new(tabs:, active_tab:) | ||
| end | ||
|
|
||
| def unchecked_report_badge(current_user:, user:) |
There was a problem hiding this comment.
確かに、その方がわかりやすくて良いと思いました。
修正します！
|
|
||
| def set_reports | ||
| @reports = user.reports.list.page(params[:page]) | ||
| @reports = case @target |
There was a problem hiding this comment.
caseからifを足したときにどうにかできないかな？と思ったのですが、==をを使えば、if/elseでいけましたね…！
修正します！
| @reports = user.reports.list.page(params[:page]) | ||
| @reports = case @target | ||
| when 'unchecked_reports' | ||
| head :forbidden unless current_user.admin_or_mentor? |
There was a problem hiding this comment.
別のページではどうなっているかを怠っていました💦
ユーザーフレンドリーではないかと思いました。🤔
それとは別でユーザーフレンドリーという部分の考えもなかったので勉強になりました🙇
修正します！
| = user_page_tabs(@user, active_tab: '日報') | ||
|
|
||
| = react_component('Reports', userId: @user.id, practices: @current_user_practice.sorted_practices) | ||
| - if mentor_login? |
There was a problem hiding this comment.
勝手な思い込みでadminはroleとしてメンターも含まれていると勘違いしておりました。
ご確認ありがとうございます🙇
修正します！
sjabcdefin
left a comment
sjabcdefin
left a comment
There was a problem hiding this comment.
修正お疲れさまです。😌🍵
すみません。追加で1点コメントさせていただきました。
ご確認お願いいたします。😌🙏
| tabs << { name: 'プロフィール', link: user_path(user) } | ||
| tabs << { name: 'ポートフォリオ', link: user_portfolio_path(user) } | ||
| tabs << { name: '日報', link: user_reports_path(user), count: user.reports.length } | ||
| tabs << { name: '日報', link: user_reports_path(user), count: user.reports.length, badge: unchecked_report_badge } |
There was a problem hiding this comment.
管理者・メンターユーザでメンターモードOFF時に、未チェック数バッチが表示されているようです。
管理者・メンターユーザでメンターモードOFF時は、未チェック数バッチが表示されない仕様だったと認識していますが、認識合っていますでしょうか？
ご確認いただけると幸いです。😌
(最初のレビューの際に気づけなくて失礼いたしました。）
There was a problem hiding this comment.
提出物の部分もメンタモードオフでバッヂが表示になっていたので、同様の対応をしました💦
勝手な推測になってしまうのですが、メンターモードオフだと確認が遅れてしまうということがあるのかな？と思いました。しかし、チームリーダーに確認すべきだと思ったので、komagataさんに確認してみます。
@komagata さん
お疲れ様です。
こちらの件ですが、提出物のタブではメンターモードオフの場合、バッヂが表示されていますが、未確認の日報数の場合は非表示にすべきでしょうか？
お手隙でご確認よろしくお願いいたします。
There was a problem hiding this comment.
勝手な推測になってしまうのですが、メンターモードオフだと確認が遅れてしまうということがあるのかな？と思いました。
こちらどういった意味でしょうか？
質問に回答するのにこの情報が必要かもなので先に聞いておきたいと思いました。
There was a problem hiding this comment.
@komagata さん
質問に回答するのにこの情報が必要かもなので先に聞いておきたいと思いました。
ユーザーとしては提出物や日報がチェックされていると安心感につながる部分だと感じています。
そのため、未チェックの日報数がadmin/mentorにはデフォルトで表示されている方が、未チェックに気づきやすく対応しやすいのではないか、という推測でした。
提出物のタブではメンターモードオフでも表示されているのに、日報では非表示にするのは統一感もないと感じ、提出物のタブに合わせる形で実装しています。
しかし、どちらが適切か判断がつかなかったため、改めて確認させていただきました🙇
There was a problem hiding this comment.
@hirokiej 返信ありがとうございます。
すみません、ちょっと僕の聞き方がわかりづらかったです。
僕がわからなかった部分は、
メンターモードオフだと確認が遅れてしまうということがあるのかな？
という部分です。「確認が遅れてしまう」というのはどういう意味でしょうか。
（「誰が」、「何を」、「なぜ」、遅れてしまうと考えたのでしょうか）
There was a problem hiding this comment.
@komagata さん
ご確認ありがとうございます。
「確認が遅れてしまう」というのはどういう意味でしょうか。
（「誰が」、「何を」、「なぜ」、遅れてしまうと考えたのでしょうか）
「メンターモードをオフにしているメンター」が「未チェックの日報の確認」を「メンターモードをオフにしている結果、バッヂが非表示になり認識しづらくなる」ため確認が遅れやすくなってしまうかも、という仮説として示したつもりでした。
こちらこそ言葉足らずで申し訳ございませんでした🙇💦
There was a problem hiding this comment.
@hirokiej なるほどですね。説明ありがとうございます。
こちらの件ですが、提出物のタブではメンターモードオフの場合、バッヂが表示されていますが、未確認の日報数の場合は非表示にすべきでしょうか？
メンターモードがオフでも未確認の日報数は非表示にしなくてOKです。
There was a problem hiding this comment.
@komagata さん
ご回答ありがとうございます🙇
@sjabcdefin さん
メンターモードがオフでも未確認の日報数は非表示にしなくてOKです。
確認したところ、表示した状態で大丈夫とのことでした🙇
There was a problem hiding this comment.
@hirokiej
ご確認ありがとうございます。😌🙏
私の方での確認は以上ですので、approveさせていただきます。🙆♀️
ご対応ありがとうございました！🎉
sjabcdefin
left a comment
sjabcdefin
left a comment
There was a problem hiding this comment.
すみません！
コメントだけして、Approve失念していました。
Approveさせていただきます。🙇♀️
|
@sjabcdefin @komagata さん |
komagata
deleted the
feature/add-unchecked-reports-tab-for-mentor-only
branch
|
@komagata さん |
|
本番で確認できましたー🎉🎉🎉 |
|
@machida さん |
5 participants
Issue
概要
変更確認方法
feature/add-unchecked-reports-tab-for-mentor-onlyをローカルに取り込むforeman start -f Procfile.dev1でローカル環境を立ち上げる。komagata、passwordtesttestでログインをする。i)タブの下に「全て」、「未チェック」のタブがあることを確認する。
ii)「日報」の右上に「未チェックの日報数のバッヂ」があることを確認する。
i)日報の隣のポートフォリオやコメントをクリックした際に「全て」、「未チェック」のタブが消えることを確認。
ii)日報のタブに戻ってから、メンターモードをオフにし、「全て」、「未チェック」のタブが消えることを確認。
Screenshot
変更前
変更後
Summary by CodeRabbit
新機能
変更
リファクタ