Skip to content
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.

Sign up for GitHub

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

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8541147
「近日開催のイベント」を、現在時刻から計算して終わっていないイベントだけを表示するようにした
sugiwe Dec 5, 2024
1572fde
特別イベント用のテストを作成
sugiwe Dec 10, 2024
9c86dc2
定期イベント用のテストを作成
sugiwe Dec 11, 2024
7a5cc91
RegularEventの方のnot_endedのロジックを修正
sugiwe Dec 19, 2024
ed505b9
当日開催で終了済みイベントを非表示にした変更に伴い既存のシステムテストで設定していた時間を2時間過去にした
sugiwe Jan 16, 2025
2b33e0d
イベントのテストデータ追加に伴いシステムテスト内のイベント数を修正
sugiwe Jan 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/models/event.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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で開催日の管理方法が異なるため共通化することは難しく、意味合いとしてはどちらも「今日開催でまだ終わっていないイベント」になるのですが、それぞれ別の方法で定義している、ということになります。

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • 特別イベントと定期イベントで「開催日」の扱いがかなり異なっている
  • 既存のscheduled_onスコープがEventとRegularEventでそれぞれ定義されている

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


def opening?
Time.current.between?(open_start_at, open_end_at)
Expand Down
5 changes: 5 additions & 0 deletions app/models/regular_event.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class RegularEvent < ApplicationRecord # rubocop:disable Metrics/ClassLength
scope :participated_by, ->(user) { where(id: all.filter { |e| e.participated_by?(user) }.map(&:id)) }
scope :organizer_event, ->(user) { where(id: user.organizers.map(&:regular_event_id)) }
scope :scheduled_on, ->(date) { holding.filter { |event| event.scheduled_on?(date) } }
scope :scheduled_on_without_ended, ->(date) { holding.filter { |event| event.scheduled_on?(date) && event.not_ended? } }

belongs_to :user
has_many :organizers, dependent: :destroy
Expand All @@ -77,6 +78,10 @@ def scheduled_on?(date)
all_scheduled_dates.include?(date)
end

def not_ended?
Time.current.strftime('%H:%M') < end_at.strftime('%H:%M')
end

def next_event_date
event_dates =
hold_national_holiday ? upcoming_scheduled_dates : upcoming_scheduled_dates.reject { |d| HolidayJp.holiday?(d) }
Expand Down
2 changes: 1 addition & 1 deletion app/models/upcoming_event.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ def date_key_to_date_class(date_key)

def original_events_scheduled_on(date)
[Event, RegularEvent].map do |model|
model.public_send(:scheduled_on, date)
model.public_send(:scheduled_on_without_ended, date)
end.flatten
end
end
Expand Down
22 changes: 22 additions & 0 deletions test/fixtures/events.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,3 +191,25 @@ event34:
open_start_at: 2024-02-26 10:00
open_end_at: 2024-03-26 09:00
user: kimura

event35:
title: "2024/12/1(日)9:00〜12:00開催の特別イベント"
description: "本日開催でまだ終了時刻を迎えていない特別イベントの表示テスト用"
location: "FJORDオフィス"
capacity: 40
start_at: <%= Time.zone.local(2024, 12, 1, 9, 0, 0) %>
end_at: <%= Time.zone.local(2024, 12, 1, 12, 0, 0) %>
open_start_at: <%= Time.zone.local(2024, 11, 1, 9, 0, 0) %>
open_end_at: <%= Time.zone.local(2024, 12, 1, 9, 0, 0) %>
user: kimura

event36:
title: "2024/12/1(日)6:00〜7:00開催の特別イベント"
description: "本日開催で終了時刻を過ぎた特別イベントの表示テスト用"
location: "FJORDオフィス"
capacity: 40
start_at: <%= Time.zone.local(2024, 12, 1, 6, 0, 0) %>
end_at: <%= Time.zone.local(2024, 12, 1, 7, 0, 0) %>
open_start_at: <%= Time.zone.local(2024, 11, 1, 6, 0, 0) %>
open_end_at: <%= Time.zone.local(2024, 12, 1, 6, 0, 0) %>
user: kimura
10 changes: 10 additions & 0 deletions test/fixtures/regular_event_repeat_rules.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,13 @@ regular_event_repeat_rule37:
frequency: 1
day_of_the_week: 6
regular_event: regular_event33

regular_event_repeat_rule38:
frequency: 0
day_of_the_week: 0
regular_event: regular_event36

regular_event_repeat_rule39:
frequency: 0
day_of_the_week: 0
regular_event: regular_event37
20 changes: 20 additions & 0 deletions test/fixtures/regular_events.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,3 +187,23 @@ regular_event35:
user: komagata
category: 0
published_at: "2023-08-01 00:00:00"

regular_event36:
title: 本日開催でまだ終了時刻を迎えていない定期イベント
description: 本日開催でまだ終了時刻を迎えていない定期イベントの表示テスト用
finished: false
hold_national_holiday: false
start_at: <%= Time.zone.local(2024, 12, 1, 9, 0, 0) %>
end_at: <%= Time.zone.local(2024, 12, 1, 12, 0, 0) %>
user: komagata
published_at: "2024-12-01 00:00:00"

regular_event37:
title: 本日開催で終了時刻を過ぎた定期イベント
description: 本日開催で終了時刻を過ぎた定期イベントの表示テスト用
finished: false
hold_national_holiday: false
start_at: <%= Time.zone.local(2024, 12, 1, 9, 0, 0) %>
end_at: <%= Time.zone.local(2024, 12, 1, 10, 0, 0) %>
user: komagata
published_at: "2024-12-01 00:00:00"
18 changes: 17 additions & 1 deletion test/models/event_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

class EventTest < ActiveSupport::TestCase
test '.scheduled_on(date)' do
travel_to Time.zone.local(2017, 4, 3, 10, 0, 0) do
travel_to Time.zone.local(2017, 4, 3, 8, 0, 0) do
today_date = Time.zone.today
today_events_count = 2
today_events = Event.scheduled_on(today_date)
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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されない」のどちらかを確認するテストだけで十分だと思います👌

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

travel_to Time.zone.local(2024, 12, 1, 10, 0, 0) do
today_date = Time.zone.today
events = Event.scheduled_on_without_ended(today_date)
assert_includes events, events(:event35)
end
end

test 'Events that have already ended today should not be displayed' do
travel_to Time.zone.local(2024, 12, 1, 10, 0, 0) do
today_date = Time.zone.today
events = Event.scheduled_on_without_ended(today_date)
assert_not_includes events, events(:event36)
end
end
end
16 changes: 16 additions & 0 deletions test/models/regular_event_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@Judeeeee Judeeeee Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

travel_to Time.zone.local(2024, 12, 1, 10, 0, 0) do
today_date = Time.zone.today
regular_event = RegularEvent.scheduled_on_without_ended(today_date)
assert_includes regular_event, regular_events(:regular_event36)
end
end

test 'Regular event that have already ended today should not be displayed' do
travel_to Time.zone.local(2024, 12, 1, 10, 0, 0) do
today_date = Time.zone.today
regular_event = RegularEvent.scheduled_on_without_ended(today_date)
assert_not_includes regular_event, regular_events(:regular_event37)
end
end
end
2 changes: 1 addition & 1 deletion test/models/upcoming_event_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ class UpcomingEventTest < ActiveSupport::TestCase

# not testing '.build_group' because this method is used in '.upcoming_events_grpups'.
test '.upcoming_events_groups' do
travel_to Time.zone.local(2017, 4, 3, 10, 0, 0) do
travel_to Time.zone.local(2017, 4, 3, 8, 0, 0) do
today_events = [
events(:event27),
events(:event33),
Expand Down
2 changes: 1 addition & 1 deletion test/system/events_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ class EventsTest < ApplicationSystemTestCase
today_events_count = 5
tomorrow_events_count = 2
day_after_tomorrow_events_count = 4
travel_to Time.zone.local(2017, 4, 3, 10, 0, 0) do
travel_to Time.zone.local(2017, 4, 3, 8, 0, 0) do
visit_with_auth events_path, 'komagata'
within('.upcoming_events_groups') do
assert_text '近日開催のイベント'
Expand Down
2 changes: 1 addition & 1 deletion test/system/home_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ class HomeTest < ApplicationSystemTestCase
end

test 'show all regular events and special events on dashbord' do
travel_to Time.zone.local(2017, 4, 3, 10, 0, 0) do
travel_to Time.zone.local(2017, 4, 3, 8, 0, 0) do
visit_with_auth '/', 'kimura'
today_event_label = find('.card-list__label', text: '今日開催')
tomorrow_event_label = find('.card-list__label', text: '明日開催')
Expand Down
4 changes: 2 additions & 2 deletions test/system/regular_events_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ class RegularEventsTest < ApplicationSystemTestCase

test 'show listing not finished regular events' do
visit_with_auth regular_events_path(target: 'not_finished'), 'kimura'
assert_selector '.card-list.a-card .card-list-item', count: 17
assert_selector '.card-list.a-card .card-list-item', count: 19
end

test 'show listing all regular events' do
Expand Down Expand Up @@ -369,7 +369,7 @@ class RegularEventsTest < ApplicationSystemTestCase
today_events_count = 5
tomorrow_events_count = 2
day_after_tomorrow_events_count = 4
travel_to Time.zone.local(2017, 4, 3, 10, 0, 0) do
travel_to Time.zone.local(2017, 4, 3, 8, 0, 0) do
visit_with_auth events_path, 'komagata'
within('.upcoming_events_groups') do
assert_text '近日開催のイベント'
Expand Down
Loading