-
-
Notifications
You must be signed in to change notification settings - Fork 109
feat: Move /events/latest
→ /dojos/activity
to be more RESTful
#1746
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
/events/latest
→ /dojos/activity
to be RESTful URLs
/events/latest
→ /dojos/activity
to be RESTful URLs/events/latest
→ /dojos/activity
to be more RESTful
c038497
to
75ac040
Compare
- RESTfulなURLパターンに従い、道場の活動状況を /dojos/activity に移動 - 情報アーキテクチャの改善(道場関連は/dojos配下に統一) - 旧URL (/events/latest) から新URLへの301リダイレクトを設定 - テストを追加(最小限の実装、YAGNI原則に従う)
- /dojos と /dojos/activity で重複していたテーブルスタイルを統一 - custom.scss の .stats-table クラスに共通スタイルを集約 - 両ビューから重複CSSを削除し、stats-tableクラスを適用
75ac040
to
9cfa869
Compare
- /dojos ページと同様に長いテキストを省略表示 - noteは60文字でtruncate(URLを含むことが多いため) - titleアトリビュートで全文を確認可能
- 掲載日(🗓): /dojos と同じロジック(created_at) - 開催日(👥): 直近のイベント開催日(なければ空欄) - ページタイトルを「活動状況まとめ」に更新 - テストを追加して新しいカラム構成を検証
- 道場名 (ID 番号) ヘッダーを /dojos と統一 - 道場名の下にIDを表示する形式も統一 - リンク先を道場詳細ページに変更(内部リンク化) - DRY原則に従い、両ページで同じ表示形式を維持
- リンク先ページ名を正式名称に統一(統計情報) - 不要な説明を削除してシンプルに - 開催日のデータソース制限を明確化
@claude Review this PR if we can merge or need additional commits to merge? ultrathink |
Claude finished @yasulab's task —— View job 📋 レビュー結果PRを詳細にレビューしました。全体的に優れた実装で、マージ可能と判断します。 ✅ 良い実装 (参考になる箇所)1. Rails Wayに沿った設計
2. DRY原則の徹底
3. データの明確な分離# 掲載日と開催日を明確に分離
created_at: dojo.created_at, # 掲載日(/dojos と同じ)
latest_event_at: latest_event.nil? ? nil : latest_event.evented_at, # 開催日 4. 適切なテストカバレッジ
5. YAGNI原則の適用
💡 提案 (より良い実装方法)1. N+1クエリの最適化
def activity
@latest_event_by_dojos = []
# N+1を避けるため、event_historiesを事前ロード
Dojo.active.includes(:event_histories).each do |dojo|
# ...
end
end 2. ソートロジックの改善現在のソートでは、同じ日付の場合にIDで並び替えていますが、より意味のある並び順(例:道場名)にすると良いでしょう: @latest_event_by_dojos.sort_by! do |dojo|
sort_date = dojo[:latest_event_at] || dojo[:note_date] || dojo[:created_at]
[sort_date, dojo[:name]] # IDではなく名前でソート
end 3. パーシャルの活用検討将来的に、テーブル構造が他のページでも使われる場合は、パーシャルに切り出すことを検討してください: <%= render 'shared/stats_table', dojos: @latest_event_by_dojos %> 4. 定数の活用1年以上開催していない判定ロジックを定数化すると、メンテナンスが容易になります: INACTIVE_THRESHOLD = 1.year
# ビューで
expired = dojo[:latest_event_at] <= Time.current - INACTIVE_THRESHOLD 🎯 結論このPRは以下の点で優れています:
特に重大な問題やセキュリティ上の懸念はありません。提案した改善点は将来的な最適化として検討できますが、現状のままでもマージ可能です。 コードの品質は高く、Rails のベストプラクティスに従っています。👍 |
- 同じ日付の道場をIDではなくorderでソート(地理的にまとまる)
- INACTIVE_THRESHOLD = 1.year をDojoモデルに定義 - メンテナンス性向上(閾値変更が容易に) - ビューでは Time.current.prev_year の代わりに定数を使用 - Claudeからのフィードバックを適用
- Dojo::INACTIVE_THRESHOLD 定数から期間を自動取得 - ハードコードされた「1年間」を変数化 - 将来の閾値変更時に自動で反映されるように改善 - 全角数字に自動変換(例: 1 → 1)
📍 概要
/events/latest
を/dojos/activity
に移動し、Rails Wayに従ったRESTfulなURL構造にしました。さらに、
/dojos
ページとの一貫性を持たせるためUIを統一し、非アクティブ判定ロジックを全サイトで一元管理するように改善しました。✅ 実装内容
1. URL構造の改善
/dojos/activity
を追加(collection route)/events/latest
から301リダイレクト設定2. UI/UXの統一化(/dojosページとの一貫性)
/dojos
と同じ形式で表示/dojos
と同じロジック(created_at).stats-table
クラスに統一/dojos/:id
)へのリンク3. データの明確化
4. 非アクティブ判定の一元管理 🆕
Dojo::INACTIVE_THRESHOLD = 1.year
をDojoモデルに定義/signup
ページの「1年間」を変数化一定期間 (現時点では1年間)
一定期間 (現時点では{{ INACTIVE_THRESHOLD }})
5. コードの改善
🧪 テスト
/dojos/activity
が正常に動作/events/latest
からのリダイレクトが機能/dojos
と同じ形式で表示/signup
ページの閾値が動的に表示される📊 コード改善効果
主要な改善点
🎯 なぜこの変更が必要か
/dojos
ページとのCSS・UI統一、定数の一元管理💡 影響範囲
/events/latest
へのリンクは自動的にリダイレクト(破壊的変更なし)📝 関連Issue/Discussion
🔄 コミット履歴
feat: Move /events/latest to /dojos/activity following Rails Way
refactor: テーブルCSSを共通化してDRYに
feat: /dojos/activity のnoteカラムにtruncateを適用
feat: /dojos/activity に掲載日カラムを追加し、開催日と分離
feat: /dojos/activity に道場IDを表示(/dojos と同じ形式)
docs: /dojos/activity の説明文を改善
fix: ソート順を地理的順序に改善
refactor: 非アクティブ判定の閾値をDojoモデルに定数化
feat: /signup ページの非アクティブ期間を動的表示に
📸 スクリーンショット
Before & After
/events/latest
→/dojos/activity
✨ レビュアーへのお願い
特に以下の点についてフィードバックをお願いします:
/dojos
ページとの統一感)