Conversation
Walkthrough複数の提出物・日報ビューで一括オープン機能を統一するため、ターゲットマッピング機構を導入。React の UnconfirmedLink コンポーネントを削除し、vanilla JavaScript 実装に置き換え。ヘルパーメソッドでターゲット値に応じた日本語ラベルを提供し、パーシャルで条件付きレンダリング実装。 Changes
Sequence DiagramsequenceDiagram
participant User as ユーザー<br/>(メンター)
participant Controller as コントローラー<br/>(e.g. ProductsController)
participant Helper as ヘルパー<br/>(ProductsHelper)
participant View as ビュー<br/>(.slim)
participant JS as JavaScript<br/>(unconfirmed-links-open.js)
participant Browser as ブラウザ<br/>(新規タブ)
User->>Controller: /products へアクセス
Controller->>Controller: set_target<br/>(`@target` = 'all')
Controller->>Controller: index<br/>(クエリ実行)
Controller->>View: `@target`, `@products`<br/>をテンプレートに渡す
View->>Helper: unconfirmed_links_label(`@target`)
Helper-->>View: 日本語ラベル<br/>('全ての提出物を...')
alt `@products.present`?
View->>View: unconfirmed_links_open<br/>パーシャル render
View->>JS: DOM 生成<br/>(js-unconfirmed-link)
end
View-->>User: HTML 返す<br/>(ボタン付き)
User->>JS: ボタンクリック
JS->>JS: document.querySelectorAll<br/>('js-unconfirmed-link')
loop 各リンク
JS->>Browser: window.open(href, '_blank')
end
Note over JS: 100ms 遅延
JS->>Browser: location.reload()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 重点確認項目：
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
sharoa119
force-pushed
the
chore/replace_vanilla_js_in_unconfirmed_link_jsx
branch
from
27f9f33 to
9a8b870
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 (2)
app/views/products/unchecked/index.html.slim (1)
25-26: LGTM! ただし、将来的なリファクタリングの検討を推奨条件付きレンダリングのロジックは正しく実装されています。ただし、同じ条件式が複数のビュー（index, unassigned, unchecked, self_assigned）で繰り返されています。
将来的には、ViewComponentパターンを使用して条件判定をコンポーネント内にカプセル化することを検討してください（コーディングガイドラインに沿った実装）。これにより、DRY原則に従いながら保守性が向上します。
コーディングガイドラインより: "Viewにpartialを作る場合はViewComponentを使うことを検討する。"
app/helpers/products_helper.rb (1)
6-11: 国際化(i18n)の検討を推奨ラベル文字列がハードコーディングされていますが、将来的な多言語対応を考慮すると、i18nファイルに移行することを推奨します。
例：
def unconfirmed_links_label(target) - case target - when 'all' then '全ての提出物を一括で開く' - when 'unchecked', 'unchecked_all' then '未完了の提出物を一括で開く' - when 'unchecked_no_replied' then '未返信の提出物を一括で開く' - when 'unassigned' then '未アサインの提出物を一括で開く' - when 'self_assigned', 'self_assigned_all' then '自分の担当の提出物を一括で開く' - when 'self_assigned_no_replied' then '未返信の担当提出物を一括で開く' - else '' - end + I18n.t("products.unconfirmed_links.#{target}", default: '') end対応するi18nファイル (
config/locales/ja.yml) に追加：ja: products: unconfirmed_links: all: '全ての提出物を一括で開く' unchecked: '未完了の提出物を一括で開く' unchecked_all: '未完了の提出物を一括で開く' unchecked_no_replied: '未返信の提出物を一括で開く' unassigned: '未アサインの提出物を一括で開く' self_assigned: '自分の担当の提出物を一括で開く' self_assigned_all: '自分の担当の提出物を一括で開く' self_assigned_no_replied: '未返信の担当提出物を一括で開く'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
app/controllers/products/unassigned_controller.rb(2 hunks)app/controllers/products/unchecked_controller.rb(2 hunks)app/controllers/products_controller.rb(2 hunks)app/helpers/products_helper.rb(1 hunks)app/javascript/components/Notifications.jsx(0 hunks)app/javascript/components/Product.jsx(1 hunks)app/javascript/components/Products.jsx(0 hunks)app/javascript/components/Reports.jsx(0 hunks)app/javascript/components/UnconfirmedLink.jsx(0 hunks)app/javascript/unconfirmed-links-open.js(1 hunks)app/views/notifications/index.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)test/models/product_test.rb(1 hunks)test/models/report_test.rb(1 hunks)test/system/product/products_view_system_test.rb(1 hunks)test/system/product/unconfirmed_links_open_test.rb(1 hunks)test/system/report/reports_view_system_test.rb(1 hunks)test/system/report/unconfirmed_links_open_test.rb(1 hunks)
💤 Files with no reviewable changes (4)
- app/javascript/components/Products.jsx
- app/javascript/components/Notifications.jsx
- app/javascript/components/UnconfirmedLink.jsx
- app/javascript/components/Reports.jsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/products/unchecked_controller.rbapp/helpers/products_helper.rbtest/models/report_test.rbtest/system/report/unconfirmed_links_open_test.rbtest/models/product_test.rbtest/system/report/reports_view_system_test.rbapp/controllers/products_controller.rbtest/system/product/unconfirmed_links_open_test.rbtest/system/product/products_view_system_test.rbapp/controllers/products/unassigned_controller.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/models/report_test.rbtest/system/report/unconfirmed_links_open_test.rbtest/models/product_test.rbtest/system/report/reports_view_system_test.rbtest/system/product/unconfirmed_links_open_test.rbtest/system/product/products_view_system_test.rb
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/unconfirmed-links-open.js
🧬 Code graph analysis (8)
app/controllers/products/unchecked_controller.rb (3)
app/controllers/products/unassigned_controller.rb (2)
before_action(3-20)set_target(17-19)app/controllers/products_controller.rb (2)
before_action(3-161)set_target(158-160)app/controllers/products/self_assigned_controller.rb (2)
before_action(3-31)target_allowlist(16-18)
test/system/report/unconfirmed_links_open_test.rb (3)
test/system/product/unconfirmed_links_open_test.rb (1)
setup(5-55)test/system/report/reports_view_system_test.rb (1)
setup(5-36)test/models/report_test.rb (1)
test(5-76)
test/models/product_test.rb (1)
app/models/product.rb (1)
self_assigned_no_replied_products(70-72)
test/system/report/reports_view_system_test.rb (2)
test/system/report/unconfirmed_links_open_test.rb (1)
setup(5-24)test/models/report_test.rb (1)
test(5-76)
app/controllers/products_controller.rb (3)
app/controllers/products/unassigned_controller.rb (2)
before_action(3-20)set_target(17-19)app/controllers/products/unchecked_controller.rb (2)
before_action(3-27)set_target(23-26)app/controllers/products/self_assigned_controller.rb (1)
before_action(3-31)
test/system/product/unconfirmed_links_open_test.rb (2)
test/system/product/products_view_system_test.rb (1)
setup(5-66)test/system/report/unconfirmed_links_open_test.rb (1)
setup(5-24)
test/system/product/products_view_system_test.rb (2)
test/system/product/unconfirmed_links_open_test.rb (1)
setup(5-55)test/models/product_test.rb (1)
test(5-288)
app/controllers/products/unassigned_controller.rb (2)
app/controllers/products/unchecked_controller.rb (2)
before_action(3-27)set_target(23-26)app/controllers/products_controller.rb (2)
before_action(3-161)set_target(158-160)
⏰ 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 (19)
app/controllers/products/unassigned_controller.rb (1)
5-19: LGTM!
before_action :set_targetの追加とset_targetメソッドの実装は、他のコントローラ(UncheckedController、ProductsController)と一貫したパターンに従っています。シンプルで明確な責務分離が保たれています。test/models/report_test.rb (1)
65-75: LGTM!
uncheckedスコープのユニットテストが適切に追加されています。テスト名が英語で記載されており、コーディングガイドラインに準拠しています。app/controllers/products_controller.rb (2)
7-7: LGTM!
before_action :set_target, only: %i[index]の追加により、indexアクションのみに適用範囲を限定しています。他のコントローラとの一貫性が保たれています。
157-160: LGTM!
set_targetメソッドの実装はシンプルで明確です。@targetの初期化により、ビューでの条件分岐に必要なコンテキストを提供しています。test/system/report/reports_view_system_test.rb (1)
1-36: LGTM!システムテストの実装が適切です:
- NotificationとWatchの副作用を適切にstub
- メンターの視点からの未チェックレポート表示を検証
js-unconfirmed-linkクラスとhref属性の存在を確認- テスト名が英語でコーディングガイドラインに準拠
app/controllers/products/unchecked_controller.rb (2)
5-5: LGTM!
before_action :set_targetの追加により、targetの初期化をindexアクションから分離し、他のコントローラとの一貫性が向上しています。
23-26: LGTM!
set_targetメソッドへのリファクタリングにより、関心の分離が改善されています。ロジックは変更されておらず、許可リストによる検証も適切に維持されています。test/models/product_test.rb (4)
254-258: LGTM!
uncheckedスコープのテストが適切に実装されています。テスト名が英語で記載され、checksが空であることとスコープへの包含を検証しています。
260-275: LGTM!
self_assigned_no_replied_productsのテストが包括的です:
- メンターに担当された提出物が含まれることを検証
- コメント追加後に除外されることを検証
- 動作の両面をカバーしています
277-281: LGTM!
unassignedスコープのテストが適切です。checker_idがnilであることとスコープへの包含を検証しています。
283-287: LGTM!
self_assigned_productスコープのテストが適切に実装されています。指定されたユーザーに担当された提出物が含まれることを検証しています。app/javascript/components/Product.jsx (1)
57-57: LGTM!
js-unconfirmed-linkクラスの追加により、システムテストとバニラJSの一括オープン機能が提出物リンクを識別できるようになります。ロジックやレンダリングに影響はありません。app/views/notifications/index.html.slim (1)
48-49: LGTM!未読通知の一括オープンUI追加が適切です:
- メンター権限、未読ステータス、未読通知の存在を適切に条件判定
- ラベルが明確で分かりやすい
- 他のビュー(products系)との一貫性が保たれています
app/views/products/unassigned/index.html.slim (1)
16-17: LGTM!条件付きレンダリングのロジックが正しく、
@productsの存在確認と@targetの値チェックが適切に行われています。ヘルパーメソッドを使用したラベル生成も適切です。app/javascript/unconfirmed-links-open.js (1)
11-11: セキュリティとブラウザ互換性の改善
window.openのターゲットを'_blank'に変更したことで、標準的な新規タブ/ウィンドウでの開き方になり、noopenerパラメータによりセキュリティも向上しています。以前の'_target'は非標準だったため、この変更は適切です。app/views/products/self_assigned/index.html.slim (1)
18-19: LGTM!実装は正しく、他のビューファイルと一貫性があります。DRY原則に関する改善提案は
app/views/products/unchecked/index.html.slimのレビューコメントを参照してください。app/views/products/index.html.slim (1)
17-18: LGTM!条件付きレンダリングが正しく実装されており、staff_login?ブロック内で適切に配置されています。
test/system/product/unconfirmed_links_open_test.rb (1)
39-54: テスト構造は適切テストケース名が英語で書かれており、コーディングガイドラインに準拠しています。カバレッジも適切です（ボタンの表示/非表示の両方のケースをテスト）。
ただし、Line 45のバグ修正が必要です（別のコメントを参照）。
test/system/report/unconfirmed_links_open_test.rb (1)
11-23: LGTM!日報に対する一括開封ボタンのテストが適切に実装されています。既存の
Report.uncheckedスコープを活用した効率的なテストです。テストケース名も英語で記述されており、コーディングガイドラインに準拠しています。
sharoa119
force-pushed
the
chore/replace_vanilla_js_in_unconfirmed_link_jsx
branch
2 times, most recently
from
d0b5e72 to
2461480
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/system/report/unconfirmed_links_open_test.rb (2)
37-39: 特定のレポートのみをチェック済みにする方が安全です。
Report.unchecked.find_eachは、fixtureに含まれる他の未チェックレポートもすべて対象にしてしまいます。テストの独立性を保つため、setupで作成した2つのレポートのみを明示的にチェック済みにする方が望ましいです。以下の差分を適用してください：
- Report.unchecked.find_each do |report| - report.checks.create!(user: @mentor) - end + @unchecked_report1.checks.create!(user: @mentor) + @unchecked_report2.checks.create!(user: @mentor)
49-50:waitオプションの追加を検討してください。Product側のテスト（
test/system/product/unconfirmed_links_open_test.rb）では.js-unconfirmed-linkセレクタにwait: 5オプションを使用している箇所があります。JavaScriptによる動的なレンダリングが発生する可能性がある場合、同様にwaitオプションを追加することでテストの安定性が向上します。以下のように修正できます：
- assert_selector "a.js-unconfirmed-link[href='#{report_path(@unchecked_report1)}']", count: 1 - assert_selector "a.js-unconfirmed-link[href='#{report_path(@unchecked_report2)}']", count: 1 + assert_selector "a.js-unconfirmed-link[href='#{report_path(@unchecked_report1)}']", count: 1, wait: 5 + assert_selector "a.js-unconfirmed-link[href='#{report_path(@unchecked_report2)}']", count: 1, wait: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/helpers/products_helper_test.rb(1 hunks)test/models/product_test.rb(1 hunks)test/system/product/unconfirmed_links_open_test.rb(1 hunks)test/system/report/unconfirmed_links_open_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/helpers/products_helper_test.rb
- test/models/product_test.rb
- test/system/product/unconfirmed_links_open_test.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:
test/system/report/unconfirmed_links_open_test.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/system/report/unconfirmed_links_open_test.rb
🧬 Code graph analysis (1)
test/system/report/unconfirmed_links_open_test.rb (2)
test/system/product/unconfirmed_links_open_test.rb (1)
setup(5-73)test/models/report_test.rb (1)
test(5-76)
⏰ 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
sharoa119
force-pushed
the
chore/replace_vanilla_js_in_unconfirmed_link_jsx
branch
3 times, most recently
from
21ab9b6 to
c87f685
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/views/products/self_assigned/index.html.slim (1)
18-19: ターゲットリストの範囲が広すぎる可能性があります。この自分の担当提出物ビューで、
'all'、'unchecked_all'、'unchecked_no_replied'、'unassigned'といったターゲットが含まれていますが、これらは self_assigned ビューには関連性が低いように見えます。このビューでは
'self_assigned'、'self_assigned_all'、'self_assigned_no_replied'のみをチェックする方が適切ではないでしょうか。以下の diff を適用して、ターゲットリストを絞り込むことを検討してください:
- - if @products.present? && %w[all unchecked_all unchecked_no_replied unassigned self_assigned self_assigned_all self_assigned_no_replied].include?(@target) + - if @products.present? && %w[self_assigned self_assigned_all self_assigned_no_replied].include?(@target) = render 'application/unconfirmed_links_open', label: unconfirmed_links_label(@target)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/javascript/components/Notifications.jsx(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)test/helpers/products_helper_test.rb(1 hunks)test/models/product_test.rb(1 hunks)test/system/product/unconfirmed_links_open_test.rb(1 hunks)test/system/report/unconfirmed_links_open_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- app/views/products/unassigned/index.html.slim
- app/javascript/components/Notifications.jsx
- test/models/product_test.rb
- app/views/products/unchecked/index.html.slim
- test/system/product/unconfirmed_links_open_test.rb
- app/views/products/index.html.slim
- test/system/report/unconfirmed_links_open_test.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:
test/helpers/products_helper_test.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/helpers/products_helper_test.rb
🧬 Code graph analysis (1)
test/helpers/products_helper_test.rb (1)
app/helpers/products_helper.rb (1)
unconfirmed_links_label(4-14)
⏰ 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/views/products/self_assigned/index.html.slim (1)
13-13: 削除されたタイトル設定の影響を確認してください。この行では
@target == 'self_assigned_no_replied'の場合にページタイトルを設定していましたが、削除されています。この削除が意図的なものか、またページタイトルの表示に影響がないか確認してください。test/helpers/products_helper_test.rb (1)
1-21: テストの実装が適切です！
unconfirmed_links_labelヘルパーのすべてのケースを網羅しており、未知のターゲットと nil の処理も確認されています。コーディングガイドラインに従って、英語のテストケース名を使用し、追加されたメソッドに対する単体テストが適切に実装されています。
sharoa119
force-pushed
the
chore/replace_vanilla_js_in_unconfirmed_link_jsx
branch
from
c87f685 to
24757b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/javascript/unconfirmed-links-open.js (1)
1-15: LGTM! セキュリティ対策が適切に実装されています。
window.openに'noopener'オプションを指定することで、開かれたウィンドウからwindow.openerへのアクセスを防ぎ、セキュリティリスクを軽減しています。ただし、ポップアップブロッカーによってリンクが開けない場合のユーザーフィードバックがありません。必要に応じて、以下のような処理を追加することを検討してください：
allOpenButton.addEventListener('click', () => { + let openedCount = 0 const links = document.querySelectorAll( '.card-list-item .js-unconfirmed-link' ) links.forEach((link) => { - window.open(link.href, '_blank', 'noopener') + const opened = window.open(link.href, '_blank', 'noopener') + if (opened) openedCount++ }) + if (openedCount === 0 && links.length > 0) { + alert('ポップアップがブロックされました。ブラウザの設定を確認してください。') + } })test/models/report_test.rb (1)
65-75: LGTM! 基本的なテストケースは適切です。
uncheckedスコープの基本動作を検証するテストが正しく実装されています。テストケース名も英語で記述されており、コーディングガイドラインに準拠しています。より堅牢なテストのために、チェック済みのレポートが
uncheckedスコープに含まれないことを検証するテストケースの追加を検討してください：test 'unchecked scope does not return reports with checks' do checked_report = reports(:report1) # Assuming this has checks assert_not_empty checked_report.checks assert_not_includes Report.unchecked, checked_report endtest/system/product/unconfirmed_links_open_test.rb (1)
55-72: wait パラメータの一貫性を確認してください。Lines 58 と 64 では
wait: 5を使用していますが、Line 70 では使用していません。動的コンテンツの読み込みタイミングが異なる可能性がありますが、一貫性のために Line 70-71 にもwait: 5を追加することを検討してください。以下のように修正することを検討してください：
test 'mentor sees self_assigned products links' do visit_with_auth '/products/self_assigned', 'komagata' - assert_selector "a.js-unconfirmed-link[href$='#{@self_assigned_product.id}']", count: 1 - assert_selector "a.js-unconfirmed-link[href$='#{@unchecked_no_replied_product.id}']", count: 1 + assert_selector "a.js-unconfirmed-link[href$='#{@self_assigned_product.id}']", count: 1, wait: 5 + assert_selector "a.js-unconfirmed-link[href$='#{@unchecked_no_replied_product.id}']", count: 1, wait: 5 end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
app/controllers/products/unassigned_controller.rb(2 hunks)app/controllers/products/unchecked_controller.rb(2 hunks)app/controllers/products_controller.rb(2 hunks)app/helpers/products_helper.rb(1 hunks)app/javascript/components/Notifications.jsx(1 hunks)app/javascript/components/Product.jsx(1 hunks)app/javascript/components/Products.jsx(0 hunks)app/javascript/components/Reports.jsx(0 hunks)app/javascript/components/UnconfirmedLink.jsx(0 hunks)app/javascript/unconfirmed-links-open.js(1 hunks)app/views/notifications/index.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)test/helpers/products_helper_test.rb(1 hunks)test/models/product_test.rb(1 hunks)test/models/report_test.rb(1 hunks)test/system/product/self_assigned_test.rb(1 hunks)test/system/product/unassigned_test.rb(1 hunks)test/system/product/unchecked_test.rb(1 hunks)test/system/product/unconfirmed_links_open_test.rb(1 hunks)test/system/products_test.rb(3 hunks)test/system/report/unconfirmed_links_open_test.rb(1 hunks)
💤 Files with no reviewable changes (3)
- app/javascript/components/Products.jsx
- app/javascript/components/UnconfirmedLink.jsx
- app/javascript/components/Reports.jsx
🚧 Files skipped from review as they are similar to previous changes (7)
- app/views/products/unassigned/index.html.slim
- app/controllers/products/unassigned_controller.rb
- app/controllers/products/unchecked_controller.rb
- app/helpers/products_helper.rb
- test/helpers/products_helper_test.rb
- test/system/report/unconfirmed_links_open_test.rb
- app/controllers/products_controller.rb
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:
test/models/report_test.rbtest/system/product/unchecked_test.rbtest/models/product_test.rbtest/system/products_test.rbtest/system/product/unassigned_test.rbtest/system/product/self_assigned_test.rbtest/system/product/unconfirmed_links_open_test.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/models/report_test.rbtest/system/product/unchecked_test.rbtest/models/product_test.rbtest/system/products_test.rbtest/system/product/unassigned_test.rbtest/system/product/self_assigned_test.rbtest/system/product/unconfirmed_links_open_test.rb
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/unconfirmed-links-open.js
🧬 Code graph analysis (6)
test/system/product/unchecked_test.rb (1)
test/supports/product_helper.rb (1)
delete_most_unchecked_products!(4-10)
app/javascript/unconfirmed-links-open.js (1)
app/javascript/user-icon.js (1)
link(7-7)
test/models/product_test.rb (1)
app/models/product.rb (1)
self_assigned_no_replied_products(70-72)
test/system/products_test.rb (2)
test/system/product/self_assigned_test.rb (1)
test(5-207)test/system/product/checker_test.rb (1)
test(5-78)
test/system/product/unassigned_test.rb (2)
test/system/product/self_assigned_test.rb (1)
test(5-207)test/supports/product_helper.rb (1)
delete_most_unassigned_products!(12-18)
test/system/product/unconfirmed_links_open_test.rb (4)
test/system/report/unconfirmed_links_open_test.rb (1)
setup(5-52)test/helpers/products_helper_test.rb (1)
test(5-21)test/system/product/self_assigned_test.rb (1)
test(5-207)test/system/products_test.rb (1)
test(5-731)
⏰ 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 (15)
app/javascript/components/Product.jsx (1)
57-57: LGTM! クラス追加により一括開封機能との連携が適切に実装されています。
js-unconfirmed-linkクラスの追加により、unconfirmed-links-open.jsのセレクタで正しく選択されるようになっています。既存の機能に影響を与えない適切な変更です。app/javascript/components/Notifications.jsx (1)
9-9: LGTM! React からバニラ JS への移行が適切に反映されています。
isMentorプロップの削除により、未確認リンクの一括開封機能がサーバーサイドレンダリングに移行したことが正しく反映されています。app/views/notifications/index.html.slimで条件付きレンダリングが行われるため、この変更は適切です。test/models/product_test.rb (1)
254-287: LGTM! スコープの動作を包括的にテストしています。追加された4つのテストケースは、各スコープの動作を適切に検証しています：
- unchecked scope: チェックのない提出物が正しく含まれることを確認
- self_assigned_no_replied_products: 担当メンターへの割り当て後、返信がない場合は含まれ、返信後は除外されることを確認（状態変化のテストとして特に優れています）
- unassigned scope: チェッカーが割り当てられていない提出物が含まれることを確認
- self_assigned_product scope: 特定のメンターに割り当てられた提出物が含まれることを確認
テストケース名も英語で記述されており、コーディングガイドラインに準拠しています。
app/views/products/unchecked/index.html.slim (1)
25-26: LGTM! 条件付きレンダリングが適切に実装されています。
@products.present?と@targetのホワイトリストチェックにより、適切な条件下でのみ一括開封ボタンが表示されます。unconfirmed_links_label(@target)ヘルパーの使用も一貫性があり、他のビューと整合性が取れています。app/views/notifications/index.html.slim (1)
48-49: LGTM! メンター向けの一括開封機能が適切に実装されています。3つの条件（
mentor_login?、params[:status] == 'unread'、current_user.notifications.unreads.any?）により、適切な状況でのみ一括開封ボタンが表示されます。未読通知が存在しない場合にボタンが表示されないことで、優れたユーザーエクスペリエンスを提供しています。app/views/products/index.html.slim (1)
17-18: LGTM! 一貫性のある実装です。条件付きレンダリングの実装が他の提出物ビュー（unchecked、unassigned、self_assigned）と一貫性があり、適切です。
@targetのホワイトリストチェックにより、対応するターゲットタイプの場合のみ一括開封機能が表示されます。app/views/products/self_assigned/index.html.slim (1)
18-19: LGTM!条件付きレンダリングのロジックは適切です。
@products.present?と@targetの値を確認し、該当する場合にunconfirmed_links_open部分テンプレートをレンダリングしています。test/system/product/self_assigned_test.rb (1)
28-39: LGTM!テストのリファクタリングは適切です。ウィンドウ操作を削除し、
js-unconfirmed-linkクラスを持つリンク要素の存在確認に変更しています。これは React から vanilla JS への移行に合致しています。test/system/products_test.rb (3)
285-286: LGTM!フォーム要素の読み込みを待つために
wait: 5を追加しています。動的コンテンツの読み込みを考慮した適切な変更です。
302-303: LGTM!Lines 285-286 と同様に、フォーム要素の読み込みを待つために
wait: 5を追加しています。
727-730: LGTM!一括で開くボタンの表示を確認する新しいテストが追加されています。テスト名は英語で記述されており、コーディングガイドラインに準拠しています。
test/system/product/unassigned_test.rb (1)
24-36: LGTM!テストのリファクタリングは他のテストファイルと一貫性があり、適切です。
js-unconfirmed-linkクラスを持つリンク要素の存在を確認するようになっています。test/system/product/unconfirmed_links_open_test.rb (2)
6-46: LGTM!setup ブロックは適切に構成されています。
Notification.stubとWatch.stubを使用して副作用を回避し、テストに必要な3つの提出物（未返信、未アサイン、自分が担当）を作成しています。
49-52: LGTM!一括で開くボタンの表示を確認するテストです。適切です。
test/system/product/unchecked_test.rb (1)
24-35: LGTM!テストのリファクタリングは他のテストファイル（unassigned_test.rb、self_assigned_test.rb）と一貫性があり、適切です。
js-unconfirmed-linkクラスを持つリンク要素の存在を確認するようになっています。
sharoa119
force-pushed
the
chore/replace_vanilla_js_in_unconfirmed_link_jsx
branch
from
24757b4 to
fc40e5c
Compare
sharoa119
changed the title
|
@tyrrell-IH さん |
sharoa119
requested review from
tyrrell-IH
and removed request for
okuramasafumi
|
@sharoa119 レビューやります！ もし「急ぎで〜」とかがあれば先におっしゃってください、その際には優先してレビューします |
|
@sharoa119 |
There was a problem hiding this comment.
@sharoa119 お疲れ様です！時間がかかって申し訳ございません。ようやくレビュー終わりました。
私も知識がないので自信はないですが、ReactからVanillaJSへの置き換えは上手くできているのではないかと思いました。
以下レビューしていて思ったことがあるので書いておきます。
動作確認手順について
動作確認手順をもう少し詳しく書いて欲しかったです。
決められた手順がないので、自分でコードを読んでから手順を考えないといけなかったのがちょっと大変でした💦
警告メッセージの適切な表示
Issueの注意点に「警告メッセージの適切な表示」とあり、sharoaさんのdescriptionにも
警告メッセージの表示確認済み
とありましたが、それがどの部分の動作を指すのかわかりませんでした。なので警告メッセージの動作確認はできていません。この辺をちょっと教えていただきたいです。
挙動についてのところ
descriptionの「挙動について」のところ
一括で開封した直後は「一括で開く」ボタンは画面上に残ります。
ただし「全て」タブを再選択すると非表示になります。
これは、React 版で行っていたような即時非表示の挙動を再現するには状態管理や非同期処理が必要となるため、今回の移行では対応していません。
今回の issue の趣旨（状態管理を使わず、バニラ JS でシンプルに移行する）に沿った実装です。
と書いていますが、
例えば通知機能だけでよければ
//app/javascript/unconfirmed-links-open.js
document.addEventListener('DOMContentLoaded', () => {
const allOpenButton = document.querySelector(
'#js-shortcut-unconfirmed-links-open'
)
if (allOpenButton) {
allOpenButton.addEventListener('click', () => {
const links = document.querySelectorAll(
'.card-list-item .js-unconfirmed-link'
)
links.forEach((link) => {
window.open(link.href, '_blank', 'noopener')
})
if (links.length > 0) { // ←追加
location.reload(); // ←追加
} // ←追加
})
}
})のようにreloadを挟めば一応表示は消えてくれると思います。
追記 2025/10/21 13:30
提出物を一括開いて確認処理をした後でもボタンが残ってしまう問題↓これ
については、この実装でいいか駒形さんによく確認しておいた方が良いと思います。
実装が難しいにしても、以前実現できていたことが、修正によってできなくなるというのは「バグ」と認識されても仕方ないのかなと思います。
Issueにも
未確認リンクの警告を表示する機能。現在はReactコンポーネントとして実装されているが、状態管理が不要な単純な表示コンポーネントのためバニラJSで十分対応可能。
と書いてあるので、何か検討する余地が残されているかもしれません。
（駒形さんに確認した上でこの実装なら、このままで良いと思います👍）
最後に
色々お伝えしましたが、私が間違っていること、勘違いしていることもあるかと思いますので、何か気づくことがあったら教えてください！
app/views/products/index.html.slim
Outdated
| .page-body | ||
| .container.is-md | ||
| = react_component('Products', title: title, selectedTab: 'all', isMentor: mentor_login?, isAdmin: admin_login?, currentUserId: current_user.id) | ||
| - if @products.present? && %w[all unchecked_all unchecked_no_replied unassigned self_assigned self_assigned_all self_assigned_no_replied].include?(@target) |
There was a problem hiding this comment.
%w[all unchecked_all unchecked_no_replied unassigned self_assigned self_assigned_all self_assigned_no_replied].include?(@target)の条件は必要でしょうか？
products_controller.rbでは
def set_target
@target = 'all'
endとなっているので基本的に@targetにはallしか入らないような気がします。
この条件自体いらないかも？と思いましたが、どんなことを想定して書いたものか教えていただきたいです🙏
app/views/products/self_assigned/index.html.slim
app/views/products/unassigned/index.html.slim
app/views/products/unchecked/index.html.slim
についても同様です。
There was a problem hiding this comment.
ご指摘ありがとうございます。
当初は unconfirmed_links_open（一括開くボタン）の部分テンプレートを
各サブタブ（unchecked, self_assignedなど）でも共通で使う想定があり、
どのタブで表示すべきかを共通条件で判定できるように実装していました。
ただ、 @tyrrell-IH さんのご指摘のとおり、現状は各タブが個別のビューを持ち、@targetも固定値になっているため、
この条件は現在では不要であると再確認しました。
ご教示くださりありがとうございます。該当箇所は全て修正いたします🙇♀️
There was a problem hiding this comment.
当初は unconfirmed_links_open（一括開くボタン）の部分テンプレートを
各サブタブ（unchecked, self_assignedなど）でも共通で使う想定があり、
どのタブで表示すべきかを共通条件で判定できるように実装していました。
なるほど、そうだったんですね！
修正は確認しOKですが、
コミットメッセージにメンションが入ってしまっているので、今後は少し気をつけたほうがいいかもしれません。
There was a problem hiding this comment.
ご指摘ありがとうございます！コミットメッセージを編集してメンションを外しました。🙇♀️
| assert_equal '全ての提出物を一括で開く', unconfirmed_links_label('all') | ||
| assert_equal '未完了の提出物を一括で開く', unconfirmed_links_label('unchecked') | ||
| assert_equal '未完了の提出物を一括で開く', unconfirmed_links_label('unchecked_all') | ||
| assert_equal '未返信の提出物を一括で開く', unconfirmed_links_label('unchecked_no_replied') |
There was a problem hiding this comment.
未返信の提出物を一括で開く(未返信の担当提出物を一括で開くも同様)という分類は以前はなかったように思いますが追加されましたでしょうか？
文言はこのままで問題ないと思いますが、細かなことでも仕様変更があったらどこかに書いといてもらえると助かります🙏
There was a problem hiding this comment.
元々uncheckedとself_assignedのサブタブには全てと未返信がありましたが、
どちらもボタンラベルが同じで表示されていました。
UX上、サブタブごとにラベルを適切にする方がわかりやすいと判断し、
ProductsHelper#unconfirmed_links_labelにunchecked_no_repliedと self_assigned_no_repliedを追加して対応しました。
文言は既存のパターンに沿っており、動作も既存と同様です。
ただ、最初にこの意図を説明しておくべきでした。すみません。😞
There was a problem hiding this comment.
UX上、サブタブごとにラベルを適切にする方がわかりやすいと判断し、
ProductsHelper#unconfirmed_links_labelにunchecked_no_repliedと self_assigned_no_repliedを追加して対応しました。
確かにサブタブごとの方が見やすいですね、了解です🙆
| # 自分が担当かつ未返信提出物 | ||
| @unchecked_no_replied_product = Product.create!( |
There was a problem hiding this comment.
自分が担当かつ未返信提出物なら@self_assigned_no_replied_productが正しいかも？
There was a problem hiding this comment.
ご指摘ありがとうございます！
確かにです！修正しました！
test/system/products_test.rb
Outdated
| test 'sees the open all products button on products page' do | ||
| visit_with_auth '/products', 'komagata' | ||
| assert_selector 'button', text: '全ての提出物を一括で開く' | ||
| end |
There was a problem hiding this comment.
test/system/product/unconfirmed_links_open_test.rbの49行目とテストが重複しているような気がします。
There was a problem hiding this comment.
ご指摘ありがとうございます！
自分で同じ内容のテストを追加していましたね💦全然気づいていませんでした💦
重複しているテストは削除いたしました。
| test 'mentor sees unassigned products links' do | ||
| visit_with_auth '/products/unassigned', 'komagata' | ||
|
|
||
| assert_selector "a.js-unconfirmed-link[href$='#{@unassigned_product.id}']", count: 1, wait: 5 | ||
| end |
There was a problem hiding this comment.
test/system/product/unassigned_test.rbの24行目のテストと重複してるかもしれません。
There was a problem hiding this comment.
ご指摘ありがとうございます。
テストの重複について、こちらで探しきれておらず申し訳ありませんでした😞
今後は既存テストの確認も含めて整理するようにします。
| test 'mentor sees self_assigned products links' do | ||
| visit_with_auth '/products/self_assigned', 'komagata' | ||
|
|
||
| assert_selector "a.js-unconfirmed-link[href$='#{@self_assigned_product.id}']", count: 1 | ||
| assert_selector "a.js-unconfirmed-link[href$='#{@unchecked_no_replied_product.id}']", count: 1 | ||
| end |
There was a problem hiding this comment.
test/system/product/self_assigned_test.rbの28行目と重複してるかもしれません。
There was a problem hiding this comment.
ご指摘ありがとうございます。
こちらも上と同様、テストの重複についてこちらで探しきれておらず申し訳ありませんでした😞
There was a problem hiding this comment.
テストを書く場所が人によってまちまちなので、気づかなくてもしょうがないですよね😅
「どこにテストを書く」みたいな指針があればいいなと思いますが、、
test/models/product_test.rb
Outdated
| test 'self_assigned_no_replied_products returns products assigned to user with no reply' do | ||
| mentor = users(:komagata) | ||
| product = Product.create!(user: users(:hajime), practice: practices(:practice2), body: '提出物', checker_id: mentor.id) | ||
|
|
||
| assert_includes Product.self_assigned_no_replied_products(mentor.id), product | ||
|
|
||
| Comment.create!( | ||
| commentable: product, | ||
| user: mentor, | ||
| description: '返信済み', | ||
| created_at: Time.current, | ||
| updated_at: Time.current | ||
| ) | ||
|
|
||
| assert_not_includes Product.self_assigned_no_replied_products(mentor.id), product | ||
| end |
There was a problem hiding this comment.
bootcamp/test/models/product_test.rb
Line 134 in fc40e5c
同じテストがあると思います。
Comment.create!以降のコードはself_assigned_no_replied_productsの動作と関係ないので必要ないかもしれません。
There was a problem hiding this comment.
ご指摘ありがとうございます。
既存の .self_assigned_no_replied_productsのテストでは「返信がない提出物が含まれること」を確認していましたが、
「返信済みの提出物が除外されること」はカバーされていなかったため、
このテストではそのケースも合わせて検証しています。
つまり、スコープの正確な条件分岐のテストとして追加している形になります。
（Comment.create! 以降の処理はその確認のために追加しています）
もしテストとして冗長すぎると判断される場合は、削除していただいて構いませんので、遠慮なくお知らせください
There was a problem hiding this comment.
「返信済みの提出物が除外されること」はカバーされていなかったため、
このテストではそのケースも合わせて検証しています。
なるほど、おっしゃる通りですね。理解しました👍
テスト内容の意図は別でもタイトルが
.self_assigned_no_replied_productsとself_assigned_no_replied_products returns products assigned to user with no reply
では同じ内容のテストに見えるので
.self_assigned_no_replied_productsに
Comment.create!(
commentable: product,
user: mentor,
description: '返信済み',
created_at: Time.current,
updated_at: Time.current
)
assert_not_includes Product.self_assigned_no_replied_products(mentor.id), product
を追記しまう。
- テスト内容自体はこのままにして、テスト名を例えば
self_assigned_no_replied_products excludes products that have already been replied toのように「返信済みのものが除外されていることを確認する」のようなニュアンスにする。
のどちらかの方が良いかなと思うのですが、どうでしょうか？
There was a problem hiding this comment.
ご指摘ありがとうございます。
いただいた案を検討した結果、今回はテスト内容をまとめて .self_assigned_no_replied_productsの1つのテスト内で、「未返信の提出物に含まれること」と「コメントが返されたら除外されること」の両方を確認する形にしました。
テスト名は従来のままにし、コメントで「メンターがコメントすると除外されることを確認」と追記して、意図がわかるようにしています⭐️
| Notification.stub(:create!, nil) do | ||
| Watch.stub(:create!, nil) do |
There was a problem hiding this comment.
ここはどういった理由で書いてますか？
特に意図がある質問ではなく、純粋に「何でだろう」と思ったので質問しました🧐
Productを作成してもNotification.create! や Watch.create!は呼ばれなそうだしafter_create ProductCallbacks.newの方で呼ばれてましたね、失礼しました🙏）、呼ばれたとしてunconfirmed_links_open_testの動作にはあんまり影響しないように思いました。
There was a problem hiding this comment.
ご質問ありがとうございます。
もともとは Product.create!時にNotificationやWatchが作成される副作用を抑えるために書いたのですが、今回のテストではボタン表示やリンク確認のみを行うため、NotificationやWatchの有無はテスト結果に影響しないことに改めて確認して気づきました。教えていただきありがとうございます🙂
そのため、このテストに関してはstubは不要であり、削除いたします。
|
@tyrrell-IH さん ①動作確認手順について ご指摘ありがとうございます！ ②警告メッセージの適切な表示 Issueの『警告メッセージの適切な表示』という文言についてですが、実際の ③挙動についてのところ コメントありがとうございます！ とりあえず、こちらの返信を先にさせていただきました〜。 |
|
@komagata さん #9184 (review) 提出物を一括開いた後でもボタンが残る件についての件なのですが、 UXや以前の挙動との差異について、駒形さんにご確認いただいた上で、この実装で問題ないか判断していただけると助かります。 もし「以前のように即時非表示にしたほうが良い」という判断であれば、別Issueで対応を検討することも可能です。 よろしくお願いいたします🙇♀️ |
|
@sharoa119 本PRと修正箇所が重なると思うので、どうするか相談して決めた方が良いのかなと思いました |
|
@sharoa119 |
|
@tyrrell-IH さん @smallmonkeykey さん
こちら、かしこまりました！ １点確認させてください。
とのことですが、リサさんの担当されていた issue を確認したところ、
という内容でした。 つまり今回は、Stimulus の導入はやめて、削除対象だったファイルを戻し、バニラJSでの対応に変更、という認識で合っていますか？ その場合、通知機能まわりはリサさんが対応される予定という理解でよいでしょうか？ とりあえず、何か変更や調整があればご連絡いただければと思います🙇♀️ |
- unconfirmed_links_label ヘルパーを更新して self_assigned_all / self_assigned_no_replied に対応 - self_assigned タブでボタンが正しく表示されるよう修正し、他のタブと整合性を揃えた
- window.open の挙動確認は廃止 - モデル・コントローラのテストに追加・修正
- 重複していたテストを削除 - それに伴いsetupなど修正
machida
force-pushed
the
chore/replace_vanilla_js_in_unconfirmed_link_jsx
branch
from
46119a4 to
590c755
Compare
|
@komagata |
|
@machida ありがとうございます！ |
5 participants
Issue
概要
UnconfirmedLink.jsxコンポーネントをReactからバニラJavaScriptに移行する。(非React化）実装内容
挙動について
一括で開封した直後は「一括で開く」ボタンは画面上に残ります。
ただし「全て」タブを再選択すると非表示になります。
これは、React 版で行っていたような即時非表示の挙動を再現するには状態管理や非同期処理が必要となるため、今回の移行では対応していません。
今回の issue の趣旨（状態管理を使わず、バニラ JS でシンプルに移行する）に沿った実装です。
確認事項
変更確認方法
chore/replace_vanilla_js_in_unconfirmed_link_jsxをローカルに取り込むforeman start -f Procfile.devでローカル環境を立ち上げるkomagataのアカウントでログイン。パスワードはtesttest/notifications?status=unread→ 表示されるボタン:未読の通知を一括で開く/products→ 表示されるボタン:全ての提出物を一括で開く/products/unchecked→ 表示されるボタン:未完了の提出物を一括で開く/products/unchecked?target=unchecked_no_replied→ 表示されるボタン:未返信の提出物を一括で開く/unassigned→ 表示されるボタン:未アサインの提出物を一括で開く/products/self_assigned→ 表示されるボタン:自分の担当の提出物を一括で開く/products/self_assigned?target=self_assigned_no_replied→ 表示されるボタン:未返信の担当の提出物を一括で開く各リンクが新しいタブで開けばOK。
もし1件しか開かない場合は、Chromeのポップアップブロックが原因。（発生した場合は、以下の動作確認時の注意を読んでください。）
2025-10-03.11.10.09.mov
※自分が担当の提出物において、最初は何もありませんが、
未完了のところからどれかを選んで「担当する」ボタンを押していただくと、
自分の担当タブの中に表示されるので、そちらでボタンが表示されるかなどのご確認をお願いいたします。
動作確認時の注意
表示されている「一括で開く」ボタンを押した際、先頭の1件しか開かない場合があります。
これは
Chromeのポップアップブロック機能（window.openを利用したタブの同時オープンも対象）が原因です。動作確認を行う場合は、以下いずれかでポップアップを許可してください。
Screenshot
内部変更で画面の変更がないので、スクリーンショットはありません。
テスト方針について
前提
通知: 開封すると既読処理が入るため、通常のテストで補える
日報・提出物: 開封しても未チェックのままなので、JS でリンクが正しく開かれることを保証する必要がある
テスト方針
今回の変更で使用しているバニラ JavaScript は、
「ボタンをクリックすると未確認リンクを
window.openで一括開封する」処理です。しかし、
window.openの挙動をシステムテストで直接検証するのは難しいため、以下の 3 つの観点で間接的にテストを行っています。
1. モデル単体テスト
2. システムテスト（ボタン表示）
※本テストでは、移行後のコードが元々の仕様に沿って動作することを確認しています。
3. システムテスト（リンク存在）
.js-unconfirmed-linkが期待通りレンダリングされているかを確認※従来のテストを活かしつつ、内容を「
.js-unconfirmed-linkが正しくレンダリングされていることを確認する」形式に修正しています。補足: window.open の挙動について
本テストでは、ボタンが正しく表示されクリック可能であることまでを確認しています。
ただし、ボタン押下時に新しいタブが開くかどうか（
window.openの挙動）はMinitest(Rails の System Test)では直接検証できない仕様です。この動作はフロントエンド側の
JavaScriptに依存しており、手動またはブラウザ上での確認により保証しています。👉 System Test では最低限、ユーザーがボタンを認識して操作できることを確認することが目的です。
補足: webpacker のエラーについて
テスト実行時に
packが最新でない場合、JavaScriptやCSSが正しく読み込まれずエラーになることがあります。その場合は以下を実行してください。
👉 この 3 点で、直接 JS をテストしなくても
「対象データが正しくレンダリングされ、必要な場面でボタンが表示される」ことを確認しています。
Summary by CodeRabbit
リリースノート
新機能
改善
✏️ Tip: You can customize this high-level summary in your review settings.