定期イベントにブックマークボタンを追加 #9306
定期イベントにブックマークボタンを追加 #9306
Conversation
Walkthrough正規イベントのページヘッダーにブックマーク操作ボタンを追加し、ブックマークの追加・削除とブックマークリスト反映を検証するシステムテストを追加しました。ボタンは初期で無効表示、bookmarkable関連のdata属性とARIA属性を持ちます。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (5)test/**/*_test.rb📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.rb📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
test/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
test/**/*⚙️ CodeRabbit configuration file
Files:
**/*.slim📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (10)📓 Common learnings📚 Learning: 2025-11-26T08:23:30.927ZApplied to files:
📚 Learning: 2025-11-26T08:23:30.927ZApplied to files:
📚 Learning: 2025-09-12T21:18:00.834ZApplied to files:
📚 Learning: 2025-07-10T12:59:27.807ZApplied to files:
📚 Learning: 2025-07-23T20:31:13.856ZApplied to files:
📚 Learning: 2025-07-30T07:26:36.599ZApplied to files:
📚 Learning: 2025-09-28T00:36:43.971ZApplied to files:
📚 Learning: 2025-11-17T00:46:30.794ZApplied to files:
📚 Learning: 2025-09-04T01:39:22.261ZApplied to files:
🧬 Code graph analysis (1)test/system/bookmark/regular_events_test.rb (1)
⏰ 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 (3)
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 |
tyrrell-IH
force-pushed
the
feature/add-bookmark-button-to-regular-events
branch
from
0068292 to
f00dbb5
Compare
|
@machida 一応スタイルの確認をしていただいても大丈夫でしょうか？ またラベル内の文字が のようになっているのですが、 のようにしたいです。 |
このように br タグを入れる対応でお願いしますー🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/javascript/components/BookmarksInDashboard.jsx (1)
177-189: コードの重複が存在します。この関数は
Bookmarks.jsxの244-256行目と同一です。Bookmarks.jsxのレビューコメントで提案した共通ユーティリティへの抽出を適用してください。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/javascript/components/Bookmarks.jsx(2 hunks)app/javascript/components/BookmarksInDashboard.jsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/javascript/components/Bookmarks.jsx (1)
app/javascript/components/BookmarksInDashboard.jsx (1)
renderBookmarkLabel(177-189)
app/javascript/components/BookmarksInDashboard.jsx (1)
app/javascript/components/Bookmarks.jsx (1)
renderBookmarkLabel(244-256)
⏰ 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/javascript/components/Bookmarks.jsx (1)
184-186: ヘルパー関数の使用は適切です。
renderBookmarkLabelヘルパーの使用により、ラベル表示ロジックが適切に分離されています。app/javascript/components/BookmarksInDashboard.jsx (1)
122-124: ヘルパー関数の使用は適切です。
renderBookmarkLabelヘルパーの使用により、ラベル表示ロジックが適切に分離されています。
tyrrell-IH
force-pushed
the
feature/add-bookmark-button-to-regular-events
branch
from
ca227e6 to
79393ab
Compare
|
@smallmonkeykey |
|
@tyrrell-IH |
|
@smallmonkeykey |
|
@tyrrell-IH |
tyrrell-IH
force-pushed
the
feature/add-bookmark-button-to-regular-events
branch
from
79393ab to
9cbc3c3
Compare
|
@smallmonkeykey |
| @regular_event = regular_events(:regular_event1) | ||
| end | ||
|
|
||
| test 'shows regular event link on list when clicking bookmark button' do |
There was a problem hiding this comment.
質問です！既存のブックマーク関連のテストと違って、RegularEventテストでは 1つのテストで一連の動作をシナリオ的にまとめている構成になっていますが、こちらにした理由はありますか？？
There was a problem hiding this comment.
一連の動作をシナリオ的にまとめているわけではないつもりでした〜
以下test/system/bookmark/products_test.rbと比較して意図を説明します
前提
ブックマークボタンを今回実装したわけではないので、ブックマークボタンに関する動作はテストしていないという前提です。
ブックマーク機能は日報にブックマーク機能を実装 by alto-I · Pull Request #2789 · fjordllc/bootcampで日報に初めて導入され、その後ボタンの実装がBookmarkButton.jsxの非React化 by jun-kondo · Pull Request #9104 · fjordllc/bootcampにてバニラJSで実装されており、いずれにおいてもブックマークボタンの動作
is-activeis-inactiveの切り替えがうまくいくか- ボタンをクリックした際にリクエストが飛ぶかどうか
等はtest/system/bookmarks_test.rbやtest/integration/api/bookmarks_test.rbで保証されていると考えました。
今回のテストの趣旨
上記の前提を踏まえた上で、今回のPRは「定期イベントにブックマークボタンを追加する」なので、テストの内容を
-
テスト1
- ブックマークされていない定期イベントにおいて
- 「ブックマーク」ボタンを押すと
- ボタンの名称が「ブックマーク中」に変更され
current_user/bookmarksにブックマークした定期イベントのリンクが表示される
-
テスト2
- ブックマークされている定期イベントにおいて
- 「ブックマークボタン中」ボタンを押すと
- ボタンの名称が「ブックマーク」に変更され
current_user/bookmarksからブックマークした定期イベントのリンクが消える
の2点で良いかなと判断しました。
test/system/bookmark/products_test.rbとの比較
test/system/bookmark/products_test.rbは以下の通りです
class Bookmark::ProductTest < ApplicationSystemTestCase
setup do
@product = products(:product1)
end
test 'show product bookmark on lists' do
visit_with_auth '/current_user/bookmarks', 'kimura'
assert_text @product.title
end
test 'show active button when bookmarked product' do
visit_with_auth "/products/#{@product.id}", 'kimura'
wait_for_javascript_components
assert_selector '#bookmark-button.is-active'
assert_no_selector '#bookmark-button.is-inactive'
end
test 'show inactive button when not bookmarked product' do
visit_with_auth "/products/#{@product.id}", 'komagata'
wait_for_javascript_components
assert_selector '#bookmark-button.is-inactive'
assert_no_selector '#bookmark-button.is-active'
end
test 'bookmark product' do
visit_with_auth "/products/#{@product.id}", 'komagata'
wait_for_javascript_components
find('#bookmark-button').click
wait_for_javascript_components
assert_selector '#bookmark-button.is-active'
assert_no_selector '#bookmark-button.is-inactive'
visit '/current_user/bookmarks'
assert_text @product.title
end
test 'unbookmark product' do
visit_with_auth "/products/#{@product.id}", 'kimura'
wait_for_javascript_components
find('#bookmark-button').click
wait_for_javascript_components
assert_selector '#bookmark-button.is-inactive'
assert_no_selector '#bookmark-button.is-active'
visit '/current_user/bookmarks'
assert_no_text @product.title
end
end上記のうち
show product bookmark on listsについてはブックマークボタンを押した後に/current_user/bookmarksに表示されていることが確認できれば良いと考えたので、個別のテストは不要と考えました。show active button when bookmarked productとshow inactive button when not bookmarked productについて、is-activeとis-inactiveの切り替えについてはtest/system/bookmarks_test.rbにて動作の確認を行なっているので改めての確認は不要だと考えました。
残るテストはbookmark productとunbookmark productであり、これらのテストは上記「今回のテストの趣旨」で述べた内容と一緒です。
なので
RegularEventテストでは 1つのテストで一連の動作をシナリオ的にまとめている構成になっていますが
とありますが、私の認識としては
test/system/bookmark/products_test.rbにおけるbookmark productunbookmark product
と
test/system/bookmark/regular_events.rbにおけるshows regular event link on list when clicking bookmark buttonupdates bookmarks list when toggling bookmark
はそれぞれ同じことをテストしており、テストをまとめたわけでは無いという認識です
There was a problem hiding this comment.
test/system/bookmark/products_test.rbにおける
bookmark product
unbookmark product
と
test/system/bookmark/regular_events.rbにおける
shows regular event link on list when clicking bookmark button
updates bookmarks list when toggling bookmark
はそれぞれ同じことをテストしており、テストをまとめたわけでは無いという認識です
すみませんでした。確かにそうですね。
is-active is-inactiveの切り替えがうまくいくか
ボタンをクリックした際にリクエストが飛ぶかどうか
等はtest/system/bookmarks_test.rbやtest/integration/api/bookmarks_test.rbで保証されていると考えました。
「ブックマーク」ボタンを押すと
ボタンの名称が「ブックマーク中」に変更され
この部分をassert_selector '#bookmark-button.is-active'ではなく文言でチェックした理由を理解できました。
私の拙い質問にも丁寧に答えてくださり、ありがとうございます！！
|
@tyrrell-IH |
tyrrell-IH
force-pushed
the
feature/add-bookmark-button-to-regular-events
branch
from
9cbc3c3 to
f243d29
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/views/regular_events/_regular_event.html.slim(1 hunks)test/system/bookmark/regular_events_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/regular_events/_regular_event.html.slim
🧰 Additional context used
📓 Path-based instructions (4)
test/**/*_test.rb
📄 CodeRabbit inference engine (AGENTS.md)
test/**/*_test.rb: Test suite should use Minitest with structure:system/,models/,controllers/, and fixtures intest/fixtures/; test files should be named*_test.rb
Use Minitest + Capybara for system tests; place unit and integration tests under matchingtest/*directories
Files:
test/system/bookmark/regular_events_test.rb
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby code should use 2-space indentation, snake_case for method names, and CamelCase for class names, enforced by RuboCop (
.rubocop.yml)
Files:
test/system/bookmark/regular_events_test.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/bookmark/regular_events_test.rb
test/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep tests deterministic and use fixtures stored in
test/fixtures/for test data
Files:
test/system/bookmark/regular_events_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/bookmark/regular_events_test.rb
🧠 Learnings (6)
📓 Common learnings
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Learnt from: tyrrell-IH
Repo: fjordllc/bootcamp PR: 9306
File: app/javascript/components/Bookmarks.jsx:248-265
Timestamp: 2025-11-17T00:46:30.794Z
Learning: fjordllc/bootcamp プロジェクトでは、Reactからバニラ JavaScript への移行作業が進行中である。そのため、新しいReactファイルの作成は避け、既存のReactコンポーネント内で完結する実装が推奨される。BookmarksInDashboard.jsx は issue #9045 で削除予定。
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 9104
File: app/javascript/bookmark-button.js:5-11
Timestamp: 2025-09-28T00:36:43.971Z
Learning: fjordllc/bootcampアプリケーションでは現在、各ページにブックマークボタンは1つのみ存在し、Turbo Driveは未使用のため、querySelector での単一要素初期化とDOMContentLoadedのみのイベントリスナーで十分である。
Learnt from: mousu-a
Repo: fjordllc/bootcamp PR: 8566
File: app/views/pair_works/_body.html.slim:0-0
Timestamp: 2025-08-31T01:05:07.447Z
Learning: システムテストでボタンやフォーム要素を識別する必要がある場合、動的な値をクラス属性に埋め込むのではなく、id属性やdata属性を使用する。特にJavaScriptでの操作やコントローラーへのデータ送信が不要な場合は、id属性を使用するのが適切。
📚 Learning: 2025-11-26T08:23:30.927Z
Learnt from: CR
Repo: fjordllc/bootcamp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T08:23:30.927Z
Learning: Applies to test/**/*_test.rb : Use Minitest + Capybara for system tests; place unit and integration tests under matching `test/*` directories
Applied to files:
test/system/bookmark/regular_events_test.rb
📚 Learning: 2025-09-12T21:18:00.834Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。
Applied to files:
test/system/bookmark/regular_events_test.rb
📚 Learning: 2025-07-23T20:31:13.856Z
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 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:
test/system/bookmark/regular_events_test.rb
📚 Learning: 2025-07-10T12:59:27.807Z
Learnt from: kitarou888
Repo: fjordllc/bootcamp PR: 8277
File: test/models/searcher_test.rb:326-331
Timestamp: 2025-07-10T12:59:27.807Z
Learning: モデルテストにおいて、1つのクラスメソッドの複数の挙動を検証する場合、機能の異なる側面を同じテストメソッドでテストすることは、包括的なテストとして適切である。特に`only_me`のような機能では、異なる検索条件（空文字と具体的な検索語）を使い分けることで、より広範囲な動作保証が可能となる。
Applied to files:
test/system/bookmark/regular_events_test.rb
📚 Learning: 2025-07-30T07:26:36.599Z
Learnt from: ryufuta
Repo: fjordllc/bootcamp PR: 8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: Railsの標準的なヘルパーメソッド（例：`question_url`、`user_path`など）が通常の方法で使用されている場合、それらの動作テストは不要である。これらのメソッドはRailsフレームワーク自体でテストされており、アプリケーション固有のロジックではないため。
Applied to files:
test/system/bookmark/regular_events_test.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 (2)
test/system/bookmark/regular_events_test.rb (2)
1-8: セットアップは適切です。クラス構造とフィクスチャの読み込みは正しく実装されています。
10-21: JavaScriptコンポーネントの初期化待機が必要です。ブックマークボタンはJavaScriptで動作するため、
products_test.rbと同様にwait_for_javascript_componentsの呼び出しが必要です。これがないと、JavaScriptの初期化前にテストが実行され、不安定なテストになる可能性があります。以下のdiffを適用してください：
test 'shows regular event link on list when clicking bookmark button' do visit_with_auth regular_event_path(@regular_event), 'machida' + wait_for_javascript_components assert_selector '#bookmark-button', text: 'Bookmark' click_button 'Bookmark' + wait_for_javascript_components assert_selector '#bookmark-button', text: 'Bookmark中' visit current_user_bookmarks_path⛔ Skipped due to learnings
Learnt from: komagata Repo: fjordllc/bootcamp PR: 9101 File: app/notifiers/discord_notifier.rb:131-135 Timestamp: 2025-09-12T21:18:00.834Z Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。Learnt from: jun-kondo Repo: fjordllc/bootcamp PR: 9104 File: app/javascript/bookmark-button.js:5-11 Timestamp: 2025-09-28T00:36:43.971Z Learning: fjordllc/bootcampアプリケーションでは現在、各ページにブックマークボタンは1つのみ存在し、Turbo Driveは未使用のため、querySelector での単一要素初期化とDOMContentLoadedのみのイベントリスナーで十分である。
|
@smallmonkeykey あと以下の通り少し修正を加えました。
よろしくお願いします |
smallmonkeykey
left a comment
smallmonkeykey
left a comment
There was a problem hiding this comment.
丁寧に答えてくださりありがとうございます。
理解できましたので、approveします！
tyrrell-IH
force-pushed
the
feature/add-bookmark-button-to-regular-events
branch
from
e8165b2 to
7d8b9c7
Compare
|
@okuramasafumi チームメンバーのレビューが終わりましたので、レビューをお願いします。 |
Issue
概要
定期イベントがブックマークできるよう、
Bookmarkボタンを追加しました。変更確認方法
feature/add-bookmark-button-to-regular-eventsをローカルに取り込むBookmarkボタンを押しブックマーク登録する。最新のブックマーク欄Bookmark中ボタンを押しブックマークを解除する。最新のブックマーク欄Screenshot
変更前
変更後
ラベルの改行について
ブックマークリンクのラベルについて
のように、「定期」の文字の後に改行が入っているのが望ましいですが、何も対応しない状態では
のように表示されています。
本来であれば適切な位置で改行を挟むことが望ましいと思います。
しかし、各ラベルについて
/current_user/bookmarksはBookmarks.jsx/(ダッシュボードのブックマーク欄)はBookmarksInDashboard.jsxにてラベルの要素が構築されていますが、各jsxファイルは
にて削除予定です。
2025/11/19の開発ミーティング夜の部にて駒形さん、町田さんに了承を得た上で
という流れで処理を進めることにしました。
その他
#9104
上記PRを参考にして、本PRを作成しています。
Summary by CodeRabbit
新機能
テスト
✏️ Tip: You can customize this high-level summary in your review settings.