Conversation
|
""" Walkthroughこの変更は、チェック機能とチェック担当者（チェッカー）管理のUI・ロジックをVue.jsコンポーネントからRailsの部分テンプレートとコントローラに置き換え、関連するルーティング、コントローラ、JavaScript、テストを全面的に調整しています。UI・UXの一部はトースト通知や確認ダイアログで補強されました。 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant RailsApp
participant DB
User->>Browser: チェック/チェッカー操作ボタンをクリック
Browser->>RailsApp: チェック/チェッカー操作のHTTPリクエスト送信
RailsApp->>DB: チェック・チェッカー情報を作成/更新/削除
DB-->>RailsApp: 結果返却
RailsApp-->>Browser: リダイレクト or レスポンス
Browser->>Browser: sessionStorageにフラグ設定 & ページリロード
Browser->>User: トースト通知表示
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes 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/components/products/product_component.rbrubocop-minitest extension supports plugin, specify config/routes/reports.rbrubocop-minitest extension supports plugin, specify config/routes/products.rbrubocop-minitest extension supports plugin, specify
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (26)
💤 Files with no reviewable changes (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (20)
🧰 Additional context used📓 Path-based instructions (2)**/*.rb⚙️ CodeRabbit Configuration File
Files:
test/**/*⚙️ CodeRabbit Configuration File
Files:
🧠 Learnings (2)📓 Common learningstest/components/product/product_component_test.rb (2)Learnt from: kitarou888 Learnt from: ryufuta 🧬 Code Graph Analysis (1)test/components/product/product_component_test.rb (2)
⏰ 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)
🔇 Additional comments (1)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/javascript/check_confirmation.js (1)
17-24: コメント数の取得ロジックを改善できますコメント数を取得するセレクタが脆弱です。要素が見つからない場合やinnerHTMLが数値でない場合の処理を強化することをお勧めします。
- const commentCountElement = document.querySelector( - 'a[href="#comments"] > span' - ) - let hasComment = false - if (commentCountElement) { - const numberOfComments = parseInt(commentCountElement.innerHTML, 10) - hasComment = numberOfComments > 0 - } + const commentCountElement = document.querySelector( + 'a[href="#comments"] > span' + ) + let hasComment = false + if (commentCountElement) { + const numberOfComments = parseInt(commentCountElement.textContent, 10) + hasComment = !isNaN(numberOfComments) && numberOfComments > 0 + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
app/components/products/product_component.html.slim(1 hunks)app/components/products/product_component.rb(1 hunks)app/controllers/checks_controller.rb(1 hunks)app/controllers/products/self_assigned_controller.rb(1 hunks)app/controllers/products/unassigned_controller.rb(1 hunks)app/controllers/products/unchecked_controller.rb(1 hunks)app/controllers/products_controller.rb(2 hunks)app/javascript/check.js(0 hunks)app/javascript/check.vue(0 hunks)app/javascript/check_confirmation.js(1 hunks)app/javascript/packs/application.js(2 hunks)app/javascript/product.vue(0 hunks)app/javascript/product_checker.vue(0 hunks)app/views/products/_product_body.html.slim(1 hunks)app/views/products/_product_list.html.slim(1 hunks)app/views/products/index.html.slim(1 hunks)app/views/products/self_assigned/index.html.slim(1 hunks)app/views/products/unassigned/index.html.slim(1 hunks)app/views/products/unchecked/index.html.slim(1 hunks)app/views/reports/_report_body.html.slim(1 hunks)app/views/shared/_check_actions.html.slim(1 hunks)config/routes/products.rb(1 hunks)config/routes/reports.rb(1 hunks)test/system/products_test.rb(3 hunks)
💤 Files with no reviewable changes (4)
- app/javascript/product.vue
- app/javascript/check.js
- app/javascript/check.vue
- app/javascript/product_checker.vue
🔇 Additional comments (38)
app/components/products/product_component.rb (1)
18-20: 実装が適切です既存の
role_classメソッドと一貫性があり、ユーザーのロールに基づくCSSクラス生成の柔軟性を提供しています。コメント一覧でのユーザーアイコン表示などで活用されるものと思われます。app/javascript/packs/application.js (2)
15-15: Vue コンポーネントから確認ダイアログへの移行
check.jsからcheck_confirmation.jsへの変更は、Vueコンポーネントを削除してシンプルな確認ダイアログに置き換える今回のリファクタリングと整合性があります。
82-82: トースト通知の追加
toast.jsの追加により、ユーザーへのフィードバック機能が強化されます。サーバーサイドレンダリングへの移行に伴い、適切なUXを提供するための変更と考えられます。app/views/products/unchecked/index.html.slim (1)
24-24: Reactコンポーネントからサーバーサイドレンダリングへの移行の検証結果
- app/controllers/products/unchecked_controller.rb:14 で
@products = … .page(params[:page]).per(50)が設定されている- app/views/products/_product_list.html.slim:99 で
= paginate @productsが呼ばれているこれにより、サーバーサイドでのページネーション実装が正しく組み込まれていることを確認しました。変更を承認します。
app/views/products/self_assigned/index.html.slim (1)
17-18: サーバーサイドレンダリングとページネーションの実装Reactコンポーネントからサーバーサイドパーシャルへの移行が適切に実装されています。
@products.respond_to?(:current_page)を使った条件付きページネーション表示は堅実なアプローチです。app/views/reports/_report_body.html.slim (1)
27-27: チェック機能の共通パーシャル化インラインのVue/Reactコンポーネントから共通パーシャル
shared/check_actionsへの移行により、レポートと提出物間でのチェックUI の一貫性と保守性が向上しています。ポリモーフィックな実装に適したパラメータ設計です。app/controllers/products/unassigned_controller.rb (1)
7-11: LGTM: サーバーサイドレンダリングへの移行に対応した適切な実装Vue.jsコンポーネントからサーバーサイドレンダリングへの移行に伴い、ページネーション機能が適切に追加されています。1ページあたり50件の表示設定も妥当です。
config/routes/reports.rb (1)
8-10: LGTM: RESTfulなネストルートの適切な実装reportsリソース配下にchecksリソースをネストし、create/destroyアクションのみに制限する設計は、チェック機能の要件に適合しており、Rails慣例に従った適切な実装です。
app/views/products/index.html.slim (1)
15-18: LGTM: React コンポーネントからサーバーサイドレンダリングへの適切な移行Reactコンポーネントの削除とサーバーサイドパーシャルレンダリングへの移行が適切に実装されています。CSSクラスの追加とページネーション機能の統合も正しく行われています。
app/controllers/products/unchecked_controller.rb (1)
10-15: LGTM: 適切なスコープ構築とページネーション実装checker_idによる条件付きフィルタリングが適切に実装され、スコープ構築とクエリ実行の責任分離も適切です。他のコントローラーとの一貫性も保たれています。
app/views/products/unassigned/index.html.slim (1)
15-16: LGTM: 一貫したサーバーサイドレンダリング実装Reactコンポーネントからパーシャルレンダリングへの移行が適切に実装されており、
respond_to?(:current_page)による防御的プログラミングも評価できます。他のビューとの一貫性も保たれています。config/routes/products.rb (1)
10-16: 新しいルート定義が適切に設計されていますVue.jsコンポーネントを削除した代わりに追加されたルート定義は、RESTfulな設計に従っており適切です。メンバールート（assign_checker、unassign_checker）とネストしたリソース（checks）の組み合わせにより、担当者の割り当て/解除とチェック機能を適切に分離できています。
app/views/products/_product_body.html.slim (1)
36-36: 共有パーシャルへの置き換えが適切ですVue.jsコンポーネントを削除し、
shared/check_actionsパーシャルに置き換えたことで、チェック機能のUIロジックが集約され、保守性が向上しています。パラメータ名も明確で、Rails の慣習に従っています。app/controllers/products_controller.rb (3)
8-13: index アクションの実装が適切ですページネーション機能が適切に実装されており、50件ずつの表示制限とIDによる並び順も妥当です。Reactコンポーネントからサーバーサイドレンダリングへの移行に必要な変更が正しく行われています。
83-87: 担当者割り当て機能が適切に実装されています
assign_checkerアクションはシンプルで明確な実装になっています。update!を使用することで例外処理も適切に行われ、redirect_backでユーザビリティも考慮されています。
89-93: 担当者解除機能が適切に実装されています
unassign_checkerアクションも同様にシンプルで明確です。checkerをnilに設定することで担当者を適切に解除し、適切な通知メッセージも表示されます。test/system/products_test.rb (3)
55-55: ボタンの存在チェックがより正確になりました
assert_textからpage.has_button?への変更により、ボタン要素の存在チェックがより正確になっています。サーバーサイドレンダリングへの移行に合わせた適切な修正です。
372-372: 担当ボタンのテストが改善されました担当ボタンの存在確認が
page.has_button?を使用することで、より信頼性の高いテストになっています。
380-380: ページネーションのテストが更新されましたページネーションリンクのセレクターが
buttonからa要素に変更されており、サーバーサイドレンダリングされたページネーションの実装に正しく対応しています。Also applies to: 402-402
app/controllers/products/self_assigned_controller.rb (3)
6-6: パラメータバリデーションが改善されました
presence_inを使用したパラメータバリデーションにより、コードがより簡潔で読みやすくなりました。不正な値の場合のデフォルト値設定も適切です。
7-11: ページネーション機能が適切に実装されていますプライベートメソッドを使用したプロダクト取得とページネーション機能の組み合わせが適切です。他のコントローラーとの一貫性も保たれています。
20-30: プライベートメソッドの設計が優秀です
build_self_assigned_productsメソッドは単一責任の原則に従って設計されており、case文を使用したターゲット別の処理分岐も明確です。メソッドの分離により保守性が向上しています。app/javascript/check_confirmation.js (3)
5-9: 早期returnの実装が適切ですボタンが見つからない場合の早期returnが正しく実装されており、エラーを防いでいます。
28-37: 確認ダイアログの条件ロジックが正確ですsad感情、コメントなし、新規チェックの3つの条件を適切に組み合わせており、ビジネスロジックが正しく実装されています。
11-12: 検証結果：クローンによる競合は発生しません
#js-shortcut-checkは以下の 3 箇所でのみ使用されています
- app/views/shared/_check_actions.html.slim（ボタン要素の定義）
- app/javascript/shortcut.js（ショートカットキー押下時の
button.click()呼び出し）- app/javascript/check_confirmation.js（
cloneNode→replaceChild→クリック確認用のイベント登録）- 他ライブラリや Turbo がこの要素に対して直接イベントリスナーをアタッチしておらず、
cloneNode(true)によって削除されるのは、元の要素に紐づくユーザー定義のリスナーのみです。- Rails の
form_with local: trueもデフォルトでは要素単位のイベントを登録しないため、フォーム送信挙動に影響はありません。以上より、この実装で問題はないと判断しました。
app/controllers/checks_controller.rb (3)
3-4: 適切な認証が実装されていますスタッフのログインが必要な機能に対して適切な認証が実装されています。
9-13: 重複チェックの防止ロジックが優秀です同一ユーザーによる重複チェックを適切に防止し、わかりやすいメッセージを表示しています。
38-46: ポリモーフィック関係の処理が適切ですfind_checkableメソッドでProductとReportの両方を適切に処理し、必要なパラメータが不足している場合の例外処理も実装されています。
app/views/shared/_check_actions.html.slim (4)
1-1: アクセス制御が適切に実装されています管理者またはメンターのみがチェック機能にアクセスできるよう適切に制限されています。
7-21: 担当者割り当てロジックが優れていますProduct特有の担当者割り当て機能が適切に実装されており、条件分岐も論理的です。担当者の表示もユーザーフレンドリーです。
33-35: ボタンのdata属性の使用法が正しいですJavaScriptでの処理に必要なdata-has_check属性が適切に設定されており、check_confirmation.jsとの連携が取れています。
25-35: ルーティング設定の確認結果
config/routes/reports.rb および config/routes/products.rb にて、それぞれresources :checks, only: %i[create destroy]がネスト定義されているため、
polymorphic_path([checkable, check])およびpolymorphic_path([checkable, :checks])の使用は問題ありません。
修正は不要です。app/views/products/_product_list.html.slim (3)
68-73: ユーザーアイコンの部分テンプレート使用が改善されていますユーザーアイコンの表示で部分テンプレートとcachedオプションを使用しており、パフォーマンスとメンテナンス性が向上しています。
92-98: 合格スタンプの表示が直感的ですチェック済みの提出物に対する合格スタンプの表示が分かりやすく、日付とチェック者の情報も適切に表示されています。
2-2: N+1クエリの懸念は解消されていますapp/controllers/products_controller.rb の user_products メソッドで以下のように includes が使われており、関連データが事前読み込みされています。
- app/controllers/products_controller.rb: user_products メソッド内
.includes(:practice, :user, :comments, :checks, comments: :user)これにより、@products.each 内でのユーザー情報、コメント、チェック、練習情報へのアクセス時に N+1 クエリは発生しません。
app/components/products/product_component.html.slim (3)
1-1: メソッド呼び出しへのリファクタリングが優秀です
@product.wip?のような適切なメソッド呼び出しにリファクタリングされており、コードの可読性が向上しています。Also applies to: 13-13, 30-30
5-5: ユーザーアイコンの部分テンプレート使用が改善されていますユーザーアイコンの表示を部分テンプレートで統一しており、メンテナンス性が向上しています。
70-70: ヘルパーメソッドの使用が適切です
role_class_for_user(user)ヘルパーメソッドを使用してCSSクラスを動的に決定しており、ロジックの分離が適切に行われています。
| def destroy | ||
| @check = Check.find(params[:id]) | ||
| @checkable = @check.checkable | ||
|
|
||
| @check.destroy | ||
| redirect_back(fallback_location: @checkable) | ||
| end |
There was a problem hiding this comment.
destroyアクションのセキュリティを強化してください
destroyアクションで、現在のユーザーがそのチェックの所有者であることを確認する必要があります。
def destroy
@check = Check.find(params[:id])
+
+ unless @check.user == current_user || current_user.admin?
+ redirect_back(fallback_location: root_path, alert: 'このチェックを削除する権限がありません。')
+ return
+ end
+
@checkable = @check.checkable
@check.destroy
redirect_back(fallback_location: @checkable)
end📝 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 destroy | |
| @check = Check.find(params[:id]) | |
| @checkable = @check.checkable | |
| @check.destroy | |
| redirect_back(fallback_location: @checkable) | |
| end | |
| def destroy | |
| @check = Check.find(params[:id]) | |
| unless @check.user == current_user || current_user.admin? | |
| redirect_back(fallback_location: root_path, alert: 'このチェックを削除する権限がありません。') | |
| return | |
| end | |
| @checkable = @check.checkable | |
| @check.destroy | |
| redirect_back(fallback_location: @checkable) | |
| end |
🤖 Prompt for AI Agents
In app/controllers/checks_controller.rb around lines 28 to 34, the destroy
action lacks a check to verify that the current user owns the check before
allowing deletion. Add a condition to confirm that the current user is the owner
of @check before calling destroy. If the user is not the owner, prevent deletion
and handle the response appropriately, such as redirecting with an error message
or raising an authorization error.
| - if current_user.mentor? && product.checks.empty? | ||
| .card-list-item__row.is-only-mentor | ||
| .card-list-item__assignee | ||
| - if product.checker_id.nil? | ||
| = form_with url: assign_checker_product_path(product), method: :patch, local: true do |f| | ||
| = f.submit '担当する', class: 'a-button is-sm is-secondary is-block' | ||
| - elsif product.checker_id == current_user.id | ||
| = form_with url: unassign_checker_product_path(product), method: :delete, local: true do |f| | ||
| = f.submit '担当から外れる', class: 'a-button is-sm is-warning is-block' | ||
| - else | ||
| .a-button.is-sm.is-block.card-list-item__assignee-button.is-only-mentor | ||
| span.card-list-item__assignee-image | ||
| = image_tag product.checker.avatar_url, class: 'a-user-icon', width: 20, height: 20 | ||
| span.card-list-item__assignee-name = product.checker.login_name |
There was a problem hiding this comment.
🛠️ Refactor suggestion
担当者割り当てロジックが重複しています
この担当者割り当てのロジックはshared/check_actionsパーシャルと重複しています。コードの重複を避けるため、共通化を検討してください。
- - if current_user.mentor? && product.checks.empty?
- .card-list-item__row.is-only-mentor
- .card-list-item__assignee
- - if product.checker_id.nil?
- = form_with url: assign_checker_product_path(product), method: :patch, local: true do |f|
- = f.submit '担当する', class: 'a-button is-sm is-secondary is-block'
- - elsif product.checker_id == current_user.id
- = form_with url: unassign_checker_product_path(product), method: :delete, local: true do |f|
- = f.submit '担当から外れる', class: 'a-button is-sm is-warning is-block'
- - else
- .a-button.is-sm.is-block.card-list-item__assignee-button.is-only-mentor
- span.card-list-item__assignee-image
- = image_tag product.checker.avatar_url, class: 'a-user-icon', width: 20, height: 20
- span.card-list-item__assignee-name = product.checker.login_name
+ - if current_user.mentor? && product.checks.empty?
+ = render 'shared/check_actions', checkable: product, checkable_type: 'Product', checkable_label: '提出物', display_only_checker_assignment: true📝 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.
| - if current_user.mentor? && product.checks.empty? | |
| .card-list-item__row.is-only-mentor | |
| .card-list-item__assignee | |
| - if product.checker_id.nil? | |
| = form_with url: assign_checker_product_path(product), method: :patch, local: true do |f| | |
| = f.submit '担当する', class: 'a-button is-sm is-secondary is-block' | |
| - elsif product.checker_id == current_user.id | |
| = form_with url: unassign_checker_product_path(product), method: :delete, local: true do |f| | |
| = f.submit '担当から外れる', class: 'a-button is-sm is-warning is-block' | |
| - else | |
| .a-button.is-sm.is-block.card-list-item__assignee-button.is-only-mentor | |
| span.card-list-item__assignee-image | |
| = image_tag product.checker.avatar_url, class: 'a-user-icon', width: 20, height: 20 | |
| span.card-list-item__assignee-name = product.checker.login_name | |
| - if current_user.mentor? && product.checks.empty? | |
| = render 'shared/check_actions', | |
| checkable: product, | |
| checkable_type: 'Product', | |
| checkable_label: '提出物', | |
| display_only_checker_assignment: true |
🤖 Prompt for AI Agents
In app/views/products/_product_list.html.slim lines 77 to 90, the checker
assignment logic duplicates the code found in the shared/check_actions partial.
To fix this, extract the duplicated logic into a single shared partial or helper
and replace the current code with a call to that shared partial, ensuring the UI
and functionality remain consistent while avoiding code duplication.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/javascript/comment-checkable.js (2)
63-64: sessionStorageとページリロードによる通知連携が機能的に正しく実装されています確認操作成功後にsessionStorageフラグを設定してページをリロードする実装は、new-comment.jsでの通知表示と適切に連携しています。ただし、ページリロードによるアプローチはUXの観点で改善の余地があります。
将来的にはページリロードを避けてDOMの部分更新を行うアプローチを検討することをお勧めします：
- sessionStorage.setItem('showCheckToast', 'true') - location.reload() + // DOM更新とトースト通知を直接実行 + updateCheckUIElements() + toast(getCheckToastMessage(checkableType))
88-90: 担当者割り当て後の処理が適切に実装されていますsessionStorageフラグの設定とページリロードにより、担当者割り当て後の通知が適切に処理されています。check操作と同様のパターンで一貫性があります。
check操作と同様に、将来的にはページリロードを避けた実装を検討することをお勧めします。また、alert()の使用も統一されたトースト通知に置き換えることを検討してください：
- alert('提出物の担当になりました。') - - const event = new Event('checkerAssigned') - window.dispatchEvent(event) - - sessionStorage.setItem('showAssignedToast', 'true') - - location.reload() + // DOM更新とトースト通知を直接実行 + updateCheckerUIElements() + toast('担当になりました。') + + const event = new Event('checkerAssigned') + window.dispatchEvent(event)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/javascript/comment-checkable.js(2 hunks)app/javascript/new-comment.js(2 hunks)app/views/comments/_comments.html.slim(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/views/comments/_comments.html.slim
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tyrrell-IH
PR: fjordllc/bootcamp#8807
File: app/views/welcome/job_support.html.slim:391-392
Timestamp: 2025-06-21T22:30:20.116Z
Learning: このプロジェクトにはRailsの組み込みmarkdownヘルパが利用できないため、markdownのHTML変換には自前でヘルパを実装する必要がある。
🧬 Code Graph Analysis (1)
app/javascript/new-comment.js (1)
app/javascript/vanillaToast.js (1)
toast(3-13)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (2)
app/javascript/new-comment.js (2)
11-25: sessionStorageを使用したトースト通知の実装が適切ですページロード時にsessionStorageのフラグを確認してトースト通知を表示する実装は、Vue コンポーネントからの移行において適切なアプローチです。担当者の割り当てと確認済み操作の両方に対応し、コメント対象の種類に応じて適切なメッセージを表示しています。
206-208: 重複通知を防ぐ条件分岐が適切です担当者の割り当てや確認済み操作が発生した場合にトースト通知を抑制する条件分岐により、重複した通知を防いでいます。これにより、ユーザーエクスペリエンスが向上します。
|
@kitarou888 |
|
@su-su-su-su |
|
@su-su-su-su |
kitarou888
left a comment
kitarou888
left a comment
There was a problem hiding this comment.
@su-su-su-su
ひととおりチェック完了しました！
動作確認、コードともに問題ありませんでした。
今回の非Vueにおいて必要なDiffなのか、リファクタリングなのかがちょっと判断できなかったため、コメントに残させていただきましたのでご確認ください🙏
| span.a-meta__value = "約#{until_next_elapsed_days}時間" | ||
|
|
||
| - if @product.comments.size > 0 | ||
| - if @product.comments.any? |
There was a problem hiding this comment.
こちらは今回の移行に必要な変更ですか？それともただのリファクタリングですか？
There was a problem hiding this comment.
エラーが出て色々修正しており、その変更したコードが残ってしまっていたのと思います。
不要なので元のコードに戻しました。
| .card-list-item-meta__items | ||
| .card-list-item-meta__item | ||
| - if @product.user.training_ends_on | ||
| .card-list-item-meta |
There was a problem hiding this comment.
この行が追加されていることで以降のネストが1段深くなっていますが、こちらは意図したものでしょうか？
There was a problem hiding this comment.
こちらについても同様でエラーが出て色々修正しており、その変更したコードが残ってしまっていたのと思います。
不要なので元のコードに戻しました。
| span.a-meta__label | ||
| | 提出 | ||
| = l(@product.published_at) | ||
| span.a-meta__label 提出 | ||
| span.a-meta__value = l(@product.published_at) |
There was a problem hiding this comment.
これと似たdiffが多く見られますが、今回の移行において必要な変更でしょうか？
それともリファクタリングでしょうか？
There was a problem hiding this comment.
こちらも不要なので元のコードに戻しました。
| const event = new Event('checkerAssigned') | ||
| window.dispatchEvent(event) | ||
|
|
||
| sessionStorage.setItem('showAssignedToast', 'true') |
There was a problem hiding this comment.
ここでsessionStorageを使用されている意図について、教えていただけるとうれしいです。（次のnew-comment.jsについても同様）
There was a problem hiding this comment.
Vue.js（非同期）からRailsへの移行によりページリロードが必要となったため、リロードを跨いでtoastの表示状態を保持する目的で使用しています。
| select 'komagata', from: 'product_checker_id' | ||
| click_button '提出する' | ||
| assert_text '担当から外れる' | ||
| assert page.has_button?('担当から外れる') |
There was a problem hiding this comment.
このdiffは今回のイシューで必要になった変更ですか？それともリファクタリングでしょうか？（他も同様）
There was a problem hiding this comment.
assert_text '担当から外れる' のテストが失敗しており、原因を確認したところassert_textではボタンの文言をうまく認識できていないようでした。
Notification::AssignedAsCheckerTest#test_not_notice_self_assigned_as_checker [test/system/notification/assigned_as_checker_test.rb:41]:
expected to find text "担当から外れる"
assert page.has_button?('担当から外れる')に修正することで、テストが成功しました。
また他のコードを確認するとassert page.has_button?は使われていなかったので、assert_buttonに変更しました。
|
@kitarou888 内容確認しました。すぐに対応できず申し訳ありません。 少しお時間いただきますが、よろしくお願いいたします🙇 |
su-su-su-su
force-pushed
the
chore/remove-vue-check-component
branch
from
42112ad to
488660a
Compare
|
@kitarou888 ご指摘頂いたコンフリクトを解決、コメントに対しての修正を行いました。 |
kitarou888
left a comment
kitarou888
left a comment
There was a problem hiding this comment.
@su-su-su-su
とくに問題は見当たりませんでしたのでApproveにしました！
1点だけコメントしましたので、必要に応じて修正をお願いできればと思います。
app/javascript/check_confirmation.js
Outdated
| if (hasSadEmotion && !hasComment && isNewCheck) { | ||
| if ( | ||
| !window.confirm( | ||
| '今日の気分は「sad」ですが、コメント無しで確認しますか？' | ||
| ) | ||
| ) { | ||
| event.preventDefault() | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
ここのところ、
- if文をネストしている点
- 条件式の改行が多い点（以下1行にまとめても62文字でした）
if (!window.confirm('今日の気分は「sad」ですが、コメント無しで確認しますか？')) {
event.preventDefault()
return false
}
が気になりました。
if文のネストについてはこのように分けている理由があれば教えていただけるとうれしいです。
There was a problem hiding this comment.
ご指摘ありがとうございます。
削除したapp/javascript/check.vueの記述に合わせた形になります。
ご指摘の通りまとめることも可能だと思いますが、どちらが良いでしょうか？
There was a problem hiding this comment.
@su-su-su-su
ご判断はお任せします！
が、もし私だったら今回のケースはリファクタリングによってDiffが出るわけではないですので、明らかに汚いコードがあれば直してしまうと思います！
There was a problem hiding this comment.
@kitarou888
なるほどですね！
こちらも修正しました！
if (
hasSadEmotion &&
!hasComment &&
isNewCheck &&
!window.confirm('今日の気分は「sad」ですが、コメント無しで確認しますか？')
) {
event.preventDefault()
return false
}
アドバイスありがとうございました！
|
@kitarou888 |
su-su-su-su
force-pushed
the
chore/remove-vue-check-component
branch
from
144be0b to
b2f42a3
Compare
|
@komagata お手すきの際にご確認お願いいたします。 |
|
Issueのタイトルをわかりやすいもの（日本語）に修正してください。 |
|
@komagata |
config/routes/products.rb
Outdated
| patch :assign_checker | ||
| delete :unassign_checker |
There was a problem hiding this comment.
極力RESTfulなURL設計だけを使いたい感じです。
There was a problem hiding this comment.
ご指摘ありがとうございます。
新しくapp/controllers/products/checker_assignments_controller.rbを作成して、RESTfulなルートに修正しました。
su-su-su-su
force-pushed
the
chore/remove-vue-check-component
branch
from
b2f42a3 to
6fc6975
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/controllers/products/checker_assignments_controller.rb (1)
18-21: パラメータの処理方法と認可について検討が必要です。現在の実装では
product_idとidの両方をチェックしていますが、以下の点を検討してください：
- 両方のパラメータをチェックする理由を明確にする
- プロダクトにアクセスする権限があるかの認可チェック
- エラーハンドリングの一貫性
以下のような改善を検討してください：
def find_product product_id = params[:product_id] || params[:id] @product = Product.find(product_id) + authorize! :read, @product +rescue ActiveRecord::RecordNotFound + redirect_to products_path, alert: '指定されたプロダクトが見つかりません。' endまた、パラメータの使い分けについてコメントを追加することを推奨します。
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
app/components/products/product_component.html.slim(3 hunks)app/components/products/product_component.rb(1 hunks)app/controllers/checks_controller.rb(1 hunks)app/controllers/products/checker_assignments_controller.rb(1 hunks)app/controllers/products/self_assigned_controller.rb(1 hunks)app/controllers/products/unassigned_controller.rb(1 hunks)app/controllers/products/unchecked_controller.rb(1 hunks)app/controllers/products_controller.rb(2 hunks)app/javascript/check.js(0 hunks)app/javascript/check.vue(0 hunks)app/javascript/check_confirmation.js(1 hunks)app/javascript/comment-checkable.js(2 hunks)app/javascript/new-comment.js(2 hunks)app/javascript/packs/application.js(2 hunks)app/javascript/product_checker.vue(0 hunks)app/views/products/_product_body.html.slim(1 hunks)app/views/products/self_assigned/index.html.slim(1 hunks)app/views/reports/_report_body.html.slim(1 hunks)app/views/shared/_check_actions.html.slim(1 hunks)config/routes/products.rb(1 hunks)config/routes/reports.rb(1 hunks)test/system/notification/assigned_as_checker_test.rb(1 hunks)test/system/product/checker_test.rb(1 hunks)test/system/products_test.rb(2 hunks)test/system/user/products_test.rb(1 hunks)
💤 Files with no reviewable changes (3)
- app/javascript/check.js
- app/javascript/check.vue
- app/javascript/product_checker.vue
🚧 Files skipped from review as they are similar to previous changes (21)
- app/javascript/packs/application.js
- test/system/user/products_test.rb
- app/views/products/self_assigned/index.html.slim
- test/system/notification/assigned_as_checker_test.rb
- test/system/product/checker_test.rb
- app/views/reports/_report_body.html.slim
- app/controllers/products/unassigned_controller.rb
- app/components/products/product_component.rb
- app/controllers/products/unchecked_controller.rb
- app/javascript/new-comment.js
- app/views/products/_product_body.html.slim
- app/controllers/products_controller.rb
- config/routes/reports.rb
- app/controllers/products/self_assigned_controller.rb
- test/system/products_test.rb
- app/javascript/comment-checkable.js
- app/javascript/check_confirmation.js
- app/controllers/checks_controller.rb
- config/routes/products.rb
- app/views/shared/_check_actions.html.slim
- app/components/products/product_component.html.slim
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
⏰ 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/controllers/products/checker_assignments_controller.rb (1)
3-4: クラス定義とbefore_actionの実装が適切です。Rails の慣習に従った適切なコントローラ構造になっています。before_action を使用してプロダクトの検索を共通化しているのも良いアプローチです。
|
@coderabbitai
を またユーザーが担当者になる権限があるかの認可チェック でチェックしていますがいかがでしょうか。 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@su-su-su-su ご質問ありがとうございます。確認させていただきます。 コントローラの実装を確認した結果、以下の点が懸念されます： セキュリティ上の問題点：
推奨される修正： class Products::CheckerAssignmentsController < ApplicationController
before_action :staff_login_required # または適切な認可メソッド
before_action :find_product
before_action :validate_mentor_permission
def create
# 既存の実装に加えて、二重チェックを追加
return redirect_back(fallback_location: @product, alert: 'この提出物は既にチェック済みです。') if @product.checks.any?
@product.update!(checker: current_user)
redirect_back(fallback_location: @product, notice: '担当になりました。')
end
def destroy
@product.update!(checker: nil)
redirect_back(fallback_location: @product, notice: '担当から外れました。')
end
private
def validate_mentor_permission
unless current_user.mentor? || current_user.admin?
redirect_back(fallback_location: root_path, alert: 'メンターまたは管理者のみ担当できます。')
end
end
endビューでの条件チェックはUIの表示制御のためのものであり、セキュリティの担保はコントローラレベルで行う必要があります。 🔒 セキュリティの原則：「信頼しないクライアント」
🏁 Script executed: Length of output: 584 🏁 Script executed: Length of output: 374 🏁 Script executed: Length of output: 498 🏁 Script executed: Length of output: 86 実装を確認した結果、以下の重要なセキュリティ問題が確認されました： 🚨 セキュリティ上の重大な問題現在の class Products::CheckerAssignmentsController < ApplicationController
before_action :find_product # 認可チェックなし！
def create
@product.update!(checker: current_user) # 誰でも担当者になれる
# ...
end
def destroy
@product.update!(checker: nil) # 誰でも担当者を外せる
# ...
end
end🔓 現在の脆弱性
✅ 必要な修正class Products::CheckerAssignmentsController < ApplicationController
before_action :staff_login_required # 認可チェック追加
before_action :find_product
before_action :ensure_mentor_or_admin # メンター/管理者のみ許可
def create
# 既にチェック済みの場合は拒否
if @product.checks.any?
redirect_back(fallback_location: @product, alert: 'この提出物は既にチェック済みです。')
return
end
@product.update!(checker: current_user)
redirect_back(fallback_location: @product, notice: '担当になりました。')
end
def destroy
@product.update!(checker: nil)
redirect_back(fallback_location: @product, notice: '担当から外れました。')
end
private
def find_product
product_id = params[:product_id] || params[:id]
@product = Product.find(product_id)
end
def ensure_mentor_or_admin
unless current_user.mentor? || current_user.admin?
redirect_back(fallback_location: root_path, alert: '権限がありません。')
end
end
endビューでの条件チェックは表示制御のみで、セキュリティの担保にはなりません。必ずコントローラレベルで認可チェックを実装してください。 🏁 Script executed: Length of output: 1525 🏁 Script executed: Length of output: 2185 🏁 Script executed: Length of output: 253 🏁 Script executed: Length of output: 2383 @su-su-su-su 認可システムを確認しました。以下が現状の問題点と解決策です： 🔍 現状の問題確認既存の認可システムが存在し、他のコントローラでは正しく使用されています： # 他のプロダクト関連コントローラは正しく認可チェックを実装
class Products::UncheckedController < ApplicationController
before_action :require_staff_login # ✅ 正しい実装
end
class Products::UnassignedController < ApplicationController
before_action :require_staff_login # ✅ 正しい実装
end
class Products::SelfAssignedController < ApplicationController
before_action :require_staff_login # ✅ 正しい実装
endしかし、 class Products::CheckerAssignmentsController < ApplicationController
before_action :find_product # ❌ 認可チェックなし！
end🚨 セキュリティ上の重大な脆弱性
✅ 必要な修正class Products::CheckerAssignmentsController < ApplicationController
before_action :require_staff_login # 🔒 この1行を追加するだけ
before_action :find_product
def create
@product.update!(checker: current_user)
redirect_back(fallback_location: @product, notice: '担当になりました。')
end
def destroy
@product.update!(checker: nil)
redirect_back(fallback_location: @product, notice: '担当から外れました。')
end
private
def find_product
product_id = params[:product_id] || params[:id]
@product = Product.find(product_id)
end
endたった1行の追加で、他のコントローラと同じレベルのセキュリティが確保されます。 🔐 セキュリティの原則
この修正により、既存のセキュリティ体系と一貫性を保ちながら、適切な認可制御が実現できます。 |
|
@komagata |
|
@su-su-su-su conflictの修正をお願いします。 |
su-su-su-su
force-pushed
the
chore/remove-vue-check-component
branch
2 times, most recently
from
0d62a1b to
1758f4a
Compare
|
@komagata |
su-su-su-su
force-pushed
the
chore/remove-vue-check-component
branch
from
2bd6bfb to
6a40438
Compare
|
@komagata |
3 participants
Issue
概要
app/javascript/check.vueapp/javascript/product_checker.vueを廃止するために、関係箇所をRailsとJSで動作するように置き換えました。
変更確認方法
chore/remove-vue-check-componentをローカルに取り込むforeman start -f Procfile.devでサーバーを立ち上げます。komagataでログインダッシュボードでの動作確認
課題一覧ページでの動作確認
日報確認機能の動作確認
Screenshot
動きは変更前と変わらないため、スクリーンショットはありません。
Summary by CodeRabbit
新機能
改善
バグ修正
リファクタリング