「近日開催のイベント」で、当日の開催時間を過ぎたイベントは非表示にする #8280

sugiwe · 2025-01-16T06:51:39Z

Issue

「近日開催のイベント」で、当日の開催時間を過ぎたイベントは非表示になってほしい #8221

概要

変更確認方法

feature/do-not-display-finished-events-in-upcoming-events-group

{feature/do-not-display-finished-events-in-upcoming-events-group}をローカルに取り込む
- git fetch origin pull/8280/head:feature/do-not-display-finished-events-in-upcoming-events-group
- git switch feature/do-not-display-finished-events-in-upcoming-events-group
rails cで rails console を立ち上げる
以下のコマンドを使い、テスト用のイベント2つを作成する

# 変数userの作成
user = User.find(253826460)

# 1つ目：特別イベント
 Event.create!(
  title: "5分後に「近日開催」の一覧から消える特別イベント",
  description: "終了後にダッシュボード等の「近日開催のイベント」の一覧から消えるかを確認するための特別イベントです",
  location: "Lokkaオフィス",
  capacity: 10,
  start_at: Time.current,
  end_at: Time.current + 5.minute,
  open_start_at: Time.current - 3.hour,
  open_end_at: Time.current - 10.minute,
  user: user
)

# 2つ目：定期イベント
regular_event = RegularEvent.new(
  title: "5分後に「近日開催」の一覧から消える定期イベント",
  description: "終了後にダッシュボード等の「近日開催のイベント」の一覧から消えるかを確認するための定期イベントです",
  finished: false,
  hold_national_holiday: false,
  start_at: Time.current,
  end_at: Time.current + 5.minute,
  user: user
)
regular_event.regular_event_repeat_rules.build(
  frequency: 0,
  day_of_the_week: Time.current.wday,
)
regular_event.user_ids << user
regular_event.save!

foreman start -f Procfile.devでローカルサーバーを立ち上げる
ダッシュボード左上の「近日開催のイベント」に、上で作成した２つのイベントが表示されていることを確認
5分後（＝イベント終了後）、「近日開催のイベント」の一覧から上記２つのイベント表示が消えていることを確認
※どちらのイベントも、end_at: Time.current + 5.minuteにより5分後に終了する状態となっております。もしご確認ステップの3〜5の間で5分以上経ってしまった場合は初めからイベントが表示されていない状態になりますので、お手数ですが改めてイベントを作成してください（end_at: Time.current + 10.minuteにするなど終了時刻を調整することで時間に余裕を持たせることができます）

Screenshot

変更前

※各スクリーンショットで、画面右上に表示されている時間についても併せてご覧ください

イベント作成直後（イベント終了前）

元から入っていたテスト用のイベントと今回追加したテスト用のイベントが表示されています

イベント作成から5分以上経過後（イベント終了後）

全てのイベントがそのまま表示されています

変更後

イベント作成直後（イベント終了前）

元から入っていたテスト用のイベントは9:00開催で終了しているので、この段階で既に非表示になっています

イベント作成から5分以上経過後（イベント終了後）

今回追加したイベントも5分が経過して終了となったので、非表示になっています

参考までに、イベント一覧画面の方では勿論イベントの情報は確認できます

sugiwe · 2025-01-23T08:22:19Z

@Judeeeee
お疲れ様です☕️
お手隙の際にこちらのレビューをお願いできますでしょうか🙏
現在、他PRの修正と別のレビュー対応で少なくとも1週間以上かかりそうな状況ですので全然急いでおりませんし、もしご都合が合わないようであれば遠慮なくおっしゃってください、どうぞよろしくお願いいたします！

Judeeeee · 2025-01-24T06:10:24Z

@sugiwe
お疲れ様です！承知しました！
2週間目処でレビューする形でも問題なければ是非担当させてくださいー🙏

sugiwe · 2025-01-25T06:01:42Z

@Judeeeee
お疲れ様です、お返事ありがとうございます！
2週間目処で問題ございません、どうぞよろしくお願いいたします🙏

Judeeeee

@sugiwe
お疲れ様ですー！
ブラウザ上での動作確認は問題ありませんでした🙆
気になった箇所にコメントをしたので、ご確認くださいー🙏

Judeeeee · 2025-01-31T06:09:02Z

test/models/event_test.rb

@@ -135,4 +135,20 @@ class EventTest < ActiveSupport::TestCase
    assert 994_018_171, ids
    assert 205_042_674, ids
  end
+
+  test 'Events in progress today should be displayed' do


modelテストなので、メソッドのみをテストした方がよさそうです🙆

test '.scheduled_on_without_ended' do # テストを記述する end

のような形になりそうですかね👀

Events in progress today should be displayed

Events that have already ended today should not be displayed

は、システムテストで確認した方がまとまりがよいかと思います〜

システムテストが非常に重いため増やしたくない意図からmodelテストに書かれたかもしれないのですが、他のテストもメソッドをテストする書き方をしているので整合性の面でもシステムテストで書いた方が良い気がします🧐(折角fixtureでイベントも作成しているので)

また、実際にシステムテストを書く際の補足ですが、私が過去にこのように指摘されたことがあるので、「表示されるorされない」のどちらかを確認するテストだけで十分だと思います👌

@Judeeeee
こちらありがとうございます🙏
どのテストに何を入れるのか、正直よくわかっていない部分も大きかったので、改めて検討して修正させていただきます🙏

Judeeeee · 2025-01-31T06:14:14Z

test/models/regular_event_test.rb

@@ -154,4 +154,20 @@ class RegularEventTest < ActiveSupport::TestCase
      assert_equal 157, participated_regular_events.count
    end
  end
+
+  test 'Regular event in progress today should be displayed' do


ここのモデルテストもevent_testのコメントと同様です🙆

Judeeeee · 2025-01-31T06:33:22Z

app/models/event.rb

@@ -44,6 +44,8 @@ class Event < ApplicationRecord
  scope :wip, -> { where(wip: true) }
  scope :related_to, ->(user) { user.job_seeker ? all : where.not(job_hunting: true) }
  scope :scheduled_on, ->(date) { where(start_at: date.midnight...(date + 1.day).midnight) }
+  scope :not_ended, -> { where('end_at > ?', Time.current) }
+  scope :scheduled_on_without_ended, ->(date) { scheduled_on(date).not_ended }


レビューのために質問させてください🙏

scheduled_on_without_endedをEventクラスとRegularEventクラスの両方で定義している意図って何でしょうか？👀
それぞれのイベント(特別イベント・定期イベント)の違いで混乱しており、補足があればご教示いただけると幸いです🙇

@Judeeeee
意味合いとしては同じ「今日開催でまだ終わっていないイベント」で命名もscheduled_on_without_ended で共通なのですが、中身のロジックが異なるものとして定義する必要があるのは、特別イベントと定期イベントで「開催日」の扱いがかなり異なっているという事情があります🙏

まず、自分が作業する前の段階から存在していたscheduled_onスコープがありまして、これも同名であるにも関わらずEventクラスとRegularEventクラスの両方で異なる内容で定義してあるという前提がございます。

# app/models/event.rb 内 # 特別イベントは開催が1回だけなので、「今日開催予定かどうか」の絞り込みがシンプル（DBの絞り込みだけで処理） scope :scheduled_on, ->(date) { where(start_at: date.midnight...(date + 1.day).midnight) } # app/models/regular_event.rb 内 # 定期イベントは毎週など複数開催のため開催日の設定がやや複雑になっている（holdingで取得後にfilterを使いRubyで判定） scope :scheduled_on, ->(date) { holding.filter { |event| event.scheduled_on?(date) } }

このように同名でも中身の異なるscheduled_onスコープを拡張するような形で今回のスコープを定義したという背景があるのですが、要はEventとRegularEventで開催日の管理方法が異なるため共通化することは難しく、意味合いとしてはどちらも「今日開催でまだ終わっていないイベント」になるのですが、それぞれ別の方法で定義している、ということになります。

うまく説明できたかどうかわからないのですが、いかがでしょうか。
ご不明点あれば何なりとお聞きください！

丁寧な説明ありがとうございます🙏

特別イベントと定期イベントで「開催日」の扱いがかなり異なっている

既存のscheduled_onスコープがEventとRegularEventでそれぞれ定義されている

ことから共通化が難しかったのですね👀
理解できたため追加の質問はないですー🙇

Judeeeee

@sugiwe
お疲れ様です！
コメントありがとうございました🙏
丁寧な説明助かりました！✨

引き続きシステムテストの検討だけ対応お願いいたします🙇

Judeeeee · 2025-02-23T10:57:46Z

app/models/event.rb

@@ -44,6 +44,8 @@ class Event < ApplicationRecord
  scope :wip, -> { where(wip: true) }
  scope :related_to, ->(user) { user.job_seeker ? all : where.not(job_hunting: true) }
  scope :scheduled_on, ->(date) { where(start_at: date.midnight...(date + 1.day).midnight) }
+  scope :not_ended, -> { where('end_at > ?', Time.current) }
+  scope :scheduled_on_without_ended, ->(date) { scheduled_on(date).not_ended }


丁寧な説明ありがとうございます🙏

特別イベントと定期イベントで「開催日」の扱いがかなり異なっている

既存のscheduled_onスコープがEventとRegularEventでそれぞれ定義されている

ことから共通化が難しかったのですね👀
理解できたため追加の質問はないですー🙇

sugiwe self-assigned this Jan 16, 2025

sugiwe added 4 commits January 19, 2025 05:54

「近日開催のイベント」を、現在時刻から計算して終わっていないイベントだけを表示するようにした

8541147

特別イベント用のテストを作成

1572fde

定期イベント用のテストを作成

9c86dc2

RegularEventの方のnot_endedのロジックを修正

7a5cc91

sugiwe force-pushed the feature/do-not-display-finished-events-in-upcoming-events-group branch from dd91341 to 47c792a Compare January 19, 2025 06:13

当日開催で終了済みイベントを非表示にした変更に伴い既存のシステムテストで設定していた時間を2時間過去にした

ed505b9

sugiwe force-pushed the feature/do-not-display-finished-events-in-upcoming-events-group branch from 47c792a to ed505b9 Compare January 19, 2025 07:30

イベントのテストデータ追加に伴いシステムテスト内のイベント数を修正

2b33e0d

sugiwe requested a review from Judeeeee January 23, 2025 08:22

sugiwe marked this pull request as ready for review January 23, 2025 08:22

Judeeeee reviewed Jan 31, 2025

View reviewed changes

Judeeeee reviewed Feb 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

「近日開催のイベント」で、当日の開催時間を過ぎたイベントは非表示にする #8280

「近日開催のイベント」で、当日の開催時間を過ぎたイベントは非表示にする #8280

sugiwe commented Jan 16, 2025 •

edited

Loading

sugiwe commented Jan 23, 2025

Judeeeee commented Jan 24, 2025

sugiwe commented Jan 25, 2025

Judeeeee left a comment

Judeeeee Jan 31, 2025

sugiwe Feb 19, 2025

Judeeeee Jan 31, 2025 •

edited

Loading

Judeeeee Jan 31, 2025

sugiwe Feb 19, 2025

Judeeeee Feb 23, 2025

Judeeeee left a comment

Judeeeee Feb 23, 2025

「近日開催のイベント」で、当日の開催時間を過ぎたイベントは非表示にする #8280

Are you sure you want to change the base?

「近日開催のイベント」で、当日の開催時間を過ぎたイベントは非表示にする #8280

Conversation

sugiwe commented Jan 16, 2025 • edited Loading

Issue

概要

変更確認方法

Screenshot

変更前

イベント作成直後（イベント終了前）

イベント作成から5分以上経過後（イベント終了後）

変更後

イベント作成直後（イベント終了前）

イベント作成から5分以上経過後（イベント終了後）

参考までに、イベント一覧画面の方では勿論イベントの情報は確認できます

sugiwe commented Jan 23, 2025

Judeeeee commented Jan 24, 2025

sugiwe commented Jan 25, 2025

Judeeeee left a comment

Choose a reason for hiding this comment

Judeeeee Jan 31, 2025

Choose a reason for hiding this comment

sugiwe Feb 19, 2025

Choose a reason for hiding this comment

Judeeeee Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Judeeeee Jan 31, 2025

Choose a reason for hiding this comment

sugiwe Feb 19, 2025

Choose a reason for hiding this comment

Judeeeee Feb 23, 2025

Choose a reason for hiding this comment

Judeeeee left a comment

Choose a reason for hiding this comment

Judeeeee Feb 23, 2025

Choose a reason for hiding this comment

sugiwe commented Jan 16, 2025 •

edited

Loading

Judeeeee Jan 31, 2025 •

edited

Loading